fix(hooks): two hook timeouts, the lexer's cubic cost and routing outside a git work tree - #328
KilimcininKorOglu wants to merge 3 commits into
Conversation
…r 4,000 characters rw_is_ripwire_call rebuilt the rest of the line for every character it read, so one long Bash command held the PreToolUse hook for minutes: 2.9 s at 2,000 characters, 20.6 s at 4,000, 155 s at 8,000 under macOS bash 3.2 (bash 5.3: 1.7 s, 12.4 s, 93.6 s). The meter runs it on every Bash call even with the nudge text retired. Two guards now come first, in all three copies of the mirrored block. A line without the word holds no call; that is exact and ends the scan for nearly every command. A line over 1,024 characters is not scanned and reads as no call, a missed call, the same direction as the 2>&1 limit the block already discloses. routehookcheck O10 holds both: two 4,000-character lines, 20 s each and one wrong answer on the old block, 0 s and correct now. Fixes redhat-et#327
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: redhat-et/ripwire/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: redhat-et/ripwire/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThree mirrored hooks now skip the character scan when a command lacks ChangesRipwire call scan guards
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The scan guards address slow long commands. Calls in commands over the limit may be missed by design; no additional merge-blocking issue is established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for chasing this down — the root-cause writeup in #327 was already excellent, and turning it into
One thing I want to flag before this merges, since it touches the comment's own claim of exactness: The substring guard ( It's a miss, not a false positive, so it stays on the side the PR promises — but it's a second, new, Small second note: this PR doesn't touch Neither of these blocks the performance fix's own correctness — O10's red/green story is solid and the |
Outside a git work tree `--help-task` has no file list from git and walks the whole tree under cwd. A session started in $HOME measured over 30 s for one prompt, past the 8 s UserPromptSubmit timeout, so Claude Code discarded the hook and printed a timeout warning on every prompt. Both prompt routers now exit before the classifier when `git rev-parse --is-inside-work-tree` does not answer. routehookcheck.sh O11 puts a stub ripwire on PATH that records each call: a prompt in a non-git cwd must not reach it, and a prompt in the fixture repo must (the positive control). RED on the pre-fix hook. codexpromptroutecheck.sh built its fixture repo as an empty .git directory, which git does not accept; it now runs `git init`. Refs redhat-et#327
KilimcininKorOglu
changed the title
… both fixes
The substring guard reads the raw line before quote removal, so a command
word the shell assembles from fragments ('rip''wire' ., rip\wire .,
"rip""wire" ., rip"wire" .) reads as no call; the lexer alone read each
as one. The block's comment called the check exact. It now names the
shape and its direction (a missed call, never a false one) in all three
mirrored copies, and O9 pins the four shapes plus two whole quoted words
that still read as calls.
CHANGELOG gains an Unreleased section with both redhat-et#327 fixes, and
docs/SUBSTITUTION_METER.md names the guard's two skipped shapes under
"Known undercount" and the prompt router's git-only population.
Refs redhat-et#327
|
Thanks for the careful review and the rebuild. You were right on both points, and both are addressed in b69ce56. The substring guard is not exact. I reproduced it. With the guard removed from the extracted block, the lexer reads
CHANGELOG and the meter doc.
A second change you have not reviewed yet. Your review covered 3b96d37. After it I pushed bb04971, which fixes a second hook timeout reported in #327 (comment). Outside a git work tree, Gates, run with the released 0.6.2 binary (I did not build the tree): all 23 gates that name a changed file, the CHANGELOG or the meter doc. 22 are green, |
joyful-ii-V-I
left a comment
joyful-ii-V-I
left a comment
There was a problem hiding this comment.
Thank you, @KilimcininKorOglu. The fix round is exactly what we asked for, and the second commit fixes a real problem well. I built b69ce563 from source. Your gate numbers came from the 0.6.2 binary, so this is the tree build you said was still needed. Here is what I checked.
The substring-guard disclosure (our ask from the last round): done.
- The COST comment no longer says "exact". It names the assembled-word shape and says which way it fails, and the O9 byte-identity arm shows the comment is the same in all three hooks (158 lines).
- O9 pins
'rip''wire' .,rip\wire .,"rip""wire" .andrip"wire" .as0, and pins the two whole-quoted controls as1. I also ran the extracted block underdash, theshon Linux CI, and under bash. They give the same answer for all of these shapes and for the 1,025+ character cap. - The CHANGELOG
[Unreleased]entries and theSUBSTITUTION_METER.md"Known undercount" note are accurate.deckcheckandmeterdisclosurecheckpass on them.
bb04971e, routing only inside a git work tree: reviewed, and we're keeping it.
- It is the right fix for #327's second timeout. It is one cheap
git -C "$cwd" rev-parsebefore the classifier, and it errs toward a missed recommendation, as the hook already does whenever it is unsure.hooks/ripwire-claude-toolroute.shalready skips non-repos with agit rev-parsecheck, so the routers now match it. - O11 fails when I delete your guard line and passes with it (the non-git prompt reached the stub). The positive control keeps the test from passing without calling anything. The
git initchange incodexpromptroutecheck.shwas needed: the old empty.gitdirectory was not a real repo. - I ran both routers on a matrix of 13
cwdshapes: a plain repo, a subdirectory, a linkedgit worktree(its path contains a space), a repo under a directory with spaces, a submodule, a non-repo, a directory with an empty.git, and theGIT_DIR/GIT_WORK_TREEcases. Every case routes or skips the way the comment says."$cwd"is quoted everywhere, so spaces are fine.
Two small notes. Neither blocks the merge. Take them or leave them:
- In a bare repository, and inside a
.gitdirectory,git rev-parse --is-inside-work-treeprintsfalseand exits 0, so the guard lets those through and the classifier runs. Those trees are small, so this is not a timeout. But "does not answer" is doing some work in the comment.[ "$(git -C "$cwd" rev-parse --is-inside-work-tree 2>/dev/null)" = true ] || exit 0would make it exact. (--show-toplevelwould not help here, because it also exits 0 in a bare repo.) - If the client itself runs with
GIT_DIRexported, anycwdcounts as "inside", because git usesGIT_DIRinstead of searching fromcwd. And with nogitonPATH, the router never routes at all. Both fail toward a missed call. A half-sentence in the comment would cover them.
Gates. I ran every gate that names a file this PR changes: 23 gates, including routehookcheck (70 PASS), hookcheck, taskroutecheck, toolcallroutecheck, releaseinstallcheck, skillinstallcheck, codexdoctorcheck, selfcheckcheck and deckcheck. All 23 pass on a tree build of b69ce563. The PR touches no C++ and does not bump kParserVer.
Merging. A trial merge onto our current integration branch conflicts only in CHANGELOG.md, where both sides open ## [Unreleased]. We'll resolve that ourselves by keeping both sets of entries under one heading. After that merge, routehookcheck, codexpromptroutecheck, deckcheck and meterdisclosurecheck still pass. There is nothing to rebase on your side. We'll bring your branch in as it is with the next integration train, so the commits stay yours. A maintainer will approve the held Actions run so the Linux and Windows jobs report before it lands.
Thanks again. The #327 write-up and this PR are a model of how to report and fix a performance cliff.
|
This landed on |
…rd lexer cost, and routing only inside a git work tree Signed head b69ce56 (KilimcininKorOglu), merged as-is; the author's commits are kept. Conflict: CHANGELOG.md, both sides opening ## [Unreleased]. Union: the PR's two (redhat-et#327) sections go at the end of the [Unreleased] block, above ## [0.6.2]; one [Unreleased] heading remains. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Fixes #327.
This PR fixes two hook timeouts reported in #327. Each has its own commit.
rw_is_ripwire_callrebuilt the rest of the line for every character it read, so its cost grew with the cube of the command's length. The PreToolUse meter runs it on every Bash call, and one command carrying a heredoc held the call for 6 min 49 s. (3b96d37)ripwire "$cwd" --help-task="$prompt"on every prompt. Outside a git work tree--help-taskhas no file list from git and walks the whole tree undercwd. A session started in$HOMEtook over 30 s for one prompt, past the 8 s hook timeout, so Claude Code discarded the hook and printedUserPromptSubmit hook [...] timed out after 8son every prompt. (bb04971, reported in this comment)Change 1: the lexer's cost
Two guards at the top of the mirrored block, identical in all three copies (
hooks/ripwire-nudge.sh,hooks/ripwire-claude-route.sh,hooks/ripwire-codex-route.sh):2>&1limit the block already discloses. The block's comment now names both, and why 1,024: the old scan of a 1,000-character line with the call at the end took 0.43 s on bash 3.2.The lexer itself is unchanged. A linear lexer (one
awkpass, say) would remove the cap too; this PR keeps the diff to the guards and leaves that choice to you.Change 2: routing outside a git work tree
Both prompt routers (
hooks/ripwire-claude-route.sh,hooks/ripwire-codex-route.sh) now exit before the classifier when the prompt'scwdis not inside a git work tree:Measured with the same prompt on macOS, warm cache:
cwdripwire "$cwd" --help-task=...$HOME(not a git work tree)The cost: a small non-git project gets no recommendation either. That is a missed recommendation, the direction the router already takes on every doubt. A walk budget or file cap inside
--help-taskfor a root without git would keep those projects routed; that is a change to the binary, so I left it to you.Gates
test/routehookcheck.sh, both arms written first and observed RED on the old code:O10, change 1:
ripwireecho <4,000 characters>; ripwire .O9's 28 shapes and its byte-identity arm stay green.
O11, change 2: a stub
ripwireonPATHrecords each call. A prompt in a non-gitcwdmust not reach it; on the old hook it did. A prompt in the fixture repo must reach it, which proves the stub runs at all.test/codexpromptroutecheck.shbuilt its fixture repo as an empty.gitdirectory, whichgit rev-parsedoes not accept. It now runsgit init, and nothing else in the gate changed.Verification
Run on macOS 27.0 (bash 3.2.57 as
sh) with the releasedripwire 0.6.2asBIN; I did not build the tree. Every gate that names a changed file, per CONTRIBUTING §1 clause 2:routehookcheck(70 PASS, including O10 and O11),hookcheck(199),codexdoctorcheck,codexpromptroutecheck,meterdisclosurecheck,routingreportcheck,skillinstallcheck,skillscanreadcheck,taskroutecheck,toolcallroutecheck,wrapverbscheckregression.shnamescodexpromptroutecheck.shbut stops at its first line withoutbuild/ripwire, so I ran that gate directly (green above).releaseinstallcheckfails one arm, "installer gates escaped their fixture homes or failed with inherited overrides". It fails the same arm with the same message on unmodifiedmainat b939ef4, and this diff does not touchinstall.sh, so the failure is not caused by this change. I have not investigated its cause. Two conditions of my run differ from CI and may explain it: the gates ran from a shell inside a Claude Code session, which exports its own environment variables to child processes, andBINwas the released 0.6.2 binary instead of a build of this tree.Summary by CodeRabbit