diff --git a/docs/results/2026-09-24-revise-a-groomed-change-s-spec-and-owned-sections-through-a-results.md b/docs/results/2026-09-24-revise-a-groomed-change-s-spec-and-owned-sections-through-a-results.md new file mode 100644 index 000000000..e3a760f3a --- /dev/null +++ b/docs/results/2026-09-24-revise-a-groomed-change-s-spec-and-owned-sections-through-a-results.md @@ -0,0 +1,62 @@ + +> ↩ **[Change 0445 — Revise a groomed change's spec and owned sections through a typed operation](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0445-revise-a-groomed-change-s-spec-and-owned-sections-through-a.md)** + +# Revise a groomed change's spec and owned sections through a typed operation — Results + +**Human action:** No action is required before merge. One optional walkthrough below lets you try the new revise path by hand. + +## Outcome + +Before this change, the only way to adjust a change after grooming was to hand-edit its files and commit them directly. That skipped the version check and the `updated:` stamp that every other metadata write gets. + +`change.groom` now accepts a third outcome, `revise`. It works only on a `proposed` change that is already groomed, meaning it has a linked spec or was marked trivial. A revise request can replace the whole spec body (the file stays at its existing path and keeps its backlink block), splice the change's owned sections (Why / What changes / Out of scope / Open questions), or both. Everything lands in one transaction, pinned to the exact version. A revise never writes `spec:` or `trivial:`, so it cannot turn a spec'd change into a trivial one or the other way round. You can revise a change as many times as you like while it stays `proposed`. + +The operation refuses, and writes nothing, in these cases: + +- `not-revisable`: the change is not groomed yet, or is not `proposed`. +- `spec-not-linked`: the request sends spec text for a change that has no spec. +- `spec-file-missing`: the linked spec file is absent. +- `empty-revise`: the request carries no effective edit. + +The result now reports its `outcome` and prints `change NNNN revised — …`. + +The `docket-groom-next` skill now sends an explicit id for an already-groomed change to a revise flow. It used to report an error and suggest clearing `spec:` by hand. `docket-new-change` now points to that same flow for adjusting a spec right after it lands. + +Review found problems that were fixed before the PR opened, and two of the fixes add to the design: + +- **Spec-body revises need a version pin.** A revise that replaces the spec body must also send `spec_version`, the git blob id of the spec the record's `spec:` field links (for example `git -C .docket rev-parse HEAD:`). If the spec changed after you read it, the call returns `contended` with a `spec-version-mismatch` finding and writes nothing, so a concurrent edit can no longer be silently overwritten. Without `spec_version` the request is refused with `empty-spec_version`; `spec_version` on any other request is refused with `invalid-spec_version`. The check runs in the operation's plan step, against the path the record links, so the caller never names the spec path. +- **Identical revises are a no-op.** If the revise would not change the record or the spec (for example, a same-day revise with identical text), the call returns `no-op`. Before the fix, the engine failed it. +- **`spec_markdown` cannot contain a backlink block.** On both `spec` and `revise`, `spec_markdown` that includes a `docket:backlink` block is refused with `invalid-spec_markdown`. The operation stamps that block itself, and a second copy would corrupt the spec file. + +Departure from the plan: the skill-size word budgets in `internal/repoguard/budgets_test.go` were raised: docket-groom-next from 1650 to 1889 words (raised in steps as the review fixes added wording), docket-new-change from 1700 to 1706. The plan prescribed the new prose word for word, and without the raise the guard would fail. + +## Human actions and testing + +### Optional — revise a groomed change by hand + +Run this if you want to see the new path work end to end. It changes metadata, so use a scratch repository and never the real docket backlog. + +Prerequisites: a docket binary built from this branch (`go build -o /tmp/docket-445 ./cmd/docket`) and a disposable repository where docket is initialized and has one `proposed` change with a linked spec. + +1. Write a request file `{"id":,"version":"","path":"","outcome":"revise","sections":[{"heading":"## Why","action":"replace","markdown":"New why text."}]}` and run `/tmp/docket-445 change groom --input req.json --json`. + Expected: `result` is `applied`, and the change's `## Why` now holds the new text. +2. Run the same request again with the new version. + Expected: `result` is `no-op`. +3. Send `spec_markdown` without `spec_version`. + Expected: `invalid-input`, with an `empty-spec_version` finding. + +Cleanup: delete the scratch repository. + +## Verification performed + +Each build task and each review fix ran its own tests through the gate driver, both failing (RED) and passing (GREEN). The new guards were mutation-checked: when a guard was removed, its test failed. The guards covered are the revise gate, the only-changed-paths declarations, the spec version pin (including a real-git race test where a stale version now returns `contended`), the backlink refusal, and the receipt spec_path. + +The first full-suite build gate failed because the embedded skill copies were stale after the skill edits. They were regenerated, and the second run passed (54/54 files). Deep-rung review then returned 6 findings: 1 blocker, 2 important, 3 minor. All six were fixed in-branch, and the full suite ran again on the final head before the PR opened. + +After the PR opened, the human reviewer asked to drop the `spec_path` request field that the version-pin fix had added: it could only ever repeat the record's `spec:` value, and a wrong value produced a confusing error. The spec's blob id is now checked in the plan step against the path the record links. The three new guards (the plan-step version check, the refusal-to-`contended` mapping, and the `invalid-spec_version` shape rule) were mutation-checked, and each removal reddened its tests. + +## Known issues and follow-ups + +### Skill word budgets were raised rather than trimmed + +The docket-groom-next and docket-new-change skill word ceilings were raised to fit the new revise instructions. Status: deliberate. If you would rather keep the skills shorter, trimming the prose is a reasonable follow-up. diff --git a/docs/superpowers/plans/2026-09-24-0445-revise-groomed-change-typed-operation.md b/docs/superpowers/plans/2026-09-24-0445-revise-groomed-change-typed-operation.md new file mode 100644 index 000000000..49f3ec021 --- /dev/null +++ b/docs/superpowers/plans/2026-09-24-0445-revise-groomed-change-typed-operation.md @@ -0,0 +1,829 @@ + +> ↩ **[Change 0445 — Revise a groomed change's spec and owned sections through a typed operation](https://github.com/danielhanold/docket/blob/docket/docs/changes/active/0445-revise-a-groomed-change-s-spec-and-owned-sections-through-a.md)** + +# Revise a Groomed Change Through change.groom Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add a `revise` outcome to the existing `change.groom` operation so an already-groomed `proposed` change's spec body and owned proposal sections can be adjusted through one exact-version CAS transaction, and route the skills that hit this gap to the new path. + +**Architecture:** No new catalog operation and no new CLI verb. `internal/app/change_groom.go` gains a third `GroomOutcome` (`revise`) gated on the exact complement of the existing groom gate (`status==proposed && (spec!="" || trivial)`). It reuses the existing `SpecMarkdown` field (whole spec-body replace, `MutationReplace` at the change's *existing* spec path) and the existing `Sections` splice; it never writes `spec:` or `trivial:`, so spec'd↔trivial flips stay structurally impossible. Skill docs (`skills/docket-groom-next/SKILL.md`, `skills/docket-new-change/SKILL.md` — the repo's own `skills/` sources, never `~/.claude/skills`) replace the documented hand-edit workaround with the typed path. + +**Tech Stack:** Go (packages `internal/app`, `internal/cli`); table-driven tests in `internal/app/change_groom_test.go`; Markdown skill bodies. + +**Spec:** `docs/superpowers/specs/2026-09-24-revise-a-groomed-change-s-spec-and-owned-sections-through-a-design.md` (on the `docket` metadata branch; also readable at `.docket/docs/superpowers/specs/…` from the repo root). + +## Global Constraints + +- `revise` never sets `spec:` or `trivial:` — no code path under the revise branch may call `ps.SetField("spec", …)` or `ps.SetField("trivial", …)` (spec, "Gate" section). +- Spec revise is whole-body replace at the change's **existing** spec path only — never a new path, never a relink, no dated-path minting. +- Repeatable: no one-shot marker; every call is a standard exact-version CAS write (the engine's existing `EntityExpectation`, unchanged). +- Out of scope (do not build): section-level spec patching, revising `in-progress`/terminal changes, autonomous revision, review workflow. +- New refusal codes on the Plan-closure channel (`not-revisable`, `spec-not-linked`, `spec-file-missing`) are plain strings passed to `refuseGroom`, matching the existing `"not-groomable"`/`"spec-path-taken"` house style. New *request-shape* code `FCEmptyRevise` must be added to both the const block and `AllFindingCodes` in `internal/app/finding_codes.go` (the census is guard-tested). +- The `groom_outcomes` schema vocabulary (`internal/app/schema_vocab.go`) must gain `revise`; `TestVocabularyConstCompleteness` holds it in correspondence with the const group. +- Cross-references in maintained source anchor on symbol names or verbatim-quoted clauses, never line numbers (repo AGENTS.md). +- The build gate runs the **whole** suite via the resolved `build.test_command`; per-task runs below are focused checks only, always with `-count=1`. + +## Review Focus + +The spec's enumerated acceptance items are covered task-by-task below. These five spec-implied inputs were *not* enumerated there; each gets a pinned test in the owning task: + +1. **A relationships-only revise request** (`depends_on` set, no `spec_markdown`, no effective section edit) — a caller would expect either a field patch or a clear refusal; the spec's "at least one of spec_markdown/sections" rule makes it a `FCEmptyRevise` refusal, and without a test an implementer could silently count `DependsOn` as an effective edit. → Task 1. +2. **Unparseable `spec_markdown` under revise** — the spec only says "non-empty"; a body that fails `document.Parse` must be refused `invalid-spec_markdown` before the engine, exactly as the `spec` outcome does, or a malformed spec lands in the tree. → Task 1. +3. **A dangling spec link** (`spec:` names a path absent from the tree) — `MutationReplace` against a missing path must not silently mint a file; refuse `spec-file-missing`. → Task 2. +4. **HumanText for an applied revise carrying `spec_markdown`** — the existing `ResultApplied` branch keys on `SpecPath != ""` and would print "groomed (spec …)"; the result must carry the outcome so revise renders "revised". → Task 3. +5. **The outcome enum's other announcement sites** — the `FCInvalidOutcome` message ("one of spec, trivial"), the CLI help line in `internal/cli/change.go` ("spec or trivial"), and the `groom_outcomes` vocabulary must all name `revise`, or the schema/CLI keep denying the outcome exists. → Tasks 1 and 3. + +--- + +### Task 1: `revise` outcome constant, request-shape validation, and finding-code census + +**Files:** +- Modify: `internal/app/change_groom.go` (const block, `validateChangeGroomShape`, new helper `hasEffectiveSectionEdit`) +- Modify: `internal/app/finding_codes.go` (const block + `AllFindingCodes`) +- Modify: `internal/app/schema_vocab.go` (`groom_outcomes` vocabulary) +- Modify: `internal/app/finding_codes_test.go` (`TestShapeValidatorCodesAreRegistered` probe list) +- Test: `internal/app/change_groom_test.go` + +**Interfaces:** +- Consumes: existing `GroomOutcome`, `ChangeGroomRequest`, `SectionEditRequest`, `validateGroomSections`, `render.SectionReplace`/`SectionRemove`, `document.Parse`. +- Produces: `GroomRevise GroomOutcome = "revise"`, `FCEmptyRevise FindingCode = "empty-revise"`, and `hasEffectiveSectionEdit(sections []SectionEditRequest) bool` — Tasks 2 and 3 rely on these exact names. + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/app/change_groom_test.go`: + +```go +// revisableChange renders a proposed change already groomed to a spec: the +// groomable fixture with spec: linked to specPath. +func revisableChange(id int, slug, specPath string) string { + return strings.Replace(groomableChange(id, slug), "spec:\n", "spec: '"+specPath+"'\n", 1) +} + +// trivialChange renders a proposed change already groomed by trivial verdict. +func trivialChange(id int, slug string) string { + return strings.Replace(groomableChange(id, slug), "trivial: false", "trivial: true", 1) +} + +// validReviseRequest is a well-formed revise request (sections + spec body) +// against the revisable fixture at id 2 / slug add-a-widget. +func validReviseRequest() ChangeGroomRequest { + return ChangeGroomRequest{ + ChangeID: 2, + Path: groomPath(2, "add-a-widget"), + Version: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + Outcome: GroomRevise, + SpecMarkdown: "# Design\n\nThe revised design body.\n", + Sections: []SectionEditRequest{ + {Heading: "## What changes", Intent: "replace", Markdown: "Narrowed what.\n"}, + }, + } +} + +func TestChangeGroomReviseShapeValidation(t *testing.T) { + cases := []struct { + name string + mut func(*ChangeGroomRequest) + code string // "" means the request must pass shape validation + }{ + {"valid revise passes", func(r *ChangeGroomRequest) {}, ""}, + {"sections-only revise passes", func(r *ChangeGroomRequest) { r.SpecMarkdown = "" }, ""}, + {"spec-only revise passes", func(r *ChangeGroomRequest) { r.Sections = nil }, ""}, + {"empty revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = nil + }, "empty-revise"}, + {"all-preserve revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = []SectionEditRequest{{Heading: "## Why", Intent: "preserve"}} + }, "empty-revise"}, + {"relationships-only revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = nil + r.DependsOn = []int{1} + }, "empty-revise"}, + {"unparseable revise spec_markdown refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "---\nid: 1\n" + }, "invalid-spec_markdown"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := validReviseRequest() + c.mut(&req) + findings := validateChangeGroomShape(req) + if c.code == "" { + if len(findings) != 0 { + t.Fatalf("unexpected shape findings: %v", findings) + } + return + } + if !hasFindingCode(findings, c.code) { + t.Errorf("missing finding %q; got %v", c.code, findings) + } + }) + } +} + +func TestChangeGroomEmptyReviseRefusedWithoutEngineCall(t *testing.T) { + req := validReviseRequest() + req.SpecMarkdown = "" + req.Sections = nil + engine := &recordingEngine{} + reader := &fakeChangeReader{pin: mainModePin([]string{"inline"})} + deps := PlanningDeps{Engine: engine, Reader: reader, Clock: testClock()} + + res := ChangeGroom(context.Background(), deps, "", req) + + if res.Result != ResultInvalidInput { + t.Fatalf("result = %q, want invalid-input", res.Result) + } + if len(engine.calls) != 0 { + t.Errorf("engine called %d times on an empty revise, want 0", len(engine.calls)) + } + if !hasFindingCode(res.Findings, "empty-revise") { + t.Errorf("missing finding empty-revise; got %v", res.Findings) + } +} +``` + +Note `TestChangeGroomReviseShapeValidation` calls `validateChangeGroomShape` directly (it is in-package) so the passing cases don't need engine plumbing; the second test pins the no-engine-call property end to end, mirroring `TestChangeGroomTrivialRequiresRationale`. + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `go test ./internal/app/ -run 'TestChangeGroomRevise|TestChangeGroomEmptyRevise' -count=1` +Expected: compile FAILURE — `undefined: GroomRevise` (the constant does not exist yet). + +- [ ] **Step 3: Implement** + +In `internal/app/change_groom.go`, extend the outcome const block: + +```go +const ( + // GroomSpec lands an authored design spec and links it from the change. + GroomSpec GroomOutcome = "spec" + // GroomTrivial marks the change trivial with an authored rationale, writing + // no spec file. + GroomTrivial GroomOutcome = "trivial" + // GroomRevise adjusts an already-groomed proposed change: a whole-body + // replace of its existing linked spec, owned proposal-section edits, or + // both. It never writes spec: or trivial:, so a change can never flip + // between spec'd and trivial through this outcome. + GroomRevise GroomOutcome = "revise" +) +``` + +In `validateChangeGroomShape`, add the case (before `default`) and update the `FCInvalidOutcome` message: + +```go + case GroomRevise: + if strings.TrimSpace(req.SpecMarkdown) != "" { + if _, perr := document.Parse([]byte(req.SpecMarkdown)); perr != nil { + addShape(FCInvalidSpecMarkdown, "spec_markdown must parse as a Markdown document: "+perr.Error()) + } + } else if !hasEffectiveSectionEdit(req.Sections) { + addShape(FCEmptyRevise, "the revise outcome requires a non-empty spec_markdown or at least one replace/remove section edit") + } + default: + addShape(FCInvalidOutcome, fmt.Sprintf("outcome %q must be one of spec, trivial, revise", req.Outcome)) +``` + +Add the helper beside `hasAuthoredRationale`: + +```go +// hasEffectiveSectionEdit reports whether the section edits carry at least one +// replace or remove — the revise outcome's minimum effective input. A +// preserve-only or empty list changes nothing and is refused as an empty +// revise; relationship-field patches alone do not qualify. +func hasEffectiveSectionEdit(sections []SectionEditRequest) bool { + for _, s := range sections { + switch render.SectionIntent(s.Intent) { + case render.SectionReplace, render.SectionRemove: + return true + } + } + return false +} +``` + +In `internal/app/finding_codes.go`, add to the const block (beside `FCMissingRationale`): + +```go + FCEmptyRevise FindingCode = "empty-revise" +``` + +and insert `FCEmptyRevise,` into `AllFindingCodes` in sorted position (after `FCEmptyReport,`, before `FCEmptySpecMarkdown,` — the census is alphabetically ordered and `TestFindingCodeRegistryIntegrity` guards it). + +In `internal/app/schema_vocab.go`, extend the vocabulary: + +```go + v["groom_outcomes"] = Vocabulary{Members: []string{string(GroomSpec), string(GroomTrivial), string(GroomRevise)}} +``` + +In `internal/app/finding_codes_test.go`, `TestShapeValidatorCodesAreRegistered`, add one probe line after the existing two groom probes so the new code's registration is exercised: + +```go + emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomRevise})...) +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `go test ./internal/app/ -run 'TestChangeGroom|TestFindingCode|TestShapeValidator|TestVocabulary|TestSchemaVocab' -count=1` +Expected: PASS (including the pre-existing groom tests — the `spec`/`trivial` legs are untouched). + +- [ ] **Step 5: Mutation-check the census guard** + +Temporarily delete the `FCEmptyRevise,` line from `AllFindingCodes` (keep the const), re-run `go test ./internal/app/ -run TestShapeValidatorCodesAreRegistered -count=1`, and confirm it REDDENS (the validator emits an unregistered code). Restore the line and confirm green. This proves the census probe added in Step 3 is load-bearing, not decorative. + +- [ ] **Step 6: Commit** + +```bash +git add internal/app/change_groom.go internal/app/finding_codes.go internal/app/schema_vocab.go internal/app/finding_codes_test.go internal/app/change_groom_test.go +git commit -m "feat(app): revise groom outcome — request shape, finding code, vocabulary (change 0445)" +``` + +--- + +### Task 2: Plan-closure revise gate and mutations + +**Files:** +- Modify: `internal/app/change_groom.go` (`Plan`, file-header comment) +- Test: `internal/app/change_groom_test.go` + +**Interfaces:** +- Consumes: `GroomRevise` (Task 1), existing `refuseGroom`, `treeHasPath`, `assembleSpecFile`, `render.BacklinkContent`, `render.ApplySectionEdits`, test helpers `groomPlanFor`, `baseGroomOp`, `groomedRecordBytes`, `assertPlanPaths`, `revisableChange`/`trivialChange`/`validReviseRequest` (Task 1). +- Produces: Plan-closure refusal codes `"not-revisable"`, `"spec-not-linked"`, `"spec-file-missing"` (plain strings via `refuseGroom`); a revise `MutationPlan` whose spec mutation is `transaction.MutationReplace` at `c.Spec().Value`. Task 3's receipt work reads the same `specPath` variable this task threads through. + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/app/change_groom_test.go`: + +```go +const reviseSpecPath = "docs/superpowers/specs/2026-08-01-add-a-widget-design.md" + +// reviseFixtureFiles is the fake tree for a revisable spec'd change: the +// record with spec: linked, and the spec file itself with a backlink block. +func reviseFixtureFiles() map[string]string { + return map[string]string{ + groomPath(2, "add-a-widget"): revisableChange(2, "add-a-widget", reviseSpecPath), + reviseSpecPath: "\n" + + "> old backlink\n" + + "\n\n# Design\n\nThe original design body.\n", + } +} + +func TestChangeGroomPlanReviseSectionsOnly(t *testing.T) { + files := reviseFixtureFiles() + files["docs/changes/BOARD.md"] = "# Backlog\n\nold\n" + req := validReviseRequest() + req.SpecMarkdown = "" // sections only + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{"inline"}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + // Spec item 1: record + board replaced; the spec file is NOT in the plan, + // so it stays byte-identical by construction. + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + "docs/changes/BOARD.md": transaction.MutationReplace, + }) + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + if strings.Contains(rec, "Original what.") || !strings.Contains(rec, "Narrowed what.") { + t.Errorf("## What changes section not replaced:\n%s", rec) + } + if !strings.Contains(rec, "updated: '2026-08-16'") { + t.Errorf("updated not stamped from the clock:\n%s", rec) + } + // Spec item 9: revise never flips the groomed-outcome fields. + if !strings.Contains(rec, "spec: '"+reviseSpecPath+"'") { + t.Errorf("spec field changed under revise:\n%s", rec) + } + if !strings.Contains(rec, "trivial: false") { + t.Errorf("trivial field changed under revise:\n%s", rec) + } +} + +func TestChangeGroomPlanReviseSpecBodyOnly(t *testing.T) { + files := reviseFixtureFiles() + before := files[groomPath(2, "add-a-widget")] + req := validReviseRequest() + req.Sections = nil // spec body only + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + // Spec item 2: the spec file is REPLACED at the existing path, never created + // at a new dated path. + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + reviseSpecPath: transaction.MutationReplace, + }) + spec := string(groomedRecordBytes(t, plan, reviseSpecPath)) + if !strings.Contains(spec, "docket:backlink:start") { + t.Errorf("revised spec file missing backlink block:\n%s", spec) + } + if !strings.Contains(spec, "The revised design body.") || strings.Contains(spec, "The original design body.") { + t.Errorf("spec body not replaced:\n%s", spec) + } + // Spec item 2: the record's sections are byte-identical apart from + // updated:. The docket:artifacts block legitimately re-renders (the same + // call every groom outcome makes — the empty fixture block gains a Spec + // row), so compare the authored body AFTER the artifacts block, plus the + // frontmatter fields, rather than the whole file. + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + bodyAfterArtifacts := func(s string) string { + i := strings.Index(s, "docket:artifacts:end") + if i < 0 { + t.Fatalf("record lacks the artifacts end marker:\n%s", s) + } + return s[i:] + } + if got, want := bodyAfterArtifacts(rec), bodyAfterArtifacts(before); got != want { + t.Errorf("authored body changed under a spec-only revise:\ngot:\n%s\nwant:\n%s", got, want) + } + if !strings.Contains(rec, "updated: '2026-08-16'") { + t.Errorf("updated not stamped:\n%s", rec) + } + if !strings.Contains(rec, "spec: '"+reviseSpecPath+"'") || !strings.Contains(rec, "trivial: false") { + t.Errorf("groomed-outcome fields changed under a spec-only revise:\n%s", rec) + } +} + +func TestChangeGroomPlanReviseBoth(t *testing.T) { + // Spec item 3: both edits land in one plan. + plan, opRes := groomPlanFor(t, reviseFixtureFiles(), baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + reviseSpecPath: transaction.MutationReplace, + }) +} + +func TestChangeGroomPlanReviseTrivialRationale(t *testing.T) { + // Spec item 4: sections-only revise of a trivial-verdicted change. + files := map[string]string{ + groomPath(2, "add-a-widget"): trivialChange(2, "add-a-widget"), + } + req := validReviseRequest() + req.SpecMarkdown = "" + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + }) + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + if !strings.Contains(rec, "trivial: true") { + t.Errorf("trivial verdict lost under revise:\n%s", rec) + } + if strings.Contains(rec, "spec: '") { + t.Errorf("revise of a trivial change wrote a spec link:\n%s", rec) + } +} + +func TestChangeGroomPlanReviseRefusals(t *testing.T) { + cases := []struct { + name string + files map[string]string + mut func(*ChangeGroomRequest) + code string + }{ + // Spec item 5: spec_markdown against a trivial-only (no-spec) change. + {"spec-not-linked", map[string]string{ + groomPath(2, "add-a-widget"): trivialChange(2, "add-a-widget"), + }, func(r *ChangeGroomRequest) {}, "spec-not-linked"}, + // Spec item 6: a needs-brainstorm change is groom's target, not revise's. + {"not-revisable needs-brainstorm", map[string]string{ + groomPath(2, "add-a-widget"): groomableChange(2, "add-a-widget"), + }, func(r *ChangeGroomRequest) {}, "not-revisable"}, + // Spec item 7: a non-proposed change. + {"not-revisable blocked", map[string]string{ + groomPath(2, "add-a-widget"): strings.Replace( + revisableChange(2, "add-a-widget", reviseSpecPath), + "status: proposed\n", "status: blocked\nblocked_by: 'waiting'\n", 1), + }, func(r *ChangeGroomRequest) {}, "not-revisable"}, + // Review Focus 3: dangling spec link — spec: names a path absent from + // the tree; never silently mint a file. + {"spec-file-missing", map[string]string{ + groomPath(2, "add-a-widget"): revisableChange(2, "add-a-widget", reviseSpecPath), + }, func(r *ChangeGroomRequest) {}, "spec-file-missing"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := validReviseRequest() + c.mut(&req) + plan, opRes := groomPlanFor(t, c.files, baseGroomOp([]string{}, req)) + if !opRes.Refused { + t.Fatalf("expected a refusal, got plan files %v", planPaths(plan)) + } + found := false + for _, f := range opRes.Findings { + if f.Code == c.code { + found = true + } + } + if !found { + t.Errorf("missing refusal code %q; got %v", c.code, opRes.Findings) + } + if len(plan.Files) != 0 { + t.Errorf("refused plan still carries files: %v", planPaths(plan)) + } + }) + } +} + +func TestChangeGroomPlanReviseRepeatable(t *testing.T) { + // Spec item 11 (plan level): a second revise over the first revise's own + // output succeeds — no one-shot marker exists. + files := reviseFixtureFiles() + plan1, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("first revise refused: %v", opRes.Findings) + } + files[groomPath(2, "add-a-widget")] = string(groomedRecordBytes(t, plan1, groomPath(2, "add-a-widget"))) + files[reviseSpecPath] = string(groomedRecordBytes(t, plan1, reviseSpecPath)) + req2 := validReviseRequest() + req2.SpecMarkdown = "# Design\n\nThe twice-revised body.\n" + plan2, opRes2 := groomPlanFor(t, files, baseGroomOp([]string{}, req2)) + if opRes2.Refused { + t.Fatalf("second revise refused: %v", opRes2.Findings) + } + spec := string(groomedRecordBytes(t, plan2, reviseSpecPath)) + if !strings.Contains(spec, "The twice-revised body.") { + t.Errorf("second revise did not land:\n%s", spec) + } +} +``` + +(Spec item 10 — stale/contended version — is the engine's existing `EntityExpectation` CAS, wired identically for every outcome by `ChangeGroom`'s `Expected:` block; it is not re-tested per outcome, matching how the `spec`/`trivial` outcomes treat it. Spec item 11's engine-level half is Task 3's integration test.) + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `go test ./internal/app/ -run TestChangeGroomPlanRevise -count=1` +Expected: FAIL — every revise plan currently hits the `not-groomable` refusal (the gate does not know `revise` yet), so the happy-path tests report "unexpected refusal" and the refusal tests report the wrong code. + +- [ ] **Step 3: Implement** + +In `internal/app/change_groom.go` `Plan`, replace the single groom gate with an outcome split. The existing gate block: + +```go + // Groom gate: the change must be proposed, still need design (no spec, not + // yet trivial). Grooming never inspects or sets claim metadata. + if c.Status() != domain.StatusProposed || c.Spec().Value != "" || c.Trivial() { + return refuseGroom("not-groomable", ...) + } +``` + +becomes: + +```go + // Groom/revise gate. A proposed change is either needs-design (groomable) + // or already-groomed (revisable) — the two gates are exact complements, so + // no proposed change satisfies both and none satisfies neither. Neither + // gate inspects or sets claim metadata. + if o.req.Outcome == GroomRevise { + if c.Status() != domain.StatusProposed || (c.Spec().Value == "" && !c.Trivial()) { + return refuseGroom("not-revisable", + fmt.Sprintf("change %04d is not an already-groomed proposed change (status %q, spec %q, trivial %v)", + o.req.ChangeID, c.Status(), c.Spec().Value, c.Trivial())) + } + } else if c.Status() != domain.StatusProposed || c.Spec().Value != "" || c.Trivial() { + return refuseGroom("not-groomable", + fmt.Sprintf("change %04d is not a proposed, needs-design change (status %q, spec %q, trivial %v)", + o.req.ChangeID, c.Status(), c.Spec().Value, c.Trivial())) + } +``` + +Immediately after the gate (before the section splice), resolve the revise spec decision so the request is refused before any mutation is assembled: + +```go + // Revise spec-body decision, resolved before any mutation is assembled: a + // non-empty SpecMarkdown replaces the change's EXISTING linked spec — never + // a new path — and requires both the link and the file to exist. + reviseSpec := o.req.Outcome == GroomRevise && strings.TrimSpace(o.req.SpecMarkdown) != "" + if reviseSpec { + if c.Spec().Value == "" { + return refuseGroom("spec-not-linked", + fmt.Sprintf("change %04d has no linked spec to revise (spec_markdown was submitted against a trivial-only change)", o.req.ChangeID)) + } + exists, err := treeHasPath(ctx, st.Tree, c.Spec().Value) + if err != nil { + return transaction.MutationPlan{}, transaction.OperationResult{}, err + } + if !exists { + return refuseGroom("spec-file-missing", + fmt.Sprintf("change %04d links spec %q but no such file exists on the tree", o.req.ChangeID, c.Spec().Value)) + } + } +``` + +The `specPath` computation and the two field-set blocks already key on `o.req.Outcome == GroomSpec` / `== GroomTrivial`, so under revise neither `spec` nor `trivial` is ever set — leave them exactly as they are (this is the structural impossibility the spec requires; do not add any revise arm that touches those fields). For revise, `specPath` (the dated new-path variable) is unused; keep its computation where it is (it is cheap and pure) or guard it under `GroomSpec` along with its `treeHasPath` probe — the probe is already inside the `GroomSpec` block. + +In the file-mutation assembly, extend the spec-file block: + +```go + if o.req.Outcome == GroomSpec { + backlink, err := render.BacklinkContent(gc, o.link) + if err != nil { + return transaction.MutationPlan{}, transaction.OperationResult{}, fmt.Errorf("change groom: rendering spec backlink: %w", err) + } + files = append(files, transaction.FileMutation{ + Path: gitcli.RepoPath(specPath), Kind: transaction.MutationCreate, + Bytes: assembleSpecFile(backlink, o.req.SpecMarkdown), + }) + } + if reviseSpec { + backlink, err := render.BacklinkContent(gc, o.link) + if err != nil { + return transaction.MutationPlan{}, transaction.OperationResult{}, fmt.Errorf("change groom: rendering spec backlink: %w", err) + } + files = append(files, transaction.FileMutation{ + Path: gitcli.RepoPath(c.Spec().Value), Kind: transaction.MutationReplace, + Bytes: assembleSpecFile(backlink, o.req.SpecMarkdown), + }) + } +``` + +Update the file-header comment (the "one of two authored outcomes" paragraph) to name the third outcome: grooming to build-ready by spec or trivial verdict, or revising an already-groomed proposed change in place. + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `go test ./internal/app/ -run TestChangeGroomPlan -count=1` +Expected: PASS — all new revise plan tests and every pre-existing groom plan test (`SpecOutcomeFileSet`, `TrivialOutcomeFileSet`, `RefusesNonGroomable`, `RefusesExistingSpecPath`, `SourcePreservation`, `RelationshipsWritten`, `ToleratesMissingUpdatedField` must stay green — the groom gate's behavior for `spec`/`trivial` is byte-for-byte unchanged). + +- [ ] **Step 5: Mutation-check the gate complement** + +Two hand mutations, each proving one conjunct of the revise gate (per the duplicated-gate learning — the gate is a whole predicate, not a threshold): + +1. Change `(c.Spec().Value == "" && !c.Trivial())` to `c.Spec().Value == ""` — run `go test ./internal/app/ -run TestChangeGroomPlanRevise -count=1`; expected: `TestChangeGroomPlanReviseTrivialRationale` REDDENS (a trivial change would be refused). Restore. +2. Delete the `c.Status() != domain.StatusProposed` conjunct from the revise arm — expected: the `not-revisable blocked` case REDDENS. Restore. + +Confirm each mutation landed with `grep -c` on the edited expression before trusting the red, then confirm green after restore. + +- [ ] **Step 6: Commit** + +```bash +git add internal/app/change_groom.go internal/app/change_groom_test.go +git commit -m "feat(app): revise groom outcome — plan-closure gate and spec replace (change 0445)" +``` + +--- + +### Task 3: Receipt, result outcome, HumanText, CLI help, and the applied-path integration test + +**Files:** +- Modify: `internal/app/change_groom.go` (`ChangeGroomResult`, `HumanText`, `changeGroomResultFromOutcome`, receipt assembly in `Plan`) +- Modify: `internal/cli/change.go` (groom subcommand help line) +- Test: `internal/app/change_groom_test.go`, `internal/app/change_integration_test.go` + +**Interfaces:** +- Consumes: `GroomRevise`, `reviseSpec` plumbing (Task 2), `changeGroomReceipt` (already carries `Outcome`), integration helpers `newWorkingRepo`, `newGitClient`, `mustMarshal`, `recordingEngine`, `mainModePin`, `testClock`. +- Produces: `ChangeGroomResult.Outcome string` (JSON `outcome,omitempty`), populated from the receipt on apply; `HumanText` revise case `"change %04d revised — %s"`. + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/app/change_groom_test.go`: + +```go +func TestChangeGroomResultHumanTextRevise(t *testing.T) { + r := newChangeGroomResult(ResultApplied, ChangeGroomResult{ + ID: 7, Outcome: string(GroomRevise), SpecPath: "docs/superpowers/specs/x.md", + Revision: "cafebabecafebabecafebabecafebabecafebabe", + }) + got := r.HumanText() + want := "change 0007 revised — cafebabecafebabecafebabecafebabecafebabe" + if got != want { + // Review Focus 4: a revise carrying a spec path must NOT render as + // "groomed (spec …)". + t.Errorf("HumanText = %q, want %q", got, want) + } +} +``` + +Append to `internal/app/change_integration_test.go` (inside the `//go:build integration` file, mirroring `TestIntegrationChangeAuthoringGroomAppliedResult`): + +```go +func TestIntegrationChangeAuthoringReviseAppliedResult(t *testing.T) { + repoDir := newWorkingRepo(t, nil).invocation + specPath := "docs/superpowers/specs/2026-08-01-add-a-widget-design.md" + receipt := mustMarshal(t, changeGroomReceipt{ + ID: 2, Op: OperationChangeGroom, Outcome: string(GroomRevise), SpecPath: specPath, + }) + engine := &recordingEngine{result: transaction.Result{ + Disposition: transaction.DispositionApplied, + AppliedCommit: "cafebabecafebabecafebabecafebabecafebabe", + Receipt: receipt, + }} + reader := &fakeChangeReader{pin: mainModePin([]string{"inline"})} + deps := PlanningDeps{Client: newGitClient(t), Engine: engine, Reader: reader, Clock: testClock()} + + res := ChangeGroom(context.Background(), deps, repoDir, validReviseRequest()) + + if res.Result != ResultApplied { + t.Fatalf("result = %q, want applied", res.Result) + } + if res.Outcome != string(GroomRevise) { + t.Errorf("outcome = %q, want revise", res.Outcome) + } + if res.ID != 2 || res.SpecPath != specPath { + t.Errorf("identity from receipt = (%d, %q)", res.ID, res.SpecPath) + } + // Spec items 10/11 (engine-level): the CAS expectation pins the exact + // submitted version on every call, revise included — repeatability is a + // second standard call with the freshly-read version, nothing more. + if len(engine.calls) != 1 { + t.Fatalf("engine calls = %d, want 1", len(engine.calls)) + } + exp := engine.calls[0].Expected + if len(exp) != 1 || string(exp[0].Version.ObjectID) != validReviseRequest().Version { + t.Errorf("revise did not pin the exact submitted version: %+v", exp) + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `go test ./internal/app/ -run TestChangeGroomResultHumanTextRevise -count=1` +Expected: compile FAILURE — `unknown field Outcome in struct literal`. + +Run: `go test -tags integration ./internal/app/ -run TestIntegrationChangeAuthoringReviseAppliedResult -count=1` +Expected: compile FAILURE for the same reason. + +- [ ] **Step 3: Implement** + +In `internal/app/change_groom.go`: + +`ChangeGroomResult` gains the outcome field (after `ID`): + +```go +type ChangeGroomResult struct { + Envelope + ID int `json:"id,omitempty"` + Outcome string `json:"outcome,omitempty"` + SpecPath string `json:"spec_path,omitempty"` + Revision string `json:"committed_revision,omitempty"` + Findings []StatusFinding `json:"findings"` +} +``` + +`HumanText` gains the revise arm first in the applied branch: + +```go + case ResultApplied: + if r.Outcome == string(GroomRevise) { + return fmt.Sprintf("change %04d revised — %s", r.ID, r.Revision) + } + if r.SpecPath != "" { + return fmt.Sprintf("change %04d groomed (spec %s) — %s", r.ID, r.SpecPath, r.Revision) + } + return fmt.Sprintf("change %04d groomed (trivial) — %s", r.ID, r.Revision) +``` + +`changeGroomResultFromOutcome` copies the receipt's outcome: + +```go + if rec, ok := decodeChangeGroomReceipt(res.Receipt); ok { + out.ID = rec.ID + out.Outcome = rec.Outcome + out.SpecPath = rec.SpecPath + } +``` + +In `Plan`'s receipt assembly, populate `SpecPath` for a revise that replaced the spec (mirroring the trivial outcome's empty `SpecPath` when only sections changed): + +```go + receiptSpecPath := "" + if o.req.Outcome == GroomSpec { + receiptSpecPath = specPath + } + if reviseSpec { + receiptSpecPath = c.Spec().Value + } +``` + +Also update the commit-subject line so a revise commit reads `change 0445 groomed (revise)` — the existing `fmt.Sprintf("change %04d groomed (%s)", …)` already does this via the outcome string; leave it unchanged. + +In `internal/cli/change.go`, update the groom subcommand's help line (quote it verbatim when searching): + +```go + groom := changeSubcommand("change", "groom", + "Groom a proposed change to build-ready (spec or trivial), or revise an already-groomed one, from a JSON request", + ... +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `go test ./internal/app/ -count=1 && go test -tags integration ./internal/app/ -run TestIntegrationChangeAuthoring -count=1 && go build ./...` +Expected: PASS (the whole untagged `internal/app` package, the authoring integration slice, and a clean build including `internal/cli`). `TestIntegrationChangeAuthoringGroomAppliedResult` must stay green — its receipt carries `Outcome: "spec"` and now also flows into `res.Outcome`, which that test does not assert. + +- [ ] **Step 5: Commit** + +```bash +git add internal/app/change_groom.go internal/app/change_groom_test.go internal/app/change_integration_test.go internal/cli/change.go +git commit -m "feat(app,cli): revise groom outcome — receipt, result outcome, human text (change 0445)" +``` + +--- + +### Task 4: Skill-doc wiring — replace the hand-edit workaround with the typed revise path + +**Files:** +- Modify: `skills/docket-groom-next/SKILL.md` ("When to use" bullet, Step 1, Step 4) +- Modify: `skills/docket-new-change/SKILL.md` (Brainstorm-mode step 5) + +These are the repo's own `skills/` sources (they ship via the harness sync), never `~/.claude/skills`. + +**Interfaces:** +- Consumes: the `change.groom` request vocabulary established in Tasks 1–3 (`outcome: revise`, `spec_markdown`, `sections`, refusals `not-revisable` / `spec-not-linked` / `spec-file-missing` / `empty-revise`). +- Produces: prose only; no code contract. + +- [ ] **Step 1: Grep the suite for the prose being removed** + +Before editing, prove the deleted sentence has no test dependents (restatements accumulate their own guards — asserts grep the copy, not the source): + +```bash +cd +grep -rn "clear .spec:. by hand" tests/ internal/ --include='*_test.go' --include='*.sh' +grep -rn "re-groom" tests/ internal/ --include='*_test.go' --include='*.sh' +``` + +Expected: no hits (verified at plan time — `internal/repoguard/prose_contracts_test.go`'s only `docket-groom-next` sentinel pins the `### Step 3 — Recap, then groom with the human` heading, which this task does not touch). If either grep hits, repoint that assert at the new prose per the relocation rule — do not keep the old sentence to appease a grep. + +- [ ] **Step 2: Edit `skills/docket-groom-next/SKILL.md`** + +(a) Replace the "When to use" bullet: + +```markdown +- Do NOT use to re-groom a change that already has a spec — drift against current reality is the reconcile pass's job in `docket-implement-next`. A human who wants to redo a design can clear `spec:` by hand first. +``` + +with: + +```markdown +- An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) is NOT an error — it routes to the revise flow (Step 4, exit 5): adjust the existing spec and owned sections through `change.groom` with `outcome: revise`. Drift against current reality at build time remains the reconcile pass's job in `docket-implement-next`. +``` + +(b) In **Step 1 — Select**, amend the explicit-id sentence. Replace: + +```markdown +Pick the top, or accept an explicit id from the caller; an explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. +``` + +with: + +```markdown +Pick the top, or accept an explicit id from the caller. An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) routes to the **revise** flow — recap what is there today (the linked spec's body, or the trivial rationale, and the owned sections), run the resolved brainstorm skill seeded with the current design framed as "what would you like to adjust," and exit via Step 4's revise exit. Any other explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. +``` + +(c) In **Step 4 — Exit**, retitle the intro from "one of four" to "one of five" (also update the heading's `(one of four; the human confirms which)`) and append a fifth exit after Defer: + +```markdown +5. **Revise** (explicit-id route only): the change is already groomed and the human wants it adjusted — apply the `change.groom` operation with `--repo-dir .docket --request ` with `outcome: revise`, the pinned `path` + `version`, and whichever of `spec_markdown` (a whole-body replace of the *existing* linked spec — the transaction re-stamps its `docket:backlink` block and never mints a new path) and owned-section `sections` edits the adjustment touched, plus any relationship-field updates. It never sets `spec:` or `trivial:`, so a change cannot flip between spec'd and trivial here. Repeatable while the change stays `proposed`. A typed refusal (`not-revisable`, `spec-not-linked`, `spec-file-missing`, `empty-revise`, version mismatch) writes nothing — surface it. The change stays build-ready. +``` + +- [ ] **Step 3: Edit `skills/docket-new-change/SKILL.md`** + +In Brainstorm-mode step 5 (**Groom to build-ready & land**), append one sentence at the end, after "STOP. Never implements.": + +```markdown +To adjust a just-landed spec or its owned sections afterwards, run `docket-groom-next ` — its explicit-id path routes an already-groomed change to `change.groom` with `outcome: revise`; never re-run this skill or hand-edit the spec file. +``` + +- [ ] **Step 4: Verify** + +```bash +cd +/usr/bin/grep -c "by hand first" skills/docket-groom-next/SKILL.md # expect 0 +/usr/bin/grep -c "outcome: revise" skills/docket-groom-next/SKILL.md # expect >= 2 (When-to-use routes + Step 4 exit) +/usr/bin/grep -c "outcome: revise" skills/docket-new-change/SKILL.md # expect 1 +go test ./internal/repoguard/ -run TestProse -count=1 +``` + +Expected: counts as annotated; the prose-contract sentinels stay green (the pinned Step 3 heading is untouched). + +- [ ] **Step 5: Commit** + +```bash +git add skills/docket-groom-next/SKILL.md skills/docket-new-change/SKILL.md +git commit -m "docs(skills): route already-groomed explicit ids to the revise flow (change 0445)" +``` + +--- + +## Acceptance-item map (spec "Acceptance and verification") + +| Spec item | Test | Task | +|---|---|---| +| 1 sections-only revise | `TestChangeGroomPlanReviseSectionsOnly` | 2 | +| 2 spec-body-only revise | `TestChangeGroomPlanReviseSpecBodyOnly` | 2 | +| 3 both in one commit | `TestChangeGroomPlanReviseBoth` | 2 | +| 4 trivial-rationale revise | `TestChangeGroomPlanReviseTrivialRationale` | 2 | +| 5 spec_markdown vs no-spec change | `TestChangeGroomPlanReviseRefusals/spec-not-linked` | 2 | +| 6 needs-brainstorm refused | `TestChangeGroomPlanReviseRefusals/not-revisable needs-brainstorm` | 2 | +| 7 non-proposed refused | `TestChangeGroomPlanReviseRefusals/not-revisable blocked` | 2 | +| 8 empty/all-preserve refused pre-engine | `TestChangeGroomReviseShapeValidation`, `TestChangeGroomEmptyReviseRefusedWithoutEngineCall` | 1 | +| 9 never writes spec:/trivial: | field asserts in `ReviseSectionsOnly` (spec'd) + `ReviseTrivialRationale` (trivial), byte-equality in `ReviseSpecBodyOnly` | 2 | +| 10 stale version CAS | `TestIntegrationChangeAuthoringReviseAppliedResult` exact-version expectation (mechanism shared with every outcome) | 3 | +| 11 repeated revise | `TestChangeGroomPlanReviseRepeatable` (plan level) + item-10 test's receipt/CAS note | 2/3 | diff --git a/internal/app/change_groom.go b/internal/app/change_groom.go index 1d93a2a70..a6f6016cc 100644 --- a/internal/app/change_groom.go +++ b/internal/app/change_groom.go @@ -1,6 +1,7 @@ package app import ( + "bytes" "context" "encoding/json" "fmt" @@ -20,14 +21,20 @@ import ( // This file is the `change groom` planning operation: it grooms a proposed, // needs-design change to build-ready by one of two authored outcomes — a full -// spec, or a trivial verdict — landing the change's source mutation and every +// spec, or a trivial verdict — or, by the third outcome (revise), adjusts an +// already-groomed proposed change in place: a whole-body replace of its existing +// linked spec and/or owned proposal-section edits, never touching spec: or +// trivial:. Every outcome lands the change's source mutation and every // affected v1-owned derived view (the change record's owned proposal sections, -// its typed fields, its artifact block; a new spec file for the spec outcome; -// the inline board) as one validated atomic transaction. Grooming is a +// its typed fields, its artifact block; a new spec file for the spec outcome, a +// replaced one for a spec-body revise; the inline board) as one validated +// atomic transaction. Grooming is a // non-allocating edit of an existing record, so it pins the submitted record -// version with an exact-blob entity expectation rather than an idempotency key, -// and it never touches claim metadata. It decides no lifecycle policy beyond the -// groom gate the spec fixes here (proposed, needs-design, not yet trivial). +// version with an exact-blob entity expectation rather than an idempotency key +// (a spec-body revise also checks the linked spec file's blob id in Plan), and +// it never touches claim metadata. It decides no lifecycle policy beyond the +// groom gate the spec fixes here (proposed, needs-design, not yet trivial) and +// its exact complement, the revise gate (proposed, already spec'd or trivial). // OperationChangeGroom is the operation key `change groom` records in its result // envelope and its transaction trailer. @@ -42,8 +49,17 @@ const ( // GroomTrivial marks the change trivial with an authored rationale, writing // no spec file. GroomTrivial GroomOutcome = "trivial" + // GroomRevise adjusts an already-groomed proposed change: a whole-body + // replace of its existing linked spec, owned proposal-section edits, or + // both. It never writes spec: or trivial:, so a change can never flip + // between spec'd and trivial through this outcome. + GroomRevise GroomOutcome = "revise" ) +// reasonSpecVersionMismatch is the Plan refusal for a stale spec_version on a +// spec-body revise; changeGroomResultFromOutcome maps it onto contended. +const reasonSpecVersionMismatch = "spec-version-mismatch" + // specsDir is the metadata-tree directory design specs live in. It is a fixed v1 // location (the Bash grooming skills write here); it is not configurable. const specsDir = "docs/superpowers/specs" @@ -62,6 +78,13 @@ type ChangeGroomRequest struct { SpecMarkdown string `json:"spec_markdown,omitempty"` // required for the spec outcome Sections []SectionEditRequest `json:"sections"` // proposal-section edits + // SpecVersion pins the change's existing linked spec file (the path the + // record's spec: field names) by its exact full blob object id. A revise + // carrying spec_markdown requires it and no other request accepts it: the + // whole-body replace overwrites the spec file, so a concurrent spec edit + // contends instead of being silently clobbered. + SpecVersion string `json:"spec_version,omitempty"` + DependsOn []int `json:"depends_on"` Related []int `json:"related"` DiscoveredFrom []int `json:"discovered_from"` @@ -85,6 +108,7 @@ type SectionEditRequest struct { type ChangeGroomResult struct { Envelope ID int `json:"id,omitempty"` + Outcome string `json:"outcome,omitempty"` SpecPath string `json:"spec_path,omitempty"` Revision string `json:"committed_revision,omitempty"` Findings []StatusFinding `json:"findings"` @@ -94,6 +118,9 @@ type ChangeGroomResult struct { func (r ChangeGroomResult) HumanText() string { switch r.Result { case ResultApplied: + if r.Outcome == string(GroomRevise) { + return fmt.Sprintf("change %04d revised — %s", r.ID, r.Revision) + } if r.SpecPath != "" { return fmt.Sprintf("change %04d groomed (spec %s) — %s", r.ID, r.SpecPath, r.Revision) } @@ -179,6 +206,10 @@ func ChangeGroom(ctx context.Context, deps PlanningDeps, repoDir string, req Cha changesDir: eff.ChangesDir.Value, } + // The engine pins the record. A spec-body revise's spec_version is checked + // in Plan instead, against the blob at the path the record links — the + // engine checks expectations before the record is read, and Plan runs on + // the same fetched base, so the check is just as exact. res, execErr := deps.Engine.Execute(ctx, transaction.Request{ Repository: repo, Remote: originRemote, @@ -195,16 +226,25 @@ func ChangeGroom(ctx context.Context, deps PlanningDeps, repoDir string, req Cha } // changeGroomResultFromOutcome folds a transaction outcome into the result -// document. A refusal from this operation is always state-shaped (the groom -// gate, a taken spec path, or an evolution refusal), so the refusal maps onto -// invalid-state. +// document. A refusal from this operation is state-shaped (the groom gate, a +// taken spec path, or an evolution refusal), so it maps onto invalid-state — +// except a stale spec_version, the spec file's analogue of a stale record +// version, which maps onto contended like the engine's own pin mismatch. func changeGroomResultFromOutcome(res transaction.Result, execErr error) ChangeGroomResult { result, _ := mapOutcome(res, execErr, ResultInvalidState) + if res.Disposition == transaction.DispositionRefused { + for _, f := range res.Findings { + if f.Code == reasonSpecVersionMismatch { + result = ResultContended + } + } + } out := ChangeGroomResult{Findings: findingsToStatus(res.Findings)} if result == ResultApplied { if rec, ok := decodeChangeGroomReceipt(res.Receipt); ok { out.ID = rec.ID + out.Outcome = rec.Outcome out.SpecPath = rec.SpecPath } out.Revision = string(res.AppliedCommit) @@ -237,21 +277,57 @@ func validateChangeGroomShape(req ChangeGroomRequest) []StatusFinding { case GroomSpec: if strings.TrimSpace(req.SpecMarkdown) == "" { addShape(FCEmptySpecMarkdown, "spec_markdown must be non-empty for the spec outcome") - } else if _, perr := document.Parse([]byte(req.SpecMarkdown)); perr != nil { - addShape(FCInvalidSpecMarkdown, "spec_markdown must parse as a Markdown document: "+perr.Error()) + } else if msg := specMarkdownShapeProblem(req.SpecMarkdown); msg != "" { + addShape(FCInvalidSpecMarkdown, msg) } case GroomTrivial: if !hasAuthoredRationale(req.Sections) { addShape(FCMissingRationale, "the trivial outcome requires a non-empty authored rationale among the section edits") } + case GroomRevise: + if strings.TrimSpace(req.SpecMarkdown) != "" { + if msg := specMarkdownShapeProblem(req.SpecMarkdown); msg != "" { + addShape(FCInvalidSpecMarkdown, msg) + } + } else if !hasEffectiveSectionEdit(req.Sections) { + addShape(FCEmptyRevise, "the revise outcome requires a non-empty spec_markdown or at least one replace/remove section edit") + } default: - addShape(FCInvalidOutcome, fmt.Sprintf("outcome %q must be one of spec, trivial", req.Outcome)) + addShape(FCInvalidOutcome, fmt.Sprintf("outcome %q must be one of spec, trivial, revise", req.Outcome)) + } + + // A spec-body revise overwrites the linked spec file, so it must pin that + // file's version exactly like the record. Nothing else checks spec_version, + // so it is refused anywhere else rather than silently ignored. + specRevise := req.Outcome == GroomRevise && strings.TrimSpace(req.SpecMarkdown) != "" + hasSpecVersion := strings.TrimSpace(req.SpecVersion) != "" + switch { + case specRevise && !hasSpecVersion: + addShape(FCEmptySpecVersion, "spec_version must be the exact full blob object id of the linked spec when a revise carries spec_markdown") + case !specRevise && hasSpecVersion: + addShape(FCInvalidSpecVersion, "spec_version applies only to a revise that carries spec_markdown") } findings = append(findings, validateGroomSections(req.Sections)...) return findings } +// specMarkdownShapeProblem returns why an authored spec_markdown is unusable, or +// "" when it is usable. It must parse as a Markdown document, and it must be the +// spec body only: assembleSpecFile prepends the rendered docket:backlink block +// itself, so a body that already carries one (a spec file resubmitted as read) +// would commit a duplicate marker pair. It is refused, never silently stripped. +func specMarkdownShapeProblem(markdown string) string { + doc, err := document.Parse([]byte(markdown)) + if err != nil { + return "spec_markdown must parse as a Markdown document: " + err.Error() + } + if _, ok := doc.Block(backlinkBlockName); ok { + return "spec_markdown must be the spec body without its docket:backlink block; the operation renders that block itself" + } + return "" +} + // hasAuthoredRationale reports whether the section edits carry at least one // replace with a non-empty body — the trivial outcome's required rationale. func hasAuthoredRationale(sections []SectionEditRequest) bool { @@ -263,6 +339,20 @@ func hasAuthoredRationale(sections []SectionEditRequest) bool { return false } +// hasEffectiveSectionEdit reports whether the section edits carry at least one +// replace or remove — the revise outcome's minimum effective input. A +// preserve-only or empty list changes nothing and is refused as an empty +// revise; relationship-field patches alone do not qualify. +func hasEffectiveSectionEdit(sections []SectionEditRequest) bool { + for _, s := range sections { + switch render.SectionIntent(s.Intent) { + case render.SectionReplace, render.SectionRemove: + return true + } + } + return false +} + // validateGroomSections checks each section edit against the owned-heading set // and the intent grammar, and enforces the empty-Markdown rule for non-replace // intents — the same rules render.ApplySectionEdits enforces, surfaced here as @@ -331,7 +421,8 @@ func (o changeGroomOp) Key() transaction.OperationKey { return OperationChangeGr // Plan gates the groom against the attempt's snapshot, splices the owned // proposal sections, patches the typed fields, re-renders the artifact block, // and assembles the closed plan: the groomed change record, the new spec file -// (spec outcome), and the re-rendered board when inline is enabled. +// (spec outcome) or the replaced existing spec file (a spec-body revise), and +// the re-rendered board when inline is enabled. func (o changeGroomOp) Plan(ctx context.Context, st transaction.AttemptState) (transaction.MutationPlan, transaction.OperationResult, error) { snap := st.State.Snapshot @@ -339,14 +430,51 @@ func (o changeGroomOp) Plan(ctx context.Context, st transaction.AttemptState) (t if out != domain.LookupFound { return refuseGroom("not-found", fmt.Sprintf("change %04d is not present in the current corpus", o.req.ChangeID)) } - // Groom gate: the change must be proposed, still need design (no spec, not - // yet trivial). Grooming never inspects or sets claim metadata. - if c.Status() != domain.StatusProposed || c.Spec().Value != "" || c.Trivial() { + // Groom/revise gate. A proposed change is either needs-design (groomable) + // or already-groomed (revisable) — the two gates are exact complements, so + // no proposed change satisfies both and none satisfies neither. Neither + // gate inspects or sets claim metadata. + if o.req.Outcome == GroomRevise { + if c.Status() != domain.StatusProposed || (c.Spec().Value == "" && !c.Trivial()) { + return refuseGroom("not-revisable", + fmt.Sprintf("change %04d is not an already-groomed proposed change (status %q, spec %q, trivial %v)", + o.req.ChangeID, c.Status(), c.Spec().Value, c.Trivial())) + } + } else if c.Status() != domain.StatusProposed || c.Spec().Value != "" || c.Trivial() { return refuseGroom("not-groomable", fmt.Sprintf("change %04d is not a proposed, needs-design change (status %q, spec %q, trivial %v)", o.req.ChangeID, c.Status(), c.Spec().Value, c.Trivial())) } + // Revise spec-body decision, resolved before any mutation is assembled: a + // non-empty SpecMarkdown replaces the change's EXISTING linked spec — never + // a new path — and requires both the link and the file to exist. + reviseSpec := o.req.Outcome == GroomRevise && strings.TrimSpace(o.req.SpecMarkdown) != "" + var existingSpec []byte + if reviseSpec { + if c.Spec().Value == "" { + return refuseGroom("spec-not-linked", + fmt.Sprintf("change %04d has no linked spec to revise (spec_markdown was submitted against a trivial-only change)", o.req.ChangeID)) + } + blob, blobID, exists, err := treeBlob(ctx, st.Tree, c.Spec().Value) + if err != nil { + return transaction.MutationPlan{}, transaction.OperationResult{}, err + } + existingSpec = blob + if !exists { + return refuseGroom("spec-file-missing", + fmt.Sprintf("change %04d links spec %q but no such file exists on the tree", o.req.ChangeID, c.Spec().Value)) + } + // The whole-body replace overwrites the spec file, so it is pinned like + // the record: a stale spec_version refuses (mapped to contended) rather + // than clobbering a concurrent spec edit. The record's pin cannot catch + // that race — a same-day spec-only revise leaves the record unchanged. + if string(blobID) != o.req.SpecVersion { + return refuseGroom(reasonSpecVersionMismatch, + fmt.Sprintf("spec %q moved since the submitted spec_version; re-read it and retry", c.Spec().Value)) + } + } + src, ok := st.State.Sources[o.req.Path] if !ok { return refuseGroom("path-mismatch", @@ -440,8 +568,16 @@ func (o changeGroomOp) Plan(ctx context.Context, st transaction.AttemptState) (t return transaction.MutationPlan{}, transaction.OperationResult{}, fmt.Errorf("change groom: writing artifact block: %w", err) } - files := []transaction.FileMutation{ - {Path: gitcli.RepoPath(o.req.Path), Kind: transaction.MutationReplace, Bytes: finalBytes}, + // Declare only paths whose bytes actually change: the engine's delta verifier + // rejects a declared path that is not an actual change, so a revise that + // re-renders the record byte-identical (updated: already today, artifacts + // already rendered, identical section text) must not declare it. An empty + // plan is the engine's clean no-op path — the same skip includeBoard makes. + var files []transaction.FileMutation + if !bytes.Equal(finalBytes, src) { + files = append(files, transaction.FileMutation{ + Path: gitcli.RepoPath(o.req.Path), Kind: transaction.MutationReplace, Bytes: finalBytes, + }) } if o.req.Outcome == GroomSpec { @@ -454,6 +590,20 @@ func (o changeGroomOp) Plan(ctx context.Context, st transaction.AttemptState) (t Path: gitcli.RepoPath(specPath), Kind: transaction.MutationCreate, Bytes: specBytes, }) } + if reviseSpec { + // Whole-body replace at the change's existing linked spec path; the + // backlink block is re-rendered exactly as the spec outcome writes it. + backlink, err := render.BacklinkContent(gc, o.link) + if err != nil { + return transaction.MutationPlan{}, transaction.OperationResult{}, fmt.Errorf("change groom: rendering spec backlink: %w", err) + } + // An identical spec body is not an actual change; skip the declaration. + if specBytes := assembleSpecFile(backlink, o.req.SpecMarkdown); !bytes.Equal(specBytes, existingSpec) { + files = append(files, transaction.FileMutation{ + Path: gitcli.RepoPath(c.Spec().Value), Kind: transaction.MutationReplace, Bytes: specBytes, + }) + } + } if o.inline { boardPath := path.Join(o.changesDir, "BOARD.md") @@ -466,6 +616,11 @@ func (o changeGroomOp) Plan(ctx context.Context, st transaction.AttemptState) (t if o.req.Outcome == GroomSpec { receiptSpecPath = specPath } + if reviseSpec { + // A revise that replaced the spec body names the existing linked path; a + // sections-only revise leaves it empty, like the trivial outcome. + receiptSpecPath = c.Spec().Value + } receipt, err := json.Marshal(changeGroomReceipt{ ID: o.req.ChangeID, Op: OperationChangeGroom, Outcome: string(o.req.Outcome), SpecPath: receiptSpecPath, }) @@ -571,9 +726,19 @@ func buildGroomCandidate(eff config.Effective, docs map[string]document.Document // treeHasPath reports whether path exists as a blob on the base tree. func treeHasPath(ctx context.Context, tree transaction.Tree, path string) (bool, error) { + _, _, found, err := treeBlob(ctx, tree, path) + return found, err +} + +// treeBlob reads path's blob bytes and object id from the base tree, reporting +// whether it exists. +func treeBlob(ctx context.Context, tree transaction.Tree, path string) ([]byte, gitcli.ObjectID, bool, error) { results, err := tree.ReadBlobs(ctx, []gitcli.RepoPath{gitcli.RepoPath(path)}) if err != nil { - return false, fmt.Errorf("change groom: probing path %q: %w", path, err) + return nil, "", false, fmt.Errorf("change groom: probing path %q: %w", path, err) + } + if len(results) != 1 || !results[0].Found { + return nil, "", false, nil } - return len(results) == 1 && results[0].Found, nil + return results[0].Blob.Bytes, results[0].Blob.ObjectID, true, nil } diff --git a/internal/app/change_groom_test.go b/internal/app/change_groom_test.go index d2e7aef74..ae1d01606 100644 --- a/internal/app/change_groom_test.go +++ b/internal/app/change_groom_test.go @@ -2,6 +2,8 @@ package app import ( "context" + "encoding/json" + "github.com/danielhanold/docket/internal/domain" "github.com/danielhanold/docket/internal/render" "github.com/danielhanold/docket/internal/repository/transaction" "strings" @@ -57,6 +59,13 @@ func padID(id int) string { return s } +// groomBacklinkedSpecMarkdown is a spec body that still carries a +// docket:backlink managed block — the file as read, not the body the groom +// operation expects. +const groomBacklinkedSpecMarkdown = "\n" + + "> old backlink\n" + + "\n\n# Design\n\nThe design body.\n" + // validGroomSpecRequest is a well-formed spec-outcome groom request against the // groomable fixture at id 2 / slug add-a-widget. func validGroomSpecRequest() ChangeGroomRequest { @@ -87,6 +96,11 @@ func TestChangeGroomRejectsBadShapeWithoutEngineCall(t *testing.T) { {"unknown outcome", func(r *ChangeGroomRequest) { r.Outcome = "maybe" }, "invalid-outcome"}, {"spec outcome empty markdown", func(r *ChangeGroomRequest) { r.SpecMarkdown = "" }, "empty-spec_markdown"}, {"spec outcome unparseable markdown", func(r *ChangeGroomRequest) { r.SpecMarkdown = "---\nid: 1\n" }, "invalid-spec_markdown"}, + // The writer prepends the backlink block itself; an authored one would + // commit a duplicate marker pair. + {"spec outcome markdown carrying a backlink block", func(r *ChangeGroomRequest) { + r.SpecMarkdown = groomBacklinkedSpecMarkdown + }, "invalid-spec_markdown"}, {"section unowned heading", func(r *ChangeGroomRequest) { r.Sections = []SectionEditRequest{{Heading: "## Nope", Intent: "replace", Markdown: "x\n"}} }, "invalid-section-heading"}, @@ -387,3 +401,561 @@ func TestChangeGroomPlanToleratesMissingUpdatedField(t *testing.T) { t.Errorf("updated not inserted from the clock on a record lacking it:\n%s", rec) } } + +// revisableChange renders a proposed change already groomed to a spec: the +// groomable fixture with spec: linked to specPath. +func revisableChange(id int, slug, specPath string) string { + return strings.Replace(groomableChange(id, slug), "spec:\n", "spec: '"+specPath+"'\n", 1) +} + +// trivialChange renders a proposed change already groomed by trivial verdict. +func trivialChange(id int, slug string) string { + return strings.Replace(groomableChange(id, slug), "trivial: false", "trivial: true", 1) +} + +// fakeTreeBlobID is the uniform blob id newFakeTree reports for every path, so +// it is the spec_version that matches the linked spec on a fake tree. +const fakeTreeBlobID = "a" + +// validReviseRequest is a well-formed revise request (sections + spec body) +// against the revisable fixture at id 2 / slug add-a-widget. +func validReviseRequest() ChangeGroomRequest { + return ChangeGroomRequest{ + ChangeID: 2, + Path: groomPath(2, "add-a-widget"), + Version: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + Outcome: GroomRevise, + SpecMarkdown: "# Design\n\nThe revised design body.\n", + SpecVersion: fakeTreeBlobID, + Sections: []SectionEditRequest{ + {Heading: "## What changes", Intent: "replace", Markdown: "Narrowed what.\n"}, + }, + } +} + +func TestChangeGroomReviseShapeValidation(t *testing.T) { + cases := []struct { + name string + mut func(*ChangeGroomRequest) + code string // "" means the request must pass shape validation + }{ + {"valid revise passes", func(r *ChangeGroomRequest) {}, ""}, + {"sections-only revise passes", func(r *ChangeGroomRequest) { r.SpecMarkdown, r.SpecVersion = "", "" }, ""}, + {"spec-only revise passes", func(r *ChangeGroomRequest) { r.Sections = nil }, ""}, + {"empty revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = nil + }, "empty-revise"}, + {"all-preserve revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = []SectionEditRequest{{Heading: "## Why", Intent: "preserve"}} + }, "empty-revise"}, + {"relationships-only revise refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + r.Sections = nil + r.DependsOn = []int{1} + }, "empty-revise"}, + {"unparseable revise spec_markdown refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "---\nid: 1\n" + }, "invalid-spec_markdown"}, + // An author resubmitting the spec file as read keeps its backlink block; + // the revise re-renders that block itself, so the resubmission is refused + // rather than committed with a duplicate marker pair. + {"revise spec_markdown carrying a backlink block refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = groomBacklinkedSpecMarkdown + }, "invalid-spec_markdown"}, + // A spec-body revise overwrites the spec file, so it must pin its version. + {"spec revise without spec_version refused", func(r *ChangeGroomRequest) { + r.SpecVersion = "" + }, "empty-spec_version"}, + {"sections-only revise without spec_version passes", func(r *ChangeGroomRequest) { + r.SpecMarkdown, r.SpecVersion = "", "" + }, ""}, + // spec_version pins only a spec-body revise; anywhere else it would be + // silently unchecked, so it is refused. + {"sections-only revise with spec_version refused", func(r *ChangeGroomRequest) { + r.SpecMarkdown = "" + }, "invalid-spec_version"}, + {"spec outcome with spec_version refused", func(r *ChangeGroomRequest) { + r.Outcome = GroomSpec + }, "invalid-spec_version"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := validReviseRequest() + c.mut(&req) + findings := validateChangeGroomShape(req) + if c.code == "" { + if len(findings) != 0 { + t.Fatalf("unexpected shape findings: %v", findings) + } + return + } + if !hasFindingCode(findings, c.code) { + t.Errorf("missing finding %q; got %v", c.code, findings) + } + }) + } +} + +func TestChangeGroomEmptyReviseRefusedWithoutEngineCall(t *testing.T) { + req := validReviseRequest() + req.SpecMarkdown = "" + req.Sections = nil + engine := &recordingEngine{} + reader := &fakeChangeReader{pin: mainModePin([]string{"inline"})} + deps := PlanningDeps{Engine: engine, Reader: reader, Clock: testClock()} + + res := ChangeGroom(context.Background(), deps, "", req) + + if res.Result != ResultInvalidInput { + t.Fatalf("result = %q, want invalid-input", res.Result) + } + if len(engine.calls) != 0 { + t.Errorf("engine called %d times on an empty revise, want 0", len(engine.calls)) + } + if !hasFindingCode(res.Findings, "empty-revise") { + t.Errorf("missing finding empty-revise; got %v", res.Findings) + } +} + +// TestChangeGroomSpecReviseWithoutSpecVersionRefusedWithoutEngineCall pins the +// review finding: a spec-body revise that does not pin the spec file's blob +// version is a shape refusal, never an unpinned whole-body replace. +func TestChangeGroomSpecReviseWithoutSpecVersionRefusedWithoutEngineCall(t *testing.T) { + req := validReviseRequest() + req.SpecVersion = "" + engine := &recordingEngine{} + reader := &fakeChangeReader{pin: mainModePin([]string{"inline"})} + deps := PlanningDeps{Engine: engine, Reader: reader, Clock: testClock()} + + res := ChangeGroom(context.Background(), deps, "", req) + + if res.Result != ResultInvalidInput { + t.Fatalf("result = %q, want invalid-input", res.Result) + } + if len(engine.calls) != 0 { + t.Errorf("engine called %d times on an unpinned spec revise, want 0", len(engine.calls)) + } + if !hasFindingCode(res.Findings, "empty-spec_version") { + t.Errorf("missing finding empty-spec_version; got %v", res.Findings) + } +} + +// TestChangeGroomSpecVersionMismatchMapsToContended pins the result mapping: a +// plan refused with spec-version-mismatch is the spec analogue of a stale record +// pin, so the caller sees contended (re-read and retry), not invalid-state. +func TestChangeGroomSpecVersionMismatchMapsToContended(t *testing.T) { + _, opRes := groomPlanFor(t, reviseFixtureFiles(), baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("precondition: a matching spec_version must plan; got %v", opRes.Findings) + } + res := changeGroomResultFromOutcome(transaction.Result{ + Disposition: transaction.DispositionRefused, + Findings: []domain.Finding{{ + Code: "spec-version-mismatch", Severity: domain.SeverityError, + Entity: domain.EntityRef{Kind: domain.EntityChange}, + }}, + }, nil) + if res.Result != ResultContended { + t.Errorf("spec-version-mismatch refusal = %q, want contended", res.Result) + } + other := changeGroomResultFromOutcome(transaction.Result{ + Disposition: transaction.DispositionRefused, + Findings: []domain.Finding{{Code: "not-revisable", Severity: domain.SeverityError}}, + }, nil) + if other.Result != ResultInvalidState { + t.Errorf("not-revisable refusal = %q, want invalid-state", other.Result) + } +} + +// TestChangeGroomReviseSpecVersionContendsRealGit drives the finding's exact +// race through a real engine and a bare origin: two revises pinned to the SAME +// record version (a same-day spec-only revise leaves the record bytes +// unchanged) and the same spec version. The first applies; the second's spec +// pin is stale, so it contends and writes nothing instead of clobbering the +// first revise's spec body. +func TestChangeGroomReviseSpecVersionContendsRealGit(t *testing.T) { + requireRealGit(t) + recPath := groomPath(2, "add-a-widget") + repo := newWorkingRepo(t, reviseFixtureFiles()) + node := planningDepsFor(t, repo.invocation) + + revise := func(body, recV, specV string) ChangeGroomResult { + req := validReviseRequest() + req.Sections = nil + req.SpecMarkdown = "# Design\n\n" + body + "\n" + req.Version, req.SpecVersion = recV, specV + return ChangeGroom(context.Background(), node.deps, node.dir, req) + } + + // Settle the record (updated: today, artifacts rendered) with a matching + // pin — the matching-version apply path. + if res := revise("Settling body.", blobVersionAt(t, repo.origin, "docket", recPath), + blobVersionAt(t, repo.origin, "docket", reviseSpecPath)); res.Result != ResultApplied { + t.Fatalf("settling revise = %q (findings %v), want applied", res.Result, res.Findings) + } + recV := blobVersionAt(t, repo.origin, "docket", recPath) + specV := blobVersionAt(t, repo.origin, "docket", reviseSpecPath) + + if res := revise("Body A.", recV, specV); res.Result != ResultApplied { + t.Fatalf("revise A = %q (findings %v), want applied", res.Result, res.Findings) + } + if got := blobVersionAt(t, repo.origin, "docket", recPath); got != recV { + t.Fatalf("precondition: a same-day spec-only revise must leave the record version unchanged (%s -> %s)", recV, got) + } + tip := originTip(t, repo.origin, "docket") + + res := revise("Body B.", recV, specV) // stale spec pin, current record pin + if res.Result != ResultContended { + t.Fatalf("revise B over a stale spec_version = %q (findings %v), want contended", res.Result, res.Findings) + } + if !hasFindingCode(res.Findings, "spec-version-mismatch") { + t.Errorf("missing finding spec-version-mismatch; got %v", res.Findings) + } + if got := originTip(t, repo.origin, "docket"); got != tip { + t.Errorf("a contended revise moved the metadata branch %s -> %s", tip, got) + } + spec, _ := originFile(t, repo.origin, "docket", reviseSpecPath) + if !strings.Contains(spec, "Body A.") || strings.Contains(spec, "Body B.") { + t.Errorf("revise A's spec body was clobbered:\n%s", spec) + } +} + +const reviseSpecPath = "docs/superpowers/specs/2026-08-01-add-a-widget-design.md" + +// reviseFixtureFiles is the fake tree for a revisable spec'd change: the +// record with spec: linked, and the spec file itself with a backlink block. +func reviseFixtureFiles() map[string]string { + return map[string]string{ + groomPath(2, "add-a-widget"): revisableChange(2, "add-a-widget", reviseSpecPath), + reviseSpecPath: "\n" + + "> old backlink\n" + + "\n\n# Design\n\nThe original design body.\n", + } +} + +// reviseFixtureAtStatus is reviseFixtureFiles with the record moved off +// proposed to status at recPath (a terminal status lives under archive/), extra +// carrying whatever frontmatter that status requires to load coherently — +// spec acceptance item 7's non-proposed refusal rows. +func reviseFixtureAtStatus(recPath, status, extra string) map[string]string { + files := reviseFixtureFiles() + src := groomPath(2, "add-a-widget") + rec := strings.Replace(files[src], "status: proposed\n", "status: "+status+"\n"+extra, 1) + if status == "implemented" { + rec = strings.Replace(rec, "plan:\n", "plan: 'docs/superpowers/plans/2026-08-10-add-a-widget.md'\n", 1) + } + delete(files, src) + files[recPath] = rec + return files +} + +const ( + reviseArchivePath = "docs/changes/archive/2026-08-10-0002-add-a-widget.md" + reviseClaimFields = "branch: 'feat/add-a-widget'\nclaimed_at: '2026-08-10T00:00:00Z'\n" + reviseImplementedFields = reviseClaimFields + "pr: 'https://github.com/o/r/pull/7'\nreconciled: true\n" +) + +// assertGroomReceiptSpecPath decodes the plan's canonical receipt and pins its +// spec_path field. +func assertGroomReceiptSpecPath(t *testing.T, plan transaction.MutationPlan, want string) { + t.Helper() + var rec changeGroomReceipt + if err := json.Unmarshal(plan.Receipt, &rec); err != nil { + t.Fatalf("decoding receipt %s: %v", plan.Receipt, err) + } + if rec.SpecPath != want { + t.Errorf("receipt spec_path = %q, want %q (receipt %s)", rec.SpecPath, want, plan.Receipt) + } +} + +func TestChangeGroomPlanReviseSectionsOnly(t *testing.T) { + files := reviseFixtureFiles() + files["docs/changes/BOARD.md"] = "# Backlog\n\nold\n" + req := validReviseRequest() + req.SpecMarkdown = "" // sections only + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{"inline"}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + // Spec item 1: record + board replaced; the spec file is NOT in the plan, + // so it stays byte-identical by construction. + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + "docs/changes/BOARD.md": transaction.MutationReplace, + }) + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + if strings.Contains(rec, "Original what.") || !strings.Contains(rec, "Narrowed what.") { + t.Errorf("## What changes section not replaced:\n%s", rec) + } + if !strings.Contains(rec, "updated: '2026-08-16'") { + t.Errorf("updated not stamped from the clock:\n%s", rec) + } + // Spec item 9: revise never flips the groomed-outcome fields. + if !strings.Contains(rec, "spec: '"+reviseSpecPath+"'") { + t.Errorf("spec field changed under revise:\n%s", rec) + } + if !strings.Contains(rec, "trivial: false") { + t.Errorf("trivial field changed under revise:\n%s", rec) + } + // A sections-only revise replaced no spec body, so its receipt names none. + assertGroomReceiptSpecPath(t, plan, "") +} + +func TestChangeGroomPlanReviseSpecBodyOnly(t *testing.T) { + files := reviseFixtureFiles() + before := files[groomPath(2, "add-a-widget")] + req := validReviseRequest() + req.Sections = nil // spec body only + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + // Spec item 2: the spec file is REPLACED at the existing path, never created + // at a new dated path. + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + reviseSpecPath: transaction.MutationReplace, + }) + spec := string(groomedRecordBytes(t, plan, reviseSpecPath)) + if !strings.Contains(spec, "docket:backlink:start") { + t.Errorf("revised spec file missing backlink block:\n%s", spec) + } + if !strings.Contains(spec, "The revised design body.") || strings.Contains(spec, "The original design body.") { + t.Errorf("spec body not replaced:\n%s", spec) + } + // Spec item 2: the record's sections are byte-identical apart from + // updated:. The docket:artifacts block legitimately re-renders (the same + // call every groom outcome makes — the empty fixture block gains a Spec + // row), so compare the authored body AFTER the artifacts block, plus the + // frontmatter fields, rather than the whole file. + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + bodyAfterArtifacts := func(s string) string { + i := strings.Index(s, "docket:artifacts:end") + if i < 0 { + t.Fatalf("record lacks the artifacts end marker:\n%s", s) + } + return s[i:] + } + if got, want := bodyAfterArtifacts(rec), bodyAfterArtifacts(before); got != want { + t.Errorf("authored body changed under a spec-only revise:\ngot:\n%s\nwant:\n%s", got, want) + } + if !strings.Contains(rec, "updated: '2026-08-16'") { + t.Errorf("updated not stamped:\n%s", rec) + } + if !strings.Contains(rec, "spec: '"+reviseSpecPath+"'") || !strings.Contains(rec, "trivial: false") { + t.Errorf("groomed-outcome fields changed under a spec-only revise:\n%s", rec) + } + // The receipt names the existing linked spec path the revise replaced. + assertGroomReceiptSpecPath(t, plan, reviseSpecPath) +} + +func TestChangeGroomPlanReviseBoth(t *testing.T) { + // Spec item 3: both edits land in one plan. + plan, opRes := groomPlanFor(t, reviseFixtureFiles(), baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + reviseSpecPath: transaction.MutationReplace, + }) +} + +func TestChangeGroomPlanReviseTrivialRationale(t *testing.T) { + // Spec item 4: sections-only revise of a trivial-verdicted change. + files := map[string]string{ + groomPath(2, "add-a-widget"): trivialChange(2, "add-a-widget"), + } + req := validReviseRequest() + // A trivial change links no spec, so there is no spec file to pin. + req.SpecMarkdown, req.SpecVersion = "", "" + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + groomPath(2, "add-a-widget"): transaction.MutationReplace, + }) + rec := string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + if !strings.Contains(rec, "trivial: true") { + t.Errorf("trivial verdict lost under revise:\n%s", rec) + } + if strings.Contains(rec, "spec: '") { + t.Errorf("revise of a trivial change wrote a spec link:\n%s", rec) + } +} + +func TestChangeGroomPlanReviseRefusals(t *testing.T) { + cases := []struct { + name string + files map[string]string + mut func(*ChangeGroomRequest) + code string + }{ + // Spec item 5: spec_markdown against a trivial-only (no-spec) change. + {"spec-not-linked", map[string]string{ + groomPath(2, "add-a-widget"): trivialChange(2, "add-a-widget"), + }, func(r *ChangeGroomRequest) {}, "spec-not-linked"}, + // Spec item 6: a needs-brainstorm change is groom's target, not revise's. + {"not-revisable needs-brainstorm", map[string]string{ + groomPath(2, "add-a-widget"): groomableChange(2, "add-a-widget"), + }, func(r *ChangeGroomRequest) {}, "not-revisable"}, + // Spec item 7: a non-proposed change. + {"not-revisable blocked", map[string]string{ + groomPath(2, "add-a-widget"): strings.Replace( + revisableChange(2, "add-a-widget", reviseSpecPath), + "status: proposed\n", "status: blocked\nblocked_by: 'waiting'\n", 1), + }, func(r *ChangeGroomRequest) {}, "not-revisable"}, + {"not-revisable in-progress", reviseFixtureAtStatus(groomPath(2, "add-a-widget"), "in-progress", reviseClaimFields), + func(r *ChangeGroomRequest) {}, "not-revisable"}, + {"not-revisable deferred", reviseFixtureAtStatus(groomPath(2, "add-a-widget"), "deferred", ""), + func(r *ChangeGroomRequest) {}, "not-revisable"}, + {"not-revisable implemented", reviseFixtureAtStatus(groomPath(2, "add-a-widget"), "implemented", reviseImplementedFields), + func(r *ChangeGroomRequest) {}, "not-revisable"}, + {"not-revisable done", reviseFixtureAtStatus(reviseArchivePath, "done", ""), + func(r *ChangeGroomRequest) { r.Path = reviseArchivePath }, "not-revisable"}, + {"not-revisable killed", reviseFixtureAtStatus(reviseArchivePath, "killed", ""), + func(r *ChangeGroomRequest) { r.Path = reviseArchivePath }, "not-revisable"}, + // Review Focus 3: dangling spec link — spec: names a path absent from + // the tree; never silently mint a file. + {"spec-file-missing", map[string]string{ + groomPath(2, "add-a-widget"): revisableChange(2, "add-a-widget", reviseSpecPath), + }, func(r *ChangeGroomRequest) {}, "spec-file-missing"}, + // The spec file is pinned at the path the record links: a stale + // spec_version refuses rather than overwriting a newer spec body. + {"spec-version-mismatch", reviseFixtureFiles(), func(r *ChangeGroomRequest) { + r.SpecVersion = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + }, "spec-version-mismatch"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := validReviseRequest() + c.mut(&req) + plan, opRes := groomPlanFor(t, c.files, baseGroomOp([]string{}, req)) + if !opRes.Refused { + t.Fatalf("expected a refusal, got plan files %v", planPaths(plan)) + } + found := false + for _, f := range opRes.Findings { + if f.Code == c.code { + found = true + } + } + if !found { + t.Errorf("missing refusal code %q; got %v", c.code, opRes.Findings) + } + if len(plan.Files) != 0 { + t.Errorf("refused plan still carries files: %v", planPaths(plan)) + } + }) + } +} + +func TestChangeGroomPlanReviseRepeatable(t *testing.T) { + // Spec item 11 (plan level): a second revise over the first revise's own + // output succeeds — no one-shot marker exists. + files := reviseFixtureFiles() + plan1, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("first revise refused: %v", opRes.Findings) + } + files[groomPath(2, "add-a-widget")] = string(groomedRecordBytes(t, plan1, groomPath(2, "add-a-widget"))) + files[reviseSpecPath] = string(groomedRecordBytes(t, plan1, reviseSpecPath)) + req2 := validReviseRequest() + req2.SpecMarkdown = "# Design\n\nThe twice-revised body.\n" + plan2, opRes2 := groomPlanFor(t, files, baseGroomOp([]string{}, req2)) + if opRes2.Refused { + t.Fatalf("second revise refused: %v", opRes2.Findings) + } + spec := string(groomedRecordBytes(t, plan2, reviseSpecPath)) + if !strings.Contains(spec, "The twice-revised body.") { + t.Errorf("second revise did not land:\n%s", spec) + } +} + +// reviseSettledFiles runs validReviseRequest once and feeds its output back as +// the tree: the record's updated: already equals the clock date and its +// docket:artifacts block is already rendered, so a follow-up revise that +// changes nothing in the record yields record bytes identical to the source. +func reviseSettledFiles(t *testing.T) map[string]string { + t.Helper() + files := reviseFixtureFiles() + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("settling revise refused: %v", opRes.Findings) + } + files[groomPath(2, "add-a-widget")] = string(groomedRecordBytes(t, plan, groomPath(2, "add-a-widget"))) + files[reviseSpecPath] = string(groomedRecordBytes(t, plan, reviseSpecPath)) + return files +} + +// TestChangeGroomPlanReviseOmitsUnchangedRecord pins the review blocker: the +// engine's verifyActualDelta rejects a declared path whose bytes did not change, +// so a spec-only revise whose record re-renders byte-identical (updated: already +// today, artifacts already rendered) must declare ONLY the spec file. +func TestChangeGroomPlanReviseOmitsUnchangedRecord(t *testing.T) { + files := reviseSettledFiles(t) + req := validReviseRequest() + req.Sections = nil + req.SpecMarkdown = "# Design\n\nA different body.\n" + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + assertPlanPaths(t, plan, map[string]transaction.MutationKind{ + reviseSpecPath: transaction.MutationReplace, + }) +} + +// TestChangeGroomPlanReviseIdenticalIsNoOp pins that a revise whose spec body +// and section text equal what is already on the tree declares nothing: an +// empty plan the engine commits as a clean no-op, never an unchanged replace +// the delta verifier fails. +func TestChangeGroomPlanReviseIdenticalIsNoOp(t *testing.T) { + files := reviseSettledFiles(t) + files["docs/changes/BOARD.md"] = "# Backlog\n\nold\n" + // Settle the board too, so inline rendering has nothing to change. + boardPlan, opRes := groomPlanFor(t, files, baseGroomOp([]string{"inline"}, validReviseRequest())) + if opRes.Refused { + t.Fatalf("board-settling revise refused: %v", opRes.Findings) + } + files["docs/changes/BOARD.md"] = string(groomedRecordBytes(t, boardPlan, "docs/changes/BOARD.md")) + + cases := []struct { + name string + mut func(*ChangeGroomRequest) + }{ + {"identical spec and section", func(r *ChangeGroomRequest) {}}, + {"identical spec only", func(r *ChangeGroomRequest) { r.Sections = nil }}, + {"identical section only", func(r *ChangeGroomRequest) { r.SpecMarkdown = "" }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := validReviseRequest() + c.mut(&req) + plan, opRes := groomPlanFor(t, files, baseGroomOp([]string{"inline"}, req)) + if opRes.Refused { + t.Fatalf("unexpected refusal: %v", opRes.Findings) + } + if len(plan.Files) != 0 { + t.Errorf("identical revise declared files %v, want an empty (no-op) plan", planPaths(plan)) + } + }) + } +} + +func TestChangeGroomResultHumanTextRevise(t *testing.T) { + r := newChangeGroomResult(ResultApplied, ChangeGroomResult{ + ID: 7, Outcome: string(GroomRevise), SpecPath: "docs/superpowers/specs/x.md", + Revision: "cafebabecafebabecafebabecafebabecafebabe", + }) + got := r.HumanText() + want := "change 0007 revised — cafebabecafebabecafebabecafebabecafebabe" + if got != want { + // Review Focus 4: a revise carrying a spec path must NOT render as + // "groomed (spec …)". + t.Errorf("HumanText = %q, want %q", got, want) + } +} diff --git a/internal/app/change_integration_test.go b/internal/app/change_integration_test.go index 53caccc21..38c78c47b 100644 --- a/internal/app/change_integration_test.go +++ b/internal/app/change_integration_test.go @@ -4020,3 +4020,43 @@ func TestIntegrationChangeRuntimeRunVerifyWaitingTerminalOverridesDeadline(t *te t.Fatalf("verdict = %q, want %q", res.Verdict, VerdictRunWaiting) } } + +func TestIntegrationChangeAuthoringReviseAppliedResult(t *testing.T) { + repoDir := newWorkingRepo(t, nil).invocation + specPath := "docs/superpowers/specs/2026-08-01-add-a-widget-design.md" + receipt := mustMarshal(t, changeGroomReceipt{ + ID: 2, Op: OperationChangeGroom, Outcome: string(GroomRevise), SpecPath: specPath, + }) + engine := &recordingEngine{result: transaction.Result{ + Disposition: transaction.DispositionApplied, + AppliedCommit: "cafebabecafebabecafebabecafebabecafebabe", + Receipt: receipt, + }} + reader := &fakeChangeReader{pin: mainModePin([]string{"inline"})} + deps := PlanningDeps{Client: newGitClient(t), Engine: engine, Reader: reader, Clock: testClock()} + + res := ChangeGroom(context.Background(), deps, repoDir, validReviseRequest()) + + if res.Result != ResultApplied { + t.Fatalf("result = %q, want applied", res.Result) + } + if res.Outcome != string(GroomRevise) { + t.Errorf("outcome = %q, want revise", res.Outcome) + } + if res.ID != 2 || res.SpecPath != specPath { + t.Errorf("identity from receipt = (%d, %q)", res.ID, res.SpecPath) + } + // Spec items 10/11 (engine-level): the CAS expectation pins the exact + // submitted version on every call, revise included — repeatability is a + // second standard call with the freshly-read version, nothing more. + if len(engine.calls) != 1 { + t.Fatalf("engine calls = %d, want 1", len(engine.calls)) + } + // The engine pins the record alone; the spec file's version is checked in + // Plan against the path the record links (no caller-supplied spec path). + exp := engine.calls[0].Expected + if len(exp) != 1 || + string(exp[0].Path) != validReviseRequest().Path || string(exp[0].Version.ObjectID) != validReviseRequest().Version { + t.Errorf("revise did not pin exactly the submitted record version: %+v", exp) + } +} diff --git a/internal/app/finding_codes.go b/internal/app/finding_codes.go index 94ad13c3c..4258c48bb 100644 --- a/internal/app/finding_codes.go +++ b/internal/app/finding_codes.go @@ -122,7 +122,10 @@ const ( FCInvalidTopics FindingCode = "invalid-topics" FCEmptySpecMarkdown FindingCode = "empty-spec_markdown" FCInvalidSpecMarkdown FindingCode = "invalid-spec_markdown" + FCEmptySpecVersion FindingCode = "empty-spec_version" + FCInvalidSpecVersion FindingCode = "invalid-spec_version" FCMissingRationale FindingCode = "missing-rationale" + FCEmptyRevise FindingCode = "empty-revise" FCInvalidOutcome FindingCode = "invalid-outcome" FCInvalidSpecSectionHeading FindingCode = "invalid-spec-section-heading" FCEmptyReconcileLogEntry FindingCode = "empty-reconcile_log_entry" @@ -152,14 +155,15 @@ const ( // through their addShape/adrFinding/learningFinding closures are now registered // FindingCode constants (change 0399, review): invalid-request_id, // invalid-stacked_on, invalid-{target-id,topics,change-id,outcome,pr_number, -// attempt,spec_markdown,spec-section-heading}, missing-rationale, and the +// attempt,spec_markdown,spec_version,spec-section-heading}, missing-rationale, and the // enumerated empty- expansions (empty-{title,why,what_changes, // out_of_scope,context,decision,consequences,alternatives,change-path, // change-version,target-path,target-version,hook,apply,war_story,spec_markdown, -// reconcile_log_entry,head}). Each expands to exactly one registered member, so -// the vocabulary is closed over every value these ops can emit and the minting -// guard (addShape/adrFinding/learningFinding in ctorLit, plus the composite- -// literal and FindingCode("…") backstops) reddens on any unregistered mint. +// spec_version,reconcile_log_entry,head}). Each expands to exactly +// one registered member, so the vocabulary is closed over every value these +// ops can emit and the minting guard (addShape/adrFinding/learningFinding in +// ctorLit, plus the composite-literal and FindingCode("…") backstops) reddens +// on any unregistered mint. // // KNOWN GAPS (still deferred): the app-local ReasonBacklink*/ReasonCloseout* // reason families surface through fail.Reason rather than a literal Code:/ @@ -206,7 +210,9 @@ var AllFindingCodes = []FindingCode{ FCEmptyReason, FCEmptyReconcileLogEntry, FCEmptyReport, + FCEmptyRevise, FCEmptySpecMarkdown, + FCEmptySpecVersion, FCEmptyTargetPath, FCEmptyTargetVersion, FCEmptyTitle, @@ -244,6 +250,7 @@ var AllFindingCodes = []FindingCode{ FCInvalidSlug, FCInvalidSpecSectionHeading, FCInvalidSpecMarkdown, + FCInvalidSpecVersion, FCInvalidStackedOn, FindingCode("invalid-successor-id"), FCInvalidTargetID, diff --git a/internal/app/finding_codes_test.go b/internal/app/finding_codes_test.go index e81333e23..be761ec91 100644 --- a/internal/app/finding_codes_test.go +++ b/internal/app/finding_codes_test.go @@ -229,6 +229,9 @@ func TestShapeValidatorCodesAreRegistered(t *testing.T) { emitted = append(emitted, validateLearningRecordShape(LearningRecordRequest{Topics: []string{""}})...) emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomSpec})...) emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomTrivial})...) + emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomRevise})...) + emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomRevise, SpecMarkdown: "# x\n"})...) + emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomTrivial, SpecVersion: "a"})...) emitted = append(emitted, validateChangeGroomShape(ChangeGroomRequest{Outcome: GroomOutcome("bogus")})...) emitted = append(emitted, validateChangeReconcileShape(ChangeReconcileRequest{ Sections: map[string]string{"## Not Owned": "x"}, @@ -254,7 +257,7 @@ func TestShapeValidatorCodesAreRegistered(t *testing.T) { FCInvalidChangeDotID, FCEmptyChangePath, FCEmptyChangeVersion, FCInvalidTargetID, FCEmptyTargetPath, FCEmptyTargetVersion, FCEmptyHook, FCEmptyApply, FCEmptyWarStory, FCInvalidTopics, - FCEmptySpecMarkdown, FCMissingRationale, FCInvalidOutcome, + FCEmptySpecMarkdown, FCEmptySpecVersion, FCInvalidSpecVersion, FCMissingRationale, FCInvalidOutcome, FCInvalidSpecSectionHeading, FCEmptyReconcileLogEntry, FCInvalidPRNumber, FCInvalidAttempt, FCEmptyHead, } diff --git a/internal/app/schema_test.go b/internal/app/schema_test.go index ac176572c..b178b21b1 100644 --- a/internal/app/schema_test.go +++ b/internal/app/schema_test.go @@ -96,6 +96,18 @@ func TestReflectDescriptorChangeGroomRequest(t *testing.T) { if sm.Required || sm.Type != "string" { t.Errorf("spec_markdown = %+v, want optional string", sm) } + // spec_version: the linked spec's pin (conditionally required by the + // validator on a spec-body revise, so not docket:"required"). + if f := fieldByKey(t, d, "spec_version"); f.Required || f.Type != "string" { + t.Errorf("spec_version = %+v, want optional string", f) + } + // No caller-supplied spec path: the spec pinned is always the one the + // record's spec: field links. + for _, f := range d.Fields { + if f.Key == "spec_path" { + t.Errorf("request carries a spec_path field; the linked spec path comes from the record") + } + } // sections repeated object with heading/intent/markdown. sec := fieldByKey(t, d, "sections") diff --git a/internal/app/schema_vocab.go b/internal/app/schema_vocab.go index b4edac3a6..793204aa6 100644 --- a/internal/app/schema_vocab.go +++ b/internal/app/schema_vocab.go @@ -68,7 +68,7 @@ func SchemaVocabularies(effects []string) map[string]Vocabulary { v["section_intents"] = Vocabulary{Members: members(len(render.AllSectionIntents), func(i int) string { return string(render.AllSectionIntents[i]) })} - v["groom_outcomes"] = Vocabulary{Members: []string{string(GroomSpec), string(GroomTrivial)}} + v["groom_outcomes"] = Vocabulary{Members: []string{string(GroomSpec), string(GroomTrivial), string(GroomRevise)}} v["change_types"] = Vocabulary{Pattern: changeTypePattern} v["effects"] = Vocabulary{Members: effects} diff --git a/internal/app/schema_vocab_test.go b/internal/app/schema_vocab_test.go index 90849d9e8..f362d3581 100644 --- a/internal/app/schema_vocab_test.go +++ b/internal/app/schema_vocab_test.go @@ -45,7 +45,7 @@ func TestSchemaVocabulariesCore(t *testing.T) { assertVocabMembers(t, v, "priorities", []string{"critical", "high", "medium", "low"}) assertVocabMembers(t, v, "section_intents", []string{"preserve", "replace", "remove"}) - assertVocabMembers(t, v, "groom_outcomes", []string{"spec", "trivial"}) + assertVocabMembers(t, v, "groom_outcomes", []string{"spec", "trivial", "revise"}) assertVocabMembers(t, v, "statuses", []string{ "proposed", "in-progress", "blocked", "deferred", "implemented", "stacked-merged", "done", "killed", diff --git a/internal/assets/embedded/manifest.json b/internal/assets/embedded/manifest.json index e7ca9a64a..9950eb2ad 100644 --- a/internal/assets/embedded/manifest.json +++ b/internal/assets/embedded/manifest.json @@ -1,7 +1,7 @@ { "format_version": 1, "asset_protocol": 1, - "asset_set_id": "sha256:8046d8abb1608ff89409f95f109c7770d0c2a617502ab7e5a7ab8878e8957d0a", + "asset_set_id": "sha256:59310c05314464c379449e18f9be6b1c93d44492585c7ca29f2f94274c4adb95", "entries": [ { "path": ".docket.example.yml", @@ -399,8 +399,8 @@ "path": "skills/docket-groom-next/SKILL.md", "role": "skill", "mode": 420, - "size": 11034, - "sha256": "f663dcabef0e5add5dfca3cc10c68fea709ed3ba7e92f58f28fa08a4ead7b841" + "size": 12889, + "sha256": "59bf547fd16711a60708531573549bd70a5a2d0f325050f7323a4389e54233d8" }, { "path": "skills/docket-implement-next/SKILL.md", @@ -434,8 +434,8 @@ "path": "skills/docket-new-change/SKILL.md", "role": "skill", "mode": 420, - "size": 11552, - "sha256": "06791b3c54cc809f728d8c398cd7daeb517b158af14ab2d621506d2c92f90a17" + "size": 11796, + "sha256": "8df6492648b47579aeafd2e1f6809ccd423b60857fa1ac184cf4ce44228a8dd1" }, { "path": "skills/docket-new-change/change-template.md", diff --git a/internal/assets/embedded/tree/skills/docket-groom-next/SKILL.md b/internal/assets/embedded/tree/skills/docket-groom-next/SKILL.md index 82e06c590..b1725de85 100644 --- a/internal/assets/embedded/tree/skills/docket-groom-next/SKILL.md +++ b/internal/assets/embedded/tree/skills/docket-groom-next/SKILL.md @@ -1,6 +1,6 @@ --- name: docket-groom-next -description: Use when stubs are sitting at needs-brainstorm on the docket board and you want the next one designed — selecting the next needs-brainstorm change (proposed, no spec, not trivial) deterministically and grooming it to build-ready through an interactive brainstorm with the human, exiting with a linked spec, a trivial verdict, a kill, or a defer. Selection is autonomous; the design conversation is not. Writes markdown only — never branches, worktrees, or code. +description: Use when stubs are sitting at needs-brainstorm on the docket board and you want the next one designed — selecting the next needs-brainstorm change (proposed, no spec, not trivial) deterministically and grooming it to build-ready through an interactive brainstorm with the human, exiting with a linked spec, a trivial verdict, a kill, or a defer — or revising an already-groomed proposed change by explicit id. Selection is autonomous; the design conversation is not. Writes markdown only — never branches, worktrees, or code. --- # docket-groom-next — the groomer (interactive) @@ -13,7 +13,7 @@ description: Use when stubs are sitting at needs-brainstorm on the docket board - Stubs show as needs-brainstorm on the board and you want to design the next one, or a specific one (pass its id explicitly to skip selection). - Do NOT use to capture a brand-new idea — that is `docket-new-change`'s job; this skill never mints ids. -- Do NOT use to re-groom a change that already has a spec — drift against current reality is the reconcile pass's job in `docket-implement-next`. A human who wants to redo a design can clear `spec:` by hand first. +- An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) is NOT an error — it routes to the revise flow (Step 4, exit 5): adjust the existing spec and owned sections through `change.groom` with `outcome: revise`. Drift against current reality at build time remains the reconcile pass's job in `docket-implement-next`. ## Recommended model/effort (advisory) @@ -27,7 +27,7 @@ Invoke the `docket-convention` skill via the Skill tool first — unless already ### Step 1 — Select -Sync the metadata working tree (the Step-0 `repository.prepare` operation), then rank every needs-brainstorm change in `active/` — `status: proposed`, no `spec:`, not `trivial: true` — by the convention's deterministic selection order (the same ranking `docket-implement-next` uses). Pick the top, or accept an explicit id from the caller; an explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. Empty queue → report that nothing needs grooming and stop. Read the selected stub's exact **record path** and opaque **entity version** (the blob object id) from the `status` operation (with `--json`) — the Step-4 groom/defer transactions pin the record with those, and re-reading them after a mid-run re-sync is a fresh `status` read. +Sync the metadata working tree (the Step-0 `repository.prepare` operation), then rank every needs-brainstorm change in `active/` — `status: proposed`, no `spec:`, not `trivial: true` — by the convention's deterministic selection order (the same ranking `docket-implement-next` uses). Pick the top, or accept an explicit id from the caller. An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) routes to the **revise** flow — recap what is there today (the linked spec's body, or the trivial rationale, and the owned sections), run the resolved brainstorm skill seeded with the current design framed as "what would you like to adjust," and exit via Step 4's revise exit. Any other explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. Empty queue → report that nothing needs grooming and stop. Read the selected stub's exact **record path** and opaque **entity version** (the blob object id) from the `status` operation (with `--json`) — the Step-4 groom/defer transactions pin the record with those, and re-reading them after a mid-run re-sync is a fresh `status` read. When autonomous grooming is in play (see the convention's *Autonomous grooming* shared definition), rank in **selection bands** — the human's attention goes first to stubs that need a human: (1) abstained stubs (a `## Auto-groom blocked` section is present — they are literally waiting on you), then (2) effective `auto_groomable: false` stubs, then (3) effective auto-groomable stubs, each flagged "#NNNN is auto-groomable — docket-auto-groom will handle it unless you want it now." Within each band, the deterministic order applies unchanged. Every needs-brainstorm stub stays selectable — bands reorder, they never exclude; an explicit id still overrides everything. @@ -56,18 +56,19 @@ The recap is an introduction, not a confirmation gate — flow directly into the Then run the **resolved brainstorm skill** — `$SKILL_BRAINSTORM` from the Step-0 config export (default `superpowers:brainstorming`) — WITH THE HUMAN, seeded with the stub's body and its `## Open questions` — the open questions are the session's starting agenda. If it resolves to `auto` or cannot be invoked, apply the brainstorm auto-fallback per the convention's *Skill layer* (design inline with the human, warning prominently on unavailability) — the artifact is unchanged: a spec, then stop. If the human asks for a consultant-written spec, invoke `docket-brainstorm` for this run regardless of `$SKILL_BRAINSTORM` — human steering of an interactive session always wins (see the README's consultant-brainstorm section). STOP AT THE SPEC — do NOT continue to `superpowers:writing-plans` (planning is build-time, owned by `docket-implement-next`). -### Step 4 — Exit (one of four; the human confirms which) +### Step 4 — Exit (one of five; the human confirms which) -All four exits reuse existing transitions — this skill introduces no new lifecycle status: +All five exits reuse existing transitions — this skill introduces no new lifecycle status: 1. **Spec** (the normal exit): **author** the settled design as the spec's Markdown body, and any owned proposal-section rewrites (proposal altitude, resolved `## Open questions` removed). Apply it with one atomic transaction — the `change.groom` operation with `--repo-dir .docket --request ` — carrying the change id, the pinned record `path` + `version` from Step 1, `outcome: spec`, the `spec_markdown`, the owned-section edits, and the desired `depends_on`/`related`/`adrs`/`discovered_from`/`stacked_on` (authored Markdown travels in the request file, never a shell-escaped flag). In one metadata commit it writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, and re-renders the `## Artifacts` block and inline board — so there is no separate render, back-link, or Board pass. A typed refusal (not-groomable, spec-path-taken, malformed markers, version mismatch) writes nothing — surface it. The change is now build-ready. 2. **Trivial verdict**: the brainstorm concludes there is no real design question — apply the `change.groom` operation with `--repo-dir .docket --request ` with `outcome: trivial`, the pinned `path` + `version`, and an owned-section edit carrying the tightened body as the trivial rationale. The transaction sets `trivial: true` + `updated:`, re-renders the `## Artifacts` block, and re-renders the inline board atomically — no spec file, no separate Board pass. Also build-ready. 3. **Kill**: the stub is obsolete, a duplicate, or decided against — follow the proposed-kill sub-path in `docket-new-change` (it owns the kill mechanics; do not restate them here). 4. **Defer**: right idea, wrong time — apply the `change.defer` operation with `--repo-dir .docket --request ` with the pinned `path` + `version` and the authored `why_deferred` section body. The transaction sets `status: deferred`, splices the `## Why deferred` section, sets `updated:`, and re-renders the inline board atomically. +5. **Revise** (explicit-id route only): the change is already groomed and the human wants it adjusted — apply the `change.groom` operation with `--repo-dir .docket --request ` with `outcome: revise`, the pinned `path` + `version`, and whichever of `spec_markdown` (a whole-body replace of the *existing* linked spec, minus its `docket:backlink` block, which the transaction re-stamps; it never mints a new path) and owned-section `sections` edits the adjustment touched, plus any relationship-field updates. With `spec_markdown`, also send `spec_version`, the linked spec's blob object id read by `git -C .docket rev-parse HEAD:` (`` is the record's `spec:` value) — a concurrent spec edit then contends instead of being overwritten. It never sets `spec:` or `trivial:`, so a change cannot flip between spec'd and trivial here. Repeatable while the change stays `proposed`. A typed refusal (`not-revisable`, `spec-not-linked`, `spec-file-missing`, `empty-revise`, `empty-spec_version`, version mismatch) writes nothing — surface it. The change stays build-ready. ### Step 5 — The transaction lands (no separate board pass) -The Step-4 typed op is the whole write: it re-checks the pinned exact `version`, commits the change record, the spec, the `## Artifacts` block, and the inline board in one metadata commit pushed to `origin/docket` under an exact-lease push — so there is no separate hand-staged commit and **no separate Board pass** (the readiness cell flips from needs-brainstorm, or the row leaves the Proposed section on a kill or defer, in that same commit). On a `contended` refusal the op writes nothing: re-sync (re-run the `repository.prepare` operation), re-read the record's `path` + `version` from the `status` operation, and — if it is no longer needs-brainstorm (someone else groomed, killed, or claimed it) — STOP and report rather than overwrite; otherwise re-author and retry. STOP — grooming never implements. +The Step-4 typed op is the whole write: it re-checks the pinned exact `version`, commits the change record, the spec, the `## Artifacts` block, and the inline board in one metadata commit pushed to `origin/docket` under an exact-lease push — so there is no separate hand-staged commit and **no separate Board pass** (the readiness cell flips from needs-brainstorm, or the row leaves the Proposed section on a kill or defer, in that same commit; a revise keeps the row build-ready). On a `contended` refusal the op writes nothing: re-sync (re-run the `repository.prepare` operation), re-read the record's `path` + `version` from the `status` operation, and — if it is no longer needs-brainstorm (someone else groomed, killed, or claimed it) — STOP and report rather than overwrite; otherwise re-author and retry. On a revise, also re-read the spec (a fresh `spec_version`) and stop only if the change is no longer an already-groomed `proposed` change. STOP — grooming never implements. ## Concurrency — no claim diff --git a/internal/assets/embedded/tree/skills/docket-new-change/SKILL.md b/internal/assets/embedded/tree/skills/docket-new-change/SKILL.md index 1797cf331..6bc88eb33 100644 --- a/internal/assets/embedded/tree/skills/docket-new-change/SKILL.md +++ b/internal/assets/embedded/tree/skills/docket-new-change/SKILL.md @@ -38,7 +38,7 @@ The default path for any non-trivial new change. Five steps: **Two draft-time scalars `create` does not carry** — `auto_groomable` and `branch_prefix`. When the human says the change may be designed without them, or names a branch prefix ("use the `hotfix/` prefix"), set the scalar directly on the created record's frontmatter and commit it with plain git plumbing (**Stage by explicit path** — that tree is shared, so never `add -A`) — no derived view depends on either, so no re-render: `auto_groomable: true` (so `docket-auto-groom` carries it to build-ready), and/or the normalized `branch_prefix: hotfix` (strip one presentation-only trailing slash, but **refuse and ask the human** on a slash-embedded or `refs/`-qualified value — claim consumes it at mint time, never prose). Leave a field unset to inherit the repo default. -5. **Groom to build-ready & land** — read the created record's `path` + exact **entity version** (blob object id) from the `status` operation (with `--json`), then apply the spec atomically: the `change.groom` operation with `--repo-dir .docket --request ` carrying the change id, the pinned `path` + `version`, `outcome: spec`, the `spec_markdown` from step 2, and any owned proposal-section rewrites. In one metadata commit the transaction writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, re-renders the `## Artifacts` block, and re-renders the inline board (pushed under an exact-lease push) — **no separate Board pass**. A version-mismatch / `contended` refusal writes nothing: re-sync (re-run the `repository.prepare` operation), re-read `path` + `version`, retry. STOP. Never implements. +5. **Groom to build-ready & land** — read the created record's `path` + exact **entity version** (blob object id) from the `status` operation (with `--json`), then apply the spec atomically: the `change.groom` operation with `--repo-dir .docket --request ` carrying the change id, the pinned `path` + `version`, `outcome: spec`, the `spec_markdown` from step 2, and any owned proposal-section rewrites. In one metadata commit the transaction writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, re-renders the `## Artifacts` block, and re-renders the inline board (pushed under an exact-lease push) — **no separate Board pass**. A version-mismatch / `contended` refusal writes nothing: re-sync (re-run the `repository.prepare` operation), re-read `path` + `version`, retry. STOP. Never implements. To adjust a just-landed spec or its owned sections afterwards, run `docket-groom-next ` — its explicit-id path routes an already-groomed change to `change.groom` with `outcome: revise`; never re-run this skill or hand-edit the spec file. **Dummy mode:** when `DUMMY_MODE_ENABLED` is `true` (Step-0 export) — or the human asks for it in-session — write step 2's `dialogue` calibrated to `DUMMY_MODE_PERSONA`, per the convention's *Dummy mode* shared definition. The spec file itself is never simplified. diff --git a/internal/cli/change.go b/internal/cli/change.go index 563a5ceb0..999e4b41b 100644 --- a/internal/cli/change.go +++ b/internal/cli/change.go @@ -60,7 +60,7 @@ func newChangeCommand(setResult func(app.OperationResult)) *cobra.Command { }, EffectMetadataWrite) groom := changeSubcommand("change", "groom", - "Groom a proposed change to build-ready (spec or trivial) from a JSON request", + "Groom a proposed change to build-ready (spec or trivial), or revise an already-groomed one, from a JSON request", func(c *cobra.Command, deps app.PlanningDeps, repoDir string) error { var req app.ChangeGroomRequest if err := decodeRequestFlag(c, &req); err != nil { diff --git a/internal/repoguard/budgets_test.go b/internal/repoguard/budgets_test.go index b3c49a910..b232a52e8 100644 --- a/internal/repoguard/budgets_test.go +++ b/internal/repoguard/budgets_test.go @@ -196,13 +196,13 @@ var skillBudgets = []skillBudget{ {"docket-convention/references/terminal-close-out.md", 240, 2150}, {"docket-finalize-change/SKILL.md", 239, 5520}, // 0442: +post-publication base-advance guidance (word ceiling 5421 -> 5520); de-duplicated the shared forward-rebase mechanic against the 0438 unpublished-case paragraph (reclaimed 57 words), but the distinct published-refresh facts plus the retained 0438 guidance cannot fit the old ceiling without deleting required guidance; 0411: +reconciliation-write recovery exception paragraph in the resolver loop (ceilings 238/5232 -> 239/5421); 0413: +generated-bundle mixed-conflict handoff sentence in the resolver-loop block (word ceiling 5200 -> 5232); 0419: +repair-attempt budget payload line and rewired repair contract (line ceiling 236 -> 238); 0393: +exact payload, marker, and direct-dispatch lines atop 0349/0410 (see note above) {"docket-finalize-change/references/gate-failure.md", 147, 1901}, // 0411: +reconciliation-write exception section and abort-set carve-out (ceilings 135/1472 -> 147/1901); 0413: +conflicted_paths-lists-authored-only rule in the resolver-report section (line ceiling 133 -> 135, word ceiling 1465 -> 1472); 0419: +repair-attempt budget payload and rewired repair contract prose (word ceiling 1450 -> 1465); 0349: +reserve-before-dispatch resolver protocol prose; 0375: +worktree-slot note for the scopeless finalize gate (120/1300 -> 133/1450) - {"docket-groom-next/SKILL.md", 77, 1650}, - {"docket-implement-next/SKILL.md", 214, 8025}, // 0448: +named-invocation branch; bounded own-dependency closeout moved to edge-paths.md (ceilings 210/7716 -> 214/8270 -> 214/8025); 0393: +exact payload, marker, and direct-dispatch lines atop 0410/0354/0376; 0375: +gate-epoch resume pointer (word ceiling 7530 -> 7547); 0440: reader-first results prose - {"docket-implement-next/references/edge-paths.md", 118, 1554}, // 0410: +resume/recovery + required-results reconciliation; 0375: +gate-epoch resume refusals (78/1091 -> 93/1261); 0448: +named own-dependency closeout moved from SKILL.md (93/1261 -> 118/1554) - {"docket-implement-next/references/fix-loop.md", 190, 1958}, // 0410: +findings-to-results checkpoint linkage (see note above) - {"docket-implement-next/results-template.md", 64, 446}, // 0440: reader-first template — action statement + merged Known issues (see note above) - {"docket-review/SKILL.md", 110, 913}, // 0410: +findings-return capture contract (see note above) - {"docket-new-change/SKILL.md", 61, 1700}, + {"docket-groom-next/SKILL.md", 77, 1889}, // 0445: +revise route for already-groomed explicit ids (Step 1) and the fifth Step-4 exit (word ceiling 1650 -> 1813); +spec_version pin for a spec-body revise (1813 -> 1849); +revise spec_markdown excludes the backlink block (1849 -> 1850); +revise in the description and the revise contended/board clauses (1850 -> 1889) + {"docket-implement-next/SKILL.md", 214, 8025}, // 0448: +named-invocation branch; bounded own-dependency closeout moved to edge-paths.md (ceilings 210/7716 -> 214/8270 -> 214/8025); 0393: +exact payload, marker, and direct-dispatch lines atop 0410/0354/0376; 0375: +gate-epoch resume pointer (word ceiling 7530 -> 7547); 0440: reader-first results prose + {"docket-implement-next/references/edge-paths.md", 118, 1554}, // 0410: +resume/recovery + required-results reconciliation; 0375: +gate-epoch resume refusals (78/1091 -> 93/1261); 0448: +named own-dependency closeout moved from SKILL.md (93/1261 -> 118/1554) + {"docket-implement-next/references/fix-loop.md", 190, 1958}, // 0410: +findings-to-results checkpoint linkage (see note above) + {"docket-implement-next/results-template.md", 64, 446}, // 0440: reader-first template — action statement + merged Known issues (see note above) + {"docket-review/SKILL.md", 110, 913}, // 0410: +findings-return capture contract (see note above) + {"docket-new-change/SKILL.md", 61, 1706}, // 0445: +pointer to the docket-groom-next revise path after landing (word ceiling 1700 -> 1706) {"docket-new-change/change-template.md", 51, 250}, {"docket-status/SKILL.md", 140, 3065}, // 0388: +sync-integration prose (see note above) } diff --git a/skills/docket-groom-next/SKILL.md b/skills/docket-groom-next/SKILL.md index 82e06c590..b1725de85 100644 --- a/skills/docket-groom-next/SKILL.md +++ b/skills/docket-groom-next/SKILL.md @@ -1,6 +1,6 @@ --- name: docket-groom-next -description: Use when stubs are sitting at needs-brainstorm on the docket board and you want the next one designed — selecting the next needs-brainstorm change (proposed, no spec, not trivial) deterministically and grooming it to build-ready through an interactive brainstorm with the human, exiting with a linked spec, a trivial verdict, a kill, or a defer. Selection is autonomous; the design conversation is not. Writes markdown only — never branches, worktrees, or code. +description: Use when stubs are sitting at needs-brainstorm on the docket board and you want the next one designed — selecting the next needs-brainstorm change (proposed, no spec, not trivial) deterministically and grooming it to build-ready through an interactive brainstorm with the human, exiting with a linked spec, a trivial verdict, a kill, or a defer — or revising an already-groomed proposed change by explicit id. Selection is autonomous; the design conversation is not. Writes markdown only — never branches, worktrees, or code. --- # docket-groom-next — the groomer (interactive) @@ -13,7 +13,7 @@ description: Use when stubs are sitting at needs-brainstorm on the docket board - Stubs show as needs-brainstorm on the board and you want to design the next one, or a specific one (pass its id explicitly to skip selection). - Do NOT use to capture a brand-new idea — that is `docket-new-change`'s job; this skill never mints ids. -- Do NOT use to re-groom a change that already has a spec — drift against current reality is the reconcile pass's job in `docket-implement-next`. A human who wants to redo a design can clear `spec:` by hand first. +- An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) is NOT an error — it routes to the revise flow (Step 4, exit 5): adjust the existing spec and owned sections through `change.groom` with `outcome: revise`. Drift against current reality at build time remains the reconcile pass's job in `docket-implement-next`. ## Recommended model/effort (advisory) @@ -27,7 +27,7 @@ Invoke the `docket-convention` skill via the Skill tool first — unless already ### Step 1 — Select -Sync the metadata working tree (the Step-0 `repository.prepare` operation), then rank every needs-brainstorm change in `active/` — `status: proposed`, no `spec:`, not `trivial: true` — by the convention's deterministic selection order (the same ranking `docket-implement-next` uses). Pick the top, or accept an explicit id from the caller; an explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. Empty queue → report that nothing needs grooming and stop. Read the selected stub's exact **record path** and opaque **entity version** (the blob object id) from the `status` operation (with `--json`) — the Step-4 groom/defer transactions pin the record with those, and re-reading them after a mid-run re-sync is a fresh `status` read. +Sync the metadata working tree (the Step-0 `repository.prepare` operation), then rank every needs-brainstorm change in `active/` — `status: proposed`, no `spec:`, not `trivial: true` — by the convention's deterministic selection order (the same ranking `docket-implement-next` uses). Pick the top, or accept an explicit id from the caller. An explicit id naming an already-groomed `proposed` change (has `spec:` or `trivial: true`) routes to the **revise** flow — recap what is there today (the linked spec's body, or the trivial rationale, and the owned sections), run the resolved brainstorm skill seeded with the current design framed as "what would you like to adjust," and exit via Step 4's revise exit. Any other explicit id that is not needs-brainstorm is an error to report, never a silent re-pick. Empty queue → report that nothing needs grooming and stop. Read the selected stub's exact **record path** and opaque **entity version** (the blob object id) from the `status` operation (with `--json`) — the Step-4 groom/defer transactions pin the record with those, and re-reading them after a mid-run re-sync is a fresh `status` read. When autonomous grooming is in play (see the convention's *Autonomous grooming* shared definition), rank in **selection bands** — the human's attention goes first to stubs that need a human: (1) abstained stubs (a `## Auto-groom blocked` section is present — they are literally waiting on you), then (2) effective `auto_groomable: false` stubs, then (3) effective auto-groomable stubs, each flagged "#NNNN is auto-groomable — docket-auto-groom will handle it unless you want it now." Within each band, the deterministic order applies unchanged. Every needs-brainstorm stub stays selectable — bands reorder, they never exclude; an explicit id still overrides everything. @@ -56,18 +56,19 @@ The recap is an introduction, not a confirmation gate — flow directly into the Then run the **resolved brainstorm skill** — `$SKILL_BRAINSTORM` from the Step-0 config export (default `superpowers:brainstorming`) — WITH THE HUMAN, seeded with the stub's body and its `## Open questions` — the open questions are the session's starting agenda. If it resolves to `auto` or cannot be invoked, apply the brainstorm auto-fallback per the convention's *Skill layer* (design inline with the human, warning prominently on unavailability) — the artifact is unchanged: a spec, then stop. If the human asks for a consultant-written spec, invoke `docket-brainstorm` for this run regardless of `$SKILL_BRAINSTORM` — human steering of an interactive session always wins (see the README's consultant-brainstorm section). STOP AT THE SPEC — do NOT continue to `superpowers:writing-plans` (planning is build-time, owned by `docket-implement-next`). -### Step 4 — Exit (one of four; the human confirms which) +### Step 4 — Exit (one of five; the human confirms which) -All four exits reuse existing transitions — this skill introduces no new lifecycle status: +All five exits reuse existing transitions — this skill introduces no new lifecycle status: 1. **Spec** (the normal exit): **author** the settled design as the spec's Markdown body, and any owned proposal-section rewrites (proposal altitude, resolved `## Open questions` removed). Apply it with one atomic transaction — the `change.groom` operation with `--repo-dir .docket --request ` — carrying the change id, the pinned record `path` + `version` from Step 1, `outcome: spec`, the `spec_markdown`, the owned-section edits, and the desired `depends_on`/`related`/`adrs`/`discovered_from`/`stacked_on` (authored Markdown travels in the request file, never a shell-escaped flag). In one metadata commit it writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, and re-renders the `## Artifacts` block and inline board — so there is no separate render, back-link, or Board pass. A typed refusal (not-groomable, spec-path-taken, malformed markers, version mismatch) writes nothing — surface it. The change is now build-ready. 2. **Trivial verdict**: the brainstorm concludes there is no real design question — apply the `change.groom` operation with `--repo-dir .docket --request ` with `outcome: trivial`, the pinned `path` + `version`, and an owned-section edit carrying the tightened body as the trivial rationale. The transaction sets `trivial: true` + `updated:`, re-renders the `## Artifacts` block, and re-renders the inline board atomically — no spec file, no separate Board pass. Also build-ready. 3. **Kill**: the stub is obsolete, a duplicate, or decided against — follow the proposed-kill sub-path in `docket-new-change` (it owns the kill mechanics; do not restate them here). 4. **Defer**: right idea, wrong time — apply the `change.defer` operation with `--repo-dir .docket --request ` with the pinned `path` + `version` and the authored `why_deferred` section body. The transaction sets `status: deferred`, splices the `## Why deferred` section, sets `updated:`, and re-renders the inline board atomically. +5. **Revise** (explicit-id route only): the change is already groomed and the human wants it adjusted — apply the `change.groom` operation with `--repo-dir .docket --request ` with `outcome: revise`, the pinned `path` + `version`, and whichever of `spec_markdown` (a whole-body replace of the *existing* linked spec, minus its `docket:backlink` block, which the transaction re-stamps; it never mints a new path) and owned-section `sections` edits the adjustment touched, plus any relationship-field updates. With `spec_markdown`, also send `spec_version`, the linked spec's blob object id read by `git -C .docket rev-parse HEAD:` (`` is the record's `spec:` value) — a concurrent spec edit then contends instead of being overwritten. It never sets `spec:` or `trivial:`, so a change cannot flip between spec'd and trivial here. Repeatable while the change stays `proposed`. A typed refusal (`not-revisable`, `spec-not-linked`, `spec-file-missing`, `empty-revise`, `empty-spec_version`, version mismatch) writes nothing — surface it. The change stays build-ready. ### Step 5 — The transaction lands (no separate board pass) -The Step-4 typed op is the whole write: it re-checks the pinned exact `version`, commits the change record, the spec, the `## Artifacts` block, and the inline board in one metadata commit pushed to `origin/docket` under an exact-lease push — so there is no separate hand-staged commit and **no separate Board pass** (the readiness cell flips from needs-brainstorm, or the row leaves the Proposed section on a kill or defer, in that same commit). On a `contended` refusal the op writes nothing: re-sync (re-run the `repository.prepare` operation), re-read the record's `path` + `version` from the `status` operation, and — if it is no longer needs-brainstorm (someone else groomed, killed, or claimed it) — STOP and report rather than overwrite; otherwise re-author and retry. STOP — grooming never implements. +The Step-4 typed op is the whole write: it re-checks the pinned exact `version`, commits the change record, the spec, the `## Artifacts` block, and the inline board in one metadata commit pushed to `origin/docket` under an exact-lease push — so there is no separate hand-staged commit and **no separate Board pass** (the readiness cell flips from needs-brainstorm, or the row leaves the Proposed section on a kill or defer, in that same commit; a revise keeps the row build-ready). On a `contended` refusal the op writes nothing: re-sync (re-run the `repository.prepare` operation), re-read the record's `path` + `version` from the `status` operation, and — if it is no longer needs-brainstorm (someone else groomed, killed, or claimed it) — STOP and report rather than overwrite; otherwise re-author and retry. On a revise, also re-read the spec (a fresh `spec_version`) and stop only if the change is no longer an already-groomed `proposed` change. STOP — grooming never implements. ## Concurrency — no claim diff --git a/skills/docket-new-change/SKILL.md b/skills/docket-new-change/SKILL.md index 1797cf331..6bc88eb33 100644 --- a/skills/docket-new-change/SKILL.md +++ b/skills/docket-new-change/SKILL.md @@ -38,7 +38,7 @@ The default path for any non-trivial new change. Five steps: **Two draft-time scalars `create` does not carry** — `auto_groomable` and `branch_prefix`. When the human says the change may be designed without them, or names a branch prefix ("use the `hotfix/` prefix"), set the scalar directly on the created record's frontmatter and commit it with plain git plumbing (**Stage by explicit path** — that tree is shared, so never `add -A`) — no derived view depends on either, so no re-render: `auto_groomable: true` (so `docket-auto-groom` carries it to build-ready), and/or the normalized `branch_prefix: hotfix` (strip one presentation-only trailing slash, but **refuse and ask the human** on a slash-embedded or `refs/`-qualified value — claim consumes it at mint time, never prose). Leave a field unset to inherit the repo default. -5. **Groom to build-ready & land** — read the created record's `path` + exact **entity version** (blob object id) from the `status` operation (with `--json`), then apply the spec atomically: the `change.groom` operation with `--repo-dir .docket --request ` carrying the change id, the pinned `path` + `version`, `outcome: spec`, the `spec_markdown` from step 2, and any owned proposal-section rewrites. In one metadata commit the transaction writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, re-renders the `## Artifacts` block, and re-renders the inline board (pushed under an exact-lease push) — **no separate Board pass**. A version-mismatch / `contended` refusal writes nothing: re-sync (re-run the `repository.prepare` operation), re-read `path` + `version`, retry. STOP. Never implements. +5. **Groom to build-ready & land** — read the created record's `path` + exact **entity version** (blob object id) from the `status` operation (with `--json`), then apply the spec atomically: the `change.groom` operation with `--repo-dir .docket --request ` carrying the change id, the pinned `path` + `version`, `outcome: spec`, the `spec_markdown` from step 2, and any owned proposal-section rewrites. In one metadata commit the transaction writes the spec file, sets `spec:` + `updated:`, stamps the spec's `docket:backlink` block, re-renders the `## Artifacts` block, and re-renders the inline board (pushed under an exact-lease push) — **no separate Board pass**. A version-mismatch / `contended` refusal writes nothing: re-sync (re-run the `repository.prepare` operation), re-read `path` + `version`, retry. STOP. Never implements. To adjust a just-landed spec or its owned sections afterwards, run `docket-groom-next ` — its explicit-id path routes an already-groomed change to `change.groom` with `outcome: revise`; never re-run this skill or hand-edit the spec file. **Dummy mode:** when `DUMMY_MODE_ENABLED` is `true` (Step-0 export) — or the human asks for it in-session — write step 2's `dialogue` calibrated to `DUMMY_MODE_PERSONA`, per the convention's *Dummy mode* shared definition. The spec file itself is never simplified.