From f171763db23837f7f668189b1000c492a1af347d Mon Sep 17 00:00:00 2001 From: CL Kao Date: Wed, 26 Aug 2026 14:10:03 -0700 Subject: [PATCH] Name completion guard failures --- docs/site/reference/command-reference.md | 2 +- internal/cli/terminal_consume_test.go | 22 +++- internal/status/entered_stage.go | 130 ++++++++++++++++++++--- internal/status/entered_stage_test.go | 68 +++++++++--- internal/status/gate_extract.go | 24 +++++ internal/status/handlers.go | 11 +- 6 files changed, 216 insertions(+), 41 deletions(-) diff --git a/docs/site/reference/command-reference.md b/docs/site/reference/command-reference.md index 066ec6c53..dbdc4d268 100644 --- a/docs/site/reference/command-reference.md +++ b/docs/site/reference/command-reference.md @@ -97,7 +97,7 @@ The first officer runs these against workflow state as it moves entities; you op | `spacedock gate withdraw --reason TEXT` | Retire the selected current-stage open prepared attempt without a Resolution, provider evidence, application, room write, or status change. Attribution is always `agent:first-officer`; the next `gate prepare` appends a successor. | | `spacedock gate record --decision approve\|revise\|hold --actor ID [--reason TEXT] [--consume]` | Record a chat decision and, for approve only, its derived one-use application. Supported chat actor IDs are `person:captain` and `agent:first-officer`. Delegated First Officer decisions require an evidence reason; the recorder does not accept or authenticate Captain-message text. A standalone close never advances status or dispatches. The current workflow stage must be an actionable gate, and the bound Briefing must use the canonical v1 stage-qualified identity and name that stage; malformed or mismatched identity fails without mutation. `--consume` is the shortest approval path: it sequences close, sync, consume, and sync in one call (usage error with `--decision revise\|hold`), each write appending a `sync=... phase=record\|consume` line. Do not run a separate state commit after a successful split-root close or consume write. Delegated First Officer decisions also require `--conn-quote` (the grant verbatim) and `--conn-source` (where it was given); citation flags are refused with `--actor person:captain`. | | `spacedock gate record --round STAGE/CYCLE --briefing PATH/briefing.json --log PATH/briefing.review.jsonl` | For a folder-form entity (`/index.md`), publish one complete workflow-neutral correction round to the immutable derived room `review//round-` and update the current `review-round` pointer. The recorder retains canonical Briefing/log bytes and does not classify findings or write workflow body projections; `STAGE` must exist in the workflow taxonomy but may differ from current status for historical backfill. Flat entities are refused because review artifacts accumulate beside the entity. Exact replay is a no-op; divergence is refused. | -| `spacedock gate consume ` | Spend a binding pending approval once and advance status atomically; stale approvals become superseded. A consumed nonterminal application becomes ordinary stage history. After the worker report is durable, one atomic terminal status write can complete that stage without `--force`. On an approval whose target stage is terminal, consume spends nothing and writes no status: it leaves the application `pending` and returns the route `approved-awaiting-merge` (idempotently, on repeat), and `merge guard` discovers/arms the delivery mechanism when it acts. In a split-root workflow, a write (an advance or a stale-pending supersede) commits and syncs itself, appending a `sync=... phase=consume` line; a refusal or a terminal route performs no sync and emits no sync line. | +| `spacedock gate consume ` | Spend a binding pending approval once and advance status atomically; stale approvals become superseded. A consumed nonterminal application becomes ordinary stage history. After the worker report is durable, one atomic terminal status write can complete that stage without `--force`. On an approval whose target stage is terminal, consume spends nothing and writes no status: it leaves the application `pending` and returns the route `approved-awaiting-merge` (idempotently, on repeat), and `merge guard` discovers/arms the delivery mechanism when it acts. For an ungated current-stage-to-terminal transition, finalize directly with `spacedock status --workflow-dir DIR --set SLUG status=TERMINAL completed verdict=PASSED worktree=`; do not use that route for a pending terminal-target approval, whose sole consumer remains `merge guard`. In a split-root workflow, a write (an advance or a stale-pending supersede) commits and syncs itself, appending a `sync=... phase=consume` line; a refusal or a terminal route performs no sync and emits no sync line. | | `spacedock merge guard --verdict passed\|rejected` | Run the terminal merge ceremony and, with delivery proven, finalize: the sole terminal consumer of a pending terminal-target approval — the `mod-block` is cleared in its own step, then `application.state: consumed`, the terminal status, `verdict`, and `completed` move in one locked write, and the `pr` merge sentinel is retained through archive as durable delivery proof. | | `spacedock merge guard --rework` | Delivery requires rework: write the pending terminal-target approval `pending→superseded`, route the entity through the record stage's declared `feedback-to`, and clear `pr`/`mod-block`. Refuses without a pending terminal approval, or with a missing/undefined/terminal `feedback-to`. | | `spacedock new` | Create an entity (`new [--folder] SLUG`) from a body on stdin | diff --git a/internal/cli/terminal_consume_test.go b/internal/cli/terminal_consume_test.go index 5e97f6afe..ef1a9f7c7 100644 --- a/internal/cli/terminal_consume_test.go +++ b/internal/cli/terminal_consume_test.go @@ -202,8 +202,8 @@ func TestConsumedNonterminalApprovalAllowsOrdinaryTerminalFields(t *testing.T) { } code, out, errOut := terminalInvoke(t, root, "status", "--workflow-dir", root, "--set", "task", - "status=done", "verdict=PASSED", "completed") - if code != 0 { + "status=done", "completed", "verdict=PASSED", "worktree=") + if code != 0 || strings.Contains(out+errOut, "ineligible") { t.Fatalf("ordinary terminal fields exit=%d stdout=%q stderr=%q", code, out, errOut) } fields := entityFields(t, entity) @@ -215,6 +215,24 @@ func TestConsumedNonterminalApprovalAllowsOrdinaryTerminalFields(t *testing.T) { } } +// TestConsumedNonterminalApprovalAllowsMergeGuard proves the alternate ordinary +// terminal journey uses consumed gate history without inventing new authority. +func TestConsumedNonterminalApprovalAllowsMergeGuard(t *testing.T) { + root, entity := consumedNonterminalWorkflow(t) + code, out, errOut := terminalInvoke(t, root, "merge", "guard", "task", "--verdict", "passed", "--workflow-dir", root) + if code != 0 || strings.Contains(out+errOut, "ineligible") { + t.Fatalf("merge guard exit=%d stdout=%q stderr=%q", code, out, errOut) + } + archived := filepath.Join(root, "_archive", filepath.Base(entity)) + fields := entityFields(t, archived) + if fields["status"] != "done" || fields["verdict"] != "PASSED" || strings.TrimSpace(fields["completed"]) == "" { + t.Fatalf("archived terminal fields = status:%q verdict:%q completed:%q", fields["status"], fields["verdict"], fields["completed"]) + } + if got := gateApplicationStates(t, archived); !slices.Equal(got, []string{"consumed"}) { + t.Fatalf("merge guard rewrote consumed authority: %v", got) + } +} + // TestTerminalDeliveryFailureReworkRoundTrip is AC-1's value spine: approval // recorded -> consume routes without spending (pending, approved-awaiting-merge) // -> merge guard arms; delivery fails beyond retry -> --rework supersedes diff --git a/internal/status/entered_stage.go b/internal/status/entered_stage.go index 1f136ddc3..babd8546b 100644 --- a/internal/status/entered_stage.go +++ b/internal/status/entered_stage.go @@ -3,6 +3,7 @@ package status import ( + "fmt" "os" "os/exec" "path/filepath" @@ -13,10 +14,14 @@ import ( // be dispatched before ordinary successor projection resumes. Initial stages // and gate, terminal, and worktree suppression are handled by the caller. func enteredStageAwaitingCompletion(e *entity, stage Stage) bool { + return enteredStageCompletionFailure(e, stage) != nil +} + +func enteredStageCompletionFailure(e *entity, stage Stage) *completionFailure { if stage.initial || stage.gate || stage.terminal { - return false + return nil } - return !hasCompleteCommittedStageReport(e.path, stage.Name) + return completionFailureForPath(e.path, stage.Name) } // gatePreparable reports whether a gated stage's promotion proof is satisfied. @@ -39,29 +44,101 @@ func gatePreparable(path string, stage Stage) bool { // non-empty Summary. The current entity bytes must also be tracked and clean // against the local HEAD; sibling dirt is deliberately outside that pathspec. func hasCompleteCommittedStageReport(path, stage string) bool { + return completionFailureForPath(path, stage) == nil +} + +type completionFailureKind uint8 + +const ( + completionMissingReport completionFailureKind = iota + completionIncompleteReport + completionUntracked + completionDirty + completionInspectionFailed +) + +type completionFailure struct { + kind completionFailureKind + line int + item string + path string + gitRoot string + detail string +} + +func (f *completionFailure) diagnostic(stage string) string { + switch f.kind { + case completionMissingReport: + return fmt.Sprintf("missing current-stage report: no heading whose first stage token is %q; add %q", stage, "## Stage Report: "+stage) + case completionIncompleteReport: + return "incomplete current-stage report: " + f.detail + case completionUntracked: + return fmt.Sprintf("untracked completion artifact: %s is not tracked in local Git root %s; add and commit that path", f.path, f.gitRoot) + case completionDirty: + return fmt.Sprintf("dirty completion artifact: %s differs from local HEAD in Git root %s; commit that path (this guard does not require a remote push)", f.path, f.gitRoot) + default: + return "unable to inspect completion artifact: " + f.detail + } +} + +func completionFailureForPath(path, stage string) *completionFailure { data, err := os.ReadFile(path) if err != nil { - return false + return &completionFailure{kind: completionInspectionFailed, detail: fmt.Sprintf("cannot read %s: %v", path, err)} + } + if failure := stageReportFailure(data, stage); failure != nil { + return failure } - return hasCompleteStageReport(data, stage) && entityPathCleanInHEAD(path) + return entityGitFailure(path) } func hasCompleteStageReport(data []byte, stage string) bool { + return stageReportFailure(data, stage) == nil +} + +func stageReportFailure(data []byte, stage string) *completionFailure { lines := splitLines(string(data)) start, end, ok := selectStageReport(lines, stage) if !ok { - return false + return &completionFailure{kind: completionMissingReport} } items := extractChecklist(lines, start, end) - if len(items) == 0 { - return false - } + var first *completionFailure for _, item := range items { - if (item.status != "DONE" && item.status != "SKIPPED") || strings.TrimSpace(item.text) == "" || !checklistItemHasEvidence(lines, item) { - return false + detail := "" + switch { + case item.status == "FAILED": + detail = fmt.Sprintf("line %d FAILED item %q is unresolved", item.start, item.text) + case strings.TrimSpace(item.text) == "": + detail = fmt.Sprintf("line %d %s item has blank text", item.start, item.status) + case !checklistItemHasEvidence(lines, item): + detail = fmt.Sprintf("line %d %s item %q has no evidence or rationale line", item.start, item.status, item.text) } + if detail != "" { + first = &completionFailure{kind: completionIncompleteReport, line: item.start, item: item.text, detail: detail} + break + } + } + if nearMiss := firstChecklistNearMiss(lines, start, end); nearMiss != nil && (first == nil || nearMiss.line < first.line) { + first = &completionFailure{ + kind: completionIncompleteReport, line: nearMiss.line, item: nearMiss.text, + detail: fmt.Sprintf("line %d %q is not canonical; use %q", nearMiss.line, nearMiss.text, "- "+nearMiss.status+": "), + } + } + if first != nil { + return first + } + if len(items) == 0 { + return &completionFailure{kind: completionIncompleteReport, detail: "no recognized checklist items; add a canonical - DONE: or - SKIPPED: item"} + } + summaryLine, found, complete := stageReportSummary(lines, start, end) + if !found { + return &completionFailure{kind: completionIncompleteReport, detail: `missing non-empty "### Summary"`} + } + if !complete { + return &completionFailure{kind: completionIncompleteReport, line: summaryLine, detail: fmt.Sprintf("line %d %q has no content", summaryLine, "### Summary")} } - return stageReportHasSummary(lines, start, end) + return nil } func checklistItemHasEvidence(lines []string, item checklistItem) bool { @@ -74,6 +151,11 @@ func checklistItemHasEvidence(lines []string, item checklistItem) bool { } func stageReportHasSummary(lines []string, start, end int) bool { + _, _, complete := stageReportSummary(lines, start, end) + return complete +} + +func stageReportSummary(lines []string, start, end int) (summaryLine int, found, complete bool) { for line := start; line <= end; line++ { if strings.TrimSpace(lines[line-1]) != "### Summary" { continue @@ -84,31 +166,45 @@ func stageReportHasSummary(lines []string, start, end int) bool { break } if strings.TrimSpace(lines[bodyLine-1]) != "" { - return true + return line, true, true } } - return false + return line, true, false } - return false + return 0, false, false } // entityPathCleanInHEAD is deliberately literal and path-scoped. A tracked // entity whose working-tree/index bytes differ from HEAD is not durable proof; // an unrelated dirty sibling does not affect the answer. func entityPathCleanInHEAD(path string) bool { + return entityGitFailure(path) == nil +} + +func entityGitFailure(path string) *completionFailure { gitRoot, rel, ok := entityGitPath(path) if !ok { - return false + return &completionFailure{kind: completionInspectionFailed, path: path, detail: fmt.Sprintf("%s is not inside a local Git worktree", path)} } rel = filepath.ToSlash(rel) tracked := exec.Command("git", "--literal-pathspecs", "ls-files", "--error-unmatch", "--", rel) tracked.Dir = gitRoot if err := tracked.Run(); err != nil { - return false + if exit, ok := err.(*exec.ExitError); ok && exit.ExitCode() == 1 { + return &completionFailure{kind: completionUntracked, path: path, gitRoot: gitRoot} + } + return &completionFailure{kind: completionInspectionFailed, path: path, gitRoot: gitRoot, detail: fmt.Sprintf("git ls-files failed in %s: %v", gitRoot, err)} } clean := exec.Command("git", "--literal-pathspecs", "diff", "--quiet", "HEAD", "--", rel) clean.Dir = gitRoot - return clean.Run() == nil + err := clean.Run() + if err == nil { + return nil + } + if exit, ok := err.(*exec.ExitError); ok && exit.ExitCode() == 1 { + return &completionFailure{kind: completionDirty, path: path, gitRoot: gitRoot} + } + return &completionFailure{kind: completionInspectionFailed, path: path, gitRoot: gitRoot, detail: fmt.Sprintf("git diff failed in %s: %v", gitRoot, err)} } func entityGitPath(path string) (gitRoot, rel string, ok bool) { diff --git a/internal/status/entered_stage_test.go b/internal/status/entered_stage_test.go index 545835219..d2b47791a 100644 --- a/internal/status/entered_stage_test.go +++ b/internal/status/entered_stage_test.go @@ -4,6 +4,7 @@ package status import ( "encoding/json" + "fmt" "os" "path/filepath" "strings" @@ -81,6 +82,7 @@ func TestEnteredStageProjectionRequiresCommittedCompleteReport(t *testing.T) { {"missing summary", "\n## Stage Report: implementation\n\n- DONE: Produce the implementation.\n Commit abc123 contains evidence.\n"}, {"empty summary", "\n## Stage Report: implementation\n\n- DONE: Produce the implementation.\n Commit abc123 contains evidence.\n\n### Summary\n\n"}, {"wrong stage", strings.ReplaceAll(completeImplementationReport, "Stage Report: implementation", "Stage Report: handoff")}, + {"stage-token near miss", strings.ReplaceAll(completeImplementationReport, "Stage Report: implementation", "Stage Report: implementation-notes")}, {"later malformed masks older valid", completeImplementationReport + "\n## Stage Report: implementation (cycle 2)\n\n- DONE: Later report has no evidence.\n\n### Summary\n\nLater but malformed.\n"}, } for _, tc := range cases { @@ -135,30 +137,58 @@ func TestEnteredStageProjectionRequiresCommittedCompleteReport(t *testing.T) { Current: "implementation", Next: "validation", Worktree: "no", }) }) + t.Run("spaced heading suffix remains accepted", func(t *testing.T) { + report := strings.Replace(completeImplementationReport, "Stage Report: implementation", "Stage Report: implementation (cycle 2)", 1) + def, _, _ := buildEnteredStageFixture(t, enteredStageEntity+report) + assertEnteredStageRows(t, def, enteredStageRow{ + ID: "entered-task", Slug: "entered-task", + Current: "implementation", Next: "validation", Worktree: "no", + }) + }) } -func TestEnteredWorktreeStageAwayStatusMutationsAreByteClean(t *testing.T) { +func TestEnteredWorktreeStageFailureDiagnosticsAreDistinctAndByteClean(t *testing.T) { + annotated := "\n## Stage Report: implementation\n\n- DONE: Produce the implementation.\n Commit abc123 contains evidence.\n- DONE (annotation): Document the edge case.\n The annotation must not hide this obligation.\n\n### Summary\n\nThe implementation is complete.\n" cases := []struct { - name string - args []string + name string + report string + setup func(t *testing.T, state, entity string) + want func(body, state, entity string) string }{ - {"successor", []string{"status=validation"}}, - {"backward", []string{"status=backlog"}}, - {"terminal", []string{"status=done"}}, - {"force successor", []string{"status=validation", "--force"}}, - {"force terminal", []string{"status=done", "--force"}}, - {"same then away", []string{"status=implementation", "status=validation"}}, - {"away then same", []string{"status=validation", "status=implementation"}}, + {"missing report", "", nil, func(_, _, _ string) string { + return `missing current-stage report: no heading whose first stage token is "implementation"; add "## Stage Report: implementation"` + }}, + {"incomplete checklist", annotated, nil, func(body, _, _ string) string { + line := strings.Count(body[:strings.Index(body, "- DONE (annotation):")], "\n") + 1 + return fmt.Sprintf(`incomplete current-stage report: line %d "- DONE (annotation): Document the edge case." is not canonical; use "- DONE: "`, line) + }}, + {"untracked entity", completeImplementationReport, func(t *testing.T, state, _ string) { + gitC(t, state, "rm", "-q", "--cached", "--", "entered-task.md") + }, func(_, state, entity string) string { + return fmt.Sprintf("untracked completion artifact: %s is not tracked in local Git root %s; add and commit that path", entity, state) + }}, + {"dirty entity", completeImplementationReport, func(t *testing.T, _, entity string) { + writeFile(t, entity, readBytes(t, entity)+"\nUncommitted entity dirt.\n") + }, func(_, state, entity string) string { + return fmt.Sprintf("dirty completion artifact: %s differs from local HEAD in Git root %s; commit that path (this guard does not require a remote push)", entity, state) + }}, } + gotMessages := map[string]bool{} for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - body := strings.Replace(enteredStageEntity, "worktree:", "worktree: .worktrees/entered-task", 1) - def, _, entity := buildEnteredStageFixture(t, body) + body := strings.Replace(enteredStageEntity+tc.report, "worktree:", "worktree: .worktrees/entered-task", 1) + def, state, entity := buildEnteredStageFixture(t, body) + if tc.setup != nil { + tc.setup(t, state, entity) + } before, err := os.ReadFile(entity) if err != nil { t.Fatal(err) } - args := append([]string{"--workflow-dir", def, "--set", "entered-task"}, tc.args...) + args := []string{"--workflow-dir", def, "--set", "entered-task", "status=validation"} + if tc.name == "missing report" { + args = append(args, "--force") + } stdout, stderr, code := runNative(t, def, pinnedEnv(t), args...) if code != 1 { t.Fatalf("away mutation exit=%d, want 1; stdout=%q stderr=%q", code, stdout, stderr) @@ -166,11 +196,12 @@ func TestEnteredWorktreeStageAwayStatusMutationsAreByteClean(t *testing.T) { if stdout != "" { t.Fatalf("away mutation emitted success stdout: %q", stdout) } - for _, want := range []string{"implementation", "Stage Report"} { - if !strings.Contains(stderr, want) { - t.Fatalf("stderr=%q, want actionable %q", stderr, want) - } + want := fmt.Sprintf("Error: entity entered-task cannot change status away from entered stage %q: %s.\n", + "implementation", tc.want(body, state, entity)) + if stderr != want { + t.Fatalf("stderr=%q, want %q", stderr, want) } + gotMessages[stderr] = true after, err := os.ReadFile(entity) if err != nil { t.Fatal(err) @@ -180,6 +211,9 @@ func TestEnteredWorktreeStageAwayStatusMutationsAreByteClean(t *testing.T) { } }) } + if len(gotMessages) != 4 { + t.Fatalf("distinct completion diagnostics=%d, want 4", len(gotMessages)) + } } func TestEnteredStageMutationControls(t *testing.T) { diff --git a/internal/status/gate_extract.go b/internal/status/gate_extract.go index c452cc211..2c3b121e7 100644 --- a/internal/status/gate_extract.go +++ b/internal/status/gate_extract.go @@ -59,6 +59,10 @@ var stageReportHeadingRe = regexp.MustCompile(`^##\s+Stage Report:\s+(\S+)`) // the stage-report protocol's fixed three. var checklistBulletRe = regexp.MustCompile(`^-\s+(DONE|SKIPPED|FAILED):\s*(.*)$`) +// checklistNearMissRe catches status-like bullets that the canonical parser +// intentionally ignores, such as "- DONE (annotation): ...". +var checklistNearMissRe = regexp.MustCompile(`^-\s+(DONE|SKIPPED|FAILED)\b`) + // acHeadingRe matches an `**AC-N**` acceptance-criteria heading and captures the // AC id. The id is tokenized on the heading boundary (AC- followed by an // alphanumeric run), never split on `-` (the spike's AC-id boundary finding). An @@ -83,6 +87,26 @@ type checklistItem struct { end int // 1-based, inclusive } +type checklistNearMiss struct { + status string + line int + text string +} + +func firstChecklistNearMiss(lines []string, start, end int) *checklistNearMiss { + for line := start; line <= end; line++ { + text := lines[line-1] + if strings.HasPrefix(text, "### ") { + break + } + match := checklistNearMissRe.FindStringSubmatch(text) + if match != nil && !checklistBulletRe.MatchString(text) { + return &checklistNearMiss{status: match[1], line: line, text: strings.TrimSpace(text)} + } + } + return nil +} + // selectStageReport returns the line range [start,end] (1-based, inclusive) of // the LATEST stage-report section whose leading stage-token equals stage, across // interleaved sections. Within a stage's multiple cycles the append-only ordering diff --git a/internal/status/handlers.go b/internal/status/handlers.go index 9137ac368..3da8e0478 100644 --- a/internal/status/handlers.go +++ b/internal/status/handlers.go @@ -115,15 +115,18 @@ func runSet(roots roots, set *setUpdate, args []string, whereFilters []whereFilt // non-status updates remain allowed. if strings.TrimSpace(currentFields["worktree"]) != "" { for _, stage := range stages { - if stage.Name != currentStatus || !enteredStageAwaitingCompletion(&entity{path: entityPath}, stage) { + if stage.Name != currentStatus { + continue + } + failure := enteredStageCompletionFailure(&entity{path: entityPath}, stage) + if failure == nil { continue } for _, u := range set.updates { if u.field == "status" && u.hasValue && u.value != currentStatus { return errExit(stderr, fmt.Sprintf( - "entity %s cannot change status away from entered stage %q until a durable, complete "+ - "## Stage Report: %s is committed.", - slug, currentStatus, currentStatus)) + "entity %s cannot change status away from entered stage %q: %s.", + slug, currentStatus, failure.diagnostic(currentStatus))) } } break