fix(claude): enable the three catalogued plugins settings never named, and gate the parity - #3235
Conversation
…, and gate the parity docs/CLOUD-SESSIONS.md states that `enabledPlugins` "turns on the whole catalog, so this repo dogfoods everything it publishes". It did not. Three plugins reached main with a catalog entry and no settings key: ai-slop (#2892) context-budget (#2932) improvement (#2985) The plugin PRs on either side of them (coupling #2913, overengineering #2961) remembered the entry, which is what an unenforced convention looks like over time. The failure is silent by construction. .claude/cloud-bootstrap.sh computes its install set from that same map, so a session comes up green reporting "67 enabled" with those three plugins' skills simply absent and no line of output naming what is missing -- the shape docs/conventions/liveness-assertion/ calls a documented guarantee with nothing behind it. It costs exactly the dogfooding the relative `directory` marketplace source exists to provide. Adds scripts/check-plugin-catalog-enablement.sh and its contract test, wired as the `plugin-catalog-enablement-gate` lane inside ci-status. It holds the two sets equal in both directions and checks the alphabetical layout the same doc relies on. A key set to `false` passes: an explicit `false` is the documented off switch for the entries whose MCP servers need credentials this environment has no reason to hold, whereas an absent key is drift, and only one of those is silent. No existing detector covers this axis for this repo. plugins/claude-config/skills/audit/scripts/check-plugin-drift.sh audits it for CONSUMER repos, but resolves each marketplace through `source.repo` and records SKIP for one that declares none -- precisely this repo's relative `directory` source. check-plugin-manifest-presence.sh holds the catalog against the filesystem and says nothing about enablement. Verified: the new gate goes red on the pre-fix settings file naming all three plugins, and green on the fixed tree at 70/70. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zgom6KEf7dEKos4cVgyor
|
Claude finished @kyle-sexton's task in 2m 1s —— View job Security review
Scope: No security issues found. Notes from the review (informational, not findings):
|
|
Last security-reviewed head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a43aa550d5
ℹ️ 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".
…sed pattern The security review on #3235 raised this as informational rather than a finding, on the grounds that the marketplace name is repo-controlled and crosses no trust boundary. That is correct about the security question and beside the point about the correctness one. `sed -n "s/@${MARKET}\$//p"` interpolates a free-form settings value into a regular expression, so a metacharacter in the name changes what the pattern MATCHES rather than what it says. With a marketplace named 'melodic.software' the '.' matches any character: in: alpha@melodic.software, beta@melodicXsoftware old: alpha, beta <- 'beta' accepted as this marketplace's key new: alpha The old output makes the gate report 'beta' as an ORPHANED ENABLED ENTRY that does not exist, and -- the direction that matters -- lets a near-miss key stand in for the real one, so a genuinely unenabled plugin reads as enabled. A gate whose whole purpose is catching silent drift must not carry a silent-wrongness path of its own. Replaced with a literal `${key%"@$MARKET"}` suffix strip guarded by a literal `*"@$MARKET"` match, which is also one less process per run. The added contract check is discriminating, not decorative: run against the previous implementation it fails with exactly the false ORPHANED ENABLED ENTRY described above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zgom6KEf7dEKos4cVgyor
|
Claude finished @kyle-sexton's task in 1m 11s —— View job Security review
Scope: incremental diff since the last reviewed head. This run covers only what changed between the previously-reviewed No security issues found in the new commit. The new commit replaces the [[ "$key" == *"@$MARKET" ]] || continue
printf '%s\n' "${key%"@$MARKET"}"This is exactly the fix for the informational note from the prior review round (regex metacharacters in Out of scope for this lane (security-only, per |
…ace identity Raised as P2 by the Codex review on #3235, and correct. The gate derives the marketplace suffix from extraKnownMarketplaces; .claude/cloud-bootstrap.sh hardcodes marketplace_name and selects its install set with endswith("@" + $n). Nothing tied the two together. Rename the marketplace, update the settings keys and the catalog, and leave that constant alone, and the bootstrap's `wanted` array matches nothing: cloud sessions install none of the catalog, the run reports "plugins 0 enabled", and the new parity lane is green over the whole thing -- a gate certifying a set nobody installs. That is the same false-green shape this gate was added to close, one level up. Verifying the constant rather than deriving it in the bootstrap is deliberate: the bootstrap has to work before anything else does, so its behavior stays byte-identical, and a rename touching only one of the two now cannot merge. Three ways it can fail, none of them silent: MARKETPLACE IDENTITY MISMATCH -- the two name different marketplaces UNREADABLE BOOTSTRAP IDENTITY -- the constant moved or was renamed, so the check cannot confirm what it claims to exit 2 -- the bootstrap is absent entirely The last two fail rather than skip on purpose: a green result that quietly verified less than it says is what scripts/check-silent-skips.sh exists to deny, and this gate should not be the exception. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Zgom6KEf7dEKos4cVgyor
|
Claude finished @kyle-sexton's task in 1m 13s —— View job Security review
Scope: incremental diff since the last reviewed head. This run covers only what changed between the previously-reviewed No security issues found in the new commit. This commit is the fix for the P2 correctness gap Codex flagged: Reviewed for the same injection class the prior round flagged as informational on the sibling suffix-stripping code:
|
… claims
plugin-gate went red on this PR:
FAIL: .github YAML should exit 0 with no suites (rc=0):
scripts/check-plugin-catalog-enablement.test.sh
Not a flake and not incidental — this PR caused it, by adding a real edge.
scripts/affected-tests.sh walks R3 (a file's basename appearing in another
file's text is a dependency), and the walk now runs:
.github/actionlint.yaml
-> .claude/cloud-bootstrap.sh (its pin comment names that file)
-> scripts/check-plugin-catalog-enablement.sh (reads cloud-bootstrap.sh)
-> ...enablement.test.sh (a suite -> selected)
The probe asserted that the sole `.github/*.yaml` file selects NO suites, which
held only while nothing referenced it. Both new edges are real: the gate does
read cloud-bootstrap.sh, and R3 over-selecting through a comment is the
documented safe direction. Neither should be severed to keep a probe
convenient, so the probe moves instead -- the same resolution the block's
existing comment already records for .github/workflows/ci.yml, which lost
probe eligibility the same way.
The candidate is now filtered to a .github YAML that no grepped-language file
names at all, rather than assuming the one `*.yaml` qualifies. The filter is a
direct `git grep`, deliberately NOT a call to affected-tests.sh: choosing the
probe with the tool under test would make the assertion tautological. The
glob-not-literal discipline the block's preamble insists on is preserved --
no basename is written out, so this file still cannot become a referencing
suite for its own probe.
Verified the assertion is still load-bearing rather than merely green: with
`.github/*` stripped from the no-suite list via AFFECTED_TESTS_NO_SUITE, the
selected probe goes UNMAPPED and the case fails. Suite: 47 pass, 0 fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zgom6KEf7dEKos4cVgyor
|
Claude finished @kyle-sexton's task in 46s —— View job Security review
Scope: incremental diff since the last reviewed head. This run covers only what changed between the previously-reviewed No security issues found in the new commit. The change replaces a single hardcoded mapfile -t wf_candidates < <(cd "$REPO_ROOT" && git ls-files '.github/*.yaml' '.github/*.yml')
wf_yaml=()
for c in ${wf_candidates[@]+"${wf_candidates[@]}"}; do
if ! (cd "$REPO_ROOT" && git grep -q -F -- "${c##*/}" \
-- '*.sh' '*.bash' '*.js' '*.mjs' '*.cjs' '*.py' '*.ps1' '*.psm1') 2>/dev/null; then
wf_yaml=("$c")
break
fi
doneThis is test-only tooling with no trust-boundary crossing:
No other files changed in this commit, so the rest of the PR's prior review findings stand unchanged. |
|
Claude finished @kyle-sexton's task in 37s —— View job Code review — skipped
Result: skipped, per the skill's skip gate. The For reference, this PR already went through three rounds of |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Run /code-tidying:dissolve-comments (strict) over 26 gate scripts and
suites: 12 files, every edit COMMENT-ONLY. Revision SHAs, "first
revision" narration, incident lists and review attributions removed or
put in present tense; shared vocabulary ("the #1513 shape"), usage
headers and shellcheck directives kept. Census delta: -47 comment
lines, -2862 bytes. 55 affected suites pass; the other 3 fail only on
missing htmlhint/strace in this environment.
Removed narrative, verbatim:
- scripts/check-detector-eval-coverage.test.sh: "which are being backfilled in the same change set this gate landed in, and would otherwise make this suite red for someone else's in-flight work." / "it is the shape claude-code-plugins#4149 found already merged (an eval asserting a scope two checks out of date)" / "Sections marked N1 and N2 are regression tests for two defects an independent verifier reproduced against c7f71c3, where the gate had over-corrected ... Each N case fails against c7f71c3 and passes against this revision." / "Sections marked P1..P6 below are regression tests for defects an independent verifier reproduced against the gate as first committed (0273b5b), where it verified MENTION rather than coverage, ... Each of those cases FAILS against that revision and passes against this one; that is what makes them regression tests rather than restatements of current behavior." / "P7 is the same shape one revision later: the stopping rule had an extractor of its own ... the same defect class (a fix applied at one call site and missed at its sibling) the verdict scanner beside it had already been hardened against. Its first case FAILS against adcbb77; ... and pass on both sides." / "Each case below passed the first revision at rc 0 while covering nothing" / "Parseability was the only shape check there was." / "Reproduced against c7f71c3: ... Each case below exits 1 against that revision and 0 against this one." / "so the first revision's only guard (zero ids) never fired." / "The first revision's greedy `.*` kept only the last" / "found by a defeat attempt against the merged gate." / "all four were SILENT LOSSES under one revision or another ... the revision that returned the bare head ... the revision that kept the whole tail ... the revision that tried to split the difference ... Three attempts, three silent losses, each found by the round after the one that shipped it." / "which defeated the last-unclosed-`$(` attempt, and a backtick substitution, which it could not see at all." / "these are the spellings the rewritten delimiter matcher had to keep reading." / "An earlier version of this helper asserted only that the body's `emit error P9` was not counted, and that is the assertion that let four regressions through" / "These were written as `arms nothing` cases when the matcher used a character class and refused whatever fell outside it." / "the guard passes against the revision that has the bug." / "Every heredoc defect this scanner has had" / "Reproduced against c7f71c3: each shape below made a well-formed detector un-gateable, because the site counter and the id extractor disagreed" / "this guard was a byte-identical copy of that fixture until an audit caught it, a duplicate masquerading as a second dimension of coverage." / "The discovery half kept a greedy sed of its own ... while the verdict scanner beside it read that same line as two call sites correctly. Discovery now runs that same scanner" / "a per-row exit 2 used to leave the FIRST row's report" / "P6: trailing argv was silently ignored"
- scripts/check-plugin-catalog-enablement.sh: "Nothing enforced it. Three plugins reached main with a catalog entry and no `enabledPlugins` key: ai-slop (#2892), context-budget (#2932) and improvement (#2985), while the plugin PRs on either side of them (coupling #2913, overengineering #2961) remembered the settings entry." / "WHERE ENABLEMENT LIVES NOW" / "this file no longer mirrors the whole catalog (that mirror was writing one ..." / "The class that shipped three times." / "Raised as P2 by the Codex review on #3235."
- scripts/check-plugin-catalog-enablement.test.sh: "the class that actually shipped (a catalogued plugin with no enabledPlugins key, three times: #2892, #2932, #2985)" / "--- 2. The class that shipped: catalogued, enabled nowhere." / "what settings no longer mirrors" / "This is the post-migration shape:" / "Under the original `sed -n ...`" / "Raised as informational by the security review on #3235." / "Raised as P2 by the Codex review on #3235."
- scripts/check-purged-em-dashes.sh: "WHY (#2891)." / "each landed shard ... every shard so far has needed a rationale-withheld reviewer to catch clauses the automated passes waved through. Nothing then stops ... because no lane enforces the policy." / "29,649 em-dash prose lines across 1,074 tracked markdown files remained when this gate was written" / "#3342 needs a several-hundred-file allowlist; that is only affordable". .test.sh: "the narrower config #3342 asked for".
- scripts/check-skill-precompute-compose.test.sh: "The #3377 fix hoists `git rev-parse` into the parent, so the mode dispatch ... is now load-bearing"
- scripts/check-docs-naming.sh: "docs/ carried a mix of UPPER-KEBAB, lower-kebab, and mixed-case names for years, and every reference to a doc had to remember which spelling that one file used."
- scripts/lib/fixture-tree.sh: "not the three-variable spelling that was drifting through the suites."
- scripts/sync-resolve-convention-home.sh: "plugin-quality (the ADR 0018 pilot) is the first carrier."
- scripts/test-git-helpers.sh: "The exported GIT_DIR behind the real incident came from an ad-hoc tool invocation, not from a git hook: this repository has no git hook at any scope and core.hooksPath is unset everywhere." .test.sh: "That is what happened to #2827 -> #2830."
- scripts/generate-catalog.mjs: "One-way gates existed before this: ... but nothing compared CATEGORY_ORDER back to the document"
Intentional-removal: dissolve-comments pass; removed comment text is recorded above.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UpA569K1JgnKu1jQvRqMjL
No linked issue
Summary
docs/CLOUD-SESSIONS.mdstates thatenabledPlugins"turns on the whole catalog, so this repo dogfoods everything it publishes and a regression in any plugin surfaces here first." It did not. Three plugins reachedmaincarrying a.claude-plugin/marketplace.jsonentry and no.claude/settings.jsonkey, so the catalog was 70 and the enabled set was 67.ai-slopcontext-budgetimprovementThe plugin PRs on either side of them (
coupling#2913,overengineering#2961) did add the settings entry, which is what an unenforced convention looks like over time.The failure is silent by construction.
.claude/cloud-bootstrap.shcomputes its install set from that same map, so a cloud session comes up green reportingplugins 67 enabledwith those three plugins' skills simply absent and no line of output naming what is missing — a documented guarantee with nothing behind it, and it costs exactly the dogfooding the relativedirectorymarketplace source exists to provide.No existing detector covers this axis for this repo:
plugins/claude-config/skills/audit/scripts/check-plugin-drift.shaudits it for consumer repos, but resolves each marketplace throughsource.repoand recordsSKIPfor one that declares none — precisely this repo's relativedirectorysource, so it structurally cannot see this repo's own drift.scripts/check-plugin-manifest-presence.shholds the catalog against the filesystem (manifest present, name matches, no unregistered directory) and says nothing about whether a catalogued plugin is ever enabled.Fix
.claude/settings.json— adds the three missing keys in alphabetical position. Enabled set is now 70/70.scripts/check-plugin-catalog-enablement.sh(new, plus its contract test) — holds the two sets equal in both directions, checks the layout the doc relies on, and checks that the bootstrap agrees on which marketplace this is:UNENABLED PLUGIN— a catalog entry with no<name>@<marketplace>key. The class that shipped three times.ORPHANED ENABLED ENTRY— a key for this marketplace naming no catalog entry. What a rename or removal leaves behind; the id resolves to nothing and the install silently no-ops.UNSORTED enabledPlugins— keys out of byte order, since the alphabetical one-per-line layout is what the doc calls the reason a single plugin can be flipped tofalsewithout disturbing the rest.MARKETPLACE IDENTITY MISMATCH—.claude/cloud-bootstrap.sh's hardcodedmarketplace_nameno longer naming the marketplace the settings file declares. Added in response to the Codex P2 below.A key set to
falsepasses. An explicitfalseis the documented off switch for the entries whose bundled MCP servers need credentials this environment has no reason to hold (miro,dometrain); an absent key is drift. Only one of those two states is silent, and the gate distinguishes them rather than forcing an off switch nobody can use.The marketplace name is derived from
extraKnownMarketplacesrather than hardcoded, so a marketplace rename cannot leave the gate quietly checking a suffix nothing uses. Two declared marketplaces exits2rather than guessing which catalog to hold the set against. Suffix stripping is a literal${key%"@$MARKET"}expansion, never asedpattern interpolating the name..github/workflows/ci.yml— addsplugin-catalog-enablement-gateand lists it inci-status.needs, so it actually gates a merge. Self-test runs first, matching the sibling gates, so a broken detector cannot mask a regression behind a green lane.docs/CLOUD-SESSIONS.md— the whole-catalog bullet now names the lane that enforces it, the identity check, and why the consumer-side detector cannot cover this.scripts/affected-tests.test.sh— moves the.githubno-suite probe off a path this PR gave a real dependency. See "Review rounds" below.Review rounds
Three findings were raised and all three are closed. Each fix is a separate commit.
$MARKETinterpolated into asedexpression, so a regex metacharacter in the marketplace name changes what the pattern matches3fc3cbd8— literal suffix stripcloud-bootstrap.shhardcodes it; a rename could pass the gate while the bootstrap installed nothinga36bc765— check 4 aboveplugin-gateCI failureaffected-tests.test.sh11ef3f81— probe movedThe
plugin-gatefailure is worth spelling out, since it was caused by this PR rather than being incidental.affected-tests.shtreats a basename appearing in another file's text as a dependency, so this PR created the chain.github/actionlint.yaml→.claude/cloud-bootstrap.sh(its pin comment names that file) →check-plugin-catalog-enablement.sh(reads cloud-bootstrap.sh) → its suite. The probe asserted the sole.github/*.yamlfile selects no suites, which held only while nothing referenced it.Both new edges are real — the gate genuinely reads
cloud-bootstrap.sh, and R3 over-selecting through a comment is the documented safe direction — so neither was severed to keep the probe convenient. The probe moves instead, which is the same resolution that block's existing comment already records for.github/workflows/ci.yml. The replacement filters candidates with a directgit greprather than by callingaffected-tests.sh, because choosing the probe with the tool under test would make the assertion tautological.Verification
Ran locally on this branch, at head
11ef3f81:scripts/check-plugin-catalog-enablement.sh→ green:Every one of the 70 catalogued plugins carries an enabledPlugins key for 'melodic-software'; none orphaned; keys sorted; .claude/cloud-bootstrap.sh installs that same marketplace.bash scripts/check-plugin-catalog-enablement.test.sh→ 13/13 contract checks pass.bash scripts/affected-tests.test.sh→ 47 pass, 0 fail..claude/settings.jsonand it went red naming all three plugins, then green on the fixed tree.sedimplementation with a falseORPHANED ENABLED ENTRY; the relocated.githubprobe fails when.github/*is stripped from the no-suite list viaAFFECTED_TESTS_NO_SUITE.scripts/check-lane-coverage.sh --check→all 41 lane(s) reachable from ci-status.needs.actionlint,shellcheck,shfmt -d(EditorConfig-driven, matching the repo'sindent_size = 2rather than hand-passed flags),scripts/check-shell-portability.sh --paths,scripts/check-silent-skips.sh,scripts/check-plugin-manifest-presence.sh,scripts/check-changelog-parity.sh --check,scripts/validate-plugins.sh,markdownlint-cli2,typos,editorconfig-checker→ all clean.Behavioral note for reviewers: enabling
context-budgetactivates itssettings-write-askPreToolUse hook, which forces a permission prompt on anyWrite/Edittargeting a Claude Code settings surface. That is the plugin working as documented and it ships asettings_write_ask_enabledkill switch, but it is a live change to sessions in this repo and worth knowing before merge.Related
Refs #3196 — the same three plugin names appear there as an unsorted
enabledPluginstail at user scope after a/claude-ops:plugins sync. Different scope and different cause (that one is Claude Code's settings writer appending rather than inserting); this PR fixes the committed project-scope map and adds the gate. The new check's sorted-keys assertion would catch the project-scope symptom of #3196 if it ever landed here.