fix(claude): register the session-start hook and install enabled plugins - #2655
Conversation
|
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 4m 40s —— View job Security review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f394cbd26
ℹ️ 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".
|
Last security-reviewed head: |
Addresses two review findings on #2655. Same-version commit drift (Codex P1). A directory-source install cache is keyed by the semver in plugin.json rather than the commit, so a presence check alone kept serving whichever commit installed first — the exact failure `docs/MIGRATION-PLAYBOOK.md` records under "Same-version commit drift" and #2061, and one that defeats the reason this repo uses a directory source. The drift was already live: plugins were installed at c88370f while HEAD had advanced. The hook now compares the `gitCommitSha` recorded at install time against HEAD and runs the documented uninstall/install/enable cycle for the plugins whose own directory changed between the two, so an ordinary resume stays cheap. Uncommitted edits stay out of scope, where the playbook already points at `--plugin-dir`. Hash-verification fail-open (security review, IMPORTANT). The previous fallback discarded pip's stderr and treated every `--require-hashes` failure as the benign interpreter mismatch, which would have downgraded a genuine digest mismatch from fatal to a warning. The benign case is now decided up front by comparing ci.yml's pinned Python against the VM's: only a real mismatch takes the ruff-only path, and when the interpreters agree — or either version can't be determined — the locked install runs with stderr intact and a failure is once again fatal. Verified: the refresh predicate selects repo-fleet-hygiene (10 files changed since the recorded SHA) and skips session-flow, adhd, and claude-config (0 changed); an uninstalled id falls through to the install path. Interpreter detection reads ci_python=3.14 against vm_python=3.11 and selects the ruff-only branch. shellcheck (with the repo's check-set-e-suppressed enabled), shfmt, bash -n, markdownlint, typos, editorconfig-checker, and both hook tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fy5VhW884igbqyZfZmJY2i
|
Both review findings are fixed in Same-version commit drift (Codex, P1) — valid, and the drift was already live on this branch: plugins were installed at The hook now reads the Verified against live state: Hash-verification failing open (security review, IMPORTANT) — also valid, and the more serious of the two. Deciding "was this the benign case?" by inspecting a failure is the wrong shape: it cannot distinguish an interpreter mismatch from a tampered wheel, and I had additionally discarded pip's stderr, so the one signal that would catch tampering became a warning in a wall of bootstrap output. Inverted to decide up front instead. The hook parses the Python version
Generated by Claude Code |
|
Claude finished @kyle-sexton's task in 2m 20s —— View job Security review —
|
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> Closes #2654 ## Summary Cloud Melodic SessionStart failed because `.github/requirements-ci.txt` hash-locked PyYAML 6.0.3 to **cp314** wheels while the VM uses **Python 3.11**, so pip selected a cp311 manylinux wheel whose hash was missing. ## Fix - Added the cp311 platform hash set for PyYAML 6.0.3 (Linux/macOS/Windows parallels to the existing cp314 list), verified against PyPI — including the blocker Linux x86_64 hash from #2654. - Noted cp311 coverage in `docs/CLOUD-FLEET-SETUP.md` verification checklist item 6. ## Verification ``` python3.11 -m pip install --dry-run --require-hashes -r .github/requirements-ci.txt # selects pyyaml-6.0.3-cp311-…-manylinux…x86_64.whl; exit 0 ``` ## Related Refs #2655 — SessionStart registration / `enabledPlugins` install. ## Residual host notes 1. **Blocker 1 — .NET proxy allowlist:** switch Melodic to Custom and add `dot.net`, `aka.ms`, `builds.dotnet.microsoft.com`, `download.visualstudio.microsoft.com`. 2. **Blocker 2 — interrupted env cache build:** rebuild the environment cache and re-run the verification checklist. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-eb3f5ee5-6c9f-48a3-8e46-071bd363f8b0?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-eb3f5ee5-6c9f-48a3-8e46-071bd363f8b0&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @kyle-sexton's task in 4m 6s —— View job Code review —
|
Addresses two review findings on #2655. Same-version commit drift (Codex P1). A directory-source install cache is keyed by the semver in plugin.json rather than the commit, so a presence check alone kept serving whichever commit installed first — the exact failure `docs/MIGRATION-PLAYBOOK.md` records under "Same-version commit drift" and #2061, and one that defeats the reason this repo uses a directory source. The drift was already live: plugins were installed at c88370f while HEAD had advanced. The hook now compares the `gitCommitSha` recorded at install time against HEAD and runs the documented uninstall/install/enable cycle for the plugins whose own directory changed between the two, so an ordinary resume stays cheap. Uncommitted edits stay out of scope, where the playbook already points at `--plugin-dir`. Hash-verification fail-open (security review, IMPORTANT). The previous fallback discarded pip's stderr and treated every `--require-hashes` failure as the benign interpreter mismatch, which would have downgraded a genuine digest mismatch from fatal to a warning. The benign case is now decided up front by comparing ci.yml's pinned Python against the VM's: only a real mismatch takes the ruff-only path, and when the interpreters agree — or either version can't be determined — the locked install runs with stderr intact and a failure is once again fatal. Verified: the refresh predicate selects repo-fleet-hygiene (10 files changed since the recorded SHA) and skips session-flow, adhd, and claude-config (0 changed); an uninstalled id falls through to the install path. Interpreter detection reads ci_python=3.14 against vm_python=3.11 and selects the ruff-only branch. shellcheck (with the repo's check-set-e-suppressed enabled), shfmt, bash -n, markdownlint, typos, editorconfig-checker, and both hook tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fy5VhW884igbqyZfZmJY2i
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
1565c70 to
8773eae
Compare
|
Addressed both unresolved review threads:
Rebased onto |
|
Confirming the fail-closed change on #2657's cp311 hashes do cover this VM's interpreter, so the strict path succeeds rather than aborting. The full CI-parity inventory is now present for the first time — all three were absent before this PR:
That closes the review finding properly: the version-string proxy is gone, the real install runs, and Two notes for later, neither blocking:
CI on this head: 32 green, remainder in flight, none failing. Generated by Claude Code |
Rebased onto latest main for merge-queue drain. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
8773eae to
44d2dcf
Compare
|
Claude finished @cursor[bot]'s task in 5m 19s —— View job Code review —
|
Incorporate the 2026-08-14 live Melodic environment verification (#2654) into CLOUD-FLEET-SETUP.md, attribute SessionStart registration to #2655 and catalog enablement to #2631, and keep residual host notes for .NET allowlist / env cache rebuild. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Incorporate the 2026-08-14 live Melodic environment verification (#2654) into CLOUD-FLEET-SETUP.md. Attribute SessionStart registration to #2655 and catalog enablement to #2631; keep residual host notes for .NET allowlist and env cache rebuild. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
) No linked issue. ## Summary `.claude/settings.json` already declared the `melodic-software` marketplace and enabled all 65 of its plugins, but that declaration does not survive an untrusted folder. Per [What runs before you trust a folder](https://code.claude.com/docs/en/permissions#what-runs-before-you-trust-a-folder), `extraKnownMarketplaces` supplied by the repository is *"Not used, and no dialog is offered"* when this exact folder is untrusted, while hooks in settings files are *"Used"* in both untrusted columns. Cloud sessions arrive untrusted, so the declaration alone resolves to nothing there. This was not theoretical — the session that wrote this PR reproduced it directly: ``` $ jq '.projects["/home/user/ci-runner"].hasTrustDialogAccepted' ~/.claude.json false $ claude plugin marketplace list --json [] $ claude plugin list --json [] $ cat ~/.claude/plugins/installed_plugins.json { "version": 2, "plugins": {} } ``` All 65 plugins enabled, none loadable. Note that [cloud-environments.md](https://code.claude.com/docs/en/cloud-environments#what-carries-over-from-your-setup) states plugins declared in `.claude/settings.json` are *"Installed at session start from the marketplace you declared."* This session is a counterexample to that claim, which is the reason the hook is warranted rather than redundant. ## Fix Add `.claude/hooks/install-plugins.sh` and register it on the `startup|resume` SessionStart matchers. Because hooks are not trust-gated, this path works in both local and cloud sessions. `enabledPlugins` and `extraKnownMarketplaces` are unchanged — the diff is additive only. The declaration is kept: it is the correct mechanism once a folder is trusted, and it stays the single source of truth the script reads from. Design points: - **No second copy of the plugin list.** The script derives the wanted set by reading `enabledPlugins` out of `.claude/settings.json`, filtering to `value == true` and the `@melodic-software` suffix. Editing the settings file is the only place a plugin is added or removed. - **Activates in the first session.** `claude plugin install` does not activate a plugin in the session that runs it, so the hook returns the SessionStart [`reloadSkills`](https://code.claude.com/docs/en/hooks#sessionstart-decision-control) field, which re-scans the skill and command directories once SessionStart hooks finish. It is requested only when a plugin was actually installed; a warm start already loaded the catalog. (Raised in review — see the thread on `.claude/settings.json`.) - **User scope only.** `--scope user` for both the marketplace and every install, so the hook never rewrites the repo's tracked settings file. - **Idempotent.** Already-installed plugins are skipped, so steady state is two CLI calls, not 65 installs. - **Best effort.** A missing `claude` or `jq`, an unaddable marketplace, or one failed plugin costs that plugin's skills, not the session. - `marketplace remove` is never invoked; it would delete the marketplace's entry from the tracked settings file. Conventions followed: `set -Eeuo pipefail` and `[[ ]]` per the repo's `require-double-brackets` rule, two-space indent per `.editorconfig`, `command -v` per `deprecate-which`, no machine-specific paths (`$CLAUDE_PROJECT_DIR` with a `BASH_SOURCE` fallback), and mode `100755` for the exec-bit lane. ## Verification Verified in-session, with output: | Check | Result | | :-- | :-- | | Trust gating reproduced | `hasTrustDialogAccepted: false`; marketplace, plugin, and installed lists all empty | | `enabledPlugins` vs catalog | 65/65 exact match, all `true`, all `@melodic-software`; no drift either direction | | **Hook actually dispatches** | `claude --init-only` debug log: `Hook SessionStart:startup (SessionStart) success`, naming `bash "$CLAUDE_PROJECT_DIR/.claude/hooks/install-plugins.sh"` | | **Hook runs untrusted** | The above fired with `hasTrustDialogAccepted` still `false` — the premise of the change | | `$CLAUDE_PROJECT_DIR` injection | Resolved by the harness; JSON output parsed and honored | | Script run (warm) | 65 enabled, 64 newly installed, exit 0, empty stderr | | Idempotent re-run | 65 enabled, 0 newly installed, exit 0, ~1s | | Final state | 65/65 enabled plugins installed, every one at `scope: user` | | Repo settings untouched by installs | marketplace landed in *user* settings; `git diff` on `.claude/settings.json` stayed at the `+13` hook lines only | | Cold-start path | In an isolated `CLAUDE_CONFIG_DIR` with zero marketplaces, the script added the marketplace itself and installed at user scope | | `reloadSkills` logic | `true` on the cold path (2 installed), `false` on the warm path (0 installed); emitted as a JSON boolean | | `false` entries excluded | A probe entry set to `false` was correctly skipped (`2 enabled`, not 3) | | ShellCheck 0.11.0 + repo `.shellcheckrc` | clean (0.9.0 from apt lacks `--rcfile`; 0.11.0 pulled to match CI) | | `shfmt -i 2 -d` | clean, no diff | | JSON Schema | `check-jsonschema` against `json.schemastore.org/claude-code-settings.json`: valid; `hooks`, `enabledPlugins`, and `extraKnownMarketplaces` all genuinely modeled, and `SessionStart` an accepted event | **Stated plainly, what is still not proven.** `reloadSkills` re-scans skill and command directories, so plugin-provided **skills and commands** activate in the first session. Plugin-provided **hooks, MCP servers, and LSP servers** still require the next session start — 18 of the 65 plugins ship a `hooks.json`, and those hooks are late by one session on a cold container. Every session after the first loads everything normally. The first-session skill activation is verified only in the sense that the field is emitted correctly and the harness accepts the JSON; I did not observe a cold container activate a freshly installed skill mid-session. The full 65-plugin install was exercised warm, and the cold marketplace-add branch was exercised with a trimmed two-plugin settings file. No single run covered both at once. First run in a fresh session installs 65 plugins and takes several minutes; subsequent starts are ~1s. ## Related - Docs consulted: [settings](https://code.claude.com/docs/en/settings#extraknownmarketplaces), [discover-plugins](https://code.claude.com/docs/en/discover-plugins#configure-team-marketplaces), [plugin-marketplaces](https://code.claude.com/docs/en/plugin-marketplaces), [permissions](https://code.claude.com/docs/en/permissions#what-runs-before-you-trust-a-folder), [cloud-environments](https://code.claude.com/docs/en/cloud-environments#what-carries-over-from-your-setup), [hooks](https://code.claude.com/docs/en/hooks#sessionstart) - Marketplace source: `melodic-software/claude-code-plugins` (plugins vendored in-repo under `./plugins/<name>`) - Background on the trust-gating behavior: `melodic-software/claude-code-plugins#2655` (referenced without a closing keyword, since it lives in another repository) --------- Co-authored-by: Claude <noreply@anthropic.com>
#3098) Closes #2959 Closes #2960 ## Summary Delete the dead repo-local hook `.claude/hooks/pr-linkage-mcp-gate.sh` (and its unused test) and add a hygiene-lane wiring-liveness check so the same class cannot ship green again. #2188 stripped project hook wiring as a bare-baseline reset: an instruction returns only with ledger evidence. #2655 restored SessionStart only. The leftover script kept claiming it loaded in every session while `.claude/settings.json` no longer named it. Policy enforcement already survives via the source-control plugin hook plus required CI `pr-issue-linkage`. This PR does not rewire the stripped hook. The new check requires every `.claude/hooks/*.sh` except `*.test.sh` to be referenced by `settings.json` hook commands (or args) or its `env` block, which is how `hook-telemetry-sink.sh` stays live via `HOOK_TELEMETRY_SINK`. ## Fix - Delete `.claude/hooks/pr-linkage-mcp-gate.sh` and `.claude/hooks/pr-linkage-mcp-gate.test.sh`. Nothing else invoked the test, and the plugin copy plus required CI remain the enforcement path. - Add `scripts/check-hook-wiring-liveness.sh` in the hygiene lane: every non-test `.claude/hooks/*.sh` must appear in `settings.json` hook commands/args or `env`, with a bounded path-segment match so `not-gate.sh` does not satisfy `gate.sh`. - Self-test first (`scripts/check-hook-wiring-liveness.test.sh`), then the gate, then feed `hook-wiring-liveness` into the existing hygiene aggregator. ## Test plan - `bash scripts/check-hook-wiring-liveness.sh` on the pre-delete tree (dead script still present) exits 1 and names `.claude/hooks/pr-linkage-mcp-gate.sh`. - After deletion, the same command exits 0. - `bash scripts/check-hook-wiring-liveness.test.sh` — all assertions passed, including the pre-delete replica, `*.test.sh` exclusion, command- and args-form wiring, bounded basename match, fail-closed missing/invalid settings, and a live-checkout pin. - `bash plugins/source-control/hooks/pr-linkage-mcp-gate.test.sh` — 24/24 (plugin copy unchanged). - `bash .claude/hooks/hook-telemetry-sink.test.sh` — PASS. - `shellcheck` on the new scripts — clean. - `bash scripts/affected-tests.sh .github/actionlint.yaml` — empty selection (the new suite no longer names a file that chains from that probe). ## Verification Pre-delete run of the new gate exited 1 naming `pr-linkage-mcp-gate.sh`. Post-delete run exited 0. The contract suite passed locally, including the #2959 replica and the bounded-match case. Plugin MCP-gate tests stayed 24/24. The first CI cycle failed on typos (`unparseable`), changelog-parity (comment-only plugin edits), plugin-gate (affected-tests R3/R4 via a bootstrap filename), and PR-body `## Fix` / `## Verification` sections; those are the fixes in this update. ## Related - #2188 (bare-baseline reset that stripped the settings hook entry) - #2655 (restored SessionStart only; did not restore the MCP gate) - #2959 (the live unwired-script specimen this PR deletes) --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
No linked issue
Summary
Cloud sessions on this repo started with zero plugins loaded and no bootstrap at all: every
/plugincommand unknown, no plugin skill available, an empty~/.claude/plugins/installed_plugins.json, and none of the CI-parity tooling.claude/hooks/session-start.shexists to provide. Three independent causes, all fixed here.Fix
1. The SessionStart hook was never registered.
.claude/settings.jsonhad nohookskey. The script has carried a header comment claiming it was registered since it was added in 39fe757, anddocs/CLOUD-SESSIONS.mddocumented the registration, butgit log -p --all -- .claude/settings.jsonshows the key was never committed — so the bootstrap had never run in a cloud session. Registered with matcherstartup|resume, pointing at the script via$CLAUDE_PROJECT_DIR.2. Declaring a marketplace is not installing it. What runs before you trust a folder groups
extraKnownMarketplacesentries with content that needs this exact folder trusted, while hooks and theenvblock are used whether or not it is. A cloud session on this repo hadhasTrustDialogAcceptedfalse, an empty plugin registry, and no plugin loaded — while the same settings file'senvblock had applied, exactly the split that table predicts. A hook is the durable fix because hooks run untrusted.The hook registers the checkout by absolute path and installs every plugin
enabledPluginssets totrue, computed from the tracked settings file withjqso it cannot drift from the catalog. It deliberately never callsclaude plugin marketplace remove— that subcommand deletes the marketplace's entry from.claude/settings.json, so a hook using it to force a re-add would silently mutate tracked repository config. (Observed directly while diagnosing this.)It also repairs same-version commit drift. A directory-source cache is keyed by the semver in
plugin.jsonrather than the commit (docs/MIGRATION-PLAYBOOK.md, #2061), so a presence check alone would keep serving whichever commit installed first — defeating the reason this repo uses a directory source. The hook compares thegitCommitSharecorded at install time againstHEADand runs the documented uninstall/install/enable cycle for the plugins whose own directory changed, so an ordinary resume stays cheap.3. The Python install aborted the bootstrap.
.github/requirements-ci.txtwas hash-locked against CI's Python 3.14 wheels only; the cloud VM ships 3.11.15 and resolves a differentpyyamlwheel, so--require-hashesrefused it andset -eaborted before the hygiene binaries and git-history steps ever ran. That surfaced #2654, fixed onmainby #2657, which added the cp311 hash set.This branch is rebased onto that fix and keeps the install fail-closed: SessionStart always runs the hash-locked install and a failure stays fatal. An earlier revision of this branch fell back to a ruff-only install on an interpreter-version mismatch; that was removed, because it both weakened the one control that catches a tampered wheel and — once #2657 landed — would have skipped an install that now succeeds, leaving
pyyaml/pytestpermanently absent.docs/CLOUD-SESSIONS.mdis corrected in the same change: it previously claimed the hook was registered and described the directory source as installing at session start.Verification
Verified live rather than by inspection — the session restarted mid-change and the newly registered hook fired on its own:
All 65 plugin skill sets became available in-session (
/claude-config:audit, the command that surfaced this, among them), andmarkdown-formatandhardcoded-path-checkhooks then fired on subsequent edits — confirming plugin hooks load, not just skills.The fail-closed install, checked on a real cloud VM at
8773eae5:Full CI-parity inventory now present, all three previously absent or wrong:
ruff0.16.2 (CI's exact pin, against the VM image's 0.15.8),pytest9.1.1,pyyaml6.0.3.Other post-run state:
claude plugin list --json | jq length→ 103 (65 enabled + 38 auto-installed dependencies)claude plugin marketplace list --json→{"name":"melodic-software","source":"directory","path":"<repo-root>"}— resolved from the checkout, so a session exercises branch code rather than publishedmainrepo-fleet-hygiene(10 files changed since the recorded SHA) and skipssession-flow,adhd,claude-config(0 changed); an uninstalled id falls through to the install pathGates, all clean:
shellcheck(with the repo'scheck-set-e-suppressed),shfmt -d,bash -n,markdownlint-cli2,typos,editorconfig-checker,check-jsonschemaagainst the settings schema, and both repo-local hook tests (17/17). Empty-array handling underset -uchecked against the VM's bash 5.2.21.Not verified: whether a
github-source marketplace auto-installs at session start without a hook. That is the path consumer repos should use, and it resolves fine here (claude plugin marketplace add melodic-software/claude-code-pluginsclones through the proxy), but confirming the no-hook auto-install needs a fresh cloud session on a consumer repo.Follow-up, not blocking: the strict path is load-bearing on
requirements-ci.txtcovering whateverpython3the cloud image ships. If that image moves to a minor with no wheel hashes, session start aborts rather than degrades — worth generating hashes per supported interpreter instead of reactively, as #2654 → #2657 just did.Related
Refs #2061 (same-version commit drift), #2654 / #2657 (cp311 wheel hashes), and the plugin-enablement setup in
docs/CLOUD-SESSIONS.mdanddocs/CLOUD-FLEET-SETUP.md.