Make the Stop hook async and UserPromptSubmit single-process (v3.0.3)#7
Conversation
Stop runs async so turn-end no longer blocks on the handoff write. UserPromptSubmit merges its two Python spawns into one, dropping an interpreter cold-start per message (the dominant per-message cost on Windows). Path resolution stays cygpath-first to avoid a POSIX/drive-relative collision; the async Stop guards the handoff replace by transcript mtime so a slow run cannot overwrite a newer one. Adds T-38g for the guard; suite 131/131. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1be683e2a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| "hooks": [ | ||
| { "type": "command", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/update-handoff.sh\"" } | ||
| { "type": "command", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/update-handoff.sh\"", "async": true } |
There was a problem hiding this comment.
The official Claude Code hook docs state that async hooks start in the background and Claude continues without waiting, so with this flag a user can submit the next prompt before update-handoff.sh has replaced handoff-<sid>.json. check-context-size.sh only checks that the handoff file exists before telling the model it “is current”, so in that window /prep-compact can read the previous turn's files/todos/requests while the reminder claims it is fresh. Either keep this hook synchronous or add a freshness/in-progress check before advertising the handoff as current.
Useful? React with 👍 / 👎.
| # After T7: EXPECTED_PASS=88, SKIPPED=39 (T7 test not Stop-dep) | ||
| # PR-comment fix: +3 Stop-dep (T-32cap +1 short-still-captured, T-32prior +2) | ||
| EXPECTED_PASS=130 | ||
| EXPECTED_PASS=131 |
There was a problem hiding this comment.
When test/fixtures/stop-real.json is absent—the local-dev path this harness explicitly supports—the new T-38g assertion is skipped with the rest of the Stop-hook block, but SKIPPED remains 59 while EXPECTED_PASS was bumped to 131. That path now fails the false-green guard with PASS=71 + SKIPPED=59, so local developers without the captured fixture can no longer run the non-Stop tests; bump the skipped count with the expected count.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b27e9d95ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(_emt, (int, float)) and _emt > transcript_mtime: | ||
| os.unlink(tmp_path) | ||
| sys.exit(0) |
There was a problem hiding this comment.
When two async Stop hooks overlap and the later run has already written a handoff for a newer transcript, this branch deletes the older run's temp file and exits. If the rapid subsequent turn pushed a user request or file reference out of the 1 MB transcript tail, the newer run could not capture that data from the prior handoff yet, and this skip drops the only run that did capture it; /prep-compact can then lose recent intent or touched files. Consider merging the stale run's additive fields into the existing newer handoff instead of dropping the temp wholesale.
Useful? React with 👍 / 👎.
| { | ||
| "hooks": [ | ||
| { "type": "command", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/update-handoff.sh\"" } | ||
| { "type": "command", "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/update-handoff.sh\"", "async": true } |
There was a problem hiding this comment.
With the Stop hook now backgrounded, a user who submits the next prompt immediately after crossing the token threshold can hit UserPromptSubmit before this async write has created or refreshed handoff-<sid>.json. In that case check-context-size.sh emits the no-handoff reminder and creates compact-warned-<sid>, so the later successful handoff write will not trigger a corrected handoff-aware reminder during the same crossing. Previously the synchronous Stop hook made the handoff available before the next prompt hook ran.
Useful? React with 👍 / 👎.
| if os.path.exists(handoff_path): | ||
| with open(handoff_path, 'r', encoding='utf-8') as f: | ||
| _existing = json.load(f) | ||
| _emt = _existing.get('transcript_mtime_at_write') |
There was a problem hiding this comment.
In an async overlap, this read-before-replace guard can still let an older Stop run clobber a newer handoff: run A can pass this check while the file is absent or older, then run B writes a fresher handoff, and finally run A resumes at os.replace and overwrites B. That interleaving leaves the stale handoff on disk even though the mtime guard was intended to prevent stale writers, so the check needs to be coupled with the replace via locking or another atomic ordering mechanism.
Useful? React with 👍 / 👎.
1 participant
What
Cuts the per-message latency the two hooks add, on Windows especially, where process spawning dominates.
Why
On a Windows laptop the hooks added noticeable lag to every message: each invocation is a bash plus Python cold-start, and the UserPromptSubmit hook spawned Python twice per turn. The context-size check alone measured around 1.2s per message in session telemetry.
Two fixes folded in (caught in review)
cygpathfirst on Git Bash, matching the pre-merge behaviour. A direct existence check could otherwise resolve a POSIX path like/tmp/xto a drive-relativeC:\tmp\xand scan the wrong file.os.replaceprevents partial reads but not writer ordering, so the hook compares the on-disk handoff'stranscript_mtime_at_writebefore replacing and skips the write when the existing file is newer.Tests
Full suite passes: 131/131. Adds
T-38g, a deterministic test for the mtime guard. The cygpath-first change is covered by the existing/tmp-path tests.Notes
Patch release (3.0.3). This does not include the larger flag-file redesign planned for 3.1.0, which moves the token scan into the Stop hook so the UserPromptSubmit hook becomes a pure-bash flag reader.