fix(hooks): avoid quadratic git branch delete matching - #320
Conversation
Signed-off-by: David Turner <davidelsey9513@gmail.com>
Rosetta Triage ReviewSummary: Fixes O(n^2) rescanning in the Findings:
Caveats:
Automated triage by Rosetta agent |
isolomatov-gd
left a comment
isolomatov-gd
left a comment
There was a problem hiding this comment.
Thank you, @atomicdjt. Two things to add before this goes in.
1. The cross-line branch has no test. GIT_BRANCH_CROSS_LINE (patterns.ts:111) is the alternative that preserves the old \s+ behavior, and nothing pins it. Deleting it from GIT_BRANCH leaves all 344 tests green across git-branch-delete.test.ts, dangerous-actions.test.ts and adapter.cursor.test.ts, while these stop being caught:
git\nbranch -D x
git\r\nbranch -D x
git\rbranch -D x
git \n branch -d -f x
The only whitespace variant covered today is the tab at line 41, which exercises GIT_BRANCH_LOCAL. Please add the four above.
2. Add the same-segment case git branch -a git branch -D x. It is the exact shape the anchor-skip optimization risks. It matches correctly today; it should be pinned.
Worth a code comment too: the optimization is sound only because both lookaheads run unbounded to the segment end, which makes every later candidate's window a suffix of the first's. Bounding a lookahead window later would silently break it. Same for \r\n staying in the separator class -- the pattern has no m flag, so narrowing that class to [;&|] would stop ^ from reaching post-newline segments.
On the fix itself: verified equivalent to the previous matcher across ~5.5M inputs (306,911 seeded fuzz, 5,229,042 exhaustive length-1..6 sequences, 56 crafted adversarial), zero divergences in both directions -- no new false negatives, no new false positives. The harness was mutation-checked first, so the zero is meaningful. O(n^2) to O(n) reproduced: 8.8 / 35.4 / 138.9 / 552.2 ms before against 0.08 / 0.16 / 0.33 / 0.66 ms after, at 1k/2k/4k/8k repetitions. The 250 ms assertion is not flaky (15/15 runs, 0.77-1.87 ms) and a revert fails it by roughly 4x.
The approach is also better than both directions suggested in #313. Bounding the window would break the test added here at line 84, which needs -f matched 75,000 characters after branch. Splitting on separators in JS would mean changing the DangerPattern contract or special-casing one pattern inside evaluate.ts.
One finding in the same file, outside this PR. git-force-push has the same unbounded-rescan shape with a nested quantifier, and it is larger: on 'git push a '.repeat(k) we measure 16.6 / 68.2 / 264.6 / 1082.6 ms at k = 1000/2000/4000/8000. It also runs earlier -- index 9 versus index 12 -- and matchPatterns is first-match-wins, so a Bash call still pays over a second on adversarial input after this merges. rm-rf-recursive, rm-rf-root, aws-s3-rm-recursive and curl-pipe-shell scale the same way. Taking it in this PR would close the class; if you would rather not, say so and we will track it separately.
Signed-off-by: David Turner <davidelsey9513@gmail.com>
|
@isolomatov-gd Thank you for the unusually thorough review and for independently validating both equivalence and scaling. Addressed in
Validation is green: focused suites 349/349, full hooks 1,257/1,257 with 45 bundles built, repository pre-commit passed in an LF WSL worktree, mypy passed across 55 files, the Python suites passed 339 MCP plus 56 CLI tests, and 20/20 production-path performance runs were 1.631–1.869 ms against the unchanged 250 ms limit. Mutation checks confirmed that removing the cross-line alternative drops all four new cases and that bounding before the next local candidate drops the same-segment case. Rosetta Hooks CI and all current CodeQL checks are also green on the new head. I would prefer to keep #320 scoped to #313 and track the |
2 participants
Summary
Remove repeated suffix rescanning from the
git-branch-deletePreToolUse detector while preserving the force-delete coverage added by #299.Root cause
The previous matcher began at every
git branchcandidate and used two independent lookaheads: one for a delete flag and one for a force flag. Each lookahead scanned forward to the next;,&,|, CR/LF, or end of input.On a separator-free command containing many candidates, unsuccessful matches therefore rescanned overlapping suffixes. Doubling the candidate count produced approximately four times the evaluation time.
Structural solution
The matcher now starts local candidate discovery at each separator-bounded segment boundary and stops the prefix at the first line-local
git branchcandidate.This is sufficient because every later candidate in that segment has a post-
branchlookahead window that is a suffix of the first candidate's window. The existing delete and force predicates are unchanged existential checks, so any qualifying flag pair visible to a later candidate is also visible to the first.A separate alternative preserves the previous
git\s+branchbehavior when the whitespace betweengitandbranchcrosses CR/LF. Those candidates cross a recognized boundary, so this legacy alternative does not recreate repeated scanning of one separator-free suffix. No arbitrary scan-distance bound or shell parser was introduced.Security behavior
The dangerous-command behavior introduced by #299 is preserved, including:
-d -fand-f -d-D,-fd, and-df;,&&,|, LF, and CRLF boundariesThe delete and force lookaheads and separator set are unchanged. Flags from different command segments still cannot combine.
An independently compiled pre-change matcher at
70d401ec6d556005c16182024edfcfad3aa588caand the optimized matcher atc0d6520ae21e054bd438389432b1f258fde8da40(unchanged by follow-up commit55d497009a726c198671106ab54b7d25b8321029) produced zero differences across 12 explicit cases plus 3,000,000 deterministically generated token sequences (seed0x313299). The harness also asserts that the baseline source equals the recorded old matcher and that the baseline and final regex sources differ.Separately, 20 ordering and clustering variants were exercised against Git 2.55.0 using an unmerged disposable branch. Git accepted every form and deleted the branch in every case.
Performance impact
Local Linux Node 24 medians after warmup through the production
evaluateDangerouspath:The fixed series grows approximately with input size and no longer exhibits the prior repeated-suffix rescan trend.
The regression test uses a 250 ms threshold for the 112 KB production-path workload. In a dedicated threshold run, the vulnerable minimum was 1,678.81 ms while the fixed maximum was 4.19 ms, leaving substantial headroom for CI variance.
Tests
Validation
npm ci— passed.npm run check— TypeScript validation passed.npm run test— passed; 45 hook bundles built, 34/34 test files passed, and 1,257/1,257 tests passed.npm audit --omit=dev— 0 vulnerabilities.git diff --check— passed.Risk / blast radius
This modifies a PreToolUse dangerous-action security detector. A false negative could permit an unreviewed destructive branch deletion, while a pathological matcher can delay every Bash tool call.
Risk was controlled by retaining the existing flag predicates and separators, expanding the #299 regression matrix, comparing independently compiled pre-change and final matchers, validating supported forms against real Git, and exercising adversarial production-path inputs. The production change is confined to the
git-branch-deletepattern.Compatibility
No dangerous-action behavior change is intended. Existing lexical separator handling, cross-line whitespace behavior, review-marker policy, evaluation interfaces, and module responsibilities are unchanged. Because this is a local matcher implementation change rather than an architecture change, no architecture documentation update is required.
Limitations
Fixes #313