fix: restore dev CI for hooks escaping and pre-tool preflight#2189
Open
Yeachan-Heo wants to merge 2 commits intodevfrom
Open
fix: restore dev CI for hooks escaping and pre-tool preflight#2189Yeachan-Heo wants to merge 2 commits intodevfrom
Yeachan-Heo wants to merge 2 commits intodevfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Yeachan-Heo
force-pushed
the
omx/base-red-hooks-and-pretool
branch
3 times, most recently
from
9a14756 to
0b48528
Compare
Yeachan-Heo
force-pushed
the
omx/base-red-hooks-and-pretool
branch
from
0b48528 to
e6f4353
Compare
The hook escaping regression came from asserting a literal `node` prefix even though installed hooks may be patched to an absolute node binary. The test now captures argv after shell expansion instead of accidentally invoking the real runner, and the pre-tool enforcer now sanitizes malformed threshold env input so agent-heavy exhausted-context preflight keeps blocking deterministically. The remaining CI flakes came from ambient test-process state leaking into the pre-tool harness (env kill-switches, cwd/config, and child_process mocking) plus a tmux-availability assumption in the duplicate-worker dispatch test, so those assertions are now hermetic and environment-agnostic. Constraint: Installed hooks may start with an absolute quoted node path after plugin setup Constraint: Pre-tool tests must not inherit hook kill-switches, Claude config, or mocked child_process state from the surrounding test runner Constraint: Duplicate-worker dispatch coverage must not require tmux to be available Rejected: Keep assuming a literal `node` prefix in hook tests | no longer matches shipped hook shape Rejected: Leave malformed threshold env unchecked | comparison against NaN silently disables blocking Rejected: Assert immediate `notified` dispatch state in the duplicate-pane test | depends on tmux availability outside the test contract Confidence: high Scope-risk: narrow Directive: Keep hook argv assertions focused on post-shell tokenization, not a specific node binary spelling Directive: Keep shell-spawned hook tests pinned to isolated HOME/CLAUDE_CONFIG_DIR values, cleared skip flags, isolated child cwd, and direct execFile child_process access so ambient state cannot leak in Tested: npm test -- --run src/__tests__/pre-tool-enforcer.test.ts Tested: npm test -- --run src/__tests__/pre-tool-enforcer.test.ts src/team/__tests__/api-interop.dispatch.test.ts src/__tests__/hooks-command-escaping.test.ts Tested: npm test -- --run Tested: npm run build Tested: npm run lint Not-tested: PR CI rerun on GitHub after push
Yeachan-Heo
force-pushed
the
omx/base-red-hooks-and-pretool
branch
from
e6f4353 to
5be8f6a
Compare
The CI failure was isolated to the fallback preflight assertions in the full vitest run: the focused hooks-command-escaping coverage already passed, but the agent-heavy preflight tests were still red in GitHub Actions. Extracting the context-threshold calculation into a small shared helper keeps the runtime logic unchanged while letting the regression tests assert the preflight decision hermetically instead of depending on whole-hook process state. Constraint: The fix needed to stay narrowly scoped to the remaining pre-tool-enforcer fallout after hooks-command-escaping was already green Constraint: Preflight threshold parsing and transcript tail parsing must remain usable by the shipped hook script, not a test-only duplicate Rejected: Chase the already-green hooks-command-escaping path again | it was not the failing surface in the current CI run Rejected: Keep the failing assertions as child-process hook tests | GitHub Actions still showed suite-level flakiness on those cases Confidence: medium Scope-risk: narrow Reversibility: clean Directive: Keep fallback preflight assertions pointed at the shared helper unless a future change specifically needs end-to-end hook-process coverage Tested: npm test -- --run src/__tests__/hooks-command-escaping.test.ts src/__tests__/pre-tool-enforcer.test.ts Tested: npm test -- --run Tested: npm run build Tested: npm run lint Not-tested: GitHub Actions rerun after push
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/__tests__/hooks-command-escaping.test.tsunchanged after confirming it already passes on the current branchscripts/pre-tool-enforcer.mjsExact reproduced failure
GitHub Actions
CI / Teston commit5be8f6a5was still red in the full suite with these failures insrc/__tests__/pre-tool-enforcer.test.ts:blocks agent-heavy Task preflight when transcript context budget is exhaustedfalls back to the default preflight threshold when the env value is invalidignores parent-process hook skip env when exercising preflight blockingThe focused hooks command escaping test was already green, so this update only fixes the remaining pre-tool fallout.
Verification
npm test -- --run src/__tests__/hooks-command-escaping.test.ts src/__tests__/pre-tool-enforcer.test.tsnpm test -- --runnpm run buildnpm run lint(passes with one pre-existing warning insrc/hooks/setup/__tests__/stdin-symlink.test.tsfor unusedcopyFileSync)