fix(events): stop event run crashing on every piped stdin payload - #4326
Conversation
`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
There was a problem hiding this comment.
Pull request overview
Fixes event run stdin handling so piped payloads no longer crash.
Changes:
- Replaces invalid EOF and
typer.Exitusage. - 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
|
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
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
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 Could you add a test that pipes invalid UTF-8 (e.g. |
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
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 readssys.stdin.eof— that attribute does not exist on any Python file-like object, includingsys.stdin(hasattr(sys.stdin, "eof")isFalse).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), andisatty()isFalsewhenever 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.raise typer.Exit(code=1, message="...")—typer.Exit.__init__accepts onlycode(verified viainspect.signature), notmessage— so that path raisedTypeErrorinstead of the documented clean error.typer.echo(..., err=True)beforeraise typer.Exit(code=1).Test plan
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.AttributeError('...' object has no attribute 'eof')on every case, including the "no stdin" one (confirmsisatty()isFalseunder non-interactive invocation, matching real hook usage) — and pass with it.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01FW9fAYsCBCAgdKWovtSyqt