From aab9b69241c1cd463bb6ec436a25e907e4c5b7da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:47:07 +0200 Subject: [PATCH 01/25] docs(workflow): materialize issue 137 artifacts --- .../2026-08-02-durable-approval-labels.md | 1339 +++++++++++++++++ ...26-08-02-durable-approval-labels-design.md | 309 ++++ 2 files changed, 1648 insertions(+) create mode 100644 docs/plans/2026-08-02-durable-approval-labels.md create mode 100644 docs/specs/2026-08-02-durable-approval-labels-design.md 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 0000000..a0faae7 --- /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 0000000..2bb410a --- /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. From 252b6dd732f78b6df58b13175ea711585e88be2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:52:20 +0200 Subject: [PATCH 02/25] fix(run-once): resolve durable approval stages --- .../commands/run-once/workflow-state.test.ts | 20 +++++++++++++++++++ src/cli/commands/run-once/workflow-state.ts | 14 ++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/cli/commands/run-once/workflow-state.test.ts b/src/cli/commands/run-once/workflow-state.test.ts index 5916e2d..668480e 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 }), diff --git a/src/cli/commands/run-once/workflow-state.ts b/src/cli/commands/run-once/workflow-state.ts index 5851e03..cbe7711 100644 --- a/src/cli/commands/run-once/workflow-state.ts +++ b/src/cli/commands/run-once/workflow-state.ts @@ -71,20 +71,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" }; } From deb589571c61bc3efd2849997db28ebadb4fb55e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:55:06 +0200 Subject: [PATCH 03/25] fix(run-once): guard approved artifacts --- .../approval-artifact-preflight.test.ts | 162 ++++++++++++++++ .../run-once/approval-artifact-preflight.ts | 180 ++++++++++++++++++ src/cli/commands/run-once/artifacts.ts | 20 +- .../run-once/pipeline-planning.test.ts | 99 +++++----- src/cli/commands/run-once/pipeline.ts | 8 + .../commands/run-once/stage-advancement.ts | 11 +- .../commands/run-once/workflow-state.test.ts | 16 -- src/cli/commands/run-once/workflow-state.ts | 11 +- 8 files changed, 409 insertions(+), 98 deletions(-) create mode 100644 src/cli/commands/run-once/approval-artifact-preflight.test.ts create mode 100644 src/cli/commands/run-once/approval-artifact-preflight.ts 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 0000000..b919f15 --- /dev/null +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -0,0 +1,162 @@ +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, + ); +}); 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 0000000..168c1ee --- /dev/null +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -0,0 +1,180 @@ +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"); + } +} diff --git a/src/cli/commands/run-once/artifacts.ts b/src/cli/commands/run-once/artifacts.ts index 55e398f..1b7d394 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/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index 16270da..fb160e6 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -936,7 +936,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 +987,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 +1164,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,25 +1228,23 @@ 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 () => { @@ -2113,7 +2110,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,32 +2167,22 @@ 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, ); }); diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 3ead753..fe2a2d8 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"; @@ -281,6 +282,13 @@ export async function runOneIssue( }); issueForRun = artifactSources.issue; resolvedArtifacts = artifactSources.resolvedArtifacts; + await assertApprovedArtifactsResolvable({ + config, + issue: issueForRun, + existingState, + resolvedArtifacts, + now: runOptions.now ?? new Date(), + }); await progress(runOptions, "info", "git", "checking repository status", { issueNumber: issue.number, diff --git a/src/cli/commands/run-once/stage-advancement.ts b/src/cli/commands/run-once/stage-advancement.ts index 0d21951..3aa945b 100644 --- a/src/cli/commands/run-once/stage-advancement.ts +++ b/src/cli/commands/run-once/stage-advancement.ts @@ -248,11 +248,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, @@ -456,7 +454,6 @@ export async function advancePlanningStages({ specPath = repoPath(planningRepoRoot, specResult.specPath).relative; specCommit = specResult.commit; specCreated = true; - specCreatedThisRun = true; await writeRunState( config.runStateDir, { @@ -541,9 +538,9 @@ export async function advancePlanningStages({ await emitSimpleStep(issue.number, "publish spec"); } - const hasCurrentSpecApproval = - issue.labels.includes(config.approvalPolicy.specApproval.approvedLabel) && - !specCreatedThisRun; + const hasCurrentSpecApproval = issue.labels.includes( + config.approvalPolicy.specApproval.approvedLabel, + ); const mustStopForSpecReview = config.approvalPolicy.specApproval.required && specPath !== undefined && @@ -692,7 +689,6 @@ export async function advancePlanningStages({ planPath = repoPath(planningRepoRoot, planned.planPath).relative; planCommit = planned.commit; planCreated = true; - planCreatedThisRun = true; await writeRunState( config.runStateDir, { @@ -784,7 +780,6 @@ export async function advancePlanningStages({ const planGate = decidePlanApprovalGate({ labels, planOnly: config.planOnly, - planCreatedThisRun, policy: config.approvalPolicy, }); diff --git a/src/cli/commands/run-once/workflow-state.test.ts b/src/cli/commands/run-once/workflow-state.test.ts index 668480e..82bd895 100644 --- a/src/cli/commands/run-once/workflow-state.test.ts +++ b/src/cli/commands/run-once/workflow-state.test.ts @@ -178,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"], diff --git a/src/cli/commands/run-once/workflow-state.ts b/src/cli/commands/run-once/workflow-state.ts index cbe7711..ee12535 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 { @@ -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 } - : {}), }; } From 40e4ba1165d7835fef4a0ebaa2f7886ab7b1cef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:56:05 +0200 Subject: [PATCH 04/25] fix(run-once): preserve approval labels --- .../run-once/pipeline-planning.test.ts | 26 +++++++++++++------ .../commands/run-once/workflow-state.test.ts | 12 ++++----- src/cli/commands/run-once/workflow-state.ts | 6 ----- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/cli/commands/run-once/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index fb160e6..27e5dfb 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -1247,7 +1247,7 @@ test("runOneIssue rejects spec-approved when no spec can be resolved", async () ); }); -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, @@ -1328,8 +1328,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 () => { @@ -2186,13 +2189,15 @@ test("runOneIssue rejects plan-approved when no plan can be resolved", async () ); }); -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, @@ -2292,10 +2297,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/workflow-state.test.ts b/src/cli/commands/run-once/workflow-state.test.ts index 82bd895..0291b56 100644 --- a/src/cli/commands/run-once/workflow-state.test.ts +++ b/src/cli/commands/run-once/workflow-state.test.ts @@ -188,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"], @@ -197,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"], @@ -210,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( [ @@ -227,7 +227,7 @@ test("cleanupLabelsForImplementation removes all workflow review and approval la ], { readyLabel: ready, policy }, ), - ["bug"], + ["spec-approved", "plan-approved", "bug"], ); }); diff --git a/src/cli/commands/run-once/workflow-state.ts b/src/cli/commands/run-once/workflow-state.ts index ee12535..8b6291d 100644 --- a/src/cli/commands/run-once/workflow-state.ts +++ b/src/cli/commands/run-once/workflow-state.ts @@ -142,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, ); @@ -158,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, ); @@ -172,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, ]); } From d4172f53c1fe86bb180b67c573d5b398dd446f44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:58:27 +0200 Subject: [PATCH 05/25] fix(run-once): resume authorized implementation --- .../pipeline-development-environment.test.ts | 12 ++++----- .../pipeline-failures-scenarios.test.ts | 26 +++++++++++++++---- src/cli/commands/run-once/pipeline.ts | 2 ++ .../commands/run-once/stage-advancement.ts | 20 ++++++++------ .../commands/run-once/workflow-state.test.ts | 4 +-- src/cli/commands/run-once/workflow-state.ts | 2 +- test-support/run-once/pipeline-fixtures.ts | 8 ++++++ 7 files changed, 52 insertions(+), 22 deletions(-) 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 85a1b85..e7398d6 100644 --- a/src/cli/commands/run-once/pipeline-development-environment.test.ts +++ b/src/cli/commands/run-once/pipeline-development-environment.test.ts @@ -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,7 @@ test("runOneIssue preserves approval labels after development environment failur call.args[1] === "edit", ) .at(-1); - assert.equal( - finalEdit?.args[finalEdit.args.indexOf("--add-labels") + 1], - "spec-approved,plan-approved", - ); + assert.equal(finalEdit?.args.includes("--add-labels"), false); assert.equal( finalEdit?.args[finalEdit.args.indexOf("--remove-labels") + 1], "in-progress", @@ -382,6 +381,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 +492,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 10b5da5..e116f36 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"), diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index fe2a2d8..c51e656 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -513,6 +513,8 @@ export async function runOneIssue( ready, inProgress, needsInfo, + approvalGatesSatisfied: + ordinaryResumableState && existingState?.status === "implementing", existingState, resolvedArtifacts, artifactPolicy, diff --git a/src/cli/commands/run-once/stage-advancement.ts b/src/cli/commands/run-once/stage-advancement.ts index 3aa945b..a970ca7 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, @@ -538,9 +540,9 @@ export async function advancePlanningStages({ await emitSimpleStep(issue.number, "publish spec"); } - const hasCurrentSpecApproval = issue.labels.includes( - config.approvalPolicy.specApproval.approvedLabel, - ); + const hasCurrentSpecApproval = + approvalGatesSatisfied || + issue.labels.includes(config.approvalPolicy.specApproval.approvedLabel); const mustStopForSpecReview = config.approvalPolicy.specApproval.required && specPath !== undefined && @@ -777,11 +779,13 @@ export async function advancePlanningStages({ await emitSimpleStep(issue.number, "publish plan"); } - const planGate = decidePlanApprovalGate({ - labels, - planOnly: config.planOnly, - 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 0291b56..6eed2bf 100644 --- a/src/cli/commands/run-once/workflow-state.test.ts +++ b/src/cli/commands/run-once/workflow-state.test.ts @@ -243,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, @@ -251,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 8b6291d..2bdb80a 100644 --- a/src/cli/commands/run-once/workflow-state.ts +++ b/src/cli/commands/run-once/workflow-state.ts @@ -188,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/pipeline-fixtures.ts b/test-support/run-once/pipeline-fixtures.ts index d346674..62daa4d 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"], From 041010d8b878efa8c3283ce0cb1a7effdf4048b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 10:59:49 +0200 Subject: [PATCH 06/25] test(run-once): retain approved recovery fixture --- .../run-once/pipeline-workspace-scenarios.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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 133575b..b3c005e 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -371,7 +371,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, { From 2ee5ce355500515b4f779891cbd93083c62e3697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:00:10 +0200 Subject: [PATCH 07/25] docs(run-once): explain durable approvals --- .../content/docs/reference/workflow-labels.md | 16 +++++++++++++--- .../src/content/docs/using-patchmill/run-once.md | 5 +++++ .../docs/using-patchmill/workflow-artifacts.md | 6 ++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/site/src/content/docs/reference/workflow-labels.md b/site/src/content/docs/reference/workflow-labels.md index 67abdab..9d7e530 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 b1c8ce1..b835951 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 8520505..c169a24 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. From 479bea5c7d27940771469279ef10ad82d7b9c676 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:14:48 +0200 Subject: [PATCH 08/25] fix(run-once): harden approved artifact preflight --- .../using-patchmill/workflow-artifacts.md | 6 +- .../approval-artifact-preflight.test.ts | 50 ++++++ .../run-once/approval-artifact-preflight.ts | 57 ++++++- .../artifact-source-materialization.ts | 43 +++-- .../pipeline-failures-scenarios.test.ts | 156 ++++++++++++++++++ .../run-once/pipeline-planning.test.ts | 66 ++++++++ 6 files changed, 358 insertions(+), 20 deletions(-) diff --git a/site/src/content/docs/using-patchmill/workflow-artifacts.md b/site/src/content/docs/using-patchmill/workflow-artifacts.md index c169a24..2cab42d 100644 --- a/site/src/content/docs/using-patchmill/workflow-artifacts.md +++ b/site/src/content/docs/using-patchmill/workflow-artifacts.md @@ -90,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 index b919f15..4cc31b0 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -138,6 +138,56 @@ test("approved spec with multiple discovered specs fails as ambiguous", async () ); }); +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("preflight uses configured approval label names", async () => { const { config, issue } = await fixture(); config.approvalPolicy = createWorkflowApprovalPolicy({ diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 168c1ee..b49fc06 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -1,4 +1,5 @@ -import { basename, join } from "node:path"; +import { basename, isAbsolute, join } from "node:path"; +import { assertIssueArtifactSourcesMaterializable } from "./artifact-source-materialization.ts"; import { findIssueArtifacts } from "./artifacts.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; import { @@ -7,6 +8,7 @@ import { type PlanningArtifactPolicy, type ResolvedPlanningArtifacts, } from "./planning-artifacts.ts"; +import { pathExists } from "./paths.ts"; import { mirrorConfiguredPathInWorktree } from "./pipeline-workspace.ts"; import type { AgentIssueConfig, @@ -96,17 +98,35 @@ function artifactDirs( ]; } -async function assertUnambiguousDiscovery( +async function savedArtifactExists( options: ApprovedArtifactPreflightOptions, kind: "spec" | "plan", - label: string, -): Promise { - if (options.resolvedArtifacts[kind]) return; +): Promise { const savedPath = kind === "spec" ? options.existingState?.specPath : options.existingState?.planPath; - if (savedPath) return; + if (!savedPath) return false; + if (isAbsolute(savedPath)) return pathExists(savedPath); + + const roots = [options.config.repoRoot]; + if (options.existingState?.worktreePath) { + roots.unshift( + join(options.config.repoRoot, options.existingState.worktreePath), + ); + } + return ( + await Promise.all(roots.map((root) => pathExists(join(root, savedPath)))) + ).some(Boolean); +} + +async function assertUnambiguousDiscovery( + options: ApprovedArtifactPreflightOptions, + kind: "spec" | "plan", + label: string, +): Promise { + if (options.resolvedArtifacts[kind]) return; + if (await savedArtifactExists(options, kind)) return; const candidates = ( await Promise.all( @@ -151,6 +171,31 @@ export async function assertApprovedArtifactsResolvable( await assertUnambiguousDiscovery(options, "plan", planLabel); } + const approvedSources = { + ...(requiresSpec && options.resolvedArtifacts.spec + ? { spec: options.resolvedArtifacts.spec } + : {}), + ...(requiresPlan && options.resolvedArtifacts.plan + ? { plan: options.resolvedArtifacts.plan } + : {}), + }; + try { + await assertIssueArtifactSourcesMaterializable({ + repoRoot: options.config.repoRoot, + issueNumber: options.issue.number, + sources: approvedSources, + }); + } catch (error) { + const labels = [ + ...(requiresSpec && approvedSources.spec ? [specLabel] : []), + ...(requiresPlan && approvedSources.plan ? [planLabel] : []), + ].join(", "); + const message = error instanceof Error ? error.message : String(error); + throw new PlanningArtifactSafetyError( + `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts cannot be materialized: ${message}`, + ); + } + let artifacts: ResolvedPlanningArtifacts; try { artifacts = await resolvePlanningArtifacts({ diff --git a/src/cli/commands/run-once/artifact-source-materialization.ts b/src/cli/commands/run-once/artifact-source-materialization.ts index eec8f9b..3360edb 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,24 @@ 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 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/pipeline-failures-scenarios.test.ts b/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts index e116f36..64133e6 100644 --- a/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-failures-scenarios.test.ts @@ -1079,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 27e5dfb..cdc1c51 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -202,6 +202,72 @@ 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 artifact conflicts before claiming", async () => { + const config = await makeConfig({ dryRun: false, execute: true }); + const specPath = "docs/specs/conflicting-approved-spec.md"; + await writeFile(join(config.repoRoot, specPath), "# Existing spec\n", "utf8"); + const selected = { + ...issue(65, ["spec-approved", "enhancement"], "Conflicting spec"), + comments: [ + { + authorLogin: "patchmill-bot", + body: formatPublishedArtifactComment({ + kind: "spec", + path: specPath, + content: "# Published spec\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 === "git" && call.args[0] === "show-ref") { + return { code: 1, 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 === "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 uses deterministic published artifacts before filename discovery", async () => { const config = await makeConfig({ dryRun: false, execute: true }); const specPath = "docs/specs/human-provided-design.md"; From 16191c03bb5fe40da029bf1a06e7f24a7ed71ea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:26:02 +0200 Subject: [PATCH 09/25] fix(run-once): validate approved resume artifacts --- .../approval-artifact-preflight.test.ts | 34 +++++++++++++++++++ .../run-once/approval-artifact-preflight.ts | 10 +++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 4cc31b0..92bd2d3 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -188,6 +188,40 @@ test("approved published artifact rejects a conflicting local target", async () ); }); +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("preflight uses configured approval label names", async () => { const { config, issue } = await fixture(); config.approvalPolicy = createWorkflowApprovalPolicy({ diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index b49fc06..230414d 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -76,6 +76,14 @@ function preflightPolicy( }; } +function artifactMaterializationRoot( + options: ApprovedArtifactPreflightOptions, +): string { + return options.existingState?.worktreePath + ? join(options.config.repoRoot, options.existingState.worktreePath) + : options.config.repoRoot; +} + function artifactDirs( options: ApprovedArtifactPreflightOptions, kind: "spec" | "plan", @@ -181,7 +189,7 @@ export async function assertApprovedArtifactsResolvable( }; try { await assertIssueArtifactSourcesMaterializable({ - repoRoot: options.config.repoRoot, + repoRoot: artifactMaterializationRoot(options), issueNumber: options.issue.number, sources: approvedSources, }); From da6e6de507f31a0240d872f1e9ba7dc876ba3c61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:35:25 +0200 Subject: [PATCH 10/25] fix(run-once): validate approved resume identity --- .../approval-artifact-preflight.test.ts | 28 +++++++++++++++++++ .../run-once/approval-artifact-preflight.ts | 17 ++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 92bd2d3..33c49b6 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -222,6 +222,34 @@ test("approved published artifact rejects a conflicting saved worktree target", ); }); +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("preflight uses configured approval label names", async () => { const { config, issue } = await fixture(); config.approvalPolicy = createWorkflowApprovalPolicy({ diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 230414d..b9eadab 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -9,7 +9,10 @@ import { type ResolvedPlanningArtifacts, } from "./planning-artifacts.ts"; import { pathExists } from "./paths.ts"; -import { mirrorConfiguredPathInWorktree } from "./pipeline-workspace.ts"; +import { + mirrorConfiguredPathInWorktree, + resumePlanningArtifactPolicy, +} from "./pipeline-workspace.ts"; import type { AgentIssueConfig, AgentIssueRunState, @@ -31,6 +34,18 @@ function preflightPolicy( options: ApprovedArtifactPreflightOptions, ): PlanningArtifactPolicy { const { config, existingState } = options; + if ( + existingState?.worktreePath && + (existingState.specPath || existingState.planPath) + ) { + return resumePlanningArtifactPolicy({ + config, + worktreePath: existingState.worktreePath, + existingState, + resolvedArtifacts: options.resolvedArtifacts, + }); + } + const worktreeRoot = existingState?.worktreePath ? join(config.repoRoot, existingState.worktreePath) : undefined; From ff671076e19b7c14e9d2f58f39e84777ba35ebb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:43:43 +0200 Subject: [PATCH 11/25] fix(run-once): validate approved fallback artifacts --- .../approval-artifact-preflight.test.ts | 28 +++++++ .../run-once/approval-artifact-preflight.ts | 73 +++++++++++-------- 2 files changed, 71 insertions(+), 30 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 33c49b6..1568759 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -222,6 +222,34 @@ test("approved published artifact rejects a conflicting saved worktree target", ); }); +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 must match a saved implementation resume plan", async () => { const { config, issue } = await fixture(); issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index b9eadab..fc488d5 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -92,11 +92,14 @@ function preflightPolicy( } function artifactMaterializationRoot( - options: ApprovedArtifactPreflightOptions, + policy: PlanningArtifactPolicy, + artifact: ResolvedPlanningArtifacts["spec"], ): string { - return options.existingState?.worktreePath - ? join(options.config.repoRoot, options.existingState.worktreePath) - : options.config.repoRoot; + const roots = [policy.primary, ...(policy.fallbacks ?? [])]; + return ( + roots.find((root) => root.source === artifact.rootSource)?.repoRoot ?? + policy.primary.repoRoot + ); } function artifactDirs( @@ -194,35 +197,11 @@ export async function assertApprovedArtifactsResolvable( await assertUnambiguousDiscovery(options, "plan", planLabel); } - const approvedSources = { - ...(requiresSpec && options.resolvedArtifacts.spec - ? { spec: options.resolvedArtifacts.spec } - : {}), - ...(requiresPlan && options.resolvedArtifacts.plan - ? { plan: options.resolvedArtifacts.plan } - : {}), - }; - try { - await assertIssueArtifactSourcesMaterializable({ - repoRoot: artifactMaterializationRoot(options), - issueNumber: options.issue.number, - sources: approvedSources, - }); - } catch (error) { - const labels = [ - ...(requiresSpec && approvedSources.spec ? [specLabel] : []), - ...(requiresPlan && approvedSources.plan ? [planLabel] : []), - ].join(", "); - const message = error instanceof Error ? error.message : String(error); - throw new PlanningArtifactSafetyError( - `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts cannot be materialized: ${message}`, - ); - } - + const policy = preflightPolicy(options); let artifacts: ResolvedPlanningArtifacts; try { artifacts = await resolvePlanningArtifacts({ - policy: preflightPolicy(options), + policy, issue: options.issue, now: options.now, }); @@ -245,4 +224,38 @@ export async function assertApprovedArtifactsResolvable( if (requiresPlan && !artifacts.plan.exists) { throw missingApprovedArtifact(options.issue, planLabel, "plan"); } + + const approvedSources = { + ...(requiresSpec && options.resolvedArtifacts.spec + ? { spec: options.resolvedArtifacts.spec } + : {}), + ...(requiresPlan && options.resolvedArtifacts.plan + ? { plan: options.resolvedArtifacts.plan } + : {}), + }; + try { + if (approvedSources.spec) { + await assertIssueArtifactSourcesMaterializable({ + repoRoot: artifactMaterializationRoot(policy, artifacts.spec), + issueNumber: options.issue.number, + sources: { spec: approvedSources.spec }, + }); + } + if (approvedSources.plan) { + await assertIssueArtifactSourcesMaterializable({ + repoRoot: artifactMaterializationRoot(policy, artifacts.plan), + issueNumber: options.issue.number, + sources: { plan: approvedSources.plan }, + }); + } + } catch (error) { + const labels = [ + ...(requiresSpec && approvedSources.spec ? [specLabel] : []), + ...(requiresPlan && approvedSources.plan ? [planLabel] : []), + ].join(", "); + const message = error instanceof Error ? error.message : String(error); + throw new PlanningArtifactSafetyError( + `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts cannot be materialized: ${message}`, + ); + } } From 377730f9ba5e6a0cf04b502c69408aeeb740720e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 11:52:46 +0200 Subject: [PATCH 12/25] fix(run-once): reuse approved partial resume artifacts --- .../approval-artifact-preflight.test.ts | 34 ++++++++++++++++++ src/cli/commands/run-once/pipeline.ts | 28 +++++++++++++++ .../run-once/planning-artifacts.test.ts | 34 ++++++++++++++++++ .../commands/run-once/planning-artifacts.ts | 36 +++++++++++++------ 4 files changed, 122 insertions(+), 10 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 1568759..f8342e3 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -250,6 +250,40 @@ test("approved published artifact rejects a conflicting fallback resume target", ); }); +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("approved explicit plan must match a saved implementation resume plan", async () => { const { config, issue } = await fixture(); issue.labels = [config.approvalPolicy.planApproval.approvedLabel]; diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index c51e656..53b7560 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -462,6 +462,34 @@ export async function runOneIssue( } if (artifactPolicy?.kind === "implementation-resume") { + 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, + }; + } await resolvePlanningArtifacts({ policy: artifactPolicy, issue: issueForRun, diff --git a/src/cli/commands/run-once/planning-artifacts.test.ts b/src/cli/commands/run-once/planning-artifacts.test.ts index 463078c..493151f 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 106043f..ce48bac 100644 --- a/src/cli/commands/run-once/planning-artifacts.ts +++ b/src/cli/commands/run-once/planning-artifacts.ts @@ -249,14 +249,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 +285,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( From f4e35d9959c8838b6a9f2bed13f22be97903545f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 12:03:24 +0200 Subject: [PATCH 13/25] fix(run-once): recover approved resume worktrees --- .../pipeline-workspace-scenarios.test.ts | 69 +++++++++- src/cli/commands/run-once/pipeline.ts | 124 ++++++++++-------- 2 files changed, 137 insertions(+), 56 deletions(-) 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 b3c005e..e0277b8 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,73 @@ 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 before 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 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 result = await baseRunner.run(...args); + const [command, commandArgs] = 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 firstHostMutation = runner.calls.findIndex( + (call) => + call.command === "tea" && + ((call.args[0] === "issues" && call.args[1] === "edit") || + call.args[0] === "comment"), + ); + assert.ok(worktreeAdd >= 0, "expected saved worktree recreation"); + assert.ok( + firstHostMutation < 0 || worktreeAdd < firstHostMutation, + "expected worktree recreation before host mutations", + ); +}); + test("runOneIssue resumes clean blocked implementation workspace after external prerequisite is fixed", async () => { const config = await makeConfig({ dryRun: false, diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 53b7560..b24df36 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -282,61 +282,6 @@ export async function runOneIssue( }); issueForRun = artifactSources.issue; resolvedArtifacts = artifactSources.resolvedArtifacts; - await assertApprovedArtifactsResolvable({ - config, - issue: issueForRun, - existingState, - resolvedArtifacts, - now: runOptions.now ?? new Date(), - }); - - 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; @@ -421,6 +366,75 @@ export async function runOneIssue( return worktree; }; + const { ready, inProgress, done, needsInfo } = lifecycleLabels(config); + const hasApprovedArtifact = [ + config.approvalPolicy.specApproval.approvedLabel, + config.approvalPolicy.planApproval.approvedLabel, + ].some((label) => issueForRun.labels.includes(label)); + if ( + hasApprovedArtifact && + resumableState && + existingState?.branch && + existingState.worktreePath && + (existingState.specPath || existingState.planPath) + ) { + await ensureIssueWorkspace(); + } + await assertApprovedArtifactsResolvable({ + config, + issue: issueForRun, + existingState, + resolvedArtifacts, + now: runOptions.now ?? new Date(), + }); + await progress(runOptions, "info", "git", "checking repository status", { + issueNumber: issue.number, + }); + await assertCleanWorktree(runner, config.repoRoot, ignoredPaths); + + 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)); From 7237b5612acd86febb3e297bdfd9b340f30752f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 12:23:01 +0200 Subject: [PATCH 14/25] refactor(run-once): centralize approved artifact resolution --- .../run-once/approval-artifact-preflight.ts | 283 +++++++----------- .../commands/run-once/pipeline-workspace.ts | 54 +++- src/cli/commands/run-once/pipeline.ts | 34 +-- .../commands/run-once/planning-artifacts.ts | 69 ++++- 4 files changed, 238 insertions(+), 202 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index fc488d5..3b3f6e2 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -1,16 +1,14 @@ -import { basename, isAbsolute, join } from "node:path"; import { assertIssueArtifactSourcesMaterializable } from "./artifact-source-materialization.ts"; -import { findIssueArtifacts } from "./artifacts.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; import { PlanningArtifactSafetyError, - resolvePlanningArtifacts, + planningArtifactRoot, + resolveApprovedPlanningArtifacts, type PlanningArtifactPolicy, type ResolvedPlanningArtifacts, } from "./planning-artifacts.ts"; -import { pathExists } from "./paths.ts"; import { - mirrorConfiguredPathInWorktree, + freshPlanningArtifactPolicy, resumePlanningArtifactPolicy, } from "./pipeline-workspace.ts"; import type { @@ -28,147 +26,38 @@ export type ApprovedArtifactPreflightOptions = { existingState?: AgentIssueRunState; resolvedArtifacts: ResolvedIssueArtifactSources; now: Date; + ensureArtifactWorkspace?: () => Promise; }; -function preflightPolicy( +export type ApprovedArtifactPreflight = { + policy: PlanningArtifactPolicy; + artifacts: ResolvedPlanningArtifacts; +}; + +function hasSavedArtifacts(state: AgentIssueRunState | undefined): boolean { + return !!(state?.specPath || state?.planPath); +} + +function approvedArtifactPolicy( options: ApprovedArtifactPreflightOptions, ): PlanningArtifactPolicy { - const { config, existingState } = options; - if ( - existingState?.worktreePath && - (existingState.specPath || existingState.planPath) - ) { + const { config, existingState, resolvedArtifacts } = options; + if (existingState?.worktreePath && hasSavedArtifacts(existingState)) { return resumePlanningArtifactPolicy({ config, worktreePath: existingState.worktreePath, existingState, - resolvedArtifacts: options.resolvedArtifacts, + resolvedArtifacts, }); } - 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, - }, + return freshPlanningArtifactPolicy({ + config, + existingState, + resolvedArtifacts, allowGeneratedSpec: false, allowGeneratedPlan: false, - }; -} - -function artifactMaterializationRoot( - policy: PlanningArtifactPolicy, - artifact: ResolvedPlanningArtifacts["spec"], -): string { - const roots = [policy.primary, ...(policy.fallbacks ?? [])]; - return ( - roots.find((root) => root.source === artifact.rootSource)?.repoRoot ?? - policy.primary.repoRoot - ); -} - -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 savedArtifactExists( - options: ApprovedArtifactPreflightOptions, - kind: "spec" | "plan", -): Promise { - const savedPath = - kind === "spec" - ? options.existingState?.specPath - : options.existingState?.planPath; - if (!savedPath) return false; - if (isAbsolute(savedPath)) return pathExists(savedPath); - - const roots = [options.config.repoRoot]; - if (options.existingState?.worktreePath) { - roots.unshift( - join(options.config.repoRoot, options.existingState.worktreePath), - ); - } - return ( - await Promise.all(roots.map((root) => pathExists(join(root, savedPath)))) - ).some(Boolean); -} - -async function assertUnambiguousDiscovery( - options: ApprovedArtifactPreflightOptions, - kind: "spec" | "plan", - label: string, -): Promise { - if (options.resolvedArtifacts[kind]) return; - if (await savedArtifactExists(options, kind)) 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( @@ -181,35 +70,88 @@ function missingApprovedArtifact( ); } +async function assertApprovedSourcesMaterializable(input: { + issue: IssueSummary; + policy: PlanningArtifactPolicy; + artifacts: ResolvedPlanningArtifacts; + sources: ResolvedIssueArtifactSources; + requireSpec: boolean; + requirePlan: boolean; + specLabel: string; + planLabel: string; +}): 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 { + await assertIssueArtifactSourcesMaterializable({ + repoRoot: planningArtifactRoot(input.policy, entry.artifact).repoRoot, + issueNumber: input.issue.number, + sources: { [entry.kind]: entry.source }, + }); + } 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 { +): 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; + const requireSpec = options.issue.labels.includes(specLabel); + const requirePlan = options.issue.labels.includes(planLabel); + if (!requireSpec && !requirePlan) return undefined; - if (requiresSpec) { - await assertUnambiguousDiscovery(options, "spec", specLabel); - } - if (requiresPlan) { - await assertUnambiguousDiscovery(options, "plan", planLabel); + if ( + options.ensureArtifactWorkspace && + options.existingState?.branch && + options.existingState.worktreePath && + hasSavedArtifacts(options.existingState) + ) { + await options.ensureArtifactWorkspace(); } - const policy = preflightPolicy(options); + const policy = approvedArtifactPolicy(options); let artifacts: ResolvedPlanningArtifacts; try { - artifacts = await resolvePlanningArtifacts({ + artifacts = await resolveApprovedPlanningArtifacts({ policy, issue: options.issue, now: options.now, + requireSpec, + requirePlan, }); } catch (error) { if (error instanceof PlanningArtifactSafetyError) { const labels = [ - ...(requiresSpec ? [specLabel] : []), - ...(requiresPlan ? [planLabel] : []), + ...(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}`, @@ -218,44 +160,23 @@ export async function assertApprovedArtifactsResolvable( throw error; } - if (requiresSpec && !artifacts.spec.exists) { + if (requireSpec && !artifacts.spec.exists) { throw missingApprovedArtifact(options.issue, specLabel, "spec"); } - if (requiresPlan && !artifacts.plan.exists) { + if (requirePlan && !artifacts.plan.exists) { throw missingApprovedArtifact(options.issue, planLabel, "plan"); } - const approvedSources = { - ...(requiresSpec && options.resolvedArtifacts.spec - ? { spec: options.resolvedArtifacts.spec } - : {}), - ...(requiresPlan && options.resolvedArtifacts.plan - ? { plan: options.resolvedArtifacts.plan } - : {}), - }; - try { - if (approvedSources.spec) { - await assertIssueArtifactSourcesMaterializable({ - repoRoot: artifactMaterializationRoot(policy, artifacts.spec), - issueNumber: options.issue.number, - sources: { spec: approvedSources.spec }, - }); - } - if (approvedSources.plan) { - await assertIssueArtifactSourcesMaterializable({ - repoRoot: artifactMaterializationRoot(policy, artifacts.plan), - issueNumber: options.issue.number, - sources: { plan: approvedSources.plan }, - }); - } - } catch (error) { - const labels = [ - ...(requiresSpec && approvedSources.spec ? [specLabel] : []), - ...(requiresPlan && approvedSources.plan ? [planLabel] : []), - ].join(", "); - const message = error instanceof Error ? error.message : String(error); - throw new PlanningArtifactSafetyError( - `Issue #${options.issue.number} has approval label ${labels}, but approved artifacts cannot be materialized: ${message}`, - ); - } + await assertApprovedSourcesMaterializable({ + issue: options.issue, + policy, + artifacts, + sources: options.resolvedArtifacts, + requireSpec, + requirePlan, + specLabel, + planLabel, + }); + + return { policy, artifacts }; } diff --git a/src/cli/commands/run-once/pipeline-workspace.ts b/src/cli/commands/run-once/pipeline-workspace.ts index ce9fbde..e1406f6 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,58 @@ export function resumePlanningArtifactPolicy(input: { }; } +export function freshPlanningArtifactPolicy(input: { + config: Pick; + existingState?: AgentIssueRunState; + resolvedArtifacts: ResolvedIssueArtifactSources; + allowGeneratedSpec: boolean; + allowGeneratedPlan: boolean; +}): PlanningArtifactPolicy { + const worktreeRoot = 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 b24df36..f727040 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -367,26 +367,19 @@ export async function runOneIssue( }; const { ready, inProgress, done, needsInfo } = lifecycleLabels(config); - const hasApprovedArtifact = [ - config.approvalPolicy.specApproval.approvedLabel, - config.approvalPolicy.planApproval.approvedLabel, - ].some((label) => issueForRun.labels.includes(label)); - if ( - hasApprovedArtifact && - resumableState && - existingState?.branch && - existingState.worktreePath && - (existingState.specPath || existingState.planPath) - ) { - await ensureIssueWorkspace(); - } - await assertApprovedArtifactsResolvable({ + const approvedArtifactPreflight = await assertApprovedArtifactsResolvable({ config, issue: issueForRun, existingState, resolvedArtifacts, now: runOptions.now ?? new Date(), + ensureArtifactWorkspace: async () => { + await ensureIssueWorkspace(); + }, }); + if (approvedArtifactPreflight?.policy.kind === "implementation-resume") { + artifactPolicy = approvedArtifactPreflight.policy; + } await progress(runOptions, "info", "git", "checking repository status", { issueNumber: issue.number, }); @@ -452,6 +445,7 @@ export async function runOneIssue( } if ( + !artifactPolicy && resumableState && existingState && (existingState.branch || existingState.worktreePath) && @@ -504,11 +498,13 @@ export async function runOneIssue( explicit: resolvedArtifacts, }; } - await resolvePlanningArtifacts({ - policy: artifactPolicy, - issue: issueForRun, - now: runOptions.now ?? new Date(), - }); + if (!approvedArtifactPreflight) { + await resolvePlanningArtifacts({ + policy: artifactPolicy, + issue: issueForRun, + now: runOptions.now ?? new Date(), + }); + } } else { const artifactWorktree = resolvedArtifacts.spec || resolvedArtifacts.plan diff --git a/src/cli/commands/run-once/planning-artifacts.ts b/src/cli/commands/run-once/planning-artifacts.ts index ce48bac..3f68171 100644 --- a/src/cli/commands/run-once/planning-artifacts.ts +++ b/src/cli/commands/run-once/planning-artifacts.ts @@ -1,4 +1,5 @@ -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 { buildPlanPath, findIssuePlan } from "./plans.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 }; @@ -368,3 +379,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; +} From 0754d4be38dc0b7054f0eeb09d24a3d193ef7fe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 12:33:55 +0200 Subject: [PATCH 15/25] fix(run-once): preflight approved artifact worktrees --- .../run-once/approval-artifact-preflight.ts | 40 ++++++++++------- .../run-once/pipeline-planning.test.ts | 44 +++++++++++++++++-- .../commands/run-once/pipeline-workspace.ts | 9 ++-- src/cli/commands/run-once/pipeline.ts | 24 +++++++++- .../commands/run-once/stage-advancement.ts | 19 ++++++-- 5 files changed, 108 insertions(+), 28 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 3b3f6e2..28e3ab4 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -26,7 +26,7 @@ export type ApprovedArtifactPreflightOptions = { existingState?: AgentIssueRunState; resolvedArtifacts: ResolvedIssueArtifactSources; now: Date; - ensureArtifactWorkspace?: () => Promise; + ensureArtifactWorkspace?: () => Promise; }; export type ApprovedArtifactPreflight = { @@ -38,11 +38,26 @@ function hasSavedArtifacts(state: AgentIssueRunState | undefined): boolean { return !!(state?.specPath || state?.planPath); } -function approvedArtifactPolicy( - options: ApprovedArtifactPreflightOptions, -): PlanningArtifactPolicy { +async function approvedArtifactPolicy(input: { + options: ApprovedArtifactPreflightOptions; + requireSpec: boolean; + requirePlan: boolean; +}): Promise { + const { options, requireSpec, requirePlan } = input; const { config, existingState, resolvedArtifacts } = options; - if (existingState?.worktreePath && hasSavedArtifacts(existingState)) { + const hasApprovedSource = + (requireSpec && !!resolvedArtifacts.spec) || + (requirePlan && !!resolvedArtifacts.plan); + const needsSavedWorkspace = + !!existingState?.worktreePath && hasSavedArtifacts(existingState); + + if ( + options.ensureArtifactWorkspace && + (needsSavedWorkspace || hasApprovedSource) + ) { + return await options.ensureArtifactWorkspace(); + } + if (needsSavedWorkspace) { return resumePlanningArtifactPolicy({ config, worktreePath: existingState.worktreePath, @@ -128,16 +143,11 @@ export async function assertApprovedArtifactsResolvable( const requirePlan = options.issue.labels.includes(planLabel); if (!requireSpec && !requirePlan) return undefined; - if ( - options.ensureArtifactWorkspace && - options.existingState?.branch && - options.existingState.worktreePath && - hasSavedArtifacts(options.existingState) - ) { - await options.ensureArtifactWorkspace(); - } - - const policy = approvedArtifactPolicy(options); + const policy = await approvedArtifactPolicy({ + options, + requireSpec, + requirePlan, + }); let artifacts: ResolvedPlanningArtifacts; try { artifacts = await resolveApprovedPlanningArtifacts({ diff --git a/src/cli/commands/run-once/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index cdc1c51..e34a866 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,12 +206,11 @@ 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 artifact conflicts before claiming", async () => { +test("runOneIssue rejects approved published worktree conflicts before claiming", async () => { const config = await makeConfig({ dryRun: false, execute: true }); - const specPath = "docs/specs/conflicting-approved-spec.md"; - await writeFile(join(config.repoRoot, specPath), "# Existing spec\n", "utf8"); + const specPath = "docs/specs/conflicting-worktree-approved-spec.md"; const selected = { - ...issue(65, ["spec-approved", "enhancement"], "Conflicting spec"), + ...issue(66, ["spec-approved", "enhancement"], "Conflicting worktree spec"), comments: [ { authorLogin: "patchmill-bot", @@ -219,6 +222,19 @@ test("runOneIssue rejects approved published artifact conflicts before claiming" }, ], }; + const workspace = expectedIssueWorkspace( + selected.number, + selected.title, + configuredWorktreeStrategy(config), + ); + await mkdir(join(config.repoRoot, workspace.worktreePath, "docs", "specs"), { + recursive: true, + }); + await writeFile( + join(config.repoRoot, workspace.worktreePath, specPath), + "# Existing worktree spec\n", + "utf8", + ); const runner = createMockRunner(async (call) => { if ( call.command === "tea" && @@ -235,6 +251,26 @@ test("runOneIssue rejects approved published artifact conflicts before claiming" 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: `worktree ${join(config.repoRoot, workspace.worktreePath)}\n`, + 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, diff --git a/src/cli/commands/run-once/pipeline-workspace.ts b/src/cli/commands/run-once/pipeline-workspace.ts index e1406f6..f3d0f02 100644 --- a/src/cli/commands/run-once/pipeline-workspace.ts +++ b/src/cli/commands/run-once/pipeline-workspace.ts @@ -95,10 +95,13 @@ export function freshPlanningArtifactPolicy(input: { resolvedArtifacts: ResolvedIssueArtifactSources; allowGeneratedSpec: boolean; allowGeneratedPlan: boolean; + workspaceRoot?: string; }): PlanningArtifactPolicy { - const worktreeRoot = input.existingState?.worktreePath - ? join(input.config.repoRoot, input.existingState.worktreePath) - : undefined; + const worktreeRoot = + input.workspaceRoot ?? + (input.existingState?.worktreePath + ? join(input.config.repoRoot, input.existingState.worktreePath) + : undefined); const primaryRoot = worktreeRoot ?? input.config.repoRoot; return { diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index f727040..59e0e08 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -55,6 +55,7 @@ import { cleanStatusIgnoredPaths, configuredWorktreeStrategy, expectedIssueWorkspace, + freshPlanningArtifactPolicy, resumePlanningArtifactPolicy, } from "./pipeline-workspace.ts"; import { @@ -374,10 +375,29 @@ export async function runOneIssue( resolvedArtifacts, now: runOptions.now ?? new Date(), ensureArtifactWorkspace: async () => { - await ensureIssueWorkspace(); + const workspace = await ensureIssueWorkspace(); + const workspaceRoot = join(config.repoRoot, workspace.worktreePath); + if ( + existingState?.worktreePath && + (existingState.specPath || existingState.planPath) + ) { + return resumePlanningArtifactPolicy({ + config, + worktreePath: workspace.worktreePath, + existingState, + resolvedArtifacts, + }); + } + return freshPlanningArtifactPolicy({ + config, + resolvedArtifacts, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + workspaceRoot, + }); }, }); - if (approvedArtifactPreflight?.policy.kind === "implementation-resume") { + if (approvedArtifactPreflight) { artifactPolicy = approvedArtifactPreflight.policy; } await progress(runOptions, "info", "git", "checking repository status", { diff --git a/src/cli/commands/run-once/stage-advancement.ts b/src/cli/commands/run-once/stage-advancement.ts index a970ca7..3f0fc86 100644 --- a/src/cli/commands/run-once/stage-advancement.ts +++ b/src/cli/commands/run-once/stage-advancement.ts @@ -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,7 +341,14 @@ 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, From 9377dd5357694783d6f9b8f304fbb4a3e821b404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 12:47:02 +0200 Subject: [PATCH 16/25] fix(run-once): preserve branch-only artifact resumes --- .../approval-artifact-preflight.test.ts | 69 +++++++++++++++++++ .../run-once/approval-artifact-preflight.ts | 46 +++++++------ .../commands/run-once/pipeline-workspace.ts | 37 ++++++++++ src/cli/commands/run-once/pipeline.ts | 32 ++------- 4 files changed, 137 insertions(+), 47 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index f8342e3..d17cc38 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -284,6 +284,48 @@ test("approved explicit plan passes preflight with a saved spec and no saved pla ); }); +test("approved branch-only resume resolves saved artifacts in the ensured 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", + ); + let ensured = false; + + 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, + ensureArtifactWorkspace: async () => { + ensured = true; + return { worktreePath }; + }, + }); + + assert.equal(ensured, true); + 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]; @@ -312,6 +354,33 @@ test("approved explicit plan must match a saved implementation resume plan", asy ); }); +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, + ensureArtifactWorkspace: async () => ({ 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({ diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 28e3ab4..a43b36e 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -9,7 +9,8 @@ import { } from "./planning-artifacts.ts"; import { freshPlanningArtifactPolicy, - resumePlanningArtifactPolicy, + hasSavedPlanningArtifactWorkspace, + planningArtifactPolicyForWorkspace, } from "./pipeline-workspace.ts"; import type { AgentIssueConfig, @@ -26,7 +27,7 @@ export type ApprovedArtifactPreflightOptions = { existingState?: AgentIssueRunState; resolvedArtifacts: ResolvedIssueArtifactSources; now: Date; - ensureArtifactWorkspace?: () => Promise; + ensureArtifactWorkspace?: () => Promise<{ worktreePath: string }>; }; export type ApprovedArtifactPreflight = { @@ -34,10 +35,6 @@ export type ApprovedArtifactPreflight = { artifacts: ResolvedPlanningArtifacts; }; -function hasSavedArtifacts(state: AgentIssueRunState | undefined): boolean { - return !!(state?.specPath || state?.planPath); -} - async function approvedArtifactPolicy(input: { options: ApprovedArtifactPreflightOptions; requireSpec: boolean; @@ -48,22 +45,29 @@ async function approvedArtifactPolicy(input: { const hasApprovedSource = (requireSpec && !!resolvedArtifacts.spec) || (requirePlan && !!resolvedArtifacts.plan); - const needsSavedWorkspace = - !!existingState?.worktreePath && hasSavedArtifacts(existingState); + const needsSavedWorkspace = hasSavedPlanningArtifactWorkspace(existingState); - if ( - options.ensureArtifactWorkspace && - (needsSavedWorkspace || hasApprovedSource) - ) { - return await options.ensureArtifactWorkspace(); - } - if (needsSavedWorkspace) { - return resumePlanningArtifactPolicy({ - config, - worktreePath: existingState.worktreePath, - existingState, - resolvedArtifacts, - }); + if (needsSavedWorkspace || hasApprovedSource) { + const workspace = options.ensureArtifactWorkspace + ? await options.ensureArtifactWorkspace() + : existingState?.worktreePath + ? { worktreePath: existingState.worktreePath } + : undefined; + if (workspace) { + return planningArtifactPolicyForWorkspace({ + config, + existingState, + resolvedArtifacts, + worktreePath: workspace.worktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } + if (needsSavedWorkspace) { + throw new PlanningArtifactSafetyError( + `Saved planning artifacts require an ensured workspace before approval preflight`, + ); + } } return freshPlanningArtifactPolicy({ diff --git a/src/cli/commands/run-once/pipeline-workspace.ts b/src/cli/commands/run-once/pipeline-workspace.ts index f3d0f02..129a05c 100644 --- a/src/cli/commands/run-once/pipeline-workspace.ts +++ b/src/cli/commands/run-once/pipeline-workspace.ts @@ -89,6 +89,43 @@ 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; diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 59e0e08..96693b4 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -55,8 +55,7 @@ import { cleanStatusIgnoredPaths, configuredWorktreeStrategy, expectedIssueWorkspace, - freshPlanningArtifactPolicy, - resumePlanningArtifactPolicy, + planningArtifactPolicyForWorkspace, } from "./pipeline-workspace.ts"; import { emitSelectionDiagnostics, @@ -374,28 +373,7 @@ export async function runOneIssue( existingState, resolvedArtifacts, now: runOptions.now ?? new Date(), - ensureArtifactWorkspace: async () => { - const workspace = await ensureIssueWorkspace(); - const workspaceRoot = join(config.repoRoot, workspace.worktreePath); - if ( - existingState?.worktreePath && - (existingState.specPath || existingState.planPath) - ) { - return resumePlanningArtifactPolicy({ - config, - worktreePath: workspace.worktreePath, - existingState, - resolvedArtifacts, - }); - } - return freshPlanningArtifactPolicy({ - config, - resolvedArtifacts, - allowGeneratedSpec: false, - allowGeneratedPlan: false, - workspaceRoot, - }); - }, + ensureArtifactWorkspace: ensureIssueWorkspace, }); if (approvedArtifactPreflight) { artifactPolicy = approvedArtifactPreflight.policy; @@ -474,11 +452,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, From 24a0809d0a871cfe3b067aefd008037a46472657 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 13:45:58 +0200 Subject: [PATCH 17/25] fix(run-once): validate approved artifact files --- .../approval-artifact-preflight.test.ts | 25 ++++++++++++++++ src/cli/commands/run-once/paths.ts | 13 ++++++++- .../run-once/pipeline-planning.test.ts | 29 ++++++++++++++++++- .../commands/run-once/planning-artifacts.ts | 7 ++++- .../commands/run-once/stage-advancement.ts | 2 +- 5 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index d17cc38..a68c76e 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -284,6 +284,31 @@ test("approved explicit plan passes preflight with a saved spec and no saved pla ); }); +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 ensured workspace", async () => { const { config, issue } = await fixture(); issue.labels = [config.approvalPolicy.specApproval.approvedLabel]; diff --git a/src/cli/commands/run-once/paths.ts b/src/cli/commands/run-once/paths.ts index 3ef15a1..74e1196 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-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index e34a866..1f3145a 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -1363,6 +1363,15 @@ test("runOneIssue writes a plan and preserves spec approval at plan review", asy 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" && @@ -1379,6 +1388,23 @@ test("runOneIssue writes a plan and preserves spec approval at plan review", asy 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" && @@ -1399,7 +1425,8 @@ test("runOneIssue writes a plan and preserves spec approval at plan review", asy 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"); diff --git a/src/cli/commands/run-once/planning-artifacts.ts b/src/cli/commands/run-once/planning-artifacts.ts index 3f68171..7e1adab 100644 --- a/src/cli/commands/run-once/planning-artifacts.ts +++ b/src/cli/commands/run-once/planning-artifacts.ts @@ -1,7 +1,7 @@ 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"; @@ -137,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, diff --git a/src/cli/commands/run-once/stage-advancement.ts b/src/cli/commands/run-once/stage-advancement.ts index 3f0fc86..438fcea 100644 --- a/src/cli/commands/run-once/stage-advancement.ts +++ b/src/cli/commands/run-once/stage-advancement.ts @@ -355,7 +355,7 @@ export async function advancePlanningStages({ now, }); if ( - !artifactPolicy && + artifactPolicyForRun.kind === "fresh" && ensurePlanningArtifactWorkspace && (planningArtifacts.plan.generated || (!planningArtifacts.plan.exists && planningArtifacts.spec.generated)) From 77a9b46ad47749b93fc2bec977444c827856e1dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 14:02:56 +0200 Subject: [PATCH 18/25] fix(run-once): retain retry worktree state --- .../run-once/development-environment-stage.ts | 2 + .../pipeline-development-environment.test.ts | 147 +++++++++++++++++- 2 files changed, 147 insertions(+), 2 deletions(-) diff --git a/src/cli/commands/run-once/development-environment-stage.ts b/src/cli/commands/run-once/development-environment-stage.ts index a0a2a92..ee68a3e 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/pipeline-development-environment.test.ts b/src/cli/commands/run-once/pipeline-development-environment.test.ts index e7398d6..1c8212a 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, @@ -376,6 +376,149 @@ test("runOneIssue preserves approval labels after development environment failur ); }); +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, + 1, + ); + 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", + ); +}); + test("runOneIssue restores a retryable label after resumed development environment failure", async () => { const planPath = "docs/plans/2026-05-14-issue-48-resumed-not-ready.md"; const config = await makeConfig({ From 8976ee2fd46b716d03d1849898bfda54cc3d7269 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 14:25:00 +0200 Subject: [PATCH 19/25] fix(run-once): keep approval preflight read-only --- .../approval-artifact-preflight.test.ts | 12 +++--- .../run-once/approval-artifact-preflight.ts | 13 +++---- .../run-once/pipeline-planning.test.ts | 38 ++++++++++++++----- .../pipeline-workspace-scenarios.test.ts | 15 ++++---- src/cli/commands/run-once/pipeline.ts | 33 ++++++++++++++-- 5 files changed, 76 insertions(+), 35 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index a68c76e..97714a6 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -309,7 +309,7 @@ test("saved approved spec directory fails preflight", async () => { ); }); -test("approved branch-only resume resolves saved artifacts in the ensured workspace", async () => { +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"; @@ -322,7 +322,7 @@ test("approved branch-only resume resolves saved artifacts in the ensured worksp "# Saved spec\n", "utf8", ); - let ensured = false; + let resolvedWorkspace = false; const preflight = await assertApprovedArtifactsResolvable({ config, @@ -339,13 +339,13 @@ test("approved branch-only resume resolves saved artifacts in the ensured worksp }, resolvedArtifacts: {}, now, - ensureArtifactWorkspace: async () => { - ensured = true; + resolveArtifactWorkspace: async () => { + resolvedWorkspace = true; return { worktreePath }; }, }); - assert.equal(ensured, true); + assert.equal(resolvedWorkspace, true); assert.equal(preflight?.policy.kind, "implementation-resume"); assert.equal(preflight?.artifacts.spec.path, specPath); assert.equal(preflight?.artifacts.spec.exists, true); @@ -400,7 +400,7 @@ test("approved branch-only resume rejects an explicit artifact that differs from }, resolvedArtifacts: { plan: source(config.repoRoot, "plan") }, now, - ensureArtifactWorkspace: async () => ({ worktreePath }), + resolveArtifactWorkspace: async () => ({ worktreePath }), }), /plan-approved.*Explicit plan artifact.*does not match saved plan/u, ); diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index a43b36e..707c580 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -27,7 +27,9 @@ export type ApprovedArtifactPreflightOptions = { existingState?: AgentIssueRunState; resolvedArtifacts: ResolvedIssueArtifactSources; now: Date; - ensureArtifactWorkspace?: () => Promise<{ worktreePath: string }>; + resolveArtifactWorkspace?: () => Promise< + { worktreePath: string } | undefined + >; }; export type ApprovedArtifactPreflight = { @@ -48,8 +50,8 @@ async function approvedArtifactPolicy(input: { const needsSavedWorkspace = hasSavedPlanningArtifactWorkspace(existingState); if (needsSavedWorkspace || hasApprovedSource) { - const workspace = options.ensureArtifactWorkspace - ? await options.ensureArtifactWorkspace() + const workspace = options.resolveArtifactWorkspace + ? await options.resolveArtifactWorkspace() : existingState?.worktreePath ? { worktreePath: existingState.worktreePath } : undefined; @@ -63,11 +65,6 @@ async function approvedArtifactPolicy(input: { allowGeneratedPlan: false, }); } - if (needsSavedWorkspace) { - throw new PlanningArtifactSafetyError( - `Saved planning artifacts require an ensured workspace before approval preflight`, - ); - } } return freshPlanningArtifactPolicy({ diff --git a/src/cli/commands/run-once/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index 1f3145a..a02e1e3 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -227,12 +227,9 @@ test("runOneIssue rejects approved published worktree conflicts before claiming" selected.title, configuredWorktreeStrategy(config), ); - await mkdir(join(config.repoRoot, workspace.worktreePath, "docs", "specs"), { - recursive: true, - }); await writeFile( - join(config.repoRoot, workspace.worktreePath, specPath), - "# Existing worktree spec\n", + join(config.repoRoot, specPath), + "# Existing primary spec\n", "utf8", ); const runner = createMockRunner(async (call) => { @@ -256,11 +253,23 @@ test("runOneIssue rejects approved published worktree conflicts before claiming" call.args[0] === "worktree" && call.args[1] === "list" ) { - return { - code: 0, - stdout: `worktree ${join(config.repoRoot, workspace.worktreePath)}\n`, - stderr: "", - }; + 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: "" }; @@ -289,6 +298,15 @@ test("runOneIssue rejects approved published worktree conflicts before claiming" () => 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, 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 e0277b8..f1c3f5e 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -136,7 +136,7 @@ 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 before preflight", async () => { +test("runOneIssue recreates an approved implementing worktree after preflight", async () => { const config = await makeConfig({ dryRun: false, execute: true, @@ -145,6 +145,7 @@ test("runOneIssue recreates an approved implementing worktree before preflight", }); 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" }, @@ -190,16 +191,14 @@ test("runOneIssue recreates an approved implementing worktree before preflight", call.args[0] === "worktree" && call.args[1] === "add", ); - const firstHostMutation = runner.calls.findIndex( - (call) => - call.command === "tea" && - ((call.args[0] === "issues" && call.args[1] === "edit") || - call.args[0] === "comment"), + 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( - firstHostMutation < 0 || worktreeAdd < firstHostMutation, - "expected worktree recreation before host mutations", + firstCleanStatus < worktreeAdd, + "expected worktree recreation after the primary clean-worktree check", ); }); diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 96693b4..ab8ab0f 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -296,8 +296,7 @@ export async function runOneIssue( issue.title, worktreeStrategy, ); - const ensureIssueWorkspace = async (): Promise => { - if (ensuredWorktree) return ensuredWorktree; + const assertExpectedWorkspaceIdentity = (): void => { if ( resumableState && existingState?.branch && @@ -316,6 +315,34 @@ export async function runOneIssue( `Saved worktree ${existingState.worktreePath} does not match expected worktree path ${expectedWorkspace.worktreePath}`, ); } + }; + const resolveArtifactWorkspace = async (): Promise< + { worktreePath: string } | undefined + > => { + assertExpectedWorkspaceIdentity(); + const result = await runner.run( + "git", + ["worktree", "list", "--porcelain"], + { cwd: config.repoRoot }, + ); + if (result.code !== 0) { + throw new AgentIssueSafetyError( + `git worktree list failed before approval preflight with exit code ${result.code}`, + ); + } + const worktreePath = + existingState?.worktreePath ?? expectedWorkspace.worktreePath; + return result.stdout + .split("\n") + .some( + (line) => line === `worktree ${join(config.repoRoot, worktreePath)}`, + ) + ? { worktreePath } + : undefined; + }; + const ensureIssueWorkspace = async (): Promise => { + if (ensuredWorktree) return ensuredWorktree; + assertExpectedWorkspaceIdentity(); const worktree = await ensureIssueWorktree( runner, @@ -373,7 +400,7 @@ export async function runOneIssue( existingState, resolvedArtifacts, now: runOptions.now ?? new Date(), - ensureArtifactWorkspace: ensureIssueWorkspace, + resolveArtifactWorkspace, }); if (approvedArtifactPreflight) { artifactPolicy = approvedArtifactPreflight.policy; From 88d7beaffc7e72a6fed7e14f76d686703dbbe8cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 14:40:14 +0200 Subject: [PATCH 20/25] fix(run-once): validate preflight worktree identity --- .../run-once/approval-artifact-preflight.ts | 33 ++- .../pipeline-development-environment.test.ts | 2 +- .../pipeline-workspace-scenarios.test.ts | 199 ++++++++++++++++++ src/cli/commands/run-once/pipeline.ts | 35 ++- 4 files changed, 254 insertions(+), 15 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 707c580..a3c32ba 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -50,17 +50,36 @@ async function approvedArtifactPolicy(input: { const needsSavedWorkspace = hasSavedPlanningArtifactWorkspace(existingState); if (needsSavedWorkspace || hasApprovedSource) { - const workspace = options.resolveArtifactWorkspace - ? await options.resolveArtifactWorkspace() - : existingState?.worktreePath - ? { worktreePath: existingState.worktreePath } - : undefined; - if (workspace) { + if (options.resolveArtifactWorkspace) { + const workspace = await options.resolveArtifactWorkspace(); + if (workspace) { + return planningArtifactPolicyForWorkspace({ + config, + existingState, + resolvedArtifacts, + worktreePath: workspace.worktreePath, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } + + return freshPlanningArtifactPolicy({ + config, + existingState: existingState + ? { ...existingState, worktreePath: undefined } + : undefined, + resolvedArtifacts, + allowGeneratedSpec: false, + allowGeneratedPlan: false, + }); + } + + if (existingState?.worktreePath) { return planningArtifactPolicyForWorkspace({ config, existingState, resolvedArtifacts, - worktreePath: workspace.worktreePath, + worktreePath: existingState.worktreePath, allowGeneratedSpec: false, allowGeneratedPlan: 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 1c8212a..b688116 100644 --- a/src/cli/commands/run-once/pipeline-development-environment.test.ts +++ b/src/cli/commands/run-once/pipeline-development-environment.test.ts @@ -502,7 +502,7 @@ test("runOneIssue retries worktree-only approved artifacts after development env call.args[0] === "-C" && call.args[2] === "branch", ).length, - 1, + 2, ); const finalEdit = runner.calls .filter( 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 f1c3f5e..30d7b35 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -136,6 +136,205 @@ test("runOneIssue reuses existing implementation worktree on resume", async () = assert.ok(runner.calls.find((call) => call.command === "pi")); }); +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 recreates an approved implementing worktree after preflight", async () => { const config = await makeConfig({ dryRun: false, diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index ab8ab0f..fc7098d 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -1,4 +1,4 @@ -import { join } from "node:path"; +import { join, resolve } from "node:path"; import { localPiAgentDir } from "../init/pi-agent-settings.ts"; import { createRunOnceHostProvider } from "../../../host/factory.ts"; @@ -332,13 +332,34 @@ export async function runOneIssue( } const worktreePath = existingState?.worktreePath ?? expectedWorkspace.worktreePath; - return result.stdout + const expectedWorktreeRoot = resolve(config.repoRoot, worktreePath); + const registered = result.stdout .split("\n") - .some( - (line) => line === `worktree ${join(config.repoRoot, worktreePath)}`, - ) - ? { worktreePath } - : undefined; + .find( + (line) => + line.startsWith("worktree ") && + resolve(line.slice("worktree ".length)) === expectedWorktreeRoot, + ); + if (!registered) return undefined; + + const branchResult = await runner.run( + "git", + ["-C", worktreePath, "branch", "--show-current"], + { cwd: config.repoRoot }, + ); + if (branchResult.code !== 0) { + throw new AgentIssueSafetyError( + `git branch failed for ${worktreePath} before approval preflight with exit code ${branchResult.code}`, + ); + } + const currentBranch = branchResult.stdout.trim(); + if (currentBranch !== expectedWorkspace.branch) { + throw new AgentIssueSafetyError( + `Existing worktree ${worktreePath} is on ${currentBranch}, expected ${expectedWorkspace.branch}`, + ); + } + + return { worktreePath }; }; const ensureIssueWorkspace = async (): Promise => { if (ensuredWorktree) return ensuredWorktree; From 0d440b10ac91c164c2d5944c6bdea540e3ed12ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 14:53:36 +0200 Subject: [PATCH 21/25] fix(run-once): preflight approved branch artifacts --- .../approval-artifact-preflight.test.ts | 17 +-- .../run-once/approval-artifact-preflight.ts | 70 ++++++---- .../artifact-source-materialization.ts | 35 +++++ src/cli/commands/run-once/git.ts | 44 ++++++ .../run-once/pipeline-planning.test.ts | 128 ++++++++++++++++++ src/cli/commands/run-once/pipeline.ts | 65 +++------ 6 files changed, 276 insertions(+), 83 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 97714a6..86dba96 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -322,8 +322,6 @@ test("approved branch-only resume resolves saved artifacts in the resolved works "# Saved spec\n", "utf8", ); - let resolvedWorkspace = false; - const preflight = await assertApprovedArtifactsResolvable({ config, issue, @@ -339,13 +337,12 @@ test("approved branch-only resume resolves saved artifacts in the resolved works }, resolvedArtifacts: {}, now, - resolveArtifactWorkspace: async () => { - resolvedWorkspace = true; - return { worktreePath }; + artifactWorkspace: { + kind: "worktree", + branch: "agent/issue-140-keep-approved-artifacts", + worktreePath, }, }); - - assert.equal(resolvedWorkspace, true); assert.equal(preflight?.policy.kind, "implementation-resume"); assert.equal(preflight?.artifacts.spec.path, specPath); assert.equal(preflight?.artifacts.spec.exists, true); @@ -400,7 +397,11 @@ test("approved branch-only resume rejects an explicit artifact that differs from }, resolvedArtifacts: { plan: source(config.repoRoot, "plan") }, now, - resolveArtifactWorkspace: async () => ({ worktreePath }), + artifactWorkspace: { + kind: "worktree", + branch: "agent/issue-140-keep-approved-artifacts", + worktreePath, + }, }), /plan-approved.*Explicit plan artifact.*does not match saved plan/u, ); diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index a3c32ba..753cb02 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -1,4 +1,7 @@ -import { assertIssueArtifactSourcesMaterializable } from "./artifact-source-materialization.ts"; +import { + assertIssueArtifactSourcesMaterializable, + assertIssueArtifactSourcesMaterializableInBranch, +} from "./artifact-source-materialization.ts"; import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; import { PlanningArtifactSafetyError, @@ -15,8 +18,10 @@ import { import type { AgentIssueConfig, AgentIssueRunState, + CommandRunner, IssueSummary, } from "./types.ts"; +import type { ReadOnlyIssueWorkspace } from "./git.ts"; export type ApprovedArtifactPreflightOptions = { config: Pick< @@ -27,9 +32,8 @@ export type ApprovedArtifactPreflightOptions = { existingState?: AgentIssueRunState; resolvedArtifacts: ResolvedIssueArtifactSources; now: Date; - resolveArtifactWorkspace?: () => Promise< - { worktreePath: string } | undefined - >; + artifactWorkspace?: ReadOnlyIssueWorkspace; + runner?: CommandRunner; }; export type ApprovedArtifactPreflight = { @@ -50,31 +54,18 @@ async function approvedArtifactPolicy(input: { const needsSavedWorkspace = hasSavedPlanningArtifactWorkspace(existingState); if (needsSavedWorkspace || hasApprovedSource) { - if (options.resolveArtifactWorkspace) { - const workspace = await options.resolveArtifactWorkspace(); - if (workspace) { - return planningArtifactPolicyForWorkspace({ - config, - existingState, - resolvedArtifacts, - worktreePath: workspace.worktreePath, - allowGeneratedSpec: false, - allowGeneratedPlan: false, - }); - } - - return freshPlanningArtifactPolicy({ + if (options.artifactWorkspace?.kind === "worktree") { + return planningArtifactPolicyForWorkspace({ config, - existingState: existingState - ? { ...existingState, worktreePath: undefined } - : undefined, + existingState, resolvedArtifacts, + worktreePath: options.artifactWorkspace.worktreePath, allowGeneratedSpec: false, allowGeneratedPlan: false, }); } - if (existingState?.worktreePath) { + if (!options.artifactWorkspace && existingState?.worktreePath) { return planningArtifactPolicyForWorkspace({ config, existingState, @@ -88,7 +79,10 @@ async function approvedArtifactPolicy(input: { return freshPlanningArtifactPolicy({ config, - existingState, + existingState: + options.artifactWorkspace && existingState + ? { ...existingState, worktreePath: undefined } + : existingState, resolvedArtifacts, allowGeneratedSpec: false, allowGeneratedPlan: false, @@ -114,6 +108,8 @@ async function assertApprovedSourcesMaterializable(input: { requirePlan: boolean; specLabel: string; planLabel: string; + artifactWorkspace?: ReadOnlyIssueWorkspace; + runner?: CommandRunner; }): Promise { const approved = [ ...(input.requireSpec && input.sources.spec @@ -140,11 +136,27 @@ async function assertApprovedSourcesMaterializable(input: { for (const entry of approved) { try { - await assertIssueArtifactSourcesMaterializable({ - repoRoot: planningArtifactRoot(input.policy, entry.artifact).repoRoot, - issueNumber: input.issue.number, - sources: { [entry.kind]: entry.source }, - }); + 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( @@ -206,6 +218,8 @@ export async function assertApprovedArtifactsResolvable( 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 3360edb..75396fb 100644 --- a/src/cli/commands/run-once/artifact-source-materialization.ts +++ b/src/cli/commands/run-once/artifact-source-materialization.ts @@ -89,6 +89,41 @@ export async function assertIssueArtifactSourcesMaterializable(input: { 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 { diff --git a/src/cli/commands/run-once/git.ts b/src/cli/commands/run-once/git.ts index 1734a16..c5f7343 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 @@ -462,6 +467,45 @@ function porcelainWorktreePaths(stdout: string): string[] { .map((line) => resolve(line.slice("worktree ".length).trim())); } +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); + if (!porcelainWorktreePaths(listed).includes(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 { try { await access(path); diff --git a/src/cli/commands/run-once/pipeline-planning.test.ts b/src/cli/commands/run-once/pipeline-planning.test.ts index a02e1e3..e974547 100644 --- a/src/cli/commands/run-once/pipeline-planning.test.ts +++ b/src/cli/commands/run-once/pipeline-planning.test.ts @@ -322,6 +322,134 @@ test("runOneIssue rejects approved published worktree conflicts before claiming" ); }); +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"; diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index fc7098d..9b2356b 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -1,4 +1,4 @@ -import { join, resolve } from "node:path"; +import { join } from "node:path"; import { localPiAgentDir } from "../init/pi-agent-settings.ts"; import { createRunOnceHostProvider } from "../../../host/factory.ts"; @@ -12,6 +12,7 @@ import { assertCleanWorktree, assertIssueBaseContainedInPrBase, ensureIssueWorktree, + inspectIssueWorkspace, type IssueWorktreeResult, } from "./git.ts"; import { @@ -316,51 +317,20 @@ export async function runOneIssue( ); } }; - const resolveArtifactWorkspace = async (): Promise< - { worktreePath: string } | undefined - > => { - assertExpectedWorkspaceIdentity(); - const result = await runner.run( - "git", - ["worktree", "list", "--porcelain"], - { cwd: config.repoRoot }, - ); - if (result.code !== 0) { - throw new AgentIssueSafetyError( - `git worktree list failed before approval preflight with exit code ${result.code}`, - ); - } - const worktreePath = - existingState?.worktreePath ?? expectedWorkspace.worktreePath; - const expectedWorktreeRoot = resolve(config.repoRoot, worktreePath); - const registered = result.stdout - .split("\n") - .find( - (line) => - line.startsWith("worktree ") && - resolve(line.slice("worktree ".length)) === expectedWorktreeRoot, - ); - if (!registered) return undefined; - - const branchResult = await runner.run( - "git", - ["-C", worktreePath, "branch", "--show-current"], - { cwd: config.repoRoot }, - ); - if (branchResult.code !== 0) { - throw new AgentIssueSafetyError( - `git branch failed for ${worktreePath} before approval preflight with exit code ${branchResult.code}`, - ); - } - const currentBranch = branchResult.stdout.trim(); - if (currentBranch !== expectedWorkspace.branch) { - throw new AgentIssueSafetyError( - `Existing worktree ${worktreePath} is on ${currentBranch}, expected ${expectedWorkspace.branch}`, - ); - } - - return { 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(); @@ -421,7 +391,8 @@ export async function runOneIssue( existingState, resolvedArtifacts, now: runOptions.now ?? new Date(), - resolveArtifactWorkspace, + artifactWorkspace, + runner, }); if (approvedArtifactPreflight) { artifactPolicy = approvedArtifactPreflight.policy; From c9b2ab8c0729c0411a72f74698075e371014f9fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 15:10:58 +0200 Subject: [PATCH 22/25] fix(run-once): preflight branch-only artifacts --- .../run-once/approval-artifact-preflight.ts | 111 +++++++++- src/cli/commands/run-once/git.ts | 41 +++- .../pipeline-workspace-scenarios.test.ts | 207 +++++++++++++++++- src/cli/commands/run-once/pipeline.ts | 16 +- 4 files changed, 363 insertions(+), 12 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 753cb02..93f200a 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -1,8 +1,12 @@ +import { join } from "node:path"; import { assertIssueArtifactSourcesMaterializable, assertIssueArtifactSourcesMaterializableInBranch, } from "./artifact-source-materialization.ts"; -import type { ResolvedIssueArtifactSources } from "./artifact-sources.ts"; +import type { + ResolvedIssueArtifactSource, + ResolvedIssueArtifactSources, +} from "./artifact-sources.ts"; import { PlanningArtifactSafetyError, planningArtifactRoot, @@ -45,9 +49,10 @@ async function approvedArtifactPolicy(input: { options: ApprovedArtifactPreflightOptions; requireSpec: boolean; requirePlan: boolean; + resolvedArtifacts: ResolvedIssueArtifactSources; }): Promise { - const { options, requireSpec, requirePlan } = input; - const { config, existingState, resolvedArtifacts } = options; + const { options, requireSpec, requirePlan, resolvedArtifacts } = input; + const { config, existingState } = options; const hasApprovedSource = (requireSpec && !!resolvedArtifacts.spec) || (requirePlan && !!resolvedArtifacts.plan); @@ -89,6 +94,87 @@ async function approvedArtifactPolicy(input: { }); } +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}^{blob}`; + const exists = await options.runner.run("git", ["cat-file", "-e", object], { + cwd: options.config.repoRoot, + }); + if (exists.code === 1 || exists.code === 128) return undefined; + if (exists.code !== 0) { + throw new PlanningArtifactSafetyError( + `git cat-file failed while resolving saved ${kind} ${path} from ${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, @@ -175,10 +261,29 @@ export async function assertApprovedArtifactsResolvable( 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 { diff --git a/src/cli/commands/run-once/git.ts b/src/cli/commands/run-once/git.ts index c5f7343..432fbde 100644 --- a/src/cli/commands/run-once/git.ts +++ b/src/cli/commands/run-once/git.ts @@ -460,11 +460,33 @@ 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( @@ -479,7 +501,16 @@ export async function inspectIssueWorkspace( "git worktree list failed", ); const expectedWorktreePath = resolve(repoRoot, expected.worktreePath); - if (!porcelainWorktreePaths(listed).includes(expectedWorktreePath)) { + const worktrees = porcelainWorktrees(listed); + const branchWorktree = worktrees.find( + (worktree) => worktree.branch === expected.branch, + ); + if (branchWorktree && branchWorktree.path !== expectedWorktreePath) { + 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" }; 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 30d7b35..4db685d 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -361,8 +361,12 @@ test("runOneIssue recreates an approved implementing worktree after preflight", const runner = { calls: baseRunner.calls, async run(...args: Parameters) { - const result = await baseRunner.run(...args); 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" && @@ -858,3 +862,204 @@ test("runOneIssue reports missing branch and worktree blocked recovery before mu ); assert.equal((await workflowPiCalls(runner.calls)).length, 0); }); + +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: "", 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 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, + ); +}); diff --git a/src/cli/commands/run-once/pipeline.ts b/src/cli/commands/run-once/pipeline.ts index 9b2356b..8ee9880 100644 --- a/src/cli/commands/run-once/pipeline.ts +++ b/src/cli/commands/run-once/pipeline.ts @@ -394,13 +394,23 @@ export async function runOneIssue( artifactWorkspace, runner, }); - if (approvedArtifactPreflight) { - artifactPolicy = approvedArtifactPreflight.policy; - } 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) From 8ec511a1b827aef6d5fee75c492e400b541fc0ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 15:27:08 +0200 Subject: [PATCH 23/25] fix(run-once): reject missing branch artifacts --- .../approval-artifact-preflight.test.ts | 51 ++ .../run-once/approval-artifact-preflight.ts | 6 +- src/cli/commands/run-once/git.ts | 6 +- ...eline-approval-workspace-preflight.test.ts | 604 ++++++++++++++++++ .../pipeline-workspace-scenarios.test.ts | 400 ------------ 5 files changed, 664 insertions(+), 403 deletions(-) create mode 100644 src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 86dba96..618d2fe 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -376,6 +376,57 @@ test("approved explicit plan must match a saved implementation resume plan", asy ); }); +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", + "-e", + "agent/issue-140-keep-approved-artifacts:docs/plans/saved-plan.md^{blob}", + ], + ]); +}); + 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]; diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index 93f200a..e173934 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -152,7 +152,11 @@ async function branchSavedArtifact(input: { const exists = await options.runner.run("git", ["cat-file", "-e", object], { cwd: options.config.repoRoot, }); - if (exists.code === 1 || exists.code === 128) return undefined; + if (exists.code === 1 || exists.code === 128) { + throw new PlanningArtifactSafetyError( + `Saved ${kind} ${path} does not exist on issue branch ${options.artifactWorkspace.branch}`, + ); + } if (exists.code !== 0) { throw new PlanningArtifactSafetyError( `git cat-file failed while resolving saved ${kind} ${path} from ${options.artifactWorkspace.branch}`, diff --git a/src/cli/commands/run-once/git.ts b/src/cli/commands/run-once/git.ts index 432fbde..15227e0 100644 --- a/src/cli/commands/run-once/git.ts +++ b/src/cli/commands/run-once/git.ts @@ -503,9 +503,11 @@ export async function inspectIssueWorkspace( const expectedWorktreePath = resolve(repoRoot, expected.worktreePath); const worktrees = porcelainWorktrees(listed); const branchWorktree = worktrees.find( - (worktree) => worktree.branch === expected.branch, + (worktree) => + worktree.branch === expected.branch && + worktree.path !== expectedWorktreePath, ); - if (branchWorktree && branchWorktree.path !== expectedWorktreePath) { + if (branchWorktree) { throw new Error( `Issue branch ${expected.branch} is already checked out at ${branchWorktree.path}; expected ${expected.worktreePath}`, ); 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 0000000..ea0e611 --- /dev/null +++ b/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts @@ -0,0 +1,604 @@ +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: "", 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-workspace-scenarios.test.ts b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts index 4db685d..a7b119c 100644 --- a/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts +++ b/src/cli/commands/run-once/pipeline-workspace-scenarios.test.ts @@ -136,205 +136,6 @@ test("runOneIssue reuses existing implementation worktree on resume", async () = assert.ok(runner.calls.find((call) => call.command === "pi")); }); -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 recreates an approved implementing worktree after preflight", async () => { const config = await makeConfig({ dryRun: false, @@ -862,204 +663,3 @@ test("runOneIssue reports missing branch and worktree blocked recovery before mu ); assert.equal((await workflowPiCalls(runner.calls)).length, 0); }); - -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: "", 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 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, - ); -}); From 2fe04112980aa475b5f526ce7ace86295454a383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 15:43:26 +0200 Subject: [PATCH 24/25] fix(run-once): resolve branch artifact blobs --- .../approval-artifact-preflight.test.ts | 94 ++++++++++++++++++- .../run-once/approval-artifact-preflight.ts | 13 ++- ...eline-approval-workspace-preflight.test.ts | 6 +- test-support/run-once/mock-runner.ts | 6 +- 4 files changed, 111 insertions(+), 8 deletions(-) diff --git a/src/cli/commands/run-once/approval-artifact-preflight.test.ts b/src/cli/commands/run-once/approval-artifact-preflight.test.ts index 618d2fe..04d1588 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.test.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.test.ts @@ -4,6 +4,7 @@ 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, @@ -421,12 +422,101 @@ test("approved branch-only resume rejects a missing saved plan even with a match assert.deepEqual(calls, [ [ "cat-file", - "-e", - "agent/issue-140-keep-approved-artifacts:docs/plans/saved-plan.md^{blob}", + "-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]; diff --git a/src/cli/commands/run-once/approval-artifact-preflight.ts b/src/cli/commands/run-once/approval-artifact-preflight.ts index e173934..5deab22 100644 --- a/src/cli/commands/run-once/approval-artifact-preflight.ts +++ b/src/cli/commands/run-once/approval-artifact-preflight.ts @@ -148,20 +148,25 @@ async function branchSavedArtifact(input: { savedCommit: commit, }); - const object = `${options.artifactWorkspace.branch}:${path}^{blob}`; - const exists = await options.runner.run("git", ["cat-file", "-e", object], { + const object = `${options.artifactWorkspace.branch}:${path}`; + const type = await options.runner.run("git", ["cat-file", "-t", object], { cwd: options.config.repoRoot, }); - if (exists.code === 1 || exists.code === 128) { + if (type.code === 1 || type.code === 128) { throw new PlanningArtifactSafetyError( `Saved ${kind} ${path} does not exist on issue branch ${options.artifactWorkspace.branch}`, ); } - if (exists.code !== 0) { + 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, }); 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 index ea0e611..31f9665 100644 --- a/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts +++ b/src/cli/commands/run-once/pipeline-approval-workspace-preflight.test.ts @@ -275,7 +275,11 @@ test("runOneIssue resumes an approved branch-only saved plan", async () => { return { code: 0, stdout: "", stderr: "" }; } 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] === "show") { return { code: 0, stdout: "# plan\n", stderr: "" }; diff --git a/test-support/run-once/mock-runner.ts b/test-support/run-once/mock-runner.ts index 79c1ce9..1b198c2 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: "" }; From f12589ccd1c709fc06d640869ff1ffc7f1ca6658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roch=C3=A9=20Compaan?= Date: Wed, 5 Aug 2026 15:57:44 +0200 Subject: [PATCH 25/25] fix(nix): include git for package tests --- nix/package.nix | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/nix/package.nix b/nix/package.nix index 9f00404..c9230b8 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";