fix(plugins): make every declared option real, and document them where they live - #2193
Conversation
hook::check_enabled exits 0 when a plugin is gated off, which is correct for a hook -- no output means no action. It is wrong for any caller that must keep running after a "disabled" answer. The statusline tee is exactly that caller: it wraps the user's real statusline, so an exit would suppress the wrapped command's stdout and blank the status line rather than merely skipping the tee's own write. Split the resolution into a predicate and make check_enabled delegate to it; check_enabled is unchanged. Adds Test 20 covering both functions, including that an empty value is treated as unset -- Claude Code exports every declared option, so an option the user never answered arrives as an empty string and must mean "default", not "disabled" -- and that is_enabled returns rather than terminating its caller. Copies synced with scripts/sync-hook-utils.sh; CI rejects drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ne tee scripts/statusline-tee.sh is invoked by absolute path from the user's settings.json statusLine, not by the plugin hook runner, so it ran whatever the plugin enablement said. It was the one code path in this plugin that kept writing rate-limits.json while the plugin was disabled. Gate the snapshot on the plugin's own option via hook::is_enabled. The passthrough stays unconditional and byte-identical either way; check_enabled would have exited 0 and blanked the status line. If the shared library cannot be read the tee still runs, consistent with this script's rule that no tee outcome ever alters the wrapped statusline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hooks/worktree-create-gate.sh reads CLAUDE_PLUGIN_OPTION_WORKTREE_CREATE_GATE_ENABLED and names the option in its own skip message, but the option was never declared in the manifest. Claude Code exports CLAUDE_PLUGIN_OPTION_<KEY> only for declared options, so the variable was never set, the hook's :-true fallback always won, and the gate could not be turned off. Setting it produced no effect and no error. Declared with default true, so behaviour is unchanged for anyone who does not set it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A plugin's userConfig options were only discoverable by reading its manifest. The /plugin UI is awkward to browse, and 54 of 65 READMEs carried a hand-written Configuration section whose completeness nothing enforced -- an option added later was silently undocumented. Generate an options-reference block into each plugin README from its own manifest: every option with its type, default, the CLAUDE_PLUGIN_OPTION_<KEY> environment variable each hook reads it from, and the three supported ways to set it (the /plugin configure prompt, claude plugin install --config, and a pluginConfigs entry in user settings), plus links to the upstream docs. Notes the trap that plugin option values are read from user, --settings and managed settings only, never from a project's .claude/settings.json. The block is marker-fenced and generated, so hand-written prose is preserved untouched -- actionlint's account of its stdin timeout is nuance a generator cannot produce. The generator guarantees completeness and freshness; the prose keeps the nuance. Both live in the plugin's own folder so they change together. Same contract as the shared hook library: the manifest is the single source of truth, the block is never hand-edited, and CI runs --check and rejects drift. The gate is wired into the ci-status needs graph, so it is required rather than informational. 32 plugins declare 130 options. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 24s —— View job Security review complete
No security issues found. This PR is documentation/gating plumbing, and the changes are narrowly scoped to what the description claims:
No |
|
Claude finished @kyle-sexton's task in 4m 16s —— View job Review complete
SummaryReviewed the diff against
Not independently verified (author-claimed in the PR description, not reproducible by me in this sandbox — No correctness bugs, security issues, or architecture concerns found beyond the one inline finding, which is a docs-completeness gap rather than a functional defect — the gating logic itself (the PR's actual bug fixes) is sound and matches the CLAUDE.md design rules (manifest as single source of truth, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce2d500a39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Four gate failures on the first push, all legitimate repo doctrine: - hook-utils-sync: every plugin carrying a copy of the shared library must bump its version when the library changes, so consumers receive it. Bumps the 14 carrying plugins with a CHANGELOG entry each. - plugin-gate: plugins/github/github.test.sh enforces marketplace agnosticism, and the generated install example hardcoded a marketplace name. The generator now emits a <marketplace> placeholder with an instruction to substitute it, which is also more correct: a plugin can be installed from a fork, a mirror, or a private catalog under a different name. - hygiene/shellcheck: SC2016 on the two `bash -c 'source "$0"'` probes, where non-expansion is the point -- $0 is the child shell's positional, bound to the library path passed after the -c string. Directives added with that rationale. - hygiene/exec-bit: the new generator needed the exec bit its siblings carry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 4s —— View job Review complete
Per Findings1. (Important, inline on 2. (Suggestion, unresolved from earlier review — not re-filed inline to avoid duplicating the existing thread) 3. (Suggestion, also flagged by the Codex review above, still unresolved) 4. (Nit) What checked out
Not independently verifiedThe PR description's test-suite/lint/validator numbers (180/0, |
|
Claude finished @kyle-sexton's task in 2m 53s —— View job Security review complete
One finding, in the logic/trust-boundary lane this pass is scoped to (zizmor already covers the Actions supply-chain/permissions surface, and I found nothing there worth adding — the new IMPORTANT — the new statusline-tee kill switch almost certainly does not fire on the real invocation path (confidence: high)
I independently checked the current official docs (fetched this session, not from training-data recall):
So the statusline process almost certainly never sees Net effect: a user who sets I was not able to spin up a real Claude Code session with a wired No other logic, authorization, injection, secret-handling, or GitHub Actions trust-boundary issues found in this PR's changed files. |
The repo's typos gate flags UNPARSEABLE; UNPARSABLE is the accepted spelling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 52s —— View job Review complete
Note on scope: I couldn't run Findings (posted inline)This PR's two newest commits (
Minor, not filed inline: What checked out
Not independently verified (author-claimed only): the PR description's and CHANGELOG's test-suite/lint/validator output ( |
|
Claude finished @kyle-sexton's task in 2m 9s —— View job Security review complete
Scoped to logic/architecture/trust-boundary security reasoning, per this repo's lane split — Actions supply-chain/permissions/trigger risk is zizmor's lane and I found nothing there worth adding (the new job is a plain IMPORTANT — the statusline-tee kill switch still appears to be a no-op on the real delivery path (confidence: high)
I checked this repo's own delivery-channel doc, Given This is a functional/access-control gap (a documented user-facing toggle that silently doesn't take effect for the write it's supposed to suppress — plausibly used for privacy, since it's what should stop rate-limit/session data being written to disk on every statusline refresh) rather than a memory-safety or injection bug, which is why it's flagged here as IMPORTANT/high-confidence rather than CRITICAL — no secrets or authorization boundary crosses, but the plugin's own stated contract (an "off" switch that's actually off) doesn't hold. I can't spin up a real Claude Code session with a wired (Same finding as the prior review passes on this PR — re-verified independently against the current diff and unresolved as of this HEAD, so repeating it rather than treating it as stale.) What checked out clean
No injection surfaces (command/SQL/path/template), no secret-handling issues, and no GitHub Actions trust-boundary problems found in this PR's changed files, beyond the access-control gap above. |
|
Claude finished @kyle-sexton's task in 1m 29s —— View job Security review complete
Re-verified against the current HEAD ( IMPORTANT — statusline-tee's new kill switch is very likely a no-op on its real invocation path (confidence: high, unresolved from prior rounds)
This repo's own If that scoping holds for This is a functional/access-control gap — a user-facing toggle silently not taking effect for the write it claims to suppress — rather than a memory-safety or injection bug; flagged IMPORTANT/high-confidence given the repeated documentary confirmation across review rounds, not CRITICAL, since no secrets or authorization boundary is crossed. Worth a maintainer confirming with an What checked out clean
No injection surfaces (command/path/template), no secret-handling issues, and no GitHub Actions trust-boundary problems found beyond the access-control gap above, which is unchanged from the prior review rounds on this PR. |
|
Claude finished @kyle-sexton's task in 2m 22s —— View job Review complete
This is a re-review triggered by a new push, but that push (
What checked out
Not independently verified (author-claimed only, unchanged from prior passes): the PR description's test-suite/lint/validator output ( |
Three defects the automated reviewers caught, all real. The tee's kill switch was decorative. CLAUDE_PLUGIN_OPTION_<KEY> is exported to hook processes only, and statusline-tee.sh is invoked by absolute path from the user's statusLine setting -- so the variable was always unset, the :-true fallback always won, and the gate never fired. It passed review only because the tests injected the variable by hand. This is the same class of defect this PR fixes in source-control: a gate reading a channel that never delivers. The tee now reads the value from pluginConfigs in user settings, the sanctioned route for a non-hook consumer, matching the marketplace by prefix so forks work, and failing open on every error path. Also NOT `// empty` in the jq: the alternative operator treats false as falsy, which would discard the exact value the gate exists to detect. Caught by test. The generator dropped multiple/required/min/max, so a repeated option rendered as a plain scalar -- visibly contradicting source-control's own hand-written prose in the same file, which documents 10 of them as "string (multiple)". The generator also left a stale block when a plugin removed its last option, and --check reported it as up to date because it returned before reading the README -- the one path where the gate silently failed at its own job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… feat/plugin-enable-toggles
|
Claude finished @kyle-sexton's task in 3m 42s —— View job Review complete
This lane is scoped to correctness/design/tests/docs; security reasoning is left to the dedicated security lane, which has already reviewed this PR extensively above. This is a re-review triggered by the new commit What's now fixed
Still open: the statusline-tee kill switch, now on its third implementation, has a narrower but real gap (posted inline)The rewrite correctly diagnoses and fixes the root cause every prior round raised —
Nit (not filed inline)
Not independently verifiedThe PR description's/CHANGELOG's test-suite and lint output ( |
|
Claude finished @kyle-sexton's task in 3m 2s —— View job Security review complete
Scoped to logic/access-control/trust-boundary reasoning per this repo's review-lane split; zizmor's static coverage (unpinned actions, dangerous triggers, permission widening) isn't re-litigated here, and this push doesn't touch IMPORTANT — the new gate skips managed settings, so the precedence guarantee it's supposed to honor doesn't hold (confidence: high)
This commit correctly fixes the previously-flagged defect for the common case: it stops relying solely on But channel F's own definition is: "the hook script reads
This isn't a hypothetical edge case in the same way as the delivery-channel-existence question the last several rounds were debating — it's directly readable from the diff: no Suggested fix direction: add the fixed per-platform managed-settings path(s) (and What checked out clean in this push
|
The 0.5.6 gate read only the user settings file, so a managed policy setting rate_limit_guard_enabled: false was ignored — a policy bypass, since managed settings are the highest-precedence scope and cannot be overridden by any user or project scope (hook-config-delivery fact 5). _rlg_tee_enabled now resolves managed → user settings → environment, highest first, mirroring the channel-F exemplars: the fixed per-platform root-owned managed paths selected by uname -s plus the managed-settings.d/ drop-ins, every resolved path re-checked as absolute. The env channel drops below user settings because a repo env block owns an unconfigured key (fact 4). Fails open on a missing file, missing jq, malformed JSON, or an unrecognized platform; prefix marketplace match retained; no `// empty` on the value. statusline-tee.test.sh gains real coverage of the gate — unconfigured, user false/true, fork marketplace, another plugin's identically-named option, malformed JSON, missing jq, and managed false-over-true plus the managed true-over-false mirror — each asserting the wrapped statusline's stdout is unchanged, plus one unstubbed end-to-end case. The script gained a main() behind the repo's existing BASH_SOURCE sourcing guard so the unwritable managed path list can be stubbed; a direct run is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 14s —— View job Security review complete
This pass is scoped to the delta since the last review round ( No security issues found in this delta — it resolves the IMPORTANT finding raised repeatedly in earlier review rounds. Previously flagged: statusline-tee kill switch didn't honor managed settings — now fixedThe last review round found
Previously flagged: no test exercised the gate — now fixed
What checked out clean
|
The manifest's option description and the README's Configuration section still called rate_limit_guard_enabled the kill switch for the StopFailure hook alone, and the README told operators that disabling the statusline tee was their own settings edit — true before 0.5.5 gated the tee's write on the same option, wrong since. Both now name the hook and the tee's snapshot write, and the README states where each surface reads the value from and that the tee's precedence is managed → user settings → environment, so a managed value outranking a user one is explicable rather than surprising. README options block regenerated by scripts/sync-plugin-options-docs.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 47s —— View job Security review complete
No security issues found. This pass re-verifies the whole PR against the current HEAD by reading the files directly (not just trusting prior review rounds' conclusions), scoped to logic/access-control/trust-boundary reasoning per this repo's lane split — zizmor's static coverage (unpinned actions, dangerous triggers, permission widening) isn't re-litigated here. What's new since the last reviewed commit ( Full-PR re-verification, independent of the prior rounds' conclusions:
No injection surfaces (command/path/template), no secret-handling issues, and no GitHub Actions trust-boundary problems found in this PR's changed files as of the current HEAD. |
… and its opt-out honest (#2309) ## Summary Two issues in one file pair, both about what a stuck user actually sees when a `WorktreeCreate` fails. **#2209 — the failure output.** `status=$?` sat inside the body of `if ! path="$(bash "$helper" …)"`, where `$?` is the status of the *negated compound* — `0` exactly when the command failed. Every failure therefore reported `exited 0`. Proven directly: ``` $ bash -c 'f(){ return 7; }; if ! out="$(f)"; then status=$?; echo "status=$status"; fi' status=0 ``` That constant is what produced the "a hook exited 0 while failing" theory a whole verification pass had to unwind. The assignment now stands alone, and the helper's documented `0/2/3/4` taxonomy is translated into distinct messages instead of being discarded — "not a repository", "no `worktree_root` configured" and "illegal branch name" were previously one indistinguishable line. Every refusal now leads with a **remedy** and follows with the diagnosis. `hook::buffer_stdin`'s status is checked, so an empty/unbufferable payload is no longer misreported as "the payload carried no `.name`". **Corrected mechanism.** #2209 was filed on the premise that the transcript surfaces only the **first** stderr line. Measured on 2.1.228, a failing hook's stderr is surfaced **in full**: ``` Error creating worktree: WorktreeCreate hook failed: bash "…/fail-hook.sh": FIRST-STDERR-LINE SECOND-STDERR-LINE ``` Remedy-first still holds — it is the line a reader acts on — but as a readability argument, not a truncation one. The CHANGELOG states the corrected version. **#2211 — the opt-out asserted an unmeasured harness behavior, and it was false.** The issue said one `claude --worktree` in a scratch repo would settle it. It was run — four arms, on Claude Code **2.1.228**: | Arm | Hook | Result | |---|---|---| | 1 | *(none — control)* | created at `<repo>/.claude/worktrees/probe0`, branch `worktree-probe0`, **locked** (`claude session probe0 (pid 29884)`) | | 2 | `exit 0`, no stdout | **creation FAILS**, CLI exit 1, nothing created | | 3 | `exit 3`, two stderr lines | creation fails; **both** lines surfaced | | 4 | prints a path it did not create | creation fails — the directory must exist | ``` $ claude -p '…' --worktree probe1 --settings <hook: exit 0, no stdout> Error creating worktree: WorktreeCreate hook failed: hook succeeded but returned no worktree path (command: echo the path to stdout; http/callback: return hookSpecificOutput.worktreePath) $ git worktree list # nothing created ``` So `worktree-create-gate.sh:46-47`'s header ("Disabled means 'let Claude Code use its own default', which is exit 0 with an empty stdout") and the two suite assertions at `:131-132` were **both false**. The old exit-0 path produced the *same* outcome as a refusal while suppressing every explanation, because an exit-0 hook's stderr is dropped — arm 2's stderr marker is absent from the harness output while arm 3's two lines are present. The option only became reachable at 0.51.7 (#2193 declared it in `userConfig`), so this is the first release anyone could hit it. The disabled path now **exits non-zero** and names the real stand-downs: `worktree.bgIsolation: "none"`, or disabling the plugin. `plugin.json` and `README.md` say the same. **The docs also agree now**, contrary to what #2211 recorded. Raw-markdown fetch of `https://code.claude.com/docs/en/hooks.md` (2026-08-11) carries "Hook failure or **missing path** fails creation" and "If the hook fails or produces **no path**, worktree creation fails with an error." #2211 recorded those sentences as unreachable; at the current revision they are reachable and they match the measurement. **The probe is now a recorded, runnable fixture** — `skills/worktree/fixtures/worktree-create-hook-probe.sh` plus a `README.md` carrying the outcome, verbatim harness strings, doc quotes, an as-of stamp (2026-08-11, 2.1.228) and a recheck trigger, composed with `docs/conventions/upstream-drift/`. A recheck is one command, not a re-derivation. **Not changed, deliberately: `hooks.json` registration scope (#2211's `SC-F1`).** `WorktreeCreate` supports no matcher — "no matcher support / always fires on every occurrence" — so registration is the only seam and there is nothing narrower to register. Per AD-4 the non-repo outcome is upstream (the harness skips isolation and edits in place), and the issue itself notes a change here would not be fleet-complete since other installed plugins may register the same event. Filed-not-fixed with that reason, not dropped. **Also fixed, in files already being edited:** both worktree suites set a throwaway git identity but not `commit.gpgsign false`, so on a machine that signs by default every fixture commit failed and the suites reported their *creation* cases as failures while their refusal cases still passed — a shape that reads as a regression rather than an unrunnable fixture. `worktree-create.test.sh` goes 94/154 → 154/154. **Security review:** no new trust surface. No new hook, grant, network read, or file write outside existing paths. The change is strictly message text plus one exit-status correction, and it moves the disabled path from exit 0 to exit 1 — i.e. **more** fail-closed, never less. The new fixture script is opt-in and run by hand. ## Test plan New assertions fail against the pre-change code and pass after — verified by checking the merge-base (`a0abaf81`) copies of both scripts back in and re-running: ``` $ bash plugins/source-control/hooks/worktree-create-gate.test.sh # OLD code, NEW tests FAIL: [16] disabled refuses non-zero, because exit 0 without a path fails creation anyway — exit expected 1 got 0 FAIL: [20] disabled names the real harness-side stand-down — expected worktree.bgIsolation in: worktree-create-gate: disabled by worktree_create_gate_enabled=false; Claude Code will place this worktree at its own default, which may be inside the repository FAIL: [21] disabled states plainly that the option cannot hand placement back FAIL: [22] the remedy leads — a reader acts on the first line, so it must not be the diagnosis FAIL: [23] a non-repository is named as such, not as an opaque exit code FAIL: [24] a non-repository names the harness-side stand-down as the remedy FAIL: [25] the constant-zero exit status is gone — forbidden "exited 0" present FAIL: [28] an illegal branch name is reported as a name problem FAIL: [31] an empty payload is reported as an empty payload — got: "the WorktreeCreate payload carried no .name" FAIL: [33] a nameless payload is reported as a nameless payload ``` After the change: ``` $ bash plugins/source-control/hooks/worktree-create-gate.test.sh PASS: [1] a configured root creates the worktree … (34 cases) PASS: [22] the remedy leads — a reader acts on the first line, so it must not be the diagnosis PASS: [23] a non-repository is named as such, not as an opaque exit code PASS: [25] the constant-zero exit status is gone PASS: [30] an empty payload refuses PASS: [32] an empty payload is NOT misreported as a missing .name field PASS: [34] field order does not matter # 34 PASS, 0 FAIL, rc 0 $ bash plugins/source-control/scripts/worktree-create.test.sh PASS=154 FAIL=0 SKIP=0 # rc 0 (was 94/154 on this machine before the gpgsign fix) ``` Lint and repo hygiene: ``` $ shellcheck -x -f gcc <the four touched shell files> # rc 0, no findings $ npx markdownlint-cli2 <the three touched markdown files> Summary: 0 issues in 0 files $ bash scripts/check-silent-skips.sh No silent prerequisite skips found in hook entry scripts. $ bash scripts/check-orphaned-fixtures.sh # rc 0 $ bash scripts/check-changelog-parity.sh --check # rc 0 $ bash scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. $ bash scripts/check-changelog-parity.sh --check-order # rc 0 ``` The `silent-skip-ok:` marker was removed along with the exit-0 skip it justified; `check-silent-skips.sh` confirms nothing now needs one. ## Related Closes #2209 Closes #2211 Inbox item: `20260811-021645-plugin-audit-four-components-and-guard-deadlock-ownership` (ledger `.work/handoff-inbox-batch-4/ledgers/I9-021645-four-components.md`, § "Lane D — `source-control` / `worktree-create-gate`"). Settles **DQ-6** from `.work/handoff-inbox-batch-4/RECONCILE.md`: the one `claude --worktree` command was run, and exit-0-with-no-path fails creation. Adjacent, not touched here: #2146 (`hook::require_jq` fail-open in the same utility file — different function), #1971 (upstream coverage of nested-worktree rule inheritance), #2208 (the upstream draft for the non-repo isolation divergence). Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #2192
Related
What this changes
Three defects, plus the convention that stops the first one recurring.
hook::is_enabled— a non-exiting predicatehook::check_enabledexits 0 when a plugin is gated off. That is right for a hook, and wrong forany caller that must keep running afterward. The statusline tee is a transparent wrapper around
the user's real statusline, so an exit there would blank the status line instead of skipping the
tee's own write.
check_enablednow delegates to the predicate and is otherwise unchanged.rate-limit-guard— the tee now honors its own optionInvoked by absolute path from
settings.jsonstatusLine, the tee ran whatever the plugin'senablement said. It was the only code path in the marketplace that kept executing while its plugin
was disabled. The snapshot is now gated; the passthrough stays unconditional and byte-identical.
source-control— a gate that could not be turned offworktree_create_gate_enabledwas read by the hook but never declared in the manifest, so ClaudeCode never exported the variable and the
:-truefallback always won. Declared withdefault: true, so behavior is unchanged for anyone who does not set it.Per-plugin options reference, generated and gated
Each plugin README gains a marker-fenced block generated from its own manifest: every option with
its type, default, the
CLAUDE_PLUGIN_OPTION_<KEY>variable each hook reads it from, the threesupported ways to set it, and links to the upstream docs. It also records the trap that plugin
option values are read from user,
--settingsand managed settings only — never from aproject's
.claude/settings.json.Hand-written prose is preserved untouched; actionlint's account of its stdin timeout is nuance a
generator cannot produce. The generator guarantees completeness and freshness, the prose keeps the
nuance, and both live in the plugin's own folder so they change together.
Same contract as
sync-hook-utils.sh: manifest is the single source of truth, the block is neverhand-edited, CI runs
--checkand rejects drift. The gate is in theci-statusneeds graph, so itis required rather than informational.
Verification
lib/hook-utils.test.sh— 180 passed, 0 failedclaude plugin validate— passes for both changed pluginsmarkdownlint-cli2 "plugins/*/README.md"— 65 files, 0 errorsscripts/check-changelog-parity.sh --check— passesactionlint .github/workflows/ci.yml— cleanconfirming
--checkexits 1, then 0 once regeneratedacross unset / option-false / option-true
Scope note
An audit of all 32 option-declaring plugins found exactly one dead gate (the
source-controloneabove) and one ungated execution path (the tee). Everything else already follows the documented
userConfig+CLAUDE_PLUGIN_OPTION_<KEY>pattern correctly — no migration was needed.