From 585c7ebcb5891852b960f7b3c60f73b12cbd9e7d Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 8 Sep 2026 18:28:53 +0000 Subject: [PATCH] test: stabilize review evidence and concurrent refresh fixtures --- ios-qa/daemon/test/daemon-integration.test.ts | 78 +++-- test/ceo-completion-handoff.test.ts | 129 ++++++++ .../ceo-completion-handoff-calls.json | 248 +++++++++++++++ test/helpers/ceo-completion-handoff.ts | 42 +++ test/helpers/claude-pty-runner.ts | 190 ++++++++++-- test/helpers/claude-pty-runner.unit.test.ts | 284 +++++++++++++++++- test/helpers/touchfiles-data.ts | 4 +- test/plan-count-completion.test.ts | 241 +++++++++++++++ test/skill-e2e-plan-ceo-finding-count.test.ts | 9 +- ...skill-e2e-plan-devex-finding-count.test.ts | 1 + test/skill-e2e-plan-eng-finding-count.test.ts | 4 + ...2e-plan-eng-multi-finding-batching.test.ts | 4 + 12 files changed, 1169 insertions(+), 65 deletions(-) create mode 100644 test/ceo-completion-handoff.test.ts create mode 100644 test/fixtures/ceo-completion-handoff-calls.json create mode 100644 test/helpers/ceo-completion-handoff.ts create mode 100644 test/plan-count-completion.test.ts diff --git a/ios-qa/daemon/test/daemon-integration.test.ts b/ios-qa/daemon/test/daemon-integration.test.ts index 8e273d039..58ee1f566 100644 --- a/ios-qa/daemon/test/daemon-integration.test.ts +++ b/ios-qa/daemon/test/daemon-integration.test.ts @@ -24,7 +24,7 @@ const STATE_SERVER_TOKEN = 'rotated-mock-token-XXXXXXXX'; // Stub iOS StateServer running on loopback. Mimics the real Swift server's // behavior for the integration test. -function startStubStateServer(): Promise<{ server: Server; port: number; receivedRequests: Array<{ method: string; path: string; headers: Record; body: string }> }> { +function startStubStateServer(opts: { onUnauthorized?: (reply: () => void) => void } = {}): Promise<{ server: Server; port: number; receivedRequests: Array<{ method: string; path: string; headers: Record; body: string }> }> { return new Promise((resolve) => { const received: Array<{ method: string; path: string; headers: Record; body: string }> = []; const server = createServer((req, res) => { @@ -37,8 +37,12 @@ function startStubStateServer(): Promise<{ server: Server; port: number; receive const auth = req.headers['authorization']; // Validate the bearer is our rotated token. if (!auth || auth !== `Bearer ${STATE_SERVER_TOKEN}`) { - res.writeHead(401, { 'content-type': 'application/json' }); - res.end(JSON.stringify({ error: 'unauthorized' })); + const reply = () => { + res.writeHead(401, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ error: 'unauthorized' })); + }; + if (opts.onUnauthorized) opts.onUnauthorized(reply); + else reply(); return; } @@ -296,49 +300,69 @@ describe('daemon — loopback listener', () => { let releaseRefresh!: () => void; const refreshStarted = new Promise((resolve) => { markRefreshStarted = resolve; }); const refreshGate = new Promise((resolve) => { releaseRefresh = resolve; }); + // Hold the actual 401 responses until all three stale attempts arrive. + // Otherwise a later request can correctly join the refresh without ever + // using the expired token, so elapsed time cannot establish concurrency. + let markFirstStale!: () => void; + let markAllStale!: () => void; + const firstStale = new Promise((resolve) => { markFirstStale = resolve; }); + const allStale = new Promise((resolve) => { markAllStale = resolve; }); + const staleReplies: Array<() => void> = []; + const releaseStale = () => { for (const reply of staleReplies.splice(0)) reply(); }; + const relaunchStub = await startStubStateServer({ + onUnauthorized: (reply) => { + staleReplies.push(reply); + if (staleReplies.length === 1) markFirstStale(); + if (staleReplies.length === 3) markAllStale(); + }, + }); const staleTunnel: DeviceTunnel = { udid: 'STUB-UDID', ipv6Addr: '127.0.0.1', - port: stub.port, + port: relaunchStub.port, bootTokenRotated: 'expired-after-relaunch', }; const refreshedTunnel: DeviceTunnel = { ...staleTunnel, bootTokenRotated: STATE_SERVER_TOKEN, }; - const d = await startDaemon({ - loopbackPort: 0, - tailnetEnabled: false, - pidfilePath: join(workDir, 'daemon-relaunch-refresh.pid'), - tunnelProvider: async () => { - bootstraps++; - if (bootstraps === 1) return staleTunnel; - if (bootstraps === 2) { - markRefreshStarted(); - await refreshGate; - return refreshedTunnel; - } - throw new Error('concurrent 401s caused duplicate bootstraps'); - }, - }); - if ('error' in d) throw new Error(d.error); - - const requestStart = stub.receivedRequests.length; + let d: RunningDaemon | undefined; try { + const started = await startDaemon({ + loopbackPort: 0, + tailnetEnabled: false, + pidfilePath: join(workDir, 'daemon-relaunch-refresh.pid'), + tunnelProvider: async () => { + bootstraps++; + if (bootstraps === 1) return staleTunnel; + if (bootstraps === 2) { + markRefreshStarted(); + await refreshGate; + return refreshedTunnel; + } + throw new Error('concurrent 401s caused duplicate bootstraps'); + }, + }); + if ('error' in started) throw new Error(started.error); + d = started; const base = `http://127.0.0.1:${d.loopbackPort}`; + const first = fetchWith('GET', `${base}/screenshot`); + await firstStale; const requests = [ - fetchWith('GET', `${base}/screenshot`), + first, fetchWith('GET', `${base}/screenshot`), fetchWith('GET', `${base}/screenshot`), ]; + await allStale; + expect(bootstraps).toBe(1); + releaseStale(); await refreshStarted; - await new Promise((resolve) => setTimeout(resolve, 25)); expect(bootstraps).toBe(2); releaseRefresh(); const responses = await Promise.all(requests); expect(responses.map((response) => response.status)).toEqual([200, 200, 200]); - const attempts = stub.receivedRequests.slice(requestStart); + const attempts = relaunchStub.receivedRequests; expect(attempts.filter((request) => request.headers.authorization === 'Bearer expired-after-relaunch')).toHaveLength(3); expect(attempts.filter((request) => request.headers.authorization === `Bearer ${STATE_SERVER_TOKEN}`)).toHaveLength(3); @@ -346,8 +370,10 @@ describe('daemon — loopback listener', () => { expect(healthyReuse.status).toBe(200); expect(bootstraps).toBe(2); } finally { + releaseStale(); releaseRefresh(); - await d.close(); + try { await d?.close(); } + finally { await new Promise((resolve, reject) => relaunchStub.server.close((error) => error ? reject(error) : resolve())); } } }); diff --git a/test/ceo-completion-handoff.test.ts b/test/ceo-completion-handoff.test.ts new file mode 100644 index 000000000..5ad6592b4 --- /dev/null +++ b/test/ceo-completion-handoff.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, test } from 'bun:test'; +import { capturePlanCountQuestion, ceoStep0Boundary, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +import captures from './fixtures/ceo-completion-handoff-calls.json'; + +type CapturedCall = typeof captures.cases[number]['calls'][number]; +function nativeCall(record: CapturedCall, sessionId = 'native-capture'): NativePlanQuestionCall { + return { + sessionId, toolUseId: record.toolUseId, answered: true, failed: false, + questions: [{ header: record.header, question: record.question, + options: record.options.map(label => ({ label })), multiSelect: false }], + answers: { [record.question]: record.answer }, unansweredQuestionIndices: [], + }; +} +const handoff = () => nativeCall(captures.cases[0]!.calls.at(-1)!); +const fingerprint = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, false); + +function replay(calls: NativePlanQuestionCall[], reviewStarted = true) { + const counts = { step0Count: 0, reviewCount: 0, administrativeCount: 0 }; + const classifications = []; + for (const call of calls) { + const fp = fingerprint(call); + const phase = planCountQuestionPhase(fp, reviewStarted, ceoStep0Boundary, + // A completion summary can mention defects; even a broad positive + // first-finding predicate must not promote a handoff into coverage. + () => true, undefined, isCeoCompletionHandoff); + if (phase.administrative) counts.administrativeCount++; + else if (phase.preReview) counts.step0Count++; + else counts.reviewCount++; + reviewStarted = phase.reviewStarted; + classifications.push(phase); + } + return { ...counts, reviewStarted, classifications }; +} + +describe('CEO completion handoff classification and selection', () => { + test('captured first attempts keep every finding/TODO and exclude only the handoff; substantive retry still fails its band', () => { + for (const scenario of captures.cases) { + const calls = scenario.calls.map(c => nativeCall(c, scenario.sessionId)); + const original = structuredClone(calls); + const result = replay(calls); + expect(result.reviewCount).toBe(scenario.expectedReviewCount); + expect(result.administrativeCount).toBe(scenario.name === 'five-retry' ? 0 : 1); + expect(result.step0Count).toBe(0); + expect(calls).toEqual(original); // Classification never discards or rewrites native evidence. + for (const [i, call] of calls.entries()) { + if (/TODO/i.test(call.questions[0]!.header)) expect(result.classifications[i]!.administrative).toBeUndefined(); + } + } + expect(replay(captures.cases[2]!.calls.map(c => nativeCall(c))).reviewCount).toBeGreaterThan(7); + }); + test('handoff-only replay adds no findings or setup and cannot establish a first finding', () => { + const result = replay([handoff()], false); + expect(result).toMatchObject({ step0Count: 0, reviewCount: 0, administrativeCount: 1, reviewStarted: false }); + expect(result.classifications[0]).toEqual({ preReview: false, reviewStarted: false, administrative: 'completion-handoff' }); + }); + test('manual/done action is selected in either option order only while the matching native question is pending', () => { + for (const reverse of [false, true]) { + const call = handoff(); call.answered = false; delete call.answers; delete call.unansweredQuestionIndices; + if (reverse) call.questions[0]!.options.reverse(); + const fp = fingerprint(call); + expect(pickCeoCompletionHandoff(fp)).toBe(reverse ? 1 : 2); + expect(isCeoCompletionHandoff(fp)).toBe(false); + } + expect(pickCeoCompletionHandoff(fingerprint(handoff()))).toBeNull(); + }); + test('substantive choices mentioning another review retain the normal choice and finding count', () => { + const call = nativeCall(captures.cases[0]!.calls[0]!); + call.questions[0]!.question += ' Run /plan-eng-review next after deciding how to fix this issue.'; + call.answers = { [call.questions[0]!.question]: call.questions[0]!.options[0]!.label }; + expect(isCeoCompletionHandoff(fingerprint(call))).toBe(false); + expect(replay([call]).reviewCount).toBe(1); + call.answered = false; + expect(pickCeoCompletionHandoff(fingerprint(call))).toBeNull(); + }); + test('mixed packets and unknown action choices are not classified as an administrative handoff', () => { + const mixed = handoff(); + const finding = nativeCall(captures.cases[0]!.calls[0]!); + mixed.questions.push(finding.questions[0]!); + mixed.answers = { ...mixed.answers, ...finding.answers }; + expect(isCeoCompletionHandoff(fingerprint(mixed))).toBe(false); + expect(replay([mixed]).reviewCount).toBe(1); + mixed.answered = false; + expect(pickCeoCompletionHandoff(fingerprint(mixed))).toBeNull(); + const unknown = handoff(); unknown.questions[0]!.options.push({ label: 'Add another payment test before continuing' }); + expect(isCeoCompletionHandoff(fingerprint(unknown))).toBe(false); + }); + test('unknown identities and generic skip choices remain counted', () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.question += ' '; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Test gap'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'Skip'; }, + ]) { + const call = handoff(); mutate(call); + call.answers = { [call.questions[0]!.question]: call.questions[0]!.options[0]!.label }; + expect(isCeoCompletionHandoff(fingerprint(call))).toBe(false); + expect(replay([call]).reviewCount).toBe(1); + } + const call = handoff(); call.answered = false; + const mismatched = { ...fingerprint(call), signature: 'another-native-call' }; + expect(pickCeoCompletionHandoff(mismatched)).toBeNull(); + }); + test('pending, failed, partial and free-form answers never create an exclusion', () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { c.answers = {}; }, + (c: NativePlanQuestionCall) => { c.answers = { [c.questions[0]!.question]: 'First add a refund test' }; }, + ]) { + const call = handoff(); mutate(call); + expect(isCeoCompletionHandoff(fingerprint(call))).toBe(false); + } + }); + test('UI-only and unrelated pending metadata cannot steer the active menu', () => { + const pending = handoff(); pending.answered = false; delete pending.answers; + const q = pending.questions[0]!; + const active = `☐ ${q.header}\n${q.question}\n❯ 1. ${q.options[0]!.label}\n 2. ${q.options[1]!.label}\nEnter to select · ↑/↓ to navigate · Esc to cancel`; + const bound = capturePlanCountQuestion(active, new Set(), 0, false, pending)!; + expect(pickCeoCompletionHandoff(fingerprint(pending), bound)).toBe(2); + const uiOnly = capturePlanCountQuestion(active, new Set(), 0, false)!; + expect(pickCeoCompletionHandoff(uiOnly)).toBeNull(); + const issue = '☐ Security finding\nChoose how to parameterize the SQL query.\n❯ 1. Fix query\n 2. Add a TODO\nEnter to select · ↑/↓ to navigate · Esc to cancel'; + const unbound = capturePlanCountQuestion(issue, new Set(), 0, false, pending)!; + expect(unbound.nativeCall).toBeUndefined(); + expect(pickCeoCompletionHandoff(fingerprint(pending), unbound)).toBeNull(); + }); +}); diff --git a/test/fixtures/ceo-completion-handoff-calls.json b/test/fixtures/ceo-completion-handoff-calls.json new file mode 100644 index 000000000..38f8fc056 --- /dev/null +++ b/test/fixtures/ceo-completion-handoff-calls.json @@ -0,0 +1,248 @@ +{ + "note": "Native F captures: exact question text, option labels and selected answers. The original complete observations remain in the run artifacts; descriptions are omitted from this focused classification replay. No expected counts are sent to a model.", + "cases": [ + { + "name": "five-first", + "sourceCapture": "plan-ceo-review-1788889172889-nqtlLV", + "sessionId": "6783d375-4023-4b03-bad1-09af36a4fd1d", + "originalReviewCount": 8, + "expectedReviewCount": 7, + "calls": [ + { + "toolUseId": "toolu_01NYTQ5FdABFQaRLZma3K2bo", + "header": "Arch: dispatcher", + "question": "D3 — Section 1 (Architecture): The plan bypasses the existing `WebhookDispatcher` module with no documented rationale beyond 'namespace separation.' This creates a parallel dispatch path that will diverge. What should the plan say about this?", + "options": [ + "A) Document the bypass inline (recommended)", + "B) Integrate with WebhookDispatcher instead", + "C) Accept the bypass as-is — no change needed" + ], + "answer": "A) Document the bypass inline (recommended)" + }, + { + "toolUseId": "toolu_01QX3MEMGL6J7ZPnAW4BVjZR", + "header": "Email error gap", + "question": "D4 — Section 2 (Error/Rescue): The email notification fires inline with no error handling. If email fails AFTER the DB update commits but BEFORE the dedup guard records completion, Stripe will retry the event. The user record gets updated twice — and still gets no email. How should the plan address this?", + "options": [ + "A) Add a try/catch around the email leg (recommended)", + "B) Move email to a background job", + "C) Defer to TODOS.md — accept the risk for now" + ], + "answer": "A) Add a try/catch around the email leg (recommended)" + }, + { + "toolUseId": "toolu_01Uf7CHkz7dkZmgb5bTrfKjq", + "header": "SQL injection", + "question": "D5 — Section 3 (Security) CRITICAL: The plan reads `request.params.userId` directly into a raw SQL fragment. This is a textbook SQL injection vulnerability. An attacker who can send a crafted webhook (or exploit a signature verification bypass) can exfiltrate or corrupt the entire database. This MUST be fixed before implementation. How?", + "options": [ + "A) Parameterized query — fix the plan (recommended)", + "B) Input validation before the query", + "C) Note it as a known issue" + ], + "answer": "A) Parameterized query — fix the plan (recommended)" + }, + { + "toolUseId": "toolu_01M76BDWgwpJrMZs1Mmh8J5S", + "header": "Tests", + "question": "D6 — Section 6 (Tests): The plan says 'None planned — we'll rely on the existing integration suite.' This is payment code. The existing suite has no tests for the new handler codepaths. What's the testing strategy?", + "options": [ + "A) Write focused tests for the critical path (recommended)", + "B) Accept the gap — rely on existing suite" + ], + "answer": "A) Write focused tests for the critical path (recommended)" + }, + { + "toolUseId": "toolu_019VwdQqcAMv5a4ygmSUFaZ6", + "header": "N+1 queries", + "question": "D7 — Section 7 (Performance): Each webhook lookup fetches orders in a loop (N+1 pattern): 1 query for user + 1 query per order. A user with 100 orders = 101 queries per webhook event. Fix now or defer?", + "options": [ + "A) Fix now with a batch query (recommended)", + "B) Defer to TODOS.md" + ], + "answer": "A) Fix now with a batch query (recommended)" + }, + { + "toolUseId": "toolu_01GpxKeqJsEc9sbC5hBafqSZ", + "header": "TODO: dispatcher", + "question": "TODOS.md — Item 1: Evaluate WebhookDispatcher integration. The plan documents why it’s bypassed but doesn’t rule out integration long-term. If the dispatcher grows new capabilities (rate limiting, circuit breaking, structured logging) the new handler will miss them. Add a future evaluation TODO?", + "options": [ + "A) Add to TODOS.md (recommended)", + "B) Skip — not valuable enough" + ], + "answer": "A) Add to TODOS.md (recommended)" + }, + { + "toolUseId": "toolu_01L5Q5bnnpCnWa6xHuPWgqCC", + "header": "TODO: async email", + "question": "TODOS.md — Item 2: Move email notification to a background job. The try/catch added in D4 prevents Stripe retry cascades, but email still runs synchronously in the webhook response window. If email becomes slow (slow SMTP, large templates), webhook response times suffer. Add an async email TODO?", + "options": [ + "A) Add to TODOS.md (recommended)", + "B) Skip — try/catch is sufficient for now" + ], + "answer": "A) Add to TODOS.md (recommended)" + }, + { + "toolUseId": "toolu_017nwDZhLb3oLysqRxToEUbZ", + "header": "Next review", + "question": "CEO Review complete. This plan had a critical SQL injection, email error gap, no tests, and N+1 queries — all now resolved through plan decisions. Eng Review is the required gate before ship. Run /plan-eng-review next?", + "options": [ + "Run /plan-eng-review next (recommended)", + "Skip — I'll handle reviews manually" + ], + "answer": "Run /plan-eng-review next (recommended)" + } + ] + }, + { + "name": "paired-first", + "sourceCapture": "plan-ceo-review-1788889172906-tPJLg7", + "sessionId": "60f4731c-327e-4712-981b-c96b0d3cf5eb", + "originalReviewCount": 5, + "expectedReviewCount": 4, + "calls": [ + { + "toolUseId": "toolu_012YREPoKQhMKnnDb73VnJME", + "header": "Receipt assert", + "question": "D6 — Happy-path assertion: what should the test verify about the receipt? The plan says 'assert correct receipt is generated' but doesn't define which fields constitute correctness. A weak assertion (receipt.present?) passes even when amount, customer, or charge ID regress. Stakes: a buggy receipt ships undetected if the assertion is too broad. Recommendation: A — specific fields, because you get regression coverage for free with the same test. Completeness: A=9/10, B=4/10, C=7/10. ", + "options": [ + "A) Assert specific receipt fields (Recommended)", + "B) Accept ‘assert correct receipt’ as-is", + "C) Add a note to the plan defining ‘correct’" + ], + "answer": "A) Assert specific receipt fields (Recommended)" + }, + { + "toolUseId": "toolu_0198Z1gZC8gkx6ezv73qyXYX", + "header": "Fail contract", + "question": "D7 — 502 error path: what does the test assert when retries are exhausted? The plan says 'fails clean' but doesn’t define the failure contract. Stakes: a test asserting `raises_exception?` passes even if the wrong exception type is raised (e.g., a generic RuntimeError instead of a PaymentError) — the test’s diagnostic value collapses. Recommendation: A — assert a named exception, because it documents processPayment()’s contract explicitly. Completeness: A=9/10, B=6/10, C=5/10. ", + "options": [ + "A) Assert a specific named exception class (Recommended)", + "B) Assert raises + message contains a keyword", + "C) Accept ‘fails clean’ as-is" + ], + "answer": "A) Assert a specific named exception class (Recommended)" + }, + { + "toolUseId": "toolu_01UArfrjX2d9JwzV3J9Bnpbs", + "header": "Call count", + "question": "D8 — Should the 502 test explicitly assert that Stripe was called exactly twice? The plan says 'retry-with-backoff fires once' (2 total calls). Without a call-count assertion, the test passes even if the retry loop fires 0 times (e.g., someone accidentally sets max_retries=0 in the factory) or 10 times — silently wrong retry behavior. Recommendation: A — assert call_count == 2, because that’s the observable proof that retry-with-backoff fired once. Completeness: A=9/10, B=6/10. ", + "options": [ + "A) Assert mock.call_count == 2 (Recommended)", + "B) Assert only the failure outcome, not call count" + ], + "answer": "A) Assert mock.call_count == 2 (Recommended)" + }, + { + "toolUseId": "toolu_01T6gSvFAp4qDwojZHKfU22b", + "header": "TODO?", + "question": "D9 — TODO: Add processPayment() unit tests for 402 and 429 paths? The Stripe adapter suite already covers 402 card declines and 429 rate limits at integration level, but processPayment() has no unit tests for these paths specifically. A future regression in how processPayment() handles these codes (e.g., it stops delegating correctly) would be caught only by the slower integration suite. Effort: S (human ~30min / CC ~5min). Priority: P3. ", + "options": [ + "A) Add to TODOS.md", + "B) Skip — integration coverage is sufficient", + "C) Build it now in this PR" + ], + "answer": "A) Add to TODOS.md" + }, + { + "toolUseId": "toolu_012xtd9WoAtgtz8VrBNxSNM2", + "header": "Next step", + "question": "D10 — What’s next? The CEO review is CLEARED. The Eng Review (/plan-eng-review) has not run and is the required gate before shipping. It covers architecture depth, test pyramid validation, and performance checks that the CEO review doesn’t duplicate. Note: options differ in kind, not coverage — no completeness score. Recommendation: A — run eng review next, it’s the required gate. ", + "options": [ + "Run /plan-eng-review next (Recommended)", + "Skip — I’ll handle reviews manually" + ], + "answer": "Run /plan-eng-review next (Recommended)" + } + ] + }, + { + "name": "five-retry", + "sourceCapture": "plan-ceo-review-1788889864558-kUTrrg", + "sessionId": "7fb8e00c-6b83-4510-b4b1-34d169fa3bd5", + "originalReviewCount": 8, + "expectedReviewCount": 8, + "calls": [ + { + "toolUseId": "toolu_011aNGxuX6kHRKCC2dRpGKpy", + "header": "Architecture", + "question": "D4 — Section 1 Architecture: WebhookDispatcher bypass needs rationale in the plan", + "options": [ + "A) Add rationale to plan — document why bypass is intentional (recommended)", + "B) Leave it as-is — namespace separation is sufficient rationale" + ], + "answer": "A) Add rationale to plan — document why bypass is intentional (recommended)" + }, + { + "toolUseId": "toolu_012qW99DV551qYXiZu7VKxLk", + "header": "Error handling", + "question": "D5 — Section 2 Errors: Email leg has no rescue handlers and timing ambiguity with dedup", + "options": [ + "A) Fix inline: wrap email in try/catch, log, enqueue retry (recommended)", + "B) Accept email failure as silent — catch, log, swallow" + ], + "answer": "A) Fix inline: wrap email in try/catch, log, enqueue retry (recommended)" + }, + { + "toolUseId": "toolu_01NnQUTDuwXSyqCYhLHj4yad", + "header": "SQL injection", + "question": "D6 — Section 3 Security: SQL injection is P0 — how should the fix be specified in the plan?", + "options": [ + "A) Parameterized queries + type validation — specify both in the plan (recommended)", + "B) Parameterized queries only — bind parameter is sufficient" + ], + "answer": "A) Parameterized queries + type validation — specify both in the plan (recommended)" + }, + { + "toolUseId": "toolu_011kzne3D3vqdCWYHU3kFZjS", + "header": "Data edge case", + "question": "D7 — Section 4 Data Flow: Missing userId in payload is unspecified", + "options": [ + "A) Add explicit guard: validate userId presence before lookup (recommended)", + "B) Rely on existing lookup guard behavior" + ], + "answer": "A) Add explicit guard: validate userId presence before lookup (recommended)" + }, + { + "toolUseId": "toolu_014DJHCbwaSPycewGhb4ayyM", + "header": "Test coverage", + "question": "D8 — Section 6 Tests: Zero test coverage for a new payment flow", + "options": [ + "A) Add 5 focused tests to the plan (recommended)", + "B) Add happy-path integration test only" + ], + "answer": "A) Add 5 focused tests to the plan (recommended)" + }, + { + "toolUseId": "toolu_013JiZ2mFSKLa2e2Nrn5FDdv", + "header": "Performance", + "question": "D9 — Section 7 Performance: N+1 query + unbounded order load", + "options": [ + "A) Eager load orders + add a LIMIT (recommended)", + "B) Eager load orders only — no LIMIT" + ], + "answer": "A) Eager load orders + add a LIMIT (recommended)" + }, + { + "toolUseId": "toolu_016EJRhhj8e4vYaRzd2nYpP2", + "header": "Observability", + "question": "D10 — Section 8 Observability: New handler needs structured logging to be debuggable", + "options": [ + "A) Add 3 structured log lines to the handler spec (recommended)", + "B) Rely on ingress wrapper logging" + ], + "answer": "A) Add 3 structured log lines to the handler spec (recommended)" + }, + { + "toolUseId": "toolu_01AaNKrDMAyzcfvqnLBc2aBV", + "header": "Deployment", + "question": "D11 — Section 9 Deployment: Post-deploy verification checklist is missing from the plan", + "options": [ + "A) Add verification checklist to the plan (recommended)", + "B) Skip — existing rollback path is sufficient" + ], + "answer": "A) Add verification checklist to the plan (recommended)" + } + ] + } + ] +} diff --git a/test/helpers/ceo-completion-handoff.ts b/test/helpers/ceo-completion-handoff.ts new file mode 100644 index 000000000..ffccae9a6 --- /dev/null +++ b/test/helpers/ceo-completion-handoff.ts @@ -0,0 +1,42 @@ +import type { AskUserQuestionFingerprint } from './claude-pty-runner'; + +/** Only the CEO's finished-review menu, never a finding/TODO mentioning another skill. */ +function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { + const call = fp.nativeCall; + // The capture path assigns this native identity only after matching the + // active question. UI-only and mismatched pending records cannot steer it. + if (!call || call.failed || fp.signature !== `${call.sessionId}:${call.toolUseId}` || call.questions.length !== 1) return null; + const q = call.questions[0]!; + if (q.multiSelect || q.options.length < 2) return null; + const id = //i.exec(q.question)?.[1]?.toLowerCase(); + if (id && id !== 'plan-ceo-next-steps') return null; + const completion = /\bCEO\s+review\s+(?:is\s+)?(?:complete\b|cleared\b)/i.test(q.question); + if (id !== 'plan-ceo-next-steps' && (!/^next\s+(?:review|steps?)$/i.test(q.header.trim()) || !completion)) return null; + + const labels = q.options.map(o => o.label.trim().replace(/^[A-Z][).]\s*/i, '').replace(/\s*\(recommended\)\s*$/i, '').trim()); + const runs = labels.map(label => /^Run\s+\/plan-(?:eng|design)-review\s+next(?:\s*\(required gate\))?$/i.test(label)); + const manual = labels.map(label => /^(?:Skip|Done)\s*[—–-]\s*I['’]ll\s+handle\s+reviews\s+manually$/i.test(label)); + // Every choice must be administrative. Adding a fix/TODO/build option + // makes the whole call substantive, even if it also offers a next review. + if (!runs.some(Boolean) || manual.filter(Boolean).length !== 1 || !labels.every((_, i) => runs[i] || manual[i])) return null; + return manual.findIndex(Boolean) + 1; +} + +/** Classification happens only after one real, successful, fully answered native call. */ +export function isCeoCompletionHandoff(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (!call?.answered || call.failed || !Array.isArray(call.unansweredQuestionIndices) || call.unansweredQuestionIndices.length) return false; + if (manualHandoffIndex(fp) === null) return false; + const q = call.questions[0]!; + // A free-form answer can introduce a new substantive request. Do not + // silently discard it merely because the menu itself was administrative. + return q.options.some(option => option.label === call.answers?.[q.question]); +} + +/** Finish this CEO fixture instead of starting another skill; reuse the existing caller-pick hook. */ +export function pickCeoCompletionHandoff( + fp: AskUserQuestionFingerprint, + activeCapture: AskUserQuestionFingerprint = fp, +): number | null { + return activeCapture.nativeCall?.answered ? null : manualHandoffIndex(activeCapture); +} diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index 45c0e3e24..8ef2fed60 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -1059,9 +1059,11 @@ export interface AskUserQuestionFingerprint { options: Array<{ index: number; label: string }>; /** Wall-clock when first observed (ms since the helper started polling). */ observedAtMs: number; - /** True if observed BEFORE the Step-0 boundary fired. */ + /** True for setup classification; completed administrative handoffs are false plus their marker. */ preReview: boolean; - /** Lossless source metadata for counted calls; UI-only fingerprints omit it. */ + /** A completed handoff is preserved but adds neither setup nor finding coverage. */ + administrative?: 'completion-handoff'; + /** Lossless source metadata for observed calls; UI-only fingerprints omit it. */ nativeCall?: NativePlanQuestionCall; } @@ -1093,15 +1095,20 @@ export type Step0BoundaryPredicate = ( answeredFingerprint: AskUserQuestionFingerprint, ) => boolean; -/** A first finding starts review immediately; a final setup question does not count as a finding. */ +/** First findings start review immediately; recognized setup stays setup even when its order varies. */ export function planCountQuestionPhase( fp: AskUserQuestionFingerprint, reviewStarted: boolean, isLastStep0AUQ: Step0BoundaryPredicate, isFirstReviewAUQ?: Step0BoundaryPredicate, -): { preReview: boolean; reviewStarted: boolean } { + isSetupAUQ?: Step0BoundaryPredicate, + isCompletionHandoffAUQ?: Step0BoundaryPredicate, +): { preReview: boolean; reviewStarted: boolean; administrative?: 'completion-handoff' } { + // A completion menu cannot start review or satisfy a finding floor, even + // if its summary mentions defects that a first-finding predicate recognizes. + if (isCompletionHandoffAUQ?.(fp)) return { preReview: false, reviewStarted, administrative: 'completion-handoff' }; const inReview = reviewStarted || Boolean(isFirstReviewAUQ?.(fp)); - return { preReview: !inReview, reviewStarted: inReview || isLastStep0AUQ(fp) }; + return { preReview: Boolean(isSetupAUQ?.(fp)) || !inReview, reviewStarted: inReview || isLastStep0AUQ(fp) }; } /** @@ -1424,6 +1431,71 @@ export function assertReviewReportAtBottom( return { ok: true }; } +/** + * A final native completion can replace a lost terminal heading, but never + * an unanswered question, an old report, or a quoted completion example. + * The expected path is supplied by the caller, not extracted for filesystem + * access from model output. This does not add any question-count coverage. + */ +export function hasNativePlanCompletion( + transcript: PlanCountTranscript, + expectedPlanPath: string, + startedAt: number, +): boolean { + if (transcript.status !== 'ready' || !path.isAbsolute(expectedPlanPath) || + !transcript.calls.length || transcript.calls.some(c => !c.answered || c.failed) || + !transcript.assistantMessages.length) return false; + const sessions = new Set([...transcript.calls.map(c => c.sessionId), + ...transcript.assistantMessages.map(m => m.sessionId)]); + if (sessions.size !== 1) return false; + const messages = [...transcript.assistantMessages].sort((a, b) => Date.parse(a.timestamp) - Date.parse(b.timestamp)); + const final = messages.at(-1)!; + const finishedAt = Date.parse(final.timestamp); + const answerTimes = transcript.calls.map(c => Date.parse(c.answeredAt ?? '')); + if (!Number.isFinite(finishedAt) || !answerTimes.every(t => Number.isFinite(t) && t < finishedAt)) return false; + const text = final.text.trim(); + // Actual native prose, not ANSI aliases: the final announcement must say + // the review is complete and identify the exact caller-owned deliverable. + if (!/^(?:DX|Design|Eng(?:ineering)?|CEO) review complete[.!](?:\s|$)/i.test(text)) return false; + const lastLine = text.split('\n').at(-1)!.trim(); + if (lastLine !== `Plan written to: \`${expectedPlanPath}\`` && + lastLine !== `Plan written to: ${expectedPlanPath}`) return false; + const prose = text.replace(/```[\s\S]*?```|`[^`]*`/g, ''); + if (/\?|\b(?:awaiting|waiting for|please (?:choose|answer|confirm))\b/i.test(prose)) return false; + try { + const stat = fs.lstatSync(expectedPlanPath); + if (!stat.isFile() || stat.size > 2 * 1024 * 1024 || + stat.mtimeMs < Math.max(startedAt, ...answerTimes) || stat.mtimeMs > finishedAt) return false; + // A template/example inside Markdown code is not the completed report. + // Fences open with 3+ identical backticks or tildes, indented at most + // three spaces. A close uses the same marker, at least the opening + // length, and only horizontal whitespace after it. + const reportLines: string[] = []; + let fence: { marker: string; length: number } | undefined; + for (const line of fs.readFileSync(expectedPlanPath, 'utf8').split(/\r?\n/)) { + if (fence) { + const close = /^ {0,3}(`{3,}|~{3,})[ \t]*$/.exec(line); + if (close && close[1]![0] === fence.marker && close[1]!.length >= fence.length) fence = undefined; + continue; + } + const open = /^ {0,3}(`{3,}|~{3,})(.*)$/.exec(line); + // Backticks are forbidden in a backtick fence's info string. + if (open && (open[1]![0] !== '`' || !open[2]!.includes('`'))) { + fence = { marker: open[1]![0]!, length: open[1]!.length }; + continue; + } + reportLines.push(line); + } + if (fence) return false; + const content = reportLines.join('\n'); + if (!assertReviewReportAtBottom(content).ok) return false; + const report = content.slice(content.indexOf('## GSTACK REVIEW REPORT')); + // Reject a provisional heading; retain both clean and issues-open reports. + return /^\|[^\n]*Review[^\n]*\|/m.test(report) && + /^(?:\*\*)?VERDICT:/m.test(report) && /(?:NO )?UNRESOLVED DECISIONS/.test(report); + } catch { return false; } +} + /** * Test helper: if `obs.planFile` was set, read it and assert * `## GSTACK REVIEW REPORT` is the last `## ` section. Throws on @@ -1501,23 +1573,49 @@ export const ceoFirstReviewAUQ: Step0BoundaryPredicate = (fp) => /\bfinding\b|\bmissing\b|\bambiguous\b|\bundefined\b|doesn['’]t\s+(?:define|specify|cover|mention)/i.test(title); }) ?? false; +/** Native setup identity comes from answered question IDs/actions, not display wording. */ +export const engSetupAUQ: Step0BoundaryPredicate = (fp) => { + const call = fp.nativeCall; + if (!call?.answered || call.failed) return false; + const answered = call.questions.filter(q => Boolean(call.answers?.[q.question])); + // A mixed packet containing an answered finding is not wholly setup. + return answered.length > 0 && answered.every(q => { + const id = //i.exec(q.question)?.[1]?.toLowerCase(); + const crossProject = id === 'cross-project-learnings' || id === 'preamble-cross-project-learnings' || + (!id && /^cross[- ]project$/i.test(q.header.trim())); + if (crossProject) { + return q.options.some((enabled, i) => /^enable\s+cross[- ]project\b/i.test(enabled.label.trim()) && + q.options.some((scoped, j) => j !== i && /\bproject[- ]scoped\b/i.test(scoped.label))); + } + const scopeComplexity = id === 'plan-eng-scope-complexity' || id === 'plan-eng-review-scope-reduce' || + // Older native captures omitted qids. Require whole-plan class/file scope + // and its opposed scope actions, never an individual implementation issue. + (!id && /\bplan\b/i.test(q.question) && /\bclasses\b/i.test(q.question) && /\bfiles\b/i.test(q.question)); + return scopeComplexity && + q.options.some((proceed, i) => + (/^proceed\s+as[- ]is\b/i.test(proceed.label.trim()) || /^accept\b.*\bdesign\b.*\bfocus\b.*\bquality\b/i.test(proceed.label.trim())) && + q.options.some((reduce, j) => j !== i && /^(?:reduce\b|flag\s+scope\s+reduction\b)/i.test(reduce.label.trim()))); + }); +}; + +/** An answered substantive finding can start review even in a mixed setup packet. */ +export const engFirstReviewAUQ: Step0BoundaryPredicate = (fp) => { + const call = fp.nativeCall; + if (!call?.answered || call.failed) return false; + return call.questions.some(q => { + if (!call.answers?.[q.question]) return false; + // These are review section identities, including the registry's + // arch-finding/test-gap IDs. Scope and onboarding IDs cannot qualify. + const id = //i.exec(q.question)?.[1] ?? ''; + if (/^plan-eng-(?:review-)?(?:arch(?:itecture)?|quality|test|perf(?:ormance)?)-(?:focus|mode|setup|routing|learnings|prerequisite|onboarding|next-steps?)(?:-|$)/i.test(id)) return false; + const title = `${q.header} ${q.question.split('\n')[0]}`.replace(/]*>/gi, ''); + return /^plan-eng-(?:review-)?(?:arch(?:itecture)?|quality|test|perf(?:ormance)?)-/i.test(id) && + /\b(?:issue|finding|gap)\b/i.test(title); + }); +}; + export const engStep0Boundary: Step0BoundaryPredicate = (fp) => - // Native onboarding identifies this choice by its header and opposed - // scopes; the question body can paraphrase "cross-project learnings". - // An answered sibling tab cannot close setup for this unanswered choice. - (fp.nativeCall?.answered === true && !fp.nativeCall.failed && fp.nativeCall.questions.some(q => - Boolean(fp.nativeCall?.answers?.[q.question]) && /^cross[- ]project$/i.test(q.header.trim()) && - q.options.some((enabled, i) => /^enable\s+cross[- ]project\b/i.test(enabled.label.trim()) && - q.options.some((scoped, j) => j !== i && /\bproject[- ]scoped\b/i.test(scoped.label))))) || - // The native complexity gate can say "Reduce scope or proceed as-is" - // without the historical "scope reduction recommendation" wording. - // It closes setup only once that exact scope question has an answer. - (fp.nativeCall?.answered === true && fp.nativeCall.questions.some(q => - Boolean(fp.nativeCall?.answers?.[q.question]) && /^scope$/i.test(q.header.trim()) && - //i.test(q.question) && - /\bcomplexity\s+check\b/i.test(q.question) && - q.options.some(o => /^proceed\s+as[- ]is\b/i.test(o.label)) && - q.options.some(o => /^reduce\b/i.test(o.label)))) || + engSetupAUQ(fp) || /scope\s*reduction\s*recommendation|cross[\s-]*project\s*learnings/i.test( fp.promptSnippet, ) || @@ -2193,7 +2291,7 @@ export async function runPlanSkillObservation(opts: { /** * Result of a `runPlanSkillCounting` run. Includes both the count summary - * (`step0Count`, `reviewCount`) and the full fingerprint list for diagnostic + * (`step0Count`, `reviewCount`, `administrativeCount`) and the full fingerprint list for diagnostic * dumps when an assertion fails. */ export interface PlanSkillCountObservation { @@ -2217,10 +2315,12 @@ export interface PlanSkillCountObservation { fingerprints: AskUserQuestionFingerprint[]; /** Actual native calls, including unanswered/failed ones that add no coverage. */ transcript: PlanCountTranscript; - /** Count of fingerprints with `preReview === true`. */ + /** Setup questions; administrative completion handoffs are excluded. */ step0Count: number; - /** Count of fingerprints with `preReview === false`. */ + /** Review questions; administrative completion handoffs are excluded. */ reviewCount: number; + /** Answered administrative completion handoffs, preserved separately. */ + administrativeCount: number; } /** @@ -2249,7 +2349,7 @@ export interface PlanSkillCountObservation { * prerequisite by label so a different skill does not rewrite the * seeded plan before this review begins. * - After the native answer, evaluate `isLastStep0AUQ(fingerprint)`. If true, - * all subsequent AUQs are review-phase. + * subsequent AUQs are review-phase unless the caller positively identifies native setup. * - Hard ceiling: if `reviewCount >= reviewCountCeiling`, return * `ceiling_reached`. This bounds runaway counts; tests should set * the ceiling above their assertion CEILING. @@ -2278,10 +2378,19 @@ export async function runPlanSkillCounting(opts: { isLastStep0AUQ: Step0BoundaryPredicate; /** Optional positive identity for a first finding when no final setup AUQ was emitted. */ isFirstReviewAUQ?: Step0BoundaryPredicate; + /** Optional native setup classifier; late/reordered setup must not become a finding. */ + isSetupAUQ?: Step0BoundaryPredicate; + /** Optional native completed-review handoff identity, excluded from both count bands. */ + isCompletionHandoffAUQ?: Step0BoundaryPredicate; /** Optional issue classifier across phases; receives full native call metadata. */ isReviewAUQ?: (fp: AskUserQuestionFingerprint) => boolean; - /** Narrow caller-specific selection; null retains the normal answer policy. */ - pickAUQ?: (fp: AskUserQuestionFingerprint) => number | null; + /** Narrow caller-specific selection; null retains the normal answer policy. + * The first argument retains full pending metadata for existing callers. + * Native-bound selection uses activeCapture, whose metadata is present only + * when capturePlanCountQuestion matched the currently visible native question. */ + pickAUQ?: (fp: AskUserQuestionFingerprint, activeCapture: AskUserQuestionFingerprint) => number | null; + /** Opt-in native completion fallback, validated against this caller-owned report path. */ + expectedPlanPath?: string; /** Additional versioned files available in the isolated fixture before the skill starts. */ fixtureFiles?: Record; /** Hard cap on review-phase count; helper returns when reached. Should be @@ -2338,6 +2447,7 @@ export async function runPlanSkillCounting(opts: { let boundaryFired = false; let step0Count = 0; let reviewCount = 0; + let administrativeCount = 0; let isFirstAUQ = true; const saveSnapshot = createPlanCountSnapshotWriter(); let lastCheckpointAt = Date.now(); @@ -2367,6 +2477,7 @@ export async function runPlanSkillCounting(opts: { transcript, step0Count, reviewCount, + administrativeCount, }; const artifacts = capture(observation); Object.assign(observation, artifacts); @@ -2394,13 +2505,19 @@ export async function runPlanSkillCounting(opts: { const signature = `${call.sessionId}:${call.toolUseId}`; if (!call.answered || countedCalls.has(signature)) continue; const fp = nativePlanCallFingerprint(call, Date.now() - startedAt, !boundaryFired); - const phase = planCountQuestionPhase(fp, boundaryFired, opts.isLastStep0AUQ, opts.isFirstReviewAUQ); - fp.preReview = opts.isReviewAUQ ? !opts.isReviewAUQ(fp) : phase.preReview; + const phase = planCountQuestionPhase(fp, boundaryFired, opts.isLastStep0AUQ, opts.isFirstReviewAUQ, opts.isSetupAUQ, opts.isCompletionHandoffAUQ); + if (phase.administrative) { + fp.preReview = false; + fp.administrative = phase.administrative; + administrativeCount += 1; + } else { + fp.preReview = opts.isReviewAUQ ? !opts.isReviewAUQ(fp) : phase.preReview; + if (fp.preReview) step0Count += 1; + else reviewCount += 1; + } fp.promptSnippet = fp.promptSnippet.replace(/\s+/g, ' ').slice(0, 240); fingerprints.push(fp); countedCalls.add(signature); - if (fp.preReview) step0Count += 1; - else reviewCount += 1; boundaryFired = phase.reviewStarted; } if (reviewCount >= opts.reviewCountCeiling) { @@ -2414,7 +2531,7 @@ export async function runPlanSkillCounting(opts: { if (Date.now() - lastCheckpointAt >= 30_000) { lastCheckpointAt = Date.now(); const saved = capture({ state: 'in_progress', elapsedMs: Date.now() - startedAt, - fingerprints, step0Count, reviewCount, transcript }); + fingerprints, step0Count, reviewCount, administrativeCount, transcript }); if (saved.artifactError) console.error(`PTY artifact write failed: ${saved.artifactError}`); } @@ -2493,6 +2610,11 @@ export async function runPlanSkillCounting(opts: { ); } + if (opts.expectedPlanPath && hasNativePlanCompletion(transcript, opts.expectedPlanPath, startedAt)) { + return snapshot('completion_summary', + `native review completion and final report verified (step0=${step0Count}, review=${reviewCount})`, visible); + } + // A dismissed or repainted permission is never a native question. if (permission === 'handled') continue; @@ -2507,7 +2629,9 @@ export async function runPlanSkillCounting(opts: { const prerequisitePick = planCountPrerequisitePick(routing); // Native tool records may flush only after the answer. Let a guarded // caller recognize that visible menu, but never override a known packet. - const callerPick = !pending || pending.questions.length === 1 ? opts.pickAUQ?.(routing) ?? null : null; + // The captured fingerprint alone proves whether native metadata matched + // this active UI; an unrelated pending record is not a routing identity. + const callerPick = !pending || pending.questions.length === 1 ? opts.pickAUQ?.(routing, fp) ?? null : null; const pickIdx = prerequisitePick ?? callerPick ?? (isFirstAUQ && opts.firstAUQPick ? opts.firstAUQPick(routing) : defaultPick); isFirstAUQ = false; diff --git a/test/helpers/claude-pty-runner.unit.test.ts b/test/helpers/claude-pty-runner.unit.test.ts index cfc8a6be0..a58cff738 100644 --- a/test/helpers/claude-pty-runner.unit.test.ts +++ b/test/helpers/claude-pty-runner.unit.test.ts @@ -47,6 +47,8 @@ import { assertReviewReportAtBottom, ceoStep0Boundary, engStep0Boundary, + engSetupAUQ, + engFirstReviewAUQ, designStep0Boundary, designFirstReviewAUQ, planCountQuestionPhase, @@ -1693,7 +1695,7 @@ describe('Step0BoundaryPredicate per-skill', () => { test('keeps D3 as setup and counts each following actual review call', () => { let started = false; const phases = captured.map((_, i) => { - const phase = planCountQuestionPhase(fingerprint(i), started, engStep0Boundary); + const phase = planCountQuestionPhase(fingerprint(i), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); started = phase.reviewStarted; return phase; }); @@ -1702,6 +1704,27 @@ describe('Step0BoundaryPredicate per-skill', () => { expect(phases.slice(3).map(p => p.preReview)).toEqual([false, false, false, false, false]); }); + test('the captured no-qid scope decision stays setup after Learnings scope', () => { + let started = false; + const phases = [0, 2, 1, 3, 4, 5, 6, 7].map(i => { + const phase = planCountQuestionPhase(fingerprint(i), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); + started = phase.reviewStarted; + return phase; + }); + expect(phases.slice(0, 3).map(p => p.preReview)).toEqual([true, true, true]); + expect(phases.slice(3).map(p => p.preReview)).toEqual([false, false, false, false, false]); + const call = structuredClone(fingerprint(1).nativeCall!); + call.questions[0].header = 'Scope complexity'; + call.questions[0].options.reverse(); + call.answers = { [call.questions[0].question]: call.questions[0].options[0].label }; + expect(engSetupAUQ(nativePlanCallFingerprint(call, 0, false))).toBe(true); + // Even with opposed scope labels, an ordinary per-issue finding lacks + // the captured whole-plan classes/files identity. + call.questions[0].question = 'How should we reduce shared mutable cache complexity?'; + call.answers = { [call.questions[0].question]: call.questions[0].options[0].label }; + expect(engSetupAUQ(nativePlanCallFingerprint(call, 0, false))).toBe(false); + }); + test('uses the native opposed scope choices without depending on a prompt suffix', () => { const fp = fingerprint(2); expect(engStep0Boundary(fp)).toBe(true); @@ -1913,7 +1936,6 @@ describe('Step0BoundaryPredicate per-skill', () => { unansweredScope.nativeCall!.answers = { 'Separate answered setup question': 'Continue' }; expect(engStep0Boundary(unansweredScope)).toBe(false); for (const alter of [ - (q: any) => { q.header = 'Architecture'; }, (q: any) => { q.question = q.question.replace('plan-eng-scope-complexity', 'plan-eng-cache-complexity'); }, (q: any) => { q.options = [{ label: 'Change cache size' }, { label: 'Keep cache size' }]; }, ]) { @@ -2108,3 +2130,261 @@ describe('file permission lifecycle replay', () => { expect(capturePlanCountQuestion(first + question, new Set(), 0, true)?.promptSnippet).toContain('File policy'); }); }); + + +describe('native Eng setup ordering (captured F)', () => { + // Exact native question stems and choice labels: two setup calls followed + // by five real findings. Descriptions do not establish phase identity. + const rows = [ + { + "header": "Learnings scope", + "question": "D1 \u2014 Should gstack search learnings from your other projects on this machine? ", + "options": [ + "Enable cross-project (Recommended)", + "Project-scoped only" + ], + "answer": "Enable cross-project (Recommended)" + }, + { + "header": "Scope complexity", + "question": "D2 \u2014 The plan introduces 4 new classes across 12 files. That's above the complexity threshold (>2 classes / >8 files). Should we reduce scope or proceed as-is? ", + "options": [ + "Reduce: merge to 2 classes (Recommended)", + "Proceed as-is \u2014 4 classes, 12 files", + "Reduce further: 1 new class only" + ], + "answer": "Reduce: merge to 2 classes (Recommended)" + }, + { + "header": "Arch: shared state", + "question": "D3 \u2014 Architecture Issue 1: AuthCache is a global mutable singleton exported at module level; both AuthBroker and (previously) SessionMint mutate it. This creates concurrent-mutation risk across tenant requests and makes the services untestable in isolation. ", + "options": [ + "Inject AuthCache via constructor (Recommended)", + "Keep global, add locking", + "Proceed as-is" + ], + "answer": "Inject AuthCache via constructor (Recommended)" + }, + { + "header": "Code quality", + "question": "D4 \u2014 Code Quality Issue 1: validateAndDispatch() is 60 lines with three nested try/catch blocks, each swallowing a different error class. Swallowed errors mean callers can't distinguish an IDP timeout from a policy rejection from a token parse failure \u2014 all three silently return the same result. ", + "options": [ + "Extract + typed error union (Recommended)", + "Add logging to each catch, keep structure", + "Proceed as-is" + ], + "answer": "Extract + typed error union (Recommended)" + }, + { + "header": "Test: regression", + "question": "D5 \u2014 Test Issue 1 (IRON RULE): legacyAuthFlow() is being rewritten with no regression test for its prior behavior. The plan explicitly says coverage 'does not exercise legacyAuthFlow() or assert compatibility with its prior behavior.' A rewrite without a behavioral snapshot means any regression is invisible until production. ", + "options": [ + "Add characterization tests before rewrite (Recommended)", + "Document expected behavior, manual verify", + "Skip regression coverage" + ], + "answer": "Add characterization tests before rewrite (Recommended)" + }, + { + "header": "Test: isolation", + "question": "D6 \u2014 Test Issue 2: The plan says 'unit and integration coverage is planned for success/error paths' but makes no mention of cross-tenant isolation tests. The AuthCache key includes tenant ID, issuer, audience, and policy version \u2014 a key-construction bug would let Tenant A read Tenant B's cached tokens. This is the highest-severity failure mode in a multi-tenant auth system. ", + "options": [ + "Add explicit cross-tenant isolation tests (Recommended)", + "Cover via integration tests only", + "Proceed with existing test plan" + ], + "answer": "Add explicit cross-tenant isolation tests (Recommended)" + }, + { + "header": "Perf: IDP calls", + "question": "D7 \u2014 Performance Issue 1: Token validation makes 5 sequential API calls to the IDP. The plan itself notes they are independent and could be parallelized via Promise.all 'trivially.' Sequential calls add latency proportional to IDP round-trip time x5 on every auth request. At p99 IDP latency of 100ms, that's 500ms of unnecessary serialization per login. ", + "options": [ + "Parallelize with Promise.all in this PR (Recommended)", + "Defer to follow-up ticket", + "Defer with in-code TODO comment" + ], + "answer": "Parallelize with Promise.all in this PR (Recommended)" + } +]; + const fingerprint = (index: number): AskUserQuestionFingerprint => { + const row = rows[index]!; + return nativePlanCallFingerprint({ + sessionId: 'captured-f-eng', toolUseId: `f-${index}`, answered: true, failed: false, + questions: [{ header: row.header, question: row.question, options: row.options.map(label => ({ label })) }], + answers: { [row.question]: row.answer }, + }, index, true); + }; + const phasesFor = (indices: number[]) => { + let started = false; + return indices.map(index => { + const phase = planCountQuestionPhase(fingerprint(index), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); + started = phase.reviewStarted; + return phase; + }); + }; + + test('both setup orders exclude setup and include the first of five actual findings', () => { + for (const order of [[0, 1], [1, 0]]) { + const phases = phasesFor([...order, 2, 3, 4, 5, 6]); + expect(phases.slice(0, 2).map(p => p.preReview)).toEqual([true, true]); + expect(phases[1]!.reviewStarted).toBe(true); + expect(phases[2]!.preReview).toBe(false); + expect(phases.filter(p => !p.preReview)).toHaveLength(5); + } + }); + + test('setup IDs plus opposed answered choices survive header and question-body variants', () => { + for (const index of [0, 1]) { + const original = fingerprint(index).nativeCall!; + const id = index === 0 ? 'cross-project-learnings' : 'plan-eng-scope-complexity'; + for (const header of ['Learnings scope', 'Scope complexity', 'Architecture', '']) { + const call = structuredClone(original); + const q = call.questions[0]!; + q.header = header; + q.question = `Choose the setup scope. `; + q.options.reverse(); + call.answers = { [q.question]: q.options[0]!.label }; + const fp = nativePlanCallFingerprint(call, 0, true); + expect(engSetupAUQ(fp)).toBe(true); + expect(engStep0Boundary(fp)).toBe(true); + } + } + }); + + test('repeated setup does not become a finding after the boundary opens', () => { + const phases = phasesFor([0, 0, 1, 1, 2, 3, 4, 5, 6]); + expect(phases.slice(0, 4).every(p => p.preReview)).toBe(true); + expect(phases.filter(p => !p.preReview)).toHaveLength(5); + }); + + test('only successful answered setup metadata can exclude a call', () => { + for (const index of [0, 1]) { + const fp = fingerprint(index); + expect(engSetupAUQ({ ...fp, nativeCall: undefined })).toBe(false); + for (const alter of [ + (call: any) => { call.answered = false; }, + (call: any) => { call.failed = true; }, + (call: any) => { call.answers = {}; }, + (call: any) => { call.questions[0].question = 'Review cache isolation. '; }, + (call: any) => { call.questions[0].options = [{ label: 'Apply fix' }, { label: 'Defer finding' }]; }, + (call: any) => { call.questions[0].options = [{ label: 'Enable cross-project with project-scoped storage; proceed as-is or reduce' }, { label: 'Discuss' }]; }, + ]) { + const call = structuredClone(fp.nativeCall!); + alter(call); + // Keep a successful answer after question/option mutations, so those + // controls exercise identity/actions rather than an absent answer key. + if (Object.keys(call.answers ?? {}).length) { + call.answers = { [call.questions[0].question]: call.questions[0].options[0].label }; + } + expect(engSetupAUQ(nativePlanCallFingerprint(call, 0, false))).toBe(false); + } + } + }); + + test('an answered sibling cannot turn an unanswered setup tab or mixed finding packet into setup', () => { + const call = structuredClone(fingerprint(0).nativeCall!); + const issue = fingerprint(2).nativeCall!.questions[0]!; + call.questions.push(issue); + call.answers = { [issue.question]: issue.options[0]!.label }; + expect(engSetupAUQ(nativePlanCallFingerprint(call, 0, false))).toBe(false); + call.answers[call.questions[0]!.question] = call.questions[0]!.options[0]!.label; + const fp = nativePlanCallFingerprint(call, 0, false); + expect(engSetupAUQ(fp)).toBe(false); + expect(planCountQuestionPhase(fp, true, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ).preReview).toBe(false); + }); + + test('the optional predicate leaves other callers and substantive Eng qids unchanged', () => { + const fp = fingerprint(2); + expect(engSetupAUQ(fp)).toBe(false); + expect(planCountQuestionPhase(fp, true, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ).preReview).toBe(false); + // Historical boundary detection can also fire on actual review qids; + // those must never be reused as the late-setup exclusion predicate. + fp.promptSnippet += ' '; + expect(engStep0Boundary(fp)).toBe(true); + expect(planCountQuestionPhase(fp, true, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ).preReview).toBe(false); + const setup = fingerprint(0); + expect(planCountQuestionPhase(setup, true, engStep0Boundary).preReview).toBe(false); + }); +}); + + +describe('native Eng first packet and registry identities', () => { + const scope = { + header: 'Scope complexity', + question: 'D1 — Choose the whole-plan scope. ', + options: [{ label: 'Reduce: merge two classes (Recommended)' }, { label: 'Proceed as-is: four classes' }], + }; + const finding = { + header: 'Arch: shared state', + question: 'D3 — Architecture Issue 1: AuthCache is a global mutable singleton exported at module level. ', + options: [{ label: 'Inject an owned cache' }, { label: 'Keep the global cache' }], + }; + const callWith = (questions: typeof scope[], answers: Record) => ({ + sessionId: 'eng-first-packet', toolUseId: 'mixed-call', answered: true, failed: false, + questions, answers, + }); + const phase = (call: ReturnType) => planCountQuestionPhase( + nativePlanCallFingerprint(call, 0, true), false, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ, + ); + + test('registry setup qids stay setup and require opposed scope actions', () => { + const call = callWith([scope], { [scope.question]: scope.options[0]!.label }); + const fp = nativePlanCallFingerprint(call, 0, true); + expect(engSetupAUQ(fp)).toBe(true); + expect(engFirstReviewAUQ(fp)).toBe(false); + expect(phase(call)).toEqual({ preReview: true, reviewStarted: true }); + call.questions = [{ ...scope, options: [{ label: 'Change cache size' }, { label: 'Keep cache size' }] }]; + call.answers = { [scope.question]: 'Change cache size' }; + expect(engSetupAUQ(nativePlanCallFingerprint(call, 0, true))).toBe(false); + const learnings = { + header: 'Learnings scope', question: 'Choose local learning scope. ', + options: [{ label: 'Enable cross-project learnings' }, { label: 'Keep project-scoped only' }], + }; + expect(engSetupAUQ(nativePlanCallFingerprint(callWith([learnings], { + [learnings.question]: learnings.options[0]!.label, + }), 0, true))).toBe(true); + }); + + test('a first packet with answered setup and a real finding counts as one review call', () => { + const call = callWith([scope, finding], { + [scope.question]: scope.options[0]!.label, + [finding.question]: finding.options[0]!.label, + }); + expect(phase(call)).toEqual({ preReview: false, reviewStarted: true }); + // The packet is one call even with two answered tabs; the reader/counter + // dedups the unchanged session/tool-use ID rather than counting each tab. + expect(nativePlanCallFingerprint(call, 0, true).signature).toBe('eng-first-packet:mixed-call'); + for (const questions of [[scope, finding], [finding, scope]]) { + expect(phase({ ...call, questions }).preReview).toBe(false); + } + }); + + test('an unanswered or failed finding cannot start review from a setup packet', () => { + const call = callWith([scope, finding], { [scope.question]: scope.options[0]!.label }); + expect(phase(call)).toEqual({ preReview: true, reviewStarted: true }); + call.answers = { [finding.question]: finding.options[0]!.label }; + expect(phase(call).preReview).toBe(false); + for (const invalid of [{ ...call, answered: false }, { ...call, failed: true }, { ...call, answers: {} }]) { + expect(engFirstReviewAUQ(nativePlanCallFingerprint(invalid, 0, true))).toBe(false); + } + }); + + test('only positive substantive native question identity starts review', () => { + for (const qid of ['plan-eng-review-scope-reduce', 'plan-eng-scope-complexity', 'cross-project-learnings', 'plan-eng-review-next-steps']) { + const q = { ...finding, question: finding.question.replace('plan-eng-arch-global-cache', qid) }; + expect(engFirstReviewAUQ(nativePlanCallFingerprint(callWith([q], { [q.question]: q.options[0]!.label }), 0, true))).toBe(false); + } + for (const qid of ['plan-eng-review-arch-finding', 'plan-eng-review-test-gap']) { + const q = { ...finding, question: finding.question.replace('plan-eng-arch-global-cache', qid) }; + expect(engFirstReviewAUQ(nativePlanCallFingerprint(callWith([q], { [q.question]: q.options[0]!.label }), 0, true))).toBe(true); + } + for (const qid of ['plan-eng-arch-focus', 'plan-eng-test-focus', 'plan-eng-quality-mode', 'plan-eng-perf-next-steps']) { + const q = { ...finding, header: 'Review setup', question: `Choose which issue to review first. ` }; + expect(engFirstReviewAUQ(nativePlanCallFingerprint(callWith([q], { [q.question]: q.options[0]!.label }), 0, true))).toBe(false); + } + const testScope = { ...finding, header: 'Test scope', question: 'D3 — Test Issue: no coverage of the rewritten legacy flow is specified. ' }; + expect(engFirstReviewAUQ(nativePlanCallFingerprint(callWith([testScope], { [testScope.question]: testScope.options[0]!.label }), 0, true))).toBe(true); + const q = { ...finding, header: 'Architecture', question: 'Choose a review focus. ' }; + expect(engFirstReviewAUQ(nativePlanCallFingerprint(callWith([q], { [q.question]: q.options[0]!.label }), 0, true))).toBe(false); + }); +}); diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 56055ea56..009216534 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -160,10 +160,10 @@ export const E2E_TOUCHFILES: Record = { // Each test drives its skill end-to-end; touchfiles include preamble + // completion-status resolvers because they affect question cadence and // terminal output (the regression surface this test catches). - 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-ceo-finding-count.test.ts'], + 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/fixtures/ceo-completion-handoff-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-ceo-finding-count.test.ts'], 'plan-eng-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-eng-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-eng-finding-count.test.ts'], 'plan-design-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-design-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/skill-e2e-plan-design-finding-count.test.ts'], - 'plan-devex-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-devex-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts'], + 'plan-devex-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-devex-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts'], // Gate-tier reviewCount-floor counterparts. Catch the May 2026 transcript // bug (model wrote a plan-mode plan and ExitPlanMode'd without firing any diff --git a/test/plan-count-completion.test.ts b/test/plan-count-completion.test.ts new file mode 100644 index 000000000..cca888444 --- /dev/null +++ b/test/plan-count-completion.test.ts @@ -0,0 +1,241 @@ +/** Native completion/report regression; no model/API calls. */ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { hasNativePlanCompletion, classifyPlanCountFrame } from './helpers/claude-pty-runner'; +import type { PlanCountTranscript } from './helpers/plan-count-transcript'; + +const CAPTURED_CALL = { + "sessionId": "b5c582af-870e-48ac-ac1e-c85458932136", + "toolUseId": "toolu_017PNDKgKXk1VyxRT9BVdDbt", + "answered": true, + "failed": false, + "answeredAt": "2026-09-08T17:45:31.244Z", + "questions": [ + { + "header": "P5: v1\u2192v2 break", + "question": "D7 \u2014 Pass 5, Finding 6: The v1\u2192v2 upgrade removes `Client.evaluate()` immediately with zero migration support, guaranteeing broken production installs on pip upgrade. ", + "options": [ + { + "label": "A \u2014 Migration guide + deprecation alias (Recommended)", + "description": "Plan must include: (1) migration guide in changelog ('replace .evaluate() with .run()'), (2) v2 ships a .evaluate() compatibility alias that raises DeprecationWarning('use Client.run() instead'), (3) one-paragraph upgrade section in README. Upgrade is boring, not scary. (human: ~2h / CC: ~20min)" + }, + { + "label": "B \u2014 Migration guide only (no alias)", + "description": "Add a migration guide in the changelog with the one-line fix. No compatibility alias \u2014 v1 users who miss the guide still break. Better than nothing but still relies on developers reading the changelog." + }, + { + "label": "C \u2014 Accept immediate removal, document in release notes", + "description": "Keep the current plan (immediate removal, no alias). Add a prominent breaking-change banner to release notes. Honest about the break, but production projects that auto-upgrade still break silently." + } + ] + } + ], + "answers": { + "D7 \u2014 Pass 5, Finding 6: The v1\u2192v2 upgrade removes `Client.evaluate()` immediately with zero migration support, guaranteeing broken production installs on pip upgrade. ": "A \u2014 Migration guide + deprecation alias (Recommended)" + } +}; +const CAPTURED_FINAL = { + "sessionId": "b5c582af-870e-48ac-ac1e-c85458932136", + "text": "DX review complete. Here's what was found and resolved:\n\n**6 findings across 5 review passes \u2014 all resolved interactively:**\n\n| # | Pass | Issue | Fix |\n|---|------|-------|-----|\n| D2 | Getting Started | `examples/first_eval.py` referenced in README but absent from package | Remove reference; make terminal demo the single path |\n| D3 | Getting Started | Demo triggers mandatory 5-min CI gate despite using bundled offline data; TTHW 6 min vs <2 min target | Exempt `evalkit.demo` from CI gate; TTHW drops to ~1-2 min |\n| D4 | API Design | `run_eval(dataset, evaluator)` and `run_batch(evaluator, dataset)` have reversed positional order \u2014 silent wrong-result bugs | Enforce keyword-only args on both functions |\n| D5 | Error Messages | `AuthError(\"request failed\")` \u2014 no code, no cause, no fix | Replace with `AuthError(\"Invalid API key \u2014 check EVALKIT_API_KEY or visit ...\", code=\"auth_invalid_key\")` |\n| D6 | Documentation | No expected output shown after demo command | Add expected output block to README |\n| D7 | Upgrade Path | `Client.evaluate()` removed immediately with no migration path | Ship deprecation shim + migration guide + README upgrade section |\n\n**Score:** 4/10 \u2192 8/10. TTHW: 6 min \u2192 ~1-2 min (Champion tier).\n\n**Critical blocker:** The CI gate finding (D3) is architecturally non-trivial \u2014 the plan currently says \"retains this gate\" but the TTHW target requires exempting demo mode. `/plan-eng-review` should validate the gate bypass implementation before this ships.\n\nPlan written to: `/tmp/gstack-paid-shard-qDnJUI/tmp/gstack-e2e-plan-devex-VWDg0F/gstack-test-plan-devex.md`", + "timestamp": "2026-09-08T17:49:33.033Z" +}; +const CAPTURED_PATH = "/tmp/gstack-paid-shard-qDnJUI/tmp/gstack-e2e-plan-devex-VWDg0F/gstack-test-plan-devex.md"; +const REPORT = '# Reviewed plan\n\n## GSTACK REVIEW REPORT\n\n' + + '| Review | Status | Findings |\n|---|---|---|\n| DX Review | clean | resolved |\n\n' + + 'VERDICT: DX CLEARED — eng review required\n\nNO UNRESOLVED DECISIONS\n'; + +function fixture() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'native-plan-completion-')); + const file = path.join(dir, 'review.md'); + const startedAt = Date.parse('2026-09-08T17:39:30.000Z'); + fs.writeFileSync(file, REPORT); + const modified = Date.parse('2026-09-08T17:48:30.000Z') / 1000; + fs.utimesSync(file, modified, modified); + const transcript: PlanCountTranscript = { + status: 'ready', calls: [structuredClone(CAPTURED_CALL)], + assistantMessages: [{ ...CAPTURED_FINAL, text: CAPTURED_FINAL.text.replace(CAPTURED_PATH, file) }], + }; + return { dir, file, startedAt, transcript, cleanup: () => fs.rmSync(dir, { recursive: true, force: true }) }; +} + +describe('native plan completion and final report', () => { + test('recognizes captured completed native DevEx prose despite a nonmatching terminal heading', () => { + const f = fixture(); + try { + const visible = '●' + f.transcript.assistantMessages[0]!.text.replace(/ /g, '') + '\nCrunched for 10m 9s ·done 5:49PM\n❯ '; + expect(classifyPlanCountFrame(visible)).toBeNull(); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(true); + } finally { f.cleanup(); } + }); + + test('rejects pending, failed, missing, mixed-session and untimestamped native coverage', () => { + const f = fixture(); + try { + for (const change of [ + (t: any) => { t.status = 'missing'; }, + (t: any) => { t.status = 'error'; }, + (t: any) => { t.calls = []; }, + (t: any) => { t.calls[0].answered = false; }, + (t: any) => { t.calls[0].failed = true; }, + (t: any) => { t.calls[0].answeredAt = undefined; }, + (t: any) => { t.calls[0].sessionId = 'foreign'; }, + (t: any) => { t.assistantMessages.push({ ...t.assistantMessages[0], sessionId: 'foreign' }); }, + (t: any) => { t.assistantMessages = []; }, + ]) { + const t = structuredClone(f.transcript); change(t); + expect(hasNativePlanCompletion(t, f.file, f.startedAt)).toBe(false); + } + } finally { f.cleanup(); } + }); + + test('requires the latest native announcement to follow every answer', () => { + const f = fixture(); + try { + f.transcript.calls[0]!.answeredAt = '2026-09-08T17:50:00.000Z'; + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(false); + f.transcript.calls[0]!.answeredAt = CAPTURED_CALL.answeredAt; + f.transcript.assistantMessages.push({ ...CAPTURED_FINAL, text: 'Please answer the remaining question.', timestamp: '2026-09-08T17:50:00.000Z' }); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(false); + } finally { f.cleanup(); } + }); + + test('quoted completions, refusals, provisional writes, wrong paths and waiting questions are not done', () => { + const f = fixture(); + try { + const complete = f.transcript.assistantMessages[0]!.text; + for (const text of [ + '> ' + complete, '```text\n' + complete + '\n```', + 'Example completion:\n' + complete, + "I cannot complete this review.\nPlan written to: `" + f.file + '`', + 'Now writing the complete plan file with all resolved findings and the review report.', + complete.replace(f.file, f.file + '.other'), + complete.replace('Plan written to:', 'Should I proceed?\nPlan written to:'), + complete.replace('Plan written to:', 'Waiting for your decision.\nPlan written to:'), + ]) { + f.transcript.assistantMessages[0]!.text = text; + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt), text).toBe(false); + } + } finally { f.cleanup(); } + }); + + test('missing, stale, changed-after-announcement and provisional reports cannot complete', () => { + const f = fixture(); + try { + for (const date of ['2026-09-08T17:38:00Z', '2026-09-08T17:44:00Z', '2026-09-08T17:50:00Z']) { + const time = Date.parse(date) / 1000; fs.utimesSync(f.file, time, time); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(false); + } + for (const report of ['# Draft\n', '## GSTACK REVIEW REPORT\n', REPORT + '\n## Still editing\n']) { + fs.writeFileSync(f.file, report); const time = Date.parse('2026-09-08T17:48:30Z') / 1000; fs.utimesSync(f.file, time, time); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(false); + } + fs.rmSync(f.file); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt)).toBe(false); + expect(hasNativePlanCompletion(f.transcript, 'review.md', f.startedAt)).toBe(false); + } finally { f.cleanup(); } + }); + + test('a fenced report example or an unclosed fence never supplies completion markers', () => { + const f = fixture(); + try { + for (const report of [ + '# Plan\nExample report:\n```markdown\n' + REPORT + '\n```\n', + '# Plan\nExample report:\n~~~markdown\n' + REPORT + '\n~~~\n', + '```markdown\n' + REPORT, + '~~~markdown\n' + REPORT, + // An apparent shorter/mismatched/annotated close leaves the report + // inside the original code fence, rather than making it a real report. + '````markdown\n```\n' + REPORT + '\n````\n', + '~~~markdown\n```\n' + REPORT + '\n~~~\n', + '```markdown\n```still-code\n' + REPORT + '\n```\n', + ' ~~~~markdown\n ~~~\n' + REPORT + '\n ~~~~\n', + REPORT + '\n```unclosed\n', + ]) { + fs.writeFileSync(f.file, report); + const time = Date.parse('2026-09-08T17:48:30Z') / 1000; + fs.utimesSync(f.file, time, time); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt), report).toBe(false); + } + } finally { f.cleanup(); } + }); + + test('real code fences before the final report do not hide that report', () => { + const f = fixture(); + try { + for (const prefix of [ + '```typescript\nconst answer = 42;\n```\n', + '~~~typescript\nconst answer = 42;\n~~~~~\n', + ' ````typescript\n```still-code\n ````` \t\n', + '```markdown\n' + REPORT + '\n```\n', + ]) { + fs.writeFileSync(f.file, '# Plan\n' + prefix + '\n' + REPORT); + const time = Date.parse('2026-09-08T17:48:30Z') / 1000; + fs.utimesSync(f.file, time, time); + expect(hasNativePlanCompletion(f.transcript, f.file, f.startedAt), prefix).toBe(true); + } + } finally { f.cleanup(); } + }); + + test.skipIf(process.platform === 'win32')('real PTY completes from a native report without a display alias', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'native-completion-pty-')); + const fake = path.join(dir, 'fake-claude'); + const worker = path.join(dir, 'worker.ts'); + const output = path.join(dir, 'plan.md'); + const record = path.join(dir, 'inputs.jsonl'); + const result = path.join(dir, 'result.json'); + const runner = pathToFileURL(path.join(import.meta.dir, 'helpers/claude-pty-runner.ts')).href; + fs.writeFileSync(fake, `#!${process.execPath}\n` + String.raw` +import * as fs from 'node:fs'; +import * as path from 'node:path'; +const sessionId = 'native-completion-fixture'; +const project = path.join(process.env.CLAUDE_CONFIG_DIR, 'projects', sessionId); +fs.mkdirSync(project, { recursive: true }); +const file = path.join(project, sessionId + '.jsonl'); +const native = (role, content, extra = {}) => fs.appendFileSync(file, JSON.stringify({ + cwd: process.cwd(), sessionId, isSidechain: false, + timestamp: new Date().toISOString(), message: { role, content }, ...extra, +}) + '\n'); +let sent = false; +process.stdin.setRawMode?.(true); +process.stdin.on('data', data => { + fs.appendFileSync(process.env.PROBE_INPUTS, JSON.stringify(data.toString()) + '\n'); + if (sent) return; sent = true; + const q = { header: 'Error quality', question: 'D1 — Fix the missing error recovery path?', options: [{ label: 'Add recovery hint' }, { label: 'Keep vague message' }] }; + native('assistant', [{ type: 'tool_use', id: 'finding', name: 'AskUserQuestion', input: { questions: [q] } }]); + const answeredAt = new Date(Date.now() - 100).toISOString(); + native('user', [{ type: 'tool_result', tool_use_id: 'finding', content: 'Answered.' }], { + timestamp: answeredAt, toolUseResult: { answers: { [q.question]: q.options[0].label } }, + }); + fs.writeFileSync(process.env.PROBE_PLAN, process.env.PROBE_REPORT); + const text = 'DX review complete. One finding resolved interactively.\n\nPlan written to: ' + String.fromCharCode(96) + process.env.PROBE_PLAN + String.fromCharCode(96); + native('assistant', [{ type: 'text', text }], { timestamp: new Date(Date.now() + 5).toISOString() }); + process.stdout.write('407 +NO UNRESOLVED DECISIONS\n●' + text.replace(/ /g, '') + '\nCrunched for 10m 9s ·done 5:49PM\n❯ '); +}); +process.stdin.resume(); +`); + fs.chmodSync(fake, 0o755); + fs.writeFileSync(worker, `import { runPlanSkillCounting } from ${JSON.stringify(runner)};\n` + + `const result = await runPlanSkillCounting({skillName:'plan-devex-review',slashCommand:'/plan-devex-review',followUpPrompt:'# Native completion fixture',expectedPlanPath:${JSON.stringify(output)},isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:20000,env:${JSON.stringify({PROBE_PLAN:output,PROBE_INPUTS:record,PROBE_REPORT:REPORT})}});\n` + + `await Bun.write(${JSON.stringify(result)},JSON.stringify(result));\n`); + const child = Bun.spawn([process.execPath, worker], { + env: { ...process.env, EVALS_HERMETIC: '1', EVALS_RUN_ID: '', BROWSE_TERMINAL_BINARY: fake }, + stdout: 'pipe', stderr: 'pipe', + }); + const timer = setTimeout(() => child.kill('SIGKILL'), 35000); + try { + const [code, stdout, stderr] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]); + expect(code, stdout + stderr).toBe(0); + const observation = JSON.parse(fs.readFileSync(result, 'utf8')); + expect(observation.outcome).toBe('completion_summary'); + expect(observation.summary).toContain('native review completion and final report verified'); + expect(observation.reviewCount).toBe(1); + expect(observation.step0Count).toBe(0); + expect(observation.transcript.calls).toHaveLength(1); + expect(fs.readFileSync(record, 'utf8').trim().split('\n').map(line => JSON.parse(line))).toEqual(['/plan-devex-review\r']); + expect(fs.existsSync(output)).toBe(true); + } finally { clearTimeout(timer); child.kill('SIGKILL'); fs.rmSync(dir, { recursive: true, force: true }); } + }, 40000); +}); diff --git a/test/skill-e2e-plan-ceo-finding-count.test.ts b/test/skill-e2e-plan-ceo-finding-count.test.ts index 0efdfee4f..77cef2483 100644 --- a/test/skill-e2e-plan-ceo-finding-count.test.ts +++ b/test/skill-e2e-plan-ceo-finding-count.test.ts @@ -17,6 +17,7 @@ import { test } from 'bun:test'; import { describeE2ETier } from './helpers/e2e-gate'; +import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -153,6 +154,8 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', () followUpPrompt: planCeo5Findings(planPath), isLastStep0AUQ: ceoStep0Boundary, isFirstReviewAUQ: ceoFirstReviewAUQ, + isCompletionHandoffAUQ: isCeoCompletionHandoff, + pickAUQ: pickCeoCompletionHandoff, reviewCountCeiling: CEILING_DISTINCT + 1, // hard cap above assertion ceiling firstAUQPick: pickSkipInterview, // bypass scope-selection, route to review timeoutMs: 1_500_000, // 25 min @@ -180,7 +183,7 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', () `Likely batching regression — agent collapsed multiple findings into fewer questions.\n` + `Fingerprints (review-phase only):\n` + obs.fingerprints - .filter((f) => !f.preReview) + .filter((f) => !f.preReview && !f.administrative) .map((f) => ` - "${f.promptSnippet.slice(0, 80)}"`) .join('\n'), ); @@ -237,6 +240,8 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', () followUpPrompt: planCeo2PairedFindings(planPath), isLastStep0AUQ: ceoStep0Boundary, isFirstReviewAUQ: ceoFirstReviewAUQ, + isCompletionHandoffAUQ: isCeoCompletionHandoff, + pickAUQ: pickCeoCompletionHandoff, reviewCountCeiling: CEILING_PAIRED + 1, timeoutMs: 1_500_000, env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' }, @@ -255,7 +260,7 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', () `Two deliberately related findings were batched into <2 questions — the rule failed under D12.\n` + `Review-phase fingerprints:\n` + obs.fingerprints - .filter((f) => !f.preReview) + .filter((f) => !f.preReview && !f.administrative) .map((f) => ` - "${f.promptSnippet.slice(0, 80)}"`) .join('\n'), ); diff --git a/test/skill-e2e-plan-devex-finding-count.test.ts b/test/skill-e2e-plan-devex-finding-count.test.ts index 839c9922b..f06915cdc 100644 --- a/test/skill-e2e-plan-devex-finding-count.test.ts +++ b/test/skill-e2e-plan-devex-finding-count.test.ts @@ -48,6 +48,7 @@ describeE2E('/plan-devex-review per-finding AskUserQuestion count (periodic)', ( skillName: 'plan-devex-review', slashCommand: '/plan-devex-review', followUpPrompt: planDevexCountFixture(planPath), + expectedPlanPath: planPath, fixtureFiles: DEVEX_COUNT_FILES, isLastStep0AUQ: devexStep0Boundary, isReviewAUQ: isDevexReviewIssue, diff --git a/test/skill-e2e-plan-eng-finding-count.test.ts b/test/skill-e2e-plan-eng-finding-count.test.ts index cd3b089a0..517f06e0b 100644 --- a/test/skill-e2e-plan-eng-finding-count.test.ts +++ b/test/skill-e2e-plan-eng-finding-count.test.ts @@ -16,6 +16,8 @@ import * as path from 'node:path'; import { runPlanSkillCounting, engStep0Boundary, + engSetupAUQ, + engFirstReviewAUQ, assertReviewReportAtBottom, } from './helpers/claude-pty-runner'; @@ -82,6 +84,8 @@ describeE2E('/plan-eng-review per-finding AskUserQuestion count (periodic)', () slashCommand: '/plan-eng-review', followUpPrompt: planEng5Findings(planPath), isLastStep0AUQ: engStep0Boundary, + isSetupAUQ: engSetupAUQ, + isFirstReviewAUQ: engFirstReviewAUQ, reviewCountCeiling: CEILING + 1, timeoutMs: 1_500_000, env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' }, diff --git a/test/skill-e2e-plan-eng-multi-finding-batching.test.ts b/test/skill-e2e-plan-eng-multi-finding-batching.test.ts index d6c423d36..17dfc9834 100644 --- a/test/skill-e2e-plan-eng-multi-finding-batching.test.ts +++ b/test/skill-e2e-plan-eng-multi-finding-batching.test.ts @@ -32,6 +32,8 @@ import * as path from 'node:path'; import { runPlanSkillCounting, engStep0Boundary, + engSetupAUQ, + engFirstReviewAUQ, } from './helpers/claude-pty-runner'; import { FORCING_BATCHING_ENG } from './fixtures/forcing-finding-seeds'; @@ -64,6 +66,8 @@ describeE2E('/plan-eng-review multi-finding batching regression (periodic)', () slashCommand: '/plan-eng-review', followUpPrompt, isLastStep0AUQ: engStep0Boundary, + isSetupAUQ: engSetupAUQ, + isFirstReviewAUQ: engFirstReviewAUQ, reviewCountCeiling: N + 3, // hard cap above floor + tolerance timeoutMs: 1_500_000, // 25 min env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' },