Repository navigation
Conversation
…ndlers A handler that runs past its timeout exited 2, which agents read from a Stop hook as "don't stop, keep going" — one slow script could keep the agent running when it should stop. Runner failures (timeout or error) now exit 1; a handler's own exit code still passes through unchanged. Handlers also had no way to tell which event fired. Both runners now export the canonical event name as SPECKIT_EVENT. Fixes github#5336
There was a problem hiding this comment.
🟡 Changes recommended
Pre-tool validation now fails open on timeouts or startup errors in both runner paths, allowing tool calls that the guards should block.
3 open issues
What changed in this PR
Addresses #5336 by updating Spec Kit’s runtime-event runners so slow handlers do not unintentionally keep agents running and handlers can identify the event that fired.
Changes:
- Return exit 1 on timeout or runner error in both implementations, while preserving handler exit codes.
- Pass the canonical event name through
SPECKIT_EVENT. - Add tests for timeout behavior, event propagation, and deliberate handler exit 2.
| File | Description |
|---|---|
tests/specify_cli/events/test_events.py |
Adds resolver and generated-dispatcher regression tests. |
src/specify_cli/events/__init__.py |
Updates failure exit codes and event environment propagation in both runners. |
Review effort: Balanced (auto)
| except Exception as e: | ||
| sys.stderr.write(f"Event command {command_name} error: {e}\n") | ||
| return 2 | ||
| return 1 |
…er contract for delegation Runner failures (timeout, handler failing to start) now exit 2 for pre_tool_use so the guard stays blocking, and 1 for every other event. The generated dispatcher requires the new EVENT_RUNNER_CONTRACT marker before delegating to an installed specify_cli, so a stale install with path confinement alone cannot bypass the github#5336 runner contract. Regression tests cover per-event timeout and startup-failure codes in both the resolver and the inline fallback, plus a stale package with confinement but no contract marker.
| return 0 | ||
| # Tell the handler which event fired so one script can serve several | ||
| # events (mirrors the inline fallback in the generated dispatcher). | ||
| env = {**os.environ, "SPECKIT_EVENT": event_name} if event_name else None |
|
The Stop-handler timeout fix makes sense, but I don’t think this is ready to merge yet. Runner failures should not accidentally tell the agent to keep working, and preserving a handler’s deliberate exit code is important. However, the current change also makes AI assistance: GitHub Copilot using GPT-6.1 Sol assessed the code and drafted this comment on behalf of @mnriem, under human supervision; @mnriem reviewed the draft and authorized posting. Reasoning-effort setting unavailable. |
…n unresolved guards Add user_prompt_submit to BLOCKING_FAILURE_EVENTS in both runners: Claude Code reads exit 2 from a UserPromptSubmit hook as "block the prompt, so it never reaches Claude" (Claude Code Hooks reference, "Exit code 2 behavior per event"), so a prompt whose validation never ran must not reach the model. Fix the inline manifest scan: strip trailing '#' comments outside quotes from name:/file: values, and reset the name/file pair on each new list item, so a name can never pair with the next entry's file. Fail closed in the inline fallback: a guard command that cannot be resolved, or resolves to a template with no runnable script, exits 2 with a stderr diagnostic; non-guard events still no-op with 0. Rename the handler env var SPECKIT_EVENT to SPECKIT_EVENT_NAME, establishing SPECKIT_EVENT_* as the namespace for future values. Document the handler contract in docs/integrations/cli.md (stdin JSON payload, UTF-8, 1 MiB cap; SPECKIT_EVENT_NAME; runner-failure exit codes; handler exit codes pass through). Regression tests cover per-event timeout and startup-failure codes for user_prompt_submit in both runners, manifest trailing-comment and cross-pairing resolution, and the fail-closed backstop, each shown red before the fix.
There was a problem hiding this comment.
🟡 Changes recommended
Guard events can still bypass validation because delegated resolution permits missing or scriptless handlers, while the inline scanner can misread description text and select the wrong handler.
5 open issues
2 resolved since last review
Review effort: Balanced (auto)
Copilot round 3 (review 5836931756) on the c1cbba7 head: The delegated resolver returned 0 when a guard command could not be resolved, or resolved to a template with no runnable script, letting the action through without validation. Mirror the inline fallback fail-closed checks: pre_tool_use / user_prompt_submit now exit 2 with a diagnostic; non-guard events still no-op with 0. Fix the inline manifest scan: match real name:/file: keys instead of substrings, so a description mentioning "file:" can no longer select the wrong handler and bypass the guard. A block-style aliases: list between name: and file: reset the entry, so the guard never resolved. Only a dash at the entry's own indent starts a new entry; deeper-indented dashes belong to nested lists. Startup-failure coverage now spans both runners and all three event classes (the inline pre_tool_use case was the missing one). The two carried-over lows needed no change: the handler contract is already documented in docs/integrations/cli.md, and stop startup failures were already covered in both runners. Regression tests for each fix, each shown red before the fix: delegated fail-closed for missing and scriptless guards, description file: text ignored, block aliases keep the command name, inline pre_tool_use startup failure.
There was a problem hiding this comment.
🟡 Changes recommended
The inline hook lookup compares canonical names against raw legacy names, leaving handlers with different filename stems unresolved and causing the fail-closed path to block matching tool calls or prompts.
3 open issues
3 resolved since last review
Review effort: Max (auto)
| file_match.group(1).strip() | ||
| ).strip('"').strip("'") | ||
| if cur_name and cur_file: | ||
| if cur_name == command_name: |
|
Please address Copilot feedback |
Description
A slow runtime-event handler could keep the agent running when it should stop, and handlers could not tell which event fired.
What changed: both event runners (the full resolver and the inline fallback in the generated dispatcher) now exit 1 on timeout or runner error, and export the canonical event name as SPECKIT_EVENT_NAME. A handler's own exit code still passes through unchanged, so a handler that deliberately exits 2 still blocks.
Follow-up (mnriem's review + Copilot round 2):
pre_tool_useanduser_prompt_submit, and 1 for every other event. Claude Code reads exit 2 from a UserPromptSubmit hook as "block the prompt, so it never reaches Claude" (Claude Code Hooks reference, "Exit code 2 behavior per event").#comments aftername:/file:values are ignored, and the name/file pair resets on each new list item, so a name can never pair with the next entry's file.SPECKIT_EVENT_NAME(it wasSPECKIT_EVENTin the first version of this PR, which is unmerged, so the rename is clean).SPECKIT_EVENT_*is the namespace for future event-related values.docs/integrations/cli.md(stdin JSON payload, UTF-8, 1 MiB cap;SPECKIT_EVENT_NAME; runner-failure exit codes; handler exit codes pass through).Fail-closed trade-off: a guard handler that cannot be resolved (missing command file, unparsable manifest, disabled extension, or no runnable script) now blocks the tool call or prompt (exit 2) instead of silently letting the action through without validation. That means a misconfigured guard can wedge the agent loop until it is fixed. The alternative is worse: a guard that silently never runs defeats the purpose of configuring one. Non-guard events are unaffected and still no-op with 0.
Note: the Windows failure in
tests/specify_cli/integrations/test_registration.py::test_script_help_includes_python_variant[args0]is pre-existing on main: the same test fails there with the same assertion (missing--scripttext inspecify init --help), e.g. on main run 42678554323, head 302482a (Windows 3.14 job: 1 failed, 14239 passed). This PR touches onlysrc/specify_cli/events/__init__.py,tests/specify_cli/events/test_events.py, anddocs/integrations/cli.md, so it is not fixed here.Fixes #5336
Testing
.venv/bin/specify --help.venv/bin/python -m pytest tests/specify_cli/events/test_events.py -q(226 passed)specify event runcheck on a stop handler. Sleeping 1s with a 2s timeout exits 0 and sees SPECKIT_EVENT_NAME=stop; sleeping 5s exits 1 with "timed out" on stderr.user_prompt_submitin both runners, manifest trailing-comment and cross-pairing resolution, fail-closed backstop) shown red before the fix, green after.ruff checkon the touched files: no new findings (47 pre-existing findings, identical set before and after this change).