diff --git a/docs/plans/2026-08-02-durable-approval-labels.md b/docs/plans/2026-08-02-durable-approval-labels.md new file mode 100644 index 00000000..a0faae7d --- /dev/null +++ b/docs/plans/2026-08-02-durable-approval-labels.md @@ -0,0 +1,1339 @@ +# Durable Workflow Approval Labels Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use +> superpowers:subagent-driven-development (recommended) or +> superpowers:executing-plans to implement this plan task-by-task. Steps use +> checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Preserve spec and plan approval labels as durable artifact facts, +reject approved issues whose artifacts cannot be resolved, and resume authorized +implementation without requiring humans to reapply approvals. + +**Architecture:** Add a read-only approved-artifact preflight before claim, +using the existing planning-artifact resolver with generation disabled. Make +workflow-state resolution stage-aware, keep approval labels out of all automatic +cleanup, and let saved `implementing` state bypass approval gates already +satisfied by that run. + +**Tech Stack:** TypeScript, Node.js built-in test runner, Patchmill run-state +and planning-artifact modules, Forgejo/GitHub label providers, Markdown/Astro +docs. + +## Global Constraints + +- Design source: `docs/specs/2026-08-02-durable-approval-labels-design.md`. +- `run-once` must never remove configured spec-approved or plan-approved labels. +- An approval label requires a resolvable corresponding artifact; missing or + invalid approved artifacts fail before claim, Pi execution, comments, or label + edits. +- An approved artifact is reused. Replacement requires a human to withdraw the + corresponding approval first. +- A saved resumable `implementing` run must not revisit spec or plan approval + gates. +- The configured label names, not default string literals, drive behavior. +- Do not change host-provider APIs or add dependencies. +- Use behavior tests for workflow transitions and recovery. Do not add tests + that merely assert documentation or static configuration text. + +--- + +## File and responsibility map + +**Create:** + +- `src/cli/commands/run-once/approval-artifact-preflight.ts` — read-only guard + that resolves artifacts with generation disabled and rejects missing approved + artifacts. +- `src/cli/commands/run-once/approval-artifact-preflight.test.ts` — focused + tests for configured approval labels, missing artifacts, and resolved + artifacts. + +**Modify:** + +- `src/cli/commands/run-once/artifacts.ts` — enumerate all matching issue + artifacts so approved-artifact preflight can reject ambiguous discovery while + preserving existing first-match behavior for unapproved workflows. +- `src/cli/commands/run-once/workflow-state.ts` — later-stage state precedence, + durable cleanup helpers, simplified plan gate, and safe retry labels. +- `src/cli/commands/run-once/workflow-state.test.ts` — unit behavior for state, + cleanup, and retry rules. +- `src/cli/commands/run-once/pipeline.ts` — invoke approved-artifact preflight + before claim and pass authoritative resume state to planning. +- `src/cli/commands/run-once/stage-advancement.ts` — remove stale-approval + branches and bypass completed approval gates on implementation resume. +- `src/cli/commands/run-once/pipeline-planning.test.ts` — strict + approved-artifact failures and durable plan/spec transition labels. +- `src/cli/commands/run-once/pipeline-failures-scenarios.test.ts` — regression + for unsupported implementation JSON followed by resume without relabeling. +- `src/cli/commands/run-once/pipeline-development-environment.test.ts` — durable + approvals and non-fabricated retry labels. +- `site/src/content/docs/reference/workflow-labels.md` — durable approval and + artifact-guard semantics. +- `site/src/content/docs/using-patchmill/run-once.md` — operator recovery and + approval behavior. +- `site/src/content/docs/using-patchmill/workflow-artifacts.md` — approved + artifact publication requirement. + +Existing `pipeline-finish.ts` continues to call the shared cleanup helper. The +integration tests must prove that this indirect path preserves approvals, so a +separate finish implementation is unnecessary. + +--- + +### Task 1: Resolve durable labels by the latest workflow stage + +**Files:** + +- Modify: `src/cli/commands/run-once/workflow-state.test.ts:44-85` +- Modify: `src/cli/commands/run-once/workflow-state.ts:66-90` + +**Interfaces:** + +- Consumes: `WorkflowStateOptions` and `WorkflowApprovalPolicy` unchanged. +- Produces: `resolveWorkflowState(labels, options): RunOnceWorkflowState` with + plan review taking precedence over durable spec approval, while approval still + wins over review for the same stage. + +- [ ] **Step 1: Add failing later-stage precedence tests** + +Add these cases beside the existing workflow-state resolution tests: + +```ts +test("resolveWorkflowState treats plan review as later than durable spec approval", () => { + assert.deepEqual( + resolveWorkflowState(["spec-approved", "plan-review"], { + readyLabel: ready, + policy, + }), + { kind: "waiting-plan-review", missingLabel: "plan-approved" }, + ); +}); + +test("resolveWorkflowState treats spec review as later than agent-ready", () => { + assert.deepEqual( + resolveWorkflowState([ready, "spec-review"], { + readyLabel: ready, + policy, + }), + { kind: "waiting-spec-review", missingLabel: "spec-approved" }, + ); +}); +``` + +Keep the existing tests proving that `spec-approved` wins over `spec-review` and +`plan-approved` wins over `plan-review`. + +- [ ] **Step 2: Run the focused tests and verify RED** + +Run: + +```bash +node --test \ + --test-name-pattern="resolveWorkflowState" \ + src/cli/commands/run-once/workflow-state.test.ts +``` + +Expected: both new tests fail because the current resolver checks +`spec-approved` and `agent-ready` before later review labels. + +- [ ] **Step 3: Implement stage-aware resolution** + +Replace `resolveWorkflowState` with this ordering: + +```ts +export function resolveWorkflowState( + labels: string[], + options: WorkflowStateOptions, +): RunOnceWorkflowState { + const { readyLabel, policy } = options; + const { specApproval, planApproval } = policy; + + if (has(labels, planApproval.approvedLabel)) return { kind: "plan-approved" }; + if (has(labels, planApproval.reviewLabel)) { + return { + kind: "waiting-plan-review", + missingLabel: planApproval.approvedLabel, + }; + } + if (has(labels, specApproval.approvedLabel)) return { kind: "spec-approved" }; + if (has(labels, specApproval.reviewLabel)) { + return { + kind: "waiting-spec-review", + missingLabel: specApproval.approvedLabel, + }; + } + if (has(labels, readyLabel)) return { kind: "agent-ready" }; + + return { kind: "not-actionable" }; +} +``` + +- [ ] **Step 4: Run the workflow-state tests and verify GREEN** + +Run: + +```bash +node --test src/cli/commands/run-once/workflow-state.test.ts +``` + +Expected: all workflow-state tests pass. + +- [ ] **Step 5: Commit the state precedence change** + +```bash +git add \ + src/cli/commands/run-once/workflow-state.ts \ + src/cli/commands/run-once/workflow-state.test.ts +git commit -m "fix(run-once): resolve durable approval stages" +``` + +--- + +### Task 2: Reject approved issues whose artifacts cannot be resolved + +**Files:** + +- Create: `src/cli/commands/run-once/approval-artifact-preflight.ts` +- Create: `src/cli/commands/run-once/approval-artifact-preflight.test.ts` +- Modify: `src/cli/commands/run-once/artifacts.ts:47-68` +- Modify: `src/cli/commands/run-once/pipeline.ts:23-31,273-289` +- Modify: `src/cli/commands/run-once/workflow-state.ts:43-51,121-145` +- Modify: `src/cli/commands/run-once/workflow-state.test.ts:127-175` +- Modify: + `src/cli/commands/run-once/stage-advancement.ts:247-255,544-550,684-789` +- Modify: + `src/cli/commands/run-once/pipeline-planning.test.ts:1168-1251,2116-2200` + +**Interfaces:** + +- Consumes: `AgentIssueConfig`, `AgentIssueRunState`, `IssueSummary`, + `ResolvedIssueArtifactSources`, and `resolvePlanningArtifacts()`. +- Produces: + +```ts +export async function findIssueArtifacts( + artifactDir: string, + issueNumber: number, +): Promise; + +export type ApprovedArtifactPreflightOptions = { + config: Pick< + AgentIssueConfig, + "repoRoot" | "specsDir" | "plansDir" | "approvalPolicy" + >; + issue: IssueSummary; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + now: Date; +}; + +export async function assertApprovedArtifactsResolvable( + options: ApprovedArtifactPreflightOptions, +): Promise; +``` + +- The function is read-only. It either returns or throws + `PlanningArtifactSafetyError` before claim. + +- [ ] **Step 1: Write failing preflight unit tests** + +Create `approval-artifact-preflight.test.ts` with these fixtures and cases: + +```ts +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { DEFAULT_PATCHMILL_CONFIG } from "../../../config/defaults.ts"; +import { createWorkflowApprovalPolicy } from "../../../workflow/approval-policy.ts"; +import { + assertApprovedArtifactsResolvable, + type ApprovedArtifactPreflightOptions, +} from "./approval-artifact-preflight.ts"; +import type { ResolvedIssueArtifactSource } from "./artifact-sources.ts"; +import type { IssueSummary } from "./types.ts"; + +const now = new Date("2026-08-02T12:00:00Z"); + +async function fixture() { + const repoRoot = await mkdtemp( + join(tmpdir(), "patchmill-approval-preflight-"), + ); + const approvalPolicy = createWorkflowApprovalPolicy({ + ...DEFAULT_PATCHMILL_CONFIG.workflow, + specApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.specApproval, + required: true, + }, + planApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.planApproval, + required: true, + }, + }); + const config: ApprovedArtifactPreflightOptions["config"] = { + repoRoot, + specsDir: join(repoRoot, "docs", "specs"), + plansDir: join(repoRoot, "docs", "plans"), + approvalPolicy, + }; + const issue: IssueSummary = { + number: 140, + title: "Keep approved artifacts", + body: "Approved workflow artifacts", + labels: [], + state: "open", + comments: [], + }; + return { config, issue }; +} + +function source( + repoRoot: string, + kind: "spec" | "plan", +): ResolvedIssueArtifactSource { + const path = + kind === "spec" + ? "docs/specs/approved-design.md" + : "docs/plans/approved-plan.md"; + return { + path, + absolutePath: join(repoRoot, path), + content: `# Approved ${kind}`, + evidence: `approved ${kind} fixture`, + }; +} + +test("approved spec without a resolvable spec fails safely", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*no spec artifact could be resolved/u, + ); +}); + +test("approved plan without a resolvable plan fails safely", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /plan-approved.*no plan artifact could be resolved/u, + ); +}); + +test("resolved approved artifacts pass preflight", async () => { + const { config, issue } = await fixture(); + issue.labels = [ + config.approvalPolicy.specApproval.approvedLabel, + config.approvalPolicy.planApproval.approvedLabel, + ]; + + await assert.doesNotReject( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: { + spec: source(config.repoRoot, "spec"), + plan: source(config.repoRoot, "plan"), + }, + now, + }), + ); +}); + +test("approved spec with multiple discovered specs fails as ambiguous", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + await mkdir(config.specsDir, { recursive: true }); + await writeFile( + join(config.specsDir, "2026-08-01-issue-140-first-design.md"), + "# First spec\n", + "utf8", + ); + await writeFile( + join(config.specsDir, "2026-08-02-issue-140-second-design.md"), + "# Second spec\n", + "utf8", + ); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*multiple spec artifacts/u, + ); +}); + +test("preflight uses configured approval label names", async () => { + const { config, issue } = await fixture(); + config.approvalPolicy = createWorkflowApprovalPolicy({ + ...DEFAULT_PATCHMILL_CONFIG.workflow, + specApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.specApproval, + required: true, + approvedLabel: "spec-reviewed", + }, + }); + issue.labels = ["spec-reviewed"]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-reviewed.*no spec artifact could be resolved/u, + ); +}); +``` + +- [ ] **Step 2: Run the new test and verify RED** + +Run: + +```bash +node --test src/cli/commands/run-once/approval-artifact-preflight.test.ts +``` + +Expected: FAIL because `approval-artifact-preflight.ts` does not exist. + +- [ ] **Step 3: Expose all deterministic filename candidates** + +Refactor `artifacts.ts` without changing existing unapproved discovery: + +```ts +export async function findIssueArtifacts( + artifactDir: string, + issueNumber: number, +): Promise { + let entries; + try { + entries = await readdir(artifactDir, { withFileTypes: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; + throw error; + } + + const marker = `-issue-${issueNumber}-`; + return entries + .filter((entry) => entry.isFile() && entry.name.includes(marker)) + .map((entry) => entry.name) + .sort((left, right) => left.localeCompare(right)) + .map((entry) => join(artifactDir, entry)); +} + +export async function findIssueArtifact( + artifactDir: string, + issueNumber: number, +): Promise { + return (await findIssueArtifacts(artifactDir, issueNumber))[0]; +} +``` + +The existing spec/plan discovery tests continue to prove that ordinary +unapproved workflow discovery selects the first deterministic match. + +- [ ] **Step 4: Implement the read-only preflight module** + +Create `approval-artifact-preflight.ts`: + +```ts +import { basename, join } from "node:path"; +import { findIssueArtifacts } from "./artifacts.ts"; +import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; +import { + PlanningArtifactSafetyError, + resolvePlanningArtifacts, + type PlanningArtifactPolicy, + type ResolvedPlanningArtifacts, +} from "./planning-artifacts.ts"; +import { mirrorConfiguredPathInWorktree } from "./pipeline-workspace.ts"; +import type { + AgentIssueConfig, + AgentIssueRunState, + IssueSummary, +} from "./types.ts"; + +export type ApprovedArtifactPreflightOptions = { + config: Pick< + AgentIssueConfig, + "repoRoot" | "specsDir" | "plansDir" | "approvalPolicy" + >; + issue: IssueSummary; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + now: Date; +}; + +function preflightPolicy( + options: ApprovedArtifactPreflightOptions, +): PlanningArtifactPolicy { + const { config, existingState } = options; + const worktreeRoot = existingState?.worktreePath + ? join(config.repoRoot, existingState.worktreePath) + : undefined; + const primaryRoot = worktreeRoot ?? config.repoRoot; + + return { + kind: "fresh", + primary: { + repoRoot: primaryRoot, + specsDir: mirrorConfiguredPathInWorktree( + config.repoRoot, + primaryRoot, + config.specsDir, + ), + plansDir: mirrorConfiguredPathInWorktree( + config.repoRoot, + primaryRoot, + config.plansDir, + ), + source: worktreeRoot ? "resume-worktree" : "primary-repo", + }, + fallbacks: worktreeRoot + ? [ + { + repoRoot: config.repoRoot, + specsDir: config.specsDir, + plansDir: config.plansDir, + source: "primary-repo", + }, + ] + : undefined, + explicit: options.resolvedArtifacts, + saved: { + specPath: existingState?.specPath, + specCommit: existingState?.specCommit, + planPath: existingState?.planPath, + planCommit: existingState?.planCommit, + specCreated: existingState?.checkpoints?.specCreated, + planCreated: existingState?.checkpoints?.planCreated, + }, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }; +} + +function artifactDirs( + options: ApprovedArtifactPreflightOptions, + kind: "spec" | "plan", +): string[] { + const configuredDir = + kind === "spec" ? options.config.specsDir : options.config.plansDir; + if (!options.existingState?.worktreePath) return [configuredDir]; + + const worktreeRoot = join( + options.config.repoRoot, + options.existingState.worktreePath, + ); + return [ + mirrorConfiguredPathInWorktree( + options.config.repoRoot, + worktreeRoot, + configuredDir, + ), + configuredDir, + ]; +} + +async function assertUnambiguousDiscovery( + options: ApprovedArtifactPreflightOptions, + kind: "spec" | "plan", + label: string, +): Promise { + if (options.resolvedArtifacts[kind]) return; + const savedPath = + kind === "spec" + ? options.existingState?.specPath + : options.existingState?.planPath; + if (savedPath) return; + + const candidates = ( + await Promise.all( + artifactDirs(options, kind).map((dir) => + findIssueArtifacts(dir, options.issue.number), + ), + ) + ).flat(); + const names = [ + ...new Set(candidates.map((candidate) => basename(candidate))), + ]; + if (names.length <= 1) return; + + throw new PlanningArtifactSafetyError( + `Issue #${options.issue.number} has approval label ${label}, but multiple ${kind} artifacts could be resolved: ${names.join(", ")}`, + ); +} + +function missingApprovedArtifact( + issue: IssueSummary, + label: string, + kind: "spec" | "plan", +): PlanningArtifactSafetyError { + return new PlanningArtifactSafetyError( + `Issue #${issue.number} has approval label ${label}, but no ${kind} artifact could be resolved; remove ${label} before creating a new ${kind}`, + ); +} + +export async function assertApprovedArtifactsResolvable( + options: ApprovedArtifactPreflightOptions, +): Promise { + const specLabel = options.config.approvalPolicy.specApproval.approvedLabel; + const planLabel = options.config.approvalPolicy.planApproval.approvedLabel; + const requiresSpec = options.issue.labels.includes(specLabel); + const requiresPlan = options.issue.labels.includes(planLabel); + if (!requiresSpec && !requiresPlan) return; + + if (requiresSpec) { + await assertUnambiguousDiscovery(options, "spec", specLabel); + } + if (requiresPlan) { + await assertUnambiguousDiscovery(options, "plan", planLabel); + } + + let artifacts: ResolvedPlanningArtifacts; + try { + artifacts = await resolvePlanningArtifacts({ + policy: preflightPolicy(options), + issue: options.issue, + now: options.now, + }); + } catch (error) { + if (error instanceof PlanningArtifactSafetyError) { + const labels = [ + ...(requiresSpec ? [specLabel] : []), + ...(requiresPlan ? [planLabel] : []), + ].join(", "); + throw new PlanningArtifactSafetyError( + `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts could not be resolved: ${error.message}`, + ); + } + throw error; + } + + if (requiresSpec && !artifacts.spec.exists) { + throw missingApprovedArtifact(options.issue, specLabel, "spec"); + } + if (requiresPlan && !artifacts.plan.exists) { + throw missingApprovedArtifact(options.issue, planLabel, "plan"); + } +} +``` + +- [ ] **Step 5: Run the preflight unit tests and verify GREEN** + +Run: + +```bash +node --test src/cli/commands/run-once/approval-artifact-preflight.test.ts +``` + +Expected: 5 tests pass. + +- [ ] **Step 6: Remove stale-approval gate branches made impossible by + preflight** + +In `workflow-state.ts`: + +- remove `staleApprovedLabel` from `PlanApprovalGateDecision`; +- remove `planCreatedThisRun` from `decidePlanApprovalGate()` options; +- let a present configured plan-approved label always return `proceed`. + +The final gate is: + +```ts +export function decidePlanApprovalGate(options: { + labels: string[]; + planOnly: boolean; + policy: WorkflowApprovalPolicy; +}): PlanApprovalGateDecision { + if (options.planOnly) return { action: "stop-for-plan-only" }; + const approval = options.policy.planApproval; + if (!approval.required) return { action: "proceed" }; + if (options.labels.includes(approval.approvedLabel)) { + return { action: "proceed" }; + } + return { + action: "stop-for-plan-review", + reviewLabel: approval.reviewLabel, + missingLabel: approval.approvedLabel, + }; +} +``` + +Delete the stale-plan unit test at `workflow-state.test.ts:161-175`. In +`stage-advancement.ts`, remove `specCreatedThisRun`, `planCreatedThisRun`, their +assignments, the `!specCreatedThisRun` condition, and the `planCreatedThisRun` +argument passed to `decidePlanApprovalGate()`. + +- [ ] **Step 7: Wire preflight before claim** + +Import `assertApprovedArtifactsResolvable` in `pipeline.ts`. Immediately after +`runArtifactSourceStage()` assigns `issueForRun` and `resolvedArtifacts`, and +before repository status checking or the claim label edit, add: + +```ts +await assertApprovedArtifactsResolvable({ + config, + issue: issueForRun, + existingState, + resolvedArtifacts, + now: runOptions.now ?? new Date(), +}); +``` + +Do not place this inside the later pipeline `try` block: a safety failure must +not become an unexpected-failure comment or mutate run state. + +- [ ] **Step 8: Rewrite contradictory pipeline scenarios as safety failures** + +Rename the test at `pipeline-planning.test.ts:1168` to: + +```ts +test("runOneIssue rejects spec-approved when no spec can be resolved", async () => { +``` + +Keep its issue labels and missing-spec fixture, but replace the old result and +label assertions with: + +```ts +await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /spec-approved.*no spec artifact could be resolved/u, +); +assert.equal( + runner.calls.some((call) => call.command === "pi"), + false, +); +assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, +); +``` + +Rename the test at `pipeline-planning.test.ts:2116` to: + +```ts +test("runOneIssue rejects plan-approved when no plan can be resolved", async () => { +``` + +Replace its Pi/result/label assertions with the corresponding +`plan-approved.*no plan artifact could be resolved` rejection and the same +no-Pi/no-label/no-comment checks. + +- [ ] **Step 9: Run strict artifact tests and build** + +Run: + +```bash +node --test \ + src/cli/commands/run-once/approval-artifact-preflight.test.ts \ + src/cli/commands/run-once/workflow-state.test.ts +node --test \ + --test-name-pattern="no (spec|plan) can be resolved" \ + src/cli/commands/run-once/pipeline-planning.test.ts +npm run build +``` + +Expected: all selected tests pass and TypeScript builds without errors. + +- [ ] **Step 10: Commit the approved-artifact guard** + +```bash +git add \ + src/cli/commands/run-once/approval-artifact-preflight.ts \ + src/cli/commands/run-once/approval-artifact-preflight.test.ts \ + src/cli/commands/run-once/artifacts.ts \ + src/cli/commands/run-once/pipeline.ts \ + src/cli/commands/run-once/workflow-state.ts \ + src/cli/commands/run-once/workflow-state.test.ts \ + src/cli/commands/run-once/stage-advancement.ts \ + src/cli/commands/run-once/pipeline-planning.test.ts +git commit -m "fix(run-once): guard approved artifacts" +``` + +--- + +### Task 3: Preserve approvals through review, implementation, and completion + +**Files:** + +- Modify: `src/cli/commands/run-once/workflow-state.test.ts:187-228` +- Modify: `src/cli/commands/run-once/workflow-state.ts:147-188` +- Modify: + `src/cli/commands/run-once/pipeline-planning.test.ts:1253-1336,2202-2312` + +**Interfaces:** + +- Consumes: existing `cleanupLabelsForSpecReview`, `cleanupLabelsForPlanReview`, + and `cleanupLabelsForImplementation` signatures. +- Produces: cleanup functions that remove only lifecycle/review labels and never + configured approval labels. `pipeline-finish.ts` inherits the behavior through + `cleanupLabelsForImplementation()`. + +- [ ] **Step 1: Change cleanup tests to require durable approvals** + +Replace the three cleanup expectations with: + +```ts +test("cleanupLabelsForSpecReview preserves approval labels", () => { + assert.deepEqual( + cleanupLabelsForSpecReview( + [ready, "spec-approved", "plan-review", "plan-approved", "bug"], + { readyLabel: ready, policy }, + ), + ["spec-approved", "plan-approved", "bug", "spec-review"], + ); +}); + +test("cleanupLabelsForPlanReview preserves approval labels", () => { + assert.deepEqual( + cleanupLabelsForPlanReview( + [ready, "spec-review", "spec-approved", "plan-approved", "bug"], + { readyLabel: ready, policy }, + ), + ["spec-approved", "plan-approved", "bug", "plan-review"], + ); +}); + +test("cleanupLabelsForImplementation preserves approval labels", () => { + assert.deepEqual( + cleanupLabelsForImplementation( + [ + ready, + "spec-review", + "spec-approved", + "plan-review", + "plan-approved", + "bug", + ], + { readyLabel: ready, policy }, + ), + ["spec-approved", "plan-approved", "bug"], + ); +}); +``` + +- [ ] **Step 2: Run cleanup tests and verify RED** + +Run: + +```bash +node --test \ + --test-name-pattern="cleanupLabels" \ + src/cli/commands/run-once/workflow-state.test.ts +``` + +Expected: all three tests fail because the current helpers remove approvals. + +- [ ] **Step 3: Remove approval labels from cleanup removal sets** + +Implement the three helpers as: + +```ts +export function cleanupLabelsForSpecReview( + labels: string[], + options: WorkflowStateOptions, +): string[] { + return addLabel( + removeLabels(labels, [ + options.readyLabel, + options.policy.planApproval.reviewLabel, + ]), + options.policy.specApproval.reviewLabel, + ); +} + +export function cleanupLabelsForPlanReview( + labels: string[], + options: WorkflowStateOptions, +): string[] { + return addLabel( + removeLabels(labels, [ + options.readyLabel, + options.policy.specApproval.reviewLabel, + ]), + options.policy.planApproval.reviewLabel, + ); +} + +export function cleanupLabelsForImplementation( + labels: string[], + options: WorkflowStateOptions, +): string[] { + return removeLabels(labels, [ + options.readyLabel, + options.policy.specApproval.reviewLabel, + options.policy.planApproval.reviewLabel, + ]); +} +``` + +- [ ] **Step 4: Strengthen the plan-review integration assertion** + +Rename the test at `pipeline-planning.test.ts:1253` to: + +```ts +test("runOneIssue writes a plan and preserves spec approval at plan review", async () => { +``` + +Replace the weak `args.includes("spec-approved")` assertion with: + +```ts +const removedLabels = + finalEdit.args[finalEdit.args.indexOf("--remove-labels") + 1]?.split(",") ?? + []; +assert.deepEqual(removedLabels.sort(), ["in-progress", "spec-review"].sort()); +assert.equal(removedLabels.includes("spec-approved"), false); +``` + +The expected add-label argument remains `plan-review`. + +- [ ] **Step 5: Strengthen successful completion coverage** + +In the test at `pipeline-planning.test.ts:2202`: + +1. Create a resolvable spec because the issue carries `spec-approved`: + +```ts +const specPath = "docs/specs/2026-05-14-issue-49-approved-spec-design.md"; +await writeFile(join(config.repoRoot, specPath), "# spec\n", "utf8"); +``` + +1. Rename the test to: + +```ts +test("runOneIssue preserves approvals while clearing review labels", async () => { +``` + +1. Replace the four weak `args.includes()` assertions with an assertion over all + label edits: + +```ts +const removedLabels = editCalls.flatMap((call) => { + const index = call.args.indexOf("--remove-labels"); + return index < 0 ? [] : (call.args[index + 1]?.split(",") ?? []); +}); +assert.equal(removedLabels.includes("spec-approved"), false); +assert.equal(removedLabels.includes("plan-approved"), false); +assert.equal(removedLabels.includes("spec-review"), true); +assert.equal(removedLabels.includes("plan-review"), true); +assert.equal(removedLabels.includes("in-progress"), true); +``` + +This single scenario covers claim, implementation cleanup, and the +`pipeline-finish.ts` completion path. + +- [ ] **Step 6: Run durable transition tests and verify GREEN** + +Run: + +```bash +node --test src/cli/commands/run-once/workflow-state.test.ts +node --test \ + --test-name-pattern="preserves spec approval|preserves approvals while" \ + src/cli/commands/run-once/pipeline-planning.test.ts +``` + +Expected: all selected tests pass. + +- [ ] **Step 7: Commit durable cleanup behavior** + +```bash +git add \ + src/cli/commands/run-once/workflow-state.ts \ + src/cli/commands/run-once/workflow-state.test.ts \ + src/cli/commands/run-once/pipeline-planning.test.ts +git commit -m "fix(run-once): preserve approval labels" +``` + +--- + +### Task 4: Resume authorized implementation without reapproval + +**Files:** + +- Modify: + `src/cli/commands/run-once/stage-advancement.ts:89-121,222-255,544-550,784-801` +- Modify: `src/cli/commands/run-once/pipeline.ts:151-155,499-525` +- Modify: `src/cli/commands/run-once/workflow-state.ts:190-209` +- Modify: `src/cli/commands/run-once/workflow-state.test.ts:230-252` +- Modify: + `src/cli/commands/run-once/pipeline-failures-scenarios.test.ts:886-1064` +- Modify: + `src/cli/commands/run-once/pipeline-development-environment.test.ts:295-501` + +**Interfaces:** + +- Adds required input: + +```ts +approvalGatesSatisfied: boolean; +``` + +to `AdvancePlanningStagesOptions`. + +- `pipeline.ts` sets it only when the selected issue is an ordinary resumable + run whose saved status is `implementing`. +- `retryableLabelsAfterDevelopmentEnvironmentFailure()` keeps its signature but + restores `readyLabel`, not fabricated plan approval, when no original + actionable label can be proven. + +- [ ] **Step 1: Turn the unsupported-JSON scenario into the reported approval + regression** + +In `pipeline-failures-scenarios.test.ts:886`, first extend the shared fixture +import: + +```ts +import { + approvalPolicy, + makeConfig, +} from "../../../../test-support/run-once/pipeline-fixtures.ts"; +``` + +Then change the existing test configuration and fixtures to require spec +approval: + +```ts +const config = await makeConfig({ + dryRun: false, + execute: true, + approvalPolicy: approvalPolicy({ specRequired: true }), +}); +const selected = issue( + 42, + ["spec-review", "spec-approved", "enhancement"], + "Handle implementation parse failure", +); +const existingSpecPath = join( + config.specsDir, + "2026-05-01-issue-42-handle-implementation-parse-failure-design.md", +); +await writeFile(existingSpecPath, "# spec\n", "utf8"); +``` + +Keep the existing plan fixture and unsupported `{"status":"unknown"}` Pi result. +Remove the old `assert.equal(editCalls.length, 1)` check because claim and +review cleanup are separate idempotent edits for this label set. After the first +run, add: + +```ts +const removedLabels = editCalls.flatMap((call) => { + const index = call.args.indexOf("--remove-labels"); + return index < 0 ? [] : (call.args[index + 1]?.split(",") ?? []); +}); +assert.equal(removedLabels.includes("spec-approved"), false); +``` + +Change the resumed issue payload to retain approval: + +```ts +issue( + 42, + ["in-progress", "spec-approved", "enhancement"], + "Handle implementation parse failure", +); +``` + +Keep the final assertion that the rerun returns `pr-created`. This reproduces +the screenshot sequence without manual relabeling. + +- [ ] **Step 2: Make the legacy resume test require plan approval** + +In `pipeline-development-environment.test.ts:380`, add a required plan gate +while leaving the resumed issue labels as only `in-progress`: + +```ts +const config = await makeConfig({ + dryRun: false, + execute: true, + approvalPolicy: specAndPlanApprovalPolicy(), + skills: { + ...DEFAULT_PATCHMILL_CONFIG.skills, + developmentEnvironment: "./skills/development-environment", + }, +}); +``` + +The existing saved state is `implementing` and has a saved plan. Keep the +assertion that the development-environment Pi prompt runs. Without authoritative +resume, this test stops at plan review before Pi. + +- [ ] **Step 3: Update retry-label tests to forbid fabricated approval** + +Change the unit test at `workflow-state.test.ts:242` to: + +```ts +test("retryableLabelsAfterDevelopmentEnvironmentFailure restores ready for legacy resume", () => { + assert.deepEqual( + retryableLabelsAfterDevelopmentEnvironmentFailure(["in-progress", "bug"], { + readyLabel: ready, + policy, + originalLabels: ["in-progress"], + inProgressLabel: "in-progress", + }), + ["bug", ready], + ); +}); +``` + +In `pipeline-development-environment.test.ts`: + +- change the default not-ready expectation at lines 322-329 from added + `plan-approved` to added `agent-ready`; +- in the durable-approval scenario at lines 333-378, assert that the final edit + has no `--add-labels` argument and removes only `in-progress`; +- in the resumed legacy scenario at lines 493-500, expect `agent-ready` instead + of `plan-approved`. + +- [ ] **Step 4: Run recovery tests and verify RED** + +Run: + +```bash +node --test \ + --test-name-pattern="unexpected implementation failures|resumed development environment failure" \ + src/cli/commands/run-once/pipeline-failures-scenarios.test.ts \ + src/cli/commands/run-once/pipeline-development-environment.test.ts +node --test \ + --test-name-pattern="retryableLabelsAfterDevelopmentEnvironmentFailure" \ + src/cli/commands/run-once/workflow-state.test.ts +``` + +Expected: the legacy resumed run stops for plan approval, and retry-label tests +fail because current code fabricates `plan-approved`. + +- [ ] **Step 5: Pass authoritative approval state into planning** + +Add this required property to `AdvancePlanningStagesOptions`: + +```ts +approvalGatesSatisfied: boolean; +``` + +Destructure it in `advancePlanningStages()`. In `pipeline.ts`, pass: + +```ts +approvalGatesSatisfied: + ordinaryResumableState && existingState?.status === "implementing", +``` + +Change spec approval calculation to: + +```ts +const hasCurrentSpecApproval = + approvalGatesSatisfied || + issue.labels.includes(config.approvalPolicy.specApproval.approvedLabel); +``` + +Change plan gate calculation to: + +```ts +const planGate = approvalGatesSatisfied + ? ({ action: "proceed" } as const) + : decidePlanApprovalGate({ + labels, + planOnly: config.planOnly, + policy: config.approvalPolicy, + }); +``` + +Do not bypass artifact/worktree validation; bypass only the two human approval +gates. + +- [ ] **Step 6: Stop fabricating plan approval on development-environment + failure** + +In `retryableLabelsAfterDevelopmentEnvironmentFailure()`, replace the fallback: + +```ts +const restore = + originalActionableLabels.length > 0 + ? originalActionableLabels + : [options.readyLabel]; +``` + +The existing `withoutInProgress` value already retains durable approvals, so +normal approved runs will not emit redundant add-label operations. + +- [ ] **Step 7: Run recovery tests and verify GREEN** + +Run: + +```bash +node --test \ + src/cli/commands/run-once/workflow-state.test.ts \ + src/cli/commands/run-once/pipeline-development-environment.test.ts +node --test \ + --test-name-pattern="unexpected implementation failures" \ + src/cli/commands/run-once/pipeline-failures-scenarios.test.ts +``` + +Expected: all selected tests pass; the unsupported-JSON rerun reaches +`pr-created`; the legacy implementing run reaches the development-environment +stage despite missing approval labels; retryable failures never invent +`plan-approved`. + +- [ ] **Step 8: Commit recovery semantics** + +```bash +git add \ + src/cli/commands/run-once/stage-advancement.ts \ + src/cli/commands/run-once/pipeline.ts \ + src/cli/commands/run-once/workflow-state.ts \ + src/cli/commands/run-once/workflow-state.test.ts \ + src/cli/commands/run-once/pipeline-failures-scenarios.test.ts \ + src/cli/commands/run-once/pipeline-development-environment.test.ts +git commit -m "fix(run-once): resume authorized implementation" +``` + +--- + +### Task 5: Document durable approvals and run final verification + +**Files:** + +- Modify: `site/src/content/docs/reference/workflow-labels.md:47-107` +- Modify: `site/src/content/docs/using-patchmill/run-once.md:71-88` +- Modify: `site/src/content/docs/using-patchmill/workflow-artifacts.md:47-64` + +**Interfaces:** + +- Consumes: implemented workflow semantics from Tasks 1-4. +- Produces: operator-facing documentation. No runtime interface changes. + +- [ ] **Step 1: Update the workflow-label reference** + +After the actionable/waiting-state paragraph in `reference/workflow-labels.md`, +add: + +```markdown +Approved labels are durable facts about the current resolvable artifacts. +Patchmill preserves them through claim, implementation, failure, resume, and +successful completion. Review, ready, in-progress, needs-info, and done labels +continue to represent transient workflow or lifecycle state. + +An approved label requires its corresponding artifact to resolve. If +`spec-approved` has no valid spec, or `plan-approved` has no valid plan, +`run-once` stops with a safety error before claiming the issue or invoking Pi. +Remove approval explicitly before replacing an approved artifact. +``` + +Replace the final sentence at lines 105-107 with: + +```markdown +Humans may either replace a review label with its approved label or leave both +in place. Approval wins over review for the same stage; a later-stage review, +such as `plan-review`, wins over durable approval from an earlier stage. +``` + +- [ ] **Step 2: Update run-once operator guidance** + +After the paragraph that instructs users to add an approved label, add: + +```markdown +Patchmill keeps that approval label after the workflow advances. A failed +implementation can therefore resume without asking a human to approve the same +artifact again. If an approved artifact is missing or invalid, restore the +published artifact or explicitly remove approval before creating a replacement. +``` + +- [ ] **Step 3: Update workflow-artifact guidance** + +After the numbered recommended workflow, add: + +```markdown +An approval label asserts that the corresponding artifact has been published or +otherwise resolves unambiguously and is the artifact Patchmill must reuse. Do +not apply `spec-approved` before a spec resolves or `plan-approved` before a +plan resolves. Patchmill fails safely rather than synthesizing a replacement for +a missing approved artifact. +``` + +- [ ] **Step 4: Verify documentation directly** + +No new automated test is warranted for prose. Run format, Markdown lint, and the +site build instead: + +```bash +npx prettier --check \ + site/src/content/docs/reference/workflow-labels.md \ + site/src/content/docs/using-patchmill/run-once.md \ + site/src/content/docs/using-patchmill/workflow-artifacts.md +npx markdownlint-cli2 \ + site/src/content/docs/reference/workflow-labels.md \ + site/src/content/docs/using-patchmill/run-once.md \ + site/src/content/docs/using-patchmill/workflow-artifacts.md +npm --prefix site run build +``` + +Expected: Prettier reports all files formatted, Markdown lint reports zero +errors, and the Astro site build exits 0. + +- [ ] **Step 5: Run focused and full project verification** + +Run: + +```bash +npm run test:run-once +npm test +npm run lint +npm run build +git diff --check +``` + +Expected: + +- run-once tests pass with zero failures; +- the full Node test suite passes with zero failures; +- Prettier, ESLint, and Markdown lint report zero errors; +- TypeScript build exits 0; +- `git diff --check` emits no output. + +No Nix build is required because package dependencies and lock files do not +change. + +- [ ] **Step 6: Commit documentation** + +```bash +git add \ + site/src/content/docs/reference/workflow-labels.md \ + site/src/content/docs/using-patchmill/run-once.md \ + site/src/content/docs/using-patchmill/workflow-artifacts.md +git commit -m "docs(run-once): explain durable approvals" +``` + +- [ ] **Step 7: Inspect final branch state** + +Run: + +```bash +git status --short +git log --oneline --decorate -8 +``` + +Expected: working tree is clean and the branch contains the design commit, plan +commit, four code commits, and documentation commit described above. diff --git a/docs/specs/2026-08-02-durable-approval-labels-design.md b/docs/specs/2026-08-02-durable-approval-labels-design.md new file mode 100644 index 00000000..2bb410a6 --- /dev/null +++ b/docs/specs/2026-08-02-durable-approval-labels-design.md @@ -0,0 +1,309 @@ +# Durable Workflow Approval Labels Design + +## Summary + +Patchmill will treat `spec-approved` and `plan-approved` as durable statements +about the current specification and implementation plan. It will no longer +consume those labels merely because `run-once` advances to a later workflow +stage. + +Patchmill will not remove approval labels automatically. When an approval label +is present, the corresponding artifact must resolve and be reused. If it is +missing or ambiguous, `run-once` will fail safely without invoking Pi or +changing labels. Active run state, rather than the absence of an approval label, +will control implementation resume behavior. + +## Problem + +The current workflow models approval labels as actionable state tokens. Before +implementation, `run-once` removes all ready, review, and approval labels while +retaining `in-progress`. A normal implementation run can therefore change: + +```text +spec-review + spec-approved -> in-progress +``` + +If implementation then fails unexpectedly, the saved run remains resumable. +Selection correctly finds the `in-progress` issue, but planning-stage +advancement checks the issue's current labels again. Because Patchmill removed +`spec-approved`, the resumed run stops at `spec-review` and removes +`in-progress`. A human must reapply an approval that Patchmill had already +validated. + +The same defect applies to `plan-approved` when plan approval is required. + +This behavior makes approval history misleading and couples recovery to labels +that Patchmill deliberately destroyed. Existing unexpected-failure tests do not +expose the defect because they use the default configuration, where spec and +plan approvals are disabled. + +## Goals + +- Preserve valid spec and plan approvals through claim, implementation, + unexpected failure, resume, and successful completion. +- Never remove an approval label automatically. +- Require an approved artifact to resolve uniquely and reuse it rather than + creating a replacement. +- Reject contradictory approval and artifact state without invoking Pi or + changing labels. +- Resume an authorized implementation without reevaluating completed approval + gates. +- Keep automatic selection correct when a durable earlier-stage approval and a + later review label coexist. +- Preserve custom configured workflow-label names. +- Cover required-approval recovery with regression tests. + +## Non-goals + +- Bind approvals cryptographically to artifact contents or commits. +- Detect every out-of-band artifact edit made after human approval. +- Restore approval labels that were removed by older Patchmill versions from + finished or non-resumable issues. +- Change how humans grant approval. +- Add an automated revision workflow for an approved artifact. +- Change triage classification or priority ordering. + +## Approval invariants + +Approval labels describe the current artifacts: + +- The configured spec-approved label means the current specification is + approved. +- The configured plan-approved label means the current implementation plan is + approved. +- Advancing to implementation does not make either statement false. +- An implementation failure does not make either statement false. +- Successful implementation does not make either statement false. + +Approval labels are also artifact-existence guards: + +- `spec-approved` requires one uniquely resolvable specification. Patchmill must + reuse it and must not invoke Pi to create or replace a spec. +- `plan-approved` requires one uniquely resolvable implementation plan. + Patchmill must reuse it and must not invoke Pi to create or replace a plan. +- A missing or ambiguous approved artifact is a safety error, not permission to + synthesize a replacement. +- Revising an approved spec requires a human to withdraw both spec approval and + downstream plan approval first. +- Revising an approved plan requires a human to withdraw plan approval first. + +`run-once` never removes an approval label on the human's behalf. Review labels +remain transient workflow-state labels. Ready, in-progress, needs-info, and done +labels remain lifecycle labels. + +## Label transitions + +The names below are defaults; all behavior uses configured label names. + +### Claim + +Claiming removes `agent-ready` and adds `in-progress`. It preserves +`spec-approved`, `plan-approved`, `spec-review`, and `plan-review` until the +resolved workflow transition determines which review labels are stale. + +### Spec creation + +Patchmill may create a spec only when neither `spec-approved` nor +`plan-approved` is present. If the pipeline determines that spec creation is +required while either approval is present, it fails with a safety error before +invoking Pi or changing labels. + +After creating an unapproved spec, Patchmill: + +- adds `spec-review` when spec approval is required; +- removes `agent-ready` and `in-progress` at the review stop; +- removes stale plan review state; +- does not remove any approval label because artifact creation was prohibited + while an approval was present. + +### Plan creation + +Patchmill may create a plan only when `plan-approved` is absent. An approved +spec may be used to create its first plan and remains approved. + +After creating an unapproved plan, Patchmill: + +- preserves `spec-approved`; +- adds `plan-review` when plan approval is required; +- removes `agent-ready`, `spec-review`, and `in-progress` at the review stop; +- does not remove `plan-approved` because plan creation was prohibited while + that approval was present. + +### Enter implementation + +Before implementation: + +- preserve `spec-approved` and `plan-approved`; +- remove `agent-ready`, `spec-review`, and `plan-review`; +- retain `in-progress`. + +### Unexpected implementation failure + +An unexpected failure: + +- preserves both approval labels; +- preserves `in-progress`; +- records a resumable `implementing` run state and the error. + +A rerun resumes implementation without requiring either approval to be added +again. + +### Successful completion + +Successful completion: + +- preserves both approval labels; +- removes `agent-ready`, review labels, `in-progress`, and `needs-info`; +- adds `agent-done`. + +## Workflow-state resolution + +Durable earlier-stage approvals can coexist with later-stage review labels. The +resolver must therefore prefer the latest workflow stage, while an approval +still wins over the review label for the same stage. + +Resolution order will be: + +1. plan approved; +2. waiting for plan review; +3. spec approved; +4. waiting for spec review; +5. agent ready; +6. not actionable. + +Examples: + +| Labels | Resolved state | +| ----------------------------------------------- | ----------------------- | +| `spec-review`, `spec-approved` | spec approved | +| `spec-approved`, `plan-review` | waiting for plan review | +| `spec-approved`, `plan-review`, `plan-approved` | plan approved | +| `agent-ready`, `spec-review` | waiting for spec review | + +This prevents a durable `spec-approved` label from causing automatic selection +while a new plan is waiting for approval. + +## Resume semantics + +Saved active run state is authoritative for stages already completed. + +When the saved state is `implementing`, `run-once` may resolve and validate the +saved artifacts and workspace, but it must not send the issue back through spec +or plan approval gates. Entering the implementing state proves those gates were +satisfied for the artifacts used by that run. + +For new runs and saved `planning` runs, approval gates continue to inspect the +current labels. Because approvals are no longer consumed on entry to +implementation, a failure before the implementing state is persisted remains +retryable without manual relabeling. + +This rule also provides compatibility for an active run created by an older +Patchmill version after that version already removed its approval labels. + +## Cleanup responsibilities + +Label cleanup will be split by purpose rather than using one helper that removes +all workflow labels: + +- artifact guards validate approved artifacts before any artifact-creation Pi + call; +- review-transition cleanup removes ready, in-progress, and stale review labels + but never approval labels; +- implementation cleanup removes ready and review labels but preserves approval + labels; +- completion cleanup removes lifecycle and review labels but preserves approval + labels; +- failure cleanup changes only labels required by the failure outcome. + +No `run-once` cleanup caller may infer that approval is stale or remove an +approval label merely because the workflow advanced or an artifact could not be +resolved. + +## Development-environment failures + +Development-environment readiness failures currently reconstruct actionable +labels because implementation cleanup has consumed them. With durable approvals, +that reconstruction is unnecessary when approval labels are already present. + +The retry path will preserve existing approvals and remove `in-progress` as +required by its terminal outcome. Compatibility behavior for a legacy active run +must use saved run state rather than inventing a new approval for an artifact +that was never approved. + +## Error handling and consistency + +Label updates and run-state writes are separate host operations and cannot be +atomic. The design minimizes inconsistent recovery states by ensuring that: + +- approved-artifact guards run before claim or artifact-creation side effects; +- a missing or ambiguous approved artifact produces a clear safety error naming + the approval label and artifact problem; +- the guard does not invoke Pi or mutate issue labels; +- approval facts survive ordinary progression and implementation errors; +- an active implementing state bypasses completed approval gates; +- label operations are idempotent; +- rerunning after any partial transition converges on the same label set. + +Patchmill must not remove a valid approval as generic cleanup after a later +operation fails. + +## Compatibility + +No configuration migration is required. Custom review and approval label names +continue to come from the workflow approval policy. + +Active legacy runs saved as `implementing` can resume even if an older version +removed their approvals. Patchmill cannot safely infer and restore historical +approvals for finished, blocked, or otherwise non-resumable issues; those labels +remain unchanged unless a human reapplies them. + +An issue that carries an approval label but lacks a uniquely resolvable approved +artifact will now fail safely instead of causing Patchmill to generate a new +artifact and remove the approval. This is an intentional tightening of an +inconsistent-state path. + +## Testing strategy + +Automated regression coverage will include: + +1. A required-spec-approval implementation returns unsupported Pi JSON, remains + in progress with `spec-approved`, and resumes to completion without manual + relabeling. +2. The equivalent required-plan-approval failure and resume preserves + `plan-approved`. +3. `spec-approved` with a missing or ambiguous spec fails before Pi or label + mutation. +4. `plan-approved` with a missing or ambiguous plan fails before Pi or label + mutation. +5. Creating an unapproved spec stops at spec review without removing any + approval label. +6. Creating an unapproved plan from an approved spec preserves spec approval and + stops at plan review. +7. Entering implementation and successful completion preserve both approvals. +8. Workflow-state resolution treats `spec-approved + plan-review` as waiting for + plan approval. +9. An existing spec approval still wins over `spec-review`, and an existing plan + approval still wins over `plan-review`. +10. A legacy saved `implementing` run with only `in-progress` bypasses approval + gates and resumes. +11. Development-environment retry behavior does not fabricate approvals. + +The focused run-once tests, full test suite, lint, and build must pass. + +## Expected implementation areas + +The implementation is expected to update: + +- `src/cli/commands/run-once/workflow-state.ts` for state resolution and focused + cleanup behavior; +- planning artifact resolution and source stages for approved-artifact safety + guards; +- `src/cli/commands/run-once/stage-advancement.ts` for artifact-creation guards + and approval-gate bypass on implementation resume; +- `src/cli/commands/run-once/pipeline.ts` and + `src/cli/commands/run-once/pipeline-finish.ts` for implementation and + completion transitions; +- `src/cli/commands/run-once/development-environment-stage.ts` for retry labels; +- run-once unit and scenario tests for required-approval recovery. + +No host-provider API change is expected. diff --git a/nix/package.nix b/nix/package.nix index 9f00404b..c9230b84 100644 --- a/nix/package.nix +++ b/nix/package.nix @@ -3,6 +3,7 @@ buildNpmPackage, nodejs_24, makeWrapper, + git, }: let @@ -30,7 +31,7 @@ buildNpmPackageNode24 rec { dontNpmBuild = true; - nativeBuildInputs = [ makeWrapper ]; + nativeBuildInputs = [ makeWrapper git ]; env = { HUSKY = "0"; diff --git a/site/src/content/docs/reference/workflow-labels.md b/site/src/content/docs/reference/workflow-labels.md index 67abdabe..9d7e5308 100644 --- a/site/src/content/docs/reference/workflow-labels.md +++ b/site/src/content/docs/reference/workflow-labels.md @@ -72,6 +72,16 @@ not triage buckets, so they are not nested under `labels` and are not added to plan-approved label as actionable workflow states. Review labels without matching approved labels are waiting states for human review. +Approved labels are durable facts about the current resolvable artifacts. +Patchmill preserves them through claim, implementation, failure, resume, and +successful completion. Review, ready, in-progress, needs-info, and done labels +continue to represent transient workflow or lifecycle state. + +An approved label requires its corresponding artifact to resolve. If +`spec-approved` has no valid spec, or `plan-approved` has no valid plan, +`run-once` stops with a safety error before claiming the issue or invoking Pi. +Remove approval explicitly before replacing an approved artifact. + ## Approval flows When both spec and plan approval are required: @@ -102,9 +112,9 @@ When neither approval is required: agent-ready --run-once--> write spec, write plan, implement, stop at agent-done ``` -Humans may either replace review labels with approved labels or add approved -labels while leaving review labels in place. Patchmill tolerates both and -removes stale `spec-*` and `plan-*` workflow labels as it advances. +Humans may either replace a review label with its approved label or leave both +in place. Approval wins over review for the same stage; a later-stage review, +such as `plan-review`, wins over durable approval from an earlier stage. `projectPolicy.planRequiresApproval` remains as a compatibility alias. If `workflow.planApproval.required` is omitted, Patchmill derives plan approval diff --git a/site/src/content/docs/using-patchmill/run-once.md b/site/src/content/docs/using-patchmill/run-once.md index b1c8ce1b..b835951b 100644 --- a/site/src/content/docs/using-patchmill/run-once.md +++ b/site/src/content/docs/using-patchmill/run-once.md @@ -85,6 +85,11 @@ Typical gates are: After review, add the configured approved label, such as `spec-approved` or `plan-approved`, then run `patchmill run-once` again. +Patchmill keeps that approval label after the workflow advances. A failed +implementation can therefore resume without asking a human to approve the same +artifact again. If an approved artifact is missing or invalid, restore the +published artifact or explicitly remove approval before creating a replacement. + ## Development environment and implementation If `skills.developmentEnvironment` is configured, Patchmill runs that skill from diff --git a/site/src/content/docs/using-patchmill/workflow-artifacts.md b/site/src/content/docs/using-patchmill/workflow-artifacts.md index 8520505f..2cab42d5 100644 --- a/site/src/content/docs/using-patchmill/workflow-artifacts.md +++ b/site/src/content/docs/using-patchmill/workflow-artifacts.md @@ -59,6 +59,12 @@ file and publish it with `set-spec` or `set-plan`. `plan-approved`, according to the repository workflow policy. 6. Run `patchmill run-once --issue `. +An approval label asserts that the corresponding artifact has been published or +otherwise resolves unambiguously and is the artifact Patchmill must reuse. Do +not apply `spec-approved` before a spec resolves or `plan-approved` before a +plan resolves. Patchmill fails safely rather than synthesizing a replacement for +a missing approved artifact. + `set-spec` and `set-plan` publish file contents to the issue. They do not commit the local files. Commit source spec and plan files through the normal repository workflow when your team wants those files in git. @@ -84,8 +90,10 @@ pre-existing artifacts. ## Updating an artifact -Run `set-spec` or `set-plan` again when a developer revises an artifact before -implementation: +Before revising or replacing an approved spec, withdraw its `spec-approved` +label and any downstream `plan-approved` label. Before revising or replacing an +approved plan, withdraw its `plan-approved` label. Then run `set-spec` or +`set-plan` again when a developer revises an artifact before implementation: ```sh patchmill set-plan --issue 99 docs/plans/log-entries-ui-v2.md diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts new file mode 100644 index 00000000..04d15881 --- /dev/null +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -0,0 +1,572 @@ +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { DEFAULT_PATCHMILL_CONFIG } from "../../../config/defaults.ts"; +import { createCommandRunner } from "../triage/command.ts"; +import { createWorkflowApprovalPolicy } from "../../../workflow/approval-policy.ts"; +import { + assertApprovedArtifactsResolvable, + type ApprovedArtifactPreflightOptions, +} from "./approval-artifact-preflight.ts"; +import type { ResolvedIssueArtifactSource } from "./artifact-sources.ts"; +import type { IssueSummary } from "./types.ts"; + +const now = new Date("2026-08-02T12:00:00Z"); + +async function fixture() { + const repoRoot = await mkdtemp( + join(tmpdir(), "patchmill-approval-preflight-"), + ); + const approvalPolicy = createWorkflowApprovalPolicy({ + ...DEFAULT_PATCHMILL_CONFIG.workflow, + specApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.specApproval, + required: true, + }, + planApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.planApproval, + required: true, + }, + }); + const config: ApprovedArtifactPreflightOptions["config"] = { + repoRoot, + specsDir: join(repoRoot, "docs", "specs"), + plansDir: join(repoRoot, "docs", "plans"), + approvalPolicy, + }; + const issue: IssueSummary = { + number: 140, + title: "Keep approved artifacts", + body: "Approved workflow artifacts", + labels: [], + state: "open", + comments: [], + }; + return { config, issue }; +} + +function source( + repoRoot: string, + kind: "spec" | "plan", +): ResolvedIssueArtifactSource { + const path = + kind === "spec" + ? "docs/specs/approved-design.md" + : "docs/plans/approved-plan.md"; + return { + path, + absolutePath: join(repoRoot, path), + content: `# Approved ${kind}`, + evidence: `approved ${kind} fixture`, + }; +} + +test("approved spec without a resolvable spec fails safely", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*no spec artifact could be resolved/u, + ); +}); + +test("approved plan without a resolvable plan fails safely", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /plan-approved.*no plan artifact could be resolved/u, + ); +}); + +test("resolved approved artifacts pass preflight", async () => { + const { config, issue } = await fixture(); + issue.labels = [ + config.approvalPolicy.specApproval.approvedLabel, + config.approvalPolicy.planApproval.approvedLabel, + ]; + + await assert.doesNotReject( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: { + spec: source(config.repoRoot, "spec"), + plan: source(config.repoRoot, "plan"), + }, + now, + }), + ); +}); + +test("approved spec with multiple discovered specs fails as ambiguous", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + await mkdir(config.specsDir, { recursive: true }); + await writeFile( + join(config.specsDir, "2026-08-01-issue-140-first-design.md"), + "# First spec\n", + "utf8", + ); + await writeFile( + join(config.specsDir, "2026-08-02-issue-140-second-design.md"), + "# Second spec\n", + "utf8", + ); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*multiple spec artifacts/u, + ); +}); + +test("stale saved approved spec does not bypass ambiguous discovery", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + await mkdir(config.specsDir, { recursive: true }); + await writeFile( + join(config.specsDir, "2026-08-01-issue-140-first-design.md"), + "# First spec\n", + "utf8", + ); + await writeFile( + join(config.specsDir, "2026-08-02-issue-140-second-design.md"), + "# Second spec\n", + "utf8", + ); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "planning", + specPath: "docs/specs/stale-design.md", + }, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*multiple spec artifacts/u, + ); +}); + +test("approved published artifact rejects a conflicting local target", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const resolved = source(config.repoRoot, "spec"); + await mkdir(config.specsDir, { recursive: true }); + await writeFile(resolved.absolutePath, "# Different local spec\n", "utf8"); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: { spec: resolved }, + now, + }), + /spec-approved.*would overwrite existing spec artifact/u, + ); +}); + +test("approved published artifact rejects a conflicting saved worktree target", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const resolved = source(config.repoRoot, "spec"); + const worktreePath = "worktrees/issue-140"; + const worktreeArtifactPath = join( + config.repoRoot, + worktreePath, + resolved.path, + ); + await mkdir(join(config.repoRoot, worktreePath, "docs", "specs"), { + recursive: true, + }); + await writeFile(worktreeArtifactPath, "# Stale worktree spec\n", "utf8"); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + worktreePath, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { spec: resolved }, + now, + }), + /spec-approved.*would overwrite existing spec artifact/u, + ); +}); + +test("approved published artifact rejects a conflicting fallback resume target", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const resolved = source(config.repoRoot, "spec"); + const worktreePath = "worktrees/issue-140"; + await mkdir(config.specsDir, { recursive: true }); + await writeFile(resolved.absolutePath, "# Stale fallback spec\n", "utf8"); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + worktreePath, + specPath: resolved.path, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { spec: resolved }, + now, + }), + /spec-approved.*would overwrite existing spec artifact/u, + ); +}); + +test("approved explicit plan passes preflight with a saved spec and no saved plan", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + const worktreePath = "worktrees/issue-140"; + const specPath = "docs/specs/saved-spec.md"; + await mkdir(join(config.repoRoot, worktreePath, "docs", "specs"), { + recursive: true, + }); + await writeFile( + join(config.repoRoot, worktreePath, specPath), + "# Saved spec\n", + "utf8", + ); + + await assert.doesNotReject( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "planning", + worktreePath, + specPath, + specCommit: "saved-spec-commit", + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { plan: source(config.repoRoot, "plan") }, + now, + }), + ); +}); + +test("saved approved spec directory fails preflight", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const specPath = "docs/specs/saved-spec-directory"; + await mkdir(join(config.repoRoot, specPath), { recursive: true }); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "planning", + specPath, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: {}, + now, + }), + /spec-approved.*Saved docs\/specs\/saved-spec-directory is not a regular file/u, + ); +}); + +test("approved branch-only resume resolves saved artifacts in the resolved workspace", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const worktreePath = "worktrees/issue-140"; + const specPath = "docs/specs/saved-spec.md"; + await mkdir(join(config.repoRoot, worktreePath, "docs", "specs"), { + recursive: true, + }); + await writeFile( + join(config.repoRoot, worktreePath, specPath), + "# Saved spec\n", + "utf8", + ); + const preflight = await assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch: "agent/issue-140-keep-approved-artifacts", + specPath, + specCommit: "saved-spec-commit", + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: {}, + now, + artifactWorkspace: { + kind: "worktree", + branch: "agent/issue-140-keep-approved-artifacts", + worktreePath, + }, + }); + assert.equal(preflight?.policy.kind, "implementation-resume"); + assert.equal(preflight?.artifacts.spec.path, specPath); + assert.equal(preflight?.artifacts.spec.exists, true); +}); + +test("approved explicit plan must match a saved implementation resume plan", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + const worktreePath = "worktrees/issue-140"; + const resolved = source(config.repoRoot, "plan"); + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch: "agent/issue-140-keep-approved-artifacts", + worktreePath, + planPath: "docs/plans/saved-plan.md", + planCommit: "saved-plan-commit", + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { plan: resolved }, + now, + }), + /plan-approved.*Explicit plan artifact.*does not match saved plan/u, + ); +}); + +test("approved branch-only resume rejects a missing saved plan even with a matching published source", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + const planPath = "docs/plans/saved-plan.md"; + const calls: string[][] = []; + const runner = { + async run(command: string, args: string[]) { + assert.equal(command, "git"); + calls.push(args); + return { code: 1, stdout: "", stderr: "missing" }; + }, + }; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch: "agent/issue-140-keep-approved-artifacts", + planPath, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { + plan: { + ...source(config.repoRoot, "plan"), + path: planPath, + absolutePath: join(config.repoRoot, planPath), + }, + }, + now, + artifactWorkspace: { + kind: "branch", + branch: "agent/issue-140-keep-approved-artifacts", + }, + runner, + }), + /Saved plan docs\/plans\/saved-plan\.md.*does not exist/u, + ); + assert.deepEqual(calls, [ + [ + "cat-file", + "-t", + "agent/issue-140-keep-approved-artifacts:docs/plans/saved-plan.md", + ], + ]); +}); + +test("approved branch-only resume resolves an existing saved artifact from a real Git branch", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const branch = "agent/issue-140-keep-approved-artifacts"; + const specPath = "docs/specs/saved-spec.md"; + const runner = createCommandRunner(); + + for (const args of [ + ["init"], + ["config", "user.email", "patchmill@example.test"], + ["config", "user.name", "Patchmill Test"], + ]) { + const result = await runner.run("git", args, { cwd: config.repoRoot }); + assert.equal(result.code, 0, result.stderr); + } + await mkdir(join(config.repoRoot, "docs", "specs"), { recursive: true }); + await writeFile(join(config.repoRoot, specPath), "# Saved spec\n", "utf8"); + for (const args of [ + ["add", specPath], + ["commit", "-m", "Add saved spec"], + ["branch", "-M", branch], + ]) { + const result = await runner.run("git", args, { cwd: config.repoRoot }); + assert.equal(result.code, 0, result.stderr); + } + + const preflight = await assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch, + specPath, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: {}, + now, + artifactWorkspace: { kind: "branch", branch }, + runner, + }); + + assert.equal(preflight?.artifacts.spec.path, specPath); + assert.equal(preflight?.artifacts.spec.exists, true); +}); + +test("approved branch-only resume rejects a saved directory", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; + const specPath = "docs/specs/saved-spec-directory"; + const runner = { + async run(command: string, args: string[]) { + assert.equal(command, "git"); + assert.deepEqual(args, [ + "cat-file", + "-t", + "agent/issue-140-keep-approved-artifacts:docs/specs/saved-spec-directory", + ]); + return { code: 0, stdout: "tree\n", stderr: "" }; + }, + }; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch: "agent/issue-140-keep-approved-artifacts", + specPath, + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: {}, + now, + artifactWorkspace: { + kind: "branch", + branch: "agent/issue-140-keep-approved-artifacts", + }, + runner, + }), + /Saved spec docs\/specs\/saved-spec-directory is not a regular file/u, + ); +}); + +test("approved branch-only resume rejects an explicit artifact that differs from saved identity", async () => { + const { config, issue } = await fixture(); + issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; + const worktreePath = "worktrees/issue-140"; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + existingState: { + issueNumber: issue.number, + title: issue.title, + status: "implementing", + branch: "agent/issue-140-keep-approved-artifacts", + planPath: "docs/plans/saved-plan.md", + planCommit: "saved-plan-commit", + createdAt: now.toISOString(), + updatedAt: now.toISOString(), + }, + resolvedArtifacts: { plan: source(config.repoRoot, "plan") }, + now, + artifactWorkspace: { + kind: "worktree", + branch: "agent/issue-140-keep-approved-artifacts", + worktreePath, + }, + }), + /plan-approved.*Explicit plan artifact.*does not match saved plan/u, + ); +}); + +test("preflight uses configured approval label names", async () => { + const { config, issue } = await fixture(); + config.approvalPolicy = createWorkflowApprovalPolicy({ + ...DEFAULT_PATCHMILL_CONFIG.workflow, + specApproval: { + ...DEFAULT_PATCHMILL_CONFIG.workflow.specApproval, + required: true, + approvedLabel: "spec-reviewed", + }, + }); + issue.labels = ["spec-reviewed"]; + + await assert.rejects( + assertApprovedArtifactsResolvable({ + config, + issue, + resolvedArtifacts: {}, + now, + }), + /spec-reviewed.*no spec artifact could be resolved/u, + ); +}); diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts new file mode 100644 index 00000000..5deab226 --- /dev/null +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -0,0 +1,340 @@ +import { join } from "node:path"; +import { + assertIssueArtifactSourcesMaterializable, + assertIssueArtifactSourcesMaterializableInBranch, +} from "./artifact-source-materialization.ts"; +import type { + ResolvedIssueArtifactSource, + ResolvedIssueArtifactSources, +} from "./artifact-sources.ts"; +import { + PlanningArtifactSafetyError, + planningArtifactRoot, + resolveApprovedPlanningArtifacts, + type PlanningArtifactPolicy, + type ResolvedPlanningArtifacts, +} from "./planning-artifacts.ts"; +import { + freshPlanningArtifactPolicy, + hasSavedPlanningArtifactWorkspace, + planningArtifactPolicyForWorkspace, +} from "./pipeline-workspace.ts"; +import type { + AgentIssueConfig, + AgentIssueRunState, + CommandRunner, + IssueSummary, +} from "./types.ts"; +import type { ReadOnlyIssueWorkspace } from "./git.ts"; + +export type ApprovedArtifactPreflightOptions = { + config: Pick< + AgentIssueConfig, + "repoRoot" | "specsDir" | "plansDir" | "approvalPolicy" + >; + issue: IssueSummary; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + now: Date; + artifactWorkspace?: ReadOnlyIssueWorkspace; + runner?: CommandRunner; +}; + +export type ApprovedArtifactPreflight = { + policy: PlanningArtifactPolicy; + artifacts: ResolvedPlanningArtifacts; +}; + +async function approvedArtifactPolicy(input: { + options: ApprovedArtifactPreflightOptions; + requireSpec: boolean; + requirePlan: boolean; + resolvedArtifacts: ResolvedIssueArtifactSources; +}): Promise { + const { options, requireSpec, requirePlan, resolvedArtifacts } = input; + const { config, existingState } = options; + const hasApprovedSource = + (requireSpec && !!resolvedArtifacts.spec) || + (requirePlan && !!resolvedArtifacts.plan); + const needsSavedWorkspace = hasSavedPlanningArtifactWorkspace(existingState); + + if (needsSavedWorkspace || hasApprovedSource) { + if (options.artifactWorkspace?.kind === "worktree") { + return planningArtifactPolicyForWorkspace({ + config, + existingState, + resolvedArtifacts, + worktreePath: options.artifactWorkspace.worktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } + + if (!options.artifactWorkspace && existingState?.worktreePath) { + return planningArtifactPolicyForWorkspace({ + config, + existingState, + resolvedArtifacts, + worktreePath: existingState.worktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } + } + + return freshPlanningArtifactPolicy({ + config, + existingState: + options.artifactWorkspace && existingState + ? { ...existingState, worktreePath: undefined } + : existingState, + resolvedArtifacts, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); +} + +function assertExplicitMatchesSaved(input: { + kind: "spec" | "plan"; + explicit?: ResolvedIssueArtifactSource; + savedPath?: string; + savedCommit?: string; +}): void { + if (!input.explicit || !input.savedPath) return; + if (input.explicit.path !== input.savedPath) { + throw new PlanningArtifactSafetyError( + `Explicit ${input.kind} artifact ${input.explicit.path} does not match saved ${input.kind} ${input.savedPath}`, + ); + } + if ( + input.explicit.commit && + input.savedCommit && + input.explicit.commit !== input.savedCommit + ) { + throw new PlanningArtifactSafetyError( + `Explicit ${input.kind} artifact commit ${input.explicit.commit} does not match saved ${input.kind} commit ${input.savedCommit}`, + ); + } +} + +async function branchSavedArtifact(input: { + options: ApprovedArtifactPreflightOptions; + kind: "spec" | "plan"; + required: boolean; +}): Promise { + const { options, kind, required } = input; + if (!required || options.artifactWorkspace?.kind !== "branch") { + return undefined; + } + if (!options.runner) { + throw new PlanningArtifactSafetyError( + "Approved branch preflight requires a command runner", + ); + } + const path = + kind === "spec" + ? options.existingState?.specPath + : options.existingState?.planPath; + if (!path) return undefined; + const commit = + kind === "spec" + ? options.existingState?.specCommit + : options.existingState?.planCommit; + const explicit = options.resolvedArtifacts[kind]; + assertExplicitMatchesSaved({ + kind, + explicit, + savedPath: path, + savedCommit: commit, + }); + + const object = `${options.artifactWorkspace.branch}:${path}`; + const type = await options.runner.run("git", ["cat-file", "-t", object], { + cwd: options.config.repoRoot, + }); + if (type.code === 1 || type.code === 128) { + throw new PlanningArtifactSafetyError( + `Saved ${kind} ${path} does not exist on issue branch ${options.artifactWorkspace.branch}`, + ); + } + if (type.code !== 0) { + throw new PlanningArtifactSafetyError( + `git cat-file failed while resolving saved ${kind} ${path} from ${options.artifactWorkspace.branch}`, + ); + } + if (type.stdout.trim() !== "blob") { + throw new PlanningArtifactSafetyError( + `Saved ${kind} ${path} is not a regular file on issue branch ${options.artifactWorkspace.branch}`, + ); + } + const content = await options.runner.run("git", ["show", object], { + cwd: options.config.repoRoot, + }); + if (content.code !== 0) { + throw new PlanningArtifactSafetyError( + `git show failed while resolving saved ${kind} ${path} from ${options.artifactWorkspace.branch}`, + ); + } + return { + path, + absolutePath: join(options.config.repoRoot, path), + content: content.stdout, + evidence: `saved ${kind} from ${options.artifactWorkspace.branch}`, + ...(commit ? { commit } : {}), + }; +} + +function missingApprovedArtifact( + issue: IssueSummary, + label: string, + kind: "spec" | "plan", +): PlanningArtifactSafetyError { + return new PlanningArtifactSafetyError( + `Issue #${issue.number} has approval label ${label}, but no ${kind} artifact could be resolved; remove ${label} before creating a new ${kind}`, + ); +} + +async function assertApprovedSourcesMaterializable(input: { + issue: IssueSummary; + policy: PlanningArtifactPolicy; + artifacts: ResolvedPlanningArtifacts; + sources: ResolvedIssueArtifactSources; + requireSpec: boolean; + requirePlan: boolean; + specLabel: string; + planLabel: string; + artifactWorkspace?: ReadOnlyIssueWorkspace; + runner?: CommandRunner; +}): Promise { + const approved = [ + ...(input.requireSpec && input.sources.spec + ? [ + { + kind: "spec" as const, + source: input.sources.spec, + artifact: input.artifacts.spec, + label: input.specLabel, + }, + ] + : []), + ...(input.requirePlan && input.sources.plan + ? [ + { + kind: "plan" as const, + source: input.sources.plan, + artifact: input.artifacts.plan, + label: input.planLabel, + }, + ] + : []), + ]; + + for (const entry of approved) { + try { + const sources = { [entry.kind]: entry.source }; + if (input.artifactWorkspace?.kind === "branch") { + if (!input.runner) { + throw new Error( + "Approved branch preflight requires a command runner", + ); + } + await assertIssueArtifactSourcesMaterializableInBranch({ + repoRoot: input.policy.primary.repoRoot, + runner: input.runner, + branch: input.artifactWorkspace.branch, + issueNumber: input.issue.number, + sources, + }); + } else { + await assertIssueArtifactSourcesMaterializable({ + repoRoot: planningArtifactRoot(input.policy, entry.artifact).repoRoot, + issueNumber: input.issue.number, + sources, + }); + } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new PlanningArtifactSafetyError( + `Issue #${input.issue.number} has approval label ${entry.label}, but its approved artifact cannot be materialized: ${message}`, + ); + } + } +} + +export async function assertApprovedArtifactsResolvable( + options: ApprovedArtifactPreflightOptions, +): Promise { + const specLabel = options.config.approvalPolicy.specApproval.approvedLabel; + const planLabel = options.config.approvalPolicy.planApproval.approvedLabel; + const requireSpec = options.issue.labels.includes(specLabel); + const requirePlan = options.issue.labels.includes(planLabel); + if (!requireSpec && !requirePlan) return undefined; + + const branchSpec = await branchSavedArtifact({ + options, + kind: "spec", + required: requireSpec, + }); + const branchPlan = await branchSavedArtifact({ + options, + kind: "plan", + required: requirePlan, + }); + const branchSavedArtifacts: ResolvedIssueArtifactSources = { + ...(branchSpec ? { spec: branchSpec } : {}), + ...(branchPlan ? { plan: branchPlan } : {}), + }; + const preflightArtifacts = { + ...options.resolvedArtifacts, + ...branchSavedArtifacts, + }; + const policy = await approvedArtifactPolicy({ + options, + requireSpec, + requirePlan, + resolvedArtifacts: preflightArtifacts, + }); + let artifacts: ResolvedPlanningArtifacts; + try { + artifacts = await resolveApprovedPlanningArtifacts({ + policy, + issue: options.issue, + now: options.now, + requireSpec, + requirePlan, + }); + } catch (error) { + if (error instanceof PlanningArtifactSafetyError) { + const labels = [ + ...(requireSpec ? [specLabel] : []), + ...(requirePlan ? [planLabel] : []), + ].join(", "); + throw new PlanningArtifactSafetyError( + `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts could not be resolved: ${error.message}`, + ); + } + throw error; + } + + if (requireSpec && !artifacts.spec.exists) { + throw missingApprovedArtifact(options.issue, specLabel, "spec"); + } + if (requirePlan && !artifacts.plan.exists) { + throw missingApprovedArtifact(options.issue, planLabel, "plan"); + } + + await assertApprovedSourcesMaterializable({ + issue: options.issue, + policy, + artifacts, + sources: options.resolvedArtifacts, + requireSpec, + requirePlan, + specLabel, + planLabel, + artifactWorkspace: options.artifactWorkspace, + runner: options.runner, + }); + + return { policy, artifacts }; +} diff --git a/src/cli/commands/run-once/artifact-source-materialization.ts b/src/cli/commands/run-once/artifact-source-materialization.ts index eec8f9bd..75396fb1 100644 --- a/src/cli/commands/run-once/artifact-source-materialization.ts +++ b/src/cli/commands/run-once/artifact-source-materialization.ts @@ -47,24 +47,25 @@ async function existingContent(path: string): Promise { } } -export async function materializeIssueArtifactSources( - options: MaterializeIssueArtifactSourcesOptions, -): Promise { - const entries = artifactEntries(options.sources); - if (entries.length === 0) return options.sources; +type ArtifactWrite = { + entry: ArtifactEntry; + content: string; +}; - const writes: Array<{ - entry: ArtifactEntry; - content: string; - }> = []; - for (const entry of entries) { +async function materializationWrites(input: { + repoRoot: string; + issueNumber: number; + sources: ResolvedIssueArtifactSources; +}): Promise { + const writes: ArtifactWrite[] = []; + for (const entry of artifactEntries(input.sources)) { const content = withTrailingNewline(entry.source.content); - const absolutePath = resolve(options.repoRoot, entry.source.path); + const absolutePath = resolve(input.repoRoot, entry.source.path); const existing = await existingContent(absolutePath); if (existing !== undefined) { if (existing !== content) { throw new Error( - `Issue #${options.issueNumber} artifact would overwrite existing ${entry.kind} artifact at ${entry.source.path}`, + `Issue #${input.issueNumber} artifact would overwrite existing ${entry.kind} artifact at ${entry.source.path}`, ); } continue; @@ -77,6 +78,59 @@ export async function materializeIssueArtifactSources( content, }); } + return writes; +} + +export async function assertIssueArtifactSourcesMaterializable(input: { + repoRoot: string; + issueNumber: number; + sources: ResolvedIssueArtifactSources; +}): Promise { + await materializationWrites(input); +} + +export async function assertIssueArtifactSourcesMaterializableInBranch(input: { + repoRoot: string; + runner: CommandRunner; + branch: string; + issueNumber: number; + sources: ResolvedIssueArtifactSources; +}): Promise { + for (const entry of artifactEntries(input.sources)) { + const object = `${input.branch}:${entry.source.path}`; + const exists = await input.runner.run("git", ["cat-file", "-e", object], { + cwd: input.repoRoot, + }); + if (exists.code === 1 || exists.code === 128) continue; + if (exists.code !== 0) { + throw new Error( + `git cat-file failed while checking issue #${input.issueNumber} ${entry.kind} artifact on ${input.branch}: ${commandOutput(exists)}`, + ); + } + + const content = await input.runner.run("git", ["show", object], { + cwd: input.repoRoot, + }); + if (content.code !== 0) { + throw new Error( + `git show failed while checking issue #${input.issueNumber} ${entry.kind} artifact on ${input.branch}: ${commandOutput(content)}`, + ); + } + if (content.stdout !== withTrailingNewline(entry.source.content)) { + throw new Error( + `Issue #${input.issueNumber} artifact would overwrite existing ${entry.kind} artifact at ${entry.source.path}`, + ); + } + } +} + +export async function materializeIssueArtifactSources( + options: MaterializeIssueArtifactSourcesOptions, +): Promise { + const entries = artifactEntries(options.sources); + if (entries.length === 0) return options.sources; + + const writes = await materializationWrites(options); for (const { entry, content } of writes) { await mkdir(dirname(entry.source.absolutePath), { recursive: true }); diff --git a/src/cli/commands/run-once/artifacts.ts b/src/cli/commands/run-once/artifacts.ts index 55e398fc..1b7d3944 100644 --- a/src/cli/commands/run-once/artifacts.ts +++ b/src/cli/commands/run-once/artifacts.ts @@ -44,25 +44,29 @@ export function buildArtifactPath( ); } -export async function findIssueArtifact( +export async function findIssueArtifacts( artifactDir: string, issueNumber: number, -): Promise { +): Promise { let entries; try { entries = await readdir(artifactDir, { withFileTypes: true }); } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") { - return undefined; - } + if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; throw error; } const marker = `-issue-${issueNumber}-`; - const match = entries + return entries .filter((entry) => entry.isFile() && entry.name.includes(marker)) .map((entry) => entry.name) - .sort((left, right) => left.localeCompare(right))[0]; + .sort((left, right) => left.localeCompare(right)) + .map((entry) => join(artifactDir, entry)); +} - return match ? join(artifactDir, match) : undefined; +export async function findIssueArtifact( + artifactDir: string, + issueNumber: number, +): Promise { + return (await findIssueArtifacts(artifactDir, issueNumber))[0]; } diff --git a/src/cli/commands/run-once/development-environment-stage.ts b/src/cli/commands/run-once/development-environment-stage.ts index a0a2a921..ee68a3ee 100644 --- a/src/cli/commands/run-once/development-environment-stage.ts +++ b/src/cli/commands/run-once/development-environment-stage.ts @@ -119,6 +119,8 @@ async function developmentEnvironmentNotReady( specCommit: options.specCommit, planPath: options.planPath, planCommit: options.planCommit, + branch: options.branch, + worktreePath: options.worktreePath, lastError: result.reason, }, timestamp, diff --git a/src/cli/commands/run-once/git.ts b/src/cli/commands/run-once/git.ts index 1734a166..15227e0d 100644 --- a/src/cli/commands/run-once/git.ts +++ b/src/cli/commands/run-once/git.ts @@ -15,6 +15,11 @@ export type BaseBranchDetectionResult = | { status: "detected"; branch: string; source: "remote-head" | "upstream" } | { status: "fallback"; branch: string; reason: string }; +export type ReadOnlyIssueWorkspace = + | { kind: "worktree"; branch: string; worktreePath: string } + | { kind: "branch"; branch: string } + | { kind: "base" }; + function resolveStrategy( strategyOrBaseRef: | GitWorktreeStrategyConfig @@ -455,11 +460,83 @@ async function existingCommitLines( .filter(Boolean); } -function porcelainWorktreePaths(stdout: string): string[] { +type PorcelainWorktree = { + path: string; + branch?: string; +}; + +function porcelainWorktrees(stdout: string): PorcelainWorktree[] { return stdout - .split("\n") - .filter((line) => line.startsWith("worktree ")) - .map((line) => resolve(line.slice("worktree ".length).trim())); + .trim() + .split("\n\n") + .flatMap((record) => { + const lines = record.split("\n"); + const worktree = lines.find((line) => line.startsWith("worktree ")); + if (!worktree) return []; + const branch = lines + .find((line) => line.startsWith("branch refs/heads/")) + ?.slice("branch refs/heads/".length); + return [ + { + path: resolve(worktree.slice("worktree ".length).trim()), + ...(branch ? { branch } : {}), + }, + ]; + }); +} + +function porcelainWorktreePaths(stdout: string): string[] { + return porcelainWorktrees(stdout).map((worktree) => worktree.path); +} + +export async function inspectIssueWorkspace( + runner: CommandRunner, + repoRoot: string, + expected: { branch: string; worktreePath: string }, +): Promise { + const listed = await commandOutput( + runner, + repoRoot, + ["worktree", "list", "--porcelain"], + "git worktree list failed", + ); + const expectedWorktreePath = resolve(repoRoot, expected.worktreePath); + const worktrees = porcelainWorktrees(listed); + const branchWorktree = worktrees.find( + (worktree) => + worktree.branch === expected.branch && + worktree.path !== expectedWorktreePath, + ); + if (branchWorktree) { + throw new Error( + `Issue branch ${expected.branch} is already checked out at ${branchWorktree.path}; expected ${expected.worktreePath}`, + ); + } + if (!worktrees.some((worktree) => worktree.path === expectedWorktreePath)) { + return (await branchExists(runner, repoRoot, expected.branch)) + ? { kind: "branch", branch: expected.branch } + : { kind: "base" }; + } + + const currentBranch = ( + await commandOutput( + runner, + repoRoot, + ["-C", expected.worktreePath, "branch", "--show-current"], + `git branch failed for ${expected.worktreePath}`, + ) + ).trim(); + if (currentBranch !== expected.branch) { + throw new Error( + `Existing worktree ${expected.worktreePath} is on ${currentBranch}, expected ${expected.branch}`, + ); + } + + return { + kind: "worktree", + branch: expected.branch, + worktreePath: expected.worktreePath, + }; } async function pathExists(path: string): Promise { diff --git a/src/cli/commands/run-once/paths.ts b/src/cli/commands/run-once/paths.ts index 3ef15a14..74e11965 100644 --- a/src/cli/commands/run-once/paths.ts +++ b/src/cli/commands/run-once/paths.ts @@ -1,4 +1,4 @@ -import { access } from "node:fs/promises"; +import { access, stat } from "node:fs/promises"; function isMissingPathError(error: unknown): boolean { return (error as NodeJS.ErrnoException).code === "ENOENT"; @@ -19,3 +19,14 @@ export async function pathExists(path: string): Promise { }); } } + +export async function pathIsRegularFile(path: string): Promise { + try { + return (await stat(path)).isFile(); + } catch (error) { + if (isMissingPathError(error)) return false; + throw new Error(`Failed to inspect path ${path}: ${errorMessage(error)}`, { + cause: error, + }); + } +} diff --git a/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts b/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts new file mode 100644 index 00000000..31f96650 --- /dev/null +++ b/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts @@ -0,0 +1,608 @@ +import assert from "node:assert/strict"; +import { mkdir, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import test from "node:test"; +import { runOneIssue } from "./pipeline.ts"; +import { formatPublishedArtifactComment } from "../../../workflow/artifacts/published-artifacts.ts"; +import { writeRunState } from "./run-state.ts"; +import { + issue, + issueListPayload, + labelListPayload, +} from "../../../../test-support/run-once/issue-fixtures.ts"; +import { + createMockRunner, + workflowPiCalls, +} from "../../../../test-support/run-once/mock-runner.ts"; +import { + makeConfig, + specAndPlanApprovalPolicy, +} from "../../../../test-support/run-once/pipeline-fixtures.ts"; + +const NOW = new Date("2026-05-09T12:00:00.000Z"); + +test("runOneIssue rejects an unregistered stale approved resume worktree before mutation", async () => { + const config = await makeConfig({ + dryRun: false, + execute: true, + issueNumber: 45, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const planPath = "docs/plans/2026-05-14-issue-45-stale-worktree.md"; + const worktreePath = ".worktrees/patchmill-issue-45-stale-worktree"; + await mkdir(join(config.repoRoot, worktreePath, "docs", "plans"), { + recursive: true, + }); + await writeFile(join(config.repoRoot, worktreePath, planPath), "# plan\n"); + await writeRunState( + config.runStateDir, + { + issueNumber: 45, + title: "Stale worktree", + status: "implementing", + planPath, + branch: "agent/issue-45-stale-worktree", + worktreePath, + }, + NOW.toISOString(), + ); + const selected = issue( + 45, + ["in-progress", "plan-approved"], + "Stale worktree", + ); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /plan-approved.*no plan artifact could be resolved/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); +}); + +test("runOneIssue rejects an approved resume worktree on the wrong branch before mutation", async () => { + const config = await makeConfig({ + dryRun: false, + execute: true, + issueNumber: 45, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const planPath = "docs/plans/2026-05-14-issue-45-wrong-branch.md"; + const worktreePath = ".worktrees/patchmill-issue-45-wrong-branch"; + await mkdir(join(config.repoRoot, worktreePath, "docs", "plans"), { + recursive: true, + }); + await writeFile(join(config.repoRoot, worktreePath, planPath), "# plan\n"); + await writeRunState( + config.runStateDir, + { + issueNumber: 45, + title: "Wrong branch", + status: "implementing", + planPath, + branch: "agent/issue-45-wrong-branch", + worktreePath, + }, + NOW.toISOString(), + ); + const selected = issue(45, ["in-progress", "plan-approved"], "Wrong branch"); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { + code: 0, + stdout: `worktree ${join(config.repoRoot, worktreePath)}\n`, + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "-C" && + call.args[2] === "branch" + ) { + return { code: 0, stdout: "agent/other-issue\n", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /Existing worktree .* is on agent\/other-issue, expected agent\/issue-45-wrong-branch/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); +}); + +test("runOneIssue resumes an approved branch-only saved plan", async () => { + const issueNumber = 68; + const title = "Branch-only saved plan"; + const planPath = "docs/plans/2026-05-14-issue-68-branch-only-saved-plan.md"; + const config = await makeConfig({ + dryRun: false, + execute: true, + planOnly: true, + issueNumber, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const branch = "agent/issue-68-branch-only-saved-plan"; + const worktreePath = ".worktrees/patchmill-issue-68-branch-only-saved-plan"; + await writeRunState( + config.runStateDir, + { + issueNumber, + title, + status: "implementing", + planPath, + branch, + worktreePath, + checkpoints: { + claimed: true, + startedCommentPosted: true, + planPathResolved: true, + }, + }, + NOW.toISOString(), + ); + const selected = issue(issueNumber, ["in-progress", "plan-approved"], title); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "cat-file") { + return { + code: 0, + stdout: call.args[1] === "-t" ? "blob\n" : "", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "show") { + return { code: 0, stdout: "# plan\n", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add" + ) { + await mkdir(join(config.repoRoot, worktreePath, "docs", "plans"), { + recursive: true, + }); + await writeFile( + join(config.repoRoot, worktreePath, planPath), + "# plan\n", + ); + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "-C" && + call.args[2] === "branch" + ) { + return { code: 0, stdout: `${branch}\n`, stderr: "" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "log") { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "pi") { + return { + code: 0, + stdout: JSON.stringify({ + status: "pr-created", + prUrl: "https://forgejo/pr/68", + branch, + commits: ["abc123"], + validation: ["focused test passed"], + }), + stderr: "", + }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + const result = await runOneIssue(runner, config, { now: NOW }); + + assert.equal(result.status, "pr-created", JSON.stringify(result)); + assert.equal((await workflowPiCalls(runner.calls)).length, 1); + const worktreeAdd = runner.calls.findIndex( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ); + const firstPi = runner.calls.findIndex((call) => call.command === "pi"); + assert.ok(worktreeAdd >= 0); + assert.ok(worktreeAdd < firstPi); +}); + +test("runOneIssue rejects a missing branch saved plan before mutation despite a matching published source", async () => { + const issueNumber = 70; + const title = "Missing branch saved plan"; + const planPath = + "docs/plans/2026-05-14-issue-70-missing-branch-saved-plan.md"; + const branch = "agent/issue-70-missing-branch-saved-plan"; + const config = await makeConfig({ + dryRun: false, + execute: true, + planOnly: true, + issueNumber, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + await writeRunState( + config.runStateDir, + { + issueNumber, + title, + status: "implementing", + branch, + planPath, + worktreePath: ".worktrees/patchmill-issue-70-missing-branch-saved-plan", + checkpoints: { + claimed: true, + startedCommentPosted: true, + planPathResolved: true, + }, + }, + NOW.toISOString(), + ); + const selected = issue(issueNumber, ["in-progress", "plan-approved"], title); + selected.comments = [ + { + author: { login: "patchmill-bot" }, + body: formatPublishedArtifactComment({ + kind: "plan", + path: planPath, + content: "# Matching published plan\n", + }), + }, + ]; + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if (call.command === "tea" && call.args[0] === "logins") { + return { + code: 0, + stdout: JSON.stringify([ + { name: "default", user: "patchmill-bot", default: true }, + ]), + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "cat-file") { + return { code: 1, stdout: "", stderr: "missing" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /Saved plan .* does not exist on issue branch/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); +}); + +test("runOneIssue rejects an issue branch already checked out elsewhere before mutation", async () => { + const config = await makeConfig({ + dryRun: false, + execute: true, + issueNumber: 69, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const title = "Branch checked out elsewhere"; + const branch = "agent/issue-69-branch-checked-out-elsewhere"; + const selected = issue(69, ["plan-approved"], title); + const otherWorktree = join(config.repoRoot, ".worktrees", "other-issue"); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { + code: 0, + stdout: `worktree ${otherWorktree}\nbranch refs/heads/${branch}\n\n`, + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /already checked out at .*other-issue/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); +}); + +test("runOneIssue rejects duplicate expected branch worktree registrations before mutation", async () => { + const issueNumber = 71; + const title = "Duplicate branch worktrees"; + const branch = "agent/issue-71-duplicate-branch-worktrees"; + const config = await makeConfig({ + dryRun: false, + execute: true, + issueNumber, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const expectedPath = join( + config.repoRoot, + ".worktrees/patchmill-issue-71-duplicate-branch-worktrees", + ); + const otherPath = join(config.repoRoot, ".worktrees/duplicate-checkout"); + const selected = issue(issueNumber, ["plan-approved"], title); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { + code: 0, + stdout: `worktree ${expectedPath}\nbranch refs/heads/${branch}\n\nworktree ${otherPath}\nbranch refs/heads/${branch}\n\n`, + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /already checked out at .*duplicate-checkout/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); +}); diff --git a/src/cli/commands/run-once/pipeline-development-environment.test.ts b/src/cli/commands/run-once/pipeline-development-environment.test.ts index 85a1b855..b6881164 100644 --- a/src/cli/commands/run-once/pipeline-development-environment.test.ts +++ b/src/cli/commands/run-once/pipeline-development-environment.test.ts @@ -1,9 +1,9 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { readFile, writeFile } from "node:fs/promises"; +import { mkdir, readFile, unlink, writeFile } from "node:fs/promises"; import { basename, dirname, join } from "node:path"; import { DEFAULT_PATCHMILL_CONFIG } from "../../../config/defaults.ts"; -import { runStatePath, writeRunState } from "./run-state.ts"; +import { readRunState, runStatePath, writeRunState } from "./run-state.ts"; import { runOneIssue } from "./pipeline.ts"; import { issue, @@ -269,6 +269,7 @@ test("runOneIssue returns development-environment-not-ready without starting imp const { result, runner } = await runPlanApprovedImplementationScenario({ issueNumber: 47, title: "Not ready", + issueLabels: ["agent-ready"], planPath: "docs/plans/2026-05-14-issue-47-not-ready.md", configOverrides: { skills: { @@ -321,7 +322,7 @@ test("runOneIssue returns development-environment-not-ready without starting imp .at(-1); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--add-labels") + 1], - "plan-approved", + "agent-ready", ); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--remove-labels") + 1], @@ -335,6 +336,7 @@ test("runOneIssue preserves approval labels after development environment failur issueNumber: 49, title: "Approved but not ready", issueLabels: ["spec-approved", "plan-approved"], + specPath: "docs/specs/2026-05-14-issue-49-approved-not-ready-design.md", planPath: "docs/plans/2026-05-14-issue-49-approved-not-ready.md", configOverrides: { approvalPolicy: specAndPlanApprovalPolicy(), @@ -367,10 +369,150 @@ test("runOneIssue preserves approval labels after development environment failur call.args[1] === "edit", ) .at(-1); + assert.equal(finalEdit?.args.includes("--add-labels"), false); assert.equal( - finalEdit?.args[finalEdit.args.indexOf("--add-labels") + 1], - "spec-approved,plan-approved", + finalEdit?.args[finalEdit.args.indexOf("--remove-labels") + 1], + "in-progress", ); +}); + +test("runOneIssue retries worktree-only approved artifacts after development environment failure", async () => { + const issueNumber = 60; + const title = "Retry worktree approved plan"; + const planPath = + "docs/plans/2026-05-14-issue-60-retry-worktree-approved-plan.md"; + const config = await makeConfig({ + dryRun: false, + execute: true, + approvalPolicy: specAndPlanApprovalPolicy(), + skills: { + ...DEFAULT_PATCHMILL_CONFIG.skills, + developmentEnvironment: "./skills/development-environment", + }, + }); + await writeFile(join(config.repoRoot, planPath), "# plan\n", "utf8"); + const selected = issue(issueNumber, ["plan-approved"], title); + const worktreePath = + ".worktrees/patchmill-issue-60-retry-worktree-approved-plan"; + const branch = "agent/issue-60-retry-worktree-approved-plan"; + const worktreeRoot = join(config.repoRoot, worktreePath); + let workspaceCreated = false; + let developmentEnvironmentRuns = 0; + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { + code: 0, + stdout: workspaceCreated ? `worktree ${worktreeRoot}\n` : "", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 1, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add" + ) { + workspaceCreated = true; + await mkdir(dirname(join(worktreeRoot, planPath)), { recursive: true }); + await writeFile( + join(worktreeRoot, planPath), + "# worktree plan\n", + "utf8", + ); + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "-C" && + call.args[2] === "branch" + ) { + return { code: 0, stdout: `${branch}\n`, stderr: "" }; + } + if (call.command === "git" && call.args[0] === "log") { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "pi") { + const prompt = await readFile(promptPath(call.args), "utf8"); + assert.match(prompt, /Prepare development environment/); + developmentEnvironmentRuns += 1; + return { + code: 0, + stdout: JSON.stringify({ + status: "not-ready", + reason: "Service unavailable", + evidence: ["service check failed"], + remediation: ["Start the service", "Re-run patchmill"], + }), + stderr: "", + }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + const first = await runOneIssue(runner, config, { now: NOW }); + assert.equal(first.status, "development-environment-not-ready"); + const failedState = await readRunState(config.runStateDir, issueNumber); + assert.equal(failedState?.branch, branch); + assert.equal(failedState?.worktreePath, worktreePath); + await unlink(join(config.repoRoot, planPath)); + + const second = await runOneIssue(runner, config, { now: NOW }); + assert.equal(second.status, "development-environment-not-ready"); + assert.equal(developmentEnvironmentRuns, 2); + assert.equal( + runner.calls.filter( + (call) => + call.command === "git" && + call.args[0] === "-C" && + call.args[2] === "branch", + ).length, + 2, + ); + const finalEdit = runner.calls + .filter( + (call) => + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "edit", + ) + .at(-1); + assert.equal(finalEdit?.args.includes("--add-labels"), false); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--remove-labels") + 1], "in-progress", @@ -382,6 +524,7 @@ test("runOneIssue restores a retryable label after resumed development environme const config = await makeConfig({ dryRun: false, execute: true, + approvalPolicy: specAndPlanApprovalPolicy(), skills: { ...DEFAULT_PATCHMILL_CONFIG.skills, developmentEnvironment: "./skills/development-environment", @@ -492,7 +635,7 @@ test("runOneIssue restores a retryable label after resumed development environme .at(-1); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--add-labels") + 1], - "plan-approved", + "agent-ready", ); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--remove-labels") + 1], diff --git a/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts b/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts index 10b5da5a..64133e6c 100644 --- a/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts @@ -16,7 +16,10 @@ import { createMockRunner, promptPath, } from "../../../../test-support/run-once/mock-runner.ts"; -import { makeConfig } from "../../../../test-support/run-once/pipeline-fixtures.ts"; +import { + approvalPolicy, + makeConfig, +} from "../../../../test-support/run-once/pipeline-fixtures.ts"; import { collectProgressEvents, commentBody, @@ -884,12 +887,21 @@ test("runOneIssue records and comments unexpected planning failures without repl }); test("runOneIssue records and comments unexpected implementation failures without replacing in-progress", async () => { - const config = await makeConfig({ dryRun: false, execute: true }); + const config = await makeConfig({ + dryRun: false, + execute: true, + approvalPolicy: approvalPolicy({ specRequired: true }), + }); const selected = issue( 42, - ["agent-ready", "enhancement"], + ["spec-review", "spec-approved", "enhancement"], "Handle implementation parse failure", ); + const existingSpecPath = join( + config.specsDir, + "2026-05-01-issue-42-handle-implementation-parse-failure-design.md", + ); + await writeFile(existingSpecPath, "# spec\n", "utf8"); const existingPlanPath = join( config.plansDir, "2026-05-01-issue-42-handle-implementation-parse-failure.md", @@ -954,7 +966,11 @@ test("runOneIssue records and comments unexpected implementation failures withou call.args[0] === "issues" && call.args[1] === "edit", ); - assert.equal(editCalls.length, 1); + const removedLabels = editCalls.flatMap((call) => { + const index = call.args.indexOf("--remove-labels"); + return index < 0 ? [] : (call.args[index + 1]?.split(",") ?? []); + }); + assert.equal(removedLabels.includes("spec-approved"), false); const failureComment = runner.calls .filter((call) => call.command === "tea" && call.args[0] === "comment") @@ -996,7 +1012,7 @@ test("runOneIssue records and comments unexpected implementation failures withou ? issueListPayload([ issue( 42, - ["in-progress", "enhancement"], + ["in-progress", "spec-approved", "enhancement"], "Handle implementation parse failure", ), issue(100, ["agent-ready", "bug"], "Do not select me either"), @@ -1063,6 +1079,162 @@ test("runOneIssue records and comments unexpected implementation failures withou assert.equal(resumed.issue.number, 42); }); +test("runOneIssue resumes required plan approval after unexpected implementation failure", async () => { + const config = await makeConfig({ + dryRun: false, + execute: true, + approvalPolicy: approvalPolicy({ planRequired: true }), + }); + const selected = issue( + 43, + ["plan-review", "plan-approved", "enhancement"], + "Handle plan approval implementation parse failure", + ); + const planPath = + "docs/plans/2026-05-01-issue-43-handle-plan-approval-implementation-parse-failure.md"; + await writeFile(join(config.repoRoot, planPath), "# plan\n", "utf8"); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + (call.args[0] === "status" || call.args[0] === "worktree") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 1, stdout: "", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "pi") { + return { code: 0, stdout: '{"status":"unknown"}', stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + const failed = await runOneIssue(runner, config, { now: NOW }); + + assert.equal(failed.status, "blocked"); + const removedLabels = runner.calls.flatMap((call) => { + const index = call.args.indexOf("--remove-labels"); + return index < 0 ? [] : (call.args[index + 1]?.split(",") ?? []); + }); + assert.equal(removedLabels.includes("plan-approved"), false); + const runState = JSON.parse( + await readFile(runStatePath(config.runStateDir, 43), "utf8"), + ); + assert.equal(runState.status, "implementing"); + assert.equal(runState.planPath, planPath); + + const resumeRunner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: + page === "1" + ? issueListPayload([ + issue( + 43, + ["in-progress", "plan-approved", "enhancement"], + "Handle plan approval implementation parse failure", + ), + ]) + : "[]", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "worktree") { + return { + code: 0, + stdout: `worktree ${join(config.repoRoot, ".worktrees/patchmill-issue-43-handle-plan-approval-implementation-parse-failure")}\n`, + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "-C" && + call.args[2] === "branch" + ) { + return { + code: 0, + stdout: + "agent/issue-43-handle-plan-approval-implementation-parse-failure\n", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "log") { + return { code: 0, stdout: "abc123 partial work\n", stderr: "" }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "pi") { + return { + code: 0, + stdout: JSON.stringify({ + status: "pr-created", + prUrl: "https://forgejo/pr/43", + branch: + "agent/issue-43-handle-plan-approval-implementation-parse-failure", + commits: ["abc123"], + validation: ["just issue-runner-test ok"], + }), + stderr: "", + }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + const resumed = await runOneIssue(resumeRunner, config, { now: NOW }); + + assert.equal(resumed.status, "pr-created"); + assert.equal(resumed.issue.number, 43); +}); + test("runOneIssue does not duplicate unexpected planning failure comments on rerun and still updates lastError", async () => { const config = await makeConfig({ dryRun: false, diff --git a/src/cli/commands/run-once/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index 16270da9..e9745476 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -4,6 +4,10 @@ import { mkdir, readFile, writeFile } from "node:fs/promises"; import { dirname, join } from "node:path"; import { runStatePath, writeRunState } from "./run-state.ts"; import { runOneIssue } from "./pipeline.ts"; +import { + configuredWorktreeStrategy, + expectedIssueWorkspace, +} from "./pipeline-workspace.ts"; import { formatPublishedArtifactComment } from "../../../workflow/artifacts/published-artifacts.ts"; import { issue, @@ -202,6 +206,250 @@ test("runOneIssue reuses a saved created plan as plan-created in plan-only mode" assert.doesNotMatch(commentBody(comments[0]), /Existing plan ready/); }); +test("runOneIssue rejects approved published worktree conflicts before claiming", async () => { + const config = await makeConfig({ dryRun: false, execute: true }); + const specPath = "docs/specs/conflicting-worktree-approved-spec.md"; + const selected = { + ...issue(66, ["spec-approved", "enhancement"], "Conflicting worktree spec"), + comments: [ + { + authorLogin: "patchmill-bot", + body: formatPublishedArtifactComment({ + kind: "spec", + path: specPath, + content: "# Published spec\n", + }), + }, + ], + }; + const workspace = expectedIssueWorkspace( + selected.number, + selected.title, + configuredWorktreeStrategy(config), + ); + await writeFile( + join(config.repoRoot, specPath), + "# Existing primary spec\n", + "utf8", + ); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 1, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add" + ) { + await mkdir( + join(config.repoRoot, workspace.worktreePath, "docs", "specs"), + { recursive: true }, + ); + await writeFile( + join(config.repoRoot, workspace.worktreePath, specPath), + "# Existing worktree spec\n", + "utf8", + ); + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "-C") { + return { code: 0, stdout: `${workspace.branch}\n`, stderr: "" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "log") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "tea" && call.args[0] === "logins") { + return { + code: 0, + stdout: JSON.stringify([ + { name: "default", user: "patchmill-bot", default: true }, + ]), + stderr: "", + }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /spec-approved.*would overwrite existing spec artifact/u, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); + assert.equal( + runner.calls.some((call) => call.command === "pi"), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); +}); + +test("runOneIssue rejects an approved source conflicting with an unregistered issue branch before mutation", async () => { + const config = await makeConfig({ dryRun: false, execute: true }); + const specPath = "docs/specs/branch-conflicting-approved-spec.md"; + const selected = { + ...issue( + 67, + ["spec-approved", "enhancement"], + "Branch conflicting worktree spec", + ), + comments: [ + { + authorLogin: "patchmill-bot", + body: formatPublishedArtifactComment({ + kind: "spec", + path: specPath, + content: "# Published spec\n", + }), + }, + ], + }; + const workspace = expectedIssueWorkspace( + selected.number, + selected.title, + configuredWorktreeStrategy(config), + ); + const runner = createMockRunner(async (call) => { + if ( + call.command === "tea" && + call.args[0] === "issues" && + call.args[1] === "list" + ) { + const page = call.args[call.args.indexOf("--page") + 1]; + return { + code: 0, + stdout: page === "1" ? issueListPayload([selected]) : "[]", + stderr: "", + }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "cat-file") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show") { + return { code: 0, stdout: "# Conflicting branch spec\n", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add" + ) { + await mkdir( + join(config.repoRoot, workspace.worktreePath, "docs", "specs"), + { recursive: true }, + ); + await writeFile( + join(config.repoRoot, workspace.worktreePath, specPath), + "# Conflicting branch spec\n", + "utf8", + ); + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "status") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "log") { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "tea" && call.args[0] === "logins") { + return { + code: 0, + stdout: JSON.stringify([ + { name: "default", user: "patchmill-bot", default: true }, + ]), + stderr: "", + }; + } + if ( + call.command === "tea" && + call.args[0] === "labels" && + call.args[1] === "list" + ) { + return { code: 0, stdout: labelListPayload(), stderr: "" }; + } + if ( + call.command === "tea" && + (call.args[0] === "issues" || call.args[0] === "comment") + ) { + return { code: 0, stdout: "", stderr: "" }; + } + throw new Error( + `unexpected command: ${call.command} ${call.args.join(" ")}`, + ); + }); + + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /spec-approved.*would overwrite existing spec artifact/u, + ); + assert.equal((await workflowPiCalls(runner.calls)).length, 0); + assert.equal( + runner.calls.some( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ), + false, + ); + assert.equal( + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, + ); +}); + test("runOneIssue uses deterministic published artifacts before filename discovery", async () => { const config = await makeConfig({ dryRun: false, execute: true }); const specPath = "docs/specs/human-provided-design.md"; @@ -936,7 +1184,7 @@ test("runOneIssue stops at spec-review when agent-ready has an existing spec and ); }); -test("runOneIssue stops at spec-review for plan-approved issues without spec approval", async () => { +test("runOneIssue rejects plan-approved when no plan can be resolved before spec review", async () => { const config = await makeConfig({ execute: true, dryRun: false, @@ -987,24 +1235,23 @@ test("runOneIssue stops at spec-review for plan-approved issues without spec app ); }); - const result = await runOneIssue(runner, config, { now: NOW }); - - assert.equal(result.status, "spec-found"); - assert.equal(result.specPath, specPath); - const finalEdit = runner.calls - .filter( + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /plan-approved.*no plan artifact could be resolved/u, + ); + assert.equal( + runner.calls.some((call) => call.command === "pi"), + false, + ); + assert.equal( + runner.calls.some( (call) => call.command === "tea" && - call.args[0] === "issues" && - call.args[1] === "edit", - ) - .at(-1); - assert.ok(finalEdit); - assert.equal( - finalEdit.args[finalEdit.args.indexOf("--add-labels") + 1], - "spec-review", + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, ); - assert.equal(finalEdit.args.includes("plan-approved"), false); }); test("runOneIssue fails fast when saved spec path access fails unexpectedly", async () => { @@ -1165,7 +1412,7 @@ test("runOneIssue fails fast when saved plan path access fails unexpectedly", as assert.equal(piCalls, 0); }); -test("runOneIssue treats a newly-created replacement spec as needing fresh approval", async () => { +test("runOneIssue rejects spec-approved when no spec can be resolved", async () => { const config = await makeConfig({ execute: true, dryRun: false, @@ -1229,28 +1476,26 @@ test("runOneIssue treats a newly-created replacement spec as needing fresh appro ); }); - const result = await runOneIssue(runner, config, { now: NOW }); - - assert.equal(result.status, "spec-created"); - assert.equal(result.specPath, expectedSpecPath); - const finalEdit = runner.calls - .filter( + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /spec-approved.*no spec artifact could be resolved/u, + ); + assert.equal( + runner.calls.some((call) => call.command === "pi"), + false, + ); + assert.equal( + runner.calls.some( (call) => call.command === "tea" && - call.args[0] === "issues" && - call.args[1] === "edit", - ) - .at(-1); - assert.ok(finalEdit); - assert.equal( - finalEdit.args[finalEdit.args.indexOf("--add-labels") + 1], - "spec-review", + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, ); - assert.equal(finalEdit.args.includes("spec-approved"), false); - assert.equal(finalEdit.args.includes("plan-approved"), false); }); -test("runOneIssue writes plan from spec-approved and cleans spec labels at plan-review", async () => { +test("runOneIssue writes a plan and preserves spec approval at plan review", async () => { const config = await makeConfig({ execute: true, dryRun: false, @@ -1264,6 +1509,15 @@ test("runOneIssue writes plan from spec-approved and cleans spec labels at plan- const specPath = "docs/specs/2026-05-09-issue-32-needs-plan-design.md"; await writeFile(join(config.repoRoot, specPath), "# Spec\n", "utf8"); const expectedPlanPath = "docs/plans/2026-05-09-issue-32-needs-plan.md"; + const expectedWorkspace = expectedIssueWorkspace( + selected.number, + selected.title, + configuredWorktreeStrategy(config), + ); + const expectedWorkspaceRoot = join( + config.repoRoot, + expectedWorkspace.worktreePath, + ); const runner = createMockRunner(async (call) => { if ( call.command === "tea" && @@ -1280,6 +1534,23 @@ test("runOneIssue writes plan from spec-approved and cleans spec labels at plan- if (call.command === "git" && call.args[0] === "status") { return { code: 0, stdout: "", stderr: "" }; } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "list" + ) { + return { code: 0, stdout: "", stderr: "" }; + } + if (call.command === "git" && call.args[0] === "show-ref") { + return { code: 1, stdout: "", stderr: "" }; + } + if ( + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add" + ) { + return { code: 0, stdout: "", stderr: "" }; + } if ( call.command === "tea" && call.args[0] === "labels" && @@ -1300,7 +1571,8 @@ test("runOneIssue writes plan from spec-approved and cleans spec labels at plan- prompt, new RegExp(specPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")), ); - assert.ok(call.cwd); + assert.equal(call.cwd, expectedWorkspaceRoot); + assert.notEqual(call.cwd, config.repoRoot); const absolutePlanPath = join(call.cwd, expectedPlanPath); await mkdir(dirname(absolutePlanPath), { recursive: true }); await writeFile(absolutePlanPath, "# Generated plan\n", "utf8"); @@ -1331,8 +1603,11 @@ test("runOneIssue writes plan from spec-approved and cleans spec labels at plan- finalEdit.args[finalEdit.args.indexOf("--add-labels") + 1], "plan-review", ); - assert.equal(finalEdit.args.includes("spec-review"), false); - assert.equal(finalEdit.args.includes("spec-approved"), false); + const removedLabels = + finalEdit.args[finalEdit.args.indexOf("--remove-labels") + 1]?.split(",") ?? + []; + assert.deepEqual(removedLabels.sort(), ["in-progress", "spec-review"].sort()); + assert.equal(removedLabels.includes("spec-approved"), false); }); test("runOneIssue resumes approved spec review with saved planning worktree", async () => { @@ -2113,7 +2388,7 @@ test("runOneIssue stops after creating a plan when plan approval is required", a ); }); -test("runOneIssue ignores stale plan approval when a new plan is created", async () => { +test("runOneIssue rejects plan-approved when no plan can be resolved", async () => { const config = await makeConfig({ dryRun: false, execute: true, @@ -2170,42 +2445,34 @@ test("runOneIssue ignores stale plan approval when a new plan is created", async ); }); - const result = await runOneIssue(runner, config, { now: NOW }); - - assert.equal(result.status, "plan-created"); - assert.equal(result.planPath, expectedPlanPath); - assert.equal((await workflowPiCalls(runner.calls)).length, 2); - assert.equal( - runner.calls.some( - (call) => call.command === "git" && call.args[0] === "worktree", - ), - true, - ); - const editCalls = runner.calls.filter( - (call) => - call.command === "tea" && - call.args[0] === "issues" && - call.args[1] === "edit", + await assert.rejects( + () => runOneIssue(runner, config, { now: NOW }), + /plan-approved.*no plan artifact could be resolved/u, ); - const restoreCall = editCalls.at(-1); - assert.ok(restoreCall); assert.equal( - restoreCall.args[restoreCall.args.indexOf("--add-labels") + 1], - "plan-review", + runner.calls.some((call) => call.command === "pi"), + false, ); assert.equal( - restoreCall.args[restoreCall.args.indexOf("--remove-labels") + 1], - "plan-approved,in-progress", + runner.calls.some( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ), + false, ); }); -test("runOneIssue proceeds when plan approval label is present and clears plan-review", async () => { +test("runOneIssue preserves approvals while clearing review labels", async () => { const config = await makeConfig({ dryRun: false, execute: true, approvalPolicy: approvalPolicy({ planRequired: true }), }); + const specPath = "docs/specs/2026-05-14-issue-49-approved-spec-design.md"; const planPath = "docs/plans/2026-05-14-issue-49-approved-plan.md"; + await writeFile(join(config.repoRoot, specPath), "# spec\n", "utf8"); await writeFile(join(config.repoRoot, planPath), "# plan\n", "utf8"); const selected = issue( 49, @@ -2305,10 +2572,15 @@ test("runOneIssue proceeds when plan approval label is present and clears plan-r ); const finalEdit = editCalls.at(-1); assert.ok(finalEdit); - assert.equal(finalEdit.args.includes("spec-review"), false); - assert.equal(finalEdit.args.includes("spec-approved"), false); - assert.equal(finalEdit.args.includes("plan-review"), false); - assert.equal(finalEdit.args.includes("plan-approved"), false); + const removedLabels = editCalls.flatMap((call) => { + const index = call.args.indexOf("--remove-labels"); + return index < 0 ? [] : (call.args[index + 1]?.split(",") ?? []); + }); + assert.equal(removedLabels.includes("spec-approved"), false); + assert.equal(removedLabels.includes("plan-approved"), false); + assert.equal(removedLabels.includes("spec-review"), true); + assert.equal(removedLabels.includes("plan-review"), true); + assert.equal(removedLabels.includes("in-progress"), true); }); test("runOneIssue claims the issue, comments automation start, writes run state, and exits plan-created for plan-only mode", async () => { diff --git a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts index 133575b3..a7b119cf 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -1,6 +1,6 @@ import test from "node:test"; import assert from "node:assert/strict"; -import { readFile, writeFile } from "node:fs/promises"; +import { mkdir, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { runStatePath, writeRunState } from "./run-state.ts"; import { runOneIssue } from "./pipeline.ts"; @@ -136,6 +136,76 @@ test("runOneIssue reuses existing implementation worktree on resume", async () = assert.ok(runner.calls.find((call) => call.command === "pi")); }); +test("runOneIssue recreates an approved implementing worktree after preflight", async () => { + const config = await makeConfig({ + dryRun: false, + execute: true, + issueNumber: 45, + approvalPolicy: specAndPlanApprovalPolicy(), + }); + const planPath = "docs/plans/2026-06-20-issue-45-recover-blocked-run.md"; + const worktreePath = ".worktrees/patchmill-issue-45-recover-blocked-run"; + await writeFile(join(config.repoRoot, planPath), "# plan\n", "utf8"); + await writeBlockedRecoveryRunState( + config, + { issueNumber: 45, status: "implementing" }, + { + createWorktreePath: false, + writePlanInPrimaryRepo: false, + writeSpecInPrimaryRepo: false, + }, + ); + const baseRunner = blockedRecoveryRunner(config, { + selectedLabels: ["in-progress", "plan-approved"], + worktreeRegistered: false, + }); + const runner = { + calls: baseRunner.calls, + async run(...args: Parameters) { + const [command, commandArgs] = args; + if (command === "git" && commandArgs[0] === "show") { + baseRunner.calls.push({ command, args: [...commandArgs] }); + return { code: 0, stdout: "# plan\n", stderr: "" }; + } + const result = await baseRunner.run(...args); + if ( + command === "git" && + commandArgs[0] === "worktree" && + commandArgs[1] === "add" + ) { + await mkdir(join(config.repoRoot, worktreePath, "docs", "plans"), { + recursive: true, + }); + await writeFile( + join(config.repoRoot, worktreePath, planPath), + "# plan\n", + "utf8", + ); + } + return result; + }, + }; + + const result = await runOneIssue(runner, config, { now: NOW }); + + assert.equal(result.status, "pr-created", JSON.stringify(result)); + const worktreeAdd = runner.calls.findIndex( + (call) => + call.command === "git" && + call.args[0] === "worktree" && + call.args[1] === "add", + ); + const firstCleanStatus = runner.calls.findIndex( + (call) => call.command === "git" && call.args[0] === "status", + ); + assert.ok(worktreeAdd >= 0, "expected saved worktree recreation"); + assert.ok(firstCleanStatus >= 0, "expected primary clean-worktree check"); + assert.ok( + firstCleanStatus < worktreeAdd, + "expected worktree recreation after the primary clean-worktree check", + ); +}); + test("runOneIssue resumes clean blocked implementation workspace after external prerequisite is fixed", async () => { const config = await makeConfig({ dryRun: false, @@ -371,7 +441,13 @@ test("runOneIssue recovers blocked state overwritten by spec review stop", async issueNumber: 45, approvalPolicy: specAndPlanApprovalPolicy(), }); - await writeBlockedRecoveryRunState(config); + await writeBlockedRecoveryRunState( + config, + {}, + { + writeSpecInPrimaryRepo: true, + }, + ); await writeRunState( config.runStateDir, { diff --git a/src/cli/commands/run-once/pipeline-workspace.ts b/src/cli/commands/run-once/pipeline-workspace.ts index ce9fbde1..129a05cb 100644 --- a/src/cli/commands/run-once/pipeline-workspace.ts +++ b/src/cli/commands/run-once/pipeline-workspace.ts @@ -8,7 +8,7 @@ import { cleanStatusIgnoredPaths as buildCleanStatusIgnoredPaths } from "./git.t import type { PlanningArtifactPolicy } from "./planning-artifacts.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; import type { readRunState } from "./run-state.ts"; -import type { AgentIssueConfig } from "./types.ts"; +import type { AgentIssueConfig, AgentIssueRunState } from "./types.ts"; export function cleanStatusIgnoredPaths( config: Pick< @@ -89,6 +89,98 @@ export function resumePlanningArtifactPolicy(input: { }; } +export function hasSavedPlanningArtifactWorkspace( + state: AgentIssueRunState | undefined, +): state is AgentIssueRunState { + return !!( + state && + (state.branch || state.worktreePath) && + (state.specPath || state.planPath) + ); +} + +export function planningArtifactPolicyForWorkspace(input: { + config: Pick; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + worktreePath: string; + allowGeneratedSpec: boolean; + allowGeneratedPlan: boolean; +}): PlanningArtifactPolicy { + if (hasSavedPlanningArtifactWorkspace(input.existingState)) { + return resumePlanningArtifactPolicy({ + config: input.config, + worktreePath: input.worktreePath, + existingState: input.existingState, + resolvedArtifacts: input.resolvedArtifacts, + }); + } + + return freshPlanningArtifactPolicy({ + config: input.config, + existingState: input.existingState, + resolvedArtifacts: input.resolvedArtifacts, + allowGeneratedSpec: input.allowGeneratedSpec, + allowGeneratedPlan: input.allowGeneratedPlan, + workspaceRoot: join(input.config.repoRoot, input.worktreePath), + }); +} + +export function freshPlanningArtifactPolicy(input: { + config: Pick; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + allowGeneratedSpec: boolean; + allowGeneratedPlan: boolean; + workspaceRoot?: string; +}): PlanningArtifactPolicy { + const worktreeRoot = + input.workspaceRoot ?? + (input.existingState?.worktreePath + ? join(input.config.repoRoot, input.existingState.worktreePath) + : undefined); + const primaryRoot = worktreeRoot ?? input.config.repoRoot; + + return { + kind: "fresh", + primary: { + repoRoot: primaryRoot, + specsDir: mirrorConfiguredPathInWorktree( + input.config.repoRoot, + primaryRoot, + input.config.specsDir, + ), + plansDir: mirrorConfiguredPathInWorktree( + input.config.repoRoot, + primaryRoot, + input.config.plansDir, + ), + source: worktreeRoot ? "resume-worktree" : "primary-repo", + }, + fallbacks: worktreeRoot + ? [ + { + repoRoot: input.config.repoRoot, + specsDir: input.config.specsDir, + plansDir: input.config.plansDir, + source: "primary-repo", + }, + ] + : undefined, + explicit: input.resolvedArtifacts, + saved: { + specPath: input.existingState?.specPath, + specCommit: input.existingState?.specCommit, + planPath: input.existingState?.planPath, + planCommit: input.existingState?.planCommit, + specCreated: input.existingState?.checkpoints?.specCreated, + planCreated: input.existingState?.checkpoints?.planCreated, + }, + allowGeneratedSpec: input.allowGeneratedSpec, + allowGeneratedPlan: input.allowGeneratedPlan, + }; +} + export function configuredWorktreeStrategy( config: Pick< AgentIssueConfig, diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 3ead753a..8ee98802 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -4,6 +4,7 @@ import { localPiAgentDir } from "../init/pi-agent-settings.ts"; import { createRunOnceHostProvider } from "../../../host/factory.ts"; import { planLabelChange } from "../triage/labels.ts"; import { materializeIssueArtifactSources } from "./artifact-source-materialization.ts"; +import { assertApprovedArtifactsResolvable } from "./approval-artifact-preflight.ts"; import { runArtifactSourceStage } from "./artifact-source-stage.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; import { ensureAutomationLabel } from "./automation-labels.ts"; @@ -11,6 +12,7 @@ import { assertCleanWorktree, assertIssueBaseContainedInPrBase, ensureIssueWorktree, + inspectIssueWorkspace, type IssueWorktreeResult, } from "./git.ts"; import { @@ -54,7 +56,7 @@ import { cleanStatusIgnoredPaths, configuredWorktreeStrategy, expectedIssueWorkspace, - resumePlanningArtifactPolicy, + planningArtifactPolicyForWorkspace, } from "./pipeline-workspace.ts"; import { emitSelectionDiagnostics, @@ -281,54 +283,6 @@ export async function runOneIssue( }); issueForRun = artifactSources.issue; resolvedArtifacts = artifactSources.resolvedArtifacts; - - await progress(runOptions, "info", "git", "checking repository status", { - issueNumber: issue.number, - }); - await assertCleanWorktree(runner, config.repoRoot, ignoredPaths); - const { ready, inProgress, done, needsInfo } = lifecycleLabels(config); - let labels = resumed - ? issue.labels.includes(inProgress) - ? issue.labels - : nextLabels(issue.labels, [ready], [inProgress]) - : nextLabels(issue.labels, [ready], [inProgress]); - if ( - !checkpoints.claimed || - (planningWorkspaceResumable && !issue.labels.includes(inProgress)) - ) { - await runStep("claim issue", async () => { - await progress( - runOptions, - "info", - "labels", - `ensuring ${inProgress} label exists`, - { issueNumber: issue.number }, - ); - await ensureAutomationLabel(host, config, inProgress); - await host.applyLabels( - planLabelChange(issue.number, issue.labels, labels), - ); - await progress( - runOptions, - "info", - "claim", - `claimed #${issue.number}: ${ready} -> ${inProgress}`, - { issueNumber: issue.number }, - ); - await writeRunState( - config.runStateDir, - { - issueNumber: issue.number, - title: issue.title, - status: "claimed", - checkpoints: { claimed: true }, - resetCheckpoints: resetStaleCheckpoints, - }, - timestamp, - ); - checkpoints.claimed = true; - }); - } let specPath: string | undefined; let specCommit: string | undefined; let planPath: string | undefined; @@ -343,8 +297,7 @@ export async function runOneIssue( issue.title, worktreeStrategy, ); - const ensureIssueWorkspace = async (): Promise => { - if (ensuredWorktree) return ensuredWorktree; + const assertExpectedWorkspaceIdentity = (): void => { if ( resumableState && existingState?.branch && @@ -363,6 +316,24 @@ export async function runOneIssue( `Saved worktree ${existingState.worktreePath} does not match expected worktree path ${expectedWorkspace.worktreePath}`, ); } + }; + const hasApprovalLabel = [ + config.approvalPolicy.specApproval.approvedLabel, + config.approvalPolicy.planApproval.approvedLabel, + ].some((label) => issueForRun.labels.includes(label)); + const artifactWorkspace = hasApprovalLabel + ? await (async () => { + assertExpectedWorkspaceIdentity(); + return await inspectIssueWorkspace(runner, config.repoRoot, { + branch: expectedWorkspace.branch, + worktreePath: + existingState?.worktreePath ?? expectedWorkspace.worktreePath, + }); + })() + : undefined; + const ensureIssueWorkspace = async (): Promise => { + if (ensuredWorktree) return ensuredWorktree; + assertExpectedWorkspaceIdentity(); const worktree = await ensureIssueWorktree( runner, @@ -413,6 +384,77 @@ export async function runOneIssue( return worktree; }; + const { ready, inProgress, done, needsInfo } = lifecycleLabels(config); + const approvedArtifactPreflight = await assertApprovedArtifactsResolvable({ + config, + issue: issueForRun, + existingState, + resolvedArtifacts, + now: runOptions.now ?? new Date(), + artifactWorkspace, + runner, + }); + await progress(runOptions, "info", "git", "checking repository status", { + issueNumber: issue.number, + }); + await assertCleanWorktree(runner, config.repoRoot, ignoredPaths); + if (artifactWorkspace?.kind === "branch") { + const workspace = await ensureIssueWorkspace(); + artifactPolicy = planningArtifactPolicyForWorkspace({ + config, + existingState, + resolvedArtifacts, + worktreePath: workspace.worktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } else if (approvedArtifactPreflight) { + artifactPolicy = approvedArtifactPreflight.policy; + } + + let labels = resumed + ? issue.labels.includes(inProgress) + ? issue.labels + : nextLabels(issue.labels, [ready], [inProgress]) + : nextLabels(issue.labels, [ready], [inProgress]); + if ( + !checkpoints.claimed || + (planningWorkspaceResumable && !issue.labels.includes(inProgress)) + ) { + await runStep("claim issue", async () => { + await progress( + runOptions, + "info", + "labels", + `ensuring ${inProgress} label exists`, + { issueNumber: issue.number }, + ); + await ensureAutomationLabel(host, config, inProgress); + await host.applyLabels( + planLabelChange(issue.number, issue.labels, labels), + ); + await progress( + runOptions, + "info", + "claim", + `claimed #${issue.number}: ${ready} -> ${inProgress}`, + { issueNumber: issue.number }, + ); + await writeRunState( + config.runStateDir, + { + issueNumber: issue.number, + title: issue.title, + status: "claimed", + checkpoints: { claimed: true }, + resetCheckpoints: resetStaleCheckpoints, + }, + timestamp, + ); + checkpoints.claimed = true; + }); + } + try { if (!checkpoints.startedCommentPosted) { await host.commentIssue(issueForRun.number, startedComment(issueForRun)); @@ -430,6 +472,7 @@ export async function runOneIssue( } if ( + !artifactPolicy && resumableState && existingState && (existingState.branch || existingState.worktreePath) && @@ -438,11 +481,13 @@ export async function runOneIssue( const resumeWorktree = await ensureIssueWorkspace(); const savedWorktreePath = existingState.worktreePath ?? resumeWorktree.worktreePath; - artifactPolicy = resumePlanningArtifactPolicy({ + artifactPolicy = planningArtifactPolicyForWorkspace({ config, - worktreePath: savedWorktreePath, existingState, resolvedArtifacts, + worktreePath: savedWorktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, }); await progress( runOptions, @@ -454,11 +499,41 @@ export async function runOneIssue( } if (artifactPolicy?.kind === "implementation-resume") { - await resolvePlanningArtifacts({ - policy: artifactPolicy, - issue: issueForRun, - now: runOptions.now ?? new Date(), - }); + const sourcesToMaterialize = { + ...(!artifactPolicy.saved.specPath && resolvedArtifacts.spec + ? { spec: resolvedArtifacts.spec } + : {}), + ...(!artifactPolicy.saved.planPath && resolvedArtifacts.plan + ? { plan: resolvedArtifacts.plan } + : {}), + }; + if (sourcesToMaterialize.spec || sourcesToMaterialize.plan) { + const materializedArtifacts = await runStep( + "materialize issue artifact sources", + async () => + materializeIssueArtifactSources({ + repoRoot: artifactPolicy.primary.repoRoot, + runner, + issueNumber: issueForRun.number, + sources: sourcesToMaterialize, + }), + ); + resolvedArtifacts = { + ...resolvedArtifacts, + ...materializedArtifacts, + }; + artifactPolicy = { + ...artifactPolicy, + explicit: resolvedArtifacts, + }; + } + if (!approvedArtifactPreflight) { + await resolvePlanningArtifacts({ + policy: artifactPolicy, + issue: issueForRun, + now: runOptions.now ?? new Date(), + }); + } } else { const artifactWorktree = resolvedArtifacts.spec || resolvedArtifacts.plan @@ -505,6 +580,8 @@ export async function runOneIssue( ready, inProgress, needsInfo, + approvalGatesSatisfied: + ordinaryResumableState && existingState?.status === "implementing", existingState, resolvedArtifacts, artifactPolicy, diff --git a/src/cli/commands/run-once/planning-artifacts.test.ts b/src/cli/commands/run-once/planning-artifacts.test.ts index 463078c3..493151fe 100644 --- a/src/cli/commands/run-once/planning-artifacts.test.ts +++ b/src/cli/commands/run-once/planning-artifacts.test.ts @@ -115,6 +115,40 @@ test("implementation resume rejects mismatched explicit artifact comments", asyn ); }); +test("implementation resume uses an explicit plan when no plan was saved", async () => { + const { policy } = await repoFixture(); + + const artifacts = await resolvePlanningArtifacts({ + policy: { + ...policy, + saved: { + specPath: "docs/specs/saved-spec.md", + specCommit: "spec123", + }, + explicit: { + plan: { + path: "docs/plans/published-plan.md", + commit: "planpub", + }, + }, + }, + issue: issue(45), + now: NOW, + }); + + assert.equal(artifacts.spec.path, "docs/specs/saved-spec.md"); + assert.equal(artifacts.spec.fromState, true); + assert.deepEqual(artifacts.plan, { + path: "docs/plans/published-plan.md", + commit: "planpub", + exists: true, + fromState: false, + created: false, + generated: false, + rootSource: "resume-worktree", + }); +}); + test("fresh policy accepts explicit artifact comments before discovery", async () => { const repoRoot = await mkdtemp(join(tmpdir(), "patchmill-artifacts-")); const artifacts = await resolvePlanningArtifacts({ diff --git a/src/cli/commands/run-once/planning-artifacts.ts b/src/cli/commands/run-once/planning-artifacts.ts index 106043f3..7e1adab3 100644 --- a/src/cli/commands/run-once/planning-artifacts.ts +++ b/src/cli/commands/run-once/planning-artifacts.ts @@ -1,6 +1,7 @@ -import { isAbsolute, join, relative } from "node:path"; +import { basename, isAbsolute, join, relative } from "node:path"; +import { findIssueArtifacts } from "./artifacts.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; -import { pathExists } from "./paths.ts"; +import { pathExists, pathIsRegularFile } from "./paths.ts"; import { buildPlanPath, findIssuePlan } from "./plans.ts"; import { buildSpecPath, findIssueSpec } from "./specs.ts"; import type { IssueSummary } from "./types.ts"; @@ -92,6 +93,16 @@ function roots(policy: PlanningArtifactPolicy): PlanningArtifactRoot[] { return [policy.primary, ...(policy.fallbacks ?? [])]; } +export function planningArtifactRoot( + policy: PlanningArtifactPolicy, + artifact: ResolvedPlanningArtifact, +): PlanningArtifactRoot { + return ( + roots(policy).find((root) => root.source === artifact.rootSource) ?? + policy.primary + ); +} + function explicitMatchesSaved(input: { kind: "spec" | "plan"; explicit?: { path: string; commit?: string }; @@ -126,6 +137,11 @@ async function findSaved(input: { for (const root of input.roots) { const savedPath = repoPath(root.repoRoot, input.savedPath); if (await pathExists(savedPath.absolute)) { + if (!(await pathIsRegularFile(savedPath.absolute))) { + throw new PlanningArtifactSafetyError( + `Saved ${input.savedPath} is not a regular file`, + ); + } return { path: savedPath.relative, commit: input.savedCommit, @@ -249,14 +265,28 @@ export async function resolvePlanningArtifacts(input: { savedCommit: input.policy.saved.planCommit, savedCreated: input.policy.saved.planCreated, }); + const explicitArtifact = (artifact: { + path: string; + commit?: string; + }): ResolvedPlanningArtifact => ({ + path: artifact.path, + commit: artifact.commit, + exists: true, + fromState: false, + created: false, + generated: false, + rootSource: input.policy.primary.source, + }); const discoveredPlan = input.policy.saved.planPath ? plan - : await findDiscovered({ - roots: policyRoots, - issue: input.issue, - kind: "plan", - }); + : input.policy.explicit?.plan + ? explicitArtifact(input.policy.explicit.plan) + : await findDiscovered({ + roots: policyRoots, + issue: input.issue, + kind: "plan", + }); const resolvedPlan = plan.exists ? plan : discoveredPlan.exists @@ -271,11 +301,13 @@ export async function resolvePlanningArtifacts(input: { ? spec : input.policy.saved.specPath ? unresolvedArtifact() - : await findDiscovered({ - roots: policyRoots, - issue: input.issue, - kind: "spec", - }); + : input.policy.explicit?.spec + ? explicitArtifact(input.policy.explicit.spec) + : await findDiscovered({ + roots: policyRoots, + issue: input.issue, + kind: "spec", + }); if (input.policy.saved.planPath && !resolvedPlan.exists) { throw new PlanningArtifactSafetyError( @@ -352,3 +384,59 @@ export async function resolvePlanningArtifacts(input: { }), }; } + +async function assertUnambiguousApprovedArtifact(input: { + policy: PlanningArtifactPolicy; + issue: IssueSummary; + kind: "spec" | "plan"; + artifact: ResolvedPlanningArtifact; +}): Promise { + const explicit = input.policy.explicit?.[input.kind]; + if (explicit || (input.artifact.fromState && input.artifact.exists)) return; + + const candidates = ( + await Promise.all( + roots(input.policy).map((root) => + findIssueArtifacts( + input.kind === "spec" ? root.specsDir : root.plansDir, + input.issue.number, + ), + ), + ) + ).flat(); + const names = [ + ...new Set(candidates.map((candidate) => basename(candidate))), + ]; + if (names.length <= 1) return; + + throw new PlanningArtifactSafetyError( + `Issue #${input.issue.number} has multiple ${input.kind} artifacts that could be resolved: ${names.join(", ")}`, + ); +} + +export async function resolveApprovedPlanningArtifacts(input: { + policy: PlanningArtifactPolicy; + issue: IssueSummary; + now: Date; + requireSpec: boolean; + requirePlan: boolean; +}): Promise { + const artifacts = await resolvePlanningArtifacts(input); + if (input.requireSpec) { + await assertUnambiguousApprovedArtifact({ + policy: input.policy, + issue: input.issue, + kind: "spec", + artifact: artifacts.spec, + }); + } + if (input.requirePlan) { + await assertUnambiguousApprovedArtifact({ + policy: input.policy, + issue: input.issue, + kind: "plan", + artifact: artifacts.plan, + }); + } + return artifacts; +} diff --git a/src/cli/commands/run-once/stage-advancement.ts b/src/cli/commands/run-once/stage-advancement.ts index 0d21951c..438fcea6 100644 --- a/src/cli/commands/run-once/stage-advancement.ts +++ b/src/cli/commands/run-once/stage-advancement.ts @@ -95,6 +95,7 @@ export type AdvancePlanningStagesOptions = { ready: string; inProgress: string; needsInfo: string; + approvalGatesSatisfied: boolean; existingState?: ExistingPlanningState; resolvedArtifacts?: ResolvedIssueArtifactSources; artifactPolicy?: PlanningArtifactPolicy; @@ -228,6 +229,7 @@ export async function advancePlanningStages({ ready, inProgress, needsInfo, + approvalGatesSatisfied, existingState, resolvedArtifacts, artifactPolicy, @@ -248,11 +250,9 @@ export async function advancePlanningStages({ let specPath: string | undefined; let specCommit: string | undefined; let specCreated: boolean; - let specCreatedThisRun = false; let planPath: string | undefined; let planCommit: string | undefined; let planCreated: boolean; - let planCreatedThisRun = false; let planningArtifactWorkspace: PlanningArtifactWorkspace = { repoRoot: config.repoRoot, @@ -324,11 +324,15 @@ export async function advancePlanningStages({ allowGeneratedPlan: true, }); - if (artifactPolicy?.kind === "implementation-resume") { + if (artifactPolicy) { planningArtifactWorkspace = { repoRoot: artifactPolicy.primary.repoRoot, - ...(existingState?.branch ? { branch: existingState.branch } : {}), - ...(existingState?.worktreePath + ...(artifactPolicy.kind === "implementation-resume" && + existingState?.branch + ? { branch: existingState.branch } + : {}), + ...(artifactPolicy.kind === "implementation-resume" && + existingState?.worktreePath ? { worktreePath: existingState.worktreePath } : {}), }; @@ -337,14 +341,21 @@ export async function advancePlanningStages({ planningPlansDir = artifactPolicy.primary.plansDir; } - let artifactPolicyForRun = artifactPolicy ?? freshArtifactPolicy(); + let artifactPolicyForRun = + artifactPolicy?.kind === "fresh" + ? { + ...artifactPolicy, + allowGeneratedSpec: true, + allowGeneratedPlan: true, + } + : (artifactPolicy ?? freshArtifactPolicy()); let planningArtifacts = await resolvePlanningArtifacts({ policy: artifactPolicyForRun, issue, now, }); if ( - !artifactPolicy && + artifactPolicyForRun.kind === "fresh" && ensurePlanningArtifactWorkspace && (planningArtifacts.plan.generated || (!planningArtifacts.plan.exists && planningArtifacts.spec.generated)) @@ -456,7 +467,6 @@ export async function advancePlanningStages({ specPath = repoPath(planningRepoRoot, specResult.specPath).relative; specCommit = specResult.commit; specCreated = true; - specCreatedThisRun = true; await writeRunState( config.runStateDir, { @@ -542,8 +552,8 @@ export async function advancePlanningStages({ } const hasCurrentSpecApproval = - issue.labels.includes(config.approvalPolicy.specApproval.approvedLabel) && - !specCreatedThisRun; + approvalGatesSatisfied || + issue.labels.includes(config.approvalPolicy.specApproval.approvedLabel); const mustStopForSpecReview = config.approvalPolicy.specApproval.required && specPath !== undefined && @@ -692,7 +702,6 @@ export async function advancePlanningStages({ planPath = repoPath(planningRepoRoot, planned.planPath).relative; planCommit = planned.commit; planCreated = true; - planCreatedThisRun = true; await writeRunState( config.runStateDir, { @@ -781,12 +790,13 @@ export async function advancePlanningStages({ await emitSimpleStep(issue.number, "publish plan"); } - const planGate = decidePlanApprovalGate({ - labels, - planOnly: config.planOnly, - planCreatedThisRun, - policy: config.approvalPolicy, - }); + const planGate = approvalGatesSatisfied + ? ({ action: "proceed" } as const) + : decidePlanApprovalGate({ + labels, + planOnly: config.planOnly, + policy: config.approvalPolicy, + }); if (planGate.action !== "proceed") { const finalLabels = diff --git a/src/cli/commands/run-once/workflow-state.test.ts b/src/cli/commands/run-once/workflow-state.test.ts index 5916e2db..6eed2bfc 100644 --- a/src/cli/commands/run-once/workflow-state.test.ts +++ b/src/cli/commands/run-once/workflow-state.test.ts @@ -73,6 +73,26 @@ test("resolveWorkflowState treats plan-approved as stronger than other workflow ); }); +test("resolveWorkflowState treats plan review as later than durable spec approval", () => { + assert.deepEqual( + resolveWorkflowState(["spec-approved", "plan-review"], { + readyLabel: ready, + policy, + }), + { kind: "waiting-plan-review", missingLabel: "plan-approved" }, + ); +}); + +test("resolveWorkflowState treats spec review as later than agent-ready", () => { + assert.deepEqual( + resolveWorkflowState([ready, "spec-review"], { + readyLabel: ready, + policy, + }), + { kind: "waiting-spec-review", missingLabel: "spec-approved" }, + ); +}); + test("resolveWorkflowState treats review-only labels as waiting", () => { assert.deepEqual( resolveWorkflowState(["spec-review"], { readyLabel: ready, policy }), @@ -158,22 +178,6 @@ test("decidePlanApprovalGate proceeds when the approved plan label is present", assert.deepEqual(decision, { action: "proceed" }); }); -test("decidePlanApprovalGate ignores stale approval on a newly-created plan", () => { - const decision = decidePlanApprovalGate({ - labels: ["in-progress", "plan-approved"], - planOnly: false, - planCreatedThisRun: true, - policy: planApprovalPolicy(true), - }); - - assert.deepEqual(decision, { - action: "stop-for-plan-review", - reviewLabel: "plan-review", - missingLabel: "plan-approved", - staleApprovedLabel: "plan-approved", - }); -}); - test("decidePlanApprovalGate stops for plan-only without workflow review labels", () => { const decision = decidePlanApprovalGate({ labels: ["in-progress"], @@ -184,7 +188,7 @@ test("decidePlanApprovalGate stops for plan-only without workflow review labels" assert.deepEqual(decision, { action: "stop-for-plan-only" }); }); -test("cleanupLabelsForSpecReview removes agent-ready and stale later approvals", () => { +test("cleanupLabelsForSpecReview preserves approval labels", () => { assert.deepEqual( cleanupLabelsForSpecReview( [ready, "spec-approved", "plan-review", "plan-approved", "bug"], @@ -193,11 +197,11 @@ test("cleanupLabelsForSpecReview removes agent-ready and stale later approvals", policy, }, ), - ["bug", "spec-review"], + ["spec-approved", "plan-approved", "bug", "spec-review"], ); }); -test("cleanupLabelsForPlanReview removes ready and all spec labels", () => { +test("cleanupLabelsForPlanReview preserves approval labels", () => { assert.deepEqual( cleanupLabelsForPlanReview( [ready, "spec-review", "spec-approved", "plan-approved", "bug"], @@ -206,11 +210,11 @@ test("cleanupLabelsForPlanReview removes ready and all spec labels", () => { policy, }, ), - ["bug", "plan-review"], + ["spec-approved", "plan-approved", "bug", "plan-review"], ); }); -test("cleanupLabelsForImplementation removes all workflow review and approval labels", () => { +test("cleanupLabelsForImplementation preserves approval labels", () => { assert.deepEqual( cleanupLabelsForImplementation( [ @@ -223,7 +227,7 @@ test("cleanupLabelsForImplementation removes all workflow review and approval la ], { readyLabel: ready, policy }, ), - ["bug"], + ["spec-approved", "plan-approved", "bug"], ); }); @@ -239,7 +243,7 @@ test("retryableLabelsAfterDevelopmentEnvironmentFailure restores original action ); }); -test("retryableLabelsAfterDevelopmentEnvironmentFailure restores plan approval for resumed in-progress issues", () => { +test("retryableLabelsAfterDevelopmentEnvironmentFailure restores ready for legacy resume", () => { assert.deepEqual( retryableLabelsAfterDevelopmentEnvironmentFailure(["in-progress", "bug"], { readyLabel: ready, @@ -247,6 +251,6 @@ test("retryableLabelsAfterDevelopmentEnvironmentFailure restores plan approval f originalLabels: ["in-progress"], inProgressLabel: "in-progress", }), - ["bug", "plan-approved"], + ["bug", ready], ); }); diff --git a/src/cli/commands/run-once/workflow-state.ts b/src/cli/commands/run-once/workflow-state.ts index 5851e038..2bdb80a6 100644 --- a/src/cli/commands/run-once/workflow-state.ts +++ b/src/cli/commands/run-once/workflow-state.ts @@ -47,7 +47,6 @@ export type PlanApprovalGateDecision = action: "stop-for-plan-review"; reviewLabel: string; missingLabel: string; - staleApprovedLabel?: string; }; function has(labels: string[], label: string): boolean { @@ -71,20 +70,20 @@ export function resolveWorkflowState( const { specApproval, planApproval } = policy; if (has(labels, planApproval.approvedLabel)) return { kind: "plan-approved" }; + if (has(labels, planApproval.reviewLabel)) { + return { + kind: "waiting-plan-review", + missingLabel: planApproval.approvedLabel, + }; + } if (has(labels, specApproval.approvedLabel)) return { kind: "spec-approved" }; - if (has(labels, readyLabel)) return { kind: "agent-ready" }; if (has(labels, specApproval.reviewLabel)) { return { kind: "waiting-spec-review", missingLabel: specApproval.approvedLabel, }; } - if (has(labels, planApproval.reviewLabel)) { - return { - kind: "waiting-plan-review", - missingLabel: planApproval.approvedLabel, - }; - } + if (has(labels, readyLabel)) return { kind: "agent-ready" }; return { kind: "not-actionable" }; } @@ -121,26 +120,18 @@ export function assertExplicitWorkflowState( export function decidePlanApprovalGate(options: { labels: string[]; planOnly: boolean; - planCreatedThisRun?: boolean; policy: WorkflowApprovalPolicy; }): PlanApprovalGateDecision { if (options.planOnly) return { action: "stop-for-plan-only" }; const approval = options.policy.planApproval; if (!approval.required) return { action: "proceed" }; - if ( - !options.planCreatedThisRun && - options.labels.includes(approval.approvedLabel) - ) { + if (options.labels.includes(approval.approvedLabel)) { return { action: "proceed" }; } return { action: "stop-for-plan-review", reviewLabel: approval.reviewLabel, missingLabel: approval.approvedLabel, - ...(options.planCreatedThisRun && - options.labels.includes(approval.approvedLabel) - ? { staleApprovedLabel: approval.approvedLabel } - : {}), }; } @@ -151,9 +142,7 @@ export function cleanupLabelsForSpecReview( return addLabel( removeLabels(labels, [ options.readyLabel, - options.policy.specApproval.approvedLabel, options.policy.planApproval.reviewLabel, - options.policy.planApproval.approvedLabel, ]), options.policy.specApproval.reviewLabel, ); @@ -167,8 +156,6 @@ export function cleanupLabelsForPlanReview( removeLabels(labels, [ options.readyLabel, options.policy.specApproval.reviewLabel, - options.policy.specApproval.approvedLabel, - options.policy.planApproval.approvedLabel, ]), options.policy.planApproval.reviewLabel, ); @@ -181,9 +168,7 @@ export function cleanupLabelsForImplementation( return removeLabels(labels, [ options.readyLabel, options.policy.specApproval.reviewLabel, - options.policy.specApproval.approvedLabel, options.policy.planApproval.reviewLabel, - options.policy.planApproval.approvedLabel, ]); } @@ -203,7 +188,7 @@ export function retryableLabelsAfterDevelopmentEnvironmentFailure( const restore = originalActionableLabels.length > 0 ? originalActionableLabels - : [options.policy.planApproval.approvedLabel]; + : [options.readyLabel]; return restore.reduce(addLabel, withoutInProgress); } diff --git a/test-support/run-once/mock-runner.ts b/test-support/run-once/mock-runner.ts index 79c1ce90..1b198c2f 100644 --- a/test-support/run-once/mock-runner.ts +++ b/test-support/run-once/mock-runner.ts @@ -36,7 +36,11 @@ export function workflowPiCalls(calls: Call[]): Call[] { function defaultGitPreflightResult(call: Call): CommandResult | undefined { if (call.command === "git" && call.args[0] === "cat-file") { - return { code: 0, stdout: "", stderr: "" }; + return { + code: 0, + stdout: call.args[1] === "-t" ? "blob\n" : "", + stderr: "", + }; } if (call.command === "git" && call.args[0] === "diff") { return { code: 0, stdout: "", stderr: "" }; diff --git a/test-support/run-once/pipeline-fixtures.ts b/test-support/run-once/pipeline-fixtures.ts index d3466743..62daa4d6 100644 --- a/test-support/run-once/pipeline-fixtures.ts +++ b/test-support/run-once/pipeline-fixtures.ts @@ -117,6 +117,7 @@ type PlanApprovedImplementationScenario = { title: string; issueLabels?: string[]; planPath?: string; + specPath?: string; configOverrides?: Partial; onPi?: (input: { call: Call; @@ -144,6 +145,13 @@ export async function runPlanApprovedImplementationScenario( scenario.planPath ?? `docs/plans/2026-05-14-issue-${scenario.issueNumber}-scenario.md`; await writeFile(join(config.repoRoot, planPath), "# plan\n", "utf8"); + if (scenario.specPath) { + await writeFile( + join(config.repoRoot, scenario.specPath), + "# spec\n", + "utf8", + ); + } const selected = issue( scenario.issueNumber, scenario.issueLabels ?? ["plan-approved"],