fix(#461): fix --body parser truncation in require-agdr-for-arch-pr.sh#462
Conversation
…pr.sh with greedy awk The extract_flag_value() function used `sed … "[^"]*" …` which stopped at the first embedded double-quote inside --body, false-blocking any PR whose body contained a `"` before the AgDR reference or skip marker. Replace the three-branch sed form with the same greedy awk extractor already used by block-private-refs-in-public-repos.sh (fixed in #227) and validate-issue-structure.sh. The awk approach matches greedily to the last closing quote and uses a flag-boundary anchor (`[[:space:]]+--[a-zA-Z]` or EOS) to avoid bleeding into the next flag. Add three regression tests covering the ACs from #461: A) AgDR reference after embedded quote → PASS (no false-block) B) Skip marker after embedded quote → PASS with skip warning C) Embedded quote but no AgDR at all → still BLOCKS (true-negative) Sibling hook audit: block-private-refs-in-public-repos.sh and validate-issue-structure.sh both already carry the awk form; validate-pr-create.sh, require-migration-ticket.sh, require-active-ticket.sh, and verify-commit-refs.sh do not own a --body extractor and are unaffected. Refs #461 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #462
Commit: 9d5a754c7d2311fb199d8fc90b45fe67f73e84f6
Summary
Fixes false-BLOCKs in require-agdr-for-arch-pr.sh (#461). The old extract_flag_value() used a sed capture ("[^"]*") that truncated --body at the first embedded double-quote, so any AgDR reference / skip marker appearing after a quote was never seen → spurious block. The fix replaces the sed extractor with the greedy awk extractor already shipped in the sibling block-private-refs-in-public-repos.sh (#227): greedy (.*) anchored on the next flag boundary or end-of-string. Three regression tests added.
Checklist Results
- ✅ Architecture & Design: Pass — extractor lifted verbatim from the sibling hook; no new abstraction
- ✅ Code Quality: Pass — shellcheck green in CI; clear in-function rationale comment
- ✅ Testing: Pass (with a non-blocking coverage nit, below)
- ✅ Security: Pass — no over-correction; true-negative still blocks
- ✅ Performance: Pass
- ✅ PR Description & Glossary: Pass — narrative Summary, full Glossary, Testing checklist,
Refs #461 - ✅ Technical Decisions (AgDR): N/A — bug fix mirroring an established sibling-hook pattern, no new decision
- ✅ Adopter Handbooks: N/A — no handbooks triggered by this diff
Verification I ran
The new extractor is byte-identical to block-private-refs-in-public-repos.sh::extract_flag_value (lines 67–101 here vs 165–199 there) — confirmed by direct comparison.
I probed the extractor directly against the double-quoted body branch where the original bug actually lived (the regression tests only exercise the single-quote branch — see nit):
| Case | Input shape | Extracted | Result |
|---|---|---|---|
| Bug path | --body "… \"greedy\" … AgDR-0007-x" |
full body incl. AgDR | AgDR found → PASS (old sed would have truncated at first " → false-block) |
| True-negative | --body "… \"greedy\" … no record" |
full body, no AgDR | AgDR absent → BLOCK (gate not over-corrected) |
| Skip marker | --body "… \"pure\" … <!-- agdr: not-applicable -->" |
marker preserved | marker detected → bypass |
| Single-quote (test path) | --body '… "greedy" … AgDR-0007-x' |
full body incl. AgDR | AgDR found → PASS |
All three new [#461-A/B/C] cases pass. CI is green (Verify Ticket ID, shellcheck, site-counts all pass).
Note:
test_require_agdr_for_arch_pr.shshows oneFAILfor the pre-existingspike active-ticket markercase when run from an isolated clone outside an ops-root anchor — it fails identically on the base branch (d4d79fc) and is unaffected by this PR. CI (properly anchored) reports the suite green.
Issues Found
None blocking.
Suggestions
- nit (test coverage): The three
#461regression tests pass a single-quoted--body '...'containing embedded double-quotes. That exercises the awk single-quote branch — but the original[^"]*truncation bug lived in the double-quote branch (--body "..."with an embedded"). So the suite doesn't directly guard the exact path that broke. Non-blocking because: (a) I verified the double-quote branch independently above and it behaves correctly for pass/block/skip-marker; (b) the extractor is byte-identical to the already-tested sibling hook; (c) the double-quote shape is the common real-world form and is exercised in practice. Worth a follow-up: add one--body "...\"...\"... AgDR-..."double-quoted case so the precise regression is pinned in the suite. Agree with the prior reviewer that this is a nit, not a blocker.
Verdict
APPROVED
A non-blocking test-coverage nit does not gate this fix — the parser correction is verified correct across all three quote branches and is identical to the established sibling-hook implementation.
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 9d5a754c7d2311fb199d8fc90b45fe67f73e84f6
…pr.sh with greedy awk (#462) The extract_flag_value() function used `sed … "[^"]*" …` which stopped at the first embedded double-quote inside --body, false-blocking any PR whose body contained a `"` before the AgDR reference or skip marker. Replace the three-branch sed form with the same greedy awk extractor already used by block-private-refs-in-public-repos.sh (fixed in #227) and validate-issue-structure.sh. The awk approach matches greedily to the last closing quote and uses a flag-boundary anchor (`[[:space:]]+--[a-zA-Z]` or EOS) to avoid bleeding into the next flag. Add three regression tests covering the ACs from #461: A) AgDR reference after embedded quote → PASS (no false-block) B) Skip marker after embedded quote → PASS with skip warning C) Embedded quote but no AgDR at all → still BLOCKS (true-negative) Sibling hook audit: block-private-refs-in-public-repos.sh and validate-issue-structure.sh both already carry the awk form; validate-pr-create.sh, require-migration-ticket.sh, require-active-ticket.sh, and verify-commit-refs.sh do not own a --body extractor and are unaffected. Refs #461 Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
extract_flag_value()inrequire-agdr-for-arch-pr.shusedsed … "[^"]*" …which truncated the extracted body at the first embedded double-quote, causing the hook to never see an AgDR reference or skip marker that appeared after any quoted text in the PR body. Replaced with the greedyawkextractor already used byblock-private-refs-in-public-repos.sh([Bug] validate-issue-structure body extractor truncates --body "..." at the first embedded double-quote #227) andvalidate-issue-structure.sh, which anchors on the next flag boundary rather than the first internal".block-private-refs-in-public-repos.shandvalidate-issue-structure.shalready carry the fixedawkform;validate-pr-create.sh,require-migration-ticket.sh,require-active-ticket.sh, andverify-commit-refs.shdo not own a--bodyextractor and are unaffected.Testing
.claude/hooks/tests/test_require_agdr_for_arch_pr.sh— all existing cases pass, three new[#461-A/B/C]cases pass."but no AgDR reference still exits 2 ("no AgDR reference").--body-filepath is unchanged — hook reads the file directly, bypasses the extractor, unaffected by this fix.Refs #461
Glossary
extract_flag_value()--body "...") out of a rawgh pr createcommand string passed via JSON to the hook.[^"]*(sed form)"— stops at the first embedded quote inside the body, truncating everything after it.awkwith(.*)(greedy) plus a flag-boundary anchor ([[:space:]]+--[a-zA-Z]or EOS) to match the full body span to its closing quote, not to the first internal".--body-filepathgh pr create --body-file <path>— the hook reads the file directly and is not affected by the extractor bug; recommended for bodies with rich Markdown.