fix: insight regex didn't match the real backtick-wrapped format - #307
Conversation
PR #303's hook never fired in production because the regex required '★' immediately after a newline, but explanatory-output-style wraps the decorator line in literal backticks (`★ Insight ─────`). Unit tests passed only because fixtures were hand-written without backticks and didn't model the real output format. - Regex: add optional leading backtick — `(?:^|\n)\`?★ Insight[ \t]+[─━═]{3,}` - Test fixtures: rewrite to use the actual backtick-wrapped format - New regression test: bare format (no backticks) still blocks, because the new regex makes the backtick optional rather than required Live-validated against this session's transcript: hook now correctly counts 3 insights and 1 publish in the current turn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Summary: Surgical fix for a regex that never matched real production output. The optional-backtick modification in enforce-insight-publish.sh:65 is correct, test fixtures are updated to mirror the actual backtick-wrapped format from explanatory-output-style, and the new regression test guards the bare (no-backtick) format.
Verdict: APPROVE - fix is correct and well-covered. One inline suggestion below, non-blocking.
Suggestion (tests/test-hooks.sh:1522): The comment "earlier versions of the regex required a backtick" is slightly misleading - no prior version of this regex ever required a backtick (pre-fix had no backtick support at all; this PR introduced the optional backtick). A clearer framing for the regression intent: "guards against a future tightening that makes the backtick required - the bare format must still match." The test itself is useful as-is; this is purely about the explanatory comment wording.
Automated review by Claude Code
No earlier version of the regex required a backtick — this PR introduced the optional-backtick support. Correct framing: the test guards against a future tightening that would make the backtick required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Feedback AddressedImplemented
|
There was a problem hiding this comment.
Code Review
Summary
Surgical regex fix: adds an optional leading backtick so the Stop hook matches the real explanatory-output-style output (backtick-wrapped decorator line), fixing the missed matches from PR #303. Test fixtures are updated to mirror real production output, and a new blocks_bare_format regression test keeps the bare (no-backtick) path covered so a future tightening of the regex would be caught.
Previously Addressed (Filtered)
- [Suggestion] tests/test-hooks.sh:1522 comment wording — rephrased to correctly describe the regression intent (guards against a future tightening, not "earlier versions required a backtick").
Verdict
APPROVE — fix is correct, tests mirror real output, and prior feedback was addressed. No new issues.
Automated review by Claude Code
…ism in hook-authoring (#309) * Add live-validate gate to /pr-create + fixture-realism nudge Two harness-level recall fixes for lessons memory alone failed to fire: 1. /pr-create: new step 4 gates on `git diff --name-only` matching home/.claude/{hooks,plugins,settings.json}. Surfaces a live-validation prompt before PR creation — for behavioral guards (hook/lint/policy that BLOCKS something), CI fixtures can't prove the guard is wired in and reachable. References the new memory. 2. hook-authoring SKILL.md: "Fixture realism" nudge in the Testing section. Sample real transcript content before writing regex; don't hand-write fixtures from a mental model. Both surfaced by improve-workflow reflection on the three-PR Stop-hook arc (#303 → #307 → #308) where each round shipped with insufficient empirical validation. Skill prose stays tight (~3-5 lines each) to respect the cost-per-invocation rule from PR #306. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address reviewer feedback on PR #309 - pr-create.md: replace prose trigger with deterministic `grep -qE` one-liner, and drop the dead `home/.claude/plugins/` arm (plugins aren't vendored in this repo; they're referenced via enabledPlugins in settings.json). - hook-authoring SKILL.md: fix dangling reference. The backtick case study lives inline in enforce-insight-publish.sh, not in hooks/README.md Gotchas (which covers jq multiline, PATH-stubbing, and the transcript race — not backticks). Point at the script with its inline comment. Verified the new regex via dry-run: matches `home/.claude/hooks/foo.sh` and `home/.claude/settings.json`, correctly does NOT match `home/.claude/skills/` or `home/.claude/commands/`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix backtick imbalance on fixture-realism line PR #309 round-2 review caught that my dangling-reference fix introduced an odd backtick count (9) on hook-authoring/SKILL.md:209 — the inlined regex contained a literal backtick that closed an inline code span prematurely. Drop the inlined regex from the prose entirely; the file pointer itself is the durable reference, and the script's inline comment has the regex + narrative anyway. Backtick count is now even (6, three pairs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
PR #303's enforce-insight-publish Stop hook never fired in production. My regex
(?:^|\n)★ Insight[ \t]+[─━═]{3,}expected★immediately after a newline, but theexplanatory-output-styleplugin wraps the decorator line in literal backticks:So the newline is followed by a backtick, not the star. Regex never matched. Unit tests in PR #303 passed because fixtures were hand-written without backticks and didn't model real output.
Fix
(?:^|\n)\?★ Insight[ \t]+[─━═]{3,}`blocks_bare_format— confirms bare (no-backtick) form still triggers, since the backtick is optional in the regexHow this got missed
Lesson captured as gotcha in event bus (event #4003): sample a real insight from the transcript before building regex fixtures.
Verification
Live-run against this session's real 1.4MB transcript:
Test plan
blocks_bare_formatregression test)make checkgreen🤖 Generated with Claude Code