fix(context-guard): stop the manifest redeclaring the auto-loaded hooks file - #1988
Conversation
…ks file plugin.json set "hooks": "./hooks/hooks.json" — the default path Claude Code already discovers. The harness rejected the second registration as a duplicate and dropped the whole hook file with it, so zone-crossing injection, the blocking gate, and the PostCompact marker were inert wherever the plugin was installed. Adds a generic contract check: no plugin manifest may point a component field at that component's auto-discovered default path. The class fails silently at load time, so a gate catches it and a reviewer does not. Closes #1985 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMBm1Ae29KhZmjFxw2wdZ7
|
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 57s —— View job Security review complete ✅
No security issues found. This PR's diff is small and self-contained:
No authorization logic, credential/token handling, permission changes, or workflow trigger changes are touched by this PR, so the categories called out in the review brief (injection, auth gaps, secret handling, dangerous Action patterns) don't apply here. |
|
Claude finished @kyle-sexton's task in 4m 15s —— View job Code reviewSummary: The core fix (dropping the duplicate I cross-checked the new check's assumptions against the Plugins reference (fetched this session, per the repo's fresh-docs mandate). 1. The new gate rejects a documented, legitimate manifest pattern for The official reference's "Path behavior rules" section states
The new loop checks each array entry independently and fails as soon as any entry equals the default path: for (const value of declared) {
...
if (defaults.includes(normalized)) {
fail(path, `${field} must not name its auto-discovered default path (${value})`);
}
}So a plugin that adopts this exact, doc-sanctioned idiom — Suggested fix: only fail when the field's entire declared value collapses to the default (a bare string default, or a single-element array/only-entry equal to default), not when the default is one entry among several, for the fields where "replaces + re-add via array" is the documented pattern. 2. The gate's own justification comment overstates "silent" failure for non-
That's demonstrated true for 3. Coverage gap: The reference's component-path-fields table lists Nothing else stood out: the version bump ( |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ed tail (#2001) No linked issue Consumer report drained from the handoff inbox: `20260730-182801-context-guard-zone-crossing-hook-times-out-100-percent`. The zone-crossing hook was reported timing out 100% of the time; all four registrations sat at `timeout: 10`. ## The number was authored here, not inherited Per the hooks page fetched this session (<https://code.claude.com/docs/en/hooks>): > `timeout` | no | Seconds before canceling. Defaults: 600 for `command`, `http`, and `mcp_tool`; 30 for `prompt`; 60 for `agent`. `UserPromptSubmit` lowers the `command`, `http`, and `mcp_tool` default to 30, and `MessageDisplay` lowers it to 10. Unit is **seconds**. The applicable defaults are 600 (`PostToolBatch`, `PreToolUse`, `PostCompact`) and 30 (`UserPromptSubmit`). So `10` was a deliberate narrowing to 1/60th of the default, not something inherited — and it is below what the hooks actually take on Windows. ## Measured, post-#1979 The prior per-invocation saving (`9b90e351`) is on `main`, so the report's timings were stale. Re-measured with a harness invoking each script exactly as the hook would — real payload on stdin, `HOME` / `CLAUDE_PLUGIN_DATA` / `CLAUDE_PLUGIN_ROOT` set, snapshot present so the resolver does real work. Two runs, 18 samples per path: | Path | min | max | |---|---|---| | `zone-crossing-inject.sh` — PostToolBatch, 150 KB payload | 2.21 s | 6.68 s | | `zone-crossing-inject.sh` — UserPromptSubmit, small payload | 2.24 s | **22.01 s** | | `zone-gate.sh` — PreToolUse | 0.45 s | 2.83 s | | `post-compact-mark.sh` — PostCompact | 1.27 s | **12.37 s** | **These are noisy and are presented as such.** CPU load was 14% before run 1 and 68% during run 2; 523 processes; Defender real-time protection enabled. Identical work spanned 3.3 s → 22.0 s, so the means are unreliable and only the maxima carry the decision. Three things the measurement establishes: 1. **`post-compact-mark.sh` reached 12.4 s — over the old 10 s cap.** The report flagged this one as *inferred, not measured*, and as the most consequential, since sibling plugins read its marker. It is now measured fact. 2. **`zone-gate.sh` peaked at 2.83 s with no observed overrun.** It is raised for uniformity and tail-safety, not because it was failing — stated plainly rather than folded into a "they were all broken" claim. 3. **The tail is environmental, not payload-scaling.** The *small* UserPromptSubmit payload (22.0 s) beat the 150 KB PostToolBatch one (6.7 s). Sizing has to survive an AV-stalled process spawn, not just the median. ## Why 60 and not the report's suggested 30 22.0 s is a **floor, not a p100**: the harness times the script alone, excluding the harness's own hook-launch overhead, and it never sets `HOOK_TELEMETRY_SINK`, so `hook::emit_telemetry` short-circuits and its per-invocation `jq -n` + sink exec are excluded too (spawns cost ~140 ms each here, per the 0.4.6 entry). 30 would leave under 8 s of margin on an already-optimistic number. 60 gives ~2.7× while staying an order of magnitude under the 600 s default, so a genuinely hung hook still cannot stall a session for ten minutes. `guardrails` and `disk-hygiene` already declare 60 in this marketplace. A timeout is a cap, not a cost. ## What #1988 changed about the edit surface `.claude-plugin/plugin.json` no longer carries a `hooks` key at all — #1988 removed the redeclaration of the default-discovered path. The manifest was re-read at HEAD rather than trusted from the report. Consequence: `hooks/hooks.json` is now unambiguously the single declaration site for a timeout, so this change touches one file plus the version bump. The report's four-registration table is still accurate. ## Deliberately not asserted The page says only "Seconds before canceling" — what a cancelled hook reports, whether partial output is discarded, and whether sibling hooks continue are **not documented**, so none of it is claimed here. Likewise the page states 30 as `UserPromptSubmit`'s *default* and does not say whether it also **caps** a larger explicit value; 60 is declared regardless, which is harmless if clamped, since 22.0 s still clears 30. ## Verification | Check | Result | |---|---| | `zone-crossing-inject.test.sh` | PASS=18 FAIL=0 | | `zone-gate.test.sh` | PASS=24 FAIL=0 | | `post-compact-mark.test.sh` | PASS=16 FAIL=0 | | `hooks.json` parses | OK — timeouts `[60, 60, 60, 60]` | | `plugin.json` parses | OK — 0.4.8, no `hooks` key | | `markdownlint-cli2` | 0 issues | | `check-changelog-parity.sh --check-bump origin/main` | pass | | shellcheck / shfmt / shell-portability / exec-bit | N/A — no `.sh` changed (2 JSON + 1 MD) | `check-orphaned-fixtures.sh` was not run locally; it exceeds a 300s timeout on this machine. CI covers it. `context-guard` 0.4.7 → **0.4.8**. ## Out of scope, surfaced not fixed - Profiling the hot path (the report's suggestions 2–3) is the durable fix for the *cost*; this PR fixes the *cap*. The Windows per-invocation cost is still real and still open. - A timeout-observability notice is already served: every hook emits `duration_ms` via `hook::emit_telemetry`, opt-in through `HOOK_TELEMETRY_SINK`. - Same defect class elsewhere, unmeasured: `rate-limit-guard` declares `timeout: 10`, `guardrails` has two entries at `10`, and `claude-ops` declares `timeout: 5` on **eight** registrations — the tightest in the marketplace, notable given the spawn tail measured above. - `plugins/context-guard/CHANGELOG.md` cites `(#1985)` for 0.4.7 and `(#1978)` for 0.4.6, but those landed as `6b5c1b96 (#1988)` and `9b90e351 (#1979)`. Consistent pattern, likely issue-vs-PR numbering; unverified and not touched. ## Related - #1988 — removed the manifest's hooks redeclaration; the reason the manifest was re-read at HEAD rather than trusted from the report. - #1979 — the per-invocation saving that made the report's original timings stale and forced a re-measure. - Inbox item `20260730-182801-context-guard-zone-crossing-hook-times-out-100-percent` — the consumer report. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #1985
Summary
context-guard's manifest pointed itshooksfield at./hooks/hooks.json—the path Claude Code discovers on its own. The harness rejected the second
registration as a duplicate and dropped the whole hook file with it, so every
hook the plugin ships was inert on any machine that installed it.
Fix
hookskey fromplugins/context-guard/.claude-plugin/plugin.json(
0.4.6→0.4.7, changelog entry).claude-opsandguardrailsalreadyship
hooks/hooks.jsonwith no manifest key; this now matches them.scripts/validate-plugin-contracts.mjs: no pluginmanifest may name a component field's own auto-discovered default path
(
hooks/hooks.json,skills/,commands/,agents/,.mcp.json,.lsp.json). The manifest field exists for components at non-defaultpaths; naming the default is a duplicate load for
hooksand redundant forthe rest. This class fails silently at load time, which is what makes it a
gate's job rather than a reviewer's.
Verification
node scripts/validate-plugin-contracts.mjs→Plugin contracts validated: 43 setup skills and 2178 plugin files checked."hooks": "./hooks/hooks.json"fails with
plugins\context-guard\.claude-plugin\plugin.json: hooks must not name its auto-discovered default path (./hooks/hooks.json); removing itpasses.
the new check starts from a clean baseline.
hookslocation as
hooks/hooks.jsonin the plugin root or inline inplugin.json,and illustrates the manifest field with a non-default path
(
"hooks": "./config/hooks.json").Related
Refs #1972 — same session's sweep of load-bearing plugin defects.