fix(hook-kit): document --json mode and scope WSCFG_* to hook scripts - #370
Merged
Conversation
The guard was registered, syntactically valid, and structurally unable to ever trigger: HG_HANDOFF_DELEGATE_PHRASES was never defined in the pattern data file, so it fell back to __NEVER_MATCH__ and Gate 1 exited 0 on every invocation. Its own docs cite it as the backstop for manual handoffs, which is precisely why nobody noticed it had never fired. Four defects, each of which alone kept it silent: - No English default. The data file's stated convention is that guards fall back to English-only patterns when no localized copy exists; this one fell back to a sentinel that matches nothing. - The localized file *replaced* the default rather than augmenting it, so installing a locale copy would have permanently disabled English matching. Now composed as EN plus the locale alternation. - Gates matched case-sensitively while the English patterns are lowercase — a handoff sentence normally starts with a capital, so every English phrasing slipped through. - Only text blocks were scanned. A handoff is most naturally delivered as an AskUserQuestion option, which is tool_use, not text; the payload is now included. Also drops Playwright-alone from the immunity set: its window is invisible, so stalling there teaches the user nothing and counting it as "already tried" turns a wrong-backend attempt into an alibi for the handoff. Verified against 10 synthetic transcripts (3 fire, 7 normal-sample no-fire), plus a locale-file-absent run confirming English-only operation.
The 17-case suite for check-session-rag.sh lived only in the dotfiles copy, so the copy that actually runs as the registered hook had no regression net of its own. Verifying a change to it meant assembling a throwaway harness that pointed a copied test file at this directory — which works, but is not something the next change will remember to do. The suite resolves its target relative to its own location, so dropping it in tests/ aims it at this checkout's resources/check-session-rag.sh with no wiring. Cases cover receiver configured/absent, MCP connected/down/unregistered, the endpoint-reachability fallback, and store/find credit for both the MCP tools and the skill's script route. 17/17 here.
…l newline The Completed section is regenerated from the entries collected during the walk, and only list items become entries. A line at section level that is not a list item therefore had nothing carrying it across the rebuild and disappeared on every run. The line this cost in practice was a provenance comment recording where deleted entry bodies had been moved, so losing it stranded the records it pointed to. Capture those lines and re-emit them at the top of the section. Second defect on the same write path: the output is assembled with a join, which leaves no terminator after the last line, so each run stripped the file's final newline and the next diff reported the last line as modified. Restore it when the source had one. Two regression tests cover both.
…red output-dir The Configuration table already lists walkthrough files among the artifacts the configured output-dir owns, but the Walkthrough Slug Policy section still named a wiki directory outright. The two read as contradictory guidance in one file, and the concrete effect was that a workspace could redirect its artifacts and still have walkthroughs leak into the wiki. Point the section at the same resolved output-dir as research and plan files, and state why a walkthrough is not a wiki write: it is a tool working note, and promotion goes through the knowledge-sharing ask, never a direct write.
Two rows in the resume topic ended with "Enforced by block-pr-url-gate.sh" - one for AskUserQuestion option text, one for TaskCreate subjects. The script is present but registered in none of the live hook configs, so neither row has a runtime backstop. Both rows now say so plainly, so the reader treats the URL/qualifier requirement as their own discipline rather than assuming a guard will catch a miss.
…orcing The pr topic row stated that block-pr-url-gate.sh is "registered in settings.json" and enforces per-PR URLs in AskUserQuestion payloads. The script exists on disk but appears in none of the 16 live hook configs across all 7 installed marketplaces, so it never fires. Asserting an enforcement that does not run is worse than asserting nothing: it tells the reader a backstop will catch a missing URL, which is exactly why the omission keeps reaching the user. State the rule as an authoring discipline and record the gate's actual status.
…lookup Research's Mandatory Corpus & RAG Pre-Lookup only checked past research/plan docs, wiki, and RAG stores — it never scanned the current session's own available-skills list for domain keywords. A session designed a new hook-monitoring subsystem from scratch without noticing skill-kit already owned hook trigger registration (its description literally says "trigger (declare + auto-register hooks)"). Add an explicit scan step so a domain-owning skill isn't missed just because its name doesn't obviously match the task.
warn-fixplan-item-schema.sh reported an item's Why/How as missing whenever the session edited only that item's header line. The advisory drew both WHICH items to inspect and WHETHER they satisfy the schema from the same source — the edit window — so sub-bullets below the cut were invisible. The existing anchor exemption could not cover this: it matches an old_string header verbatim, and a header-rewording edit changes that very line. Split the two questions. Candidate headers still come from the edited text, so items the session did not touch stay silent, but the schema check now runs against the file on disk, which PostToolUse guarantees is already written. The edited text remains the fallback when a header is not found there. Item spans now end at the last non-blank line, so the blank line separating two items no longer counts toward the 7-line budget. On disk a budget-sized item always has that neighbour; inside an edit window it did not. Adds tests/test-warn-fixplan-item-schema.sh — 10 cases: one reproducing the false positive, nine pinning the carve-outs ([x], [BLOCKED], non-tracker path, non-edit tool, insert-before-anchor) and the warnings that must survive.
…kills
hook-kit's own frontmatter describes it as a hook management toolkit —
audit / edit / install / move / remove. It is not a repository for every
hook. Two guards had accumulated there whose logic belongs to a single
other skill's domain:
block-squash-recommend-multi-commit.sh -> github-flow/resources/
Gates a PR merge-method recommendation. github-flow already owns
the PR-domain ask guard block-pr-url-gate.sh, and merge.md defines
the commit-count/distinctness rule this enforces.
block-new-skill-ready-without-ask.sh -> skill-kit/resources/
Gates readying a PR that introduces a new skill directory. Skill
publication scope is skill-kit's domain.
Placing them here was justified by precedent ("similar guards already live
in hook-kit") rather than by ownership — the reasoning the workspace notes
explicitly warn against, since it lets hook-kit accrete unrelated hooks
until nothing can be moved without touching it.
Neither script carries relative references, so this is a pure relocation
plus the two hooks.json path updates. Both self-tests still pass and bats
24/24 passes, including the check that every registered command path
resolves on disk.
hook-kit carried its own ask-guard.sh and block-axis-merged-ask.sh while the ask-user plugin carried the same two under the same names. Only the ask-user copies were ever registered, so every commit landing on these files ran nowhere — and the two lines kept diverging anyway. By the time this was noticed the unregistered copies were the NEWER of the two (2026-08-21 vs 08-20) and each side held fixes the other lacked. Both lines were merged into the registered copies first, so this removal drops no behaviour. Their three standalone test scripts move with them; no CI target referenced those, so nothing here loses coverage. The registry entries become tombstones rather than deletions — the hook-kit registry treats deletion records as data that survives regeneration, and a duplicate that silently never fired is exactly the history worth keeping. hook_registry_verify --check: 8 findings, unchanged from the pre-change baseline (the 2 SCHEMA findings this introduced are resolved by the tombstone blocks). bats 24/24.
…ulated-skills-fixes
DrumRobot
changed the base branch from
next-fix
to
fix/relocate-domain-owned-ask-hooks
August 26, 2026 17:19
DrumRobot
marked this pull request as ready for review
August 26, 2026 17:20
The resolver's -h/header only described --export, leaving --json undocumented (deferred review finding on the resolver PR). Document both modes and state the consumer-tier contract: WSCFG_* env vars are a hook-internal namespace (one parse, many reads in one shell process), while non-hook consumers (skills, python scripts, LLM instructions) read fields from --json directly — exported vars do not survive across harness Bash tool calls. Comment/help text only; no behavior change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe workspace configuration script documents the ChangesWorkspace configuration documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized documentation change does not introduce an actionable merge-blocking risk; it is merge-ready after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DrumRobot
changed the base branch from
fix/relocate-domain-owned-ask-hooks
to
next-fix
August 26, 2026 17:21
DrumRobot
changed the title
DrumRobot
force-pushed
the
fix/wscfg-json-scope-doc
branch
2 times, most recently
from
August 26, 2026 17:23
0cf9661 to
e7ae387
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Documents the resolver's
--jsonmode in its header/-houtput and states the consumer-tier scope contract settled by the WSCFG naming/necessity review:--export(WSCFG_* shell vars) is a hook-internal contract — one parse, many reads within a single shell process.--jsondirectly (workspace-config.sh --json | jq -r '.roles.artifacts.path') — exported env vars do not survive across harness Bash tool calls, so the indirection buys them nothing.Also resolves the deferred polish finding on the resolver PR (#345 AI Review Summary, finding 5:
-hlacked--jsondocumentation). Finding 4 (stderr warning for a missing explicitAGENT_WORKSPACE_CONFIG) is a behavior change and stays deferred.Comment/help text only; no behavior change.
-hsed range adjusted for the extended header.Test plan
workspace-config.sh -hprints the full extended header (40 lines)--jsonand--exportsmoke-tested, output unchangedtests/test-workspace-config.sh— 22/22 pass🤖 Generated with Claude Code
Summary by CodeRabbit
--exportand--jsonoutput modes.--helpto display the additional guidance.