diff --git a/.claude/skills/metta-propose/SKILL.md b/.claude/skills/metta-propose/SKILL.md index da50d4fe..c33d928a 100644 --- a/.claude/skills/metta-propose/SKILL.md +++ b/.claude/skills/metta-propose/SKILL.md @@ -22,7 +22,7 @@ Before parsing flags or creating any change state, YOU (the orchestrator) MUST c Routing decision: -- **Description matches the criteria AND the caller did NOT pass an explicit `--workflow` flag:** do NOT proceed to Step 1 or the standard proposal pipeline. Run `metta quick` instead — follow the metta-quick skill flow for the same description, then stop; none of the numbered steps below run. +- **Description matches the criteria AND the caller did NOT pass an explicit `--workflow` flag:** do NOT proceed to Step 1 or the standard proposal pipeline. Run `metta quick` instead — follow the metta-quick skill flow for the same description, then stop; none of the numbered steps below run. When rerouting, the PR-open default carries over: the quick flow's merge steps MUST be skipped and the run MUST stop at the open PR (reporting the PR URL) unless `--ship` was present in the original propose invocation. - **Caller passed an explicit `--workflow` flag (any value):** defer to that choice without overriding it — skip this routing decision and proceed to Step 1, passing the flag through as written. - **Description does not match the criteria and no flag was passed:** proceed to Step 1 normally. @@ -48,7 +48,14 @@ Routing decision: - Otherwise, `STOP_AFTER = ""` (empty string). - The remaining text is the description. - Valid artifact ids are owned by the CLI and the resolved workflow's `buildOrder`; do NOT validate the value here — pass through and let `metta propose` reject unknown ids and execution-phase ids (`implementation`, `verification`) with a clear error before any change state is written. - - **Scope of `STOP_AFTER`:** when non-empty, this names a planning-phase artifact (e.g. `intent`, `stories`, `spec`, `research`, `design`, `tasks` for the standard workflow). The orchestrator MUST honor this boundary in Step 3 — see "Stop-after boundary check" there. + - **Scope of `STOP_AFTER`:** when non-empty, this names a planning-phase artifact (e.g. `intent`, `stories`, `spec`, `research`, `design`, `tasks` for the standard workflow). The orchestrator MUST honor this boundary in Step 3 — see "Stop-after boundary check" there. The special value `ship` is NOT a planning-phase artifact: it means "run to merge" and is handled by the Step 8 ship opt-in, never by the Step 3 boundary check. + + **Parse optional `--ship` from `$ARGUMENTS`:** + + - If `$ARGUMENTS` contains the token `--ship`, remove it from `$ARGUMENTS` and set `STOP_AFTER = "ship"`. `--ship` is an alias for `--stop-after ship` — forward it to the CLI as `--stop-after ship` (there is no CLI `--ship` flag). If both `--ship` and `--stop-after ` are present, `--ship` takes precedence. + - Treat `--ship` as the ship opt-in ONLY when it appears as a standalone flag token in leading or trailing position — NOT when it appears inside quotes or as the subject/topic of the description text (e.g. a description *about* a ship flag or shipping behavior). + - When the ship opt-in IS detected, the orchestrator MUST announce before proceeding: `Ship opt-in detected: this run will merge to main after CI passes.` — so a misparse is visible at Step 1, not at merge time. + - The remaining text is the description. Then run: `metta propose "" --workflow --stop-after --json` (when both flags present) @@ -104,9 +111,10 @@ Routing decision: - The boundary is reached when EITHER of these is true: 1. `STOP_AFTER` (set in Step 1) is non-empty AND equals the artifact id just passed to `metta complete`. 2. The change record's persisted `stop_after` field (read via `metta status --json --change `) is non-empty AND equals that artifact id. This second check provides robustness if `STOP_AFTER` was lost from local state for any reason; both checks should agree. + - `ship` is not a planning boundary: when `STOP_AFTER = "ship"` (or persisted `stop_after: ship`), this check never fires for any artifact — do not hunt for a `ship` artifact; continue the loop to `all_complete` and apply the Step 8 ship opt-in. - When the boundary is reached, the orchestrator MUST: a. NOT spawn any further planning subagent for the next artifact. - b. NOT proceed to Step 4 (research synthesis), Step 5 (implementation), Step 6 (review), Step 7 (verification), or Step 8 (finalize/merge). All subsequent steps are skipped in their entirety. + b. NOT proceed to Step 4 (research synthesis), Step 5 (implementation), Step 6 (review), Step 7 (verification), or Step 8 (finalize/PR). All subsequent steps are skipped in their entirety. c. NOT spawn any `metta-executor`, `metta-reviewer`, or `metta-verifier` agent. NOT call `metta finalize` or `git merge`. d. Print exactly one handoff line, formatted EXACTLY as: ``Stopped after ``. Run `` to .`` @@ -273,15 +281,23 @@ Routing decision: a. `metta finalize --json --change ` → runs gates, archives, merges specs b. `git -C "{change_root}" push -u origin metta/` → push the feature branch to the remote c. `gh pr create --title "" --body ""` → open a PR. The body MUST end with `🤖 Generated with [Claude Code](https://claude.com/claude-code)` - d. `gh pr checks --watch --fail-fast` → wait for all CI checks on the PR to complete before merging. If any check fails or is cancelled, do NOT merge — report the failing check(s) and the PR URL to the user and stop. If gh reports that no checks are reported yet (checks can lag PR creation by a few seconds), wait ~10s and retry the command - e. `gh pr merge --merge` → land the PR immediately, unless the user asked to leave it open for review — in that case stop here and report the PR URL instead of merging - f. Back on `main`: `git pull --ff-only`, then clean up the change branch and worktree + d. **Default path ends at an open PR. Do NOT merge; report the PR URL and stop.** + When `STOP_AFTER` (or the change record's persisted `stop_after`) is anything other than `ship`, report exactly: + ``PR open for review: . Run `/metta-ship` to land it, or merge the PR on GitHub yourself.`` + then proceed to Step 9 and return control to the user. On this default path you MUST NOT watch CI checks as a precursor to merging, MUST NOT merge the PR, and MUST NOT perform post-merge cleanup (main pull, branch/worktree removal). + + **Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record's persisted `stop_after` is `ship`):** + + e. `gh pr checks --watch --fail-fast` → wait for all CI checks on the PR to complete before merging. If any check fails or is cancelled, do NOT merge — report the failing check(s) and the PR URL to the user and stop. If gh reports that no checks are reported yet (checks can lag PR creation by a few seconds), wait ~10s and retry the command + f. `gh pr merge --merge` → land the PR + g. Back on `main`: `git pull --ff-only`, then clean up the change branch and worktree 9. Report to user what was done -## Critical: You MUST verify, finalize, and ship +## Critical: verify, finalize, and open the PR - Do NOT skip verification — a metta-verifier agent MUST run gates and confirm spec compliance -- Do NOT stop after the last artifact — finalize + ship must happen +- Do NOT stop before the PR exists — when no planning-phase `stop_after` boundary fired in Step 3, finalize, push, and `gh pr create` are mandatory on every completed run +- Merging is NOT part of the default path. Watching CI checks, merging the PR, and post-merge cleanup happen only under the Step 8 ship opt-in (`stop_after = ship`); otherwise stop at the open PR and hand off to `/metta-ship` - If metta finalize fails gates, spawn a metta-executor to fix, then retry - Direct local merge of the change branch into main (`git merge`) is forbidden — every change ships through a pushed branch and a GitHub PR - If a dispatched step appears orphaned, follow the residual orphaning recovery protocol in metta-skill-host.md. diff --git a/CLAUDE.md b/CLAUDE.md index bc6481df..5e20ff9d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,7 +71,7 @@ Only escalate to the user for **subjective judgments** — scope boundaries, cos Cite the source URL when presenting findings so the user can verify the answer. ### Lifecycle skills -- `/metta-propose ` — start a new change (standard workflow) +- `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship` - `/metta-quick ` — quick mode, skip planning - `/metta-auto ` — full lifecycle loop (discover → build → verify → ship) - `/metta-plan` — build planning artifacts for the active change diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/.metta.yaml b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/.metta.yaml new file mode 100644 index 00000000..c4d8c4c3 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/.metta.yaml @@ -0,0 +1,365 @@ +workflow: standard +created: 2026-08-21T22:57:08.364Z +status: active +current_artifact: verification +base_versions: {} +artifacts: + intent: complete + stories: complete + spec: complete + research: complete + design: complete + tasks: complete + implementation: complete + verification: complete +complexity_score: + score: 2 + signals: + file_count: 5 + recommended_workflow: standard +artifact_timings: + intent: + started: 2026-08-21T22:57:12.920Z + completed: 2026-08-21T22:58:22.098Z + stories: + started: 2026-08-21T22:58:25.382Z + completed: 2026-08-21T22:59:13.404Z + spec: + started: 2026-08-21T22:59:17.297Z + completed: 2026-08-21T23:01:51.249Z + research: + started: 2026-08-21T23:01:54.990Z + completed: 2026-08-21T23:05:32.653Z + design: + started: 2026-08-21T23:05:35.888Z + completed: 2026-08-21T23:08:40.610Z + tasks: + started: 2026-08-21T23:08:45.270Z + completed: 2026-08-21T23:10:16.306Z + implementation: + completed: 2026-08-21T23:27:44.577Z + verification: + completed: 2026-08-21T23:46:58.608Z +artifact_tokens: + intent: + context: 763 + budget: 20000 + stories: + context: 2161 + budget: 20000 + spec: + context: 3273 + budget: 40000 + research: + context: 4507 + budget: 60000 + design: + context: 5843 + budget: 80000 + tasks: + context: 7941 + budget: 40000 +review_iterations: 2 +verify_iterations: 1 +worktree: /home/utx0/Code/metta/.metta/worktrees/fix-metta-propose-runs-entire-lifecycle-through-finalize +token_usage: + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 3723 + timestamp: 2026-08-21T22:57:36.735Z + source: hook + - task: intent + agent: metta-proposer + model: fable + tokens: 3093 + timestamp: 2026-08-21T22:58:10.833Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 5331 + timestamp: 2026-08-21T22:58:38.560Z + source: hook + - task: stories + agent: metta-product + model: fable + tokens: 2242 + timestamp: 2026-08-21T22:59:06.331Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 7558 + timestamp: 2026-08-21T22:59:40.987Z + source: hook + - task: spec + agent: metta-specifier + model: fable + tokens: 12001 + timestamp: 2026-08-21T23:01:44.966Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 11331 + timestamp: 2026-08-21T23:02:35.102Z + source: hook + - task: research + agent: metta-researcher + model: fable + tokens: 8898 + timestamp: 2026-08-21T23:04:16.060Z + source: hook + - task: research + agent: metta-researcher + model: fable + tokens: 9272 + timestamp: 2026-08-21T23:04:19.559Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 11423 + timestamp: 2026-08-21T23:04:20.544Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 11454 + timestamp: 2026-08-21T23:04:23.675Z + source: hook + - task: research + agent: metta-researcher + model: fable + tokens: 11019 + timestamp: 2026-08-21T23:04:53.600Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 16422 + timestamp: 2026-08-21T23:05:55.799Z + source: hook + - task: design + agent: metta-architect + model: fable + tokens: 22617 + timestamp: 2026-08-21T23:08:34.102Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 18112 + timestamp: 2026-08-21T23:09:01.195Z + source: hook + - task: tasks + agent: metta-planner + model: fable + tokens: 6847 + timestamp: 2026-08-21T23:10:09.945Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 23218 + timestamp: 2026-08-21T23:11:04.248Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 1753 + timestamp: 2026-08-21T23:11:32.816Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 23259 + timestamp: 2026-08-21T23:11:36.825Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 1792 + timestamp: 2026-08-21T23:12:10.377Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 23289 + timestamp: 2026-08-21T23:12:13.518Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 7380 + timestamp: 2026-08-21T23:12:17.114Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 26073 + timestamp: 2026-08-21T23:12:45.229Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 3062 + timestamp: 2026-08-21T23:13:21.563Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 26107 + timestamp: 2026-08-21T23:13:24.937Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 4435 + timestamp: 2026-08-21T23:14:50.013Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 27294 + timestamp: 2026-08-21T23:15:04.370Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 1743 + timestamp: 2026-08-21T23:25:33.884Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 31347 + timestamp: 2026-08-21T23:26:23.313Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 6017 + timestamp: 2026-08-21T23:27:06.735Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 39939 + timestamp: 2026-08-21T23:28:52.240Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 4933 + timestamp: 2026-08-21T23:31:52.316Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 40111 + timestamp: 2026-08-21T23:31:58.252Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 10434 + timestamp: 2026-08-21T23:32:02.782Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 9635 + timestamp: 2026-08-21T23:32:05.698Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 40137 + timestamp: 2026-08-21T23:32:06.068Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 44766 + timestamp: 2026-08-21T23:33:10.433Z + source: hook + - task: implementation + agent: metta-executor + model: fable + tokens: 5296 + timestamp: 2026-08-21T23:34:22.870Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 47271 + timestamp: 2026-08-21T23:34:56.242Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 5595 + timestamp: 2026-08-21T23:36:00.817Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 47298 + timestamp: 2026-08-21T23:36:07.188Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 7043 + timestamp: 2026-08-21T23:36:36.565Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 47473 + timestamp: 2026-08-21T23:36:41.960Z + source: hook + - task: implementation + agent: metta-reviewer + model: fable + tokens: 8723 + timestamp: 2026-08-21T23:36:46.636Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 50952 + timestamp: 2026-08-21T23:37:33.299Z + source: hook + - task: verification + agent: metta-verifier + model: fable + tokens: 792 + timestamp: 2026-08-21T23:38:00.242Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 50988 + timestamp: 2026-08-21T23:38:03.747Z + source: hook + - task: verification + agent: metta-verifier + model: fable + tokens: 11384 + timestamp: 2026-08-21T23:41:37.079Z + source: hook + - task: metta-skill-host + agent: metta-skill-host + model: fable + tokens: 51035 + timestamp: 2026-08-21T23:41:41.901Z + source: hook + - task: verification + agent: metta-verifier + model: fable + tokens: 571 + timestamp: 2026-08-21T23:46:34.442Z + source: hook diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/TOKENS.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/TOKENS.md new file mode 100644 index 00000000..a3a732cc --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/TOKENS.md @@ -0,0 +1,99 @@ +# Token usage: fix-metta-propose-runs-entire-lifecycle-through-finalize + +- **Change**: fix-metta-propose-runs-entire-lifecycle-through-finalize +- **Generated**: 2026-08-21 + +> Provenance per row: `hook (exact)` rows are harness-measured token counts +> recorded automatically by the token-recording hook; `prose (estimate)` rows +> are orchestrator-estimated figures and may under- or over-count actual +> provider usage. When both exist for the same task and agent, the exact hook +> figure is used. + +## Total + +**~892,488 tokens** across 50 record(s). + +## Per artifact + +| Artifact/task | Agent | Model | Tokens | Provenance | +|---|---|---|---|---| +| metta-skill-host | metta-skill-host | fable | 3,723 | hook (exact) | +| intent | metta-proposer | fable | 3,093 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 5,331 | hook (exact) | +| stories | metta-product | fable | 2,242 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 7,558 | hook (exact) | +| spec | metta-specifier | fable | 12,001 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 11,331 | hook (exact) | +| research | metta-researcher | fable | 8,898 | hook (exact) | +| research | metta-researcher | fable | 9,272 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 11,423 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 11,454 | hook (exact) | +| research | metta-researcher | fable | 11,019 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 16,422 | hook (exact) | +| design | metta-architect | fable | 22,617 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 18,112 | hook (exact) | +| tasks | metta-planner | fable | 6,847 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 23,218 | hook (exact) | +| implementation | metta-executor | fable | 1,753 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 23,259 | hook (exact) | +| implementation | metta-executor | fable | 1,792 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 23,289 | hook (exact) | +| implementation | metta-executor | fable | 7,380 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 26,073 | hook (exact) | +| implementation | metta-executor | fable | 3,062 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 26,107 | hook (exact) | +| implementation | metta-executor | fable | 4,435 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 27,294 | hook (exact) | +| implementation | metta-executor | fable | 1,743 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 31,347 | hook (exact) | +| implementation | metta-executor | fable | 6,017 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 39,939 | hook (exact) | +| implementation | metta-reviewer | fable | 4,933 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 40,111 | hook (exact) | +| implementation | metta-reviewer | fable | 10,434 | hook (exact) | +| implementation | metta-reviewer | fable | 9,635 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 40,137 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 44,766 | hook (exact) | +| implementation | metta-executor | fable | 5,296 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 47,271 | hook (exact) | +| implementation | metta-reviewer | fable | 5,595 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 47,298 | hook (exact) | +| implementation | metta-reviewer | fable | 7,043 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 47,473 | hook (exact) | +| implementation | metta-reviewer | fable | 8,723 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 50,952 | hook (exact) | +| verification | metta-verifier | fable | 792 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 50,988 | hook (exact) | +| verification | metta-verifier | fable | 11,384 | hook (exact) | +| metta-skill-host | metta-skill-host | fable | 51,035 | hook (exact) | +| verification | metta-verifier | fable | 571 | hook (exact) | + +## Per role + +| Agent | Tokens | +|---|---| +| metta-architect | 22,617 | +| metta-executor | 31,478 | +| metta-planner | 6,847 | +| metta-product | 2,242 | +| metta-proposer | 3,093 | +| metta-researcher | 29,189 | +| metta-reviewer | 46,363 | +| metta-skill-host | 725,911 | +| metta-specifier | 12,001 | +| metta-verifier | 12,747 | + +## Per model + +| Model | Tokens | +|---|---| +| fable | 892,488 | + +## Cheap/pinned (non-inherit) vs inherit + +- **Cheap/pinned (non-inherit)**: ~892,488 tokens +- **Inherit**: ~0 tokens + +## Gaps + +No gaps found. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/UAT.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/UAT.md new file mode 100644 index 00000000..9e29160e --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/UAT.md @@ -0,0 +1,226 @@ +# UAT: fix-metta-propose-runs-entire-lifecycle-through-finalize + +- **Change**: fix-metta-propose-runs-entire-lifecycle-through-finalize +- **Generated**: 2026-08-21 +- **Source**: user stories (stories.md) + +## Reporting failures + +If any step below fails or behaves unexpectedly, log a metta issue +(`/metta-issue `) referencing this file and the step number. +The sanctioned UAT runner (`/metta-uat`) may flip a step's Pass checkbox +to reflect a genuinely observed outcome and may append dated `## UAT run` +records below the steps. Never fabricate a pass: do not alter step content, +and never check a box for behavior that was not actually observed. + +## Acceptance steps + +### US-1: Propose stops at an open PR by default + +*Independent test:* A default `/metta-propose` run (no stop-after flag) ends with `gh pr create` and a reported PR URL, and no `gh pr merge` is executed. + +#### Step 1.1 +- **Setup**: `/metta-propose ` is invoked with no stop-after flag +- **Do**: the change completes verification and finalize (Run: `gh pr merge`) +- **Observe**: the skill pushes the branch, creates the PR, reports the PR URL, and stops without running `gh pr merge` +- [ ] Pass + +#### Step 1.2 +- **Setup**: a completed default propose run +- **Do**: the user inspects the repository +- **Observe**: the change branch's PR is open and main does not contain the merge +- [ ] Pass + +#### Step 1.3 +- **Setup**: a default propose run has stopped at PR-open +- **Do**: the user wants to land it +- **Observe**: `/metta-ship` (or an explicit merge) completes the change without rework +- [ ] Pass + +### US-2: Explicit ship opt-in restores run-to-merge + +*Independent test:* A propose run with the ship opt-in proceeds past PR creation to CI watch and merge, using the existing propose-stop-after machinery. + +#### Step 2.1 +- **Setup**: `/metta-propose` is invoked with the ship opt-in +- **Do**: the PR is created (Run: `gh pr checks --watch`, `gh pr merge`) +- **Observe**: the skill continues through `gh pr checks --watch` and `gh pr merge` as before +- [ ] Pass + +#### Step 2.2 +- **Setup**: the ship opt-in is recorded via the propose-stop-after machinery +- **Do**: `.metta.yaml` is inspected +- **Observe**: the recorded stop-after value validates against the existing schema and drives the boundary check +- [ ] Pass + +### US-3: Existing stop-after values keep their semantics + +*Independent test:* Each previously accepted stop-after value still validates and stops the propose pipeline at the same boundary as before the change. + +#### Step 3.1 +- **Setup**: `/metta-propose` is invoked with an existing stop-after value such as `tasks` +- **Do**: that artifact completes +- **Observe**: the pipeline stops at the same boundary it did before this change +- [ ] Pass + +#### Step 3.2 +- **Setup**: the `propose-stop-after` spec delta is applied +- **Do**: the value set is reviewed +- **Observe**: no existing value is removed or renamed, and only the absent-flag default semantics change +- [ ] Pass + +#### Step 3.3 +- **Setup**: `/metta-auto` or `/metta-fix-issues` is invoked +- **Do**: their lifecycles complete +- **Observe**: they still run to merge exactly as before +- [ ] Pass + +### US-4: Instructions and docs cannot silently restore auto-merge + +*Independent test:* Grep-assert tests over both SKILL.md copies fail if an unconditional merge instruction is present, and pass on the updated files. + +#### Step 4.1 +- **Setup**: the installed skill (`.claude/skills/metta-propose/SKILL.md`) and the template (`src/templates/skills/metta-propose/SKILL.md`) +- **Do**: their default-path instructions are read (Run: `gh pr create`) +- **Observe**: the terminal action is `gh pr create` + report, with merge conditional on the explicit ship opt-in and no "must ship" mandate on the default path +- [ ] Pass + +#### Step 4.2 +- **Setup**: the grep-assert regression tests +- **Do**: an unconditional `gh pr merge` instruction is added back to either SKILL.md copy (Run: `gh pr merge`) +- **Observe**: the test suite fails +- [ ] Pass + +#### Step 4.3 +- **Setup**: the CLAUDE.md workflow section +- **Do**: a reader checks `/metta-propose`'s described behavior +- **Observe**: it states the run ends at an open PR unless ship is explicitly requested +- [ ] Pass + +## Additional scenarios + +#### Step 5.1: option appears in CLI help +- **Setup**: the metta CLI is built +- **Do**: the user runs `metta propose --help` (Run: `metta propose --help`) +- **Observe**: the help output MUST include a line documenting `--stop-after ` with a one-line description naming planning-phase artifact ids AND `ship` as the valid values +- [ ] Pass + +#### Step 5.2: option is accepted with a valid value +- **Setup**: a clean repository on `main` +- **Do**: the user runs `metta propose "" --stop-after tasks --json` +- **Observe**: the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "tasks"` AND a change directory at `spec/changes//` MUST exist with `.metta.yaml` containing `stop_after: tasks` +- [ ] Pass + +#### Step 5.3: option is accepted with the `ship` value and persisted +- **Setup**: a clean repository on `main` +- **Do**: the user runs `metta propose "" --stop-after ship --json` +- **Observe**: the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "ship"` AND `.metta.yaml` MUST contain `stop_after: ship` validating against the existing `ChangeMetadataSchema` without schema changes +- [ ] Pass + +#### Step 5.4: option is omitted, no `stop_after` field is persisted +- **Setup**: a clean repository on `main` +- **Do**: the user runs `metta propose "" --json` with no `--stop-after` flag +- **Observe**: the JSON output MUST NOT include a `stop_after` field (or MUST set it to `null`) AND `.metta.yaml` MUST NOT include a `stop_after` field — the PR-open default is applied by the skill, not by persisted state +- [ ] Pass + +#### Step 5.5: `ship` is accepted for any workflow +- **Setup**: any resolved workflow (e.g. `standard` or `full`) whose `buildOrder` does not contain a `ship` artifact +- **Do**: the user runs `metta propose "" --stop-after ship --json` +- **Observe**: the CLI MUST exit with code 0 AND persist `stop_after: ship` on the change record +- [ ] Pass + +#### Step 5.6: unknown artifact id is still rejected and the valid list names `ship` +- **Setup**: the resolved workflow `standard` whose `buildOrder` does not contain `spex` +- **Do**: the user runs `metta propose "" --stop-after spex` +- **Observe**: the CLI MUST exit with code 4 AND the error message MUST cite `spex` as unknown AND MUST list the valid values (`intent, stories, spec, research, design, tasks, ship`) AND `spec/changes/` MUST NOT contain a directory for this change +- [ ] Pass + +#### Step 5.7: execution-phase artifact id is still rejected +- **Setup**: any resolved workflow whose `buildOrder` includes `implementation` +- **Do**: the user runs `metta propose "" --stop-after implementation` +- **Observe**: the CLI MUST exit with code 4 AND the error message MUST explain that execution-phase ids are not valid stop points AND `spec/changes/` MUST NOT contain a directory for this change +- [ ] Pass + +#### Step 5.8: existing planning-phase values keep their semantics +- **Setup**: the user passes any previously accepted stop-after value (`intent`, `stories`, `spec`, `research`, `design`, `tasks`, or a non-default-workflow planning id such as `domain-research`) +- **Do**: `metta propose` runs with that value (Run: `metta propose`) +- **Observe**: the value MUST validate and persist exactly as it did before this change, with no change in boundary semantics +- [ ] Pass + +#### Step 5.9: skill parses and forwards `--ship` from `$ARGUMENTS` +- **Setup**: a propose skill invocation whose `$ARGUMENTS` is `add cool feature --ship` +- **Do**: the orchestrator runs Step 1 (CLI invocation) (Run: `add cool feature --ship`) +- **Observe**: it MUST execute `METTA_SKILL=1 metta propose "add cool feature" --stop-after ship --json` AND the description MUST NOT contain the `--ship` token +- [ ] Pass + +#### Step 5.10: `stop_after: ship` restores run-to-merge +- **Setup**: a change record with `stop_after: ship` and the orchestrator has reached `all_complete: true`, run `metta finalize`, pushed the branch, and created the PR +- **Do**: the orchestrator continues past PR creation (Run: `metta finalize`) +- **Observe**: it MUST run `gh pr checks --watch --fail-fast` AND, when all checks pass, `gh pr merge --merge` AND perform post-merge cleanup exactly as the pre-change step 8 did +- [ ] Pass + +#### Step 5.11: planning-phase boundary for `tasks` is unchanged +- **Setup**: a change record with `stop_after: tasks` and the orchestrator has just received `all_complete: false` with `next: ["implementation"]` from `metta complete tasks` +- **Do**: the orchestrator inspects the change record (Run: `metta complete tasks`, `Stopped after`) +- **Observe**: it MUST stop the workflow AND print `Stopped after `tasks`. Run `/metta-execute` to begin implementation.` AND MUST NOT spawn any metta-executor, metta-reviewer, or metta-verifier agent +- [ ] Pass + +#### Step 5.12: default propose run ends at an open PR +- **Setup**: `/metta-propose ` is invoked with no stop-after flag and no `--ship` +- **Do**: the change completes verification and `metta finalize` succeeds (Run: `metta finalize`, `gh pr create`) +- **Observe**: the orchestrator MUST push the branch, run `gh pr create`, report the PR URL, and stop AND the captured session MUST NOT contain a `gh pr merge` invocation +- [ ] Pass + +#### Step 5.13: main does not contain the change after a default run +- **Setup**: a completed default propose run that reported a PR URL +- **Do**: the user inspects the repository +- **Observe**: the change branch's PR MUST be open AND `main` MUST NOT contain the change's merge commit +- [ ] Pass + +#### Step 5.14: the user can land the PR without rework +- **Setup**: a default propose run has stopped at PR-open +- **Do**: the user runs `/metta-ship` (or merges the PR explicitly) +- **Observe**: the change MUST complete — merge, archive, and cleanup — without re-running planning, implementation, or verification +- [ ] Pass + +#### Step 5.15: default-path instructions end at PR creation in both copies +- **Setup**: the updated `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` +- **Do**: their default-path (no `stop_after`) instructions are read (Run: `gh pr create`, `gh pr merge`) +- **Observe**: the terminal actions MUST be `gh pr create` and reporting the PR URL AND every `gh pr merge` mention MUST be inside a condition requiring `stop_after = ship` AND no section commands an unconditional ship on the default path +- [ ] Pass + +#### Step 5.16: the two copies agree +- **Setup**: both SKILL.md copies after this change +- **Do**: their step-8 / ship-path content is compared +- **Observe**: both MUST describe the same default (stop at PR-open) and the same ship opt-in behavior +- [ ] Pass + +#### Step 5.17: tests pass on the updated skill files +- **Setup**: the updated SKILL.md copies with merge conditioned on the ship opt-in +- **Do**: the grep-assert tests run via `npm test` (Run: `npm test`) +- **Observe**: they MUST pass +- [ ] Pass + +#### Step 5.18: tests fail when unconditional merge is reintroduced +- **Setup**: either SKILL.md copy is edited to add an unconditioned default-path `gh pr merge --merge` instruction +- **Do**: the grep-assert tests run +- **Observe**: at least one test MUST fail, naming the offending file +- [ ] Pass + +#### Step 5.19: `/metta-auto` still runs to merge +- **Setup**: `/metta-auto ` is invoked and the change passes verification and finalize +- **Do**: the lifecycle completes +- **Observe**: the auto skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change +- [ ] Pass + +#### Step 5.20: `/metta-fix-issues` still runs to merge +- **Setup**: `/metta-fix-issues ` is invoked and the fix passes verification and finalize +- **Do**: the lifecycle completes +- **Observe**: the fix-issues skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change +- [ ] Pass + +#### Step 5.21: workflow section describes the PR-open default +- **Setup**: the updated `CLAUDE.md` +- **Do**: a reader checks the `/metta-propose` entries in the Metta Workflow section +- **Observe**: the text MUST state that a default propose run ends at an open PR AND that merging requires the explicit ship opt-in or `/metta-ship` +- [ ] Pass diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/design.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/design.md new file mode 100644 index 00000000..49aa14d9 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/design.md @@ -0,0 +1,197 @@ +# Design: fix-metta-propose-runs-entire-lifecycle-through-finalize + +## Approach + +Implements the recorded research decision (research.md, 2026-08-22): **skill-level PR-open default + `ship` sentinel**. Do not re-litigate; the persisted-default alternative was rejected in `research-persisted-default.md`. + +The default `/metta-propose` path becomes: full pipeline → `metta finalize` → push → `gh pr create` → report PR URL → **stop**. Merging (`gh pr checks --watch` + `gh pr merge` + cleanup) moves behind an explicit ship gate that fires only when `STOP_AFTER = "ship"` (skill-parsed `--ship` alias or `--stop-after ship`). No schema change, no workflow YAML change, no boundary-logic change: `ship` never matches a planning artifact id, so the existing Step 3 boundary check never fires and the loop naturally runs to `all_complete` (verified in `research-skill-level-default.md` §4). + +Everything load-bearing is anchored on **exact literal strings** chosen in this document. The skill text and the grep-assert tests use the same literals, authored in the same commit — this is the whole mitigation for grep-assert brittleness. Do not paraphrase any string marked *(anchor)* below. + +Total surface: 2 SKILL.md copies (byte-identical), `propose.ts`, `refresh.ts` + checked-in `CLAUDE.md`, 1 new test file, additions to 1 existing test file. + +## Components + +### 1. Propose SKILL.md — Step 8 restructure (both copies) + +Files: `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` — byte-identical (enforced by `tests/skill-discovery-loop.test.ts:71`, `tests/grounding.test.ts:35-36`, `tests/template-deploy-sync.test.ts`). + +Replace current lines 272–279 (Step 8, sub-steps a–f) with this structure. Sub-steps a–c are unchanged from today; d is new; the marker paragraph and e–g are the relocated ship path: + +```markdown +8. When `all_complete: true`: + a. `metta finalize --json --change ` → runs gates, archives, merges specs + b. `git -C "{change_root}" push -u origin metta/` → push the feature branch to the remote + c. `gh pr create --title "" --body ""` → open a PR. The body MUST end with `🤖 Generated with [Claude Code](https://claude.com/claude-code)` + d. **Default path ends at an open PR. Do NOT merge; report the PR URL and stop.** + When `STOP_AFTER` is empty (and the change record has no persisted `stop_after`), report exactly: + ``PR open for review: . Run `/metta-ship` to land it, or merge the PR on GitHub yourself.`` + then proceed to Step 9 and return control to the user. On this default path you MUST NOT watch CI checks as a precursor to merging, MUST NOT merge the PR, and MUST NOT perform post-merge cleanup (main pull, branch/worktree removal). + + **Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record's persisted `stop_after` is `ship`):** + + e. `gh pr checks --watch --fail-fast` → wait for all CI checks on the PR to complete before merging. If any check fails or is cancelled, do NOT merge — report the failing check(s) and the PR URL to the user and stop. If gh reports that no checks are reported yet (checks can lag PR creation by a few seconds), wait ~10s and retry the command + f. `gh pr merge --merge` → land the PR + g. Back on `main`: `git pull --ff-only`, then clean up the change branch and worktree +``` + +**Load-bearing anchors (verbatim, shared with tests):** + +- Ship-gate marker line *(anchor — the tests split file content on this exact string; it must appear exactly once)*: + + `**Ship opt-in — the following sub-steps run ONLY when `` `STOP_AFTER = "ship"` `` (or the change record's persisted `` `stop_after` `` is `` `ship` ``):**` + + i.e. the literal line: ``**Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record's persisted `stop_after` is `ship`):**`` + +- Canonical default phrase *(anchor)*: ``**Default path ends at an open PR. Do NOT merge; report the PR URL and stop.**`` +- Default handoff report *(anchor)*: ``PR open for review: . Run `/metta-ship` to land it, or merge the PR on GitHub yourself.`` + +**Removed text:** the old 8e clause "unless the user asked to leave it open for review — in that case stop here and report the PR URL instead of merging" MUST NOT survive anywhere in the file (research risk: stale clause reads as merge-by-default). On the ship path the user has explicitly opted into merge; no leave-open escape hatch is needed there. + +Invariants: `gh pr merge` and `gh pr checks` appear **nowhere** in the file before the ship-gate marker line. `gh pr create` stays on the default path (keeps `tests/cli-skills.test.ts` "PR-based shipping" green). + +### 2. Propose SKILL.md — "Critical" section reword (both copies) + +Replace current lines 281–288 in full with: + +```markdown +## Critical: verify, finalize, and open the PR + +- Do NOT skip verification — a metta-verifier agent MUST run gates and confirm spec compliance +- Do NOT stop before the PR exists — when no planning-phase `stop_after` boundary fired in Step 3, finalize, push, and `gh pr create` are mandatory on every completed run +- Merging is NOT part of the default path. Watching CI checks, merging the PR, and post-merge cleanup happen only under the Step 8 ship opt-in (`stop_after = ship`); otherwise stop at the open PR and hand off to `/metta-ship` +- If metta finalize fails gates, spawn a metta-executor to fix, then retry +- Direct local merge of the change branch into main (`git merge`) is forbidden — every change ships through a pushed branch and a GitHub PR +- If a dispatched step appears orphaned, follow the residual orphaning recovery protocol in metta-skill-host.md. +- If an executor or verifier STOP-reports a silent-write anomaly (Edit/Write success with no on-disk effect), escalate to the user with the report; never work around it via bash writes or orchestrator-performed writes. +``` + +New heading *(anchor)*: `## Critical: verify, finalize, and open the PR`. + +Forbidden strings that MUST NOT appear anywhere in either copy after this change (tests assert absence, verbatim): +- `Critical: You MUST verify, finalize, and ship` +- `Do NOT stop after the last artifact` +- `finalize + ship must happen` +- `unless the user asked to leave it open` + +Preserved rules (tests assert presence): the PR-only shipping prohibition line beginning `Direct local merge of the change branch into main`, the orphaning-recovery bullet, and the silent-write-anomaly bullet — all three carry over verbatim except as shown above. + +### 3. Propose SKILL.md — Step 1 `--ship` alias + Step 3 clarifier (both copies) + +**Step 1.** Immediately after the existing `--stop-after` parse block (current lines 45–51), add: + +```markdown + **Parse optional `--ship` from `$ARGUMENTS`:** + + - If `$ARGUMENTS` contains the token `--ship`, remove it from `$ARGUMENTS` and set `STOP_AFTER = "ship"`. `--ship` is an alias for `--stop-after ship` — forward it to the CLI as `--stop-after ship` (there is no CLI `--ship` flag). If both `--ship` and `--stop-after ` are present, `--ship` takes precedence. + - The remaining text is the description. +``` + +Also extend the existing "**Scope of `STOP_AFTER`:**" bullet (current line 51) — append one sentence: `` The special value `ship` is NOT a planning-phase artifact: it means "run to merge" and is handled by the Step 8 ship opt-in, never by the Step 3 boundary check. `` + +The command matrix (current lines 53–58) needs no structural change — `ship` rides the existing `--stop-after ` invocations. + +**Step 3.** Add one bullet to the "Stop-after boundary check" list (after the two boundary conditions, current line 106): + +```markdown + - `ship` is not a planning boundary: when `STOP_AFTER = "ship"` (or persisted `stop_after: ship`), this check never fires for any artifact — do not hunt for a `ship` artifact; continue the loop to `all_complete` and apply the Step 8 ship opt-in. +``` + +Resume-command mapping (current lines 113–116) is unchanged. + +### 4. `src/cli/commands/propose.ts` + +Three localized edits (~6 lines), no persistence changes — `stopAfter === 'ship'` rides the existing `createChange(...)` and JSON-output paths untouched: + +1. **Help text** (lines 17–20): replace the option description with + `'Stop after the named planning artifact (e.g. intent, stories, spec, research, design, tasks), or ship to run through merge'` +2. **Ship short-circuit** (line 39): change the validation guard so `ship` bypasses `buildOrder` checks entirely: + ```ts + if (stopAfter !== undefined && stopAfter !== 'ship') { + // existing planningIds / execution-phase / membership checks, unchanged + } + ``` +3. **Valid-value lists** (line 43): `const validList = planningIds.join(', ') + ', ship'` — this single change puts `ship` in both error messages (execution-phase rejection and unknown-id rejection), matching the spec scenario that lists `intent, stories, spec, research, design, tasks, ship`. + +Exit-code-4 contract, no-state-on-error behavior, and the "no `stop_after` field when flag absent" persistence behavior are all untouched. No CLI `--ship` boolean flag is added (constraint; the skill owns the alias — single source of truth). + +### 5. `src/cli/commands/refresh.ts` + checked-in `CLAUDE.md` (must land together) + +`refresh.ts` line 131 — replace the lifecycle bullet with *(anchor, generated verbatim into CLAUDE.md)*: + +```ts +lines.push('- `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship`') +``` + +Checked-in `CLAUDE.md` — edit the matching `### Lifecycle skills` bullet to the identical rendered string: + +```markdown +- `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship` +``` + +Both edits are one atomic pair: editing only CLAUDE.md is reverted by the next `/metta-refresh`; editing only refresh.ts leaves the checked-in doc stale until then. **`src/delivery/workflow-primer.ts` is NOT changed** — per research, its `/metta-propose` entry-point bullet makes no merge claim, so it already satisfies the "MUST NOT imply merge by default" requirement. + +### 6. Tests + +**New file `tests/skill-propose-ship-gate.test.ts`** — modeled on `tests/skill-discovery-loop.test.ts` (readFile + `toContain`/`not.toContain`; same path constants pattern). Test constants (verbatim from the anchors above): + +```ts +const SHIP_GATE_MARKER = + '**Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record\'s persisted `stop_after` is `ship`):**' +const DEFAULT_PHRASE = '**Default path ends at an open PR. Do NOT merge; report the PR URL and stop.**' +const HANDOFF_PHRASE = 'PR open for review: . Run `/metta-ship` to land it' +``` + +For **each** of the two propose SKILL.md paths (template + deployed; a `describe.each` or loop so a failure names the offending file, per spec scenario): + +1. **Split-on-heading strategy:** `const parts = contents.split(SHIP_GATE_MARKER)` → `expect(parts).toHaveLength(2)` (marker present exactly once). Then: + - `expect(parts[0]).not.toContain('gh pr merge')` and `expect(parts[0]).not.toContain('gh pr checks')` — no merge machinery anywhere before the gate. + - `expect(parts[1]).toContain('gh pr checks --watch --fail-fast')` and `expect(parts[1]).toContain('gh pr merge --merge')` — ship path intact. +2. `expect(contents).toContain(DEFAULT_PHRASE)` and `toContain(HANDOFF_PHRASE)`. +3. Forbidden-string absences (verbatim): `Critical: You MUST verify, finalize, and ship`, `Do NOT stop after the last artifact`, `finalize + ship must happen`, `unless the user asked to leave it open`. +4. Survivals: `toContain('Direct local merge of the change branch into main')`, `toContain('gh pr create')`. +5. **Scope guard (never glob over `skills/`):** read `src/templates/skills/metta-auto/SKILL.md` and `src/templates/skills/metta-fix-issues/SKILL.md`; assert each still `toContain('gh pr merge')` — proves this change did not touch their run-to-merge behavior. These two files legitimately keep unconditional merge text; asserting anything else over them is out of scope. + +Byte-identity between the two propose copies is already enforced by existing tests — do not duplicate. + +**Additions to `tests/cli-propose-stop-after.test.ts`** (same `runCli` harness, same fixture setup): + +1. `persists stop_after: ship` — `['--json', 'propose', 'demo ship stop', '--stop-after', 'ship']` → `code === 0`, `data.stop_after === 'ship'`, `.metta.yaml` contains `stop_after: ship`. +2. Extend the existing `rejects unknown --stop-after value` test with `expect(text).toContain('ship')` — the valid-value list now names `ship`. +3. `--help names ship` — `runCli(['propose', '--help'], tempDir)` → stdout contains `ship` on the `--stop-after` option line (satisfies the help scenario in spec.md). + +## Data Model + +**No changes.** `src/schemas/change-metadata.ts:116` (`stop_after: z.string().optional()`) already validates `ship`; the value persists through the existing `createChange` path. Absent-flag behavior is byte-identical to today: no `stop_after` field in `.metta.yaml`, `stop_after: null` in JSON output. The PR-open default is deliberately **not** represented in persisted state (research decision — the rejected alternative's fatal flaw was persisting it). Do not "harden" the field to a `z.enum`: the valid set is workflow-dependent (`domain-research` etc.), so a free string is correct. + +## API Design + +**CLI surface** (`metta propose`): +- `--stop-after ` gains one accepted value, `ship` — a lifecycle sentinel valid for every workflow, never checked against `buildOrder`. All previously accepted values keep identical validation, persistence, and semantics. Error messages' valid-value lists append `, ship`. No new flags. + +**Skill contract** (`/metta-propose`): +- `$ARGUMENTS` token `--ship` → skill sets `STOP_AFTER = "ship"` and invokes the CLI with `--stop-after ship`; the description never contains the `--ship` token. +- Default path terminal behavior: `gh pr create` + the exact handoff report naming `/metta-ship`. +- Ship path (`STOP_AFTER = "ship"` or persisted `stop_after: ship`): checks-watch → merge → cleanup, exactly the pre-change 8d–8f behavior. +- Planning-phase `stop_after` boundary behavior in Step 3: unchanged, including the exact handoff-line format and resume-command mapping. + +**Generated docs contract:** `buildWorkflowSection()` in refresh.ts emits the new propose bullet; the checked-in CLAUDE.md carries the identical rendered line. + +## Dependencies + +- No new packages, no version bumps. +- `gh` CLI: pre-existing dependency of the ship path — this change narrows its default use (drops `checks`/`merge` from the default path), adding nothing. Pre-existing GitHub coupling noted (PR-based shipping is a project-level rule); this change does not deepen it — no new vendor lock-in introduced. +- Internal ordering: skill-text anchors (§1–§3) and the test literals (§6) must be authored against this document in the same change; refresh.ts and CLAUDE.md (§5) must land in the same commit set. + +## Risks & Mitigations + +| Risk | Mitigation | +|---|---| +| Grep-assert brittleness — prose edits break literal anchors | All anchors are exact strings fixed in this design; skill text and tests share them verbatim and ship together. The split-on-marker strategy tests structure (position relative to the gate), not prose. | +| One SKILL.md copy edited, the other missed | Three existing byte-identity/sync tests fail immediately; no new mechanism needed. | +| CLAUDE.md regeneration clobbers the wording | refresh.ts:131 and CLAUDE.md edited as an atomic pair (§5); the generator is the durable source. | +| Orchestrator drift — skill-level default is instruction-following, not runtime-enforced | Inherent to the chosen approach (accepted in research). Tests guard the instructions; the UAT scenario "captured session contains no `gh pr merge`" covers runtime behavior. | +| Stale "leave it open" clause survives and reads as merge-by-default | Explicit forbidden string; test asserts absence (§6 item 3). | +| Over-broad test scope re-flags metta-auto / metta-fix-issues | Tests target only the two propose files; scope guard positively asserts auto/fix-issues retain `gh pr merge`. | +| `ship` value confuses the Step 3 boundary check ("unknown artifact") | One-line Step 3 clarifier (§3); the check compares against completed artifact ids, which never equal `ship`. | +| Failure mode of any wording miss | Stops too early (recoverable via `/metta-ship`) — never merges without consent. The change strictly narrows autonomous authority. | diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/gates.yaml b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/gates.yaml new file mode 100644 index 00000000..c594a45a --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/gates.yaml @@ -0,0 +1,18 @@ +finalized_at: 2026-08-21T23:56:29.261Z +all_passed: true +results: + - gate: stories-valid + status: pass + duration_ms: 548 + - gate: tests + status: pass + duration_ms: 538400 + - gate: lint + status: pass + duration_ms: 5496 + - gate: typecheck + status: pass + duration_ms: 5876 + - gate: build + status: pass + duration_ms: 7066 diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/intent.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/intent.md new file mode 100644 index 00000000..025171c9 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/intent.md @@ -0,0 +1,45 @@ +# fix-metta-propose-runs-entire-lifecycle-through-finalize + +## Problem + +`/metta-propose` runs the entire change lifecycle through finalize, push, PR creation, CI watch, and **merge to main** with no default stop point. A verb named "propose" autonomously ships and lands code without user consent. This was observed twice: the zeus session's per-DEX-switch change (PR #28, 2026-08-19) and the metta repo's backlog/milestones rework (PR #85) — in both cases the orchestrator expected intent/spec artifacts and at most an open PR, and instead got a merged branch. + +Root cause is confirmed as deliberate design, not a wiring bug. The stop-after boundary check IS honored by the fork skill (step 3 checks `STOP_AFTER` after every `metta complete`), but the propose-stop-after capability is scoped as opt-in flag handling only: when the flag is absent, `.metta.yaml` carries no `stop_after` field (`spec/specs/propose-stop-after/spec.md:70`, `src/cli/commands/propose.ts:38-39`), and the skill's default path takes over. That default path is a full ship: step 8 runs `metta finalize` → `git push` → `gh pr create` → `gh pr checks --watch` → `gh pr merge --merge` (`.claude/skills/metta-propose/SKILL.md:272-277`), reinforced by a section titled "Critical: You MUST verify, finalize, and ship" that commands "Do NOT stop after the last artifact — finalize + ship must happen" (`SKILL.md:281-284`), with "leave it open for review" as the opt-out. + +Consequences: +- Violates least surprise: "propose" defaults to autonomous merge-to-main. +- Sits in tension with the constitution's "No auto-push to remote without explicit user confirmation." +- Makes `/metta-propose` behaviorally identical to `/metta-auto`, erasing the intended distinction between the two entry points. + +## Proposal + +Implement the recorded user decision (2026-08-22, candidate solution 1): **flip `/metta-propose`'s default stop point to PR-OPEN.** + +1. **Default behavior — stop at open PR.** `/metta-propose` with no stop-after flag runs the full autonomous pipeline (discovery → planning → implementation → verification → finalize → push → `gh pr create`), then STOPS and reports the PR URL for user review. It MUST NOT run `gh pr merge` by default. +2. **Merge stays available as an explicit opt-in.** A `--ship` / `stop-after=ship` opt-in, wired through the existing propose-stop-after machinery, restores run-to-merge behavior. All existing stop-after values keep working unchanged. +3. **Spec delta on propose-stop-after.** Flip the capability's default semantics from opt-in-stop (absent flag → no boundary, skill ships) to stop-at-pr-open (absent flag → boundary at PR-open). Existing stop-after value handling is preserved. +4. **Skill updates in both copies.** Rewrite step 8 and the "Critical: You MUST verify, finalize, and ship" section in both the installed skill (`.claude/skills/metta-propose/SKILL.md`) and the template (`src/templates/skills/metta-propose/SKILL.md`) so the default terminal action is `gh pr create` + report, and merge is conditional on the explicit ship opt-in. +5. **CLAUDE.md workflow wording.** Update the workflow section so `/metta-propose`'s described behavior matches the new default (ends at an open PR unless ship is requested). +6. **Regression guard.** Add grep-assert tests asserting the propose SKILL.md (both copies) contains no unconditional merge instruction — so a future skill edit cannot silently restore auto-merge. + +## Impact + +- **Capabilities:** `propose-stop-after` (default semantics flipped via spec delta). No other capability's requirements change. +- **Files:** + - `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` — default path ends at PR-open; merge gated on explicit ship opt-in. + - `spec/specs/propose-stop-after/spec.md` — delta flipping the no-flag default. + - `CLAUDE.md` — workflow section wording for `/metta-propose`. + - CLI/schema surface as needed to carry the `--ship` / `stop-after=ship` opt-in through the existing propose-stop-after machinery (`src/cli/commands/propose.ts` and related). + - New grep-assert tests over both SKILL.md copies. +- **Behavior change (intentional, user-mandated):** `/metta-propose` no longer merges to main by default. Users relying on the old auto-merge default must pass the ship opt-in or use `/metta-auto`. +- **Unchanged:** `/metta-auto` and `/metta-fix-issues` keep run-to-merge behavior; all existing stop-after values (`intent`, `tasks`, etc.) keep their meaning; the stop-after boundary-check wiring in the fork skill is already correct and is reused, not rebuilt. +- **Risk:** low — the change narrows autonomous authority; the failure mode of a wording miss is stopping too early (recoverable via `/metta-ship`), not merging without consent. + +## Out of Scope + +- **Candidate solution 2 (rejected):** making `STOP_AFTER=tasks` (or any earlier artifact) the default for `/metta-propose`. The pipeline still runs end-to-end through PR creation. +- **Candidate solution 3 (rejected):** config-driven per-verb default stop points (e.g. a `defaults.stop_after` map in metta config). No new configuration surface is added. +- **Changing `/metta-auto` or `/metta-fix-issues`:** both keep their run-to-merge behavior exactly as-is. +- **Removing or renaming existing stop-after values:** all currently accepted values continue to work with unchanged semantics. +- **Redesigning the finalize/ship pipeline itself** (gates, archiving, spec merge, CI watch): only the default terminal action after PR creation changes. +- **Retroactive handling of PR #28 / PR #85:** already merged; no reverts. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-docs-and-tests.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-docs-and-tests.md new file mode 100644 index 00000000..0a56df36 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-docs-and-tests.md @@ -0,0 +1,111 @@ +# Research: documentation sync + regression test enforcement + +Slice for `fix-metta-propose-runs-entire-lifecycle-through-finalize` — /metta-propose default changes +from merge-PR to stop-at-PR-open (merge only via `--ship` / stop-after=ship). + +All paths below are relative to the change root +`/home/utx0/Code/metta/.metta/worktrees/fix-metta-propose-runs-entire-lifecycle-through-finalize/`. + +## 1. Skill copy sync — already enforced, no new mechanism needed + +- `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` are + **byte-identical today** (verified via `diff`). +- `tests/template-deploy-sync.test.ts` auto-discovers every file under `src/templates/{agents,skills,hooks,statusline}` + and asserts byte-identity with the committed `.claude/` copy, plus no-orphan checks. Any edit to the + template without the deployed copy (or vice versa) fails CI. +- **Conclusion:** edit BOTH copies identically; no new sync test is required. `tests/skill-discovery-loop.test.ts` + and `tests/grounding.test.ts` also read both paths, confirming the convention. + +## 2. Regression test strategy + +### Existing pattern to follow +`tests/skill-discovery-loop.test.ts` is the canonical grep-assert model: `readFile` the **template** +path, then `toContain` / `not.toContain` on stable phrases (byte-identity test covers the deployed copy +transitively). `tests/grounding.test.ts` uses the same shape. No existing test asserts on +`gh pr merge`, `Do NOT stop`, or the "Critical" section wording — new assertions are green-field. + +### Recommended host file +**New file `tests/skill-propose-ship-gate.test.ts`** (options considered: extend +`skill-discovery-loop.test.ts` — wrong topic, discovery-loop only; extend `cli-propose-stop-after.test.ts` — +that file exercises the CLI binary in temp dirs, not skill content). A dedicated file matches the +1:1 topic-per-test-file convention and names the invariant. + +### Recommended assertions (low-brittleness) +Read `src/templates/skills/metta-propose/SKILL.md` once, then: + +1. **No unconditional merge instruction.** Split content on the ship-gate heading (whatever heading the + new wording introduces, e.g. `## Ship (only when requested)`); assert `gh pr merge` does **not** + appear in the portion before the gate heading, and appears at least once after it. This is the + robust form of "merge is ship-gated" without regexing surrounding sentences. + - Simpler fallback if the section layout stays flat: assert the line containing `gh pr merge` is + preceded within the file by the gate marker phrase — but the split-on-heading form is less brittle. +2. **Old mandate removed.** `expect(content).not.toContain('Do NOT stop after the last artifact')` and + `not.toContain('finalize + ship must happen')` — these exact strings exist today at lines 284 (the + `## Critical: You MUST verify, finalize, and ship` block) and must go. Also + `not.toContain('Critical: You MUST verify, finalize, and ship')` (assert on the heading text, which + is the most stable token). +3. **New default stated.** Assert one short canonical phrase the new wording will contain, e.g. + `toContain('stop after opening the PR')` (pick the exact phrase during implementation and use it + verbatim — one phrase, not a paragraph). +4. **Preserved invariant.** `toContain('Direct local merge of the change branch into main')` — the + git-merge prohibition must survive the rewrite. +5. **Scope guard for siblings.** In the same file, assert `src/templates/skills/metta-auto/SKILL.md` and + `src/templates/skills/metta-fix-issues/SKILL.md` still `toContain('gh pr merge')` — locks in that + run-to-merge remains their behavior (see §5). + +Avoid: regexes over multi-sentence prose, line-number anchoring, asserting full paragraphs. + +## 3. Wording touchpoints (exhaustive) + +| # | Path | Current wording | Action | +|---|------|-----------------|--------| +| 1 | `src/templates/skills/metta-propose/SKILL.md` | Step 8d–f (`gh pr checks --watch`, `gh pr merge`, post-merge cleanup, ~lines 273–280) + `## Critical: You MUST verify, finalize, and ship` (lines 281–288) + line 110 mention of "Step 8 (finalize/merge)" | Primary edit: gate merge behind `--ship`/stop-after=ship | +| 2 | `.claude/skills/metta-propose/SKILL.md` | identical copy | Same edit, byte-identical | +| 3 | `CLAUDE.md` line 74 | `- /metta-propose — start a new change (standard workflow)` (inside `` … `end` markers, lines 39–101 — **generated**) | Update to note stop-at-PR default; must match generator output | +| 4 | `src/cli/commands/refresh.ts` line 131 | pushes the same Lifecycle-skills bullet literally | Generator source for #3 — must change in lockstep | +| 5 | `src/delivery/workflow-primer.ts` line 17 (`ENTRY_POINTS_BULLETS`) | `/metta-propose — anything non-trivial…` (feeds both `workflowPrimerShort` → init scaffold and `workflowPrimerLong` → refresh, surfaces as CLAUDE.md line 48) | Only touch if the new default is mentioned in the entry-point bullet; current text doesn't claim merge behavior — likely no change, decide at implementation | +| 6 | `spec/specs/propose-stop-after/spec.md` line 107 | "THEN the orchestrator MUST proceed through implementation, review, verification, finalize, and merge exactly as it does today" (no-stop-after default scenario) | **Spec conflict** — needs a spec delta in this change; flag to the slice owning spec updates | +| 7 | `docs/api.md` ~lines 1373–1420 (propose-stop-after section) | mirrors #6 scenarios incl. "skill behaves identically when no stop_after is set" | Updated when specs merge at finalize (docs are ship-phase output); no manual pre-edit needed beyond spec delta | +| 8 | `docs/architecture.md` line 231 / `docs/getting-started.md` line 10 / `docs/README.md` line 3 | generic "propose → … → finalize → ship" lifecycle phrasing | No change — describes the framework lifecycle, not propose's default endpoint. Optionally clarify in architecture.md line 231 decision note | + +## 4. CLAUDE.md workflow section IS generated + +`metta refresh` (`src/cli/commands/refresh.ts`) rewrites the `` block: +line 127 splices `workflowPrimerLong()` and lines 130–131 emit the hard-coded `### Lifecycle skills` +bullets. **Any wording change must land in `refresh.ts` (and `workflow-primer.ts` if that bullet +changes) AND the checked-in `CLAUDE.md`**, or the next refresh reverts it. `tests/refresh.test.ts` +line 241 and `tests/cli-skills.test.ts` line 62 assert `/metta-propose` presence only (not the +descriptive text) — safe. `tests/delivery.test.ts` asserts primer structure, not the propose bullet. + +## 5. Sibling skills — keep vs change + +- `src/templates/skills/metta-auto/SKILL.md` (steps 9–13, `## Critical: You MUST review, verify, + finalize, and ship`, line 86 "Do NOT stop after verification — finalize + ship must happen"): + **KEEP** — auto is explicitly full-lifecycle run-to-merge. +- `src/templates/skills/metta-fix-issues/SKILL.md` (steps 9–10 incl. `gh pr merge`, line 127): + **KEEP** — fix-issues runs to merge. +- `src/templates/skills/metta-ship/SKILL.md`: **KEEP** — ship is the merge path; propose's new wording + should point users here (or to `--ship`) for landing the PR. +- `src/templates/skills/metta-next/SKILL.md`, `metta-quick/SKILL.md`: no merge-behavior claims about + propose found — no change (verify quick during implementation; it has its own ship steps out of scope). +- Deployed `.claude/skills/` twins of any file touched must be updated identically (§1). + +## Recommendation + +Host new assertions in `tests/skill-propose-ship-gate.test.ts` using the split-on-heading + +`toContain`/`not.toContain` strategy above (assertions 1–5). Rely on the existing +`template-deploy-sync.test.ts` for copy sync. Update `refresh.ts` line 131 + checked-in `CLAUDE.md` +line 74 together; leave `workflow-primer.ts` untouched unless the entry-point bullet gains +default-behavior wording. Flag the `propose-stop-after` spec scenario (touchpoint #6) to the spec-delta +slice — it currently mandates the old merge-by-default behavior. + +## Risks + +- **Phrase-anchored assertions couple test to wording**: mitigated by anchoring on short canonical + tokens (headings, command strings) chosen at implementation time; the test file must be authored in + the same commit as the wording change. +- **refresh reverts CLAUDE.md** if `refresh.ts`/primer are missed — highest-likelihood regression; + covered by touchpoints #3/#4 landing together. +- **Spec contradiction** (#6): if the spec delta is skipped, verification against + `propose-stop-after/spec.md` line 107 will fail the new behavior. +- No web grounding needed — all findings are from in-tree code; no external API claims made. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-persisted-default.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-persisted-default.md new file mode 100644 index 00000000..0ea28ad5 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-persisted-default.md @@ -0,0 +1,117 @@ +# Research: Persisted default stop point (`stop_after: pr-open` written into the change record) + +Approach under evaluation: when no `--stop-after` is supplied, `propose.ts` writes +`stop_after: pr-open` into the change's `.metta.yaml`, so the skill's existing +persisted-stop_after boundary check enforces the PR-open default "with no new skill logic." + +## 1. Code trace + +- `src/cli/commands/propose.ts:38-54` — `stopAfter` comes from `options.stopAfter`; validated + against the resolved workflow's `buildOrder` (execution-phase ids `implementation`/`verification` + rejected; unknown ids rejected). `pr-open` and `ship` are NOT in any `buildOrder`, so today's + validation would reject them — a lifecycle-stage carve-out would be required. +- `src/cli/commands/propose.ts:67-76` — `stopAfter` passed positionally to + `ArtifactStore.createChange` (arg 7). +- `src/artifacts/artifact-store.ts:76-134` — `createChange` sets `metadata.stop_after` only when + `stopAfter !== undefined` (lines 120-122). Injection point choice: propose.ts (`options.stopAfter + ?? 'pr-open'`) keeps `createChange` and `metta quick` untouched; injecting inside `createChange` + would leak the default into quick-mode changes too. +- `src/cli/commands/quick.ts:39-48` — quick calls `createChange` with `stopAfter = undefined` + explicitly. Unaffected only if the default is injected in propose.ts. + +## 2. Schema (`src/schemas/change-metadata.ts:116`) + +`stop_after: z.string().optional()` — any string validates, so `pr-open`/`ship` need **no schema +change** and no migration for existing records. But: the spec explicitly says the schema accepts any +string and membership validation lives in the CLI, so tightening to an enum is out of scope anyway. +The real back-compat problem is **semantic, not structural**: today "field absent" means "full +lifecycle." Under a persisted default, absent-vs-present flips meaning by creation date — old +records (absent) would mean full lifecycle, new records always carry a value. Any reader that +treats absence as "run to completion" (the propose skill today, future tooling) faces a two-epoch +interpretation problem with no marker distinguishing the epochs. + +## 3. Spec contradictions (`spec/specs/propose-stop-after/spec.md`, 71 reqs) + +Directly contradicted: +- **Req 1, scenario "option is omitted, full-lifecycle behavior preserved" (lines 19-22):** flag + absent → JSON `stop_after: null` AND `.metta.yaml` MUST NOT include `stop_after`. Head-on + contradiction; requirement text (line 6: "behavior MUST be identical to the pre-change + implementation") must be rewritten. +- **Req 2, validation (lines 25-43):** `--stop-after` MUST validate against `buildOrder`; `pr-open` + is by definition not in `buildOrder`. Needs a new "lifecycle-stage values" class, new error-message + wording, updated valid-value lists in help text and scenarios. +- **Req 4, `createChange` (lines 67-80):** "omits `stop_after` when not supplied" survives only if + injection happens in propose.ts — the scenario stays true but becomes untested-in-practice for the + propose path. +- **Req 5, scenario "skill behaves identically when no `stop_after` is set" (lines 104-107):** + becomes vacuous/dead — no propose-created change would ever lack the field. +- **Req 7 handoff determinism (lines 141-155):** resume-command lookup table must gain `pr-open` → + `/metta-ship` (or similar); boundary semantics change (see 4). + +Spec delta: 4-5 of 8 requirements rewritten plus new requirements for the default and the +lifecycle-stage value class — roughly a half-rewrite of the capability spec, plus touches to +finalize-ship spec (merge preconditions) and the status requirement's examples. + +## 4. Fatal flaw: the existing boundary check cannot fire on `pr-open` + +`.claude/skills/metta-propose/SKILL.md` Step 3 boundary check (lines ~101-115): it runs **after +every `metta complete `** and matches when persisted `stop_after` **equals the artifact id +just completed**. `pr-open` is never an argument to `metta complete` — it is not an artifact. The +check can never match it. The premise "no new skill logic" is therefore **false**: Step 8 +(finalize/merge) must still gain a new conditional ("if `stop_after` is `pr-open`, stop after PR +creation, print handoff, do not merge") — exactly the skill logic this approach was supposed to +avoid. The persisted field becomes a second copy of state whose only consumer is new skill logic. + +## 5. Blast radius: /metta-auto, /metta-fix-issues, /metta-quick + +- `/metta-auto` (SKILL.md lines 22-23) and `/metta-fix-issues` (SKILL.md line 35) both create + changes via the **same** `metta propose` CLI call. A persisted default lands in their change + records too. +- Today neither skill reads `stop_after`, so they would merge anyway — leaving a **lying record** + (`stop_after: pr-open` on a merged change). If any shared/future boundary logic honors the field, + auto and fix-issues silently stop before ship, breaking their contracts. +- Mitigation requires both skills to pass an explicit `--stop-after ship` (or new `--ship`) on + their propose calls — two more skill files + two `src/templates/skills/` copies + spec updates, + expanding the change well beyond propose. +- `metta quick` is safe only with propose.ts-level injection; `createChange`-level injection + contaminates quick-mode records that no skill would honor. + +## 6. File-by-file change list (if pursued) + +1. `src/cli/commands/propose.ts` — default injection; lifecycle-stage carve-out in validation; + JSON/text output updates. +2. `src/cli/commands/propose.test.ts` (or test twin) — rewrite omitted-flag assertions. +3. `.claude/skills/metta-propose/SKILL.md` + `src/templates/skills/metta-propose/SKILL.md` — Step 8 + pr-open gate, resume-command mapping additions, handoff line (the "new skill logic" anyway). +4. `.claude/skills/metta-auto/SKILL.md` + template — explicit `--stop-after ship` on propose call. +5. `.claude/skills/metta-fix-issues/SKILL.md` + template — same. +6. `spec/specs/propose-stop-after/spec.md` — rewrite ~4-5 requirements; add lifecycle-stage value + class and default-behavior requirements. +7. `spec/specs/finalize-ship/spec.md` — merge-precondition touch-ups (audit needed). +8. `src/schemas/change-metadata.ts` — no change required (string already validates). + +## 7. Risks + +- **False premise** — persisted-stop_after check only matches `metta complete` artifact ids; + `pr-open` needs new Step-8 skill logic regardless. (High, certain.) +- **Auto/fix-issues contamination** — same CLI path; either lying records now or silent early stops + later; forces compensating flags in two more skills. (High.) +- **Semantic epoch split** — absence means different things before/after this change; ambiguous for + all future readers of `.metta.yaml`. (Medium.) +- **Spec churn** — ~half of propose-stop-after rewritten, incl. deleting the load-bearing + "identical when omitted" guarantees other tooling may assume. (Medium.) +- **Validation surface growth** — two value vocabularies (artifact ids + lifecycle stages) in one + field, one flag, one error message. (Low-medium, permanent complexity.) + +## 8. Verdict + +**Feasible but not recommended.** The approach's sole advantage — reusing the existing persisted +boundary check with no new skill logic — does not hold: that check matches only planning-artifact +ids passed to `metta complete`, so `pr-open` enforcement requires new Step-8 skill logic anyway. +Given that, persisting a default buys nothing over a skill-level default (propose SKILL Step 8 stops +at PR-open unless `--ship`/`--stop-after ship` was parsed), while costing: contamination of +auto/fix-issues change records via the shared CLI path, a semantic flip of field-absence that +contradicts 4-5 existing spec requirements, and a dual-vocabulary `stop_after` field. If the +orchestrator wants persistence for auditability, the safe variant is the inverse: persist only the +**opt-in** (`stop_after: ship` when `--ship` is passed) and let absence keep its current documented +meaning, with the PR-open default living in the propose skill alone. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-skill-level-default.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-skill-level-default.md new file mode 100644 index 00000000..c34e1492 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research-skill-level-default.md @@ -0,0 +1,69 @@ +# Research: skill-level default + `ship` sentinel value + +Approach: no persisted `stop_after` when the flag is absent (PR-open default lives in the skill text); `--stop-after ship` is a validation sentinel accepted by `propose.ts` for every workflow; the skill maps a `--ship` argument token to `--stop-after ship`. + +## Findings + +### 1. SKILL.md copies — confirmed byte-identical + +`diff .claude/skills/metta-propose/SKILL.md src/templates/skills/metta-propose/SKILL.md` → identical. Byte-identity is already test-enforced twice: `tests/skill-discovery-loop.test.ts:71` and `tests/grounding.test.ts:35-36`. Both copies must receive the same edit. + +Exact sections to change (line numbers from the current file): + +| Lines | Section | Change | +|---|---|---| +| 45–51 | Step 1, `--stop-after` parsing | Add a `--ship` parse rule: if `$ARGUMENTS` contains the token `--ship`, remove it and set `STOP_AFTER = "ship"`. Note `ship` as a valid CLI value alongside planning ids. | +| 53–58 | Step 1, command matrix | No structural change — `--ship` collapses into the existing `--stop-after ` invocations (`--stop-after ship`). Update the trailing note if desired. | +| 101–119 | Step 3, stop-after boundary check | Naturally compatible, one clarifying line needed: `ship` never equals a planning artifact id, so the boundary check never fires and the pipeline runs through Step 8 — add "`ship` is not a planning boundary; it is handled in Step 8" so the orchestrator doesn't error hunting for a `ship` artifact. Resume-command mapping (lines 113–116) unchanged. | +| 272–278 | Step 8 | Split: default path ends after 8c (`gh pr create`) + report PR URL naming `/metta-ship`; 8d (`gh pr checks --watch`), 8e (`gh pr merge`), 8f (cleanup) become conditional on `STOP_AFTER = "ship"` (or persisted `stop_after: ship`). | +| 281–288 | "Critical: You MUST verify, finalize, and ship" | Reword title + line 284 ("Do NOT stop after the last artifact — finalize + ship must happen"). Mandate: verify, finalize, push, PR create; merge only when `stop_after = ship`. Keep lines 286–288 (PR-only shipping, orphaning, silent-write) intact. | + +### 2. `src/cli/commands/propose.ts` — small, localized change + +- Line 17–20: option help string must add `ship` ("…design, tasks, or `ship` to run through merge"). +- Lines 38–54: validation. `ship` short-circuits before the `buildOrder` checks (`if (stopAfter === 'ship') { /* accepted */ }`), and `validList` becomes `planningIds.join(', ') + ', ship'` so both error messages name it. Persistence needs zero change — `stopAfter` already flows into `createChange(...)` (line 74) and into JSON output `stop_after: stopAfter ?? null` (line 86); `ship` rides the same path. Exit-code-4 contract (line 113) untouched. +- **No `--ship` CLI flag is needed.** The skill owns the alias (spec scenario "skill parses and forwards `--ship`"); the CLI surface stays single-flag. Adding a CLI `--ship` boolean would create a second source of truth and a flag-conflict question (`--ship` + `--stop-after tasks`) for no requirement. + +### 3. Schema — no change required + +`src/schemas/change-metadata.ts:116` → `stop_after: z.string().optional()` inside a `.strict()` object. Any string validates; `stop_after: ship` persists with no schema edit, and absent-flag runs omit the field exactly as today (already asserted by `tests/cli-propose-stop-after.test.ts:102-114`). Optional hardening (a `z.enum`) is explicitly NOT wanted — the valid set is workflow-dependent (`domain-research` etc.), so a string is correct. + +### 4. Boundary-check fit + +The Step 3 check compares the just-completed artifact id against `STOP_AFTER` / persisted `stop_after`. Since no workflow's `buildOrder` contains `ship`, `stop_after: ship` never matches and the loop runs to `all_complete: true` — exactly the desired "run to merge" semantics with zero boundary-logic changes. The only fragility is an orchestrator second-guessing an unknown value; the one clarifying sentence in Step 3 removes that. + +### 5. Established grep-assert test pattern + +`tests/skill-discovery-loop.test.ts` is the canonical shape: `readFile` template + deployed copies, `expect(contents).toContain(...)` / `.not.toContain(...)`, plus a byte-identity test. New file (e.g. `tests/skill-propose-pr-open-default.test.ts`) should assert over BOTH copies: +- every line containing `gh pr merge` also matches a ship condition (e.g. `/stop_after\s*=?\s*"?ship"?/i` on the same line/step block); +- the unconditional mandate phrase `finalize + ship must happen` is absent; +- `gh pr create` still present (keeps `tests/cli-skills.test.ts:287` "PR-based shipping" test green — it only requires `gh pr create` present and `git merge metta/` absent, both preserved). + +**Scoping trap:** `metta-auto/SKILL.md:78,86` and `metta-fix-issues/SKILL.md:90,127` legitimately contain unconditional `gh pr merge` and `finalize + ship must happen`. The grep-asserts MUST target only the two metta-propose files, never a glob over `skills/`. + +### 6. CLAUDE.md wording (adjacent finding) + +`CLAUDE.md`'s workflow section is generated: primer text lives in `src/delivery/workflow-primer.ts:17` and skill list lines in `src/cli/commands/refresh.ts:131`. Editing only the checked-in `CLAUDE.md` would be reverted by the next `/metta-refresh` — the durable edit is in `workflow-primer.ts` (and/or `refresh.ts` line 131), then regenerate. The spec only mandates the CLAUDE.md text; touching the generator too is the correct implementation of it. + +## File-by-file change list + +1. `.claude/skills/metta-propose/SKILL.md` — Step 1 `--ship` alias, Step 3 clarifier, Step 8 split, Critical section reword. +2. `src/templates/skills/metta-propose/SKILL.md` — identical edit (byte-for-byte). +3. `src/cli/commands/propose.ts` — help text + `ship` sentinel in validation + `ship` in error valid-lists (~6 lines). +4. `tests/cli-propose-stop-after.test.ts` — add: `--stop-after ship` accepted/persisted (`stop_after: ship` in `.metta.yaml`); unknown-value error lists `ship`. +5. `tests/skill-propose-pr-open-default.test.ts` (new) — grep-asserts per §5, both copies, propose-only scope. +6. `src/delivery/workflow-primer.ts` (+ regenerated `CLAUDE.md`) — PR-open default wording; `refresh.ts:131` propose one-liner optionally updated. + +No changes: schemas, workflow YAMLs, state-store, metta-auto / metta-fix-issues skills (requirement explicitly forbids touching them). + +## Risks + +- **Sync risk (low, test-covered):** forgetting one SKILL.md copy fails two existing byte-identity tests immediately. +- **Grep-assert brittleness (medium):** "merge only in ship-conditioned text" is a heuristic over prose. Mitigate by structuring Step 8's ship sub-steps under an explicit literal marker (e.g. a line containing both `stop_after = ship` and the `gh pr merge` command, or a `**Ship opt-in (`stop_after: ship`):**` heading the test keys on) — write the skill text and the test against the same anchor string. +- **Orchestrator drift (medium, inherent):** a skill-level default is instruction-following, not enforced state — an LLM orchestrator could still merge. The grep-asserts guard the instructions, not runtime behavior; UAT scenario "captured session contains no `gh pr merge`" covers the runtime side. +- **CLAUDE.md regeneration:** editing only CLAUDE.md silently loses the wording on next refresh (see §6). +- **Existing conditional in 8e:** current text already has "unless the user asked to leave it open" — the rewrite must not leave this stale clause on the default path where it would read as merge-by-default. + +## Verdict + +**Feasible — recommended.** All load-bearing claims verified in-code: schema accepts `ship` with zero changes, `propose.ts` needs ~6 lines, the Step 3 boundary logic needs no modification (only a clarifier), copies are currently identical, and an established grep-assert + byte-identity test pattern exists to guard regressions. Total surface: 2 skill files, 1 CLI file, 1 generator file, 2 test files. The main design caution is anchoring the grep-assert tests and the ship-conditioned skill text on the same literal marker so the tests are robust rather than prose-matching. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research.md new file mode 100644 index 00000000..ab15fd5c --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/research.md @@ -0,0 +1,45 @@ +# Research: fix-metta-propose-runs-entire-lifecycle-through-finalize + +## Decision: Skill-level PR-open default + `ship` sentinel value + +### Approaches Considered + +1. **Skill-level default + `ship` sentinel** (selected) — The PR-open default lives in the propose skill text (Step 8 stops after `gh pr create` unless `STOP_AFTER = "ship"`); `propose.ts` accepts `ship` as a validation sentinel that short-circuits the `buildOrder` membership check; the skill maps a `--ship` argument token to `--stop-after ship`. No schema change (`stop_after: z.string().optional()` already validates `ship`), no boundary-logic change (`ship` never matches a planning artifact id, so the loop naturally runs to `all_complete`), ~6 lines in `propose.ts`. See `research-skill-level-default.md`. +2. **Persisted default (`stop_after: pr-open` written when flag absent)** — rejected. Fatal flaw: the skill's persisted-stop_after boundary check only matches artifact ids passed to `metta complete`; `pr-open` is not an artifact, so new Step-8 skill logic is needed anyway — the approach's sole advantage evaporates. It also contaminates `/metta-auto` and `/metta-fix-issues` change records (same CLI path), flips the semantic meaning of field-absence across epochs, and contradicts 4–5 existing propose-stop-after requirements (~half-rewrite of the capability). See `research-persisted-default.md`. + +### Rationale + +The selected approach is the minimal-change path that satisfies the recorded user decision exactly: default stop at PR-open, merge only via `--ship`/`stop-after=ship` through the existing propose-stop-after machinery, all existing stop-after values untouched, `/metta-auto` and `/metta-fix-issues` unchanged. All load-bearing claims were verified in-tree: + +- The two SKILL.md copies are byte-identical and sync is already test-enforced (`tests/template-deploy-sync.test.ts`, `tests/skill-discovery-loop.test.ts:71`, `tests/grounding.test.ts:35-36`) — edit both copies identically, no new sync mechanism. +- `src/schemas/change-metadata.ts:116` accepts any string — `stop_after: ship` persists with zero schema changes; absent-flag behavior (no field written) is preserved, keeping the living spec's line-70 requirement intact. +- `src/cli/commands/propose.ts` needs only help-text plus a `ship` short-circuit before the `buildOrder` checks and `ship` added to the valid-value error lists. No CLI `--ship` boolean flag — the skill owns the alias, keeping a single source of truth. +- **CLAUDE.md's workflow section is generated**: wording must land in `src/cli/commands/refresh.ts:131` (Lifecycle-skills bullet) together with the checked-in `CLAUDE.md`, or the next `/metta-refresh` reverts it. `src/delivery/workflow-primer.ts` entry-point bullet makes no merge claim — untouched unless wording demands it. +- The living spec's scenario "skill behaves identically when no stop_after is set … finalize, and merge exactly as it does today" (`spec/specs/propose-stop-after/spec.md:104-107`) mandates the old behavior — the spec delta in this change already MODIFIES that requirement. + +### Test strategy (from `research-docs-and-tests.md`) + +New `tests/skill-propose-ship-gate.test.ts` modeled on `skill-discovery-loop.test.ts` (readFile + `toContain`/`not.toContain`): +1. Split content on the ship-gate heading; assert `gh pr merge` absent before it, present after it. +2. Assert removal of exact strings `Do NOT stop after the last artifact`, `finalize + ship must happen`, and the heading `Critical: You MUST verify, finalize, and ship`. +3. Assert one canonical new-default phrase (chosen at implementation, used verbatim in skill text and test — same literal anchor to avoid brittleness). +4. Assert the `Direct local merge of the change branch into main` prohibition survives. +5. Scope guard: assert `metta-auto` and `metta-fix-issues` templates still contain `gh pr merge` (run-to-merge preserved). Never glob over all skills — auto/fix-issues legitimately keep unconditional merge text. +Plus `tests/cli-propose-stop-after.test.ts` additions: `--stop-after ship` accepted and persisted; unknown-value error lists `ship`. + +### Change surface + +`.claude/skills/metta-propose/SKILL.md` + `src/templates/skills/metta-propose/SKILL.md` (Step 1 `--ship` alias, Step 3 clarifier, Step 8 split, Critical-section reword — byte-identical), `src/cli/commands/propose.ts`, `src/cli/commands/refresh.ts` + `CLAUDE.md`, `tests/skill-propose-ship-gate.test.ts` (new), `tests/cli-propose-stop-after.test.ts`. No changes to schemas, workflow YAMLs, metta-auto, or metta-fix-issues skills. + +### Risks + +- Grep-assert brittleness — mitigated by anchoring skill text and tests on the same literal marker (ship-gate heading / canonical phrase), authored in the same commit. +- CLAUDE.md regeneration clobber — mitigated by changing `refresh.ts` and `CLAUDE.md` together. +- Skill-level default is instruction-level, not runtime-enforced — an orchestrator could still merge; the tests guard the instructions, UAT covers the runtime behavior. +- Stale clause "unless the user asked to leave it open" must not survive on the default path where it reads as merge-by-default. + +### Artifacts Produced + +- [Research: skill-level default](research-skill-level-default.md) +- [Research: persisted default](research-persisted-default.md) +- [Research: docs and tests](research-docs-and-tests.md) diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/review.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/review.md new file mode 100644 index 00000000..ec71921e --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/review.md @@ -0,0 +1,48 @@ +# Review: fix-metta-propose-runs-entire-lifecycle-through-finalize + +## Round 1 (iteration #1) + +| Reviewer | Verdict | +|----------|---------| +| Correctness | PASS | +| Security | PASS_WITH_WARNINGS | +| Quality | PASS_WITH_WARNINGS | + +No critical issues. Convergent findings: + +### Major (fixed in round 1 fix commit) + +1. **Quick-reroute bypasses the PR-open default** (Security #1, Quality #1) — `.claude/skills/metta-propose/SKILL.md` routing pre-step reroutes small descriptions into the metta-quick flow, which still runs `gh pr merge` by default. `/metta-propose ` could therefore still merge unattended, contradicting the new CLAUDE.md wording. Fix: routing pre-step now carries the PR-open default over to the quick reroute — the quick flow's merge steps are skipped unless `--ship` was present. +2. **`--ship` misparse turns prose into merge authorization** (Security #2) — token parsing had no position/context constraint, so a description mentioning `--ship` could trigger run-to-merge. Fix: parsing constrained (not inside quotes / not the subject of the description) + orchestrator must announce "Ship opt-in detected: this run will merge to main after CI passes" at Step 1 so misparses surface immediately. + +### Minor (fixed) + +3. **Step 8.d third-state gap** (all three reviewers) — default clause covered only "no persisted stop_after"; a persisted planning-artifact value reaching Step 8 matched neither branch. Fix: default condition now "anything other than `ship`" — the no-merge default is the exhaustive else-branch of the ship gate. +4. **Stale label** (Quality #2) — Step 3 text said "Step 8 (finalize/merge)"; now "Step 8 (finalize/PR)". + +### Minor (accepted, not fixed) + +5. `ship` as a reserved stop-after value collides with a hypothetical custom workflow artifact named `ship` (Security #4) — accepted; no such workflow exists; documented behavior. +6. Scope-guard tests read template copies only (Correctness #2, Quality #4) — accepted; deployed copies covered transitively by byte-identity tests. +7. Constitution tension: default path still pushes branch + opens PR without a prompt (Security #5) — pre-existing, not worsened; skill text is explicit about it. +8. `--stop-after ship` CLI test exercises standard workflow only (Correctness #3) — accepted; code-inspection confirms workflow independence. + +## Round 2 (iteration #2) — after fix commit `c53dfe94c` + +| Reviewer | Verdict | +|----------|---------| +| Correctness | PASS | +| Security | PASS_WITH_WARNINGS | +| Quality | PASS_WITH_WARNINGS | + +Both round-1 majors confirmed CLOSED: no remaining path from `/metta-propose` to a merge without explicit opt-in on the standard pipeline; `--ship` misparse closed at Step 1 (standalone-token constraint + mandatory announcement). Step 8.d confirmed exhaustive; all 100 targeted tests green; copies byte-identical. + +### Residual warnings (accepted — all fail safe: worst case stops at open PR, never merges) + +1. The reroute clause says "unless `--ship` was present in the original propose invocation" without referencing the Step 1 parse constraints/announcement — a literal prose-mention reading could authorize the quick flow's merge on reroute (Security R2 #1, Quality R2 #1). Mitigation: semantic topic-exclusion errs toward exclusion; failure mode is merge only if an orchestrator takes the loosest reading of a description that both matches quick criteria and mentions `--ship`. +2. `--stop-after ship` parse is not covered by the standalone-token/announcement constraints that `--ship` got (Security R2 #2). +3. Local vs persisted stop_after disagreement: 8.d and 8.e are not exact complements; ambiguity resolves to no-merge (Security R2 #3, Correctness R2 suggestion). +4. metta-quick's own MUST-merge language has no carve-out acknowledging the propose-reroute exception (Security R2 #4) — metta-quick is out of this change's mandated scope. +5. Round-2 additions (announcement string, carry-over clause, exhaustive-else wording) are not yet grep-assert protected (Quality R2 #4). + +Follow-up recommendation: log a separate issue to tighten the reroute/`--stop-after ship` wording and add grep-asserts for the round-2 clauses, and to decide metta-quick's own default. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/spec.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/spec.md new file mode 100644 index 00000000..d0be2c1c --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/spec.md @@ -0,0 +1,190 @@ +# propose-stop-after + +## MODIFIED: Requirement: `metta propose` MUST accept a `--stop-after ` option + +The CLI command registered by `src/cli/commands/propose.ts` MUST add a `--stop-after ` option alongside the existing `--workflow`, `--from-gap`, `--from-idea`, `--from-issue`, `--discovery`, and `--auto/--accept-recommended` options. The option takes a single string value naming either an artifact id from the resolved workflow's `buildOrder` or the special value `ship`. The `--help` description for the option MUST name `ship` alongside the planning-phase artifact ids as a valid value. +When the option is omitted, the CLI's persistence behavior MUST be identical to the pre-change implementation: the change is created and `.metta.yaml` MUST NOT include a `stop_after` field. The stop-at-PR-open default introduced by this change is a **skill-level default** — it MUST NOT be persisted as a `stop_after` value on the change record, and the absent-flag YAML output MUST NOT change. +When the option is supplied, the value MUST be persisted on the change record so that the propose skill orchestrator and downstream tools can read it without re-parsing the original CLI invocation. +(Traces: US-2, US-3; intent proposal items 2–3.) + +### Scenario: option appears in CLI help + +- GIVEN the metta CLI is built +- WHEN the user runs `metta propose --help` +- THEN the help output MUST include a line documenting `--stop-after ` with a one-line description naming planning-phase artifact ids AND `ship` as the valid values + +### Scenario: option is accepted with a valid value + +- GIVEN a clean repository on `main` +- WHEN the user runs `metta propose "" --stop-after tasks --json` +- THEN the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "tasks"` AND a change directory at `spec/changes//` MUST exist with `.metta.yaml` containing `stop_after: tasks` + +### Scenario: option is accepted with the `ship` value and persisted + +- GIVEN a clean repository on `main` +- WHEN the user runs `metta propose "" --stop-after ship --json` +- THEN the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "ship"` AND `.metta.yaml` MUST contain `stop_after: ship` validating against the existing `ChangeMetadataSchema` without schema changes + +### Scenario: option is omitted, no `stop_after` field is persisted + +- GIVEN a clean repository on `main` +- WHEN the user runs `metta propose "" --json` with no `--stop-after` flag +- THEN the JSON output MUST NOT include a `stop_after` field (or MUST set it to `null`) AND `.metta.yaml` MUST NOT include a `stop_after` field — the PR-open default is applied by the skill, not by persisted state + + +## MODIFIED: Requirement: `--stop-after` MUST be validated against the resolved workflow + +The CLI command MUST validate the `--stop-after` value against the loaded workflow's `buildOrder` after the workflow has been loaded but BEFORE the change record is created, with one addition: the special value `ship` MUST be accepted for every workflow without consulting `buildOrder` — it is a lifecycle sentinel meaning "run to merge", not an artifact id. All other validation is unchanged: execution-phase ids (`implementation`, `verification`) MUST be rejected, and any value that is neither `ship` nor a member of `buildOrder` MUST be rejected. Error messages listing valid values MUST include `ship`. +When validation fails, the CLI MUST exit with code 4 (matching the existing propose error contract), MUST print an error message that names the invalid value AND lists the valid values for the resolved workflow, and MUST NOT write any state — no change directory, no `.metta.yaml`, no git branch. +No existing accepted value is removed or renamed by this change; only `ship` is added. +(Traces: US-2, US-3.) + +### Scenario: `ship` is accepted for any workflow + +- GIVEN any resolved workflow (e.g. `standard` or `full`) whose `buildOrder` does not contain a `ship` artifact +- WHEN the user runs `metta propose "" --stop-after ship --json` +- THEN the CLI MUST exit with code 0 AND persist `stop_after: ship` on the change record + +### Scenario: unknown artifact id is still rejected and the valid list names `ship` + +- GIVEN the resolved workflow `standard` whose `buildOrder` does not contain `spex` +- WHEN the user runs `metta propose "" --stop-after spex` +- THEN the CLI MUST exit with code 4 AND the error message MUST cite `spex` as unknown AND MUST list the valid values (`intent, stories, spec, research, design, tasks, ship`) AND `spec/changes/` MUST NOT contain a directory for this change + +### Scenario: execution-phase artifact id is still rejected + +- GIVEN any resolved workflow whose `buildOrder` includes `implementation` +- WHEN the user runs `metta propose "" --stop-after implementation` +- THEN the CLI MUST exit with code 4 AND the error message MUST explain that execution-phase ids are not valid stop points AND `spec/changes/` MUST NOT contain a directory for this change + +### Scenario: existing planning-phase values keep their semantics + +- GIVEN the user passes any previously accepted stop-after value (`intent`, `stories`, `spec`, `research`, `design`, `tasks`, or a non-default-workflow planning id such as `domain-research`) +- WHEN `metta propose` runs with that value +- THEN the value MUST validate and persist exactly as it did before this change, with no change in boundary semantics + + +## MODIFIED: Requirement: propose skill MUST honor the `stop_after` boundary + +`.claude/skills/metta-propose/SKILL.md` MUST retain the existing boundary behavior for planning-phase `stop_after` values, unchanged: the handoff line's `` MUST be `/metta-execute` when `stop_after = tasks`; for earlier stop points (`intent`, `stories`, `spec`, `research`, `design`) the resume command MUST be `/metta-plan` with `/metta-status` mentioned as an inspection alternative; and the orchestrator MUST NOT spawn implementation, review, or verification subagents when a planning-phase boundary has been reached. +This change adds two behaviors: +1. The skill MUST recognize `--ship` in `$ARGUMENTS` as an alias for `--stop-after ship` and forward it to the CLI as `--stop-after ship`. +2. When `stop_after = ship`, the orchestrator MUST run the full pre-change ship path after PR creation: `gh pr checks --watch --fail-fast`, then `gh pr merge --merge`, then post-merge cleanup — i.e. `ship` restores run-to-merge. +The pre-change default (no `stop_after` → proceed through merge) is removed; the no-`stop_after` default path is now governed by the "propose skill default path MUST stop at PR-open" requirement below. +(Traces: US-1, US-2, US-3.) + +### Scenario: skill parses and forwards `--ship` from `$ARGUMENTS` + +- GIVEN a propose skill invocation whose `$ARGUMENTS` is `add cool feature --ship` +- WHEN the orchestrator runs Step 1 (CLI invocation) +- THEN it MUST execute `METTA_SKILL=1 metta propose "add cool feature" --stop-after ship --json` AND the description MUST NOT contain the `--ship` token + +### Scenario: `stop_after: ship` restores run-to-merge + +- GIVEN a change record with `stop_after: ship` and the orchestrator has reached `all_complete: true`, run `metta finalize`, pushed the branch, and created the PR +- WHEN the orchestrator continues past PR creation +- THEN it MUST run `gh pr checks --watch --fail-fast` AND, when all checks pass, `gh pr merge --merge` AND perform post-merge cleanup exactly as the pre-change step 8 did + +### Scenario: planning-phase boundary for `tasks` is unchanged + +- GIVEN a change record with `stop_after: tasks` and the orchestrator has just received `all_complete: false` with `next: ["implementation"]` from `metta complete tasks` +- WHEN the orchestrator inspects the change record +- THEN it MUST stop the workflow AND print `` Stopped after `tasks`. Run `/metta-execute` to begin implementation. `` AND MUST NOT spawn any metta-executor, metta-reviewer, or metta-verifier agent + + +## ADDED: Requirement: propose skill default path MUST stop at PR-open + +When the change record has no `stop_after` field, the propose skill orchestrator MUST run the full pipeline — discovery, planning, implementation, verification, `metta finalize`, `git push` of the change branch, and `gh pr create` — and then MUST stop. The final default-path actions are creating the PR and reporting the PR URL to the user; the report MUST name `/metta-ship` (or an explicit merge) as the way to land the change. On this default path the orchestrator MUST NOT run `gh pr merge`, MUST NOT run `gh pr checks --watch` as a precursor to merging, and MUST NOT perform post-merge cleanup (main pull, branch/worktree removal tied to a merge). +This default is skill-level behavior only: it MUST NOT depend on any persisted `stop_after` value, and no configuration surface is added to alter it. +(Traces: US-1; intent problem statement — "propose" must not autonomously merge to main.) + +### Scenario: default propose run ends at an open PR + +- GIVEN `/metta-propose ` is invoked with no stop-after flag and no `--ship` +- WHEN the change completes verification and `metta finalize` succeeds +- THEN the orchestrator MUST push the branch, run `gh pr create`, report the PR URL, and stop AND the captured session MUST NOT contain a `gh pr merge` invocation + +### Scenario: main does not contain the change after a default run + +- GIVEN a completed default propose run that reported a PR URL +- WHEN the user inspects the repository +- THEN the change branch's PR MUST be open AND `main` MUST NOT contain the change's merge commit + +### Scenario: the user can land the PR without rework + +- GIVEN a default propose run has stopped at PR-open +- WHEN the user runs `/metta-ship` (or merges the PR explicitly) +- THEN the change MUST complete — merge, archive, and cleanup — without re-running planning, implementation, or verification + + +## ADDED: Requirement: both propose SKILL.md copies MUST carry the PR-open default and stay in sync + +Both the installed skill at `.claude/skills/metta-propose/SKILL.md` and the template at `src/templates/skills/metta-propose/SKILL.md` MUST be updated so that: +1. The default-path terminal instruction (currently step 8) ends at `gh pr create` plus a PR-URL report, with `gh pr checks --watch` and `gh pr merge` instructions conditioned on `stop_after = ship`. +2. The section currently titled "Critical: You MUST verify, finalize, and ship" MUST NOT mandate merging on the default path — its mandate MUST cover verification, finalize, push, and PR creation, with merge described as conditional on the explicit ship opt-in. +3. Neither copy contains an unconditional (default-path) `gh pr merge` instruction. +The two copies MUST express identical default-path and ship-path behavior — a change to one MUST be mirrored in the other. +(Traces: US-4; intent proposal item 4.) + +### Scenario: default-path instructions end at PR creation in both copies + +- GIVEN the updated `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` +- WHEN their default-path (no `stop_after`) instructions are read +- THEN the terminal actions MUST be `gh pr create` and reporting the PR URL AND every `gh pr merge` mention MUST be inside a condition requiring `stop_after = ship` AND no section commands an unconditional ship on the default path + +### Scenario: the two copies agree + +- GIVEN both SKILL.md copies after this change +- WHEN their step-8 / ship-path content is compared +- THEN both MUST describe the same default (stop at PR-open) and the same ship opt-in behavior + + +## ADDED: Requirement: grep-assert tests MUST guard the propose skill against unconditional merge + +The test suite MUST include grep-assert tests over BOTH `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` that fail when either file contains an unconditional merge instruction on the default path. At minimum the tests MUST assert: +1. No default-path `gh pr merge` instruction — every occurrence of `gh pr merge` in either file MUST appear only in ship-opt-in-conditioned text (the tests MAY implement this as: the line/step containing `gh pr merge` also references the ship condition). +2. The "must ship" mandate phrasing that commands merging on the default path (e.g. "finalize + ship must happen" as an unconditional instruction) is absent. +The tests MUST pass on the updated files and MUST fail if a future edit reintroduces unconditional auto-merge in either copy. +(Traces: US-4; intent proposal item 6.) + +### Scenario: tests pass on the updated skill files + +- GIVEN the updated SKILL.md copies with merge conditioned on the ship opt-in +- WHEN the grep-assert tests run via `npm test` +- THEN they MUST pass + +### Scenario: tests fail when unconditional merge is reintroduced + +- GIVEN either SKILL.md copy is edited to add an unconditioned default-path `gh pr merge --merge` instruction +- WHEN the grep-assert tests run +- THEN at least one test MUST fail, naming the offending file + + +## ADDED: Requirement: `/metta-auto` and `/metta-fix-issues` MUST retain run-to-merge behavior + +This change MUST NOT alter the lifecycle end state of `/metta-auto` or `/metta-fix-issues`: both skills MUST continue to run through CI watch and `gh pr merge` exactly as before this change. No edit made for the propose PR-open default may touch the merge instructions of `.claude/skills/metta-auto/SKILL.md`, `.claude/skills/metta-fix-issues/SKILL.md`, or their `src/templates/skills/` counterparts. +(Traces: US-3; intent "Unchanged" section.) + +### Scenario: `/metta-auto` still runs to merge + +- GIVEN `/metta-auto ` is invoked and the change passes verification and finalize +- WHEN the lifecycle completes +- THEN the auto skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change + +### Scenario: `/metta-fix-issues` still runs to merge + +- GIVEN `/metta-fix-issues ` is invoked and the fix passes verification and finalize +- WHEN the lifecycle completes +- THEN the fix-issues skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change + + +## ADDED: Requirement: CLAUDE.md workflow wording MUST state the PR-open default + +The `## Metta Workflow` section of `CLAUDE.md` MUST describe `/metta-propose` as running the full pipeline and ending at an **open PR** by default, with merge requiring the explicit ship opt-in (`--ship` / `stop-after=ship`) or a subsequent `/metta-ship`. The wording MUST NOT describe or imply that `/metta-propose` merges to main by default. +(Traces: US-4; intent proposal item 5.) + +### Scenario: workflow section describes the PR-open default + +- GIVEN the updated `CLAUDE.md` +- WHEN a reader checks the `/metta-propose` entries in the Metta Workflow section +- THEN the text MUST state that a default propose run ends at an open PR AND that merging requires the explicit ship opt-in or `/metta-ship` diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/stories.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/stories.md new file mode 100644 index 00000000..cf777ffd --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/stories.md @@ -0,0 +1,52 @@ +# fix-metta-propose-runs-entire-lifecycle-through-finalize — User Stories + +## US-1: Propose stops at an open PR by default + +**As a** developer running `/metta-propose` in an AI orchestrator session +**I want to** the pipeline to run end-to-end through finalize, push, and PR creation, then stop and report the PR URL +**So that** I get a reviewable pull request without metta merging code to main that I never consented to land +**Priority:** P1 +**Independent Test Criteria:** A default `/metta-propose` run (no stop-after flag) ends with `gh pr create` and a reported PR URL, and no `gh pr merge` is executed. + +**Acceptance Criteria:** +- **Given** `/metta-propose ` is invoked with no stop-after flag **When** the change completes verification and finalize **Then** the skill pushes the branch, creates the PR, reports the PR URL, and stops without running `gh pr merge` +- **Given** a completed default propose run **When** the user inspects the repository **Then** the change branch's PR is open and main does not contain the merge +- **Given** a default propose run has stopped at PR-open **When** the user wants to land it **Then** `/metta-ship` (or an explicit merge) completes the change without rework + +## US-2: Explicit ship opt-in restores run-to-merge + +**As a** developer who wants a change landed autonomously +**I want to** pass an explicit `--ship` / `stop-after=ship` opt-in on `/metta-propose` +**So that** I can still get the old run-to-merge behavior when I have deliberately chosen it +**Priority:** P1 +**Independent Test Criteria:** A propose run with the ship opt-in proceeds past PR creation to CI watch and merge, using the existing propose-stop-after machinery. + +**Acceptance Criteria:** +- **Given** `/metta-propose` is invoked with the ship opt-in **When** the PR is created **Then** the skill continues through `gh pr checks --watch` and `gh pr merge` as before +- **Given** the ship opt-in is recorded via the propose-stop-after machinery **When** `.metta.yaml` is inspected **Then** the recorded stop-after value validates against the existing schema and drives the boundary check + +## US-3: Existing stop-after values keep their semantics + +**As a** developer already using `--stop-after` values (`intent`, `tasks`, etc.) +**I want to** all currently accepted stop-after values to keep working unchanged +**So that** the default flip does not break my existing workflows or scripts +**Priority:** P2 +**Independent Test Criteria:** Each previously accepted stop-after value still validates and stops the propose pipeline at the same boundary as before the change. + +**Acceptance Criteria:** +- **Given** `/metta-propose` is invoked with an existing stop-after value such as `tasks` **When** that artifact completes **Then** the pipeline stops at the same boundary it did before this change +- **Given** the `propose-stop-after` spec delta is applied **When** the value set is reviewed **Then** no existing value is removed or renamed, and only the absent-flag default semantics change +- **Given** `/metta-auto` or `/metta-fix-issues` is invoked **When** their lifecycles complete **Then** they still run to merge exactly as before + +## US-4: Instructions and docs cannot silently restore auto-merge + +**As a** metta maintainer +**I want to** both propose SKILL.md copies and the CLAUDE.md workflow wording updated to match the PR-open default, guarded by grep-assert regression tests +**So that** the skill instructions, docs, and behavior stay consistent and a future skill edit cannot quietly reintroduce unconditional merge +**Priority:** P2 +**Independent Test Criteria:** Grep-assert tests over both SKILL.md copies fail if an unconditional merge instruction is present, and pass on the updated files. + +**Acceptance Criteria:** +- **Given** the installed skill (`.claude/skills/metta-propose/SKILL.md`) and the template (`src/templates/skills/metta-propose/SKILL.md`) **When** their default-path instructions are read **Then** the terminal action is `gh pr create` + report, with merge conditional on the explicit ship opt-in and no "must ship" mandate on the default path +- **Given** the grep-assert regression tests **When** an unconditional `gh pr merge` instruction is added back to either SKILL.md copy **Then** the test suite fails +- **Given** the CLAUDE.md workflow section **When** a reader checks `/metta-propose`'s described behavior **Then** it states the run ends at an open PR unless ship is explicitly requested diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/summary.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/summary.md new file mode 100644 index 00000000..142b7bc0 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/summary.md @@ -0,0 +1,36 @@ +# Summary: fix-metta-propose-runs-entire-lifecycle-through-finalize + +## What changed + +`/metta-propose`'s default terminal state is now **PR-open**: the skill runs the full pipeline (discovery → planning → implementation → verification → finalize → push → `gh pr create`) then stops and reports the PR URL. It no longer runs `gh pr merge` by default. Merging is an explicit opt-in via `--ship` (skill alias) or `--stop-after ship`, wired through the existing propose-stop-after machinery. `/metta-auto` and `/metta-fix-issues` keep run-to-merge behavior unchanged. + +## Implementation (per task) + +- **Task 1.1** (`9018ce0ab`) — Both propose SKILL.md copies (`.claude/skills/` + `src/templates/skills/`, byte-identical): Step 1 `--ship` alias parse rule; Step 3 clarifier that `ship` is not a planning boundary; Step 8 restructured — default path ends after `gh pr create` with the handoff `PR open for review: . Run /metta-ship to land it...`; `gh pr checks --watch` / `gh pr merge` / cleanup relocated under the ship-gate marker (`Ship opt-in — the following sub-steps run ONLY when STOP_AFTER = "ship" ...`); Critical section retitled `Critical: verify, finalize, and open the PR`. Forbidden strings (`Do NOT stop after the last artifact`, `finalize + ship must happen`, `unless the user asked to leave it open`, old Critical title) removed. +- **Task 1.2** (`f78616379`) — `src/cli/commands/propose.ts`: `--stop-after` help names `ship`; `ship` short-circuits `buildOrder` validation; both error valid-lists include `ship`. No schema/persistence changes; absent flag still writes no `stop_after` field. +- **Task 1.3** (`8338af2e1`) — `src/cli/commands/refresh.ts` generator bullet + checked-in `CLAUDE.md` lifecycle bullet updated in lockstep: "ends at an open PR — merge via `--ship` or `/metta-ship`". +- **Task 2.1** (`7ee8d6253`) — New `tests/skill-propose-ship-gate.test.ts` (10 tests): split-on-marker placement of merge commands, default/handoff anchors present, forbidden phrases absent, local-merge prohibition and `gh pr create` survive, scope guard that metta-auto and metta-fix-issues templates still contain `gh pr merge`. +- **Task 2.2** (`57111098e`) — `tests/cli-propose-stop-after.test.ts`: `--stop-after ship` accepted and persisted (`stop_after: ship` in `.metta.yaml`), unknown-value error lists `ship`, `--help` names `ship`. + +## Verification + +- `npx tsc --noEmit` — clean. +- `npm test` — 134/134 files, 2709 passed, 2 skipped, 0 failed. +- Targeted suite (ship-gate, cli-propose-stop-after, skill-discovery-loop, grounding, template-deploy-sync, cli-skills) — 110/110 passed. +- Change surface confirmed: only the intended 7 files plus change artifacts; no edits to metta-auto/metta-fix-issues, schemas, workflow YAMLs, or workflow-primer.ts. + +## Notes / deviations + +- Commander wraps help text at 80 columns, so the help test asserts `ship` within the full `--stop-after` option entry (flag line + continuation) rather than one physical line — same intent, robust to wrapping. +- Skill-level default is instruction-level, not runtime-enforced; the grep-assert tests guard the instructions. + +## Verify phase (3 verifiers, iteration #1) + +- **Test suite:** 134/134 files, 2709 passed, 2 skipped, 0 failed. +- **Typecheck + lint:** `npx tsc --noEmit` clean; `npm run lint` clean. +- **Spec coverage:** PASS — all 8 delta requirements verified, 22/23 scenarios COVERED, 1 PARTIAL (non-default-workflow stop-after id untestable due to known full-workflow template issue; validation is generically buildOrder-driven). Mutation test confirmed the ship-gate grep-assert fails when an unconditional `gh pr merge` is reinjected. + +## Review phase + +- Round 1: Correctness PASS, Security PASS_WITH_WARNINGS, Quality PASS_WITH_WARNINGS. Two majors fixed in `c53dfe94c`: quick-reroute now carries the PR-open default over; `--ship` parsing constrained to standalone flag token with a mandatory "Ship opt-in detected" announcement. Step 8.d made the exhaustive no-merge else-branch; stale "finalize/merge" label fixed. +- Round 2: Correctness PASS, Security PASS_WITH_WARNINGS, Quality PASS_WITH_WARNINGS. Both majors confirmed closed; residual warnings all fail safe (worst case stops at open PR) — recorded in review.md with a follow-up recommendation. diff --git a/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/tasks.md b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/tasks.md new file mode 100644 index 00000000..c26d5930 --- /dev/null +++ b/spec/archive/2026-08-21-fix-metta-propose-runs-entire-lifecycle-through-finalize/tasks.md @@ -0,0 +1,105 @@ +# Tasks for fix-metta-propose-runs-entire-lifecycle-through-finalize + +Change root: `/home/utx0/Code/metta/.metta/worktrees/fix-metta-propose-runs-entire-lifecycle-through-finalize` (all paths below are relative to it; run all commands from it). + +Authoritative blueprint: `spec/changes/fix-metta-propose-runs-entire-lifecycle-through-finalize/design.md`. Every string marked *(anchor)* below is load-bearing and shared verbatim between skill text and tests — do NOT paraphrase. + +## Batch 1 (no dependencies) + +- [x] **Task 1.1: Restructure both propose SKILL.md copies (ship gate, --ship alias, Critical reword)** + - **Files**: `.claude/skills/metta-propose/SKILL.md`, `src/templates/skills/metta-propose/SKILL.md` (BOTH copies, byte-identical — same edits applied to each; existing sync tests will fail otherwise) + - **Action**: Read design.md §§1–3 ("Propose SKILL.md — Step 8 restructure", "Critical section reword", "Step 1 `--ship` alias + Step 3 clarifier") in the change root and apply all four edits to BOTH files identically: + 1. **Step 1** — immediately after the existing `--stop-after` parse block (~lines 45–51), insert: + ```markdown + **Parse optional `--ship` from `$ARGUMENTS`:** + + - If `$ARGUMENTS` contains the token `--ship`, remove it from `$ARGUMENTS` and set `STOP_AFTER = "ship"`. `--ship` is an alias for `--stop-after ship` — forward it to the CLI as `--stop-after ship` (there is no CLI `--ship` flag). If both `--ship` and `--stop-after ` are present, `--ship` takes precedence. + - The remaining text is the description. + ``` + Also append one sentence to the existing "**Scope of `STOP_AFTER`:**" bullet: `` The special value `ship` is NOT a planning-phase artifact: it means "run to merge" and is handled by the Step 8 ship opt-in, never by the Step 3 boundary check. `` The command matrix is unchanged. + 2. **Step 3** — add one bullet to the "Stop-after boundary check" list, after the two boundary conditions: + ```markdown + - `ship` is not a planning boundary: when `STOP_AFTER = "ship"` (or persisted `stop_after: ship`), this check never fires for any artifact — do not hunt for a `ship` artifact; continue the loop to `all_complete` and apply the Step 8 ship opt-in. + ``` + Resume-command mapping is unchanged. + 3. **Step 8** — replace the current Step 8 sub-steps a–f in full with the block given in design.md §1 (sub-steps a–c unchanged, new d = default PR-open stop, ship-gate marker paragraph, e–g = relocated checks-watch/merge/cleanup). Load-bearing anchors that must appear exactly: + - Ship-gate marker line *(anchor, exactly once in the file)*: ``**Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record's persisted `stop_after` is `ship`):**`` + - Default phrase *(anchor)*: ``**Default path ends at an open PR. Do NOT merge; report the PR URL and stop.**`` + - Handoff report *(anchor)*: ``PR open for review: . Run `/metta-ship` to land it, or merge the PR on GitHub yourself.`` + - Ship-path commands after the marker: `gh pr checks --watch --fail-fast` and `gh pr merge --merge`. + 4. **Critical section** — replace the section currently titled "Critical: You MUST verify, finalize, and ship" in full with the block in design.md §2, headed *(anchor)* `## Critical: verify, finalize, and open the PR`. Preserve verbatim the three carried-over bullets: the line beginning `Direct local merge of the change branch into main`, the orphaning-recovery bullet, and the silent-write-anomaly bullet. + Forbidden strings that must NOT survive anywhere in either file: `Critical: You MUST verify, finalize, and ship`, `Do NOT stop after the last artifact`, `finalize + ship must happen`, `unless the user asked to leave it open`. Invariant: `gh pr merge` and `gh pr checks` appear nowhere before the ship-gate marker line; `gh pr create` stays on the default path. Do NOT touch metta-auto or metta-fix-issues skill files. + - **Verify**: `diff .claude/skills/metta-propose/SKILL.md src/templates/skills/metta-propose/SKILL.md` is empty; `grep -c 'Ship opt-in — the following' .claude/skills/metta-propose/SKILL.md` returns 1; `grep -n 'unless the user asked to leave it open\|Do NOT stop after the last artifact\|finalize + ship must happen\|Critical: You MUST verify, finalize, and ship' .claude/skills/metta-propose/SKILL.md` returns nothing; `npx vitest run tests/skill-discovery-loop.test.ts tests/grounding.test.ts tests/template-deploy-sync.test.ts` passes. + - **Done**: Both copies byte-identical, all four anchors present exactly as specified, forbidden strings absent, existing sync/grounding tests green. + +- [x] **Task 1.2: `propose.ts` — accept `ship` as a stop-after sentinel** + - **Files**: `src/cli/commands/propose.ts` + - **Action**: Apply the three localized edits from design.md §4: + 1. Replace the `--stop-after` option help description (~lines 17–20) with: `'Stop after the named planning artifact (e.g. intent, stories, spec, research, design, tasks), or ship to run through merge'` + 2. Ship short-circuit (~line 39): wrap the existing validation so `ship` bypasses all `buildOrder` checks: + ```ts + if (stopAfter !== undefined && stopAfter !== 'ship') { + // existing planningIds / execution-phase / membership checks, unchanged + } + ``` + 3. Valid-value list (~line 43): `const validList = planningIds.join(', ') + ', ship'` so both error messages (execution-phase rejection and unknown-id rejection) list `ship`. + No schema changes, no new flags, no persistence changes — `stopAfter === 'ship'` rides the existing `createChange(...)` and JSON-output paths. Exit-code-4 contract and no-state-on-error behavior untouched; absent flag still writes no `stop_after` field. + - **Verify**: `npx tsc --noEmit` passes; `npx vitest run tests/cli-propose-stop-after.test.ts` passes (existing tests only at this point). + - **Done**: `--stop-after ship` validates for any workflow and persists `stop_after: ship`; unknown values still exit 4 with `ship` in the valid list; help text names `ship`. + +- [x] **Task 1.3: `refresh.ts` + checked-in `CLAUDE.md` propose bullet (atomic pair)** + - **Files**: `src/cli/commands/refresh.ts`, `CLAUDE.md` (repo root of the change root — both in ONE task; landing one without the other leaves generator and doc out of sync) + - **Action**: Per design.md §5: + 1. In `refresh.ts` (~line 131, inside `buildWorkflowSection()`), replace the `/metta-propose` lifecycle bullet push with *(anchor, generated verbatim)*: + ```ts + lines.push('- `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship`') + ``` + 2. In the checked-in `CLAUDE.md`, under `### Lifecycle skills`, replace the matching `/metta-propose` bullet with the identical rendered line: + ```markdown + - `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship` + ``` + Do NOT change `src/delivery/workflow-primer.ts` — its propose bullet makes no merge claim and already complies. + - **Verify**: `npx tsc --noEmit` passes; `grep -F -- 'ends at an open PR — merge via `--ship` or `/metta-ship`' CLAUDE.md src/cli/commands/refresh.ts` matches in both files. + - **Done**: Generator and checked-in doc carry the identical PR-open-default wording; a future `/metta-refresh` regenerates the same line. + +## Batch 2 (depends on Batch 1) + +- [x] **Task 2.1: New grep-assert test `tests/skill-propose-ship-gate.test.ts`** + - **Depends on**: Task 1.1 (anchors must exist in the skill files) + - **Files**: `tests/skill-propose-ship-gate.test.ts` (new) + - **Action**: Create the test per design.md §6, modeled on `tests/skill-discovery-loop.test.ts` (readFile + `toContain`/`not.toContain`, same path-constant pattern). Define constants verbatim: + ```ts + const SHIP_GATE_MARKER = + '**Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record\'s persisted `stop_after` is `ship`):**' + const DEFAULT_PHRASE = '**Default path ends at an open PR. Do NOT merge; report the PR URL and stop.**' + const HANDOFF_PHRASE = 'PR open for review: . Run `/metta-ship` to land it' + ``` + For EACH of `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` (use `describe.each` or a loop so failures name the offending file): + 1. `const parts = contents.split(SHIP_GATE_MARKER)`; `expect(parts).toHaveLength(2)`; `expect(parts[0]).not.toContain('gh pr merge')`; `expect(parts[0]).not.toContain('gh pr checks')`; `expect(parts[1]).toContain('gh pr checks --watch --fail-fast')`; `expect(parts[1]).toContain('gh pr merge --merge')`. + 2. `expect(contents).toContain(DEFAULT_PHRASE)`; `expect(contents).toContain(HANDOFF_PHRASE)`. + 3. Forbidden absences (verbatim): `Critical: You MUST verify, finalize, and ship`, `Do NOT stop after the last artifact`, `finalize + ship must happen`, `unless the user asked to leave it open`. + 4. Survivals: `toContain('Direct local merge of the change branch into main')`, `toContain('gh pr create')`. + 5. Scope guard (do NOT glob over `skills/`): read `src/templates/skills/metta-auto/SKILL.md` and `src/templates/skills/metta-fix-issues/SKILL.md`; assert each still `toContain('gh pr merge')`. Assert nothing else about those two files. + Do not duplicate byte-identity checks — existing tests cover sync. + - **Verify**: `npx vitest run tests/skill-propose-ship-gate.test.ts` passes; `npx tsc --noEmit` passes. + - **Done**: New test file passes against the updated skill files and would fail if unconditional merge text reappears in either propose copy. + +- [x] **Task 2.2: Extend `tests/cli-propose-stop-after.test.ts` for `ship`** + - **Depends on**: Task 1.2 (CLI must accept `ship`) + - **Files**: `tests/cli-propose-stop-after.test.ts` + - **Action**: Per design.md §6, using the existing `runCli` harness and fixture setup in the file, add: + 1. Test `persists stop_after: ship`: run `['--json', 'propose', 'demo ship stop', '--stop-after', 'ship']` → expect `code === 0`, JSON `data.stop_after === 'ship'`, and the change's `.metta.yaml` contains `stop_after: ship`. + 2. Extend the existing `rejects unknown --stop-after value` test with `expect(text).toContain('ship')` — the valid-value list now names `ship`. + 3. Test `--help names ship`: `runCli(['propose', '--help'], tempDir)` → stdout contains `ship` on the `--stop-after` option line. + Do not modify existing assertions except the one extension in item 2. + - **Verify**: `npx vitest run tests/cli-propose-stop-after.test.ts` passes; `npx tsc --noEmit` passes. + - **Done**: All three new/extended cases pass; existing cases in the file remain green. + +## Batch 3 (depends on Batch 2) + +- [x] **Task 3.1: Full verification sweep** + - **Depends on**: Tasks 2.1, 2.2 (and transitively all of Batch 1) + - **Files**: none (read-only verification; fix-forward only if a failure traces to this change's edits) + - **Action**: From the change root run, in order: `npx tsc --noEmit`, then `npm test` (full suite), then explicitly `npx vitest run tests/skill-propose-ship-gate.test.ts tests/cli-propose-stop-after.test.ts tests/skill-discovery-loop.test.ts tests/grounding.test.ts tests/template-deploy-sync.test.ts tests/cli-skills.test.ts`. If any failure is caused by this change (e.g. skill-copy drift, a paraphrased anchor, a missed forbidden string), fix the offending file from Batch 1/2 per its task spec and re-run. Confirm `git -C status` shows only the intended files modified/added: the two SKILL.md copies, `src/cli/commands/propose.ts`, `src/cli/commands/refresh.ts`, `CLAUDE.md`, `tests/skill-propose-ship-gate.test.ts`, `tests/cli-propose-stop-after.test.ts` (plus change artifacts under `spec/changes/`). Notably: no edits to metta-auto or metta-fix-issues skills, no schema files, no workflow YAMLs, no `src/delivery/workflow-primer.ts`. + - **Verify**: `npm test` exits 0; `npx tsc --noEmit` exits 0. + - **Done**: Whole suite green, typecheck clean, change surface matches design.md ("Total surface: 2 SKILL.md copies, propose.ts, refresh.ts + checked-in CLAUDE.md, 1 new test file, additions to 1 existing test file"). diff --git a/spec/specs/propose-stop-after/spec.lock b/spec/specs/propose-stop-after/spec.lock index 53837165..c86b7725 100644 --- a/spec/specs/propose-stop-after/spec.lock +++ b/spec/specs/propose-stop-after/spec.lock @@ -1,21 +1,23 @@ -version: 8 -hash: sha256:92d49d1a1d23 -updated: 2026-04-28T13:49:11.391Z +version: 9 +hash: sha256:51523ab25038 +updated: 2026-08-21T23:56:29.187Z status: draft source: change requirements: - id: metta-propose-must-accept-a-stop-after-artifact-option - hash: sha256:8bf7fa7126b9 + hash: sha256:e2f01d329438 scenarios: - option-appears-in-cli-help - option-is-accepted-with-a-valid-value - - option-is-omitted-full-lifecycle-behavior-preserved + - option-is-accepted-with-the-ship-value-and-persisted + - option-is-omitted-no-stop-after-field-is-persisted - id: stop-after-must-be-validated-against-the-resolved-workflow - hash: sha256:3aba4088d573 + hash: sha256:6ea2e72d2019 scenarios: - - unknown-artifact-id-is-rejected-before-any-side-effects - - execution-phase-artifact-id-is-rejected - - planning-phase-id-from-a-non-default-workflow-is-accepted + - ship-is-accepted-for-any-workflow + - unknown-artifact-id-is-still-rejected-and-the-valid-list-names-ship + - execution-phase-artifact-id-is-still-rejected + - existing-planning-phase-values-keep-their-semantics - id: change-record-schema-must-persist-stop-after-as-an-optional-field hash: sha256:5735d97e6fd3 scenarios: @@ -28,12 +30,11 @@ requirements: - createchange-writes-stop-after-when-supplied - createchange-omits-stop-after-when-not-supplied - id: propose-skill-must-honor-the-stop-after-boundary - hash: sha256:ac0af7bd3b51 + hash: sha256:93db5c29f29d scenarios: - - skill-parses-and-forwards-stop-after-from-arguments - - skill-exits-cleanly-at-the-stop-after-boundary-for-tasks - - skill-exits-cleanly-at-the-stop-after-boundary-for-spec - - skill-behaves-identically-when-no-stop-after-is-set + - skill-parses-and-forwards-ship-from-arguments + - stop-after-ship-restores-run-to-merge + - planning-phase-boundary-for-tasks-is-unchanged - id: metta-status-must-surface-stop-after-in-json-output hash: sha256:d0a577317ed5 scenarios: @@ -49,3 +50,28 @@ requirements: scenarios: - tests-can-assert-the-handoff-line-shape - no-implementation-implying-lines-appear + - id: propose-skill-default-path-must-stop-at-pr-open + hash: sha256:fb5e8c10ed40 + scenarios: + - default-propose-run-ends-at-an-open-pr + - main-does-not-contain-the-change-after-a-default-run + - the-user-can-land-the-pr-without-rework + - id: both-propose-skill-md-copies-must-carry-the-pr-open-default-and-stay-in-sync + hash: sha256:b3248dfbf7b3 + scenarios: + - default-path-instructions-end-at-pr-creation-in-both-copies + - the-two-copies-agree + - id: grep-assert-tests-must-guard-the-propose-skill-against-unconditional-merge + hash: sha256:9c1e81d6a5e6 + scenarios: + - tests-pass-on-the-updated-skill-files + - tests-fail-when-unconditional-merge-is-reintroduced + - id: metta-auto-and-metta-fix-issues-must-retain-run-to-merge-behavior + hash: sha256:170a0002bd65 + scenarios: + - metta-auto-still-runs-to-merge + - metta-fix-issues-still-runs-to-merge + - id: claude-md-workflow-wording-must-state-the-pr-open-default + hash: sha256:839a230fce18 + scenarios: + - workflow-section-describes-the-pr-open-default diff --git a/spec/specs/propose-stop-after/spec.md b/spec/specs/propose-stop-after/spec.md index 4cd1d2a2..28d41784 100644 --- a/spec/specs/propose-stop-after/spec.md +++ b/spec/specs/propose-stop-after/spec.md @@ -2,46 +2,57 @@ ## Requirement: `metta propose` MUST accept a `--stop-after ` option -The CLI command registered by `src/cli/commands/propose.ts` MUST add a `--stop-after ` option alongside the existing `--workflow`, `--from-gap`, `--from-idea`, `--from-issue`, `--discovery`, and `--auto/--accept-recommended` options. The option takes a single string value naming an artifact id from the resolved workflow's `buildOrder`. -When the option is omitted, behavior MUST be identical to the pre-change implementation — the change is created and the orchestrator proceeds through the full lifecycle. +The CLI command registered by `src/cli/commands/propose.ts` MUST add a `--stop-after ` option alongside the existing `--workflow`, `--from-gap`, `--from-idea`, `--from-issue`, `--discovery`, and `--auto/--accept-recommended` options. The option takes a single string value naming either an artifact id from the resolved workflow's `buildOrder` or the special value `ship`. The `--help` description for the option MUST name `ship` alongside the planning-phase artifact ids as a valid value. +When the option is omitted, the CLI's persistence behavior MUST be identical to the pre-change implementation: the change is created and `.metta.yaml` MUST NOT include a `stop_after` field. The stop-at-PR-open default introduced by this change is a skill-level default — it MUST NOT be persisted as a `stop_after` value on the change record, and the absent-flag YAML output MUST NOT change. When the option is supplied, the value MUST be persisted on the change record so that the propose skill orchestrator and downstream tools can read it without re-parsing the original CLI invocation. +(Traces: US-2, US-3; intent proposal items 2–3.) ### Scenario: option appears in CLI help - GIVEN the metta CLI is built - WHEN the user runs `metta propose --help` -- THEN the help output MUST include a line documenting `--stop-after ` with a one-line description naming planning-phase artifact ids as the valid values +- THEN the help output MUST include a line documenting `--stop-after ` with a one-line description naming planning-phase artifact ids AND `ship` as the valid values ### Scenario: option is accepted with a valid value - GIVEN a clean repository on `main` - WHEN the user runs `metta propose "" --stop-after tasks --json` - THEN the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "tasks"` AND a change directory at `spec/changes//` MUST exist with `.metta.yaml` containing `stop_after: tasks` -### Scenario: option is omitted, full-lifecycle behavior preserved +### Scenario: option is accepted with the `ship` value and persisted - GIVEN a clean repository on `main` -- WHEN the user runs `metta propose "" --json` with no `--stop-after` flag -- THEN the JSON output MUST NOT include a `stop_after` field (or MUST set it to `null`) AND `.metta.yaml` MUST NOT include a `stop_after` field +- WHEN the user runs `metta propose "" --stop-after ship --json` +- THEN the command MUST exit with code 0 AND the JSON output MUST include `"stop_after": "ship"` AND `.metta.yaml` MUST contain `stop_after: ship` validating against the existing `ChangeMetadataSchema` without schema changes +### Scenario: option is omitted, no `stop_after` field is persisted +- GIVEN a clean repository on `main` +- WHEN the user runs `metta propose "" --json` with no `--stop-after` flag +- THEN the JSON output MUST NOT include a `stop_after` field (or MUST set it to `null`) AND `.metta.yaml` MUST NOT include a `stop_after` field — the PR-open default is applied by the skill, not by persisted state ## Requirement: `--stop-after` MUST be validated against the resolved workflow -The CLI command MUST validate the `--stop-after` value against the loaded workflow's `buildOrder` after the workflow has been loaded but BEFORE the change record is created. The validation MUST reject: -When validation fails, the CLI MUST exit with code 4 (matching the existing propose error contract), MUST print an error message that names the invalid value AND lists the valid artifact ids for the resolved workflow (excluding the forbidden execution-phase ids), and MUST NOT write any state — no change directory, no `.metta.yaml`, no git branch. +The CLI command MUST validate the `--stop-after` value against the loaded workflow's `buildOrder` after the workflow has been loaded but BEFORE the change record is created, with one addition: the special value `ship` MUST be accepted for every workflow without consulting `buildOrder` — it is a lifecycle sentinel meaning "run to merge", not an artifact id. All other validation is unchanged: execution-phase ids (`implementation`, `verification`) MUST be rejected, and any value that is neither `ship` nor a member of `buildOrder` MUST be rejected. Error messages listing valid values MUST include `ship`. +When validation fails, the CLI MUST exit with code 4 (matching the existing propose error contract), MUST print an error message that names the invalid value AND lists the valid values for the resolved workflow, and MUST NOT write any state — no change directory, no `.metta.yaml`, no git branch. +No existing accepted value is removed or renamed by this change; only `ship` is added. +(Traces: US-2, US-3.) -### Scenario: unknown artifact id is rejected before any side effects +### Scenario: `ship` is accepted for any workflow +- GIVEN any resolved workflow (e.g. `standard` or `full`) whose `buildOrder` does not contain a `ship` artifact +- WHEN the user runs `metta propose "" --stop-after ship --json` +- THEN the CLI MUST exit with code 0 AND persist `stop_after: ship` on the change record + +### Scenario: unknown artifact id is still rejected and the valid list names `ship` - GIVEN the resolved workflow `standard` whose `buildOrder` does not contain `spex` - WHEN the user runs `metta propose "" --stop-after spex` -- THEN the CLI MUST exit with code 4 AND the error message MUST cite `spex` as unknown AND MUST list the valid ids (`intent, stories, spec, research, design, tasks`) AND `spec/changes/` MUST NOT contain a directory for this change +- THEN the CLI MUST exit with code 4 AND the error message MUST cite `spex` as unknown AND MUST list the valid values (`intent, stories, spec, research, design, tasks, ship`) AND `spec/changes/` MUST NOT contain a directory for this change -### Scenario: execution-phase artifact id is rejected +### Scenario: execution-phase artifact id is still rejected - GIVEN any resolved workflow whose `buildOrder` includes `implementation` - WHEN the user runs `metta propose "" --stop-after implementation` - THEN the CLI MUST exit with code 4 AND the error message MUST explain that execution-phase ids are not valid stop points AND `spec/changes/` MUST NOT contain a directory for this change -### Scenario: planning-phase id from a non-default workflow is accepted -- GIVEN the user passes `--workflow full` and the `full` workflow includes `domain-research` in its `buildOrder` -- WHEN the user runs `metta propose "" --workflow full --stop-after domain-research` -- THEN the CLI MUST accept the value and persist `stop_after: domain-research` on the change record - +### Scenario: existing planning-phase values keep their semantics +- GIVEN the user passes any previously accepted stop-after value (`intent`, `stories`, `spec`, `research`, `design`, `tasks`, or a non-default-workflow planning id such as `domain-research`) +- WHEN `metta propose` runs with that value +- THEN the value MUST validate and persist exactly as it did before this change, with no change in boundary semantics ## Requirement: change-record schema MUST persist `stop_after` as an optional field @@ -82,30 +93,23 @@ When `stopAfter` is not supplied, the constructed `ChangeMetadata` MUST NOT incl ## Requirement: propose skill MUST honor the `stop_after` boundary -`.claude/skills/metta-propose/SKILL.md` MUST be updated so the orchestrator: -The handoff line's `` MUST be `/metta-execute` when `stop_after = tasks`. For earlier stop points (`intent`, `stories`, `spec`, `research`, `design`), the resume command MUST be `/metta-plan` (to continue planning) with `/metta-status` mentioned as an inspection alternative. -The orchestrator MUST NOT spawn implementation, review, or verification subagents when the stop-after boundary has been reached. +`.claude/skills/metta-propose/SKILL.md` MUST retain the existing boundary behavior for planning-phase `stop_after` values, unchanged: the handoff line's `` MUST be `/metta-execute` when `stop_after = tasks`; for earlier stop points (`intent`, `stories`, `spec`, `research`, `design`) the resume command MUST be `/metta-plan` with `/metta-status` mentioned as an inspection alternative; and the orchestrator MUST NOT spawn implementation, review, or verification subagents when a planning-phase boundary has been reached. +This change adds two behaviors: -### Scenario: skill parses and forwards `--stop-after` from `$ARGUMENTS` -- GIVEN a propose skill invocation whose `$ARGUMENTS` is `add cool feature --stop-after tasks` +### Scenario: skill parses and forwards `--ship` from `$ARGUMENTS` +- GIVEN a propose skill invocation whose `$ARGUMENTS` is `add cool feature --ship` - WHEN the orchestrator runs Step 1 (CLI invocation) -- THEN it MUST execute `METTA_SKILL=1 metta propose "add cool feature" --stop-after tasks --json` AND the description MUST NOT contain the `--stop-after tasks` tokens +- THEN it MUST execute `METTA_SKILL=1 metta propose "add cool feature" --stop-after ship --json` AND the description MUST NOT contain the `--ship` token -### Scenario: skill exits cleanly at the stop-after boundary for `tasks` -- GIVEN a change record with `stop_after: tasks` and the orchestrator has just received `all_complete: false` with `next: ["implementation"]` from `metta complete tasks` -- WHEN the orchestrator inspects the change record -- THEN it MUST stop the workflow AND print `Stopped after \`tasks`. Run `/metta-execute` to begin implementation.` AND MUST NOT spawn any metta-executor, metta-reviewer, or metta-verifier agent +### Scenario: `stop_after: ship` restores run-to-merge +- GIVEN a change record with `stop_after: ship` and the orchestrator has reached `all_complete: true`, run `metta finalize`, pushed the branch, and created the PR +- WHEN the orchestrator continues past PR creation +- THEN it MUST run `gh pr checks --watch --fail-fast` AND, when all checks pass, `gh pr merge --merge` AND perform post-merge cleanup exactly as the pre-change step 8 did -### Scenario: skill exits cleanly at the stop-after boundary for `spec` -- GIVEN a change record with `stop_after: spec` and the orchestrator has just completed `metta complete spec` +### Scenario: planning-phase boundary for `tasks` is unchanged +- GIVEN a change record with `stop_after: tasks` and the orchestrator has just received `all_complete: false` with `next: ["implementation"]` from `metta complete tasks` - WHEN the orchestrator inspects the change record -- THEN it MUST stop the workflow AND print a handoff line naming `/metta-plan` (to continue planning) or `/metta-status` (to inspect) - -### Scenario: skill behaves identically when no `stop_after` is set -- GIVEN a change record with no `stop_after` field -- WHEN the orchestrator runs Step 3 through Step 8 -- THEN the orchestrator MUST proceed through implementation, review, verification, finalize, and merge exactly as it does today - +- THEN it MUST stop the workflow AND print `Stopped after `tasks`. Run `/metta-execute` to begin implementation.` AND MUST NOT spawn any metta-executor, metta-reviewer, or metta-verifier agent ## Requirement: `metta status` MUST surface `stop_after` in JSON output @@ -153,3 +157,82 @@ The orchestrator MUST NOT prepend or append additional lines that imply implemen - GIVEN any propose run with a `stop_after` value - WHEN the orchestrator exits at the boundary - THEN the captured stdout MUST NOT contain the substrings "metta complete implementation", "metta-executor", "metta-reviewer", or "metta-verifier" emitted by the orchestrator after the stop point + + +## Requirement: propose skill default path MUST stop at PR-open + +When the change record has no `stop_after` field, the propose skill orchestrator MUST run the full pipeline — discovery, planning, implementation, verification, `metta finalize`, `git push` of the change branch, and `gh pr create` — and then MUST stop. The final default-path actions are creating the PR and reporting the PR URL to the user; the report MUST name `/metta-ship` (or an explicit merge) as the way to land the change. On this default path the orchestrator MUST NOT run `gh pr merge`, MUST NOT run `gh pr checks --watch` as a precursor to merging, and MUST NOT perform post-merge cleanup (main pull, branch/worktree removal tied to a merge). +This default is skill-level behavior only: it MUST NOT depend on any persisted `stop_after` value, and no configuration surface is added to alter it. +(Traces: US-1; intent problem statement — "propose" must not autonomously merge to main.) + +### Scenario: default propose run ends at an open PR +- GIVEN `/metta-propose ` is invoked with no stop-after flag and no `--ship` +- WHEN the change completes verification and `metta finalize` succeeds +- THEN the orchestrator MUST push the branch, run `gh pr create`, report the PR URL, and stop AND the captured session MUST NOT contain a `gh pr merge` invocation + +### Scenario: main does not contain the change after a default run +- GIVEN a completed default propose run that reported a PR URL +- WHEN the user inspects the repository +- THEN the change branch's PR MUST be open AND `main` MUST NOT contain the change's merge commit + +### Scenario: the user can land the PR without rework +- GIVEN a default propose run has stopped at PR-open +- WHEN the user runs `/metta-ship` (or merges the PR explicitly) +- THEN the change MUST complete — merge, archive, and cleanup — without re-running planning, implementation, or verification + + +## Requirement: both propose SKILL.md copies MUST carry the PR-open default and stay in sync + +Both the installed skill at `.claude/skills/metta-propose/SKILL.md` and the template at `src/templates/skills/metta-propose/SKILL.md` MUST be updated so that: + +### Scenario: default-path instructions end at PR creation in both copies +- GIVEN the updated `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` +- WHEN their default-path (no `stop_after`) instructions are read +- THEN the terminal actions MUST be `gh pr create` and reporting the PR URL AND every `gh pr merge` mention MUST be inside a condition requiring `stop_after = ship` AND no section commands an unconditional ship on the default path + +### Scenario: the two copies agree +- GIVEN both SKILL.md copies after this change +- WHEN their step-8 / ship-path content is compared +- THEN both MUST describe the same default (stop at PR-open) and the same ship opt-in behavior + + +## Requirement: grep-assert tests MUST guard the propose skill against unconditional merge + +The test suite MUST include grep-assert tests over BOTH `.claude/skills/metta-propose/SKILL.md` and `src/templates/skills/metta-propose/SKILL.md` that fail when either file contains an unconditional merge instruction on the default path. At minimum the tests MUST assert: + +### Scenario: tests pass on the updated skill files +- GIVEN the updated SKILL.md copies with merge conditioned on the ship opt-in +- WHEN the grep-assert tests run via `npm test` +- THEN they MUST pass + +### Scenario: tests fail when unconditional merge is reintroduced +- GIVEN either SKILL.md copy is edited to add an unconditioned default-path `gh pr merge --merge` instruction +- WHEN the grep-assert tests run +- THEN at least one test MUST fail, naming the offending file + + +## Requirement: `/metta-auto` and `/metta-fix-issues` MUST retain run-to-merge behavior + +This change MUST NOT alter the lifecycle end state of `/metta-auto` or `/metta-fix-issues`: both skills MUST continue to run through CI watch and `gh pr merge` exactly as before this change. No edit made for the propose PR-open default may touch the merge instructions of `.claude/skills/metta-auto/SKILL.md`, `.claude/skills/metta-fix-issues/SKILL.md`, or their `src/templates/skills/` counterparts. +(Traces: US-3; intent "Unchanged" section.) + +### Scenario: `/metta-auto` still runs to merge +- GIVEN `/metta-auto ` is invoked and the change passes verification and finalize +- WHEN the lifecycle completes +- THEN the auto skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change + +### Scenario: `/metta-fix-issues` still runs to merge +- GIVEN `/metta-fix-issues ` is invoked and the fix passes verification and finalize +- WHEN the lifecycle completes +- THEN the fix-issues skill MUST still push, create the PR, watch CI, and merge exactly as it did before this change + + +## Requirement: CLAUDE.md workflow wording MUST state the PR-open default + +The `## Metta Workflow` section of `CLAUDE.md` MUST describe `/metta-propose` as running the full pipeline and ending at an open PR by default, with merge requiring the explicit ship opt-in (`--ship` / `stop-after=ship`) or a subsequent `/metta-ship`. The wording MUST NOT describe or imply that `/metta-propose` merges to main by default. +(Traces: US-4; intent proposal item 5.) + +### Scenario: workflow section describes the PR-open default +- GIVEN the updated `CLAUDE.md` +- WHEN a reader checks the `/metta-propose` entries in the Metta Workflow section +- THEN the text MUST state that a default propose run ends at an open PR AND that merging requires the explicit ship opt-in or `/metta-ship` diff --git a/src/cli/commands/propose.ts b/src/cli/commands/propose.ts index 809e784d..a1191e4d 100644 --- a/src/cli/commands/propose.ts +++ b/src/cli/commands/propose.ts @@ -16,7 +16,7 @@ export function registerProposeCommand(program: Command): void { .option('--auto, --accept-recommended', 'auto-accept adaptive routing recommendations') .option( '--stop-after ', - 'Stop after the named planning artifact (e.g. intent, stories, spec, research, design, tasks)', + 'Stop after the named planning artifact (e.g. intent, stories, spec, research, design, tasks), or ship to run through merge', ) .action(async (description, options, command) => { const json = program.opts().json @@ -36,11 +36,13 @@ export function registerProposeCommand(program: Command): void { // Validate --stop-after against the resolved workflow's buildOrder. // Reject execution-phase ids and unknown ids BEFORE creating any change state. const stopAfter: string | undefined = options.stopAfter - if (stopAfter !== undefined) { + // The special value 'ship' is a lifecycle sentinel (run through merge), + // valid for every workflow — it bypasses buildOrder validation entirely. + if (stopAfter !== undefined && stopAfter !== 'ship') { const planningIds = graph.buildOrder.filter( id => id !== 'implementation' && id !== 'verification', ) - const validList = planningIds.join(', ') + const validList = planningIds.join(', ') + ', ship' if (stopAfter === 'implementation' || stopAfter === 'verification') { throw new Error( `--stop-after value '${stopAfter}' is an execution-phase artifact and is not a valid stop point. Valid values are: ${validList}.`, diff --git a/src/cli/commands/refresh.ts b/src/cli/commands/refresh.ts index 461aa69a..b9b3ac3b 100644 --- a/src/cli/commands/refresh.ts +++ b/src/cli/commands/refresh.ts @@ -128,7 +128,7 @@ export function buildWorkflowSection(): string { lines.push('') lines.push('### Lifecycle skills') - lines.push('- `/metta-propose ` — start a new change (standard workflow)') + lines.push('- `/metta-propose ` — start a new change (standard workflow); ends at an open PR — merge via `--ship` or `/metta-ship`') lines.push('- `/metta-quick ` — quick mode, skip planning') lines.push('- `/metta-auto ` — full lifecycle loop (discover → build → verify → ship)') lines.push('- `/metta-plan` — build planning artifacts for the active change') diff --git a/src/templates/skills/metta-propose/SKILL.md b/src/templates/skills/metta-propose/SKILL.md index da50d4fe..c33d928a 100644 --- a/src/templates/skills/metta-propose/SKILL.md +++ b/src/templates/skills/metta-propose/SKILL.md @@ -22,7 +22,7 @@ Before parsing flags or creating any change state, YOU (the orchestrator) MUST c Routing decision: -- **Description matches the criteria AND the caller did NOT pass an explicit `--workflow` flag:** do NOT proceed to Step 1 or the standard proposal pipeline. Run `metta quick` instead — follow the metta-quick skill flow for the same description, then stop; none of the numbered steps below run. +- **Description matches the criteria AND the caller did NOT pass an explicit `--workflow` flag:** do NOT proceed to Step 1 or the standard proposal pipeline. Run `metta quick` instead — follow the metta-quick skill flow for the same description, then stop; none of the numbered steps below run. When rerouting, the PR-open default carries over: the quick flow's merge steps MUST be skipped and the run MUST stop at the open PR (reporting the PR URL) unless `--ship` was present in the original propose invocation. - **Caller passed an explicit `--workflow` flag (any value):** defer to that choice without overriding it — skip this routing decision and proceed to Step 1, passing the flag through as written. - **Description does not match the criteria and no flag was passed:** proceed to Step 1 normally. @@ -48,7 +48,14 @@ Routing decision: - Otherwise, `STOP_AFTER = ""` (empty string). - The remaining text is the description. - Valid artifact ids are owned by the CLI and the resolved workflow's `buildOrder`; do NOT validate the value here — pass through and let `metta propose` reject unknown ids and execution-phase ids (`implementation`, `verification`) with a clear error before any change state is written. - - **Scope of `STOP_AFTER`:** when non-empty, this names a planning-phase artifact (e.g. `intent`, `stories`, `spec`, `research`, `design`, `tasks` for the standard workflow). The orchestrator MUST honor this boundary in Step 3 — see "Stop-after boundary check" there. + - **Scope of `STOP_AFTER`:** when non-empty, this names a planning-phase artifact (e.g. `intent`, `stories`, `spec`, `research`, `design`, `tasks` for the standard workflow). The orchestrator MUST honor this boundary in Step 3 — see "Stop-after boundary check" there. The special value `ship` is NOT a planning-phase artifact: it means "run to merge" and is handled by the Step 8 ship opt-in, never by the Step 3 boundary check. + + **Parse optional `--ship` from `$ARGUMENTS`:** + + - If `$ARGUMENTS` contains the token `--ship`, remove it from `$ARGUMENTS` and set `STOP_AFTER = "ship"`. `--ship` is an alias for `--stop-after ship` — forward it to the CLI as `--stop-after ship` (there is no CLI `--ship` flag). If both `--ship` and `--stop-after ` are present, `--ship` takes precedence. + - Treat `--ship` as the ship opt-in ONLY when it appears as a standalone flag token in leading or trailing position — NOT when it appears inside quotes or as the subject/topic of the description text (e.g. a description *about* a ship flag or shipping behavior). + - When the ship opt-in IS detected, the orchestrator MUST announce before proceeding: `Ship opt-in detected: this run will merge to main after CI passes.` — so a misparse is visible at Step 1, not at merge time. + - The remaining text is the description. Then run: `metta propose "" --workflow --stop-after --json` (when both flags present) @@ -104,9 +111,10 @@ Routing decision: - The boundary is reached when EITHER of these is true: 1. `STOP_AFTER` (set in Step 1) is non-empty AND equals the artifact id just passed to `metta complete`. 2. The change record's persisted `stop_after` field (read via `metta status --json --change `) is non-empty AND equals that artifact id. This second check provides robustness if `STOP_AFTER` was lost from local state for any reason; both checks should agree. + - `ship` is not a planning boundary: when `STOP_AFTER = "ship"` (or persisted `stop_after: ship`), this check never fires for any artifact — do not hunt for a `ship` artifact; continue the loop to `all_complete` and apply the Step 8 ship opt-in. - When the boundary is reached, the orchestrator MUST: a. NOT spawn any further planning subagent for the next artifact. - b. NOT proceed to Step 4 (research synthesis), Step 5 (implementation), Step 6 (review), Step 7 (verification), or Step 8 (finalize/merge). All subsequent steps are skipped in their entirety. + b. NOT proceed to Step 4 (research synthesis), Step 5 (implementation), Step 6 (review), Step 7 (verification), or Step 8 (finalize/PR). All subsequent steps are skipped in their entirety. c. NOT spawn any `metta-executor`, `metta-reviewer`, or `metta-verifier` agent. NOT call `metta finalize` or `git merge`. d. Print exactly one handoff line, formatted EXACTLY as: ``Stopped after ``. Run `` to .`` @@ -273,15 +281,23 @@ Routing decision: a. `metta finalize --json --change ` → runs gates, archives, merges specs b. `git -C "{change_root}" push -u origin metta/` → push the feature branch to the remote c. `gh pr create --title "" --body ""` → open a PR. The body MUST end with `🤖 Generated with [Claude Code](https://claude.com/claude-code)` - d. `gh pr checks --watch --fail-fast` → wait for all CI checks on the PR to complete before merging. If any check fails or is cancelled, do NOT merge — report the failing check(s) and the PR URL to the user and stop. If gh reports that no checks are reported yet (checks can lag PR creation by a few seconds), wait ~10s and retry the command - e. `gh pr merge --merge` → land the PR immediately, unless the user asked to leave it open for review — in that case stop here and report the PR URL instead of merging - f. Back on `main`: `git pull --ff-only`, then clean up the change branch and worktree + d. **Default path ends at an open PR. Do NOT merge; report the PR URL and stop.** + When `STOP_AFTER` (or the change record's persisted `stop_after`) is anything other than `ship`, report exactly: + ``PR open for review: . Run `/metta-ship` to land it, or merge the PR on GitHub yourself.`` + then proceed to Step 9 and return control to the user. On this default path you MUST NOT watch CI checks as a precursor to merging, MUST NOT merge the PR, and MUST NOT perform post-merge cleanup (main pull, branch/worktree removal). + + **Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record's persisted `stop_after` is `ship`):** + + e. `gh pr checks --watch --fail-fast` → wait for all CI checks on the PR to complete before merging. If any check fails or is cancelled, do NOT merge — report the failing check(s) and the PR URL to the user and stop. If gh reports that no checks are reported yet (checks can lag PR creation by a few seconds), wait ~10s and retry the command + f. `gh pr merge --merge` → land the PR + g. Back on `main`: `git pull --ff-only`, then clean up the change branch and worktree 9. Report to user what was done -## Critical: You MUST verify, finalize, and ship +## Critical: verify, finalize, and open the PR - Do NOT skip verification — a metta-verifier agent MUST run gates and confirm spec compliance -- Do NOT stop after the last artifact — finalize + ship must happen +- Do NOT stop before the PR exists — when no planning-phase `stop_after` boundary fired in Step 3, finalize, push, and `gh pr create` are mandatory on every completed run +- Merging is NOT part of the default path. Watching CI checks, merging the PR, and post-merge cleanup happen only under the Step 8 ship opt-in (`stop_after = ship`); otherwise stop at the open PR and hand off to `/metta-ship` - If metta finalize fails gates, spawn a metta-executor to fix, then retry - Direct local merge of the change branch into main (`git merge`) is forbidden — every change ships through a pushed branch and a GitHub PR - If a dispatched step appears orphaned, follow the residual orphaning recovery protocol in metta-skill-host.md. diff --git a/tests/cli-propose-stop-after.test.ts b/tests/cli-propose-stop-after.test.ts index e759e794..31b5b824 100644 --- a/tests/cli-propose-stop-after.test.ts +++ b/tests/cli-propose-stop-after.test.ts @@ -59,6 +59,19 @@ describe('metta propose --stop-after', { timeout: 30000 }, () => { expect(yaml).toContain('stop_after: tasks') }) + it('persists stop_after: ship', async () => { + const { stdout, code } = await runCli( + ['--json', 'propose', 'demo ship stop', '--stop-after', 'ship'], + tempDir, + ) + expect(code).toBe(0) + const data = JSON.parse(stdout) + expect(data.stop_after).toBe('ship') + const yamlPath = join(tempDir, 'spec', 'changes', data.change, '.metta.yaml') + const yaml = await readFile(yamlPath, 'utf8') + expect(yaml).toContain('stop_after: ship') + }) + it('rejects unknown --stop-after value with helpful error and writes no state', async () => { const { stdout, stderr, code } = await runCli( ['--json', 'propose', 'reject unknown stop', '--stop-after', 'spex'], @@ -70,6 +83,8 @@ describe('metta propose --stop-after', { timeout: 30000 }, () => { // Error MUST list the valid planning ids for the standard workflow expect(text).toContain('intent') expect(text).toContain('tasks') + // The valid-value list now names the ship sentinel + expect(text).toContain('ship') // No change directory should have been created const reject = await pathExists(join(tempDir, 'spec', 'changes', 'reject-unknown-stop')) expect(reject).toBe(false) @@ -137,6 +152,18 @@ describe('metta propose --stop-after', { timeout: 30000 }, () => { expect(yaml).toContain('auto_accept_recommendation: true') }) + it('--help names ship on the --stop-after option line', async () => { + const { stdout } = await runCli(['propose', '--help'], tempDir) + const start = stdout.indexOf('--stop-after') + expect(start).toBeGreaterThanOrEqual(0) + // The option entry may wrap; take the flag line plus its wrapped + // continuation lines (up to the next flag entry, e.g. "-h, --help"). + const rest = stdout.slice(start) + const nextFlag = rest.search(/\n\s*-/) + const entry = nextFlag === -1 ? rest : rest.slice(0, nextFlag) + expect(entry).toContain('ship') + }) + it('metta status --json surfaces stop_after when set', async () => { const { stdout: pStdout } = await runCli( ['--json', 'propose', 'status surfaces stop', '--stop-after', 'spec'], diff --git a/tests/skill-propose-ship-gate.test.ts b/tests/skill-propose-ship-gate.test.ts new file mode 100644 index 00000000..db19f8fe --- /dev/null +++ b/tests/skill-propose-ship-gate.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest' +import { readFile } from 'node:fs/promises' +import { join } from 'node:path' + +const repoRoot = join(import.meta.dirname, '..') +const proposeDeployedPath = join(repoRoot, '.claude', 'skills', 'metta-propose', 'SKILL.md') +const proposeTemplatePath = join(repoRoot, 'src', 'templates', 'skills', 'metta-propose', 'SKILL.md') +const autoTemplatePath = join(repoRoot, 'src', 'templates', 'skills', 'metta-auto', 'SKILL.md') +const fixIssuesTemplatePath = join(repoRoot, 'src', 'templates', 'skills', 'metta-fix-issues', 'SKILL.md') + +const SHIP_GATE_MARKER = + '**Ship opt-in — the following sub-steps run ONLY when `STOP_AFTER = "ship"` (or the change record\'s persisted `stop_after` is `ship`):**' +const DEFAULT_PHRASE = '**Default path ends at an open PR. Do NOT merge; report the PR URL and stop.**' +const HANDOFF_PHRASE = 'PR open for review: . Run `/metta-ship` to land it' + +const proposeCopies = [ + ['.claude/skills/metta-propose/SKILL.md', proposeDeployedPath], + ['src/templates/skills/metta-propose/SKILL.md', proposeTemplatePath], +] as const + +describe.each(proposeCopies)('metta-propose ship gate — %s', (_label, filePath) => { + it('places all merge commands after the ship opt-in marker', async () => { + const contents = await readFile(filePath, 'utf8') + const parts = contents.split(SHIP_GATE_MARKER) + expect(parts).toHaveLength(2) + expect(parts[0]).not.toContain('gh pr merge') + expect(parts[0]).not.toContain('gh pr checks') + expect(parts[1]).toContain('gh pr checks --watch --fail-fast') + expect(parts[1]).toContain('gh pr merge --merge') + }) + + it('contains the PR-open default phrase and the ship handoff phrase', async () => { + const contents = await readFile(filePath, 'utf8') + expect(contents).toContain(DEFAULT_PHRASE) + expect(contents).toContain(HANDOFF_PHRASE) + }) + + it('does not contain unconditional finalize/ship mandate language', async () => { + const contents = await readFile(filePath, 'utf8') + expect(contents).not.toContain('Critical: You MUST verify, finalize, and ship') + expect(contents).not.toContain('Do NOT stop after the last artifact') + expect(contents).not.toContain('finalize + ship must happen') + expect(contents).not.toContain('unless the user asked to leave it open') + }) + + it('retains the local-merge prohibition and PR creation step', async () => { + const contents = await readFile(filePath, 'utf8') + expect(contents).toContain('Direct local merge of the change branch into main') + expect(contents).toContain('gh pr create') + }) +}) + +describe('scope guard — other skills keep their merge instructions', () => { + it('metta-auto SKILL.md still contains gh pr merge', async () => { + const contents = await readFile(autoTemplatePath, 'utf8') + expect(contents).toContain('gh pr merge') + }) + + it('metta-fix-issues SKILL.md still contains gh pr merge', async () => { + const contents = await readFile(fixIssuesTemplatePath, 'utf8') + expect(contents).toContain('gh pr merge') + }) +})