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 8bec80d80..f8a86d1f0 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 @@ -19,6 +19,7 @@ import { resolveBuilderContext, readPorchContext, protocolFromStatus, + issueNumberFromPorchId, modeFromBuilderPrompt, harnessFromLaunchScript, ContextResolutionError, @@ -627,6 +628,7 @@ describe('mode resolution for the ad-hoc task lane (Spec 1273 F2)', () => { builderId: 'builder-task-re_v', worktree: WORKTREE, branch: 'builder/task-RE_V', + taskText: 'Be a probe.', }); expect(ctx.mode).toBe('soft'); @@ -657,6 +659,7 @@ describe('mode resolution for the ad-hoc task lane (Spec 1273 F2)', () => { builderId: 'builder-task-re_v', worktree: WORKTREE, branch: 'builder/task-RE_V', + taskText: 'Be a probe.', modeOverride: 'strict', }); expect(ctx.mode).toBe('strict'); @@ -812,3 +815,117 @@ describe('a bare project NUMBER is not enough to claim a builder (Spec 1273 veri expect(() => readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })).toThrow(/Ambiguous/); }); }); + +// ============================================================================ +// Retroactive codex review of the merged #1308 (2026-07-31) +// ============================================================================ + +describe('post-merge codex findings (Spec 1273)', () => { + it('a status.yaml that states a DIFFERENT id is not overruled by its directory name', () => { + // `codev/projects/1273-old/` holding `id: '999'` belongs to 999, whatever + // the directory is called. The dir-name fallback previously overruled that, + // letting a renamed or recycled directory claim a builder — and + // manufacturing false ambiguities beside the real project. + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '1273-old', 'status.yaml')]: + "id: '999'\nprotocol: aspir\nphase: implement\n", + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['1273-old'] }, + ); + + expect(readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })).toBeNull(); + }); + + it('still falls back to the directory name when the file states no id at all', () => { + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '1273-x', 'status.yaml')]: + 'protocol: aspir\nphase: implement\n', + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['1273-x'] }, + ); + + expect(readPorchContext(fs, WORKTREE, { builderId: BUILDER_ID })?.projectName).toBe('1273-x'); + }); + + it('rejects a weak claim when the builder id yields no protocol to corroborate with', () => { + // The previous code's own comment said a noncanonical id "cannot be + // corroborated and is not trusted" while `if (expectedProtocol && mismatch)` + // let EVERY weak claim through when expectedProtocol was null. A legacy + // builder could adopt any historical project sharing its tail. + const fs = makeFs( + { + [join(WORKTREE, 'codev', 'projects', '1273-x', 'status.yaml')]: STATUS_YAML, + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: ['1273-x'] }, + ); + + expect(readPorchContext(fs, WORKTREE, { builderId: 'some-legacy-name-1273' })).toBeNull(); + }); + + it('recovers a BUGFIX issue number from its - porch id', () => { + // BUGFIX deliberately stores `bugfix-` (spawn.ts:817). The strict /^\d+$/ + // guard threw that identity away when the registry row had none — and on + // BUGFIX the issue body IS the spec, so the re-orientation lost the + // requirements it exists to carry. + expect(issueNumberFromPorchId('bugfix-887')).toBe('887'); + expect(issueNumberFromPorchId('1273')).toBe('1273'); + // But an ad-hoc task id still cannot masquerade as an issue. + expect(issueNumberFromPorchId('builder-task-abc')).toBeUndefined(); + expect(issueNumberFromPorchId(undefined)).toBeUndefined(); + }); + + it('does not treat a --task --protocol builder as bare when porch init failed', () => { + // initPorchInWorktree is deliberately non-fatal, so that lane can have task + // text AND no porch. Inferring "bare" from that stripped its real protocol + // template. The prompt is written BEFORE porch init, so its rendered + // template — and its `## Mode:` line — is the positive evidence. + const fs = makeFs( + { + [join(WORKTREE, '.builder-prompt.txt')]: BUILDER_PROMPT, // rendered template, has Mode + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: [] }, + ); + + const ctx = resolveBuilderContext({ + fs, + builderId: 'builder-task-abc', + worktree: WORKTREE, + branch: 'builder/task-abc', + taskText: 'ad-hoc work', + }); + + expect(ctx.isBareTask).toBe(false); + // And its real mode survives, rather than being defaulted to soft. + expect(ctx.mode).toBe('strict'); + expect(ctx.modeSource).toBe('builder-prompt'); + }); + + it('is bare only with task text AND no rendered template', () => { + const bare = makeFs( + { + [join(WORKTREE, '.builder-prompt.txt')]: 'You are a Builder.\n\n# Task\n\nBe a probe.', + [join(WORKTREE, '.builder-start.sh')]: LAUNCH_SCRIPT, + }, + { [join(WORKTREE, 'codev', 'projects')]: [] }, + ); + + const ctx = resolveBuilderContext({ + fs: bare, + builderId: 'builder-task-re_v', + worktree: WORKTREE, + branch: 'builder/task-RE_V', + taskText: 'Be a probe.', + }); + + expect(ctx.isBareTask).toBe(true); + }); +}); 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 bf7e572c5..79686c9d4 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 @@ -63,6 +63,7 @@ function makeContext(overrides: Partial = {}): ResolvedB specPath: 'codev/specs/1273-builder-context-reset-should-b.md', planPath: 'codev/plans/1273-builder-context-reset-should-b.md', issueNumber: '1273', + isBareTask: false, ...overrides, }; } @@ -466,6 +467,7 @@ describe('long-form re-orientation (Spec 1273)', () => { planPath: null, issueNumber: undefined, taskText: 'You are a reset-test probe.', + isBareTask: true, }), statePath: STATE_PATH, buildSpawnPrompt: port, diff --git a/packages/codev/src/agent-farm/commands/reset/context.ts b/packages/codev/src/agent-farm/commands/reset/context.ts index 1dca3218a..ac41da849 100644 --- a/packages/codev/src/agent-farm/commands/reset/context.ts +++ b/packages/codev/src/agent-farm/commands/reset/context.ts @@ -87,6 +87,21 @@ export interface ResolvedBuilderContext { * this field the task lane is indistinguishable from an issue-driven one. */ taskText?: string; + /** + * True only for a BARE `afx spawn --task` builder (no `--protocol`). + * + * Established from positive evidence — task text present AND the prompt file + * carries no `## Mode:` heading — rather than inferred from the absence of a + * porch project. `initPorchInWorktree` is deliberately non-fatal, so a + * `--task --protocol X` builder whose porch init failed also has task text and + * no porch, and inferring from that would strip its real protocol template and + * hand it the raw task text instead. + * + * The prompt is built BEFORE porch init (`spawn.ts:545-548`), so a + * `--task --protocol` builder's prompt carries the rendered template — and its + * `## Mode:` line — regardless of whether porch init later succeeded. + */ + isBareTask: boolean; } export interface PorchContext { @@ -219,8 +234,16 @@ function claimStrength( if (statusId && normalizeId(statusId) === rawId && !/^\d+$/.test(rawId)) return 'strong'; const wanted = candidateProjectIds(identity).map(normalizeId); - if (statusId && wanted.includes(normalizeId(statusId))) return 'weak'; + // A status.yaml that STATES its id is authoritative about what it is. If that + // id does not claim this builder, the directory name cannot overrule it — + // `codev/projects/1273-old/` holding `id: '999'` belongs to 999, whatever the + // directory is called. Falling through to the name here let a renamed or + // recycled directory claim a builder, and manufactured false ambiguities + // alongside the real project. + if (statusId) return wanted.includes(normalizeId(statusId)) ? 'weak' : 'none'; + + // Directory-name fallback applies ONLY when the file states no id at all. const dirNorm = normalizeId(dir); return wanted.some(c => dirNorm === c || dirNorm.startsWith(`${c}-`)) ? 'weak' : 'none'; } @@ -273,7 +296,15 @@ export function readPorchContext( // 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; + // + // And when the id is not in canonical form there IS no protocol to + // corroborate against, so the claim cannot be trusted at all. The previous + // version's comment said exactly that while the code did the opposite — + // `if (expectedProtocol && mismatch) continue` let every weak claim through + // whenever `expectedProtocol` was null. A legacy or noncanonical builder + // could adopt any historical project sharing its tail. + if (!expectedProtocol) continue; + if (protocol.toLowerCase() !== expectedProtocol.toLowerCase()) continue; weak.push(ctx); } @@ -338,6 +369,25 @@ export function modeFromBuilderPrompt(fs: ContextFsPort, worktree: string): 'str return m[1].toLowerCase() as 'strict' | 'soft'; } +/** + * Recover an issue number from a porch project id. + * + * Porch ids are not uniformly numeric. PIR and SPIR use the bare issue number; + * **BUGFIX deliberately uses `-`** (`spawn.ts:817`, "historical, kept + * untouched"). A strict `/^\d+$/` guard therefore threw away BUGFIX's issue + * identity whenever the registry row lacked one — and on BUGFIX the issue body + * IS the spec, so the re-orientation lost the requirements it was meant to carry. + * + * Accepts a bare number or a canonical `-`; rejects anything else, + * so an ad-hoc task id (`builder-task-abc`) still cannot masquerade as an issue. + */ +export function issueNumberFromPorchId(projectId?: string): string | undefined { + if (!projectId) return undefined; + if (/^\d+$/.test(projectId)) return projectId; + const m = projectId.match(/^[a-z]+-(\d+)$/i); + return m ? m[1] : undefined; +} + // ============================================================================ // Harness // ============================================================================ @@ -508,13 +558,20 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB } // --- Mode: flag → .builder-prompt.txt → abort --------------------------- + const promptMode = modeFromBuilderPrompt(fs, worktree); + + // Positive evidence, not a negative inference: a bare `--task` spawn writes a + // prompt with no `## Mode:` heading, while `--task --protocol X` renders the + // full template (built BEFORE porch init, so it survives a failed init). + const isBareTask = Boolean(taskText) && promptMode === null; + let mode = modeOverride ?? null; let modeSource: ResolvedBuilderContext['modeSource'] = 'flag'; if (!mode) { - mode = modeFromBuilderPrompt(fs, worktree); + mode = promptMode; modeSource = 'builder-prompt'; } - if (!mode && !porch) { + if (!mode && isBareTask) { // A `--task` spawn writes a bare prompt with no `## Mode:` heading, so this // lane could never auto-detect and every `afx reset ` hard-errored. // @@ -531,8 +588,8 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB if (!mode) { throw new ContextResolutionError( `Cannot determine the mode (strict/soft) for '${builderId}': no '## Mode:' line in ` + - `${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. ` + + `${join(worktree, '.builder-prompt.txt')}, and this builder is not the bare ad-hoc-task ` + + `lane that defaults to soft (no task text, or a rendered protocol template). ` + `Mode is not persisted anywhere else — pass --mode strict or --mode soft explicitly.`, ); } @@ -583,8 +640,8 @@ export function resolveBuilderContext(options: ResolveContextOptions): ResolvedB // 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), + issueNumber: issueNumber ?? issueNumberFromPorchId(porch?.projectId), taskText, + isBareTask, }; } diff --git a/packages/codev/src/agent-farm/commands/reset/reorient.ts b/packages/codev/src/agent-farm/commands/reset/reorient.ts index dabb26ba2..ea7751a76 100644 --- a/packages/codev/src/agent-farm/commands/reset/reorient.ts +++ b/packages/codev/src/agent-farm/commands/reset/reorient.ts @@ -322,11 +322,12 @@ function buildLongForm(options: AssembleOptions): string { // 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) { + // Identified by a POSITIVE fact rather than by catching that error, and + // rather than by `taskText && !porch`: `initPorchInWorktree` is non-fatal, so + // a `--task --protocol X` builder whose porch init FAILED also has task text + // and no porch. `isBareTask` is established from the prompt file itself (see + // context.ts), which carries the rendered template for that lane regardless. + if (c.isBareTask) { const longFormTask = [ 'You are a Builder. Read codev/roles/builder.md for your full role definition.', '',