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 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. 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..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,14 +122,42 @@ 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', () => { - expect(protocolFromStatus(makeFs({}), WORKTREE)).toBeNull(); + expect(protocolFromStatus(makeFs({}), WORKTREE, { builderId: BUILDER_ID })).toBeNull(); }); }); @@ -139,7 +167,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 +178,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 +188,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 +476,339 @@ 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', () => { + // 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', + }); + 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 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 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'); + }); +}); + +// ============================================================================ +// 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'); + }); +}); + +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'); + // 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(); + }); +}); + +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/__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..1dca3218a 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,136 @@ 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); + + // 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-/, ''); + if (bare !== identity.builderId) 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)/, ''); +} + +/** + * 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}-`)) ? 'weak' : 'none'; +} + +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; + // 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'); const content = fs.read(statusPath); @@ -135,8 +252,11 @@ export function readPorchContext(fs: ContextFsPort, worktree: string): PorchCont const id = matchScalar(content, 'id'); if (!protocol || !phase) 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, @@ -144,9 +264,34 @@ export function readPorchContext(fs: ContextFsPort, worktree: string): PorchCont 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. */ @@ -161,11 +306,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 +483,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 +514,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 +560,6 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB ); } - const porch = readPorchContext(fs, worktree); const { specName, specPath, planPath } = artifactPaths(fs, worktree, porch); return { @@ -407,8 +577,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, }; } 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`; }