From efa0b8c79f7ec64cc49024c0984008c8aa60f420 Mon Sep 17 00:00:00 2001 From: pseudo Date: Mon, 24 Aug 2026 08:49:17 -0600 Subject: [PATCH] Do not attach a guessed plan or spec as review context (#65) The original report blamed spawn. The uiv2 architect corrected it by observation: `afx spawn 83` resolved codev/specs/83-v2-client-shell.md correctly with 0083-protocol-agnostic-spawn.md sitting right beside it, so the spawn-side lookup (exact-match only since #69) is fine. The consult lane is not. codev/plans/ held 0083-protocol-agnostic-spawn.md and no 83-*, so eleven consecutive `consult --type spec --issue 83` rounds each attached a stale January draft about a different subject as "the plan", and every reviewer spent part of its answer saying the plan looked unrelated. artifactHeading already warns on an inexact match, and that warning WORKED -- the reviewers all flagged it. It just does not help. The round is spent either way. So the rule turns on who asked for the document. The PRIMARY artifact, named by --type, keeps the lenient zero-stripped fallback and its warning: you asked for it by id, refusing would block the review outright, and genuinely zero-padded legacy projects must still resolve. The SECONDARY one -- the plan attached to a spec review, the spec attached to a plan review, both on an impl review -- is offered because it usually helps, and a guess usually does not. Omitting it costs nothing; attaching it costs a review. Dropping it says so on stderr. A plan that is silently absent is the same defect facing the other way: the operator cannot tell "no plan exists" from "we declined one". Not a one-file collision. 82 project ids in this repo have only a zero-padded plan, so renumbering 0083 would fix project 83 and leave 81 others. Issue #6 in this fork -- an open, unrelated CI bug -- currently resolves to 0006-tutorial-mode, and now drops instead. The reported id no longer reproduces: the project-83 run created codev/plans/83-v2-client-shell.md, so 83 resolves exactly. The tests are written against the rule rather than against that id. Co-Authored-By: Claude Opus 5 (1M context) --- .../issue-65-guessed-secondary.test.ts | 183 ++++++++++++++++++ packages/codev/src/commands/consult/index.ts | 62 +++++- 2 files changed, 235 insertions(+), 10 deletions(-) create mode 100644 packages/codev/src/commands/consult/__tests__/issue-65-guessed-secondary.test.ts diff --git a/packages/codev/src/commands/consult/__tests__/issue-65-guessed-secondary.test.ts b/packages/codev/src/commands/consult/__tests__/issue-65-guessed-secondary.test.ts new file mode 100644 index 000000000..b3cc274b2 --- /dev/null +++ b/packages/codev/src/commands/consult/__tests__/issue-65-guessed-secondary.test.ts @@ -0,0 +1,183 @@ +/** + * Issue #65 — a guessed secondary artifact burns the review round it is attached to. + * + * The original report was that `afx spawn` hands builders an unrelated old spec + * when the issue number collides with a zero-padded filename. That half is fixed + * (#69: the spawn-side lookup is exact-match only), and the uiv2 architect + * confirmed it by observation — `afx spawn 83` resolved + * `codev/specs/83-v2-client-shell.md` correctly with + * `0083-protocol-agnostic-spawn.md` sitting right beside it. + * + * What was still broken is narrower and was mis-attributed to spawn: the consult + * lane. `codev/plans/` held `0083-protocol-agnostic-spawn.md` and no `83-*`, so + * eleven consecutive `consult --type spec --issue 83` rounds each attached a + * stale January draft about a different subject as "the plan", and every + * reviewer spent part of its answer saying the plan looked unrelated. + * + * `artifactHeading` already warns on an inexact match, and that warning WORKED — + * the reviewers all flagged it. It just does not help. The round is spent either + * way. + * + * So the rule turns on who asked for the document. The PRIMARY artifact (the one + * named by `--type`) keeps the lenient fallback and its warning: you asked for it + * by id, refusing would block the review, and genuinely zero-padded legacy + * projects must still resolve. The SECONDARY one — the plan attached to a spec + * review, the spec attached to a plan review — is offered because it usually + * helps, and a guess usually does not. Omitting it costs nothing. + * + * Not a one-file collision: 82 project ids in this repo have only a zero-padded + * plan, so renumbering the colliding artifact would fix #83 and leave 81 others. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { getResolver, matchesProjectIdExact } from '../../porch/artifacts.js'; +import { dropIfGuessed } from '../index.js'; + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..', '..', '..'); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('#65: the collision is real and is not one file', () => { + it('this repo still has both spellings sitting side by side', () => { + // If someone renumbers these, the test below stops proving anything, so say + // so here rather than letting it silently pass. + const specs = fs.readdirSync(path.join(REPO_ROOT, 'codev', 'specs')); + + expect(specs).toContain('83-v2-client-shell.md'); + expect(specs).toContain('0083-protocol-agnostic-spawn.md'); + }); + + it('dozens of ids have ONLY a zero-padded plan, which is what makes this general', () => { + const plans = fs.readdirSync(path.join(REPO_ROOT, 'codev', 'plans')); + const padded = new Set(); + const unpadded = new Set(); + for (const f of plans) { + const p = /^0+(\d+)-/.exec(f); + if (p) padded.add(p[1]); + const u = /^([1-9]\d*)-/.exec(f); + if (u) unpadded.add(u[1]); + } + const onlyPadded = [...padded].filter(id => !unpadded.has(id)); + + expect(onlyPadded.length).toBeGreaterThan(50); + }); +}); + +describe('#65: exactness is what separates the two cases', () => { + it('treats an unpadded name as project N', () => { + expect(matchesProjectIdExact('83-v2-client-shell', '83')).toBe(true); + }); + + it('does NOT treat a zero-padded name as project N', () => { + // The whole distinction. `0083-...` is a different document that collides on + // the number, not project 83 wearing leading zeros. + expect(matchesProjectIdExact('0083-protocol-agnostic-spawn', '83')).toBe(false); + }); + + it('still treats a zero-padded name as its own literal id', () => { + // A genuinely zero-padded legacy project asked for by its padded id resolves + // exactly, so the lenient fallback is not the only thing keeping it working. + expect(matchesProjectIdExact('0083-protocol-agnostic-spawn', '0083')).toBe(true); + }); +}); + +describe('#65: the resolver still hands back the guess, which is why the caller must decide', () => { + it('returns a zero-stripped plan for an id with no exact plan', () => { + // Not a bug in the resolver: the lenient fallback exists so genuinely + // zero-padded legacy projects resolve. It is the CALLER that must not attach + // this to a review as if it were context. + const r = getResolver(REPO_ROOT); + const plans = fs.readdirSync(path.join(REPO_ROOT, 'codev', 'plans')); + const padded = plans.find(f => /^0+\d+-/.test(f)); + const id = String(Number(/^0+(\d+)-/.exec(padded!)![1])); + + const resolved = r.findPlanBaseName(id, ''); + + // Either it resolved exactly (someone added a canonical plan since) or it + // guessed — and if it guessed, the guess is the padded sibling. + if (resolved && !matchesProjectIdExact(resolved, id)) { + expect(resolved).toMatch(/^0+\d+-/); + } + }); + + it('resolves 83 exactly now that the canonical plan exists', () => { + // The project-83 run created codev/plans/83-v2-client-shell.md, so the + // reported symptom no longer reproduces on THIS id. That is why the fix is + // tested against the rule rather than against project 83. + const r = getResolver(REPO_ROOT); + + expect(r.findPlanBaseName('83', '')).toBe('83-v2-client-shell'); + expect(r.findSpecBaseName('83', '')).toBe('83-v2-client-shell'); + }); +}); + +describe('#65: dropIfGuessed — the rule itself', () => { + const ref = (label: string) => ({ content: '# doc', label, requestedId: '83' }); + + it('keeps an exactly-matched secondary artifact', () => { + const kept = dropIfGuessed(ref('83-v2-client-shell'), 'plan', '83'); + + expect(kept?.label).toBe('83-v2-client-shell'); + }); + + it('drops the zero-stripped guess that cost eleven review rounds', () => { + const dropped = dropIfGuessed(ref('0083-protocol-agnostic-spawn'), 'plan', '83'); + + expect(dropped).toBeNull(); + }); + + it('says on stderr why it dropped it, so the omission is not itself silent', () => { + // A missing plan with no explanation is the same defect facing the other + // way: the operator cannot tell "no plan exists" from "we declined one". + const lines: string[] = []; + vi.spyOn(console, 'error').mockImplementation((...a: unknown[]) => { lines.push(a.join(' ')); }); + + dropIfGuessed(ref('0083-protocol-agnostic-spawn'), 'plan', '83'); + + const said = lines.join('\n'); + expect(said).toContain('0083-protocol-agnostic-spawn'); + expect(said).toContain('zero-stripping'); + expect(said).toContain('--plan-file'); + }); + + it('passes null through — a genuinely absent artifact is not a dropped one', () => { + const lines: string[] = []; + vi.spyOn(console, 'error').mockImplementation((...a: unknown[]) => { lines.push(a.join(' ')); }); + + expect(dropIfGuessed(null, 'plan', '83')).toBeNull(); + expect(lines).toEqual([]); + }); + + it('leaves a non-numeric label alone rather than guessing about it', () => { + // The bare-id fallback and prefix-style ids are not id-prefixed artifact + // names; exactness says nothing useful about them. + const kept = dropIfGuessed(ref('some-unnumbered-doc'), 'plan', '83'); + + expect(kept?.label).toBe('some-unnumbered-doc'); + }); + + it('names the right kind in the message', () => { + const lines: string[] = []; + vi.spyOn(console, 'error').mockImplementation((...a: unknown[]) => { lines.push(a.join(' ')); }); + + dropIfGuessed(ref('0083-protocol-agnostic-spawn'), 'spec', '83'); + + expect(lines.join('\n')).toContain('--spec-file'); + }); + + it('keeps a padded artifact when the padded id is what was asked for', () => { + // A genuinely zero-padded legacy project consulting on its own id must not + // lose its own plan to this rule. + const kept = dropIfGuessed( + { content: '# doc', label: '0083-protocol-agnostic-spawn', requestedId: '0083' }, + 'plan', + '0083', + ); + + expect(kept?.label).toBe('0083-protocol-agnostic-spawn'); + }); +}); diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 5a7489dd4..84f2abcb4 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -328,6 +328,48 @@ function loadDotenv(workspaceRoot: string): void { } } +/** + * Drop a SECONDARY artifact that was only guessed at (#65). + * + * A spec review attaches the plan for context, and a plan review attaches the + * spec. Nobody asked for that second document — it is offered because it usually + * helps. When the id resolves only through the zero-stripping fallback, it + * usually does not: `codev/plans/` held `0083-protocol-agnostic-spawn.md` and no + * `83-*`, so eleven consecutive `consult --type spec --issue 83` rounds each + * attached a stale January draft about a different subject, and every reviewer + * spent part of its answer saying the plan looked unrelated. + * + * `artifactHeading` already warns on an inexact match, and that warning WORKED — + * the reviewers all flagged it. It just does not help: the round is spent either + * way. For a document nobody requested, omitting a guess costs nothing and + * attaching one costs a review. + * + * The PRIMARY artifact keeps the lenient fallback plus its warning. There you + * asked for it by id, refusing would block the review outright, and a genuinely + * zero-padded legacy project must still resolve. + * + * 82 project ids in this repo have only a zero-padded plan, so this is not a + * one-file collision — renumbering the colliding artifact would fix #83 alone. + */ +export function dropIfGuessed( + ref: ContentRef | null, + kind: 'plan' | 'spec', + id: string, +): ContentRef | null { + if (!ref) return null; + if (matchesProjectIdExact(ref.label, id)) return ref; + // Nothing numeric to compare means the label is not an id-prefixed artifact + // name (e.g. the bare id fallback); leave those alone. + if (!/^\d/.test(ref.label)) return ref; + + console.error( + `Not attaching a ${kind} for context: '${ref.label}' matched ${id} only by zero-stripping, ` + + `so it is probably a different project's ${kind} that collides on the number (#65). ` + + `Pass --${kind}-file to attach one explicitly.`, + ); + return null; +} + /** * Find spec content by project ID using the artifact resolver. * Returns a ContentRef with content and label, or null if not found. @@ -2586,7 +2628,7 @@ function resolveBuilderQuery(workspaceRoot: string, type: string, options: Consu case 'spec': { const spec = findSpecContent(workspaceRoot, projectId); if (!spec) throw new Error(`Spec ${projectId} not found`); - const plan = findPlanContent(workspaceRoot, projectId); + const plan = dropIfGuessed(findPlanContent(workspaceRoot, projectId), 'plan', projectId); console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); return buildSpecQuery(spec, plan); @@ -2595,15 +2637,15 @@ function resolveBuilderQuery(workspaceRoot: string, type: string, options: Consu case 'plan': { const plan = findPlanContent(workspaceRoot, projectId); if (!plan) throw new Error(`Plan ${projectId} not found`); - const spec = findSpecContent(workspaceRoot, projectId); + const spec = dropIfGuessed(findSpecContent(workspaceRoot, projectId), 'spec', projectId); console.error(`Plan: ${plan.label}`); if (spec) console.error(`Spec: ${spec.label}`); return buildPlanQuery(plan, spec); } case 'impl': { - const spec = findSpecContent(workspaceRoot, projectId); - const plan = findPlanContent(workspaceRoot, projectId); + const spec = dropIfGuessed(findSpecContent(workspaceRoot, projectId), 'spec', projectId); + const plan = dropIfGuessed(findPlanContent(workspaceRoot, projectId), 'plan', projectId); console.error(`Project: ${projectId}`); if (spec) console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); @@ -2622,8 +2664,8 @@ function resolveBuilderQuery(workspaceRoot: string, type: string, options: Consu if (!currentPhase) { throw new Error('No current plan phase detected. Use --plan-phase to specify.'); } - const spec = findSpecContent(workspaceRoot, projectId); - const plan = findPlanContent(workspaceRoot, projectId); + const spec = dropIfGuessed(findSpecContent(workspaceRoot, projectId), 'spec', projectId); + const plan = dropIfGuessed(findPlanContent(workspaceRoot, projectId), 'plan', projectId); console.error(`Phase: ${currentPhase}`); if (spec) console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); @@ -2713,7 +2755,7 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con const { resolver, sourceLabel } = resolveArtifactSource(workspaceRoot, issueId, options.branch); const spec = findSpecContent(workspaceRoot, issueId, resolver); if (!spec) throw new Error(`Spec ${issueId} not found at ${sourceLabel}`); - const plan = findPlanContent(workspaceRoot, issueId, resolver); + const plan = dropIfGuessed(findPlanContent(workspaceRoot, issueId, resolver), 'plan', issueId); console.error(`Source: ${sourceLabel}`); console.error(`Spec: ${spec.label}`); if (plan) console.error(`Plan: ${plan.label}`); @@ -2724,7 +2766,7 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con const { resolver, sourceLabel } = resolveArtifactSource(workspaceRoot, issueId, options.branch); const plan = findPlanContent(workspaceRoot, issueId, resolver); if (!plan) throw new Error(`Plan ${issueId} not found at ${sourceLabel}`); - const spec = findSpecContent(workspaceRoot, issueId, resolver); + const spec = dropIfGuessed(findSpecContent(workspaceRoot, issueId, resolver), 'spec', issueId); console.error(`Source: ${sourceLabel}`); console.error(`Plan: ${plan.label}`); if (spec) console.error(`Spec: ${spec.label}`); @@ -2794,8 +2836,8 @@ function resolveArchitectQuery(workspaceRoot: string, type: string, options: Con // change diff scope, only artifact source — cmap-3 finding). const ref = options.branch ?? `origin/${pr.headRefName}`; const resolver = new GitRefResolver(workspaceRoot, ref); - const spec = findSpecContent(workspaceRoot, issueId, resolver); - const plan = findPlanContent(workspaceRoot, issueId, resolver); + const spec = dropIfGuessed(findSpecContent(workspaceRoot, issueId, resolver), 'spec', issueId); + const plan = dropIfGuessed(findPlanContent(workspaceRoot, issueId, resolver), 'plan', issueId); console.error(`Project: ${issueId} (PR #${pr.number}, ${pr.headRefName} → ${pr.baseRefName})`); console.error(`Source: ${ref}`); if (spec) console.error(`Spec: ${spec.label}`);