From d05097b157d830257425b9d86eec3f0a1feefcf3 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 29 Sep 2026 15:04:28 +0000 Subject: [PATCH] test: structural Design count boundary; TODO proposals are not findings Replaying run 36385945043 through the Design count predicates: routing, focus and learnings setup was not recognized as setup, Issue 1 was counted pre-review in both attempts (the boundary fired on it), and attempt 2 counted the Font TODO proposal as a finding (review=4 and review=5 for five issues). The paid caller now starts review at the first answered native decision that is not setup (recognized packet, or setup header/question ID), a completion handoff, artifact rendering or a TODO proposal (the review's Add to TODOS.md / Skip / Build it now menu). TODO proposals are recorded as administrative extra decisions. The replay asserts each counted call: both attempts review=5 (Issues 1-5). isDesignCountFirstReview and its controls are unchanged. --- test/design-count-structural-boundary.test.ts | 83 +++++++++++++++++++ test/helpers/design-count-review.ts | 40 ++++++++- ...kill-e2e-plan-design-finding-count.test.ts | 10 ++- 3 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 test/design-count-structural-boundary.test.ts diff --git a/test/design-count-structural-boundary.test.ts b/test/design-count-structural-boundary.test.ts new file mode 100644 index 000000000..e1d37b04f --- /dev/null +++ b/test/design-count-structural-boundary.test.ts @@ -0,0 +1,83 @@ +/** Free replay of run 36385945043's Design count attempts through the structural boundary; no model calls. */ +import { expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import captured from './fixtures/design-completion-36385945043.json'; +import { designStep0Boundary, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +import { isDesignCompletionHandoff, isDesignCountReviewStart, isDesignCountStructuralSetup, isDesignTodoProposal } from './helpers/design-count-review'; +import { isDesignArtifactGeneration } from './helpers/design-artifact-question'; + +const replay = (calls: NativePlanQuestionCall[]) => { + let started = false; + return calls.map(call => { + const fp = nativePlanCallFingerprint(structuredClone(call), 0, !started); + const phase = planCountQuestionPhase(fp, started, designStep0Boundary, isDesignCountReviewStart, + isDesignCountStructuralSetup, isDesignCompletionHandoff, isDesignArtifactGeneration, isDesignTodoProposal); + started = phase.reviewStarted; + return { header: call.questions[0]!.header, kind: phase.administrative ?? (phase.preReview ? 'setup' : 'review') }; + }); +}; +const calls = (i: number) => captured.attempts[i]!.transcript.calls as NativePlanQuestionCall[]; + +test('the paid Design caller uses the structural boundary replayed here', () => { + const caller = fs.readFileSync(path.join(import.meta.dir, 'skill-e2e-plan-design-finding-count.test.ts'), 'utf8'); + for (const wiring of ['isFirstReviewAUQ: isDesignCountReviewStart,', 'isSetupAUQ: isDesignCountStructuralSetup,', + 'isArtifactGenerationAUQ: isDesignArtifactGeneration,', 'isTodoProposalAUQ: isDesignTodoProposal,']) + expect(caller).toContain(wiring); +}); + +test('captured attempts record the old miscount: Issue 1 pre-review, and a TODO proposal counted as a finding', () => { + expect(captured.attempts.map(a => a.observed.reviewCount)).toEqual([4, 5]); + expect(captured.attempts[0]!.observed.preReview.find(row => row.header === 'Issue 1')!.preReview).toBe(true); + expect(captured.attempts[1]!.observed.preReview.find(row => row.header === 'Primary CTA')!.preReview).toBe(true); + expect(captured.attempts[1]!.observed.preReview.find(row => row.header === 'Font TODO')!.preReview).toBe(false); +}); + +test('attempt 1: setup, then Issues 1-5 each count as one finding', () => { + expect(replay(calls(0))).toEqual([ + { header: 'Routing', kind: 'setup' }, { header: 'Focus', kind: 'setup' }, { header: 'Learnings', kind: 'setup' }, + { header: 'Issue 1', kind: 'review' }, { header: 'Issue 2', kind: 'review' }, { header: 'Issue 3', kind: 'review' }, + { header: 'Issue 4', kind: 'review' }, { header: 'Issue 5', kind: 'review' }, + ]); +}); + +test('attempt 2: setup, Issues 1-5 count, and the TODO proposal is an extra decision, not a finding', () => { + const rows = replay(calls(1)); + expect(rows).toEqual([ + { header: 'Routing', kind: 'setup' }, { header: 'Learnings', kind: 'setup' }, + { header: 'Primary CTA', kind: 'review' }, { header: 'Save pending', kind: 'review' }, { header: 'Type roles', kind: 'review' }, + { header: 'Spacing', kind: 'review' }, { header: 'Error color', kind: 'review' }, { header: 'Font TODO', kind: 'todo-proposal' }, + ]); + expect(rows.filter(row => row.kind === 'review')).toHaveLength(5); + const issues = calls(1).filter((_, i) => rows[i]!.kind === 'review').map(call => /Issue ([1-5])/.exec(call.questions[0]!.question)![1]); + expect(issues).toEqual(['1', '2', '3', '4', '5']); +}); + +test('setup stays setup after review starts and a TODO proposal never starts review', () => { + const [routing, , first] = calls(1), todo = calls(1).at(-1)!; + expect(replay([todo, first!])).toEqual([{ header: 'Font TODO', kind: 'todo-proposal' }, { header: 'Primary CTA', kind: 'review' }]); + expect(replay([first!, routing!])).toEqual([{ header: 'Primary CTA', kind: 'review' }, { header: 'Routing', kind: 'setup' }]); +}); + +test('an unanswered, failed or foreign call cannot start review', () => { + const first = calls(1)[2]!; + const unanswered = { ...structuredClone(first), answered: false, answers: {} }; + const failed = { ...structuredClone(first), answered: false, failed: true }; + for (const call of [unanswered, failed]) expect(isDesignCountReviewStart(nativePlanCallFingerprint(call, 0, true))).toBe(false); + expect(isDesignCountReviewStart({ ...nativePlanCallFingerprint(structuredClone(first), 0, true), signature: 'foreign:call' })).toBe(false); +}); + +test('only the contracted Add to TODOS.md / Skip / Build it now menu is a TODO proposal', () => { + const todo = calls(1).at(-1)!; + expect(isDesignTodoProposal(nativePlanCallFingerprint(structuredClone(todo), 0, false))).toBe(true); + for (const labels of [['6A) Add to plan as a task', '6B) Skip', '6C) Build it now'], ['6A) Add to TODOS.md', '6B) Skip'], + ['6A) Add to TODOS.md', '6B) Build it now', '6C) Skip'], ['6A) Add to TODOS.md', '6B) Skip', '6C) Build it now', '6D) Other']]) { + const call = structuredClone(todo), q = call.questions[0]!; + q.options = labels.map(label => ({ label, description: 'x' })); + call.answers = { [q.question]: labels[0]! }; + const phase = planCountQuestionPhase(nativePlanCallFingerprint(call, 0, false), true, designStep0Boundary, isDesignCountReviewStart, + isDesignCountStructuralSetup, isDesignCompletionHandoff, isDesignArtifactGeneration, isDesignTodoProposal); + expect(phase.administrative, labels.join(' / ')).toBeUndefined(); + } +}); diff --git a/test/helpers/design-count-review.ts b/test/helpers/design-count-review.ts index b14721ad0..4e525e4bd 100644 --- a/test/helpers/design-count-review.ts +++ b/test/helpers/design-count-review.ts @@ -2,6 +2,11 @@ import { designFirstReviewAUQ, designReviewSetupAUQ } from './claude-pty-runner' import type { AskUserQuestionFingerprint } from './claude-pty-runner'; import { pickDesignCountOutsideVoices } from './design-count-outside'; +// Native header / question-ID vocabulary this review assigns only to setup and navigation. +const DESIGN_SETUP_HEADER = /^(?:focus|scope|learnings|routing|next steps?|outside(?: design)? voices)$/i; +const DESIGN_SETUP_ID = /(?:^|-)(?:focus|scope|setup|routing|learnings|onboarding|next-steps?|posture|mockups?|target)(?:-|$)/i; +const questionId = (question: string) => //i.exec(question)?.[1] ?? ''; + /** Choosing reviewer participation is setup, even when numbered or asked late. */ export function isDesignCountSetup(fp: AskUserQuestionFingerprint): boolean { if (designReviewSetupAUQ(fp)) return true; @@ -767,9 +772,9 @@ export function isDesignCountFirstReview(fp: AskUserQuestionFingerprint): boolea if (designFirstReviewAUQ(fp)) return true; return call.questions.some(q => { if (!call.answers?.[q.question] || q.options.length < 2) return false; - if (/^(?:focus|scope|learnings|routing|next steps?|outside(?: design)? voices)$/i.test(q.header.trim())) return false; - const id = //i.exec(q.question)?.[1] ?? ''; - if (/(?:^|-)(?:focus|scope|setup|routing|learnings|onboarding|next-steps?|posture|mockups?|target)(?:-|$)/i.test(id)) return false; + if (DESIGN_SETUP_HEADER.test(q.header.trim())) return false; + const id = questionId(q.question); + if (DESIGN_SETUP_ID.test(id)) return false; // Native fingerprints prepend the menu header. Inspect the actual question // for an explicit finding that offers a plan amendment and deferral. if (call.answered === true && call.failed === false && /^Pass\s*[1-7]\s*\([^)]*\)\s*[—–:]\s*Finding\s*[1-9]\d*:\s+\S/i.test(q.question.trim()) && @@ -814,6 +819,35 @@ export function isDesignCountFirstReview(fp: AskUserQuestionFingerprint): boolea }); } +/** Setup by structure: the recognized setup packet, or a native call whose every + * question carries a setup header or setup question ID. */ +export function isDesignCountStructuralSetup(fp: AskUserQuestionFingerprint): boolean { + if (isDesignCountSetup(fp)) return true; + const call = fp.nativeCall; + return !!call && call.questions.length > 0 && call.questions.every(q => + DESIGN_SETUP_HEADER.test(q.header.trim()) || DESIGN_SETUP_ID.test(questionId(q.question))); +} + +/** The review's TODO contract offers exactly A) Add to TODOS.md, B) Skip, C) Build it now. */ +export function isDesignTodoProposal(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (!call?.answered || call.failed || call.questions.length !== 1) return false; + const labels = call.questions[0]!.options.map(option => option.label.trim() + .replace(/^\d*[A-C][).:]?\s+/, '').replace(/\s*\(recommended\)\s*$/i, '')); + return labels.length === 3 && /^Add to TODOS\.md\b/i.test(labels[0]!) && /^Skip\b/i.test(labels[1]!) && + /^Build it now\b/i.test(labels[2]!); +} + +/** After setup, the first answered native decision that is not setup, a TODO + * proposal, a completion handoff or artifact rendering starts review. Handoff and + * artifact calls are classified before this predicate runs. */ +export function isDesignCountReviewStart(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + return !!call && call.answered === true && call.failed === false && fp.signature === `${call.sessionId}:${call.toolUseId}` && + call.questions.length > 0 && call.questions.every(q => Boolean(call.answers?.[q.question])) && + !isDesignCountStructuralSetup(fp) && !isDesignTodoProposal(fp); +} + /** A closed recap may explain why Eng is next; it cannot request another fix. */ function closedDesignGateRecap(tail: string, descriptions: string[]): boolean { const navigation = /\bWhat(?:['’]s)?\s+next\?\s*\s*$/i.exec(tail); diff --git a/test/skill-e2e-plan-design-finding-count.test.ts b/test/skill-e2e-plan-design-finding-count.test.ts index e3003ee53..5befaa0ec 100644 --- a/test/skill-e2e-plan-design-finding-count.test.ts +++ b/test/skill-e2e-plan-design-finding-count.test.ts @@ -10,7 +10,7 @@ import { test } from 'bun:test'; import { describeE2ETier } from './helpers/e2e-gate'; -import { isDesignCountFirstReview, isDesignCountSetup, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review'; +import { isDesignCountReviewStart, isDesignCountStructuralSetup, isDesignTodoProposal, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review'; import { isDesignArtifactGeneration } from './helpers/design-artifact-question'; import { designCountExistingInteractionStates as existingInteractionStates } from './helpers/design-count-fixture'; import * as fs from 'node:fs'; @@ -215,10 +215,14 @@ describeE2E('/plan-design-review per-finding AskUserQuestion count (periodic)', followUpPrompt: planDesign5Findings(planPath), expectedPlanPath: planPath, isLastStep0AUQ: designStep0Boundary, - isFirstReviewAUQ: isDesignCountFirstReview, - isSetupAUQ: isDesignCountSetup, + // Structural boundary: after setup, the first answered native decision + // that is not setup, a handoff, artifact rendering or a TODO proposal + // starts review. TODO proposals are extra decisions, never findings. + isFirstReviewAUQ: isDesignCountReviewStart, + isSetupAUQ: isDesignCountStructuralSetup, isCompletionHandoffAUQ: isDesignCompletionHandoff, isArtifactGenerationAUQ: isDesignArtifactGeneration, + isTodoProposalAUQ: isDesignTodoProposal, fixtureFiles: { 'DESIGN.md': designSystem }, // Design's explicit opt-in is separate from codex_reviews. Keep // this native-cadence fixture within its declared review scope.