From d4d467d9260d16f2458ac574c688fb591f96b614 Mon Sep 17 00:00:00 2001 From: Matthew Smith Date: Tue, 27 Oct 2026 10:00:14 -0700 Subject: [PATCH] fix(plan): fill task issue numbers at apply time instead of predicting them The planner had to guess the issue numbers its new tasks would get and wrote those guesses into every `Depends on` line and e2e file name. Any issue or PR opened between drafting and approval shifted the real numbers, so tasks pointed at the wrong, already-closed issues and were dispatched before their dependencies existed (litart phase 9: off by two after two builder PRs). Plan task bodies now write `{{task:K}}` for the plan's K-th task and `{{issue}}` for their own number. apply_plan creates or reuses every issue first, then fills both with the real numbers before arming a task. A `{{task:K}}` outside the plan refuses the whole plan before any write. Plans without placeholders apply exactly as before. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01JrFSqr3eJsNihHcgYph1hB --- README.md | 10 ++++++- roles/planner.md | 4 +-- src/loop.test.ts | 67 +++++++++++++++++++++++++++++++++++++++++++++++ src/loop.ts | 23 +++++++++++++++- src/phase.test.ts | 31 ++++++++++++++++++++++ src/phase.ts | 36 +++++++++++++++++++++++++ src/roles.test.ts | 6 +++-- 7 files changed, 171 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index c3fd6d8..e24ecdd 100644 --- a/README.md +++ b/README.md @@ -238,7 +238,11 @@ Every `pollSeconds` (default 300) the daemon wakes and runs one iteration: clears `needs-owner` and puts the epic back In Progress. Re-adding `agent-ready` asks for a re-plan. Applying a plan reads `/-phase-NN-plan.issues.json` from the default branch on `origin`, so the clone never has to be pulled by hand. If the file is not there yet, - the foreman logs and retries next tick rather than recording the plan as applied. + the foreman logs and retries next tick rather than recording the plan as applied. A task body + names another task in the plan as `{{task:K}}` (its 1-based position) and its own number as + `{{issue}}`; the foreman creates every issue first, then fills both with the real numbers, so + issues or PRs opened while the plan waited for approval cannot shift a `Depends on` line. A + `{{task:K}}` past the end of the plan refuses the whole plan before anything is written. These run in order and stop after the first action that would start a `claude -p` session, so at most one session runs per iteration per Mac. @@ -921,6 +925,10 @@ branch. find it to merge — or the plan file's `NN` does not match the epic's `phase:N` label. The foreman looks for `/-phase-NN-plan.issues.json` where `NN` is that label, zero-padded; the planner is told the same rule, so a mismatch means the plan was hand-edited. +- **`loop iteration failed` with `…plan.issues.json: task K ("…") names {{task:J}}; the plan has + N tasks`.** The approved plan names a task position it does not have, so the foreman refuses + the whole plan before writing anything and every tick fails the same way. Fix the placeholder in + the `.issues.json` on the default branch; the next tick applies the plan. - **`~/.foreman//artifacts//` is empty after a validation run.** The foreman only copies what the validator left in `/.validation-artifacts//`, and only on a `passed`/`failed` outcome — a session that died without an outcome archives nothing. Look for the diff --git a/roles/planner.md b/roles/planner.md index 1a6524d..b1073b5 100644 --- a/roles/planner.md +++ b/roles/planner.md @@ -9,10 +9,10 @@ The prompt's "task issue" is the **next phase's epic**, which has no approved pl ```json { "epic": , "tasks": [ { "number": , "title": "(): …", - "body": "## Goal\n…\n## Acceptance criteria\n- [ ] …\n## Touches\n…\n## Depends on\n#…, #… or None\n## Spec\n §
\n\nParent epic: #", + "body": "## Goal\n…\n## Acceptance criteria\n- [ ] … (`e2e/{{issue}}-.spec.ts`)\n## Touches\n…\n## Depends on\n#{{task:1}}, #{{task:2}} or None\n## Spec\n §
\n\nParent epic: #", "labels": ["phase:N", "area:…", "size:S|M|L"] } ] } ``` - One entry per plan task, in dependency order. Every body ends with the `Parent epic: #` line; the foreman ignores a task without it. Existing sub-issues (`gh issue list --search "Parent epic: # in:body" --state all --limit 100 --json number,title,state`) are updated by number, not duplicated. Do **not** include `agent-ready` in labels; the foreman adds it after approval. + One entry per plan task, in dependency order. A task that is not created yet has no issue number, so never predict one: in a body, write `{{task:K}}` for the plan's K-th task (1-based, so a dependency reads `#{{task:3}}`) and `{{issue}}` for the task's own number (for example `e2e/{{issue}}-.spec.ts`). The foreman fills both with the real numbers when it creates the issues. Name an issue that already exists by its real number. In the plan's Markdown, call tasks "Task K", not by an issue number. Every body ends with the `Parent epic: #` line; the foreman ignores a task without it. Existing sub-issues (`gh issue list --search "Parent epic: # in:body" --state all --limit 100 --json number,title,state`) are updated by number, not duplicated. Do **not** include `agent-ready` in labels; the foreman adds it after approval. 5. Commit on the given branch, push, open a PR titled `docs(plan): phase NN plan (#)` whose body starts with `Refs #` (not `Closes`) and summarises the assumptions. Comment on the epic: `Plan drafted: . Label me plan-approved after review.` 6. Return `plan_drafted` with the PR number. The foreman notifies the owner. diff --git a/src/loop.test.ts b/src/loop.test.ts index 734c4ac..54599d0 100644 --- a/src/loop.test.ts +++ b/src/loop.test.ts @@ -751,6 +751,73 @@ describe("execute", () => { expect(calls).toContain("removeLabels issue 200 needs-owner"); expect(calls).toContain("setStatus PVTI_200 In Progress"); }); + it("apply_plan fills each task's placeholders with the numbers its issues got", async () => { + // Two PRs opened between the draft and the apply: the tasks land on #282 and #283, not on + // whatever the planner would have predicted. + const { gh: base, calls } = fakeGh([ + issue({ + number: 200, + labels: ["epic", "phase:2", "plan-approved"], + body: "## Spec\ndocs/superpowers/specs/2026-09-03-phase-02-x-design.md", + }), + issue({ number: 201, labels: ["phase:2"], status: "Backlog" }), + ]); + let next = 282; + const gh: GitHubApi = { ...base, createIssue: async () => next++ }; + const file = JSON.stringify({ + epic: 200, + tasks: [ + { title: "t1", body: "## Depends on\nNone", labels: ["phase:2"] }, + { title: "t2", body: "e2e/{{issue}}-a.spec.ts\n## Depends on\n#{{task:1}}", labels: [] }, + { number: 201, title: "t3", body: "## Depends on\n#{{task:2}}, #{{task:1}}", labels: [] }, + ], + }); + await execute( + { type: "apply_plan", epic: 200 }, + ctx({ + gh, + planFiles: async () => ["docs/superpowers/plans/2026-09-10-phase-02-plan.issues.json"], + readPlanFile: async () => file, + }), + ); + expect(calls).toContain("editBody 283 e2e/283-a.spec.ts\n## Depends on\n#282"); + expect(calls).toContain("editBody 201 ## Depends on\n#283, #282"); + // A created task with nothing to fill is not edited a second time. + expect(calls.filter((c) => c.startsWith("editBody 282"))).toEqual([]); + // A task is armed only once its own body is filled. + for (const n of [283, 201]) { + const edit = calls.findIndex((c) => c.startsWith(`editBody ${n} `)); + const arm = calls.findIndex((c) => c.startsWith(`addLabels issue ${n} `)); + expect(edit).toBeGreaterThanOrEqual(0); + expect(edit).toBeLessThan(arm); + } + }); + it("apply_plan refuses a plan whose {{task:K}} names no task, before writing anything", async () => { + const { gh, calls } = fakeGh([ + issue({ + number: 200, + labels: ["epic", "phase:2", "plan-approved"], + body: "## Spec\ndocs/superpowers/specs/2026-09-03-phase-02-x-design.md", + }), + ]); + const file = JSON.stringify({ + epic: 200, + tasks: [{ title: "t1", body: "## Depends on\n#{{task:2}}", labels: ["phase:2"] }], + }); + await expect( + execute( + { type: "apply_plan", epic: 200 }, + ctx({ + gh, + planFiles: async () => ["docs/superpowers/plans/2026-09-10-phase-02-plan.issues.json"], + readPlanFile: async () => file, + }), + ), + ).rejects.toThrow( + 'docs/superpowers/plans/2026-09-10-phase-02-plan.issues.json: task 1 ("t1") names {{task:2}}; the plan has 1 tasks', + ); + expect(calls).toEqual([]); + }); it("apply_plan reads the board once, not once per task", async () => { const { gh } = fakeGh([ issue({ diff --git a/src/loop.ts b/src/loop.ts index 92a6f5d..859a828 100644 --- a/src/loop.ts +++ b/src/loop.ts @@ -28,10 +28,13 @@ import { fmt, openClaim, parseClaim } from "./ledger.ts"; import { log } from "./log.ts"; import { createNotifier, type NotifyPort, noopNotify } from "./notify.ts"; import { + fillPlanBody, + hasPlanPlaceholder, PlanIssuesSchema, parseDirectionCritical, parseSpecPath, planPhaseNumber, + planPlaceholderErrors, } from "./phase.ts"; import { matchedSkipLabels } from "./pick.ts"; import { fetchOrigin, listPlanFilesOnMain, readPlanFileOnMain } from "./plans.ts"; @@ -737,6 +740,10 @@ async function applyPlan(ctx: Ctx, epicNumber: number, snapshot?: Snapshot): Pro return false; } const planFile = PlanIssuesSchema.parse(JSON.parse(text)); + // Refused before the first write, like a body that fails the schema: half-applied, the plan's + // created issues would point their dependencies at whatever the bad placeholder became. + const placeholderErrors = planPlaceholderErrors(planFile); + if (placeholderErrors.length > 0) throw new Error(`${file}: ${placeholderErrors.join("; ")}`); await ensurePlanLabels( gh, planFile.tasks.flatMap((t) => t.labels), @@ -758,6 +765,10 @@ async function applyPlan(ctx: Ctx, epicNumber: number, snapshot?: Snapshot): Pro ); return cache; }; + // Every task's number first, so a body's `{{task:K}}` can name a task created after it. + const numbers: number[] = []; + const created = new Set(); + const reusedTasks = new Set(); for (const t of planFile.tasks) { // A task without a number is created — unless an earlier attempt already did: an apply that // failed later in the tick and was retried once created an exact duplicate. The title plus the "Parent epic" line the planner puts in every body is the key. @@ -771,10 +782,10 @@ async function applyPlan(ctx: Ctx, epicNumber: number, snapshot?: Snapshot): Pro )?.number; let number = t.number ?? reused; if (number) { - await gh.editBody(number, t.body); // Only for a reused issue: on the t.number path the planner is naming an issue it already // linked, and addSubIssue on an existing link is a 422 that would fail the whole tick. if (reused) { + reusedTasks.add(reused); const linked = await linkedTasks(); if (!linked.has(reused)) { await gh.addSubIssue(epicNumber, reused); @@ -783,9 +794,19 @@ async function applyPlan(ctx: Ctx, epicNumber: number, snapshot?: Snapshot): Pro } } } else { + // Created with its placeholders still in; nothing picks it up before the edit below, + // because it has no `agent-ready` until then, and a retry reuses it by title. number = await gh.createIssue({ title: t.title, body: t.body, labels: t.labels }); + created.add(number); await gh.addSubIssue(epicNumber, number); } + numbers.push(number); + } + for (const [i, t] of planFile.tasks.entries()) { + const number = numbers[i] as number; + if (!created.has(number) || hasPlanPlaceholder(t.body)) + await gh.editBody(number, fillPlanBody(t.body, number, numbers)); + const reused = reusedTasks.has(number) ? number : undefined; // A reused task the board has already moved on (In Progress / In Review / Done) must not be // re-armed: setStatus("Ready") makes mergeDecision reject its approved PR forever, and // buildCandidates would dispatch a second builder round on finished work. The retry that diff --git a/src/phase.test.ts b/src/phase.test.ts index fb160c0..5cca6f4 100644 --- a/src/phase.test.ts +++ b/src/phase.test.ts @@ -3,6 +3,8 @@ import { comment, epic, hoursAgo, issue, pr, snapshot } from "../test/helpers.ts import { fmt } from "./ledger.ts"; import { applyPlanActions, + fillPlanBody, + hasPlanPlaceholder, nextUnplannedEpic, PlanIssuesSchema, parseDirectionCritical, @@ -11,6 +13,7 @@ import { phaseComplete, planIssuesPath, planPhaseNumber, + planPlaceholderErrors, } from "./phase.ts"; describe("parsers", () => { @@ -45,6 +48,34 @@ describe("parsers", () => { ); expect(planPhaseNumber(["epic"], "docs/sandbox.md")).toBeNull(); }); + it("fillPlanBody fills the task's own number and other tasks' numbers", () => { + const body = + "## Acceptance criteria\n- [ ] `e2e/{{issue}}-tab.spec.ts`\n## Depends on\n#{{task:1}}, #{{task:3}}, #50"; + expect(fillPlanBody(body, 288, [286, 287, 288])).toBe( + "## Acceptance criteria\n- [ ] `e2e/288-tab.spec.ts`\n## Depends on\n#286, #288, #50", + ); + expect(fillPlanBody("no placeholders {{other}}", 1, [1])).toBe("no placeholders {{other}}"); + }); + it("hasPlanPlaceholder", () => { + expect(hasPlanPlaceholder("#{{task:2}}")).toBe(true); + expect(hasPlanPlaceholder("x {{issue}}")).toBe(true); + expect(hasPlanPlaceholder("#12 {{other}}")).toBe(false); + }); + it("planPlaceholderErrors names a {{task:K}} outside the plan", () => { + const plan = { + epic: 1, + tasks: [ + { title: "a", body: "#{{task:2}} {{issue}}", labels: [] }, + { title: "b", body: "#{{task:3}} #{{task:0}}", labels: [] }, + ], + }; + expect(planPlaceholderErrors(plan)).toEqual([ + 'task 2 ("b") names {{task:3}}; the plan has 2 tasks', + 'task 2 ("b") names {{task:0}}; the plan has 2 tasks', + ]); + const fine = { title: "c", body: "#{{task:1}} #{{task:2}} {{issue}}", labels: [] }; + expect(planPlaceholderErrors({ epic: 1, tasks: [fine, fine] })).toEqual([]); + }); it("PlanIssuesSchema", () => { expect( PlanIssuesSchema.parse({ epic: 150, tasks: [{ title: "t", body: "b", labels: ["phase:1"] }] }) diff --git a/src/phase.ts b/src/phase.ts index c971587..649aae3 100644 --- a/src/phase.ts +++ b/src/phase.ts @@ -15,6 +15,42 @@ export const PlanIssuesSchema = z.object({ }); export type PlanIssues = z.infer; +/** + * What a plan task's body writes in place of an issue number that does not exist until the plan + * is applied: `{{issue}}` is the task's own number, `{{task:K}}` the number of the plan's K-th + * task (1-based). Both fill in as the bare number, so a dependency reads `#{{task:3}}`. The + * planner used to predict these numbers instead, and every issue or PR opened between the draft + * and the apply shifted them onto the wrong issues. + */ +const PLAN_PLACEHOLDER = /\{\{(?:issue|task:(\d+))\}\}/g; + +/** Every `{{task:K}}` in the plan that names no task in it; empty when every body can be filled. */ +export function planPlaceholderErrors(plan: PlanIssues): string[] { + const errors: string[] = []; + plan.tasks.forEach((t, i) => { + for (const m of t.body.matchAll(PLAN_PLACEHOLDER)) { + const k = m[1] === undefined ? null : Number(m[1]); + if (k !== null && (k < 1 || k > plan.tasks.length)) + errors.push( + `task ${i + 1} ("${t.title}") names ${m[0]}; the plan has ${plan.tasks.length} tasks`, + ); + } + }); + return errors; +} + +/** True when the body has a placeholder `fillPlanBody` would replace. */ +export function hasPlanPlaceholder(body: string): boolean { + return new RegExp(PLAN_PLACEHOLDER.source).test(body); +} + +/** The body with its placeholders filled; `numbers[K - 1]` is the K-th task's issue number. */ +export function fillPlanBody(body: string, own: number, numbers: number[]): string { + return body.replace(PLAN_PLACEHOLDER, (_all, k: string | undefined) => + String(k === undefined ? own : numbers[Number(k) - 1]), + ); +} + export function parseDirectionCritical(specText: string): boolean { return /^\*\*Direction-critical:\*\*\s*yes\b/im.test(specText); } diff --git a/src/roles.test.ts b/src/roles.test.ts index 1e7ff24..cfbd6b7 100644 --- a/src/roles.test.ts +++ b/src/roles.test.ts @@ -64,8 +64,10 @@ describe("the base prompts say nothing about one particular repository", () => { for (const f of files) { const text = readFileSync(join(dir, `${f}.md`), "utf8"); const found = [...text.matchAll(/\{\{(\w+)\}\}/g)].map((m) => m[1]); - for (const p of found) - expect(["checks", "plansDir", "specGlob"], `${f}.md uses {{${p}}}`).toContain(p); + // `{{issue}}` reaches the planner as written: it goes into the plan file, and apply_plan + // fills it, not the composer. + const filled = ["checks", "plansDir", "specGlob", ...(f === "planner" ? ["issue"] : [])]; + for (const p of found) expect(filled, `${f}.md uses {{${p}}}`).toContain(p); } }); });