Skip to content

Make the mutation gate honest on any runner, and move to the current stack - #37

Merged
ddteeter merged 12 commits into
mainfrom
upgrade-stack-and-mutation-guard
Sep 5, 2026
Merged

Make the mutation gate honest on any runner, and move to the current stack#37
ddteeter merged 12 commits into
mainfrom
upgrade-stack-and-mutation-guard

Conversation

@ddteeter

@ddteeter ddteeter commented Sep 5, 2026

Copy link
Copy Markdown
Owner

The finding

A fourth greenfield adoption, run from the packed tarball with the analyzer set at the versions npm resolves today, found that the mutation gate is broken on the current stack — and broken in the worst direction. Same code, same tests, one variable:

stack result
stryker 10 + vitest-runner 10 + vitest 4 12 Killed
stryker 10 + vitest-runner 10 + vitest 5 12 Survived
stryker 9 + vitest-runner 9 + vitest 4 12 Killed

Cause is upstream and known: stryker-js#6210 — vitest 5 changed testNamePattern to join the name chain with ' > ', the runner still uses a space join, so its per-test filter selects nothing and each mutant run executes zero tests. Confirmed with a probe: the suite ran once (the dry run) and never again. No configuration works around it — all six combinations of coverageAnalysis × vitest.related report 12/12 Survived.

This does not miss violations, it manufactures them. In an agent-driven repo the fixer is handed twelve unkillable mutants, the ladder exhausts, and the only exits it can reach — a sanctioned suppression, or analyzers.stryker: "off" — are both forbidden by the scaffolded AGENTS.md. The guardrail teaches the agent to disable the guardrail.

What this does about it

Stops it lying. The report carries an exactly discriminating signal:

coveredBy testsCompleted
genuine survivor (vitest 4, vacuous test) non-empty 1 — all 9
broken runner (vitest 5) non-empty 0 — all 12

A Survived verdict with covering tests and zero executions is not evidence of survival; it is a run that never happened. runStryker now raises one guardrails/analyzer-failed naming the count instead of a storm of false stryker/survived. Upstream proposes the same invariant for itself in #6146 (unmerged since July), and that runner has three more open false-survivor bugs — so the guard stays after #6210 is fixed.

Adds the guard that was missing. test/drift/stryker-runner.test.ts runs real stryker through the vitest runner over code whose tests kill every mutant, and asserts kills come back. Verified both ways: passes on vitest 4, fails on vitest 5 with 14 unrun survivors. It is the only test in the suite that would have failed on a vitest@5 bump.

Closes the dogfooding gap behind all of it. This repo was validating a stack no adopter gets — vitest 4.1 / stryker 9.6 / eslint 9.39 / TypeScript 5.9, against a greenfield's 5.0 / 10.0 / 10.10 / 6.0. Everything but vitest is now current. TypeScript 6 needed tsup's dts pass replaced with tsc --emitDeclarationOnly, since tsup 8.5.1 (the latest) cannot parse it.

vitest is pinned to ^4 with the reason in the file; unpinning is tracked in #35. This PR does not close #35 — it pins, it does not unpin.

Also fixed, from the same adoption

  • git -c core.hooksPath=/dev/null commit walked past the Bash gate. Unlike the accepted git -C <path> miss (the hooks still run), this one defeats the hook floor — and it is the exact bypass AGENTS.md forbids by name. Prose still returns in 0.03s.
  • The seed-ordering trap. A bare greenfield init --apply seeds no analyzer configs (the gating is deliberate), and installing the tools afterwards meets analyzer-failed with upstream's --init advice attached, which writes a different config than the seed. The skipped-analyzer warning now names guardrails init --apply as the second half of the fix.
  • reports/stryker-incremental.json was committed by the first git add -A. Not a gate fail-open — runStryker deletes it before every run.
  • ESLint 10 loads config files per directory, so it walked into the 11 nested .claude/worktrees/ and failed on a rule id this upgrade renamed. Same untracked-but-not-ignored class as the existing nested-worktree finding, new surface.
  • Test helpers and #! entry points were mutated as production codeisTestFile now covers test directories, and a shebang file is excluded from mutation only (eslint and tsc still check it), exactly as isConfigFile already is.

Every unicorn 74 finding in guardrails-core/src is fixed in the code. Two rules are relaxed for test files only, each with its reason in eslint.config.js.

Verification

npm run build, npm test (1106 passing), npm run lint, guardrails verify (clean), npm run test:coverage (97.9% lines), npm run check:graph, sanctions-check, and scripts/smoke-tarball.mjs — all green. No new sanctioned suppression: where the new code would have duplicated one, the parse was shared instead.

Spec: docs/superpowers/specs/2026-09-05-mutation-integrity-and-stack-upgrade-design.md
Plan: docs/superpowers/plans/2026-09-05-mutation-integrity-and-stack-upgrade.md
Findings: plan.md → "Findings: a fourth greenfield adoption"

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4

Test and others added 8 commits September 4, 2026 23:58
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
A Survived verdict is only sound if a covering test actually executed.
@stryker-mutator/vitest-runner reports every covered mutant as Survived on
vitest 5 (stryker-js#6210) because its per-test filter matches nothing, so
each mutant run executes nothing and the runner returns an unexamined
default. unrunSurvivedMutants recognises that shape from the report alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
Replaces a storm of false stryker/survived with one guardrails/analyzer-failed
naming the count. Fails closed: dropping the mutants and reporting nothing
would be a fail-open on the analyzer this pack most depends on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
The banner guard proves we read stryker's output correctly. Nothing proved
the runner still detects anything, which is the gap stryker-js#6210 walked
through: on vitest 5 every covered mutant comes back Survived, so the gate
manufactures violations. Verified both ways -- passes on vitest 4, fails on
vitest 5 with 14 unrun survivors.

The fixture is built in a temp dir rather than checked in, because a fixture
inside the repo is copied into stryker's sandbox during the repo's own
mutation pass and the nested run cannot resolve a runner plugin from there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
unicorn 74 renamed prevent-abbreviations -> name-replacements (same allowList
option) and added ~15 rules. Every finding in guardrails-core/src is fixed in
the code, not silenced. Two rules are relaxed for TEST FILES ONLY, each with
its reason in eslint.config.js: no-top-level-assignment-in-function (flags
vitest's own `let x; beforeEach(() => x = ...)` idiom) and max-nested-calls
(raised to 4, the depth of `expect(await run(build(arg)))`).

Tool-upgrade review per CLAUDE.md: loose-rules.ts needed no change (the new
unicorn rules are style, not the loose classes -- test-integrity, architecture,
mutation, dead-code); audit.ts needed no change (no suppression syntax moved).

Three findings surfaced by running the upgraded gate on ourselves:

- ESLint 10 resolves config files per directory, so it LOADED the eslint
  configs inside .claude/worktrees/ and failed on a rule id renamed by this
  upgrade. Same untracked-but-not-ignored class as the nested-worktree finding
  already in plan.md; both worktrees and .stryker-tmp are now ignored.
- The drift guards cannot run inside our own mutation run -- eslint 10's
  project service uses worker threads that die in a stryker worker, and the new
  runner guard nests a vitest in a vitest. Both now stand down there, sharing
  one documented rule; they still run everywhere else.
- lib was ES2023 while engines.node is >=24, so Iterator#toArray was
  unavailable. Raised to ES2024 + ESNext.Iterator, matching the declared
  runtime.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
tsup 8.5.1's dts pass cannot parse TypeScript 6 -- `Error: error occurred in
dts build` -- and it is the latest release, so `dts: false` and a
`tsc -p tsconfig.build.json` step after it (tsup cleans dist/, so order
matters). The type output becomes one .d.ts per module instead of a bundle;
package-exports now asserts the exports map's `types` target actually exists,
and the tarball smoke test passes, which is what proves the published shape.

TypeScript 7 stays out of reach: typescript-eslint has no v9 and 8.69 declares
typescript >=4.8.4 <6.1.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
- .gitignore now covers reports/stryker-incremental.json. Stryker's
  incrementalFile default is not under reports/mutation/, so a greenfield
  repo's first `git add -A` committed a mutation-result cache. Not a gate
  fail-open -- runStryker deletes that file before every run.
- The skipped-analyzer warning now names `guardrails init --apply`. init
  seeds a starter config only for an analyzer the repo already declares, so a
  bare greenfield init writes none of them, and installing the tools afterwards
  meets analyzer-failed with upstream's --init advice attached, which writes a
  different config than the seed. The gating stays; this is the missing pointer.
- GIT_WRITE learned git's global options, so
  `git -c core.hooksPath=/dev/null commit` is gated. Unlike `git -C <path>`
  (an accepted miss -- the hooks still run), that one defeats the hook floor,
  and it is the exact bypass the scaffolded AGENTS.md forbids by name. Prose
  still returns in 0.03s.
- adopting-guardrails now pins vitest with stryker-js#6210 as the reason, and
  the worked-example table carries the runner rows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
Also DRYs stryker-adapter's report parsing into one `parseReport`. The new
unrunSurvivedMutants had copied the try/catch that carries a sanctioned
equivalent-mutant directive, which sanctions-check correctly caught as the
declared count drifting from 1 to 2. Sharing the parse is the fix; widening the
exemption would have bought no new behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Claude finished @ddteeter's task in 4m 25s —— View job


Review: PR #37 — mutation gate honesty + stack upgrade

  • Gather context (PR body, changed files, CLAUDE.md)
  • Read .github/claude-review-prompt.md for review guidelines
  • Review diff against origin/main...HEAD
  • Post findings

Reviewed the diff directly (git diff origin/main...HEAD) against the guardrails-core review guidelines (security/open-source boundary, guardrail integrity, quality-gate gaming). No .github/workflows changes in this PR, so that section doesn't apply.

Core fix: unrunSurvivedMutants (guardrails-core/src/verify/stryker-adapter.ts)

The discriminating signal (coveredBy non-empty + testsCompleted === 0) is correctly implemented and matches the PR's own truth table. isUnrunSurvivor is written as sequential early-returns rather than one && chain — good given the stated reason (per-clause mutation coverage attribution). NoCoverage is correctly excluded via the covering === 0 early return, so runners without per-test data aren't affected. runStryker (guardrails-core/src/verify/index.ts) correctly discards all findings from a report once any unrun-survivor is detected, rather than partially trusting the rest — right call, since the shared test-runner process means one broken run taints every verdict in it, not just the flagged mutants.

One thing worth confirming, not a blocker: treating a missing testsCompleted as unrun (?? 0 === 0) fails safe for vitest/jest as documented, but if any other supported testRunner plugin (e.g. dotnet, karma) reports coveredBy without ever populating testsCompleted, this would reclassify its genuine Survived verdicts as analyzer-failed instead. Worth a quick check across the runners this repo's adopters actually use, if not already done — the drift guard (test/drift/stryker-runner.test.ts) only exercises the vitest runner.

test/drift/stryker-runner.test.ts

Good guard — runs real stryker through the real vitest runner against a fixture with fully-killable mutants, so it would have caught #6210 directly. Sensible to build the fixture in a tmpdir rather than checking it in (avoids the nested-mutation-sandbox problem the banner test already worked around), and describe.skipIf(isUnderMutationRun(...)) correctly avoids infinite regress when this repo mutates itself.

Bash-gate regex fix (guardrails-core/src/cli-core.ts, GIT_WRITE)

This is the security-relevant change (closes the git -c core.hooksPath=/dev/null commit bypass of the Bash PreToolUse gate). Traced through the regex by hand:

  • GIT_GLOBAL_OPTIONS correctly consumes -c <value>/-C <value> as two tokens and any other -x flag as one token, before requiring commit/push.
  • The [ \t]+ / [^ \t]+ character classes stay disjoint the same way COMMAND_START's do, so no catastrophic-backtracking path opens up — consistent with the existing ReDoS-avoidance discipline in this file.
  • Confirmed via test/cli-core.test.ts cases: git -c core.hooksPath=/dev/null commit -m x, git -c a=b -c c=d commit -m x, git --no-pager commit -m x, git -C packages/web commit -m x, and the cd /tmp && ... compound form, plus the existing negative cases (legit commit, git commits-are-fun, mention-only). Good coverage of the fix and no regressions to the documented misses (FOO=1 git commit, xargs git commit — both still legitimately fall back to the git-native hook floor).

Diff-auditor / loose-rules review (per CLAUDE.md's tool-upgrade checklist)

Checked both halves this PR is supposed to review on every leveraged-tool bump:

  • audit.ts's suppression signatures (eslint-disable, @ts-*, .skip, etc.) are unchanged in this diff — consistent with eslint 10 / unicorn 74 not altering suppression syntax.
  • loose-rules.ts additions (unicorn/name-replacements rename handling via ruleName) look correct; the ruleName simplification removing the slash === -1 branch is backed by the stated equivalent-mutant argument and looks right (lastIndexOf-1, slice(0) → whole string).
  • eslint.config.js's two test-only rule relaxations (unicorn/no-top-level-assignment-in-function off, unicorn/max-nested-calls at 4) are scoped to test files only, each with a concrete justification — not a repo-wide loosening.

Everything else

exec.ts, config.ts, tsc-adapter.ts, gate.ts/gate-decision.ts changes in this diff are mechanical (unicorn-74-driven refactors, e.g. reordering && conditions per unicorn/prefer-simple-condition-first, ternary→&& object-spread rewrites) with equivalent-mutant reasoning preserved in comments — no behavior changes spotted. exec.ts still spawns with shell: false; no new shell-interpolation surface introduced. tsup.config.ts / tsconfig.build.json split (tsup bundles JS, tsc --emitDeclarationOnly emits types) is a reasonable workaround for tsup 8.5.1 not parsing TS 6, and runs in the right order (tsc after tsup, since tsup's clean: true would otherwise wipe the .d.ts output). vitest is correctly left pinned at ^4.1.10 in both package.json files, matching the PR's stated scope (pins, does not unpin #35).

No secrets, no org-specific values, no sanctionedSuppressions additions, and no gate-weakening changes found.

Overall: solid. The core fix is well-targeted, the security-relevant regex change is correctly reasoned through and tested, and the drift guard is real (executes actual stryker rather than asserting against a synthetic report). Only the one non-blocking cross-runner question above.

From PR review: if a runner populated coveredBy but never testsCompleted, the
guard would reclassify its genuine survivors as analyzer-failed. Measured on
the command runner -- the one init seeds by default -- with a vacuous test:
11 genuine survivors, every one coveredBy: []. A runner that cannot attribute
tests to mutants reports no covering list at all, so it exits at the
covering === 0 check. Zero misfires. Recorded on the test that covers it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
@ddteeter

ddteeter commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Checked the testsCompleted-missing concern empirically rather than reasoning about it, because the runner it would matter most for is the one init seeds by default.

It cannot misfire, and the reason is the other half of the condition. A runner without per-test data does not report a partial record — it reports no covering list at all. Measured on the command runner with a deliberately vacuous test:

COMMAND RUNNER — survivors: 11
  testsCompleted values: [1]
  coveredBy lengths:     [0]
  WOULD MISFIRE:         0

Every genuine survivor comes back coveredBy: [], so it exits at the covering === 0 early return before testsCompleted is ever consulted. That is why the guard keys on coveredBy being non-empty rather than on testsCompleted alone: a runner that cannot attribute tests to mutants has nothing to contradict, and only one that claims covering tests and then runs none is making an unsupported claim.

So the ?? 0 fallback only bites a runner that populates coveredBy and omits testsCompleted — which would be a self-contradicting report, and failing closed there is the direction we want.

Pinned in 4dbefd8 on the test that already covered the shape, with the measurement, so the reasoning survives the next runner bump.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Claude finished @ddteeter's task in 6m 11s —— View job


Review: PR #37 (re-review after 4dbefd8)

  • Gather context (PR body, diff, CLAUDE.md, prior automated review + author's follow-up)
  • Read .github/claude-review-prompt.md
  • Review diff against origin/main...HEAD, focused on what's new since the last pass
  • Post findings

The prior review's one open question — whether a runner that populates coveredBy but never testsCompleted could misfire — is resolved. 4dbefd8 documents the empirical answer directly on the covering test (stryker-adapter.test.ts): a runner without per-test data (measured on the command runner) reports coveredBy: [], so it exits at the covering === 0 check before testsCompleted is ever consulted. That's the right fix — pinned as a comment on the test that already exercised the shape, not a new assertion bolted on. No concerns with unrunSurvivedMutants, runStryker's all-or-nothing discard, or the drift guard (test/drift/stryker-runner.test.ts) — re-checked them this pass and they hold up; it genuinely runs stryker through the real vitest runner over a fixture where every mutant is killable.

New finding: possible ReDoS in the rebuilt GIT_WRITE regex

guardrails-core/src/cli-core.ts:336-343 (GIT_GLOBAL_OPTIONS):

const GIT_GLOBAL_OPTIONS = String.raw`(?:[ \t]+(?:-[cC][ \t]+[^ \t]+|-[^ \t]+))*`;

The two alternatives inside the star are not length-disjoint: -[cC][ \t]+[^ \t]+ can consume a -c token plus a following --prefixed token as its value, while -[^ \t]+ can consume that same following token as its own, independent option. For input like -c -c -c -c ... -c (all tokens starting with -), every adjacent pair of tokens can be grouped either as one "-c + value" iteration or as two independent-option iterations — the same shape as the textbook (a|aa)* ReDoS pattern, with a Fibonacci-many number of equivalent partitions for n tokens. If the tail after the option run never resolves to commit/push, the backtracking engine has to explore all of them before failing, which is exponential in n.

This regex tests input.command from the PreToolUse hook payload (cli-core.ts:363) — the literal shell command the agent is about to run, i.e. attacker/agent-controlled input reaching a regex, which is exactly the class this repo's review guidelines call out as highest-priority. A crafted command like git + -c -c .repeat(30) + X (no real "commit"/"push" needed) would be trivial for a compromised or prompt-injected agent to emit, and — if the pattern is as ambiguous as it looks — would hang the gate process on every Bash tool call matching that shape.

I wasn't able to run a live timing check in this sandboxed job (no permission to execute arbitrary node), so this is reasoned from the regex's structure rather than confirmed by measurement — but the shape matches a well-documented ReDoS class closely enough that it's worth a two-minute empirical check before merging: time GIT_WRITE.test() against 'git ' + '-c -c '.repeat(30) + 'X' (and a few larger n). Existing tests only cover small, well-formed cases ('git -c a=b -c c=d commit -m x'), nothing adversarial/long. If it does blow up, the fix is likely to make the value-token alternative unambiguous (e.g. disallow a leading - in [^ \t]+ for the -c/-C value, since a real -c key=value value never starts with -) rather than to touch the padding/separator classes, which are correctly reasoned through in the file's own comments.

Fix this →

Everything else

Spot-checked the rest of the delta (audit.ts, loose-rules.ts, verify/git.ts's isTestFile directory handling, the tsup/tsconfig.build.json split, package.json version bumps): all mechanical or correctly reasoned, consistent with the PR description. ruleName's branchless rewrite in loose-rules.ts is a correct equivalent-mutant simplification (lastIndexOf-1, slice(0) → whole string). No secrets, no org-specific values, no new sanctionedSuppressions, no gate-weakening changes.

Overall: solid, and the author's response to the prior review's question is well-supported by measurement. The one item worth a look before merge is the GIT_GLOBAL_OPTIONS alternation above — everything else checks out.

Confirms in a real consumer repo what the unit tests assert: the exit criterion
is reachable in one pass, and on vitest 5 the adopter gets one honest
analyzer-failed instead of 17 false survivors handed to the fixer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Claude finished @ddteeter's task in 2m 4s —— View job


Review: PR #37 (re-review after new push)

  • Gather context (PR body, diff, CLAUDE.md, prior review comments)
  • Check whether the previously-flagged GIT_GLOBAL_OPTIONS ReDoS concern was addressed
  • Review diff against origin/main...HEAD for anything new
  • Post findings

The new commit since the last automated pass is 914d9b0 only — a docs-only addition to plan.md (23 lines) recording a real-repo tarball verification (init --apply naming the second-half fix, the exit criterion reachable in one pass, and the vitest-5 stack turning 17 false survivors into one honest analyzer-failed). No code changed, so it doesn't alter anything from the prior two review passes.

Outstanding: GIT_GLOBAL_OPTIONS ReDoS is still unaddressed

Flagged in the previous pass and still present, unchanged, in guardrails-core/src/cli-core.ts:340:

const GIT_GLOBAL_OPTIONS = String.raw`(?:[ \t]+(?:-[cC][ \t]+[^ \t]+|-[^ \t]+))*`;

I tried to get an empirical timing measurement this pass too but node execution isn't permitted in this job's sandbox (same limitation as last time) — so this is reasoned from structure, not measured. Worked through it token-by-token though, and the ambiguity is concrete, not just "shape looks similar":

For a run of k -c tokens (each separated by a single space) followed by a non-commit/push tail, each adjacent pair of tokens can be grouped two ways — as one alt1 iteration (-c flag + next -c as its value) or as two independent alt2 iterations (each -c on its own). That's the same tiling-with-1s-and-2s structure as the textbook (a|aa)* catastrophic-backtracking pattern: the number of equivalent partitions is Fibonacci(k). Since the overall GIT_WRITE match ultimately fails on this input (no commit/push at the end), a backtracking engine has to exhaust all of them before giving up — Fibonacci(k) is already ~1.5×10⁹ at k=60 (i.e. n=30 in '-c '.repeat(30), the case suggested last pass). V8's Irregexp doesn't memoize sub-pattern failures across an alternation like this, so nothing short-circuits that search. test/cli-core.test.ts still has no adversarial/long-input case for GIT_WRITE (checked — only small well-formed strings like 'git -c a=b -c c=d commit -m x').

Since this regex runs on input.command straight from the Bash PreToolUse hook payload (cli-core.ts:363) — i.e., a string an agent (potentially compromised or prompt-injected) controls — and a hang here stalls the gate on every matching Bash call, this is worth resolving before merge rather than carrying forward. The fix suggested last pass still looks right: make the -c/-C value token exclude a leading - (a real -c key=value value never starts with -), which removes the alt1/alt2 overlap without touching the padding/separator classes the file's comments already reason through carefully.

Fix this →

Everything else (unchanged from prior passes)

Re-confirmed the core fix is sound: unrunSurvivedMutants's discriminating signal (coveredBy non-empty + testsCompleted === 0), runStryker's all-or-nothing discard, and test/drift/stryker-runner.test.ts running real stryker through the real vitest runner all hold up. The author's 4dbefd8 follow-up empirically closed the one open question from the first pass (a runner without per-test data reports coveredBy: [], so it never reaches the testsCompleted check). Diff-auditor/loose-rules review, the Bash-gate global-options fix's non-ReDoS behavior, and the tsup/tsc --emitDeclarationOnly split all check out as previously reported. No secrets, no org-specific values, no new sanctionedSuppressions, no gate-weakening changes.

Overall: solid, with one carried-forward item — the GIT_GLOBAL_OPTIONS ReDoS — worth resolving (or empirically ruling out, if node can be run in a less restricted environment) before merge.

Caught in PR review. The option alternation could tile a run of flags two ways
(`-c -c` as flag-plus-value or as two flags), the (a|aa)* shape: Fibonacci(k)
partitions a failing match must exhaust. Measured 5.69s at 40 tokens and 51s
at 64 on the Bash PreToolUse hook's own input -- a long enough command would
have hung the session. Requiring a -c value to start with a non-dash leaves one
parse: 0.02ms at n=1000, every gating case unchanged.

Worth recording why lint missed it: splitting the literal into String.raw
fragments to satisfy sonarjs/regex-complexity also took it out of static
analysis. Pinned by an adversarial timing test instead.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
@ddteeter

ddteeter commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

You were right, and I was wrong to ship it. Measured it before fixing:

flags time
20 0.5ms
24 2.9ms
40 5.69s
64 51.6s

Exactly the Fibonacci tiling you described — -c -c readable as one flag-plus-value or as two flags — and it runs on input.command from the Bash PreToolUse hook, so a long enough command would have hung the session. That is the same denial-of-service the command-position anchoring was added to end, reintroduced by the fix layered on top of it.

Fixed in 43ca11a by requiring a -c VALUE to start with a non-- character. A token beginning - can then only be read as a flag and one that does not only as a value, so there is exactly one parse. Linear now — 0.02ms at n=1000 — with every gating case unchanged (-c core.hooksPath=…, -C <path>, --no-pager, chained -c a=b -c c=d, and the negatives).

The part worth keeping from this. This repo's own sonarjs rejected the previous super-linear pattern before it landed. This one got past because I had split the literal into String.raw fragments — to get it under sonarjs/regex-complexity's ceiling — and a pattern assembled at runtime is invisible to static analysis. Satisfying the complexity rule is what removed the pattern from the rule's own view. So it is now pinned by an adversarial timing test rather than by lint, since the guarantee is "no exponential path exists" and no example input can assert that. Recorded in plan.md alongside the finding.

Thanks — this was the one thing in the PR that could have made the gate worse than not having it.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Claude finished @ddteeter's task in 2m 21s —— View job


Review: PR #37 (re-review after 43ca11a — ReDoS fix)

  • Gather context (prior reviews, PR body, .github/claude-review-prompt.md)
  • Verify the GIT_GLOBAL_OPTIONS ReDoS fix in 43ca11a actually closes the reported issue
  • Check for regressions in the gating cases the fix must preserve
  • Review anything else new since the last automated pass
  • Post findings

The GIT_GLOBAL_OPTIONS catastrophic-backtracking finding from the prior two passes is resolved in 43ca11a. (Same sandbox limitation as previous passes: node/npx execution isn't permitted in this job, so I re-derived this from the regex structure rather than re-measuring the timings the author already posted — 5.69s→0.02ms — which are consistent with the fix below.)

Why the fix closes the ambiguity. Old value alternative: -[cC][ \t]+[^ \t]+. New: -[cC][ \t]+[^-\s][^ \t]* — the value's first character can no longer be -. Walk the attack input -c -c -c ... status through it: every token is literally -c, which starts with -. Under the new pattern, a --leading token can never satisfy the value slot ([^-\s] excludes it), so it can only ever be consumed by the standalone-flag alternative -[^ \t]+. That removes the second parse entirely — there is no longer a way to read two adjacent -c tokens as "one flag + its value" or "two flags"; only the latter is legal. That's exactly what turns the Fibonacci(k)-partition search into a single linear pass, matching the measured 0.02ms at n=1000.

Checked it doesn't reintroduce a false-negative on legitimate git -c usage where the config value itself starts with - (e.g. -c http.extraHeader=-x) — rare, but worth checking since the fix restricts the value grammar. Even in that edge case the gate's actual job (detecting commit/push anywhere in the token stream) still holds: the --leading "value" just gets consumed by the standalone-flag alternative instead of the value alternative, and the trailing commit/push is still matched either way. So the fix trades an internal parse distinction for linear time without weakening detection.

Test (test/cli-core.test.ts, "cannot be made to backtrack catastrophically"): builds git ${'-c '.repeat(64)}status — a failing match (tail is status, not commit/push), which is the right shape, since it forces the engine to exhaustively fail rather than short-circuit on an early success. Asserts wall-clock <1000ms, consistent with the documented reasoning that "no exponential path exists" can only be pinned by timing, not by an example input.

Also checked: no other regex in cli-core.ts is assembled from String.raw fragments the way GIT_GLOBAL_OPTIONS was (the thing that hid this from sonarjs/regex-complexity in the first place) — COMMAND_START is a single non-nested character-class alternation with no overlapping-quantifier shape, so it isn't at risk of the same class of bug. The diff since the last full pass (914d9b0) is scoped to exactly this fix (cli-core.ts, the new test, and a plan.md writeup) — no other production code changed.

Overall

All findings raised across this review thread are now resolved: the mutation-gate honesty fix, the Bash-gate global-options bypass fix, and the ReDoS in that fix's own regex. Nothing outstanding from my side.

Raised in review as the one place the linearity fix could have traded time for
a false negative: `git -c http.extraHeader=-x commit` is now read as two flags
rather than flag-plus-value. Different internal parse, same verdict.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y7QdHVqb851SbSZCeitqg4
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Claude finished @ddteeter's task in 3m 5s —— View job


Review: PR #37 (re-review after 3311eb2)

  • Gather context (prior reviews, PR body, CLAUDE.md, .github/claude-review-prompt.md)
  • Check what changed since 43ca11a (last reviewed commit)
  • Review diff against origin/main...HEAD for anything new
  • Post findings

Only one commit landed since the last pass: 3311eb2, test-only (guardrails-core/test/cli-core.test.ts, +7/-0). It adds two regression cases to the 'still fires wherever a git write is actually a command' block — git -c http.extraHeader=-x commit -m y and git -c a=-1 -c b=-2 push — pinning that a -c config value containing a dash doesn't slip past the linearity fix from 43ca11a.

Note: node/npx execution still isn't permitted in this job's sandbox (same limitation as the ReDoS review passes), so I traced the regex by hand again rather than running it — GIT_GLOBAL_OPTIONS's value alternative is -[cC][ \t]+[^-\s][^ \t]*. For both new strings, the token immediately after -c (http.extraHeader=-x, a=-1, b=-2) starts with a non-- character, so each is consumed whole by the flag+value alternative in a single iteration — no backtracking into the two-flags fallback is needed, and the match proceeds straight through to commit/push. So the assertion (toContain('deny')) holds, and — walking the true two-flags-fallback case by hand too (a token that itself starts with - right after -c, e.g. git -c -x commit) — that path still gates correctly as well, via two alt2 iterations. No regression, no bypass.

Minor nit, non-blocking: the new comment (cli-core.test.ts:837-841) says these two inputs are "read as two flags instead of flag-plus-value" — but per the trace above, neither actually exercises that fallback path; both are still parsed as flag+value in one shot, since the dash in http.extraHeader=-x/a=-1/b=-2 isn't the token's first character. The genuinely two-flags-triggering case is a value token that itself starts with - immediately after -c (e.g. git -c -x commit), which isn't among the added strings. Functionally harmless — I confirmed by hand that case also still gates — but if the intent is to pin the fallback path specifically (not just "still gates"), consider adding that exact shape as a third case, or adjusting the comment to describe what's actually being tested (a dash inside the value, not a leading one).

Overall

All substantive findings from this review thread are resolved: the mutation-gate honesty fix, the Bash-gate global-options bypass fix, and the GIT_GLOBAL_OPTIONS ReDoS and its fix are all sound and now backed by tests, including this last regression case. Nothing blocking remains from my side — the one item above is a documentation/test-precision nit, not a functional or security issue.

@ddteeter
ddteeter merged commit 2579091 into main Sep 5, 2026
4 checks passed
@ddteeter
ddteeter deleted the upgrade-stack-and-mutation-guard branch September 8, 2026 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unpin vitest@^4 once stryker-js#6210 (Vitest 5 false survivors) is fixed

1 participant