docs: hook gotchas + improvement-shape guidance - #306
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Summary
Docs-only PR capturing two jq/PATH gotchas rediscovered in PR 303 and adding a one-line shape-selection rule to CLAUDE.md. Both claims verified against live code: the jq regex recommendation matches enforce-insight-publish.sh:61 (explicit newline alternation), and the PATH-stubbing pattern matches tests/test-hooks.sh:1354-1357. No runtime impact, zero risk.
Verdict
APPROVE — documentation is accurate and well-scoped. One suggestion below; non-blocking.
Inline Suggestion (home/.claude/hooks/README.md:276)
[Suggestion] The real test at tests/test-hooks.sh:1361 uses env -i PATH="$no_jq_dir" HOME="$HOME" bash ..., and the comment at test-hooks.sh:1350-1353 explicitly warns why: without env -i, an inherited PATH on Debian/Ubuntu CI runners can find /usr/bin/jq and mask regressions — the test then passes via an unrelated branch.
Since the whole point of this Gotchas section is preventing rediscovery, consider showing the env -i form (or at least a parenthetical note about CI environments where jq lives on /usr/bin). The simpler form in the snippet works locally but is exactly the shape that bit someone before.
Note: the first review on this PR (body "Review body test.") was an accidental probe while working around shell-escaping constraints; this is the intended review. Apologies for the noise.
Automated review by Claude Code
Feedback AddressedImplemented
|
There was a problem hiding this comment.
Code Review
Summary
Docs-only PR: adds a Gotchas subsection to home/.claude/hooks/README.md capturing two jq/PATH traps from PR #303, plus a one-line shape-selection rule in home/.claude/CLAUDE.md under Proactive Improvements. Both claims verified against current code — the jq regex recommendation matches home/.claude/hooks/enforce-insight-publish.sh:61 (explicit newline alternation), and the env -i PATH=... HOME=... snippet matches tests/test-hooks.sh:1354-1362.
Previously Addressed (Filtered)
- [Suggestion] Show
env -iform in the snippet — implemented in the current README text (line 276) with a one-line explanation on line 279. Not re-raised.
Verdict
APPROVE — documentation is accurate, well-scoped, and zero runtime risk. No new issues found.
Note: used gh pr review instead of gh api because no inline comments were needed and the gh api --input path was blocked by the sandbox's "brace with quote" guard when writing the review JSON file.
Automated review by Claude Code
- Adds "Gotchas" subsection to hooks README capturing two jq/PATH traps rediscovered during PR #303 (jq 1.8.1 multiline anchor quirk; cat+bash symlinks needed for no-jq PATH stubbing). - Extends CLAUDE.md Proactive Improvements with a one-liner on preferring zero-runtime-cost shapes (memory, event bus, docs) over skill-prompt edits for low-trigger-rate fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The no-jq test pattern in the Gotchas section omitted `env -i`, which is the exact mistake the section is supposed to prevent. Without env -i, inherited PATH on Linux CI runners can find /usr/bin/jq and mask the regression the test is meant to catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9eb7711 to
797ac88
Compare
Code ReviewSummaryDocs-only PR adding a Gotchas subsection to Previously Addressed (Filtered)
VerdictAPPROVE — documentation is accurate, well-scoped, and carries zero runtime risk. No new issues found. Note: the preceding empty-body APPROVE review was accidental — the sandbox guard blocked inline body-passing to both 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
scan("^..";"m")unreliable; usematch("(?:^|\n)..";"g")), and the cat+bash symlink requirement for no-jq test PATH stubbing. Future hook authors shouldn't rediscover these.Why
improve-workflow surfaced three findings after #301 merged. Finding #3 (hooks README additions) was the only one with favorable value-per-token — runtime cost is zero, benefit is concentrated on hook-authoring sessions. Findings #1 and #2 (skill edits) were deferred because they'd tax every invocation across all sessions for ~1% trigger rate. The CLAUDE.md one-liner encodes that shape-selection judgment itself.
Follow-ups from the same reflection that are not included here (intentionally):
/pr-createlive-validation checkpoint — already captured as memory + event-busimprovement_suggested. Revisit as standalone/live-validateskill if the pattern recurs./workenforcement-gap classifier — same disposition.Test plan
make checkpasses (29/29 bootstrap tests; no shell changes so nothing to re-lint)🤖 Generated with Claude Code