fix(guardrails): replay a wrapper's chdir in the lease hash probe and rescope skill-reference-verify reconstruction - #2100
Conversation
Unverified batch preserved from a session that ended before its findings were falsified against origin/main. Touches block-dangerous-git and skill-reference-verify plus their test suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps this branch's 0.20.0 entry above the 0.19.1 and 0.19.2 entries that landed on main, and bumps the manifest to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ed-review-threads
…d skills-path exception The reconstruction docblock cited 65b4f67 for the stale-path-verify line-anchoring fix. That SHA is a pre-squash branch commit and is not an ancestor of main; the commit that actually landed it is a2d98f8 (#1432). The skill_roots docblock stated the manifest `skills` additive rule without its marketplace-root exception, where declared subdirectories replace the default scan instead. Record the exception and why it is left unmodelled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the branch's 0.20.0 entry above main's 0.19.3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Renumber the guardrails bump to 0.21.0 above main's 0.20.0 and keep the CHANGELOG strictly descending; take main's plugin description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard's acceptance behavior changes in both directions, so the entry says so rather than leaving a reader to infer the bump from the diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 5m 3s —— View job Security reviewScope: the 6 files changed in this PR (
One finding worth flagging on this file: SUGGESTION / PLAUSIBLE — skill-reference-verify.sh#L338-L384
Failure scenario: an Edit whose No other injection, authorization, secret-handling, or fail-open/fail-closed logic issues found in the changed files. I did not find any issue that would let the |
![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: e414162a1f
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 8m 1s —— View job Code review — PR #2100
This repo has a Scope note: couldn't run
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
… line Dropping the per-line subprocesses removed only half the reconstruction cost. The per-line RESCAN remained, and it is anchors times file size: a thousand span-free hunk lines cost 82 s against the hook's 30-second budget on a Windows/Git Bash host. Nothing bounded the product — the span cap stops early only when the hunk's lines carry code spans, which is why the earlier measurement missed it. The hunk reaches disk contiguously, so locate it whole: one scan for the whole edit. The span set is unchanged, because a line anchor's extent is the text the edit wrote on that line and the whole hunk's extent is the union of exactly those. Same measurement: 11 s at a thousand lines, 11 s at four thousand. Locating whole also scopes better than the walk it replaces — a hunk whose every line repeats but whose whole text does not used to be dropped as ambiguous line by line, and now resolves to the one place it names. The walk stays as a fallback for a hunk no longer on disk verbatim, under a total scanning budget rather than an anchor count. anchor_offsets now does one pattern operation per iteration instead of two; testing with `[[ == *"$anchor"* ]]` and then stripping searched the same text twice, which doubled the term the whole-hunk locate is dominated by. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ady has The whole-hunk locate leaves this anchor's offsets in `$offs`, and the loop immediately recomputed them. One redundant full-file scan per anchor is the same per-anchor cost the whole-hunk locate exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 4s —— View job Code review — PR #2100 (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 2m 46s —— View job Security review
Scope: the 6 files changed in this PR (
I don't have a live sandbox to re-run the benchmark, but the fix is structural (a budget divides total chars by content length, not an anchor-count heuristic that could be out-argued by a different input shape), so I'm confident in the analysis without re-measuring. Residual, informational only, not new: No new injection, authorization, secret-handling, or fail-open/fail-closed logic issues found in either changed hook, the two test files, Verdict: no security issues to report on this synchronize. The one prior SUGGESTION-severity finding (anchor-count-driven timeout risk) is fixed by |
…ed-review-threads
The caps were set against a linear cost model the scan does not have. Measured on a quiescent Windows/Git Bash host, best of three, one anchor_offsets scan costs 0.07 s at 32 KiB, 0.24 s at 64, 0.53 s at 96, 1.07 s at 128, 2.18 s at 192 and 3.94 s at 256 — quadratic, because bash's `%%` pattern strip walks the string instead of indexing it. Extrapolated, the previous 4 MiB file cap allowed a SINGLE scan of about eighteen minutes, so the worst case had never been bounded, only moved off the per-line loop. Reconstruction now stops above 256 KiB, where one scan is still ~4 s, and the fallback's anchor cap falls on the same curve rather than being a flat count or a total-characters budget: 123 anchors at 32 KiB, 30 at 64, 7 at 128, 1 at 256. Above the file cap the direct hunk scan is untouched, so a complete reference is still reported and only partial-edit recovery stops — the permitted direction. Tests: a case that combines a large file WITH the fallback path, which neither the existing timing case (whole-hunk fast path) nor the correctness cases (three lines) reached, and a case above the file cap. Both assert what the bound delivers, not only that it exists — the scale case's hunk carries a bare substring of the reference, so the direct scan cannot report it and a bound that admitted nothing would fail. Also: the unresolved-reference advisory named plugins/<plugin>/skills/ as the place searched whatever the manifest declared. It now lists the directories the search actually covered, from the same skill_roots the resolution used. The message ends by telling the reader to confirm against the tree, so naming the wrong part of it is the one thing it cannot do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 23s —— View job Security review (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 3m 7s —— View job Code review — PR #2100 (re-run at HEAD
|
…clock Retunes the two measured bounds and stops asserting them with a clock. The first pair (256 KiB file cap, 126000 budget) was derived from an isolated scan curve that is a FLOOR rather than the cost: it times an anchor matching near the end, so one strip walks the file and the second is free, while a no-match strip walks it twice (2.31 s at 200 KiB) and the whole-hunk probe pays a scan before the fallback runs at all. End to end that pair spent 21 s of a 30 s budget. The cap is now 128 KiB with a 60000 budget, calibrated against the hook rather than the curve. The scale case no longer asserts wall time. On this host the same fixture read 21 s loaded and a smaller one 23 s, against an isolated scan of ~1 s at that size — a timing assertion that noisy fails on load and passes on a regression that happens to run on a quiet box. It now pins the cap from both sides instead: a reference on a hunk line inside the anchor cap is still reported, one on a line far past it is not. Both are reachable only through reconstruction, so neither can pass on the direct hunk scan. Two assertions added where a prefix match was hiding the subject: the advisory's directory list is now asserted whole for a plugin with declared paths, and the conventional layout is pinned to the rendering it always had. The file-size gate moves ahead of the CR strip. Every step past that gate is a whole-string operation, so a gate placed after one of them leaves that one unbounded; measuring the raw text also errs toward not reconstructing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 30s —— View job Code review — PR #2100 (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 1m 2s —— View job Security review (re-run at HEAD
|
…ed-review-threads
|
Claude finished @kyle-sexton's task in 1m 4s —— View job Security review (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 47s —— View job Code review — PR #2100 (re-run at HEAD
|
…oding a literal search (#2127) ## Summary `skill-reference-verify`'s `reconstruct_partial_edit` searched in whatever locale the invoking shell happened to carry. Every search it performs is **literal**, but bash's `%%` pattern strip **decodes** rather than compares under a multibyte locale, so the scan paid for a decode it never used — and the hook's cost and its matcher semantics both became a function of the consumer's ambient environment rather than of its own code. This lands as a **correctness and determinism** fix, not a performance claim. No wall-clock bound is asserted anywhere in the diff. The measured ratio below is evidence of magnitude only. ## Measurements (mine, this host) Windows / Git Bash, bash 5.3.15, 32 logical cores at **~18% CPU**, 30.7 GB of 63.7 GB free, 63 bash processes alive — several agents share this box, so these are **ratios on a lightly-loaded host**, not a bound. One no-match `%%` strip, best of three, measured in-process (no fork inside the timed region): | size | `LC_ALL=C` | `en_US.UTF-8` | ratio | | ---: | ---: | ---: | ---: | | 32 KiB | 0.054 s | 0.395 s | 7.3x | | 64 KiB | 0.221 s | 1.447 s | 6.5x | | 128 KiB | 0.880 s | 5.786 s | 6.6x | | 192 KiB | 1.964 s | 12.458 s | 6.3x | | 256 KiB | 3.410 s | — | — | The ambient environment on this host is `LANG=en_US.UTF-8` with `LC_ALL` unset, so the hook really did run in the multibyte column. ## Why `local +x`, not `local` The `+x` is load-bearing, and this is the one place the prepared patch was wrong. The entire ~6.5x is **bash's own matcher**. The child processes are locale-insensitive here — the inline-code-span `grep -oE` over the same 64 KiB measured **0.139 s under both locales**. So exporting the pin buys nothing — and it costs real behavior. A plain `local LC_ALL=C` inherits the export attribute whenever the consumer exported `LC_ALL`, which pushes the pin into `emit_refs`' `grep`/`sed`. GNU `[[:space:]]` matches U+00A0 / U+3000 / U+2028 under a UTF-8 locale but only ASCII under C. Instrumented on the real hook, with the caller exporting `LC_ALL=en_US.UTF-8` and a reference reachable only through reconstruction: ``` local LC_ALL=C ctx=…ghost-nbsp c2 a0 arg… emit_refs -> [] SILENT local +x LC_ALL=C ctx=…ghost-nbsp c2 a0 arg… emit_refs -> [/alpha:ghost-nbsp] REPORTED ``` Same `ctx` bytes in both — the byte slicing is correct either way. The plain `local` form silently **drops a real finding**. `+x` keeps the whole benefit and none of that. The prepared patch's comment asserted the children's "output is byte-identical either way, verified on non-ASCII input". That is false as written; it is also moot under `+x`, and the comment now says what was actually measured. ## Correctness sub-claims, each verified rather than taken | claim | verdict | | --- | --- | | assigning `LC_ALL` re-runs `setlocale` even for a `local` (and for `local +x`) | **holds** — `${#}` and `%%` both switch to byte semantics inside the function | | bash restores the prior value on return | **holds** in all three caller states (unset / set-unexported / exported) | | …and restores the **export attribute** | **holds** — an exported `LC_ALL` is still `declare -x` with its original value after return | | children inherit the pin only when the consumer exported `LC_ALL` | **holds** for plain `local`; under `+x` they never inherit it in any state | | byte-vs-char offsets stay inside the function | **holds** — every offset is produced and consumed within the pinned region | | no UTF-8 multibyte sequence contains an ASCII byte | **holds**; slices land only at a literal match or a newline, so a byte slice cannot split a character | ## The regression test: what it can and cannot discriminate The shipped multibyte fixture is **not vacuous** in the way the first draft was — the skill name is ASCII (the `emit_refs` grammar is `[a-z0-9-]`, so a non-ASCII name is unreportable by design) and the multibyte text sits around the anchor. But **it cannot discriminate the pin**. I mutated it: with the pin reverted entirely, it still passes. That is expected and correct — byte offsets and character offsets are each internally self-consistent, so a mis-slice is **not constructible** while every offset is produced and consumed inside one locale. I state that plainly rather than claim the case catches something it does not. What *is* constructible, and what I added, is a case that discriminates the **pin form** — the thing that can actually regress. With the consumer exporting a UTF-8 locale, a reference whose argument separator is U+00A0 must still be reported; a plain `local` pin makes it silent. Its separator is built from `printf '\xc2\xa0'` rather than a literal byte, because a literal one **was** silently normalized to an ASCII space while I was writing it, which made an earlier run of my own A/B vacuous in exactly the way the previous agent's first draft had been. Two observable **threshold** shifts the pin does introduce, both toward *less* work and both noted in the docblock: - `RECONSTRUCT_MAX_CHARS` is now read as bytes, the stricter reading — it cannot raise the ceiling it exists to set. - the fallback's KiB estimate stops understating a multibyte file and over-granting its anchor cap. ## Docblock **Re-labelled, not re-measured.** The constants docblock published `0.07 s at 32 KiB … 3.94 s at 256 KiB` with no locale named. Those figures match the C column almost exactly, but the hook did not then run in C — so the table described a locale the code never used. The pin makes C the actual locale, so the label is now correct as written. I added a re-check from this host (0.054 / 0.221 / 0.880 / 3.410 s at 32 / 64 / 128 / 256 KiB) as corroboration, and a note explaining why the label is not a footnote. ## Coverage gap — left open, deliberately The fallback-scale case previously traded its wall-clock assertion for behavior assertions. That was defensible (those readings were mostly ambient overhead), but it left **no test that would catch a locale-driven cost regression**. This PR **does not close that gap**. The one wall-clock assertion still present — `big_elapsed < 30` on the 1000-line case — cannot close it either: that fixture is ≈38 KiB, so one scan is ~0.07 s under C and ~0.5 s under UTF-8, both three orders below the ceiling. The new case pins the pin's *form*, not its *cost*. Closing the cost gap needs a deterministic proxy rather than a wall clock, and I did not invent one here. ## Noted, not fixed The **direct** `emit_refs` scan (outside the reconstruction) still runs in the ambient locale, so its `[[:space:]]` breadth remains locale-dependent. Pinning the whole hook would change the grammar the guard reports on and needs its own justification, so it is out of scope here. No linked issue ## Related - #2100 — the PR that introduced the reconstruction cost curve and the caps this change re-labels and pins the locale for. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…t REPO_ROOT read (#2128) No linked issue ## Summary Test-only follow-up to #2121. That PR shipped a comment asserting the `REPO_ROOT` override's *inertness* was **not behaviourally observable**, and left the negative case unwritten on that basis. The assertion was wrong. A fourth instrument exists, it works, and the negative is now written. The claim mattered beyond the missing case: a comment saying a thing cannot be observed tells the next maintainer to stop looking. ## Why three instruments failed All three try to observe an **effect** of `REPO_ROOT`: 1. **Lint output.** Force the override to fire on a file at a real repository root — replace the probe with `false` — and the file is still not rewritten. `markdownlint-cli2` performs its own config discovery and does not cross the repository boundary, so widening the hook's gate changes no observable byte. 2. **Telemetry `data.file`**, which is derived from `REPO_ROOT` and looked like the obvious answer. The forced-override run emits an **empty** value rather than a relative-to-outer path — and empty is also what a sink that never populated looks like, so the assertion could not separate a regression from a flaky sink. 3. **Exit status** is 0 either way. ## The fourth instrument reads `REPO_ROOT` directly The hook resolves a repo-local linter at `"$REPO_ROOT/node_modules/.bin/markdownlint-cli2"`. Plant a distinguishable shim at **both** candidate roots and whichever one runs names the root the hook actually computed. That is a read of the variable from outside the process, not an inference. ``` POST: negative (file dir IS a repo) -> INNER positive (file dir is NOT a repo) -> OUTER ``` **Control for the negative** — a hook whose probe is forced to `false`, so the override always fires: ``` correct hook -> INNER forced-override hook -> no marker ``` The assertion is **positive**: the marker must read `INNER`. A wrongly-firing override produces `OUTER` or no marker at all, and both fail it. ### Two mechanics that silently defeat this Recorded in the test file, because each one makes the instrument look like a dead end: - **The `PATH` copy of `markdownlint-cli2` wins** over the repo-local one, so the shim never runs while the real binary is reachable. The case strips only the directories carrying it, leaving `jq` and `git` on `PATH` — remove those and the hook exits early for unrelated reasons. - **The shim cannot announce itself on stdout or stderr.** The hook captures both into a variable, so anything printed is swallowed. It must write a **marker file**. ## Tests ``` ok: git present, dir is no repo: the override fires and CLAUDE_PROJECT_DIR terminates the walk ok: git present: the override stays inert — the hook resolved REPO_ROOT to the git toplevel ``` The unusable-environment branch emits a **visible** `ok` rather than passing over in silence — this suite has no skip helper and sources none, and a silent omission is exactly what `scripts/check-silent-skips.sh` exists to catch. ## Credit and provenance The instrument was found by the session that wrote the guardrails work on #2100, after I concluded the negative was unwritable. I reproduced it independently before building on it, including the forced-override control above. Worth recording alongside it: while testing this, that session hit the same defect the reviewer found in #2121's first attempt — fixtures built under a Windows 8.3 shortname (`C:/Users/KYLESE~1/…`) while `hook::repo_root` returns the long form, so the guard `"$REPO_ROOT" == "$(dirname "$FILE")"` compared two spellings of one directory and was always false. The branch never executed and the output looked plausible throughout. Same lesson as the rest of this sequence: **prove the fixture reached the path under test.** ## Related - #2121 — where the override landed and where the incorrect comment shipped - #1938 — the stranded post-merge review-findings sweep --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s from every git guard (#2147) ## What Two live holes on `origin/main`. One is specific to `block-dangerous-git`'s lease-width probe; the other is in the **shared argv resolver** and reached every guard in every plugin. `hook-utils.sh` exists in **17 places** — `lib/hook-utils.sh` plus a synced copy in each of 16 plugins — and all 17 were stale. An independent adversary confirmed the resolver hole is not lease-specific: behind `env -S`, `block-no-verify` allowed `git commit --no-verify` and `block-dangerous-git` allowed `git reset --hard`. All 17 copies are patched here. It also proved the lease hole live rather than theoretical: in a SHA-256 repository carrying a ref literally named `0123456789abcdef0123456789abcdef01234567`, the cleared force push **clobbered the remote branch with unrelated orphan history**, rc=0, with `rev-parse` captured before and after. The guard allows `--force-with-lease=<ref>:<expect>` only when `<expect>` is a **full-width object id for that repository's hash format**, because git cannot resolve one to something newer at push time. Hex of the *other* width is an ordinary, movable ref name there — a 40-hex lease in a SHA-256 repository is exactly the hole `--force-with-lease` exists to close. **Route 1 — the payload's `cwd` was never read.** The probe ran `git rev-parse --show-object-format` from the **hook process's** directory. Claude Code launches hooks from the session root and runs the Bash tool wherever the session stands, so the two differ routinely. No wrapper and no `cd` were required: a plain `git push` was enough. **Route 2 — `env -S` / `--split-string` spliced options past the parser.** `-S` exists so a shebang line can pass OPTIONS to env (`#!/usr/bin/env -S -i prog`), so its split words are env's own arguments. `hook::git_resolve_index` spliced them back into its scan but resumed at the **command dispatcher**, which read a leading option in the split string as the command NAME and abandoned the segment. `env -S '-C <dir> git push --force'` resolved to *no git at all* — so this was not only a lease-width hole; a bare `env -S '-v git push --force'` also went unexamined. ## The fix - The payload's `.cwd` is read and replayed as a **leading `-C`**, ahead of `HOOK_GIT_RESOLVED_WRAPPER_DIRS`, which already precede git's own options. That reproduces execution order end to end and composes under git's own rules — a later `-C` composes onto an earlier one, an absolute one wins — so it is the same mechanism the wrapper replay already ships, with a first term added. Not a `cd`: a `cd` would move the hook process and leak across the recursive alias walk. - The base chain is `HOOK_EFFECTIVE_BASE` → `HOOK_CWD` → `CLAUDE_PROJECT_DIR` → `.`, adopted verbatim from `block-noncanonical-commit` rather than invented a second time. `HOOK_EFFECTIVE_BASE` is not decoration: a `!` shell alias runs its body as a fresh command in the relocated repository, so the base is relocated for that reparse and save/restored around it. This guard recurses through `!` aliases the same way the sibling does. - `hook::git_resolve_index` resumes inside **env's own option loop** after an `-S` splice. That also keeps env's single chdir slot last-wins across the splice (`env -C a -S '-C b git …'` lands in `b`), matching GNU env. - The `repo_oid_width` known-gap docblock is restated at its real width (see below). ## Behaviour change, stated so it is not read as a regression **A RELATIVE `-C` / `--git-dir` / `--work-tree` / `--namespace` now rebases onto the payload cwd** instead of the hook process's directory. That is the correct resolution — a relative path written in a tool call means relative to where that call runs — and it is a change only in the sense that the previous answer was measured from the wrong origin. An **absolute** one is unaffected. Cases 4b/4c below pin it, and there is a test for the absolute form staying put. One further consequence of adopting the sibling's chain: with **no `.cwd` in the payload at all**, `CLAUDE_PROJECT_DIR` is preferred over the hook process's directory. A real PreToolUse payload always carries `cwd`, and this matches `block-noncanonical-commit`; case 5b pins it either way. ## Verification Every row was run against **both trees from one script** — PRE is `origin/main` extracted verbatim, POST is this branch — over real SHA-1 and SHA-256 fixture repositories. Exit 2 = BLOCKED, 0 = ALLOWED. Two independent liveness columns, because a table can be inert in two different ways: - **pPOST** — the width the hook's own probe resolved, scraped from `bash -x` (`_repo_oid_width=NN`). The guard fails closed on width `0`, so a BLOCK from `0` is fail-closed noise, not the fix working. Every POST=BLOCKED row below resolved a real width. - **EXEC** — what the command's git *actually does*: the push replaced by `rev-parse --show-object-format`, the exact wrapper form run for real from the payload cwd. A form that never reaches git is not a bypass. | case | PRE | POST | pPRE | pPOST | EXEC | what it pins | |---|---|---|---|---|---|---| | 1a | 0 | **2** | 40 | 64 | sha256 | payload cwd = SHA-256 repo, hook process in SHA-1 one, 40-hex lease — **the bypass** | | 1b | 2 | 2 | 64 | 64 | sha256 | control: both directories agree; fixture discriminates | | 1c | **2** | **0** | 64 | 40 | sha1 | **opposite direction** — payload cwd = SHA-1 repo, 40-hex is a genuine object id where it runs | | 2a | 0 | **2** | – | 64 | sha256 | `env -S '-C <sha256> git …'` | | 2b | 0 | **2** | – | 64 | sha256 | `env --split-string='-C <sha256> git …'` | | 2c | 0 | **2** | – | – | sha1 | `env -S '-v git push --force'` — a plain force push hidden behind a leading option | | 2d | 2 | 2 | – | – | sha1 | no-regression: `env -S 'git push --force'` (no leading option) was and stays blocked | | 2e | 0 | **2** | – | 64 | sha256 | `env -C <sha1> -S '-C <sha256> …'` — one slot, last wins | | 2f | 0 | 0 | – | 40 | sha1 | `env -C <sha256> -S '-C <sha1> …'` — last wins the other way (semantics pin, paired with 2e) | | 3a | 0 | **2** | 40 | 64 | sha256 | `git -C <sha256> -c alias.y='!git <lease>' y` — the `!` body runs in the relocated repo | | 3b | **2** | **0** | 64 | 40 | sha1 | opposite direction through the same `!` path | | 4a | 2 | 2 | 64 | 64 | sha256 | relative `git -C` with both directories agreeing — unchanged | | 4b | **2** | **0** | 0 | 40 | sha1 | relative `git -C` resolves against the payload cwd (PRE probed width `0` — it was resolving nothing) | | 4c | **2** | **0** | 0 | 40 | sha1 | relative `--git-dir` rebases the same way — the disclosed change | | 5a | 2 | 2 | 64 | 64 | sha256 | no `.cwd`, no `CLAUDE_PROJECT_DIR` → `.` (pre-fix behaviour preserved) | | 5b | 2 | **0** | 64 | 40 | sha256 | no `.cwd` → `CLAUDE_PROJECT_DIR` (chain rung 2; EXEC differs because the divergence is synthetic) | | 6a | 0 | 0 | – | – | *(none)* | inert-form control: `env FOO=1 -C <dir> git …` — coreutils stops at `NAME=VALUE`, rc 127, git never runs, so there is nothing to block | `–` in a probe column means no probe ran (no lease expectation on that row, or no git resolved). **Every case that claims a fix carries a control that FAILS against `origin/main`**: 1a, 2a, 2b, 2c, 2e, 3a (PRE allowed, POST blocked) and 1c, 3b, 4b, 4c, 5b (PRE blocked, POST allowed). 1b, 2d, 4a, 5a and 6a answer the same on both trees by design and are labelled as controls, not as evidence. ### Regression coverage added - `plugins/guardrails/hooks/block-dangerous-git.test.sh` — 341 → **363 pass / 0 fail**. `run_in` now states the payload `cwd` alongside the process directory (without it the suite silently measures `CLAUDE_PROJECT_DIR`, i.e. the host repository, in any session that exports it); `run_split` and `run_nocwd` cover the divergent and degraded payload shapes. - `lib/hook-utils.test.sh` — **164 pass / 0 fail**, with resolver-level `env -S` cases including the attached-operand spelling, the last-wins slot across a splice, and a self-referential `env -S '-S -S'` termination check. ## Not in scope, deliberately - **A shell `cd` relocation** (`cd X && git push …`, `(cd X && …)`, `sh -c 'cd X && …'`). Resolving it means evaluating arbitrary shell word expansion, which this guard deliberately does not do. It remains a documented gap — and the docblock describing it is corrected in this PR, because it listed a "compound `cd`" as one of three required conjuncts when at the time **none** of them were required. A documented gap that reads narrower than it is, is how this one survived review. - **A persisted (config-file) alias carrying the lease** (`git config alias.yolo 'push --force-with-lease=…'` then `env -C <dir> git yolo`). This guard resolves inline `-c` aliases only; persisted-alias resolution is a separate capability `block-noncanonical-commit` has and this one does not. Flagged in #2124 for triage, not asserted there as a bypass. - **An explicit `--git-dir` / `--work-tree` inherited by a `!` shell-alias body.** git EXPORTS them into the body's environment (verified on git 2.54.0 — the body prints `sha256` from a SHA-1 directory and sees `GIT_DIR` set), so the body works in a repository the composed directory does not name. `effective_dir` composes `-C` only, so the lease is judged against the base. **Reproduced against BOTH `origin/main` and this branch (PRE=0, POST=0, EXEC=sha256)** — it is pre-existing and of the same family, not introduced here, and closing it means replaying the inherited globals rather than a directory: a larger mechanism than the base chain #2124's design section scopes this change to. Now documented in the `effective_dir` docblock and the CHANGELOG rather than left implicit, on the same principle that motivated the docblock correction above. - **The claimed relative-`git -C` misprobe that does not reproduce.** #2124 records it as tested against `origin/main` and not reproducing — the relative form resolves against the hook process's cwd *and* the command's cwd, which are the same directory in that scenario. It is subsumed by route 1, not separate, and no separate change was made for it. ## Two findings from adversarial review, folded in - **A false git semantic in the diff's own prose.** It said a `!` shell-alias body "starts in THIS segment's relocated directory". Measured: a `!` body runs from the repository **top level**, not the caller's directory (`alias.wd='!pwd'` from `<repo>/sub` prints `<repo>`). The conclusion is unchanged — an object format is a property of the repository, and the composed directory and its top level are the same repository — but the claim is corrected rather than left load-bearing on a wrong premise. - **An unexplained asymmetry that turned out to be correct.** `effective_dir` composes only `-C` while `collect_git_locating_opts` also replays `--git-dir`/`--work-tree`/`--namespace`. The reviewer expected a bug and found it right: only `-C` relocates a `!` body (`git -C <other> -c alias.wd='!pwd' wd` moves, `git --git-dir=<other> …` does not). A comment now says why, so the next reader does not file it as the bug this one nearly did. ## The known gap's primary symptom is a FALSE BLOCK, not a bypass Worth stating plainly because reviewers reasonably read "known gap" as "hole": with a shell `cd`, the probe measures a base that is frequently not a repository at all, answers width `0`, and fails closed. So ``` cd <repo> && git push --force-with-lease=main:<literal full-width sha> origin main -> BLOCKED ``` — the exact form the guard's own block message prescribes — is denied from a session root that is not itself a repository. Fail-closed is the right default for an unresolvable base, and this is not a regression (it behaves the same on `origin/main`), but the docblock now records the false block as the symptom to measure, because a guard that refuses correct usage it just recommended teaches people to route around it. Conversely, the fix **removes** a false block as well as a bypass: the inverse-skew row (hook process in SHA-256, payload cwd in SHA-1, 40-hex lease) goes DENY → ALLOW, which is correct because that word is a genuine object id where the command runs. ## What was NOT tested — carried forward rather than buried - **No PowerShell payloads were used by the adversarial pass at all.** The guard matches `Bash|PowerShell`, so the entire lease-width and `env -S` surface is unverified on that arm by the adversary. This branch adds PowerShell cases of its own (payload-cwd pinning plus a missing-`cwd` tool-name case) but they do not cover the `env -S` surface. - **`hook::require_jq` was not read**, and this guard now requests three payload fields instead of two. The behaviour when jq is absent — the guard skipping entirely — is a separate, already-filed concern, not something this branch changes. - The abbreviated-hex rows (7 and 12 hex) were examined and deliberately **not** "fixed": ambiguity with a short ref name is real, and blocking them is correct. - `+refspec` force detection held on every form tried; `-S` termination held across six degenerate operands under a 25 s timeout. - The 13/0 PRE-vs-POST discrimination split reproduced twice, but the final uncontended full pass was still running when the adversary reported. ## Blast radius `lib/hook-utils.sh` is a synced library: `scripts/sync-hook-utils.sh` distributes it to every plugin carrying `hooks/hook-utils.sh` — 16 plugin copies plus the `lib/` source, 17 files, all stale on `origin/main` — and each plugin must bump so consumers receive the change. All 16 carrying plugins are bumped with a CHANGELOG entry; `guardrails` takes a minor bump (0.23.1 → 0.24.0) for the behaviour change above, the other 15 take a patch. `scripts/sync-hook-utils.sh --check-bump origin/main` and `scripts/check-changelog-parity.sh --check-bump origin/main` both pass, as do `--check-order`, `check-silent-skips.sh` and `check-cross-plugin-source-drift.sh --check`. Closes #2124 ## Related - #1275 — where `PRRT_kwDOTCGFQM6TzGBZ` was filed - #2100 — the partial fix this completes, and the round-one verification that wrongly closed the thread - #1938 — the stranded post-merge review-findings sweep - #2120 — the previous `lib/hook-utils.sh` change, whose 15-plugin fan-out this one mirrors --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…git's own globals `effective_dir` scanned EVERY word of the command for `-C`. No `[git, subcommand)` slice, no wrapper replay -- the pre-#1785 shape, and the last un-migrated caller after #2100 finished the sibling in `block-dangerous-git`. It failed in the OPPOSITE direction from that sibling: not blind to a chdir, but inventing chdirs that were never there. `env -u -C git <alias>` moves nothing -- GNU env's `-u NAME` consumes `-C` as the variable to unset -- yet the every-word scan composed `<cwd>/git` and read that directory's aliases. The reachable consumer is the gitconfig alias lookup, which has neither a stdin-form gate nor an exemption gate and fails OPEN: the wrong repository's config silently misses the expansion, the guard never learns the subcommand is `commit`, and the convention goes unenforced. The sequencer probe at the same call site is corrected with it. `effective_dir` now takes git's own globals only -- the slice from the resolved git token to the subcommand -- preceded by any genuine wrapper chdir replayed from HOOK_GIT_RESOLVED_WRAPPER_DIRS, which is the one parser that can tell a real `env -C <dir>` from the `-C` in `env -u -C git`. Controls measured against origin/main before the fix, and they flip in OPPOSITE directions, which a single-direction fixture cannot fake: case pre post env -u -C git qc, alias in the TRUE repo 0 2 env -u -C git qc, alias in a DECOY <cwd>/git 2 0 git qs -C dec (post-subcommand -C) 2 0 env -C inner git qc (genuine wrapper chdir) 2 2 git qc (no wrapper; machinery liveness) 2 2 `git commit -C HEAD` is deliberately NOT the control. `-C` sets the reuse-message exemption and the hook returns before `effective_dir` is ever called, so that invocation answers "allowed" on both trees and reads as already fixed. The positional case runs through the alias lookup instead, with an alias ending in `--` so the appended `-C dec` cannot re-trigger the exemption in the recursed frame -- without that it answered 0 on both trees for an unrelated reason. HOOK_GIT_RESOLVED_WRAPPER_DIRS was printed and confirmed EMPTY for `env -u -C git`, so the lead control passes because the slice is right and not because the resolver invented a compensating wrapper dir. Suite: 37 pass, 0 fail; all 3 affected suites green. Closes #2113 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…git's own globals (#2152) ## What changed `block-convention-violation.sh`'s `effective_dir` scanned **every word** of the command for `-C` — no `[git, subcommand)` slice, no wrapper replay. That is the pre-#1785 shape, and this hook was the last un-migrated caller after #2100 finished the sibling in `block-dangerous-git.sh`. It failed in the **opposite direction** from that sibling. #2100's hole was blindness to a chdir that really happened. This one is indiscriminate: it invents chdirs that are not there. In `env -u -C git <alias> …` GNU env's `-u NAME` consumes `-C` as the *variable to unset*, so git never moves — yet the every-word scan composed `<cwd>/git` and read that directory's aliases. The reachable consumer is the **gitconfig alias lookup** (`:310`), which has neither a stdin-form gate nor an exemption gate, and which fails **open**: reading the wrong repository's config silently misses the expansion, so the guard never learns the real subcommand is `commit` and the team convention goes unenforced. The `sequencer_in_progress` probe at the same call site is corrected with it. `effective_dir` now receives git's own globals only — the slice from the resolved git token (`gi`) to the subcommand — preceded by any genuine wrapper chdir replayed from `HOOK_GIT_RESOLVED_WRAPPER_DIRS`. That resolver is the one parser able to tell a real `env -C <dir>` from the `-C` in `env -u -C git`. This is exactly the shape `block-noncanonical-commit.sh` and `block-dangerous-git.sh` already use; the docblock points at the sibling's rationale rather than restating it. ## Verification Every case was written as a standalone fixture and **run against a pristine `origin/main` worktree before the fix existed**, so both columns below are measured, not reasoned about. The controls flip in **opposite directions**, which a single-direction fixture cannot fake. | case | pre | post | expected | discriminates? | |---|---|---|---|---| | `env -u -C git qc`, alias in the **true** repo | 0 | **2** | 2 | yes — fails pre-fix | | `env -u -C git qc`, alias only in a **decoy** `<cwd>/git` | 2 | **0** | 0 | yes — fails pre-fix, opposite direction | | `git qs -C dec` (post-subcommand `-C`) | 2 | **0** | 0 | yes — fails pre-fix | | `env -C inner git qc` (genuine wrapper chdir) | 2 | 2 | 2 | no — must not regress | | `git qc` (no wrapper; machinery liveness) | 2 | 2 | 2 | no — proves the fixture is real | **Liveness.** The `git qc` baseline row is the proof the alias machinery is actually wired and the fixture repositories are real git repos with a real `.claude/source-control.md` — without it, a uniformly silent hook would read as "all controls pass". The decoy/true pair is the second liveness proof: the hook speaks when the alias sits in the composed directory and goes quiet when it does not, which can only happen if it genuinely read a repository rather than echoing the payload. **`git commit -C HEAD` is deliberately NOT the control**, per the issue's reachability section. `-C` sets `exempt=1` at `:343-345` and `:350-351` returns before `effective_dir` is ever called, so that invocation answers "allowed" on both trees and reads as already fixed. The positional case runs through the alias lookup instead, with an alias ending in `--` so the `-C dec` git appends to the expansion cannot re-trigger the reuse-message exemption in the recursed frame. Without that `--`, the case answered 0 on both trees for a reason unrelated to `effective_dir` — that first draft was caught and discarded. `HOOK_GIT_RESOLVED_WRAPPER_DIRS` was printed and confirmed **empty** for `env -u -C git`, so the lead control passes because the slice is right, not because the resolver invented a compensating wrapper directory. Suite: **37 pass, 0 fail**; all 3 affected suites green (`scripts/affected-tests.sh --run`). ## Scope A relative `-C` inside a `!`-shell-alias body still composes from the payload cwd here, because this hook has no `HOOK_EFFECTIVE_BASE` tracking the way `block-noncanonical-commit.sh` does. That is a distinct pre-existing gap, not this defect, and is deliberately left alone. ## Adversarial verification status A fresh-context adversarial verifier was spawned for this PR and did **not** return a verdict before the authoring session ended — the machine was saturated by concurrent agents and every spawned verifier stalled inside a long test sweep. Treat this PR as carrying the author's own evidence only. What partially substitutes for it, and why it is not nothing: the PRE column in the table above was produced by running the **shipped test file** against the **unmodified `origin/main` hook** in a pristine worktree, which is precisely the headline check such a verifier performs. What is still unverified by a second party is the "can you break it" attack surface and the payload-supply question called out below. Closes #2113 ## Related - #2100 — the sibling fix in `block-dangerous-git.sh`, and where this was found - #1785 — built the shared parser in `hooks/hook-utils.sh` and migrated the first caller - #2129 — the other guardrails defect from this sweep, shipped separately - Another agent is concurrently bumping `guardrails` for #2124; this branch bumps 0.23.1 → 0.24.0 and the second of the two to merge will need to re-bump. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
…ction to the lines the Edit wrote (#2153) ## Disposition: fixed, not closed won't-fix The issue offered three dispositions and flagged the defect as possibly payload-inherent. It is not, and that is the whole reason this ships as a fix. Partial-edit reconstruction separates an occurrence the call wrote from a coincidental one by requiring the anchor to occur exactly once. `replace_all` is precisely where that rule is suspended, on the reasoning that there every occurrence *is* the edit's own footprint. It is not: after `ghost` replaces `setup` everywhere, the `ghost` inside a pre-existing `ghost-old` matches the anchor too. The issue's premise — "nothing in the payload distinguishes a `ghost` this call wrote from the `ghost` inside a pre-existing `ghost-old`" — **holds for `tool_input` and fails for `tool_response`.** The Edit tool's structured output carries `structuredPatch`, which marks the lines the call actually wrote with a leading `+`. Under `replace_all` only, an occurrence is now kept just when its physical line is one the patch reports as written. The suspended uniqueness rule gets an external witness instead of nothing. Both halves were confirmed against pages **fetched 2026-08-10**, per the repo's fresh-docs mandate: - `PostToolUse` input "includes both `tool_input`, the arguments sent to the tool, and `tool_response`, the result it returned. The exact schema for both depends on the tool", and that field is "the tool's structured `Output` object" — <https://code.claude.com/docs/en/hooks>, "PostToolUse input". - `Output` for Edit is `FileEditOutput`, whose `structuredPatch` is `Array<{oldStart, oldLines, newStart, newLines, lines: string[]}>` — <https://code.claude.com/docs/en/agent-sdk/typescript>, "Edit". ## Why line TEXT and not line numbers Numbers are wrong the moment another PostToolUse hook reformats the file between the write and this read — the exact case the reconstruction fallback already exists for. And mapping a character offset back to a line number costs a whole-prefix scan per occurrence, which would reintroduce the quadratic term 0.21.0 spent a release removing. Text matching is a hash lookup and survives renumbering. Its one imprecision is conservative: an untouched line whose text duplicates an edited one is kept, so the filter can only ever drop findings the payload itself calls untouched. ## Deliberately inert outside its one case - A multi-line `new_string` is **not** filtered: its anchor extent spans several lines, matches no single patch line, and filtering would erase every finding rather than narrow them. - A payload with no `tool_response`, and every non-`replace_all` Edit, behaves exactly as before — the filter is inert by construction, not by a flag. ## Verification The table below was produced by running the **shipped test file** against the **unmodified `origin/main` hook** in a pristine worktree — not against a separate throwaway fixture. That distinction is load-bearing here; see the traps below. | assertion | pre | post | discriminates? | |---|---|---|---| | the WRITTEN reference is still reported | pass | pass | no — must not regress | | **the UNTOUCHED reference is not reported** | **FAIL** | **pass** | **yes** | | genuine multi-site: both refs survive the filter | pass | pass | no — proves no findings lost | | liveness: empty target yields nothing at all | pass | pass | no — see below | **Liveness**, using the technique the issue names: the identical payload is run against a truncated, empty target file. The hook is silent there, so every finding in the real fixture demonstrably came from **reading the file** rather than from the payload text. A filter that merely echoed `new_string` back would have spoken in both. Suite: **106 pass, 0 fail**. ## Two traps hit and fixed while building this Both are recorded because both produced a green assertion for the wrong reason, which is the failure mode this sweep exists to stop. 1. The first draft of the payload builder passed the diff lines as jq `--args` positionals. Every line starts with `-`, `+` or a space; jq parsed the leading `-` as an option and died on `Unknown option -u`. The payload came back empty, the hook went silent, and `assert_absent "the UNTOUCHED reference is not reported"` went **green** — while testing nothing at all. It was caught only because the paired `assert_contains` failed alongside it. Lines now reach jq on stdin. This is also why the PRE column above is measured with the shipped test file rather than the standalone fixture: the two artifacts had already diverged behaviorally once. 2. The positive needle `UNRESOLVED_SKILL: /alpha:ghost` is a substring of the `/alpha:ghost-old` line it exists to exclude, so it was sound only as long as the paired absence assertion stayed next to it. It now carries its own right boundary. ## Not verified A live PostToolUse payload carrying `structuredPatch` was **not** directly observed — no hook-event capture existed on the authoring machine to read. The schema is documented (above, fetched today) and corroborated by real Edit records in Claude Code's own transcript JSONL, which carry `originalFile` and a `structuredPatch` with `+`/`-`/` `-prefixed lines. If the field never arrives in a hook payload, the filter never engages and nothing regresses — but a reviewer with a hook-event capture could close this gap in one grep. ## Adversarial verification status A fresh-context adversarial verifier was spawned for this PR and did **not** return a verdict before the authoring session ended — the machine was saturated by concurrent agents and every spawned verifier stalled inside a long test sweep. Treat this PR as carrying the author's own evidence only. What partially substitutes for it, and why it is not nothing: the PRE column in the table above was produced by running the **shipped test file** against the **unmodified `origin/main` hook** in a pristine worktree, which is precisely the headline check such a verifier performs. What is still unverified by a second party is the "can you break it" attack surface and the payload-supply question called out below. Closes #2129 ## Related - #1319 — where the parent finding was filed - #2100 — closed the parent finding via span-overlap scoping; this is the residual it left - #1938 — the stranded post-merge review-findings sweep - #2113 — the other guardrails defect from this sweep, shipped separately - Another agent is concurrently bumping `guardrails` for #2124, and #2113's branch bumps to the same 0.24.0; whichever of the three merges later will need to re-bump. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 participant
No linked issue
Summary
Discharges six stranded bot review threads against the
guardrailsplugin, all filed on already-mergedPRs. One is a defeated security guard, the rest are
skill-reference-verifycorrectness andtimeout-budget defects. Four further threads raised on this PR are also addressed below.
Fix
block-dangerous-git— the hash-width probe ignored a wrapper's chdir (thread on #1275). A--force-with-leaseexpectation is judged immutable only when it is an object id of the hash width ofthe repository the push will run in.
collect_git_locating_optsreads only the slice between the gitword and the subcommand — as it must, since that walk cannot know which of
env's orsudo's optionstake a value — so a wrapper's relocation was invisible to it.
env -C <sha256-repo> git push --force-with-lease=main:<40-hex>therefore probed the invoking SHA-1 directory, read the 40-hex word asan object id, and allowed the push; where git actually runs that word is an ordinary movable ref name,
which is exactly the hole
--force-with-leaseexists to close.hook::git_resolve_indexalready recordsthe relocation in
HOOK_GIT_RESOLVED_WRAPPER_DIRS— the only parser that tells a realenv -C <dir>from the
-Cinenv -u -C git, which moves nothing — and the probe now replays those directories asleading
-Cwords so they compose ahead of git's own under git's rules rather than being modelled. Thismirrors the migration
848df9e9(#1785) made inblock-noncanonical-commit.skill-reference-verify— partial-Edit reconstruction (threads on #1319 and #1466, one span). Theold shape located the hunk by line and then filtered the whole physical line by word token. Three
defects, all that filter: an untouched broken reference sharing a line with the hunk was readmitted by
any word it happened to share; an Edit replacing fewer than four lowercase characters produced no token
at all, so every short-substring edit went uncovered; and locating spent two full-file
grepprocessesper hunk line, which a large Edit turned into the hook's 30s timeout. Reconstruction now keeps only the
inline-code spans whose extent OVERLAPS the located anchor. The occurrence-uniqueness gate is unchanged.
skill-reference-verify— the cost model behind the timeout fix was wrong, twice. Removing thesubprocesses left a per-line RESCAN, so the hunk is now located WHOLE — one scan for the whole edit,
producing the same span set, since a line anchor's extent is the text the edit wrote on that line and the
whole hunk's extent is the union of exactly those. Measuring the scan itself then contradicted the bound
placed on it: one scan is QUADRATIC in file size, not linear, because bash's
%%pattern strip walks thestring rather than indexing it. The previous 4 MiB file cap therefore allowed a single scan of roughly
eighteen minutes — the worst case had been moved off the per-line loop, not bounded. Both caps are now
set from the measured curve.
skill-reference-verify— manifest-declared skill paths (thread on #1319). Resolution hard-codedplugins/<plugin>/skills/. Per the Pluginsreference (fetched 2026-08-09),
skillsis astring|arraywhose paths ADD to the defaultskills/scan, a path may point straight at a directoryholding
SKILL.md, and a rootSKILL.mdwith noskills/and noskillskey auto-loads as asingle-skill plugin. All three now resolve. The documented marketplace-root exception is deliberately not
modelled and is recorded as such at the call site — leaving it out only ever suppresses an advisory,
never invents one. The advisory's own text carried the same hard-coded assumption and now lists the
directories the search actually covered.
Verification
Security defect, reproduced before and after against the same fixture tree (SHA-1 and SHA-256 repos),
hook cwd = the SHA-1 repo unless noted.
origin/main'sblock-dangerous-git.shvs this branch's:env -C <sha256> git push --force-with-lease=main:<40-hex>env -C <sha256> git push --force-with-lease=main:<64-hex>env -C <sha1> git push …:<64-hex>(cwd = sha256)env --chdir=<sha256> git push …:<40-hex>sudo -D <sha256> git push …:<40-hex>sudo --chdir=<sha256> git push …:<40-hex>bash -c 'env -C <sha256> git push …:<40-hex>'env -C <parent> git -C repo-sha256 …:<64-hex>env -u -C git push …:<40-hex>(-Cis-u's operand)The last row is the control that keeps the fix honest: an option that only looks like a chdir still moves
nothing, so the guard did not simply get stricter. Two rows flip BLOCKED → ALLOWED, which a fail-closed
regression could not produce.
Scan cost, measured rather than assumed. One
anchor_offsetsscan, isolated, Windows/Git Bash,quiescent, best of three:
That is ~0.065s × (KiB/32)² — quadratic. Those figures are a FLOOR, not the cost: they time an anchor
matching near the end, so one strip walks the file and the second is free, while a no-match strip walks
it twice (2.31s at 200 KiB) and the whole-hunk probe pays a scan before the fallback runs at all. So the
two bounds are calibrated end to end, not from the table:
RECONSTRUCT_MAX_CHARSis 128 KiB, and thefallback's anchor cap is
RECONSTRUCT_FALLBACK_SCAN_BUDGET / (KiB)²— 58 anchors at 32 KiB, 14 at 64,3 at 128. Above the file cap the direct hunk scan is untouched, so a complete reference is still
reported and only partial-edit recovery stops.
End-to-end, the shape the defect actually lived in (a hunk of distinct span-free lines, so the span
cap never binds and every anchor would rescan).
origin/mainvs this branch, same fixture:origin/mainBaseline per-invocation overhead on this host is 0.8–1s quiescent, so the branch numbers are the scan,
not the harness. An earlier revision of this PR reported far flatter pre-fix numbers; that benchmark used
hunk lines carrying inline code spans, which trip
RECONSTRUCT_MAX_SPANSand stop the loop after 40anchors — it measured the capped path, not the defect. The table above is the corrected measurement. A
4000-line row from that revision is dropped rather than restated: at ~156 KiB it now exceeds the file
cap, so it would time the skip path, not reconstruction.
Why the scale test asserts behavior instead of wall time. The new large-file fallback case pins the
cap from both sides — a reference inside the anchor cap is still reported, one past it is not — rather
than timing it. On this host the same fixture read 21s loaded and a smaller one 23s, against an isolated
scan of ~1s at that size; a timing assertion that noisy fails on load and passes on a regression that
happens to run on a quiet box. The scan cost is measured directly instead, in the constants' docblock.
Gates run from the worktree root, all green:
shellcheck -xon the four changed shell files;markdownlint-cli2on the changelog;check-changelog-parity.sh --check,--check-bump origin/main,--check-order;check-shell-portability.sh origin/main;sync-hook-utils.sh --checkand--check-bump;check-cross-plugin-source-drift.sh --check;validate-plugins.sh;check-changed-skills.sh origin/main. Contract suites:block-dangerous-git.test.sh341 pass / 0 fail;skill-reference-verify.test.sh96 pass / 0 fail (see also the CIplugin-gatejob, which runs both onLinux).
Four threads raised on this PR.
Xp-3r(quadratic rescan) andXqJ0e(nothing bounds the anchorcount) are both discharged by the whole-hunk locate plus
RECONSTRUCT_FALLBACK_SCAN_BUDGET; the suitefixture the first was measured against at 35s now runs in 0s.
XqJ1e(the advisory hard-codedplugins/<x>/skills/) is fixed and asserted on its full rendering, not a prefix.XrcOx(no testcombines a large file with the fallback path) is the case described above.
Not fixed here, flagged instead:
block-convention-violation.sh'seffective_dir(:186-201) scansevery word for
-C, with no[git, subcommand)slice and no wrapper replay — the pre-848df9e9shape,failing the opposite direction from the one fixed here. It accepts a
-Cthat moves nothing(
env -u -C git) and a-Cafter the subcommand (git commit -C HEAD, reuse-message), soeffective_dircan name the wrong repository. Different defect class, needs its own tests; not widenedinto this PR.
Related
block-dangerous-gitwrapper-chdir hash probe (the security defect)skill-reference-verifyreconstruction and manifest skill paths848df9e9) — the wrapper-chdir parser in the shared lib this fix consumes527dcd85) — already landed the hunk-line anchoring; a further thread on it needed no new fixa2d98f8a) — the siblingstale-path-verifyfix the reconstruction docblock cites