fix(guardrails): pin the reconstruction scan's locale so it stops decoding a literal search - #2127
Conversation
…oding a literal search 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. Measured on a quiescent Windows/Git Bash host (bash 5.3, 32 logical cores at ~18%), one no-match scan costs 0.054 s at 32 KiB / 0.221 s at 64 KiB / 0.880 s at 128 KiB under LC_ALL=C, against 0.395 s / 1.447 s / 5.786 s under en_US.UTF-8 — ~6.5x. The ratio is the finding; no wall-clock bound is claimed from it, and the caps the hook already carries are calibrated end to end rather than off that table. The pin is `local +x LC_ALL=C`, and the `+x` is load-bearing. A plain `local` inherits the export attribute whenever the consumer exported LC_ALL, pushing the pin into the grep/sed children. GNU `[[:space:]]` matches U+00A0 and U+3000 under a UTF-8 locale but not under C, so an exported pin makes the hook silently drop a reference whose argument separator is a non-ASCII space. That costs detection and buys nothing: the whole ~6.5x is bash's own matcher, and `grep -oE` over the same 64 KiB measured 0.139 s under BOTH locales. Un-exported, children keep the caller's locale exactly as before, and no offset ever crosses a process boundary — the fallback's grep yields anchor strings and emit_refs consumes a context string. Scope and lifetime were verified rather than assumed: assigning LC_ALL re-runs setlocale even for a `local +x`, and bash restores the previous value and its export attribute on return in all three caller states (unset, set, exported). Two regression cases. The multibyte-content case pins that byte offsets never split a character. It cannot discriminate the pin on its own — both locales are internally self-consistent, so it passes with and without one — so a second case covers the pin FORM, which is what can actually regress: with the consumer exporting a UTF-8 locale, a non-ASCII-separated reference must still be reported. Its separator is built from escapes rather than a literal byte, because a literal one was silently normalized to an ASCII space while the case was being written, which made an earlier draft vacuous. Also re-labels the constants docblock's cost curve as C-locale figures. They always matched the C column, 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. Re-labelled rather than re-measured, with a re-check on a second host of the same shape. Co-Authored-By: Claude Opus 5 <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. |
…-pin # Conflicts: # plugins/guardrails/.claude-plugin/plugin.json # plugins/guardrails/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 6m 51s —— View job Review complete
SummaryThis is a well-reasoned, well-documented fix. I read the full diff (
Two things worth a look (posted inline):
No correctness bugs found in the core locale-pinning logic itself. Verification limits, stated plainly: this sandboxed review session's Bash tool would not execute anything beyond read-only |
|
Claude finished @kyle-sexton's task in 1m 31s —— View job Security review
No security issues found. Scope reviewed (
No |
![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. |
…eable Two review findings, both real. The non-ASCII separator case is the ONLY one that separates `local +x LC_ALL=C` from a plain `local LC_ALL=C`; every other case in the file passes identically under both. Its `locale -a` guard could therefore retire the single regression guard this fix exists to add, silently, while the suite stayed green. The skip message now says so outright, and the comment records that CI's Ubuntu runners carry en_US.UTF-8, so the branch is expected to be DEAD there -- a skip in a CI log is the finding, not a shrug. It stays a skip rather than a hard failure only so a developer on a minimal host is not blocked by a locale they cannot generate. The MINOR bump also lacked the "Note on the version bump" rationale that the 0.22.1 entry established as the precedent. There IS an observable narrowing here -- RECONSTRUCT_MAX_CHARS is now read as bytes, so a large multibyte file that fit under the character cap can exceed the byte cap and skip reconstruction -- which is what puts this above a patch. Stated now instead of left to inference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 56s —— View job Security review complete
No security issues found. Scope reviewed (
No |
|
Claude finished @kyle-sexton's task in 2m 17s —— View job Review complete
SummaryThis review is scoped to what's new since the last automated pass: commit Both prior review findings are adequately addressed:
No new issues found. The Verification limits, stated plainly: this session's Bash tool required approval for any nested-shell invocation ( |
… lane CI's typos lane failed on the French multibyte fixture: it read the preposition "sur" as a truncated "sure". The prose is French only to get multibyte bytes cheaply -- the words carry no meaning for the assertion -- so the fixture is reworded rather than adding a global dictionary entry for a throwaway string. The comment explaining this deliberately does not spell the flagged word either, since the lane scans comments too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 10s —— View job Review complete
SummaryScoped this pass to what's new since the prior review rounds (which already covered
The commit swaps No correctness, architecture, or test-coverage issues in this commit — it's a pure fixture-wording fix with no logic change. Not independently verified: CI ( No new inline findings to post. |
|
Claude finished @kyle-sexton's task in 50s —— View job Security review complete
No security issues found. Scope reviewed (
No |
…rdcoding a libc's table The regression case added with the `+x` pin hardcoded U+00A0 as the argument separator whose classification differs between C and a UTF-8 locale. That is a property of the host C library, not of this hook, and it is not portable: glibc dropped U+00A0 and U+202F from `space` in 2.26 (a no-break space is deliberately not a separator), while Cygwin/MSYS still classifies them. So the case passed on Windows and failed on Linux CI — PASS=99 FAIL=1 — asserting a libc's table as if it were this hook's behavior. Measured on both platforms rather than reasoned about. `local +x LC_ALL=C` behaves IDENTICALLY on Git Bash (Cygwin 3.6.9, bash 5.3) and Linux (glibc 2.39, bash 5.2): it pins bash's own matcher to byte semantics and leaves the child environment at the caller's locale, restoring value and export attribute on return. That rules out a platform-specific `+x` semantic as the cause. What differs is only which codepoints `[[:space:]]` admits: separator MSYS glibc 2.39 U+00A0 NO-BREAK SPACE DIFFER SAME <- the bad fixture U+202F NARROW NBSP DIFFER SAME U+3000 IDEOGRAPHIC SPACE DIFFER DIFFER U+2000 / U+2028 / U+205F DIFFER DIFFER The pin itself is unchanged and its correctness argument is untouched. The case now DISCOVERS its separator: it takes the first candidate this host actually classifies differently between the two locales, probed through the very sed stage the assertion depends on. It cannot go vacuous (a candidate that does not discriminate is never selected) and cannot go platform-brittle (no codepoint is baked in). Both platforms select U+3000. If no candidate discriminates the case reports a loud, reasoned skip naming the platform rather than passing quietly. Verified: PASS=100 FAIL=0 on Windows/Git Bash AND on Linux/glibc 2.39, with the case executing rather than skipping on both. Mutated back to a plain `local` pin, it fails on Linux too (PASS=99 FAIL=1) — so it discriminates on glibc rather than merely passing there. Also corrects the hook docblock and the CHANGELOG, which both published the same unmeasured U+00A0 claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 49s —— View job Review complete
SummaryScoped this pass to what changed since the prior review rounds (which already covered What this commit fixes: the PR body notes the prior hardcoded U+00A0 separator passed on Windows/Cygwin (which still classifies U+00A0 as I traced the regex logic by hand rather than executing it (see verification limits below):
One minor nit posted inline: skill-reference-verify.test.sh:457 — No correctness issues found in this commit. The CHANGELOG update mirrors the code accurately (glibc version/codepoint claims match what's in the code comments), and no Verification limits, stated plainly: as with every prior automated pass on this PR, this session's Bash tool required approval for shell execution ( |
|
Claude finished @kyle-sexton's task in 51s —— View job Security review complete
No security issues found. This pass is scoped to what's new since the prior security review (which covered
No |
… mirrors SEP_SED is a byte-for-byte copy of emit_refs' reference-extraction sed at skill-reference-verify.sh:220, and nothing enforces that they stay identical. Noted in review. The comment now names the exact source line so a future editor of emit_refs knows to check here. Deliberately left as a copy rather than sourced: the probe must run the grammar in isolation to classify a candidate separator, and a drift cannot pass silently anyway -- the end-to-end assertion below runs the REAL hook, so a separator discovered against a stale copy fails assert_contains rather than quietly selecting the wrong codepoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 35s —— View job Security review complete
No security issues found. This pass is scoped to what's new since the prior security review (which covered through
No other files changed in this commit. No |
|
Claude finished @kyle-sexton's task in 48s —— View job Review complete
SummaryScoped this pass to what changed since the prior review round (
This is exactly the fix requested by the maintainability nit from the previous pass: This is a comment-only addition — no logic, test-assertion, or behavior change. No new issues found. Not independently verified: as with every prior pass on this PR, this session's Bash tool required approval for shell execution beyond read-only No new inline findings to post; all previously-raised findings on this PR have been addressed by the author across earlier commits ( |
1 participant
Summary
skill-reference-verify'sreconstruct_partial_editsearched in whatever locale the invokingshell happened to carry. Every search it performs is literal, but bash's
%%pattern stripdecodes 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):
LC_ALL=Cen_US.UTF-8The ambient environment on this host is
LANG=en_US.UTF-8withLC_ALLunset, so the hook reallydid run in the multibyte column.
Why
local +x, notlocalThe
+xis 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 -oEover the same 64 KiB measured 0.139 s under both locales. Soexporting the pin buys nothing — and it costs real behavior.
A plain
local LC_ALL=Cinherits the export attribute whenever the consumer exportedLC_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-8and a reference reachable only through reconstruction:Same
ctxbytes in both — the byte slicing is correct either way. The plainlocalform silentlydrops a real finding.
+xkeeps 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 commentnow says what was actually measured.
Correctness sub-claims, each verified rather than taken
LC_ALLre-runssetlocaleeven for alocal(and forlocal +x)${#}and%%both switch to byte semantics inside the functionLC_ALLis stilldeclare -xwith its original value after returnLC_ALLlocal; under+xthey never inherit it in any stateThe 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_refsgrammar is[a-z0-9-], so a non-ASCII name is unreportable by design) andthe 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
localpin makes it silent. Its separator isbuilt from
printf '\xc2\xa0'rather than a literal byte, because a literal one was silentlynormalized 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_CHARSis now read as bytes, the stricter reading — it cannot raise the ceiling itexists to set.
Docblock
Re-labelled, not re-measured. The constants docblock published
0.07 s at 32 KiB … 3.94 s at 256 KiBwith no locale named. Those figures match the C column almost exactly, but the hook did notthen 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 < 30on 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_refsscan (outside the reconstruction) still runs in the ambient locale, so its[[:space:]]breadth remains locale-dependent. Pinning the whole hook would change the grammar theguard reports on and needs its own justification, so it is out of scope here.
No linked issue
Related
and pins the locale for.