fix(docs): correct setup-contract drift in docs and tracked PR config - #3139
Conversation
|
Claude finished @kyle-sexton's task in 40s —— View job Security Review
Skip gate triggered — no security-relevant surface. This PR is documentation-only: all four changed files are markdown docs Reviewed the full diff (
No security findings to report for this PR. |
|
Claude finished @kyle-sexton's task in 3m 22s —— View job CI code review —
|
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c4bae410
ℹ️ 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. |
|
Claude finished @kyle-sexton's task in 5m 18s —— View job CI code review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude finished @kyle-sexton's task in 4m 52s —— View job CI code review —
|
c0ae202 to
fa6786e
Compare
|
Claude finished @kyle-sexton's task in 51s —— View job Security Review
Skip gate triggered — no security-relevant surface. Reviewed the full diff at this head (
No Checked against this lane's criteria regardless:
No security findings to report for this PR. |
|
Claude finished @kyle-sexton's task in 2m 48s —— View job CI code review —
|
|
Claude finished @kyle-sexton's task in 51s —— View job Security Review
Skip gate triggered — no security-relevant surface. Last security-reviewed head was
That commit touches only
Confirmed no No security findings to report for this PR. |
|
Claude finished @kyle-sexton's task in 40s —— View job Security Review
Skip gate triggered — no security-relevant surface. Last security-reviewed head was
That commit touches only
Confirmed no No security findings to report for this PR. |
|
Claude finished @kyle-sexton's task in 2m 2s —— View job CI code review —
|
452dd11 to
5fbd6a9
Compare
|
Claude finished @kyle-sexton's task in 1m 23s —— View job Security Review
Skip gate triggered — no security-relevant surface. Reviewed the full diff at HEAD
No The delta since the last-reviewed head is one commit, Checked against this lane's criteria regardless:
No security findings to report for this PR. |
0bcebe4 to
c6184fd
Compare
|
Claude finished @kyle-sexton's task in 1m 8s —— View job Security Review
Skip gate triggered — no security-relevant surface. Reviewed the full PR diff (
No The delta since the last-reviewed head ( Checked against this lane's criteria regardless:
No security findings to report for this PR. |
…3173 #3173 merged part 1 of #3127 while this branch was open, correcting the `--config` claim in the same three docs. Its version is good and in places cleaner than this branch's, so rather than resolve eleven commits of conflicts, the branch is rebuilt on `main`: every part-1 rewrite here is dropped in favour of what shipped, and `extensibility-contract- smoke-tests.md` is now untouched entirely. What remains is the five things #3173 deliberately left open. **The one part-1 delta that survives.** Both playbook steps pair a `--scope project` example with the rerun-writes claim citing smoke-test C but never stating its conditions. The 2.1.240 verification was `-s user` with a non-sensitive option; `project`/`local` were never run. Reviewers raised this six times against this branch's own earlier wording, and the shipped text has the same gap. Both sites now state the scope inline, name the shown `--scope project` as untested, and route the reader to a readback — at the location step 3 records, attributed rather than asserted, since that record generalises from `--scope local` runs alone. **Part 2 — the setup contract's read-back clause.** Stated as a property of the setup SKILL, not the `apply` verb: five plugins ship a setup skill with no `apply`, so an `apply`-scoped clause would be false on arrival. Carries the false-failure hazard the merged setup skills already describe — the write can succeed while the running session reports the old value, and calling that a failed write is the error. Both timing mechanisms are marked untested, with what smoke-tests B and D actually recorded stated rather than implied. Part 2's coverage half is already on `main` via #3115 and is not re-landed. **Part 4 — evals.** The hook-plugin skip rested on "no model-invoked skill", false for a hook plugin shipping a setup skill. Narrowed to what its rationale supports, with the gate recorded as operative and the residual pure-reference conflict routed to #3135. **Part 5 — org-agnosticism** gets a named home, declared a normative target whose enforcement reaches a strict subset, naming what is ungated rather than implying coverage. #3136. **Part 3 — `.claude/source-control.md`** declares the sections the gate actually enforces, read from the reusable at the pinned SHA. Part 6 needs no doc change: the check-only carve-out already names the `userConfig` exemption, so `dometrain` and `miro` conform. The real gap is validator-side and is #3137. Refs #3127 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
372ac4a to
2f8f7c1
Compare
|
Claude finished @kyle-sexton's task in 58s —— View job Security Review
Skip gate triggered — no security-relevant surface. Reviewed the full PR diff ( No Checked against this lane's criteria regardless:
No security findings to report for this PR. |
|
No description provided. |
Closes #3182 ## Summary Post-merge verification of #3139 (merged as `ef4d53959`) found three defects in the prose it shipped, each the same class that PR existed to correct: **a claim or rule reaching past what backs it.** They were filed rather than quietly patched because the content was already on `main`. This fixes those three plus the smaller items #3182 lists, in 46 added lines across three files. ## Fix **1. The fetch prohibition's stated cause did not entail its stated rule.** `PLUGIN-PHILOSOPHY.md` sanctioned "a documentation URL", then condemned any skill instructed to fetch it as having "made the publisher a runtime dependency". Fetching `code.claude.com` creates no such dependency, and a practice already shipping in the tree was condemned by it. The prohibition now turns on the target's owner and reaches publisher-owned targets only. For those targets, distinguishing an instruction to fetch from a citation offered for a reader is genuinely hard, and the statement says so rather than implying it has been settled — `plugins/architecture/reference/topic-docs.md` is named as the open case, and no ticket owns it (#3136 is enforcement-site consolidation, not this). **2. The `evidence-bearing` bullet was unsatisfiable as worded.** It required setup to report "the effective value it observed", while the same section pins *effective value* to running-session behaviour and directs verification to a fresh session. A same-session run can only observe the **stored** value. One word: `effective` → `stored`. **3. A narrowing presented as a faithful clarification.** The hook-plugin eval skip stated its rationale as "no model-facing skill at all" where the prior text said "no model-**invoked** skill". Neither works: a `setup` skill sets `disable-model-invocation: true`, so either phrasing is satisfied by a plugin that ships one — admitting as skips exactly the plugins the rest of the rule excludes. The defect was stating the condition in terms of invocation mode at all. It now reads **"no skill carrying a judgment-bearing contract"**, the test the warrant rule two sentences above already uses. Outcome unchanged: 19 hook plugins ship a setup skill, all 19 carry setup evals. **Smaller items.** Both paired reconfiguration sites in `MIGRATION-PLAYBOOK.md` now name the readback location and agree in substance, including the sensitive-value limit — an asymmetry between them would have sent a reader reconfiguring a sensitive option at project scope to look in user settings, find nothing, and report a failed write, which is the false failure `PLUGIN-PHILOSOPHY.md` exists to prevent. Their provenance cites seam 1, which documents both halves, rather than smoke-test C, which explicitly disclaims covering a sensitive option. The "step 3 above" cross-reference — which pointed from inside Reintegration's step 1 at Reintegration's own step 3, about verify-before-retiring — is replaced by a direct citation of seam 1. The `github.test.sh` sweep's wider/narrower axes are named, and "both steps of the same job" is corrected to "each running in its own step". The workflow header's gate description is corrected: the pinned reusable requires four sections, not a closing keyword plus `## Related`. ## Verification Local gates at the final commit: `markdownlint-cli2` 0 issues; `check-contract-clause-coverage.py` exit 0; `lychee --offline` 0 errors across 103 unique links; `zizmor` no findings. The workflow change is comment-only, confirmed by diff. Six fresh-context verification passes, each given the bounded criteria plus an unbounded criterion instructing it to hunt for claims reaching past their evidence anywhere in the touched paragraphs. **All six returned FAIL**, and each round's fixes introduced at least one new instance of the defect being repaired. The sixth pass found two, both in the single paragraph this PR had to *write* rather than cut, and both repairs were deletions: a tracker-wide "no ticket owns that question" that the tracker contradicts (#432 carries an accepted ruling on it, and `scripts/skill-portability-tokens.txt` stages a lint class blocked on that ruling), and a hedge that denied the statement its own preceding clause had just made. Every deletion from the prior round verified clean against the tree. Reviewing where the findings came from settled the approach. Items 2 and 3 were clean from round three onward; essentially every finding from round two on landed in material added *beyond* what #3182 asked for — an enumeration of nonconforming instances, a paragraph grounding the prohibition against the tree, a rewritten security rationale, a sensitive-value carve-out. Each was written to close the previous round's finding and opened one or two of its own. The final revision deletes those elaborations rather than repairing them again, which is why the diff is 46 lines rather than the 2,926 it peaked at. Twenty-five instances of the defect class were found across the five rounds. One was caught by the author re-reading their own writing, four by the review bots, and the rest by fresh-context verification. **None by self-review.** Two things generalise. A verifier is bounded by its criteria, so a defect nobody names survives any number of green passes — every round's findings came from the unbounded criterion, not the checklist. And under-claiming is not the safe direction: round four's findings were mostly repairs to what round three's *removals* broke. Both directions are the same failure to say exactly what the evidence supports. One pre-existing defect is deliberately left alone and filed as #3184: the workflow header's security rationale ("reads PR body metadata from the event payload only") is false against the pinned reusable, which live-refetches. It is outside #3182's scope, the `zizmor` suppression it backs is independently sound, and three separate rewrites of that comment block each introduced a new inaccuracy. ## Related - #3182 — the issue this closes; its line references were verified against `origin/main` at `ef4d53959`. - #3139 — introduced this prose; its own post-merge verification found these defects and filed them rather than patching silently. - #3184 — the workflow-comment defects this PR deliberately did not rewrite. - #3173 — shipped part 1 into the same files; item 3's prior wording is its text. - #3136 — enforcement-site consolidation; it does not own the cite-versus-fetch question, and this PR no longer claims it does. - #3115 / #3116 / #3148 — the rest of the campaign whose doc half #3139 was. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #3136 ## Summary Org-agnosticism was enforced by two independent token sets in the same `plugin-gate` job, plus a stricter autonomy sweep, none of which shared data. The doctrine also named classes the gates did not check. ## Fix `scripts/org-agnosticism-tokens.txt` is the single token file. `validate-plugin-contracts.mjs` reads `fleet-id`, `fleet-key`, `setup`, and `autonomy` from it. `plugins/github/github.test.sh` stays the documented `github` extension (melodic/medley/pulumi plus github-iac); the validator fails if that regex drifts. Enforcement is deliberately those classes — agent content, schema files, and a fleet-wide bare organization name stay ungated. `docs/PLUGIN-PHILOSOPHY.md` now states that scope. The staged portability-lint publisher class is pointed at this file so it cannot become a third set. ## Verification - `node scripts/validate-plugin-contracts.mjs` — 51 setup skills and 2912 plugin files checked - `bash plugins/github/github.test.sh` — PASS=36 FAIL=0 - `scripts/check-changelog-parity.sh --check-bump origin/main` — pass ## Related Refs #3127, #3139, #3115, #3182 --------- Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
1 participant
Closes #3127
Summary
Parts 2–6 of #3127. Part 1 — the unstamped
--configclaim across three docs — shipped separately in #3173 while this branch was open, and #3173 deliberately did not close the issue. This PR was rebuilt on top of it: every part-1 rewrite here was dropped in favour of what shipped,docs/extensibility-contract-smoke-tests.mdis no longer touched at all, and three files remain.One part-1 delta survives, because #3173's text has a gap this branch's reviewers had already found six times — see the first item under Fix.
Fix
The surviving part-1 delta — scope qualifier at two
--scope projectsteps. BothMIGRATION-PLAYBOOK.mdreintegration/onboarding steps pair a--scope projectinstall example with the rerun-writes claim, citing smoke-test C without stating its conditions. The 2.1.240 verification was-s userwith a non-sensitive option;projectandlocalwere never run. Both sites now state the scope inline, name the shown--scope projectas untested, and route the reader to a readback — at the location step 3 records, attributed rather than asserted, since that record itself generalises from--scope localruns alone (smoke-test A). No project-scope install was run to make the sentences true; the qualifier is the fix.Part 2 — the setup contract's read-back clause (
docs/PLUGIN-PHILOSOPHY.md§ Setup is explicit and repeatable). #3115 converged its setup skills on identical wording with no contract to derive it from. The clause is stated as a property of the setup skill, not of theapplyverb — five plugins ship a setup skill with noapply, so anapply-scoped clause would be false on arrival, which is the defect #3115's own fourth commit existed to fix.It carries the false-failure hazard the merged skills already describe: the write can succeed while the running session still reports the old value, and reporting that as a failed write is the error the clause prevents. Both timing mechanisms are marked untested, with what smoke-tests B and D actually recorded stated rather than implied.
Part 2's coverage half is already on
mainvia #3115 ("Stated as fleet coverage") and is not re-landed here — one rule, one place.Part 3 —
.claude/source-control.md.pr_body_required_sectionsset to Summary / Fix / Verification / Related, read from the enforced gate's ownrequiredSectionsarray at the SHA the caller pins. "Test plan" appears nowhere in the gate. The file records that the gate is the authority, that the caller exemptsdependabot[bot]and no other author, and to read the reusable at the pin. Two adjacent corrections: the intro claimed only one key was set here while five are, and the new prose sits above the first## <key>H2 so the list-valued key's grammar stays intact.Part 4 — evals doctrine vs the evals gate + #3135. The hook-plugin eval skip rested on a rationale — "no model-invoked skill" — false for a hook plugin shipping a
setupskill, which the same paragraph calls warrantable. Narrowed to the case its rationale describes. The gate is then recorded as operative rather than left to win silently:check-changed-skills.shrequires evals for any touchedSKILL.md,check-skill.shhard-FAILs for any shape, and the one CI caller cannot override it. The pure-reference skip remains a live contradiction; #3135 carries the two exits.Part 5 — org-agnosticism + #3136. The rule had no named home. It now has one, declared a normative target whose enforcement reaches a strict subset — naming what is ungated (the bare org name fleet-wide, agent content entirely) rather than implying coverage, exactly as the setup contract below already models. The sanctioned use is a citation that names a source rather than a target the plugin acts on, with the boundary drawn explicitly: prose citing a URL is conforming, a skill instructed to fetch or write to it is not.
Part 6 — no doc change owed.
applyis not unconditionally mandatory: the check-only carve-out already names NativeuserConfigas a qualifying surface, anddometrain/miroown no writable artifact, so both conform. The residual gap is validator-side —validate-plugin-contracts.mjspasses any skill whose body contains the substringcheck-onlywithout checking the precondition. Filed as #3137.Verification
Three files changed; nothing under
plugins/**orscripts/**.markdownlint-cli2@0.23.2with the repo config over all three — 0 issueslychee --offline --config lychee.tomlover all three — 0 errorscheck-contract-clause-coverage.py— PASS;node scripts/validate-plugin-contracts.mjs— PASS (50 setup skills, 2817 files);check-changelog-parity.sh --check— PASS;check-skill-count-claims.sh --check— PASSgh apion the reusable at the caller's pinned SHA, read out of itsrequiredSectionsarray. This body is the test: it carries exactly those four headings.On verification itself — the honest record. This branch shipped eight instances of the defect it exists to correct: a claim outrunning its evidence. Six were caught by human review (one finding, raised across six rounds — an unqualified "verified" beside an untested scope). Two were caught by an unbounded verification criterion instructing the verifier to hunt for what no named criterion covers. None was caught by self-review.
The transferable lesson is not "the verifier was weak". Five bounded passes returned PASS while the scope defect was live, because scope was not among the criteria they were handed: a verifier is bounded by its criteria, so a defect nobody thought to name survives an arbitrary number of green passes. Re-verification now carries the scope criterion explicitly and an open-ended hunt — which is what caught instances seven and eight.
Related
visualizationsetup skill was removed under the trivial test. Part 2's read-back clause is the contract its skills had no home to derive from.plugins/claude-ops/andplugins/source-control/; no file overlap with this PR.🤖 Generated with Claude Code