feat: check-only setup carve-out for userConfig-only plugins (bug-report, miro) - #361
Conversation
…ort, miro) Closes the wave #314 design question the issue asked to raise before mass-refactoring: where a plugin's entire configuration surface is native userConfig, an apply action would have nothing to write except the pluginConfigs the setup contract forbids. PLUGIN-PHILOSOPHY's setup section gains the one-line sanction: a check-only setup is conforming there — check verifies and reports, reconfiguration routes through the native flow. bug-report 0.5.0: check is the sole action; the interactive "ask whether" step becomes a stated tradeoff + recommendation; reconfigure guidance states --config's fresh-install-only semantics; rechecks defer to a fresh session. miro 0.2.0: check [verify-api] — the optional read-only credential probe becomes an explicit argument instead of an in-flow question; MCP server version realigned across package.json, Implementation version, lockfile, and rebuilt bundle (drift check green). ## Related - Part of #314 - Part of #313 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 17s —— View job PR Review: feat: check-only setup carve-out for userConfig-only plugins
SummaryThis PR is clean, well-scoped, and correctly implements the check-only setup carve-out doctrine for two userConfig-only plugins. No blocking issues found. A few minor observations are noted below.
|
| Location | Before | After |
|---|---|---|
plugin.json |
0.1.2 |
0.2.0 ✅ |
package.json |
0.1.2 |
0.2.0 ✅ |
package-lock.json |
0.1.2 |
0.2.0 ✅ |
src/index.ts McpServer |
0.1.2 |
0.2.0 ✅ |
dist/index.min.js (bundle) |
0.1.2 |
0.2.0 ✅ |
Security boundaries unchanged and correct: token never read/echoed/logged, no mutating Miro tools called during setup, pluginConfigs never touched. ✅
No blocking issues. ✅
Verdict
Approve. The doctrine change is minimal, precise, and correctly positioned in the philosophy doc. Both plugin updates faithfully implement the check-only carve-out and the non-interactive contract. The miro version drift is fully resolved. The PR achieves its stated scope (29/31 wave conformance) and correctly defers the remaining two.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6e87c335c
ℹ️ 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".
| description: "Configure and verify the Miro plugin without reading or exposing its API token. Use when: 'set up Miro', 'configure Miro', 'Miro setup', the Miro MCP server is unavailable, or a Miro tool reports an authentication error." | ||
| argument-hint: "(no arguments — interactive configuration and verification)" | ||
| description: "Verify the Miro plugin without reading or exposing its API token. Use when: 'set up Miro', 'configure Miro', 'Miro setup', the Miro MCP server is unavailable, or a Miro tool reports an authentication error. Actions: check (read-only verification, default and only action — this plugin's entire configuration is native userConfig, so there is nothing an apply could write); check verify-api additionally authorizes one read-only API call." | ||
| argument-hint: "check [verify-api]" |
There was a problem hiding this comment.
Document the
verify-api invocation
The plugin README still tells users to run /miro:setup and then provide an explicit confirmation for an API check (plugins/miro/README.md:29-31), but this change explicitly forbids in-flow confirmation and only runs the probe when the command receives verify-api. Consequently, users following the documented setup flow can no longer perform the advertised credential check; update the README to show /miro:setup check verify-api.
Useful? React with 👍 / 👎.
| 5. Optional credential check — only when the invocation passed `verify-api` (the explicit opt-in; | ||
| never offer it as an in-flow question): call the read-only `miro_list_boards` tool with | ||
| `limit: 1`. Never create, update, or delete a board during setup. |
There was a problem hiding this comment.
Update the Miro setup API-probe evaluation
The existing second setup eval invokes /miro:setup followed by natural-language verification text and expects an in-flow confirmation prompt before miro_list_boards. Under this new rule, that invocation lacks verify-api, so the skill must neither ask nor call the API and the eval no longer tests valid behavior. Change the eval prompt and expectations to use check verify-api and assert the direct read-only probe.
Useful? React with 👍 / 👎.
| 3. State the tradeoff instead of asking: machine-private (the default under | ||
| `${CLAUDE_PLUGIN_DATA}`) versus a repository path committed alongside code. For the |
There was a problem hiding this comment.
Update the bug-report setup evaluation
The first setup eval still expects the skill to ask whether reports should stay private or use a repository location (plugins/bug-report/skills/setup/evals/evals.json:8), but this change replaces that question with a required non-interactive tradeoff statement. The evaluation is now asserting the retired behavior, so it can reward a regression and should instead assert that the recommendation is reported without blocking on a question.
Useful? React with 👍 / 👎.
) ## Summary Closing tranche of wave #314 (epic #313, dim 8) — with this PR, every plugin in the fleet conforms to the uniform setup contract and the contract is machine-enforced. - **disk-hygiene 0.4.0** — setup skill created (deferred until #286 landed): `check` probes Python 3.11+ (absolute-interpreter note), conditional Git, and the current OS family's documented lane (Linux `lsof` optional-execution note, macOS audit-only by design as INFO); guidance-only `apply`; disabled-toggle downgrade. - **guardrails 0.7.0** — setup skill created (deferred until #305 landed): `check` reads the guard scripts + `hooks.json` as truth and probes Bash 5.0+, `jq` (absence = every guard fails open — surfaced as FAIL), all eight guard toggles, the `cli-flag-verify` scan surface, and the `block-dangerous-git` allowlist; guidance-only `apply`. - **review 0.14.0** — its setup sat outside the wave list and was caught by the new gate on first run: adopts `check | apply` while keeping the standards-contract binding's by-reference discipline and explicit-confirmation gates intact. - **Contract gate** — `validate-plugin-contracts.mjs` now enforces the shape on every `skills/setup/SKILL.md`: `check` leads the `argument-hint`, the read-only check action is documented, and `apply` exists unless the skill declares the doctrine's check-only userConfig-only carve-out (#361). 33 setup skills pass fleet-wide; future setups are gated, not hand-reviewed. ## Verification `validate-plugins.sh`, `validate-plugin-contracts.mjs` (33 setup skills, 1541 files), markdownlint, `typos`: green locally. The gate change was validated by its own first catch (review). ## Related - Closes #314 - Part of #313 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Resolves the wave #314 design question (raised in the issue before mass-refactoring bug-report/miro): where a plugin's entire configuration surface is native
userConfig— no tracked project config, no prerequisite an install could resolve — anapplyaction would have nothing to write except thepluginConfigsthe setup contract itself forbids.docs/PLUGIN-PHILOSOPHY.md, setup section): one added sentence sanctioning the check-only shape for exactly that case. Kept in its own PR so the doctrine change is reviewable in isolation.checkis the sole action; the interactive "ask whether reports should be private or repo-resident" step becomes a stated tradeoff with a recommendation (non-interactive); reconfigure guidance states--config's fresh-install-only semantics; rechecks defer to a fresh session since the rendered value is injected at load.check [verify-api]— the optional read-only credential probe (list one board, count only) becomes an explicit argument instead of an in-flow question; token never read, printed, or persisted (unchanged). MCP server version realigned acrosspackage.json, the serverImplementationversion, the lockfile, and the rebuilt committed bundle (drift check + typecheck green; lockfile delta is the two version fields).With this, 29 of 31 wave plugins conform. Remaining: disk-hygiene + guardrails (deferred behind the #286/#305 lanes) and the closing contract-gate rule.
Verification
validate-plugins.sh,validate-plugin-contracts.mjs, markdownlint,typos, miro bundle drift check + typecheck: all green locally.Related
No linked issue: carve-out slice of #314 — the issue closes with the wave's final tranche.
🤖 Generated with Claude Code