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}`);