Skip to content

fix(events): stop event run crashing on every piped stdin payload - #4326

Merged
KSchlobohm merged 4 commits into
github:mainfrom
Noor-ul-ain001:fix/event-run-stdin-eof-attribute
Aug 26, 2026
Merged

fix(events): stop event run crashing on every piped stdin payload#4326
KSchlobohm merged 4 commits into
github:mainfrom
Noor-ul-ain001:fix/event-run-stdin-eof-attribute

Conversation

@Noor-ul-ain001

Copy link
Copy Markdown
Contributor

Summary

  • event_run (src/specify_cli/commands/event.py) capped its stdin read at 1 MiB to prevent a DoS (fix: cap stdin read at 1 MiB to prevent DoS #3857), but the truncation check reads sys.stdin.eofthat attribute does not exist on any Python file-like object, including sys.stdin (hasattr(sys.stdin, "eof") is False).
  • Every piped-stdin invocation raised AttributeError: '...' object has no attribute 'eof' instead of running. Piped stdin is this command's documented primary use case ("Resolve and run an event-driven command script with stdin payload" — a native hook feeds it a JSON payload this way), and isatty() is False whenever stdin isn't an interactive terminal, so this fired on essentially every real invocation, not only oversized ones — the fix: cap stdin read at 1 MiB to prevent DoS #3857 DoS fix left the feature entirely broken.
  • The intended oversized-payload branch was also broken a second, independent way: raise typer.Exit(code=1, message="...")typer.Exit.__init__ accepts only code (verified via inspect.signature), not message — so that path raised TypeError instead of the documented clean error.
  • Fix: detect truncation the standard way — after reading the 1 MiB cap, read one more byte; a non-empty result means more data was waiting beyond it. Report the oversized-payload error via typer.echo(..., err=True) before raise typer.Exit(code=1).

Test plan

  • Added tests/test_event_command.py (no prior test coverage existed for this command): a normal piped payload reaches the handler intact, a TTY/no-stdin invocation falls back to "{}", and an oversized piped payload exits 1 with the limit message instead of crashing.
  • Verified all 3 tests fail without the fix — reproduced the exact AttributeError('...' object has no attribute 'eof') on every case, including the "no stdin" one (confirms isatty() is False under non-interactive invocation, matching real hook usage) — and pass with it.
  • Ran the new test module standalone — 3 passed.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt

@Noor-ul-ain001
Noor-ul-ain001 requested a review from mnriem as a code owner August 25, 2026 15:29
`event_run` (src/specify_cli/commands/event.py) capped its stdin read at
1 MiB to prevent a DoS (github#3857), but the truncation check reads a `.eof`
attribute that does not exist on any Python file-like object, including
`sys.stdin` (`hasattr(sys.stdin, "eof")` is False). Every piped-stdin
invocation raised `AttributeError: '...' object has no attribute 'eof'`
instead of running — piped stdin is the command's documented primary use
case (a native hook feeds it a JSON payload this way), and `isatty()` is
False whenever stdin isn't an interactive terminal, so this fired on
essentially every real invocation, not just oversized ones.

Even the intended oversized-payload branch was broken a second way:
`typer.Exit(code=1, message=...)` — `typer.Exit.__init__` only accepts
`code`, not `message` — so that path raised `TypeError` instead of the
documented clean error.

Fix: detect truncation the standard way (read one more byte once the cap
is hit; a non-empty result means more data was waiting beyond it), and
report the oversized-payload error via `typer.echo(..., err=True)` before
`raise typer.Exit(code=1)`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes event run stdin handling so piped payloads no longer crash.

Changes:

  • Replaces invalid EOF and typer.Exit usage.
  • Adds tests for stdin handling and oversized payloads.
Show a summary per file
File Description
src/specify_cli/commands/event.py Detects excess stdin and reports a clean error.
tests/test_event_command.py Adds CLI stdin regression tests.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/specify_cli/commands/event.py Outdated
Comment thread tests/test_event_command.py
@KSchlobohm

Copy link
Copy Markdown

Please address Copilot feedback

Address Copilot review feedback on PR github#4326:
- sys.stdin is a text stream, so reading MAX_STDIN_BYTES counted Unicode
  characters, not encoded bytes. A multibyte payload (e.g. ~300k emoji,
  ~1.14 MiB in UTF-8) could slip past the 1 MiB DoS guard. Read from
  sys.stdin.buffer instead so the cap counts real bytes, then decode.
- The TTY-fallback test invoked via CliRunner, which always supplies a
  non-TTY stream even without input=, so it never exercised the `"{}"`
  fallback. Split it into an empty-pipe test (CliRunner) and a real TTY
  test that calls event_run directly with a mocked isatty()=True stdin.
- Added a regression test proving the byte-vs-character cap distinction.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PJHJ2dHP2RVCNncHqN8Qm9

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/commands/event.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@KSchlobohm

Copy link
Copy Markdown

Thanks for the quick update to add the try/catch. Our standard is that any new behavior branch carries both a positive and a negative test — the happy path and the failure path. The valid-UTF-8 path is already covered, but the new UnicodeDecodeError guard (the negative case) isn't, so nothing proves it exits cleanly or guards against regression.

Could you add a test that pipes invalid UTF-8 (e.g. b"\xff\xfe") and asserts exit code 1, the "must be valid UTF-8" message, and that the handler isn't called?

Reviewer noted the new UnicodeDecodeError guard in event_run had no
test proving it exits cleanly instead of leaking a raw
UnicodeDecodeError. Add a case piping invalid UTF-8 (b"\xff\xfe") and
assert exit code 1, the "must be valid UTF-8" message, and that the
handler is never invoked.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M9aV6DhKNL7k3HreTczcb1

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@KSchlobohm
KSchlobohm merged commit 6fe81f3 into github:main Aug 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants