From 089474b14747545c29a87dec1ebe3f5ec4c2dc87 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Thu, 30 Jul 2026 04:24:12 -0700 Subject: [PATCH 1/6] chore(porch): 1273 verify phase-transition --- .../1273-builder-context-reset-should-b/status.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/codev/projects/1273-builder-context-reset-should-b/status.yaml b/codev/projects/1273-builder-context-reset-should-b/status.yaml index d4470840d..8be464ffb 100644 --- a/codev/projects/1273-builder-context-reset-should-b/status.yaml +++ b/codev/projects/1273-builder-context-reset-should-b/status.yaml @@ -1,7 +1,7 @@ id: '1273' title: builder-context-reset-should-b protocol: aspir -phase: review +phase: verify plan_phases: - id: phase_1 title: afx interrupt + ESC delivery path @@ -33,7 +33,7 @@ gates: verify-approval: status: pending iteration: 1 -build_complete: true +build_complete: false history: - iteration: 1 plan_phase: phase_2 @@ -276,5 +276,5 @@ history: file: >- /Users/mwk/Development/cluesmith/codev/.builders/aspir-1273/codev/projects/1273-builder-context-reset-should-b/1273-phase_6-iter6-claude.txt started_at: '2026-07-28T00:57:25.240Z' -updated_at: '2026-07-30T11:20:42.698Z' +updated_at: '2026-07-30T11:24:12.936Z' pr_ready_for_human: false From 9faa6d79e47da187538acd13be22d78975d4e1f5 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 14:50:14 -0700 Subject: [PATCH 2/6] [Spec 1273][Verify] fix: resolve the builder's OWN porch project, and support the task lane MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-phase e2e found afx reset non-functional in production on every lane. Three fixes, all confirmed against live builders with --dry-run. F1 (BLOCKER, all lanes) — wrong-winner project selection. readPorchContext returned the FIRST directory under codev/projects with a parsable status.yaml. Correct for a repo with one project dir; wrong here, because porch history is committed to main and every worktree inherits every project ever run — 203 of them. The alphabetically-first is 0087-porch-timeout-termination-retries, protocol `spider`, so EVERY builder resolved protocol spider and died on `Protocol "spider" has no builder-prompt.md`. The comment I wrote at context.ts:163 — "when a worktree somehow holds more than one project directory" — named the assumption that broke. It was not "somehow"; it is the normal state of every worktree in this repo. This is the #1235 wrong-winner family: pick-the-first over a set assumed to be a singleton. Now selects by identity, never by position. readPorchContext takes a PorchProjectIdentity and matches status.yaml's `id` (or the directory prefix) against the issue number, the builder id, or the builder id minus its protocol prefix. Comparison ignores case and leading zeros — the registry lowercases ids while directories preserve case (builder-task-re_v vs .builders/task-RE_V, which macOS hides and Linux would not), and dirs use 0087 where ids use 87. No match returns null. That is the honest answer for a genuinely non-porch builder, and it is also the safe one for a porch builder: a missing porch block is visible in the re-orientation, whereas another project's protocol is a confident lie. porch is now resolved ONCE and reused for both the protocol chain and the result, so two scans cannot disagree. F1b — the bare --task lane has no protocol template. With F1 fixed, the probe resolved protocol `task` from its builder id and died on `Protocol "task" has no builder-prompt.md`. `afx spawn --task` without --protocol takes spawn.ts's else-branch (:551) and builds the prompt straight from the task text; there is no protocols/task/builder-prompt.md to render. buildLongForm now reproduces that shape for the bare-task lane, selected on a POSITIVE fact (taskText present and no porch) rather than by catching the error. The --task --protocol variant does get a porch project (:548), so it is unaffected and still renders its real template. Header, state pointer, addendum and porch re-entry moved into a shared assembleLongFormDocument so the framing cannot drift between lanes. F2 — the task lane could never auto-detect its mode. Mode detection requires a `## Mode:` line, which task spawns never render, so afx reset hard-errored without --mode. A no-porch builder now defaults to soft with modeSource 'task-default'. Not a guess dressed as a fact: "strict" means porch orchestrates, and a builder with no porch project cannot be strict. The source is recorded so the report says where it came from. Scoped to the no-porch case — a porch builder missing its Mode line is a genuine ambiguity and still aborts, now naming the project that proves it is not the task lane. F3 — reported, not a defect in reset. The path printed (.builders/task-RE_V) is CORRECT and comes verbatim from the registry, which stores id `builder-task-re_v` alongside worktree `.builders/task-RE_V`. The id/directory case divergence originates at spawn, not here. Reset was already using registry paths verbatim; the new id matching is case-insensitive so a case-sensitive filesystem behaves like macOS. Verified live with --dry-run (zero writes): task-re_v -> TASK/soft with spawn's task prompt reproduced verbatim; 1286 -> ASPIR/strict on project 1286-consult-configurable-per-lane-; 1273 -> ASPIR. No spider anywhere. Tests: multi-project-dir fixture reproducing the 203-dir shape (own project wins over three foreign spider projects; null when the builder owns none; issue-number match; leading-zero and case tolerance), task-lane mode default plus the porch-builder abort it must not swallow, and two reorient tests pinning that the template port is never called for a bare task and still is for --task --protocol. Suite 4016 -> 4027, build clean. --- .../__tests__/spec-1273-reset-context.test.ts | 164 +++++++++++++++++- .../spec-1273-reset-reorient.test.ts | 52 ++++++ .../src/agent-farm/commands/reset/context.ts | 134 ++++++++++++-- .../src/agent-farm/commands/reset/reorient.ts | 40 ++++- 4 files changed, 372 insertions(+), 18 deletions(-) diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts index d71250651..e965caa49 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts @@ -129,7 +129,7 @@ describe('protocol resolution (Spec 1273)', () => { }); it('protocolFromStatus returns null when there are no project dirs', () => { - expect(protocolFromStatus(makeFs({}), WORKTREE)).toBeNull(); + expect(protocolFromStatus(makeFs({}), WORKTREE, { builderId: BUILDER_ID })).toBeNull(); }); }); @@ -139,7 +139,7 @@ describe('protocol resolution (Spec 1273)', () => { describe('porch context (Spec 1273)', () => { it('reads phase and current plan phase', () => { - const porch = readPorchContext(fullWorktree(), WORKTREE); + const porch = readPorchContext(fullWorktree(), WORKTREE, { builderId: BUILDER_ID }); expect(porch?.phase).toBe('implement'); expect(porch?.currentPlanPhase).toBe('phase_4'); expect(porch?.projectId).toBe('1273'); @@ -150,7 +150,7 @@ describe('porch context (Spec 1273)', () => { [join(WORKTREE, 'codev', 'projects', '1273-builder-context-reset-should-b', 'status.yaml')]: STATUS_YAML.replace('current_plan_phase: phase_4', 'current_plan_phase: null'), }); - expect(readPorchContext(fs, WORKTREE)?.currentPlanPhase).toBeNull(); + expect(readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })?.currentPlanPhase).toBeNull(); }); it('returns null for a non-porch lane instead of throwing', () => { @@ -160,7 +160,7 @@ describe('porch context (Spec 1273)', () => { [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, }); - expect(readPorchContext(fs, WORKTREE)).toBeNull(); + expect(readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })).toBeNull(); const ctx = resolveBuilderContext({ fs, ...BASE }); expect(ctx.porch).toBeNull(); @@ -448,3 +448,159 @@ describe('harness detection matches command position only (Spec 1273)', () => { expect(calls.every(c => /^(exists|read|listDirs):/.test(c))).toBe(true); }); }); + +// ============================================================================ +// F1 — the wrong-winner blocker found by the live e2e (2026-07-31) +// ============================================================================ + +describe('porch project selection is by identity, never by position (Spec 1273 F1)', () => { + /** + * A worktree shaped like this repo's real ones. + * + * Porch history is committed to `main`, so every worktree inherits every + * project ever run — 203 of them when this bug was found. The + * alphabetically-first is a `spider`-era project, and the original + * implementation returned the first parsable `status.yaml`, so EVERY builder + * resolved protocol `spider` and died on + * `Protocol "spider" has no builder-prompt.md`. + * + * The fixture reproduces the essential shape: historical dirs sorting before + * the builder's own, each with a valid but foreign status.yaml. + */ + function inheritedHistoryWorktree(ownDir = '1273-builder-context-reset-should-b') { + const historical = [ + '0087-porch-timeout-termination-retries', + '0088-porch-version-constant', + '0092-terminal-file-links', + ]; + const files: Record = { + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + [join(WORKTREE, 'codev', 'projects', ownDir, 'status.yaml')]: STATUS_YAML, + }; + for (const [i, dir] of historical.entries()) { + files[join(WORKTREE, 'codev', 'projects', dir, 'status.yaml')] = + `id: '00${87 + i}'\ntitle: ${dir}\nprotocol: spider\nphase: implement\n`; + } + return makeFs(files, { + [join(WORKTREE, 'codev', 'projects')]: [...historical, ownDir], + }); + } + + it('picks the builder\'s own project, not the alphabetically-first', () => { + const porch = readPorchContext(inheritedHistoryWorktree(), WORKTREE, { + builderId: BUILDER_ID, + }); + + expect(porch?.projectId).toBe('1273'); + expect(porch?.protocol).toBe('aspir'); + // The exact regression: `spider` must never be the answer. + expect(porch?.protocol).not.toBe('spider'); + }); + + it('resolves the whole context correctly despite 3 foreign projects', () => { + const ctx = resolveBuilderContext({ fs: inheritedHistoryWorktree(), ...BASE }); + expect(ctx.protocol).toBe('aspir'); + expect(ctx.protocolSource).toBe('status.yaml'); + expect(ctx.porch?.projectName).toBe('1273-builder-context-reset-should-b'); + }); + + it('returns null — not a foreign project — when this builder owns none', () => { + // The live probe `task-re_v` had no porch project of its own but inherited + // 203 others. Returning any of them is a confident lie; null is the truth, + // and phase 5 renders a frame without a porch block for it. + const porch = readPorchContext(inheritedHistoryWorktree(), WORKTREE, { + builderId: 'builder-task-re_v', + }); + + expect(porch).toBeNull(); + }); + + it('matches on the issue number for an issue-driven lane', () => { + const porch = readPorchContext(inheritedHistoryWorktree('1279-fix-something'), WORKTREE, { + builderId: 'builder-bugfix-1279', + issueNumber: '1279', + }); + expect(porch?.projectName).toBe('1279-fix-something'); + }); + + it('tolerates leading zeros between the id and the directory name', () => { + // Project dirs use `0087-…` while porch ids and CLI arguments use `87` + // (the same split as `afx cleanup -p 466` vs `0466`). + const porch = readPorchContext(inheritedHistoryWorktree(), WORKTREE, { + builderId: 'builder-bugfix-87', + }); + expect(porch?.projectName).toBe('0087-porch-timeout-termination-retries'); + }); + + it('matches case-insensitively, so a case-sensitive filesystem behaves like macOS', () => { + // The registry lowercases builder ids while directories preserve case + // (`builder-task-re_v` vs `.builders/task-RE_V`). macOS hides the mismatch. + const porch = readPorchContext(inheritedHistoryWorktree('task-RE_V-probe'), WORKTREE, { + builderId: 'builder-task-re_v', + }); + expect(porch?.projectName).toBe('task-RE_V-probe'); + }); +}); + +// ============================================================================ +// F2 — the task lane could never auto-detect its mode +// ============================================================================ + +describe('mode resolution for the ad-hoc task lane (Spec 1273 F2)', () => { + /** A `--task` spawn: bare prompt, no `## Mode:` heading, no porch project. */ + function taskWorktree() { + return makeFs( + { + [join(WORKTREE, '.builder-prompt.txt')]: + 'You are a Builder. Read codev/roles/builder.md for your full role definition.\n\n# Task\n\nBe a probe.', + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: [] }, + ); + } + + it('defaults a no-porch builder to soft instead of hard-erroring', () => { + // Before this, `afx reset ` could not run at all without an + // explicit --mode: task spawns never render the `## Mode:` line. + const ctx = resolveBuilderContext({ + fs: taskWorktree(), + builderId: 'builder-task-re_v', + worktree: WORKTREE, + branch: 'builder/task-RE_V', + }); + + expect(ctx.mode).toBe('soft'); + // The source is recorded, so the report says where it came from rather than + // implying the worktree stated it. + expect(ctx.modeSource).toBe('task-default'); + }); + + it('still aborts when a PORCH builder is missing its Mode line', () => { + // The default is scoped to the no-porch lane. A porch-driven builder with no + // `## Mode:` line is a genuine ambiguity — strict vs soft changes whether + // porch orchestrates — and must not be guessed. + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '1273-x', 'status.yaml')]: STATUS_YAML, + [join(WORKTREE, '.builder-prompt.txt')]: 'No mode heading here.', + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['1273-x'] }, + ); + + expect(() => resolveBuilderContext({ fs, ...BASE })).toThrow(/Cannot determine the mode/); + }); + + it('lets an explicit --mode win over the task default', () => { + const ctx = resolveBuilderContext({ + fs: taskWorktree(), + builderId: 'builder-task-re_v', + worktree: WORKTREE, + branch: 'builder/task-RE_V', + modeOverride: 'strict', + }); + expect(ctx.mode).toBe('strict'); + expect(ctx.modeSource).toBe('flag'); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-reorient.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-reorient.test.ts index 3c133e4d6..bf7e572c5 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-reorient.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-reorient.test.ts @@ -443,6 +443,58 @@ describe('long-form re-orientation (Spec 1273)', () => { expect((port as any).mock.calls[0][1].task_text).toBeUndefined(); }); + // ========================================================================== + // The bare --task lane has no protocol template (Spec 1273 F1, live e2e) + // ========================================================================== + + it('builds the long form from task text, without asking for a protocol template', () => { + // `afx spawn --task` without `--protocol` takes spawn.ts's else-branch and + // builds the prompt straight from the task text — there is no + // `protocols/task/builder-prompt.md`. Calling the template port for it is a + // category error, and the live e2e died on + // `Protocol "task" has no builder-prompt.md`. + const port = vi.fn(() => { + throw new Error('Protocol "task" has no builder-prompt.md'); + }) as unknown as SpawnPromptPort; + + const { longForm } = assembleReorientation({ + context: makeContext({ + protocol: 'task', + porch: null, + specName: null, + specPath: null, + planPath: null, + issueNumber: undefined, + taskText: 'You are a reset-test probe.', + }), + statePath: STATE_PATH, + buildSpawnPrompt: port, + }); + + // The template port is never called — selected on a positive fact, not by + // catching the error it would have thrown. + expect(port).not.toHaveBeenCalled(); + // And the body matches spawn's bare-task shape (spawn.ts:551). + expect(longForm).toContain('# Task'); + expect(longForm).toContain('You are a reset-test probe.'); + }); + + it('still renders the real template for a --task --protocol lane', () => { + // That variant DOES get a porch project, so it must not be swallowed by the + // bare-task branch. + const port = vi.fn(() => SPAWN_PROMPT) as unknown as SpawnPromptPort; + + const { longForm } = assembleReorientation({ + context: makeContext({ taskText: 'ad-hoc work' }), + statePath: STATE_PATH, + buildSpawnPrompt: port, + buildResumeNotice: resumeNoticePort, + }); + + expect(port).toHaveBeenCalled(); + expect(longForm).toContain(SPAWN_PROMPT); + }); + it('omits the issue from the prompt context when none was supplied', () => { const port = vi.fn(() => SPAWN_PROMPT) as unknown as SpawnPromptPort; assembleReorientation({ diff --git a/packages/codev/src/agent-farm/commands/reset/context.ts b/packages/codev/src/agent-farm/commands/reset/context.ts index eeee23b47..850159a8c 100644 --- a/packages/codev/src/agent-farm/commands/reset/context.ts +++ b/packages/codev/src/agent-farm/commands/reset/context.ts @@ -60,7 +60,7 @@ export interface ResolvedBuilderContext { /** Where the protocol came from — surfaced in the report so it is auditable. */ protocolSource: 'status.yaml' | 'builder-id'; mode: 'strict' | 'soft'; - modeSource: 'flag' | 'builder-prompt'; + modeSource: 'flag' | 'builder-prompt' | 'task-default'; harnessName: string; harness: HarnessProvider; /** Null for a non-porch lane; that is a branch, not a failure. */ @@ -112,19 +112,97 @@ export class ContextResolutionError extends Error { // ============================================================================ /** - * Locate and parse the worktree's porch status file. + * Locate and parse THIS BUILDER's porch status file. * - * Returns null when there is no porch project — a task or shell builder is a - * legitimate reset target, it simply gets no porch re-entry block. + * Returns null when this builder has no porch project — a task or shell builder + * is a legitimate reset target, it simply gets no porch re-entry block. + * + * ## Why this takes an identity instead of picking the first parsable file + * + * The first implementation returned the first directory under `codev/projects` + * with a parsable `status.yaml`. That is correct only for a repo holding exactly + * one project directory. **This repo commits porch history to `main`**, so every + * worktree inherits every project ever run — 203 of them at the time of writing. + * The alphabetically-first is `0087-porch-timeout-termination-retries`, whose + * protocol is `spider`, so `afx reset` resolved protocol "spider" for *every* + * builder and died on `Protocol "spider" has no builder-prompt.md`. It failed + * loudly, which is why nothing was corrupted — but it failed for every lane. + * + * The fix is to select by identity, never by position: a directory qualifies + * only when it belongs to the builder being reset. No match returns null, which + * is the honest answer for a genuinely non-porch builder and is also safe for a + * porch one — a missing porch block is visible in the re-orientation, whereas + * *another project's* protocol is a confident lie. + * + * (This is the #1235 wrong-winner family: pick-the-first over a set that is + * assumed to be a singleton and is not.) * * Parsed with targeted line matching rather than a YAML dependency: this module * needs four scalar fields, and the file is machine-written by porch. */ -export function readPorchContext(fs: ContextFsPort, worktree: string): PorchContext | null { +export interface PorchProjectIdentity { + /** Registry builder id, e.g. `aspir-1273`, `builder-task-re_v`. */ + builderId: string; + /** Issue number when the lane has one. */ + issueNumber?: string; +} + +/** + * Project ids this builder could legitimately own, most specific first. + * + * Three forms, because porch names projects differently per lane: + * - the issue number, for issue-driven lanes; + * - the builder id itself — `spawn.ts` passes `builderId` as the porch project + * id on the `--task` path; + * - the builder id with its protocol prefix stripped (`aspir-1273` → `1273`), + * which is how spec-driven lanes are named. + */ +export function candidateProjectIds(identity: PorchProjectIdentity): string[] { + const out: string[] = []; + if (identity.issueNumber) out.push(identity.issueNumber); + + // Registry ids may carry a `builder-` prefix; porch never does. + const bare = identity.builderId.replace(/^builder-/, ''); + out.push(bare); + + const dash = bare.indexOf('-'); + if (dash > 0) out.push(bare.slice(dash + 1)); + + return out.filter(v => v !== ''); +} + +/** + * Compare ids ignoring case and leading zeros. + * + * Case: the registry lowercases builder ids while worktree and directory names + * preserve the original (`builder-task-re_v` vs `.builders/task-RE_V`). macOS + * hides that; a case-sensitive filesystem would not. + * Leading zeros: project dirs use `0087-…` while porch ids and CLI arguments use + * `87` (`afx cleanup -p 466` vs `0466` is the same long-standing split). + */ +function normalizeId(value: string): string { + return value.trim().toLowerCase().replace(/^0+(?=\d)/, ''); +} + +function ownsProject(dir: string, statusId: string | null, candidates: string[]): boolean { + const wanted = candidates.map(normalizeId); + if (statusId && wanted.includes(normalizeId(statusId))) return true; + + const dirNorm = normalizeId(dir); + return wanted.some(c => dirNorm === c || dirNorm.startsWith(`${c}-`)); +} + +export function readPorchContext( + fs: ContextFsPort, + worktree: string, + identity: PorchProjectIdentity, +): PorchContext | null { const projectsDir = join(worktree, 'codev', 'projects'); const dirs = fs.listDirs(projectsDir); if (!dirs || dirs.length === 0) return null; + const candidates = candidateProjectIds(identity); + for (const dir of dirs) { const statusPath = join(projectsDir, dir, 'status.yaml'); const content = fs.read(statusPath); @@ -135,6 +213,11 @@ export function readPorchContext(fs: ContextFsPort, worktree: string): PorchCont const id = matchScalar(content, 'id'); if (!protocol || !phase) continue; + // Identity check LAST, after parsing, so a malformed status.yaml belonging + // to this builder is skipped the same way any other unparsable one is — + // rather than matching on the directory name and then failing. + if (!ownsProject(dir, id, candidates)) continue; + const currentPlanPhase = matchScalar(content, 'current_plan_phase'); return { projectId: id ?? dir.split('-')[0], @@ -161,11 +244,16 @@ function matchScalar(content: string, key: string): string | null { * The protocol recorded in status.yaml, if there is one. * * Reads through `readPorchContext` rather than re-scanning the project dirs, so - * the two cannot disagree about which status.yaml is authoritative when a - * worktree somehow holds more than one project directory. + * the two cannot disagree about which status.yaml is authoritative. Worktrees + * routinely hold MANY project directories here (porch history is committed to + * `main`), which is exactly why both go through one identity-aware selector. */ -export function protocolFromStatus(fs: ContextFsPort, worktree: string): string | null { - return readPorchContext(fs, worktree)?.protocol ?? null; +export function protocolFromStatus( + fs: ContextFsPort, + worktree: string, + identity: PorchProjectIdentity, +): string | null { + return readPorchContext(fs, worktree, identity)?.protocol ?? null; } // ============================================================================ @@ -333,8 +421,14 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB ); } + // Resolved once, with identity, and reused for both the protocol chain and + // the result. Two independent scans could disagree about which project is + // this builder's — the exact ambiguity the identity match exists to remove. + const identity: PorchProjectIdentity = { builderId, issueNumber }; + const porch = readPorchContext(fs, worktree, identity); + // --- Protocol: status.yaml → builder id → abort ------------------------- - let protocol = protocolFromStatus(fs, worktree); + let protocol = porch?.protocol ?? null; let protocolSource: ResolvedBuilderContext['protocolSource'] = 'status.yaml'; if (!protocol) { const parsed = parseAgentName(builderId); @@ -358,11 +452,26 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB mode = modeFromBuilderPrompt(fs, worktree); modeSource = 'builder-prompt'; } + if (!mode && !porch) { + // A `--task` spawn writes a bare prompt with no `## Mode:` heading, so this + // lane could never auto-detect and every `afx reset ` hard-errored. + // + // Defaulting to SOFT is not a guess dressed up as a fact: "strict" means + // *porch orchestrates this builder*, and a builder with no porch project + // cannot be strict. The source is recorded as `task-default` so the report + // says where it came from rather than implying the worktree stated it. + // + // Scoped deliberately to the no-porch case. A porch-driven builder missing + // its `## Mode:` line is a genuine ambiguity and still aborts below. + mode = 'soft'; + modeSource = 'task-default'; + } if (!mode) { throw new ContextResolutionError( `Cannot determine the mode (strict/soft) for '${builderId}': no '## Mode:' line in ` + - `${join(worktree, '.builder-prompt.txt')}. Mode is not persisted anywhere else — ` + - `pass --mode strict or --mode soft explicitly.`, + `${join(worktree, '.builder-prompt.txt')}, and this builder HAS a porch project ` + + `(${porch?.projectName}), so it is not the ad-hoc-task lane that defaults to soft. ` + + `Mode is not persisted anywhere else — pass --mode strict or --mode soft explicitly.`, ); } @@ -389,7 +498,6 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB ); } - const porch = readPorchContext(fs, worktree); const { specName, specPath, planPath } = artifactPaths(fs, worktree, porch); return { diff --git a/packages/codev/src/agent-farm/commands/reset/reorient.ts b/packages/codev/src/agent-farm/commands/reset/reorient.ts index 6067aefc7..dabb26ba2 100644 --- a/packages/codev/src/agent-farm/commands/reset/reorient.ts +++ b/packages/codev/src/agent-farm/commands/reset/reorient.ts @@ -314,6 +314,29 @@ function buildLongForm(options: AssembleOptions): string { const { context: c, statePath, addendum, buildSpawnPrompt, buildResumeNotice, issue } = options; let spawnPrompt: string; + + // The bare `--task` lane has NO protocol template, and asking for one is not a + // near-miss — it is a category error. `afx spawn --task` without `--protocol` + // takes spawn.ts's `else` branch and builds the prompt directly from the task + // text (`spawn.ts:551`); there is no `protocols/task/builder-prompt.md` to + // render, so the live e2e died on + // `Protocol "task" has no builder-prompt.md`. + // + // Identified by a POSITIVE fact rather than by catching that error: task text + // present and no porch project. The `--task --protocol X` variant does get a + // porch project (`spawn.ts:548`), so this cannot swallow it — that lane still + // renders its real template below. + if (c.taskText && !c.porch) { + const longFormTask = [ + 'You are a Builder. Read codev/roles/builder.md for your full role definition.', + '', + '# Task', + '', + c.taskText, + ].join('\n'); + return assembleLongFormDocument(options, longFormTask); + } + try { spawnPrompt = buildSpawnPrompt(c.protocol, { protocol_name: c.protocol.toUpperCase(), @@ -339,6 +362,21 @@ function buildLongForm(options: AssembleOptions): string { ); } + return assembleLongFormDocument(options, spawnPrompt); +} + +/** + * Wrap a rendered prompt body in the reset header, state-file pointer, addendum + * and porch re-entry. + * + * Shared by both lanes so the framing cannot drift between them: the bare + * `--task` lane supplies spawn's task prompt, every other lane supplies the + * rendered protocol template. Only the BODY differs — everything R3 requires is + * assembled here, once. + */ +function assembleLongFormDocument(options: AssembleOptions, promptBody: string): string { + const { context: c, statePath, addendum, buildResumeNotice, issue } = options; + const header = [ '', '', @@ -416,5 +454,5 @@ function buildLongForm(options: AssembleOptions): string { header.push('', '---', '', '## Protocol framing (as delivered at spawn)', ''); - return `${header.join('\n')}\n${spawnPrompt}\n`; + return `${header.join('\n')}\n${promptBody}\n`; } From e720624eac515bff306e18503e2c87fdc6140d55 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 14:56:19 -0700 Subject: [PATCH 3/6] [Spec 1273][Verify] fix: keep the --task --protocol lane's porch project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-PR CMAP iter 1: Gemini APPROVE, Claude REQUEST_CHANGES, Gemini/Codex lane missing. Claude's finding accepted — a real regression introduced by my own F1 fix. candidateProjectIds stripped the `builder-` prefix and offered only the stripped forms. But `spawn --task --protocol X` passes the FULL builderId to porch init (spawn.ts:548), and initPorchInWorktree keeps dashes when sanitising (spawn-worktree.ts:472), so that lane's porch project id really is `builder-task-`. No candidate matched it, so readPorchContext returned null and the builder was re-oriented as protocol TASK with no porch re-entry. That failure mode is worse than the one F1 fixed: `spider` aborted loudly and touched nothing, whereas this silently produced a DEGRADED frame — a porch-driven builder told it had no porch. Exactly the silent-drift class R3 exists to prevent, reintroduced by the fix for a loud bug. Narrowing a match is as dangerous as widening it. Now offers the raw registry id as a candidate as well as the stripped form. Tests: the porch id with a raw `builder-` prefix resolves, and the lane end-to-end keeps protocolSource 'status.yaml' and modeSource 'builder-prompt' (NOT the task default — this lane has porch, so the F2 default must not fire for it). Live re-verified, unchanged: 1273 -> ASPIR, 1286 -> ASPIR, task-re_v -> TASK. Suite 4027 -> 4029, build clean. --- .../__tests__/spec-1273-reset-context.test.ts | 53 +++++++++++++++++++ .../src/agent-farm/commands/reset/context.ts | 13 ++++- 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts index e965caa49..52f9ea039 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts @@ -604,3 +604,56 @@ describe('mode resolution for the ad-hoc task lane (Spec 1273 F2)', () => { expect(ctx.modeSource).toBe('flag'); }); }); + +describe('the --task --protocol lane keeps its porch project (Spec 1273 verify iter 1)', () => { + it('matches a porch project id that carries the raw builder- prefix', () => { + // `spawn --task --protocol X` passes the FULL builderId to porch init + // (spawn.ts:548) and the sanitiser keeps dashes, so the project id really is + // `builder-task-`. Matching only the stripped forms orphaned this lane: + // porch resolved to null and the builder was re-oriented as protocol TASK + // with no porch re-entry — degraded silently rather than failing loudly. + const dir = 'builder-task-abc-task-abc'; + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '0087-old', 'status.yaml')]: + "id: '0087'\nprotocol: spider\nphase: implement\n", + [join(WORKTREE, 'codev', 'projects', dir, 'status.yaml')]: + "id: 'builder-task-abc'\nprotocol: air\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['0087-old', dir] }, + ); + + const porch = readPorchContext(fs, WORKTREE, { builderId: 'builder-task-abc' }); + + expect(porch?.projectId).toBe('builder-task-abc'); + expect(porch?.protocol).toBe('air'); + }); + + it('still resolves protocol and porch re-entry for that lane end to end', () => { + const dir = 'builder-task-abc-task-abc'; + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', dir, 'status.yaml')]: + "id: 'builder-task-abc'\nprotocol: air\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: [dir] }, + ); + + const ctx = resolveBuilderContext({ + fs, + builderId: 'builder-task-abc', + worktree: WORKTREE, + branch: 'builder/task-abc', + }); + + expect(ctx.protocol).toBe('air'); + expect(ctx.protocolSource).toBe('status.yaml'); + expect(ctx.porch).not.toBeNull(); + // Mode comes from the prompt, NOT the task default — this lane has porch. + expect(ctx.modeSource).toBe('builder-prompt'); + }); +}); diff --git a/packages/codev/src/agent-farm/commands/reset/context.ts b/packages/codev/src/agent-farm/commands/reset/context.ts index 850159a8c..50a0896d3 100644 --- a/packages/codev/src/agent-farm/commands/reset/context.ts +++ b/packages/codev/src/agent-farm/commands/reset/context.ts @@ -161,9 +161,18 @@ export function candidateProjectIds(identity: PorchProjectIdentity): string[] { const out: string[] = []; if (identity.issueNumber) out.push(identity.issueNumber); - // Registry ids may carry a `builder-` prefix; porch never does. + // The raw registry id, INCLUDING any `builder-` prefix. `spawn --task + // --protocol X` passes `builderId` straight to `porch init` + // (`spawn.ts:548`), and `initPorchInWorktree` keeps dashes when sanitising + // (`spawn-worktree.ts:472`), so that lane's porch project id really is + // `builder-task-`. Omitting this form orphaned it: porch resolved to + // null, and the builder was re-oriented as protocol TASK with no porch + // re-entry — a silently degraded frame rather than a loud failure. + out.push(identity.builderId); + + // And without the prefix, which is how spec/bugfix lanes are named. const bare = identity.builderId.replace(/^builder-/, ''); - out.push(bare); + if (bare !== identity.builderId) out.push(bare); const dash = bare.indexOf('-'); if (dash > 0) out.push(bare.slice(dash + 1)); From e8ea199d8bdab15eaf966dd5f9fdc4039c6742b1 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 15:11:20 -0700 Subject: [PATCH 4/6] [Spec 1273][Verify] fix: never fabricate an issue number from a porch project id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-PR CMAP iter 2: Gemini APPROVE, Claude COMMENT (one item flagged worth fixing before merge), Codex lane unavailable. Accepted. `issueNumber: issueNumber ?? porch?.projectId` assumed every porch project id is an issue number. For the `--task --protocol` lane the previous commit just un-orphaned, that id is `builder-task-`, so the re-orientation rendered `- Issue: #builder-task-abc` and an unfollowable `gh issue view builder-task-abc`. A fabricated issue reference is worse than no issue line: on issue-driven protocols the issue body IS the spec, so this sends a freshly-reset builder — one with no history to notice the oddity — to look up requirements that do not exist. Now guarded with /^\d+$/; a non-numeric project id yields undefined and the frame simply omits the issue. Pinned in the existing --task --protocol end-to-end test rather than a new one, so the lane's expectations live together. Live re-verified, unchanged: 1273 -> ASPIR, 1286 -> ASPIR, task-re_v -> TASK. Suite 4029, build clean. --- .../__tests__/spec-1273-reset-context.test.ts | 5 +++++ .../codev/src/agent-farm/commands/reset/context.ts | 10 ++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts index 52f9ea039..c350e59ab 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts @@ -655,5 +655,10 @@ describe('the --task --protocol lane keeps its porch project (Spec 1273 verify i expect(ctx.porch).not.toBeNull(); // Mode comes from the prompt, NOT the task default — this lane has porch. expect(ctx.modeSource).toBe('builder-prompt'); + // And no fabricated issue: this lane's porch id is `builder-task-abc`, not + // an issue number. Falling back to it would render `- Issue: #builder-task-abc` + // and an unfollowable `gh issue view` in the re-orientation — sending a + // reset builder to look up requirements that do not exist. + expect(ctx.issueNumber).toBeUndefined(); }); }); diff --git a/packages/codev/src/agent-farm/commands/reset/context.ts b/packages/codev/src/agent-farm/commands/reset/context.ts index 50a0896d3..655732954 100644 --- a/packages/codev/src/agent-farm/commands/reset/context.ts +++ b/packages/codev/src/agent-farm/commands/reset/context.ts @@ -524,8 +524,14 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB specPath, planPath, // Issue-driven protocols name the porch project after the issue, so the - // project id is the correct value when the registry row has none. - issueNumber: issueNumber ?? porch?.projectId, + // project id is the correct value when the registry row has none — but ONLY + // when it is actually an issue number. The `--task --protocol` lane's porch + // id is `builder-task-`, which would render `- Issue: #builder-task-abc` + // and an unfollowable `gh issue view builder-task-abc` in the + // re-orientation. A fabricated issue reference is worse than none: it sends + // a freshly-reset builder to look up requirements that do not exist. + issueNumber: + issueNumber ?? (porch && /^\d+$/.test(porch.projectId) ? porch.projectId : undefined), taskText, }; } From 909e1c0360bb228d82f0b6bbbfd5030eb468ebe8 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 15:27:25 -0700 Subject: [PATCH 5/6] [Spec 1273][Verify] fix: a bare project number cannot claim a builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verify-PR CMAP iter 3: Gemini APPROVE, Claude REQUEST_CHANGES (verified live). Accepted — the wrong-winner class SURVIVED my first fix. F1 stopped alphabetically-first from winning, but a bare-NUMBER match still let one project claim any builder sharing the digits, across protocols. Confirmed in this repo: issue 799 has a PIR project (799-vscode-builder-changed-file-ro, protocol pir) and a bugfix, so `builder-bugfix-799` silently adopted the PIR project's protocol and porch id. Silent, not loud — the degraded-frame failure mode, worse than the `spider` abort it replaced. Claims are now graded: strong — the project id equals the raw registry builder id and is NON-numeric (`builder-task-abc`). Globally unique, so it needs no corroboration — which matters because that lane's protocol legitimately differs from its id prefix (task vs air). weak — matched on the bare number or a directory prefix. Requires the project's protocol to AGREE with the protocol the builder id declares. none — not this builder's. Two or more surviving claims abort loudly rather than picking one. Choosing the least-wrong of several is the original bug with better manners. DELIBERATE SEMANTIC REVERSAL, called out because it changes phase-4 behaviour and breaks a test I wrote there: "prefers status.yaml over the builder id when they disagree" no longer holds for the PROTOCOL field. status.yaml remains authoritative for a builder's own project (phase, plan phase, identity), but it can no longer DECIDE which project is its own. The two principles cannot both hold — the signal is identical in each case — and the live harm settles it: a hypothetical id/status protocol disagreement within one project is the cost; adopting a stranger's project is what is actually happening here. The test now encodes the new rule with that reasoning. Tests: the 799 collision (bugfix builder gets null, PIR builder gets the project, and the bugfix builder still resolves protocol from its id with no porch block — visible absence, not a confident lie), the non-numeric id crossing a protocol mismatch, and the two-equal-claims abort. Three earlier fixtures paired a builder with a foreign-protocol project and were unrealistic; rebuilt protocol-consistent so they still exercise zero/case tolerance. Live re-verified: 1273 -> ASPIR, 1286 -> ASPIR, task-re_v -> TASK. Suite 4029 -> 4035, build clean. --- .../__tests__/spec-1273-reset-context.test.ts | 172 ++++++++++++++++-- .../src/agent-farm/commands/reset/context.ts | 75 ++++++-- 2 files changed, 225 insertions(+), 22 deletions(-) diff --git a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts index c350e59ab..8bec80d80 100644 --- a/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spec-1273-reset-context.test.ts @@ -122,10 +122,38 @@ describe('protocol resolution (Spec 1273)', () => { ).toThrow(ContextResolutionError); }); - it('prefers status.yaml over the builder id when they disagree', () => { - // status.yaml is what porch is actually running. - const ctx = resolveBuilderContext({ fs: fullWorktree(), ...BASE, builderId: 'builder-spir-1273' }); + it('prefers status.yaml over the builder id when they agree on ownership', () => { + // status.yaml is what porch is actually running, so it wins for phase, + // plan phase and project identity. + const ctx = resolveBuilderContext({ fs: fullWorktree(), ...BASE }); expect(ctx.protocol).toBe('aspir'); + expect(ctx.protocolSource).toBe('status.yaml'); + }); + + it('treats a PROTOCOL disagreement as "not my project", not as a correction', () => { + // DELIBERATE REVERSAL of the original phase-4 behaviour, forced by the + // verify-phase e2e. + // + // Before: a project matching on the number won even when its protocol + // disagreed with the builder id, on the principle that status.yaml is + // authoritative. That principle is right for a builder's OWN project and + // wrong as a way of DECIDING which project is its own — and the two cannot + // both hold, because the signal is identical in each case. + // + // Live harm decided it: issue 799 is used by both a PIR project and a + // bugfix, so `builder-bugfix-799` silently adopted the PIR project's + // protocol and porch id. A hypothetical id/status protocol disagreement + // within one project is the cost; adopting a stranger's project is the + // thing actually happening in this repo. + const ctx = resolveBuilderContext({ + fs: fullWorktree(), + ...BASE, + builderId: 'builder-spir-1273', + }); + + expect(ctx.protocol).toBe('spir'); + expect(ctx.protocolSource).toBe('builder-id'); + expect(ctx.porch).toBeNull(); }); it('protocolFromStatus returns null when there are no project dirs', () => { @@ -517,7 +545,22 @@ describe('porch project selection is by identity, never by position (Spec 1273 F }); it('matches on the issue number for an issue-driven lane', () => { - const porch = readPorchContext(inheritedHistoryWorktree('1279-fix-something'), WORKTREE, { + // The builder's own project carries its own protocol — a bugfix builder's + // project is a bugfix project. A number match against a project of a + // DIFFERENT protocol is rejected; see the number-collision suite below. + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '0087-old', 'status.yaml')]: + "id: '0087'\nprotocol: spider\nphase: implement\n", + [join(WORKTREE, 'codev', 'projects', '1279-fix-something', 'status.yaml')]: + "id: '1279'\nprotocol: bugfix\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['0087-old', '1279-fix-something'] }, + ); + + const porch = readPorchContext(fs, WORKTREE, { builderId: 'builder-bugfix-1279', issueNumber: '1279', }); @@ -527,18 +570,34 @@ describe('porch project selection is by identity, never by position (Spec 1273 F it('tolerates leading zeros between the id and the directory name', () => { // Project dirs use `0087-…` while porch ids and CLI arguments use `87` // (the same split as `afx cleanup -p 466` vs `0466`). - const porch = readPorchContext(inheritedHistoryWorktree(), WORKTREE, { - builderId: 'builder-bugfix-87', - }); - expect(porch?.projectName).toBe('0087-porch-timeout-termination-retries'); + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '0087-old-bugfix', 'status.yaml')]: + "id: '0087'\nprotocol: bugfix\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['0087-old-bugfix'] }, + ); + + const porch = readPorchContext(fs, WORKTREE, { builderId: 'builder-bugfix-87' }); + expect(porch?.projectName).toBe('0087-old-bugfix'); }); it('matches case-insensitively, so a case-sensitive filesystem behaves like macOS', () => { // The registry lowercases builder ids while directories preserve case // (`builder-task-re_v` vs `.builders/task-RE_V`). macOS hides the mismatch. - const porch = readPorchContext(inheritedHistoryWorktree('task-RE_V-probe'), WORKTREE, { - builderId: 'builder-task-re_v', - }); + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', 'task-RE_V-probe', 'status.yaml')]: + "id: 'task-RE_V'\nprotocol: task\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['task-RE_V-probe'] }, + ); + + const porch = readPorchContext(fs, WORKTREE, { builderId: 'builder-task-re_v' }); expect(porch?.projectName).toBe('task-RE_V-probe'); }); }); @@ -662,3 +721,94 @@ describe('the --task --protocol lane keeps its porch project (Spec 1273 verify i expect(ctx.issueNumber).toBeUndefined(); }); }); + +describe('a bare project NUMBER is not enough to claim a builder (Spec 1273 verify iter 3)', () => { + /** Issue 799 really is used by both a PIR project and a bugfix in this repo. */ + function numberCollisionWorktree() { + return makeFs( + { + [join(WORKTREE, 'codev', 'projects', '799-vscode-builder-changed-file-ro', 'status.yaml')]: + "id: '799'\nprotocol: pir\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['799-vscode-builder-changed-file-ro'] }, + ); + } + + it('does not hand a bugfix builder the PIR project that shares its number', () => { + // The wrong-winner class, second instance: the first fix stopped + // alphabetically-first from winning, but a bare-number match still let ONE + // project claim any builder sharing the digits — across protocols. Verified + // live on builder-bugfix-799. + const porch = readPorchContext(numberCollisionWorktree(), WORKTREE, { + builderId: 'builder-bugfix-799', + issueNumber: '799', + }); + + expect(porch).toBeNull(); + }); + + it('accepts the same number when the protocol agrees', () => { + const porch = readPorchContext(numberCollisionWorktree(), WORKTREE, { + builderId: 'builder-pir-799', + issueNumber: '799', + }); + + expect(porch?.projectName).toBe('799-vscode-builder-changed-file-ro'); + expect(porch?.protocol).toBe('pir'); + }); + + it('falls back to the builder id for the protocol, rather than adopting a foreign one', () => { + // The end-to-end consequence: with no project of its own, the bugfix builder + // still resolves protocol `bugfix` from its id and simply gets no porch + // block — a visible absence, not a confident lie. + const ctx = resolveBuilderContext({ + fs: numberCollisionWorktree(), + builderId: 'builder-bugfix-799', + worktree: WORKTREE, + branch: 'builder/bugfix-799', + issueNumber: '799', + }); + + expect(ctx.protocol).toBe('bugfix'); + expect(ctx.protocolSource).toBe('builder-id'); + expect(ctx.porch).toBeNull(); + }); + + it('still lets a non-numeric project id claim across a protocol mismatch', () => { + // The --task --protocol lane: id `builder-task-abc` is globally unique, so + // it needs no corroboration — which matters because its protocol (air) + // legitimately differs from its id prefix (task). + const dir = 'builder-task-abc-task-abc'; + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', dir, 'status.yaml')]: + "id: 'builder-task-abc'\nprotocol: air\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: [dir] }, + ); + + expect(readPorchContext(fs, WORKTREE, { builderId: 'builder-task-abc' })?.protocol).toBe('air'); + }); + + it('aborts rather than choosing between two equally-strong claims', () => { + // Picking the "least wrong" of several is the original bug with better + // manners. If two projects genuinely claim a builder, say so. + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '1273-a', 'status.yaml')]: + "id: '1273'\nprotocol: aspir\nphase: implement\n", + [join(WORKTREE, 'codev', 'projects', '1273-b', 'status.yaml')]: + "id: '1273'\nprotocol: aspir\nphase: review\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['1273-a', '1273-b'] }, + ); + + expect(() => readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })).toThrow(/Ambiguous/); + }); +}); diff --git a/packages/codev/src/agent-farm/commands/reset/context.ts b/packages/codev/src/agent-farm/commands/reset/context.ts index 655732954..1dca3218a 100644 --- a/packages/codev/src/agent-farm/commands/reset/context.ts +++ b/packages/codev/src/agent-farm/commands/reset/context.ts @@ -193,12 +193,36 @@ function normalizeId(value: string): string { return value.trim().toLowerCase().replace(/^0+(?=\d)/, ''); } -function ownsProject(dir: string, statusId: string | null, candidates: string[]): boolean { - const wanted = candidates.map(normalizeId); - if (statusId && wanted.includes(normalizeId(statusId))) return true; +/** + * How strongly a project directory claims to belong to this builder. + * + * `none` — not this builder's. + * `weak` — matched on the bare project NUMBER (or a directory prefix). Numbers + * are reused across protocols: issue 799 has both a PIR project + * (`799-vscode-builder-changed-file-ro`) and a bugfix, so a bare-number + * match alone would hand `builder-bugfix-799` the PIR project's + * protocol and porch id. Requires protocol corroboration. + * `strong` — matched on a globally unique, non-numeric project id, i.e. the raw + * registry builder id. `spawn --task --protocol X` stores exactly that + * (`builder-task-`), so it identifies one builder and needs no + * corroboration — which matters because that lane's PROTOCOL + * legitimately differs from its id prefix (`task` vs `air`). + */ +type ProjectClaim = 'none' | 'weak' | 'strong'; + +function claimStrength( + dir: string, + statusId: string | null, + identity: PorchProjectIdentity, +): ProjectClaim { + const rawId = normalizeId(identity.builderId); + if (statusId && normalizeId(statusId) === rawId && !/^\d+$/.test(rawId)) return 'strong'; + + const wanted = candidateProjectIds(identity).map(normalizeId); + if (statusId && wanted.includes(normalizeId(statusId))) return 'weak'; const dirNorm = normalizeId(dir); - return wanted.some(c => dirNorm === c || dirNorm.startsWith(`${c}-`)); + return wanted.some(c => dirNorm === c || dirNorm.startsWith(`${c}-`)) ? 'weak' : 'none'; } export function readPorchContext( @@ -210,7 +234,13 @@ export function readPorchContext( const dirs = fs.listDirs(projectsDir); if (!dirs || dirs.length === 0) return null; - const candidates = candidateProjectIds(identity); + // The protocol the builder id claims, used to corroborate weak (number-only) + // matches. Absent for ids that are not in canonical form, in which case a weak + // match cannot be corroborated and is not trusted. + const expectedProtocol = parseAgentName(identity.builderId)?.protocol ?? null; + + const strong: PorchContext[] = []; + const weak: PorchContext[] = []; for (const dir of dirs) { const statusPath = join(projectsDir, dir, 'status.yaml'); @@ -222,13 +252,11 @@ export function readPorchContext( const id = matchScalar(content, 'id'); if (!protocol || !phase) continue; - // Identity check LAST, after parsing, so a malformed status.yaml belonging - // to this builder is skipped the same way any other unparsable one is — - // rather than matching on the directory name and then failing. - if (!ownsProject(dir, id, candidates)) continue; + const claim = claimStrength(dir, id, identity); + if (claim === 'none') continue; const currentPlanPhase = matchScalar(content, 'current_plan_phase'); - return { + const ctx: PorchContext = { projectId: id ?? dir.split('-')[0], projectName: dir, protocol, @@ -236,9 +264,34 @@ export function readPorchContext( currentPlanPhase: currentPlanPhase && currentPlanPhase !== 'null' ? currentPlanPhase : null, statusPath, }; + + if (claim === 'strong') { + strong.push(ctx); + continue; + } + + // A weak match must agree with the protocol the builder id declares. + // Without this, `builder-bugfix-799` adopts the PIR project that happens to + // share the number — wrong protocol, wrong porch id, and silently so. + if (expectedProtocol && protocol.toLowerCase() !== expectedProtocol.toLowerCase()) continue; + weak.push(ctx); } - return null; + const matches = strong.length > 0 ? strong : weak; + if (matches.length === 0) return null; + + if (matches.length > 1) { + // Never pick one arbitrarily — that is the bug this whole function exists to + // fix, and picking the "least wrong" of several is the same mistake with + // better manners. + throw new ContextResolutionError( + `Ambiguous porch project for '${identity.builderId}': ${matches + .map(m => `${m.projectName} (${m.protocol})`) + .join(', ')} all claim it. Refusing to guess which one governs this builder.`, + ); + } + + return matches[0]; } /** Read a top-level scalar from porch's status.yaml, stripping quotes. */ From ae0ede36fae28318b562c5ab95a3934e8796bdbe Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Fri, 31 Jul 2026 15:33:24 -0700 Subject: [PATCH 6/6] [Spec 1273][Verify] Review: record the verify-phase findings and lessons --- .../1273-builder-context-reset-should-b.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/codev/reviews/1273-builder-context-reset-should-b.md b/codev/reviews/1273-builder-context-reset-should-b.md index 17ad64399..a4233ab0b 100644 --- a/codev/reviews/1273-builder-context-reset-should-b.md +++ b/codev/reviews/1273-builder-context-reset-should-b.md @@ -151,6 +151,56 @@ files were genuine so the history reconstructed cleanly, but porch does not watc unrelated to this work. The sections added here are byte-identical across both trees; the existing drift is out of scope and left for a reconciliation pass. +## Verify phase — the e2e found it non-functional (2026-07-31) + +The live run vindicated the caveat above: **the headline path failed on every lane in production**, and +every failure was in *resolution*, upstream of the destructive path. The R1–R4 invariants held — +`--dry-run` wrote nothing, every failure was a refusal rather than a corruption, and no builder ever +received a wrong re-orientation. + +**F1 — wrong-winner project selection (blocker, all lanes).** `readPorchContext` returned the *first* +directory under `codev/projects` with a parsable `status.yaml`. Correct for a repo with one project dir; +wrong here, because porch history is committed to `main` and every worktree inherits every project ever +run — 203 of them. The alphabetically-first is a `spider`-era project, so every builder resolved protocol +`spider` and died on `Protocol "spider" has no builder-prompt.md`. + +The comment I had written at that function — *"when a worktree somehow holds more than one project +directory"* — named the assumption that broke. It was not "somehow": it is the normal state of every +worktree in this repo. **Writing down an assumption is not the same as checking it.** + +**F1b — the fix's own regressions, both found by review.** Narrowing the match orphaned the +`--task --protocol` lane (porch stores the raw `builder-task-` there), which produced a *silently +degraded* frame — a porch-driven builder told it had no porch. That is worse than the loud bug it +replaced. Then the first fix's bare-number matching still let issue 799's PIR project claim +`builder-bugfix-799`. **Narrowing a match is as dangerous as widening it**, and the wrong-winner class +took three passes to actually close: alphabetical → numeric collision → protocol corroboration. + +**F2 — the task lane could never auto-detect its mode.** Mode detection requires a `## Mode:` line that +`--task` spawns never render. Now defaults to soft with `modeSource: 'task-default'`, scoped to the +no-porch case, because *strict* means porch orchestrates and a builder with no porch project cannot be +strict. + +**F3 — reported, but not a defect here.** The printed path came verbatim from the registry and was +correct; the id/directory case divergence originates at spawn. Matching is now case-insensitive anyway so +a case-sensitive filesystem behaves like macOS. + +### A deliberate reversal + +`status.yaml` is no longer allowed to override the builder id on the **protocol** field. It remains +authoritative for a builder's own project, but it can no longer *decide which project is its own* — the +two principles cannot both hold, because the signal is identical in each case. A hypothetical +protocol disagreement within one project is the cost; adopting a stranger's project was actually +happening. + +### The lesson this phase adds + +**"Tests pass" and "CI is green" could not have found any of this.** Every F1 variant was a property of +*this repo's data shape* — 203 inherited project directories, a number reused across protocols — that no +unit fixture encoded because I wrote the fixtures from my model of the world rather than from the world. +The e2e was the first thing to run reset against reality, and it failed instantly on every lane. That is +the strongest possible restatement of the hot-tier lesson: *"it compiled" / "tests pass" is not "it +works" — verify the real user path end-to-end.* + ## Flaky Tests None encountered. No tests were skipped by this work.