Skip to content

feat(fx-dev): offer duvet adoption from setup and upgrade - #22

Merged
fx merged 4 commits into
mainfrom
feat/setup-duvet-adoption
Aug 10, 2026
Merged

feat(fx-dev): offer duvet adoption from setup and upgrade#22
fx merged 4 commits into
mainfrom
feat/setup-duvet-adoption

Conversation

@fx

@fx fx commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Why

PR #21 taught fx-dev:spec-writer to switch into duvet mode when .duvet/ exists, but nothing in the toolchain ever creates .duvet/. Traceability was reachable only by hand-building the config, so the mode switch sat behind a door with no handle. This adds the missing step: setup and upgrade each notice when a repo has no .duvet/, offer adoption, and on acceptance follow one shared procedure.

The offer is a short deferring step in each skill; the procedure itself lives once in plugins/fx-dev/skills/setup/references/duvet-adoption.md. setup/references/instruction-files.md set that precedent — a reference file owned by one skill and pointed at from elsewhere — and duplicating a multi-step install into two skills would drift within a release. Single ownership is enforced in fact, not just intent: cargo:duvet and awslabs/duvet appear zero times in either SKILL.md.

Adoption gates on .duvet/ at the repository root, resolved with git rev-parse --show-toplevel rather than cwd. Already adopted means say nothing and do nothing. Otherwise: install duvet (merging rust and cargo:duvet into an existing mise config, or asking how to proceed when there is none), verify duvet runs, establish [[source]] patterns, scaffold .duvet/config.toml, bootstrap the snapshot, gitignore the regenerated directories while committing snapshot.txt, and wire CI when .github/workflows/ exists.

Design decisions

.duvet/ existence is the adoption flag, matching spec-writer's gate from #21. Offering adoption belongs to setup and upgrade; spec-writer must never create .duvet/ itself, or the flag would set itself as a side effect of writing a spec.

The install path is a runtime question, not baked-in policy. With no mise config present the user is asked — create one, cargo install, or abort — rather than having a default imposed on their toolchain.

Adoption is opt-in behind an explicit prompt. That is precisely what separates it from upgrade's standing refusal to silently scaffold a documentation system: the distinction is consent, not scope. Adoption is in fact the larger change of the two, and it is allowed only because it is named, described, and declinable.

Any failure part-way through is a loud error. Because .duvet/ existing is what flips spec-writer into duvet mode, a half-adoption is not a harmless no-op, and the procedure calls that hazard out explicitly.

Versions: fx-dev 3.6.0 → 3.7.0, marketplace 0.19.0 → 0.20.0. Minor — new capability, backward compatible.

Review findings fixed

A pre-PR /code-review found 13 findings, 2 critical, all fixed in the second commit. The two criticals are the substance of this PR:

  1. CRITICAL — duvet --version is not a valid duvet 0.4.3 invocation. It exits 2; the CLI requires a subcommand. The verification step would therefore have failed on a correctly installed duvet and aborted every single adoption — after mise.toml had already been edited. Now uses duvet help. Independently confirmed: duvet help exits 0, duvet --version exits 2.
  2. CRITICAL — the scaffolded config had no [[source]] stanza, making duvet query -c implementation unsatisfiable forever once the first spec landed. Proven by control experiment: two identical repos differing only by the [[source]] block give Fully implemented: 1 / PASS versus Not implemented: 1 / FAIL (exit 1). Adoption now infers candidate patterns from the languages present and confirms them with the user, aborting rather than writing a config that can never pass.
  3. HIGH — appending a [tools] header to an existing mise.toml is a TOML duplicate-key error, which makes mise load no tools for the repo at all. The snippet is reduced to bare keys, with rules for leaving existing rust / cargo:duvet pins alone.
  4. HIGH — duvet is cwd-relative, so from a subdirectory the bootstrap passed vacuously: Loaded 0 specifications, no snapshot written, both gates exit 0 — and the user's first CI run fails. All duvet commands now run from the resolved repo root.
  5. MEDIUM — the invocation verified in step 2 is carried into later steps, since the mise path is not on PATH non-interactively.
  6. MEDIUM — the no-CI branch no longer half-adopts silently; it ends in a keep-or-revert prompt, and the success report's CI line is conditional.
  7. MEDIUM — upgrade ran adoption after its final report; adoption and reporting are now swapped.
  8. MEDIUM — setup could report "no changes needed" after adoption wrote five files.
  9. MEDIUM — a decline cannot be persisted, so the offer recurs once per session. Stated plainly now instead of claiming "no nagging", with a durable opt-out recorded as an open question.
    10-13. LOW — callers claiming a single prompt; mise config detection covering only 2 of 11 supported paths; two now-false upgrade invariants; an unqualified .gitignore path.

Test plan

  • duvet query -c implementation — exit 0, 16/16 unchanged (this PR adds no requirements)
  • duvet report --ci — exit 0
  • All 6 fenced toml/yaml blocks across the three files parse (tomllib / yaml.safe_load), plus both SKILL.md frontmatters
  • End-to-end in a throwaway git init repo using the reference's verbatim config: spec-less bootstrap → all gates exit 0; with a spec plus annotated source → Fully implemented: 1 / PASS
  • Control: the same repo with only [[source]] removed → FAIL, query exit 1 — proving finding 2
  • Two-block multi-comment-style run (#=/#% and duvet=/duvet#) → Fully implemented: 2 / PASS
  • duvet help exits 0; duvet --version exits 2 — verified independently
  • Single ownership: cargo:duvet appears 0 times in both SKILL.md files
  • Marketplace validate job passes (verified by this PR's CI)
  • Requirements traceability job passes (verified by this PR's CI)

Not in this PR

Teaching the implementing skills to establish duvet traces is a separate change. This PR only makes adoption possible and offers it.

Follow-ups

  • Next change: implementing skills (coder, orchestrated by dev / team / fix) establish duvet traces. Annotation mechanics belong there.
  • fx-dev:copilot-review has three defects, all of which make it under-report review state: the review-request POST is inert (200 with empty requested_reviewers, confirmed 5 times); the ruleset fallback is unreliable (fired rounds 1-2, not round 3); and "generated no comments" is not proof of clean — two reviews hid real findings inside Suppressed comments blocks that create no thread. Arguably higher priority than the trace work, since it is a merge gate that can lie.
  • Durable opt-out for a duvet decline (currently recurs once per session).
  • team/SKILL.md self-contradicts on commit authority; dev and team disagree on human review threads.
  • AGENTS.md lacks a /project-management marker; REVIEW.md lacks a docs/changes/ marker.

Review coverage caveat: local cr and codex remain unauthenticated, so two of the four intended pre-PR review passes did not run. Claude Code review and the duvet gates did.

fx added 2 commits August 10, 2026 08:11
`.duvet/` at the repo root is what flips spec-writer into duvet mode, but
nothing ever created it — a repo could only adopt traceability by hand.

Add `setup/references/duvet-adoption.md` as the single owner of the adoption
procedure: gate on `.duvet/` at the git root, offer once, then install duvet
(mise `cargo:duvet` if a mise config exists, otherwise ask), verify it runs
BEFORE writing anything, scaffold `.duvet/config.toml` with no
`[[specification]]` entries, bootstrap the snapshot, gitignore the regenerated
artifacts, and wire the two CI gates.

Bootstrapping is mandatory rather than optional: `duvet report --ci` exits 1 on
a repo with no snapshot, so without it the first CI run fails. Both CI commands
are needed because `--ci` skips coverage enforcement when a snapshot is
configured, leaving `duvet query -c implementation` as the real gate.

setup Step 9.5 and upgrade Step 7 only offer and defer — the procedure is stated
once so the two cannot drift. Ordering is chosen so a failure never leaves a
half-adopted repo: `.duvet/` appears only after duvet is verified runnable,
because its mere existence makes spec authoring stricter with nothing enforcing
it yet.

fx-dev 3.6.0 -> 3.7.0 and marketplace 0.19.0 -> 0.20.0: new capability in two
skills, no existing behaviour changed or removed.
Adoption had two defects that made it fail for every user, plus reporting
and ordering bugs that hid partial adoptions.

Blocking:

- `duvet --version` is not a valid duvet 0.4.3 invocation — it exits 2 on a
  working install, so the step-2 verification gate failed for everyone and
  aborted adoption after step 1 had already edited the mise config. Use
  `duvet help`, and fix the two fallbacks that shared the flag.
- The scaffolded config had no `[[source]]` stanza, which makes
  `duvet query -c implementation` unsatisfiable forever: duvet parses
  annotations only from matched files. A spec-less repo passes vacuously, so
  the failure surfaced the day the first spec landed and was unfixable by
  annotating. spec-writer only reports `[[source]]` gaps, never supplies
  them. Adoption now infers candidate patterns from the languages present,
  confirms them with the user, requires a `comment-style` where the language
  needs one, and aborts rather than writing a config that cannot pass.

Also:

- Merge the install keys INTO the existing `[tools]` table instead of
  appending a second `[tools]` header, which is a TOML parse error that makes
  mise load no tools at all. Leave an existing `rust` pin alone, and pin
  rather than float both versions.
- Require every duvet command to run from the repository root. duvet is
  cwd-relative and does not search upwards, so from a subdirectory it loads 0
  specifications, writes no snapshot, and still exits 0 — adoption reported
  success and CI failed on the first run.
- Carry the invocation resolved in step 2 into every later duvet call, so the
  mise path does not die with command-not-found after `.duvet/` exists.
- End the no-CI branch in an explicit keep-or-revert question instead of
  reaching half-adoption silently, and make every line of the success report
  conditional on what was actually written.
- Root-qualify the `.gitignore` step like its neighbours.
- Detect all eleven mise config paths, not just two.
- State honestly that a decline persists for the session only, and record a
  durable opt-out as an open question rather than inventing a config key.
- setup: report duvet in step 10 instead of short-circuiting to "no changes
  needed" over five new files.
- upgrade: run adoption before the report so `git status` covers it, correct
  the idempotency invariant, and explain why an opt-in CI job is allowed
  where silently scaffolding docs/ is not.
- Correct both callers' claim that the offer is a single AskUserQuestion.

Verified: duvet query -c implementation and duvet report --ci both exit 0 on
this repo (16/16). In a throwaway repo, the prescribed config bootstraps and
both commands exit 0; adding a spec and an annotated source keeps them at 0,
while deleting only the `[[source]]` block flips query to exit 1.
Copilot AI lite review requested due to automatic review settings August 10, 2026 15:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes duvet requirements-traceability adoption discoverable by having fx-dev:setup and fx-dev:upgrade detect the absence of .duvet/, offer opt-in adoption, and then defer to a single shared procedure document so setup/upgrade don’t duplicate multi-step guidance.

Changes:

  • Add an opt-in duvet adoption offer to setup (Step 9.5) and upgrade (Step 6), gated by .duvet/ at the repo root.
  • Introduce a single-owner shared procedure at skills/setup/references/duvet-adoption.md to keep adoption steps consistent across callers.
  • Bump fx-dev plugin and marketplace versions to reflect the new capability.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
plugins/fx-dev/skills/upgrade/SKILL.md Adds duvet adoption offer step and updates flow so reporting covers any adoption writes.
plugins/fx-dev/skills/setup/SKILL.md Adds Step 9.5 duvet adoption offer and strengthens reporting requirements when adoption occurs.
plugins/fx-dev/skills/setup/references/duvet-adoption.md New canonical adoption procedure (install, verify, config scaffold, snapshot bootstrap, gitignore, CI wiring).
plugins/fx-dev/.claude-plugin/plugin.json Bumps plugin version 3.6.0 → 3.7.0.
.claude-plugin/marketplace.json Bumps marketplace metadata version 0.19.0 → 0.20.0.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread plugins/fx-dev/skills/upgrade/SKILL.md
Comment thread plugins/fx-dev/skills/setup/references/duvet-adoption.md
Comment thread plugins/fx-dev/skills/setup/references/duvet-adoption.md
Comment thread plugins/fx-dev/skills/setup/SKILL.md
fx added 2 commits August 10, 2026 08:44
…gaps

Four defects found in Copilot review of the duvet adoption work:

- upgrade/SKILL.md M1.6 pointed uncertain task-tracking rules at the "Step 6
  report"; renaming the report step 6 -> 7 to make room for the duvet offer
  left this reference aimed at the offer step. Now Step 7.
- duvet-adoption.md step 3e wrote a bare `duvet report` into the user's
  .duvet/config.toml header, which is command-not-found for a contributor on
  the mise install path in a non-activated shell. The comment now names the
  `mise exec -- duvet` and `~/.cargo/bin/duvet` forms, and step 2's `$duvet`
  rule enumerates this as the second explicit exception alongside the CI
  workflow, so its count is accurate again.
- duvet-adoption.md step 6 hardcoded `branches: [main]` on the workflow's push
  trigger, so on a master/develop repo the post-merge snapshot-drift run never
  fired. Flagged as a placeholder to substitute, mirroring step 1's Rust pin.
- setup/SKILL.md step 10 printed a literal `Duvet adopted:` header and then
  told the reader to insert the reference's report block, whose first line is
  that same header. Now a placeholder only, matching upgrade/SKILL.md and
  leaving the block single-owned by the reference.
@fx
fx requested a lite review from Copilot August 10, 2026 15:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (1)

plugins/fx-dev/skills/setup/references/duvet-adoption.md:601

  • Typo in the prompt text: the string contains entries<, which will be rendered verbatim in the AskUserQuestion option description and is likely confusing. Remove the stray < and adjust punctuation so the sentence reads cleanly.
      description: "Delete .duvet/, undo the .gitignore entries<, and undo the [tools] edits in <mise config path> — include this clause ONLY if step 1 took the mise path>. spec-writer returns to its normal mode"

@fx

fx commented Aug 10, 2026

Copy link
Copy Markdown
Owner Author

Copilot review at a60e803 — suppressed-comment triage

Copilot reviewed 6/6 files at a60e803 and its body says it "generated no new comments" — but it also carries a <details><summary>Suppressed comments (1)</summary> block, which creates no review thread and so appears nowhere in a thread query. Recording it here, since there is no thread to resolve it on.

Suppressed finding: duvet-adoption.md:601entries<,

Typo in the prompt text: the string contains entries<, which will be rendered verbatim in the AskUserQuestion option description […] Remove the stray < and adjust punctuation.

Classification: incorrect as diagnosed — no change made. The < is not stray and is not a typo; it is the opening delimiter of a conditional span, and the suggested fix would break the line.

The full string is:

"Delete .duvet/, undo the .gitignore entries<, and undo the [tools] edits in <mise config path> — include this clause ONLY if step 1 took the mise path>. spec-writer returns to its normal mode"

The span runs from the < after entries to the final >, and it is opened before the comma on purpose so that punctuation is correct on both install paths:

  • mise path (clause kept) → "…undo the .gitignore entries, and undo the [tools] edits in mise.toml. spec-writer returns…"
  • cargo install path (clause dropped) → "…undo the .gitignore entries. spec-writer returns…"

Moving the < after the comma would strand a comma before a period on the cargo path. Removing it, as suggested, would leave the span unopened and leak the meta-instruction — "include this clause ONLY if step 1 took the mise path>" — verbatim into a user-facing prompt, which is the precise defect Copilot thought it was reporting. The angle-bracket-instruction convention is used throughout this file (<mise config path>, <CI line — exactly one of:>, <the confirmed patterns, with comment styles>) and in upgrade/SKILL.md:281, and line 604 states the conditionality in prose immediately below.

The underlying readability concern is fair, though, and worth its own note: nesting <mise config path> inside an outer <…> span makes the delimiters genuinely hard to parse — Copilot misreading it is the evidence. Not fixed here because it is cosmetic and this branch is already three rounds deep in fix-induced regressions; logged as a follow-up rather than a fifth same-file edit.

Independent scrutiny of the two fix rounds

The [tools] branching (a60e803) and the conditionalised failure/revert text were re-read specifically, since each of the last two rounds introduced a defect. No blocking finding. One nit, not fixed:

  • Branch A/B is selected by grep -n ^\[tools\]. A config whose only tools table is a [tools.<name>] sub-table correctly routes to branch B (documented at 171-173, and a super-table after a sub-table is valid TOML). The uncovered shape is [tools.rust] specifically: branch B says to add "the two keys", and adding a bare rust key when tools.rust is already a table is a type conflict — the same "mise loads no tools at all" outcome the file warns about at 189-192. The general rule at 194 ("if rust is already pinned, leave it exactly as it is and add only cargo:duvet") does yield the correct behaviour for a reader who applies it, so this is a precision gap in the example rather than a defect. [ tools ] with inner whitespace is legal TOML that the grep also misses, on the same theory.

Verified clean: all 23 Step N cross-references in upgrade/SKILL.md against actual headings; Step 6 report zero hits; Duvet adopted: zero hits in setup/SKILL.md; cargo:duvet and awslabs/duvet still zero hits in both SKILL.md files (single ownership holds); version pins (0.4.3, 1.97.1, action v0.4.2, schema v0.4.0.json) mutually consistent; every "if that fails, ERROR" path still terminates, with step 2's failure text, step 3b's abort and step 6's revert prompt all now conditional on which install path step 1 took.

Gate status: Copilot has reviewed a60e803 (submitted 15:54:20Z, ~2 min after the request) and all 4 pre-existing threads are resolved. 0 unresolved threads. No code changes were made in this round, so the reviewed SHA is still the head.

@fx
fx merged commit 5a552d4 into main Aug 10, 2026
3 checks passed
@fx
fx deleted the feat/setup-duvet-adoption branch August 10, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants