fix(diffanalysis): anchor sensitive-path keywords at both ends #88
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/sensitive-path-anchoring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
|binds looser than concatenation, so(^|/)auth|oauth|...parsed as((^|/)auth)|(oauth)|...— only the first branch was anchored to a path segment, and every other branch was a bare substring match anywhere in the path.Grouping the alternation fixes the leading end. That alone left the prefix case: with the boundary satisfied,
tokenstill matchedpkg/tokenizer/andchartmatcheddashboard/charts.go. A trailing([^a-zA-Z]|$)closes it.Observed before the fix:
internal/design/layout.gointernal/assignment/assign.goweb/static/signup.cssinternal/dashboard/charts.gopkg/tokenizer/lex.gocmd/immigration/main.goThese reach the model as flat assertions (
- Touches **security**: ...), so a false one aims the review at a threat model the diff has nothing to do with.The trailing boundary costs the ability to match derived forms implicitly, so they're spelled out (migration/migrations, encrypt/encrypted/encryption) — nothing distinguishes "tokens" from "tokenizer" by shape alone. Bare
signis dropped rather than enumerated, since signup/signal/assign swamped the code-signing sense;signingandsignaturecarry it now. Helm keys offhelm/,charts/, orChart.yamlspecifically.Tags are also deduped per file, since Helm now has three spellings and a path matching two of them would otherwise be listed twice under the same tag.
Tests cover both directions: the false positives above stay untagged, and real auth/crypto/migration/Helm paths still match at root and nested.
🤖 Generated with Claude Code
`|` binds looser than concatenation, so `(^|/)auth|oauth|...` parsed as `((^|/)auth)|(oauth)|...` — only the first branch was anchored to a path segment and the rest were bare substring matches. internal/design/ was tagged "security", cmd/immigration/ "database migration". Grouping the alternation fixes the leading end. That alone left the prefix case: with the boundary satisfied, "token" still matched pkg/tokenizer/ and "chart" matched dashboard/charts.go. A trailing ([^a-zA-Z]|$) closes it, at the cost of spelling out derived forms, since nothing distinguishes "tokens" from "tokenizer" by shape. Bare "sign" is dropped rather than enumerated — signup/signal/assign swamped the code-signing sense — with "signing" and "signature" left to carry it. Helm now keys off helm/, charts/, or Chart.yaml specifically. These tags reach the model as flat assertions ("Touches **security**"), so a false one aims the review at a threat model the diff has nothing to do with. Tags are also deduped per file, since Helm now has three spellings and a path matching two of them would be listed twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Automated review by pr-reviewer v0.41.3 | Safety Check | Claude Sonnet 5 | tracking id
r-6826be-6565abThis is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: LGTM — The PR is a targeted, well-tested regex bug fix with no security or stability concerns.
The change is a well-scoped regex fix: grouping the alternation and adding a trailing boundary correctly addresses the described false-positive bug, and the reasoning in the comments matches the code. Tests cover both the previously-false-positive paths and real matches at root/nested paths, plus dedup behavior.
Quick sanity checks on the regex logic:
(?i)(^|/)(migrate|migrates|migration|migrations)([^a-zA-Z]|$)etc. — grouping looks correct, trailing boundary excludes alphabetic continuations, which is the intent.helm,charts/,Chart.yaml) — reasonable, avoids over-matchingchartas prefix.seenTagsmap keyed by tag correctly prevents duplicate tags per file from multiple patterns.No secrets, no unrelated changes, no breaking API changes — this only affects internal tagging heuristics used for review commentary, not build/deploy logic. Nothing alarming.
d05342cde1a986b589dbAutomated review by pr-reviewer v0.41.5 | Safety Check | Claude Sonnet 5 | tracking id
r-6838c3-c7d95cThis is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: LGTM — The PR is a targeted, well-tested regex fix for a real precedence/prefix-matching bug in sensitive-path tagging, with no signs of secrets, breaking changes, or other issues.
This is a well-scoped regex fix with clear before/after reasoning and tests covering both the false-positive and true-positive directions. I checked the regex logic:
(auth|authn|...)inside(^|/)(...)([^a-zA-Z]|$)correctly fixes the precedence bug described.([^a-zA-Z]|$)correctly prevents prefix matches liketokenizermatchingtoken.seenTagsmap is straightforward and correctly prevents duplicate tags per file.helm/,charts/,Chart.yaml) are more precise and match the stated intent.No secrets, no committed binaries, no breaking API changes — this only affects internal tagging/heuristics used for review context, not core diff logic. I don't see any correctness issues with the regex changes as described.