Skip to content

🐛 Fix trailing boundary regex in verify-deliverables#58

Open
TechNickAI wants to merge 1 commit intomainfrom
fix/verify-deliverables-regex-boundary
Open

🐛 Fix trailing boundary regex in verify-deliverables#58
TechNickAI wants to merge 1 commit intomainfrom
fix/verify-deliverables-regex-boundary

Conversation

@TechNickAI
Copy link
Copy Markdown
Owner

Summary

  • Fixes space-terminated completion patterns (implemented , created the , wrote the , added the , saved the ) that were effectively dead code due to trailing boundary ([^a-zA-Z]|$) requiring a non-letter after the space
  • Splits regex into two groups: patterns needing trailing boundary vs space-terminated patterns where the space IS the boundary
  • Adds test case confirming "Implemented the fix in src/x.ts" now triggers advisory

Addresses review feedback from PR #54 (Claude review, Cursor Bugbot).

Test plan

  • All 20 tests pass (including new space-terminated pattern test)
  • "fixed itself" still correctly does NOT match "fixed it"
  • Existing boundary-needing patterns unchanged

🤖 Generated with Claude Code

The completion regex applied `([^a-zA-Z]|$)` uniformly to all
alternatives. For space-ending patterns like `implemented `,
`created the `, `wrote the `, `added the `, `saved the `, the
boundary required a non-letter after the trailing space — which
in English is almost always a letter (the next word). These five
patterns were effectively dead code.

Fix: split into two grep calls — one for patterns needing a
trailing boundary (e.g., "fixed it" vs "fixed itself") and one
for space-terminated patterns where the space IS the boundary.

Adds test case confirming "Implemented the fix in src/x.ts"
now correctly triggers the advisory.

Addresses review feedback from PR #54 (Claude review #2, Cursor).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@TechNickAI TechNickAI added review-sweep Automated review sweep fixes Hex Generated by Hex labels May 5, 2026
@claude
Copy link
Copy Markdown

claude Bot commented May 5, 2026

Code Review — PR #58

Overview

This PR correctly identifies and fixes a genuine regex bug: space-terminated patterns like implemented , created the , etc. were dead code because the trailing boundary ([^a-zA-Z]|$) required a non-letter after what was already a space — but in the most common phrase shapes (e.g., "Implemented the fix"), the character after the space is the start of the next word (a letter), so the boundary never matched. The split into two grep groups is a clean, readable fix.


Correctness ✅

The fix is sound. Key points that hold up:

  • (^|[^a-z]) with -i effectively becomes [^a-zA-Z] (grep's case-insensitive flag makes [a-z] match [a-zA-Z]), so "reimplemented" still won't trigger a false positive — the e before i is a letter and blocks the prefix.
  • Negation suppression (not implemented, didn't create, etc.) applies uniformly to both groups — the guard runs after either grep matches, so safety is unchanged.
  • The file-path token check ([a-zA-Z0-9_/-]+\.[a-zA-Z]{1,5}) still gates the advisory, keeping false-positive rate low.

Suggestions

1. Test coverage for the other four space-terminated patterns

Only implemented gets a new test. The four remaining patterns (created the, wrote the, added the, saved the) work by identical logic, but a regression in one of them wouldn't be caught. Consider adding cases — even a single parameterized loop over the pattern list would do:

for phrase in "created the" "wrote the" "added the" "saved the"; do
    TX=$(write_transcript "..." '...<text>'"$phrase"' solution in src/x.ts</text>...')
    run_case "$phrase triggers advisory" "$(make_payload "$TX")" "made no file changes"
done

2. Edge case: pattern at end of string without trailing word

What happens with "I implemented." (period, no following word)? The period isn't a space, so Group 2 (implemented ) misses it, and Group 1 doesn't include implemented\.. This may be intentional (keeping false-positive rate low), but worth a brief comment explaining the deliberate gap, since it could surprise a future maintainer.

3. Minor comment nit

The inline comment is accurate and helpful — no change needed. The PR description is also clear and well-structured.


Risks

Low. The change is confined to a single advisory hook that:

  • Always exits 0 (never blocks)
  • Has a kill switch (VERIFY_DELIVERABLES_DISABLED=1)
  • Has existing tests (20 cases, all passing per PR description)

The only risk is a new false positive from Group 2 lacking a trailing boundary, but that's mitigated by the other guards (zero-mutations check, negation filter, file-path token).


Summary

The fix is correct and well-motivated. The test addition validates the primary regression. The main gap is test coverage for the four non-implemented patterns — worth adding before merge to make the suite a reliable regression net.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Hex Generated by Hex review-sweep Automated review sweep fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant