mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 02:30:39 +01:00
test: fix planning eval routing, counting, and timeout handling
This commit is contained in:
Garry Tan committed
1 parent
2ab4912635
commit
878deebcf0
25 files changed
+2246
-48
No files matched your search
@@ -0,0 +1,142 @@
|
||||
import { describe, expect, test } from 'bun:test';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { capturePlanCountQuestion, nativePlanCallFingerprint, planCountQuestionInput } from './helpers/claude-pty-runner';
|
||||
import { pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff';
|
||||
import { pickCeoCountQuestion, pickCeoRecommendedApproach } from './helpers/ceo-approach-pick';
|
||||
import type { NativePlanQuestionCall } from './helpers/plan-count-transcript';
|
||||
import recorded from './fixtures/ceo-approach-q-call.json';
|
||||
import pairedRecorded from './fixtures/ceo-approach-q-paired-call.json';
|
||||
import handoffs from './fixtures/ceo-completion-handoff-m-call.json';
|
||||
|
||||
function pending(source: NativePlanQuestionCall = recorded as NativePlanQuestionCall): NativePlanQuestionCall {
|
||||
const call = structuredClone(source);
|
||||
call.answered = false;
|
||||
delete call.answers;
|
||||
delete call.unansweredQuestionIndices;
|
||||
return call;
|
||||
}
|
||||
const fingerprint = (call: NativePlanQuestionCall, preReview = true) => nativePlanCallFingerprint(call, 0, preReview);
|
||||
function screen(call: NativePlanQuestionCall): string {
|
||||
const q = call.questions[0]!;
|
||||
return `☐ ${q.header}\n${q.question}\n${q.options.map((o, i) => `${i ? ' ' : '❯'} ${i + 1}. ${o.label}`).join('\n')}\nEnter to select · ↑/↓ to navigate · Esc to cancel`;
|
||||
}
|
||||
|
||||
describe('CEO pre-review approach recommendation', () => {
|
||||
test('the exact Q menu changes the old default risk acceptance to its offered recommendation', () => {
|
||||
const call = pending();
|
||||
const visible = screen(call);
|
||||
const active = capturePlanCountQuestion(visible, new Set(), 0, true, call)!;
|
||||
expect(active.nativeCall).toBe(call);
|
||||
const before = pickCeoCompletionHandoff(fingerprint(call), active) ?? 1;
|
||||
const after = pickCeoCountQuestion(fingerprint(call), active) ?? 1;
|
||||
expect(before).toBe(1);
|
||||
expect(after).toBe(2);
|
||||
expect(planCountQuestionInput(visible, active, after)).toBe('2');
|
||||
expect(recorded.answers[recorded.questions[0]!.question]).toBe(recorded.questions[0]!.options[0]!.label);
|
||||
expect(pickCeoCountQuestion(fingerprint(recorded as NativePlanQuestionCall))).toBeNull();
|
||||
});
|
||||
|
||||
test('the paired first native approach uses the same offered recommendation policy', () => {
|
||||
const call = pending(pairedRecorded as NativePlanQuestionCall);
|
||||
const visible = screen(call);
|
||||
const active = capturePlanCountQuestion(visible, new Set(), 0, true, call)!;
|
||||
expect(active.nativeCall).toBe(call);
|
||||
expect(pickCeoCompletionHandoff(fingerprint(call), active) ?? 1).toBe(1);
|
||||
const after = pickCeoCountQuestion(fingerprint(call), active) ?? 1;
|
||||
expect(after).toBe(2);
|
||||
expect(planCountQuestionInput(visible, active, after)).toBe('2');
|
||||
expect(pairedRecorded.answers[pairedRecorded.questions[0]!.question]).toBe(pairedRecorded.questions[0]!.options[0]!.label);
|
||||
expect(pickCeoCountQuestion(fingerprint(pairedRecorded as NativePlanQuestionCall))).toBeNull();
|
||||
});
|
||||
|
||||
test('paired approach grammar is function-agnostic and follows reordered options', () => {
|
||||
const call = pending(pairedRecorded as NativePlanQuestionCall);
|
||||
const q = call.questions[0]!;
|
||||
q.question = 'D3 — Which implementation approach for the renderWidget() tests? <gstack-qid:plan-ceo-review-impl-approach>';
|
||||
q.options = [{ label: 'A) Custom renderer' }, { label: 'B) Existing renderer (Recommended)' }];
|
||||
expect(pickCeoRecommendedApproach(fingerprint(call))).toBe(2);
|
||||
q.options.reverse();
|
||||
expect(pickCeoRecommendedApproach(fingerprint(call))).toBe(1);
|
||||
});
|
||||
|
||||
test.each([
|
||||
['non-approach question', 'Should the renderWidget() tests be deleted? <gstack-qid:plan-ceo-review-impl-approach>'],
|
||||
['negated question', 'Which implementation approach should the renderWidget() tests not use? <gstack-qid:plan-ceo-review-impl-approach>'],
|
||||
['negated test subject', 'Which implementation approach for not testing renderWidget()? <gstack-qid:plan-ceo-review-impl-approach>'],
|
||||
['wrong approach identity', 'Which implementation approach for the renderWidget() tests? <gstack-qid:plan-ceo-approach>'],
|
||||
['extra action before question', 'Delete the tests. Which implementation approach for the renderWidget() tests? <gstack-qid:plan-ceo-review-impl-approach>'],
|
||||
])('does not apply paired approach selection to %s', (_name, question) => {
|
||||
const call = pending(pairedRecorded as NativePlanQuestionCall);
|
||||
call.questions[0]!.question = question;
|
||||
expect(pickCeoRecommendedApproach(fingerprint(call))).toBeNull();
|
||||
});
|
||||
|
||||
test('recommendation follows actual option position and arbitrary approach content, never seed words', () => {
|
||||
for (const order of [[0, 1, 2], [1, 2, 0], [2, 0, 1]]) {
|
||||
const call = pending();
|
||||
const q = call.questions[0]!;
|
||||
const options = [{ label: 'A) Compare two renderers' }, { label: 'B) Existing renderer (Recommended)' }, { label: 'C) Custom renderer' }];
|
||||
q.options = order.map(index => options[index]!);
|
||||
q.question = 'D1 — Which implementation approach should this plan use? <gstack-qid:plan-ceo-approach>';
|
||||
expect(pickCeoRecommendedApproach(fingerprint(call))).toBe(order.indexOf(1) + 1);
|
||||
}
|
||||
});
|
||||
|
||||
test.each([
|
||||
['no recommendation', (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'B) Secure Baseline'; }],
|
||||
['duplicate recommendation', (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.label += ' (Recommended)'; }],
|
||||
['duplicate offered label', (c: NativePlanQuestionCall) => { c.questions[0]!.options[2]!.label = c.questions[0]!.options[1]!.label; }],
|
||||
['negated recommendation', (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'B) Not (Recommended)'; }],
|
||||
['conflicting recommendation', (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'B) Not recommended here (Recommended)'; }],
|
||||
['description-only recommendation', (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'B) Secure Baseline'; c.questions[0]!.options[1]!.description = 'Recommended'; }],
|
||||
['unknown qid', (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('plan-ceo-approach', 'plan-ceo-security'); }],
|
||||
['missing qid', (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('<gstack-qid:plan-ceo-approach>', ''); }],
|
||||
['malformed extra qid', (c: NativePlanQuestionCall) => { c.questions[0]!.question += '<gstack-qid:broken'; }],
|
||||
['duplicate qid', (c: NativePlanQuestionCall) => { c.questions[0]!.question += '<gstack-qid:plan-ceo-approach>'; }],
|
||||
['negated approach question', (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('should this plan use?', 'should this plan not use?'); }],
|
||||
['non-approach question', (c: NativePlanQuestionCall) => { c.questions[0]!.question = 'Should we accept this security risk? <gstack-qid:plan-ceo-approach>'; }],
|
||||
['non-approach header', (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Review Mode'; }],
|
||||
['multi-select', (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }],
|
||||
['mixed packet', (c: NativePlanQuestionCall) => { c.questions.push(structuredClone(c.questions[0]!)); }],
|
||||
['failed native call', (c: NativePlanQuestionCall) => { c.failed = true; }],
|
||||
])('keeps the old caller/default policy for %s', (_name, change) => {
|
||||
const call = pending();
|
||||
change(call);
|
||||
const fp = fingerprint(call);
|
||||
expect(pickCeoRecommendedApproach(fp)).toBeNull();
|
||||
expect(pickCeoCountQuestion(fp)).toBe(pickCeoCompletionHandoff(fp));
|
||||
});
|
||||
|
||||
test('requires current native binding and pre-review phase', () => {
|
||||
const call = pending();
|
||||
const fp = fingerprint(call);
|
||||
expect(pickCeoRecommendedApproach({ ...fp, preReview: false })).toBeNull();
|
||||
expect(pickCeoRecommendedApproach({ ...fp, signature: 'foreign:call' })).toBeNull();
|
||||
expect(pickCeoRecommendedApproach({ ...fp, nativeQuestionIndex: 1 })).toBeNull();
|
||||
expect(pickCeoRecommendedApproach({ ...fp, options: fp.options.slice().reverse() })).toBeNull();
|
||||
const visibleOnly = capturePlanCountQuestion(screen(call), new Set(), 0, true)!;
|
||||
expect(visibleOnly.nativeCall).toBeUndefined();
|
||||
expect(pickCeoCountQuestion(fp, visibleOnly)).toBeNull();
|
||||
const foreign = '☐ Finding\nShould we add validation?\n❯ 1. Add fix\n 2. Defer\nEnter to select · ↑/↓ to navigate · Esc to cancel';
|
||||
const active = capturePlanCountQuestion(foreign, new Set(), 0, true, call)!;
|
||||
expect(active.nativeCall).toBeUndefined();
|
||||
expect(pickCeoCountQuestion(fp, active)).toBeNull();
|
||||
});
|
||||
|
||||
test('the existing completed-review manual picker still runs after approach selection declines', () => {
|
||||
const call = structuredClone(handoffs.calls.at(-1)!) as NativePlanQuestionCall;
|
||||
call.answered = false; delete call.answers; delete call.unansweredQuestionIndices;
|
||||
const fp = fingerprint(call, false);
|
||||
const expected = pickCeoCompletionHandoff(fp);
|
||||
expect(expected).not.toBeNull();
|
||||
expect(pickCeoCountQuestion(fp)).toBe(expected);
|
||||
});
|
||||
|
||||
test('both count callers use the composed picker while leaving first-scope and count predicates intact', () => {
|
||||
const caller = readFileSync(join(import.meta.dir, 'skill-e2e-plan-ceo-finding-count.test.ts'), 'utf8');
|
||||
expect(caller.match(/pickAUQ: pickCeoCountQuestion/g)).toHaveLength(2);
|
||||
expect(caller.match(/isFirstReviewAUQ: ceoFirstReviewAUQ/g)).toHaveLength(2);
|
||||
expect(caller).toContain('firstAUQPick: pickSkipInterview');
|
||||
});
|
||||
});
|
||||
@@ -6,6 +6,7 @@ import { ceoFirstReviewAUQ, ceoStep0Boundary, hasNativePlanTerminal, nativePlanC
|
||||
import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff';
|
||||
import type { NativePlanQuestionCall } from './helpers/plan-count-transcript';
|
||||
import captured from './fixtures/ceo-completion-handoff-o-call.json';
|
||||
import capturedQ from './fixtures/ceo-completion-handoff-q-call.json';
|
||||
|
||||
const calls = () => structuredClone(captured.calls) as NativePlanQuestionCall[];
|
||||
const handoff = () => calls().at(-1)!;
|
||||
@@ -150,3 +151,120 @@ describe('closed CEO navigation with the native review-prefixed identity', () =>
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('CEO completion recap after native project metadata', () => {
|
||||
const qCalls = () => structuredClone(capturedQ.calls) as NativePlanQuestionCall[];
|
||||
const qHandoff = () => qCalls().at(-1)!;
|
||||
|
||||
test('the exact Q sequence keeps all three substantive calls and four setup calls', () => {
|
||||
let started = false;
|
||||
const counts = { setup: 0, review: 0, administrative: 0 };
|
||||
for (const call of qCalls()) {
|
||||
const phase = planCountQuestionPhase(fingerprint(call), started, ceoStep0Boundary,
|
||||
ceoFirstReviewAUQ, undefined, isCeoCompletionHandoff);
|
||||
started = phase.reviewStarted;
|
||||
if (phase.administrative) counts.administrative++;
|
||||
else if (phase.preReview) counts.setup++;
|
||||
else counts.review++;
|
||||
}
|
||||
expect(counts).toEqual({ setup: 4, review: 3, administrative: 1 });
|
||||
expect(qCalls().slice(4, 7).map(call => isCeoCompletionHandoff(fingerprint(call)))).toEqual([false, false, false]);
|
||||
expect(isCeoCompletionHandoff(fingerprint(qHandoff()))).toBe(true);
|
||||
});
|
||||
|
||||
test('only the current offered manual option is selected, including reordered choices', () => {
|
||||
for (const reverse of [false, true]) {
|
||||
const call = qHandoff();
|
||||
call.answered = false; delete call.answers; delete call.unansweredQuestionIndices;
|
||||
if (reverse) call.questions[0]!.options.reverse();
|
||||
expect(pickCeoCompletionHandoff(fingerprint(call))).toBe(reverse ? 1 : 2);
|
||||
expect(pickCeoCompletionHandoff({ ...fingerprint(call), signature: 'foreign:call' })).toBeNull();
|
||||
expect(isCeoCompletionHandoff(fingerprint(call))).toBe(false);
|
||||
}
|
||||
expect(pickCeoCompletionHandoff(fingerprint(qHandoff()))).toBeNull();
|
||||
});
|
||||
|
||||
test('unconditional line recaps support ordinary completion wording and Eng sequencing', () => {
|
||||
for (const state of ['done and clear', 'done', 'complete', 'cleared']) {
|
||||
const call = qHandoff();
|
||||
call.questions[0]!.question = call.questions[0]!.question.replace('done and clear', state);
|
||||
call.questions[0]!.options[1]!.description = 'Once implementation is finished, run /plan-eng-review. After Eng review is complete, proceed to shipping.';
|
||||
expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
test('the recap cannot hide contradictory, conditional, quoted or new work in question or choices', () => {
|
||||
for (const extra of [
|
||||
'CEO review is not complete.', 'The review remains incomplete.', 'Not all decisions are resolved.',
|
||||
'One test gap remains.', 'Validation is still pending.', 'There are unresolved findings.',
|
||||
'Once tests pass, the CEO review will be complete.', 'All decisions resolved after tests pass.',
|
||||
'We should fix a missing authorization test.', 'We could repair a missing authorization check.',
|
||||
'Repair the missing authorization test.', 'Recommendation: repair the missing authorization test.',
|
||||
'We may repair the missing authorization test.', 'We might fix the missing authorization test.',
|
||||
'Proceed to add a new regression.', 'Do you want to add a missing test?',
|
||||
'```text\nCEO review is complete.', '> CEO review is complete.', 'Example: CEO review is complete.',
|
||||
]) {
|
||||
for (const target of ['question', 'description']) {
|
||||
const call = qHandoff();
|
||||
if (target === 'question') call.questions[0]!.question += `\n${extra}`;
|
||||
else call.questions[0]!.options[1]!.description += ` ${extra}`;
|
||||
expect(isCeoCompletionHandoff(fingerprint(reanswer(call))), `${target}: ${extra}`).toBe(false);
|
||||
call.answered = false;
|
||||
expect(pickCeoCompletionHandoff(fingerprint(call)), `${target}: ${extra}`).toBeNull();
|
||||
}
|
||||
}
|
||||
for (const first of [
|
||||
'Should we add a missing authorization test as the next step after this CEO review?',
|
||||
'The CEO review did not finish. What is next after this CEO review?',
|
||||
'Can you first fix authorization? What is next after this CEO review?',
|
||||
]) {
|
||||
const call = qHandoff();
|
||||
call.questions[0]!.question = call.questions[0]!.question.replace("What's next after this CEO review?", first);
|
||||
expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
test('native failures, mixed choices, absent gates and source copies cannot become administrative', () => {
|
||||
for (const mutate of [
|
||||
(c: NativePlanQuestionCall) => { c.failed = true; },
|
||||
(c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; },
|
||||
(c: NativePlanQuestionCall) => { c.questions.push(qCalls()[4]!.questions[0]!); },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Missing tests'; },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.question += ' <gstack-qid'; },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('plan-ceo-review-next-step', 'plan-ceo-new-test'); },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('required shipping gate', 'optional check'); c.questions[0]!.options[0]!.description = 'Optional check.'; },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'Fix the missing assertion'; },
|
||||
(c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('ELI10:', ' ELI10:'); },
|
||||
]) {
|
||||
const call = qHandoff(); mutate(call);
|
||||
expect(isCeoCompletionHandoff(fingerprint(reanswer(call)))).toBe(false);
|
||||
}
|
||||
const call = qHandoff();
|
||||
call.answers![call.questions[0]!.question] = 'Please fix another gap first';
|
||||
expect(isCeoCompletionHandoff(fingerprint(call))).toBe(false);
|
||||
});
|
||||
|
||||
test('actual full report and Exit chronology retain last substantive-answer freshness', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-metadata-navigation-'));
|
||||
const report = path.join(dir, 'plan.md');
|
||||
try {
|
||||
fs.writeFileSync(report, capturedQ.reportContent);
|
||||
const reportAt = Date.parse(capturedQ.reportAt) / 1000;
|
||||
fs.utimesSync(report, reportAt, reportAt);
|
||||
const transcript = { status: 'ready' as const, calls: qCalls(), assistantMessages: [], planReadyRequests: structuredClone(capturedQ.planReadyRequests) };
|
||||
const administrative = new Set([`${qHandoff().sessionId}:${qHandoff().toolUseId}`]);
|
||||
const start = Date.parse('2026-09-09T03:25:54Z');
|
||||
expect(hasNativePlanTerminal(transcript, report, start, 'plan_ready')).toBe(false);
|
||||
expect(hasNativePlanTerminal(transcript, report, start, 'plan_ready', administrative)).toBe(true);
|
||||
transcript.planReadyRequests[0]!.failed = true;
|
||||
expect(hasNativePlanTerminal(transcript, report, start, 'plan_ready', administrative)).toBe(false);
|
||||
transcript.planReadyRequests = structuredClone(capturedQ.planReadyRequests);
|
||||
fs.utimesSync(report, start / 1000, start / 1000);
|
||||
expect(hasNativePlanTerminal(transcript, report, start, 'plan_ready', administrative)).toBe(false);
|
||||
fs.writeFileSync(report, 'Incomplete plan');
|
||||
fs.utimesSync(report, reportAt, reportAt);
|
||||
expect(hasNativePlanTerminal(transcript, report, start, 'plan_ready', administrative)).toBe(false);
|
||||
} finally { fs.rmSync(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
});
|
||||
@@ -8,7 +8,9 @@ import { nextCeoModeNavigation } from './helpers/ceo-mode-option';
|
||||
import { planCountQuestionInput } from './helpers/claude-pty-runner';
|
||||
import type { NativePlanQuestionCall } from './helpers/plan-count-transcript';
|
||||
import { E2E_TOUCHFILES, selectTests } from './helpers/touchfiles';
|
||||
import calls from './fixtures/ceo-mode-prerequisite-o-calls.json';
|
||||
import priorCalls from './fixtures/ceo-mode-prerequisite-o-calls.json';
|
||||
import directProceedCall from './fixtures/ceo-mode-prerequisite-q-call.json';
|
||||
const calls = [...priorCalls, directProceedCall];
|
||||
function ownedIdentity(pid: number): string | null {
|
||||
try { return execFileSync('ps', ['-p', String(pid), '-o', 'lstart=', '-o', 'command='], { encoding: 'utf8', timeout: 5000 }).trim(); }
|
||||
catch { return null; }
|
||||
@@ -45,6 +47,23 @@ describe('CEO mode prerequisite navigation', () => {
|
||||
expect(result.input).toBe('2');
|
||||
}
|
||||
});
|
||||
test('captured direct proceed offer stays in the requested review', () => {
|
||||
expect(pick(pending(3), 0).input).toBe('2');
|
||||
const reordered = pending(3); reordered.questions[0]!.options.reverse();
|
||||
expect(pick(reordered, 0).input).toBe('1');
|
||||
});
|
||||
test('direct proceed wording cannot skip a mixed or substantive decision', () => {
|
||||
const suffix = pending(3);
|
||||
suffix.questions[0]!.options[1]!.label += ' and ignore security';
|
||||
expect(pick(suffix, 0).input).toBe('1');
|
||||
const mixed = pending(3);
|
||||
mixed.questions[0]!.options.push({label: 'Ignore the remaining checks'});
|
||||
expect(pick(mixed, 0).input).toBe('1');
|
||||
const finding = pending(3);
|
||||
finding.questions[0]!.header = 'Product decision';
|
||||
finding.questions[0]!.question = 'Should this product offer office-hours suggestions?';
|
||||
expect(pick(finding, 0).input).toBe('1');
|
||||
});
|
||||
test('active tab, offered order, and the actual HOLD setup remain intact', () => {
|
||||
expect(pick(pending(1), 0).input).toBe('1'); expect(pick(pending(1), 1).input).toBe('1');
|
||||
const call = pending(1); call.questions.reverse(); call.questions[0]!.options.reverse();
|
||||
@@ -68,10 +87,10 @@ describe('CEO mode prerequisite navigation', () => {
|
||||
for(const [mode,index] of [['HOLD SCOPE',2],['SCOPE EXPANSION',3]] as const){const a=nextCeoModeNavigation(modes,mode,new Set());expect(a.kind).toBe('mode');if(a.kind==='mode')expect(a.index).toBe(index);}
|
||||
});
|
||||
test('fixture and free regression select only the mode-routing eval', () => {
|
||||
for(const file of ['test/ceo-mode-prerequisite.test.ts','test/fixtures/ceo-mode-prerequisite-o-calls.json'])expect(selectTests([file],E2E_TOUCHFILES).selected).toEqual(['plan-ceo-mode-routing']);
|
||||
for(const file of ['test/ceo-mode-prerequisite.test.ts','test/fixtures/ceo-mode-prerequisite-o-calls.json','test/fixtures/ceo-mode-prerequisite-q-call.json'])expect(selectTests([file],E2E_TOUCHFILES).selected).toEqual(['plan-ceo-mode-routing']);
|
||||
});
|
||||
});
|
||||
for(const fixtureIndex of [1,2])test.skipIf(process.platform==='win32')(`fake native PTY skips prerequisite ${fixtureIndex} and confirms target posture`,async()=>{
|
||||
for(const fixtureIndex of [1,2,3])test.skipIf(process.platform==='win32')(`fake native PTY skips prerequisite ${fixtureIndex} and confirms target posture`,async()=>{
|
||||
const dir=fs.mkdtempSync(path.join(os.tmpdir(),'ceo-mode-prerequisite-')),fake=path.join(dir,'fake-claude'),events=path.join(dir,'events.jsonl'),worker=path.join(dir,'worker.ts');
|
||||
const root=path.resolve(import.meta.dir,'..'),call=pending(fixtureIndex),screens=call.questions.map((_,i)=>pane(call,i)),expected=call.questions.map(q=>q.header==='Design doc'?'2':'1');
|
||||
fs.writeFileSync(fake,`#!${process.execPath}\n`+`
|
||||
|
||||
@@ -162,3 +162,35 @@ describe('structured native stale-fill finding', () => {
|
||||
|
||||
])('rejects %s', (_name, text) => expect(hasStaleFillRaceFinding(text)).toBe(false));
|
||||
});
|
||||
|
||||
|
||||
// Actual Q report amended the original contract to accept later stale reads.
|
||||
// The oracle must not count that permission paragraph as an unresolved defect.
|
||||
describe('accepted consistency model is not a stale-fill finding', () => {
|
||||
const report = require('node:fs').readFileSync(require('node:path').join(import.meta.dir, 'fixtures/ceo-section-loading-q-report.md'), 'utf8');
|
||||
const accepted = report.slice(report.indexOf('- **AMENDED (stale-fill race):**'), report.indexOf('\n\n', report.indexOf('- **AMENDED (stale-fill race):**')));
|
||||
test('rejects the exact amended consistency paragraph', () => {
|
||||
expect(accepted).toMatch(/accepted consistency\s+model/);
|
||||
expect(hasStaleFillRaceFinding(accepted)).toBe(false);
|
||||
});
|
||||
test('rejects the complete Q report that accepts the late-fill race', () => {
|
||||
expect(hasStaleFillRaceFinding(report)).toBe(false);
|
||||
});
|
||||
test.each([
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. This is the accepted consistency model; TTL expiry is the consistency deadline.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. This remains the documented consistency contract.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. This is an intentional consistency policy.',
|
||||
])('rejects a declared consistency allowance: %s', text => {
|
||||
expect(hasStaleFillRaceFinding(text)).toBe(false);
|
||||
});
|
||||
test.each([
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. Every later read must observe the committed version; this is the accepted consistency model. The stale refill violates that contract.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. Every later read must observe the committed version. This is the accepted consistency model. The stale refill violates that contract.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value, violating the accepted consistency model.',
|
||||
'An in-flight read refills stale data after write invalidation. The accepted consistency model requires a subsequent read to observe the committed version; this violates that contract.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. This is not the accepted consistency model.',
|
||||
'An in-flight read refills stale data after write invalidation. A subsequent read sees the old value. This is not an accepted consistency model.',
|
||||
])('retains an unresolved later-reader violation without demanding a correct remedy: %s', text => {
|
||||
expect(hasStaleFillRaceFinding(text)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -6,6 +6,7 @@ import { designStep0Boundary, hasNativePlanTerminal, nativePlanCallFingerprint,
|
||||
import { isDesignCountFirstReview, isDesignCompletionHandoff, pickDesignCountQuestion } from './helpers/design-count-review';
|
||||
import type { NativePlanQuestionCall } from './helpers/plan-count-transcript';
|
||||
import captured from './fixtures/design-handoff-n-calls.json';
|
||||
import capturedQ from './fixtures/design-handoff-q-calls.json';
|
||||
|
||||
const calls = () => structuredClone(captured.calls) as NativePlanQuestionCall[];
|
||||
const handoff = () => calls().at(-1)!;
|
||||
@@ -136,3 +137,101 @@ describe('scored Design completion and required next gate', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('completed Design review with added decisions and an offered manual stop', () => {
|
||||
const qCalls = () => structuredClone(capturedQ.calls) as NativePlanQuestionCall[];
|
||||
const qHandoff = () => qCalls().at(-1)!;
|
||||
|
||||
test('the actual six calls retain five findings and one completed navigation decision', () => {
|
||||
const input = qCalls();
|
||||
const original = structuredClone(input);
|
||||
let started = false;
|
||||
const counts = { setup: 0, review: 0, administrative: 0 };
|
||||
for (const call of input) {
|
||||
const phase = planCountQuestionPhase(fp(call), started, designStep0Boundary,
|
||||
isDesignCountFirstReview, undefined, isDesignCompletionHandoff);
|
||||
started = phase.reviewStarted;
|
||||
if (phase.administrative) counts.administrative++;
|
||||
else if (phase.preReview) counts.setup++;
|
||||
else counts.review++;
|
||||
}
|
||||
expect(counts).toEqual({ setup: 0, review: 5, administrative: 1 });
|
||||
expect(input.slice(0, -1).every(call => !isDesignCompletionHandoff(fp(call)))).toBe(true);
|
||||
expect(input).toEqual(original);
|
||||
});
|
||||
|
||||
test('pending navigation selects only the offered manual stop in its actual order', () => {
|
||||
for (const reverse of [false, true]) {
|
||||
const call = pending(qHandoff());
|
||||
if (reverse) call.questions[0]!.options.reverse();
|
||||
expect(pickDesignCountQuestion(fp(call), fp(call))).toBe(reverse ? 1 : 3);
|
||||
expect(isDesignCompletionHandoff(fp(call))).toBe(false);
|
||||
expect(pickDesignCountQuestion(fp(call), { ...fp(call), signature: 'foreign:call' })).toBeNull();
|
||||
}
|
||||
const call = pending(qHandoff());
|
||||
call.questions[0]!.options.pop();
|
||||
expect(pickDesignCountQuestion(fp(call), fp(call))).toBeNull();
|
||||
});
|
||||
|
||||
test('the new spelling cannot hide described repairs, unfinished work or conditional closure', () => {
|
||||
const mutations: Array<(call: NativePlanQuestionCall) => void> = [
|
||||
call => { call.questions[0]!.question = call.questions[0]!.question.replace('is complete', 'is not complete'); },
|
||||
call => { call.questions[0]!.question = call.questions[0]!.question.replace('What’s next?', 'Should we add the missing contrast test? What’s next?'); },
|
||||
call => { call.questions[0]!.question = call.questions[0]!.question.replace('What’s next?', 'Once the tests pass, all decisions are resolved. What’s next?'); },
|
||||
call => { call.questions[0]!.options[0]!.description = 'Optional next review.'; },
|
||||
call => { call.questions[0]!.options[2]!.label += ' and fix the missing contrast test'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Proceed to fix the missing contrast test before Eng.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Should we add the missing authorization test before Eng?'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'We could fix the missing authorization test before Eng.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'One contrast gap remains unresolved; handle it manually.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'All decisions will be resolved after the tests pass.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Design review complete after the tests pass.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Design review is not complete.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Not all decisions are resolved.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'The review remains incomplete.'; },
|
||||
call => { call.questions[0]!.options[2]!.description = 'Required gate before shipping. We must repair the missing contrast test.'; },
|
||||
call => { call.questions[0]!.options.push({ label: 'Add a typeface TODO' }); },
|
||||
call => { call.questions[0]!.multiSelect = true; },
|
||||
call => { call.questions.push(qCalls()[0]!.questions[0]!); },
|
||||
call => { call.questions[0]!.question += ' <gstack-qid:plan-design-review-next-steps>'; },
|
||||
];
|
||||
for (const mutate of mutations) {
|
||||
const call = qHandoff();
|
||||
mutate(call);
|
||||
call.answers = Object.fromEntries(call.questions.map(q => [q.question, q.options[0]!.label]));
|
||||
expect(isDesignCompletionHandoff(fp(call))).toBe(false);
|
||||
const active = fp(pending(call));
|
||||
expect(pickDesignCountQuestion(active, active)).toBeNull();
|
||||
}
|
||||
});
|
||||
|
||||
test('only the administrative answer may postdate the actual completed report', () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'design-q-handoff-'));
|
||||
const file = path.join(dir, 'plan.md');
|
||||
try {
|
||||
fs.writeFileSync(file, capturedQ.report.content);
|
||||
const written = Date.parse(capturedQ.report.successfulUpdateAt) / 1000;
|
||||
fs.utimesSync(file, written, written);
|
||||
const input = qCalls();
|
||||
const transcript = { status: 'ready' as const, calls: input, assistantMessages: [],
|
||||
planReadyRequests: structuredClone(capturedQ.planReadyRequests) };
|
||||
const administrative = new Set(input.filter(c => isDesignCompletionHandoff(fp(c))).map(c => fp(c).signature));
|
||||
const started = Date.parse('2026-09-09T03:25:54Z');
|
||||
expect(Date.parse(input.at(-2)!.answeredAt!)).toBeLessThan(written * 1000);
|
||||
expect(Date.parse(input.at(-1)!.answeredAt!)).toBeGreaterThan(written * 1000);
|
||||
expect(hasNativePlanTerminal(transcript, file, started, 'plan_ready')).toBe(false);
|
||||
expect(hasNativePlanTerminal(transcript, file, started, 'plan_ready', administrative)).toBe(true);
|
||||
transcript.planReadyRequests[0]!.failed = true;
|
||||
expect(hasNativePlanTerminal(transcript, file, started, 'plan_ready', administrative)).toBe(false);
|
||||
transcript.planReadyRequests[0]!.failed = false;
|
||||
const stale = Date.parse(input.at(-2)!.answeredAt!) / 1000 - 1;
|
||||
fs.utimesSync(file, stale, stale);
|
||||
expect(hasNativePlanTerminal(transcript, file, started, 'plan_ready', administrative)).toBe(false);
|
||||
fs.utimesSync(file, written, written);
|
||||
fs.writeFileSync(file, '# Incomplete report\n');
|
||||
expect(hasNativePlanTerminal(transcript, file, started, 'plan_ready', administrative)).toBe(false);
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
+32
@@ -0,0 +1,32 @@
|
||||
{
|
||||
"sessionId": "ff398bfc-d56d-4244-87b4-f77d45bda139",
|
||||
"toolUseId": "toolu_01SZEBqEx2x5dEeYCYk2Wz9Z",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D1 \u2014 Which implementation approach should this plan use?\n\nProject/branch: Payment Processing Integration on main branch.\nELI10: The plan describes adding a Stripe webhook handler, but it has three serious problems baked in: it injects user IDs directly into raw SQL (classic SQL injection vulnerability), it calls the notification email inline with zero error handling (an email failure will cause Stripe to retry the payment event and potentially double-notify), and it has no tests. We have three paths: patch just the SQL hole, fix all three P1 issues together, or fix everything plus build a production-grade test suite and async email system. Which level of correctness do we target?\nStakes if we pick wrong: Approach A ships with broken email error handling \u2014 a transient SMTP failure causes Stripe to retry the payment event, potentially confusing the dedup guard. Approach C introduces an async job queue that may not be available yet, delaying the feature.\nRecommendation: B because it fixes all three P1 issues (SQL injection, email error handling, test coverage) without introducing new infrastructure dependencies.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-approach>",
|
||||
"header": "Approach",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Minimal Patch \u2014 fix SQL injection only",
|
||||
"description": "Parameterize the user ID query. Ship everything else (no email error handling, no tests, N+1 query, bypasses WebhookDispatcher) as-is. Human: ~1h / CC: ~5min. Risk: Med."
|
||||
},
|
||||
{
|
||||
"label": "B) Secure Baseline \u2014 fix all P1 issues (Recommended)",
|
||||
"description": "Parameterized query + email error handling + integrate with WebhookDispatcher + fix N+1 + happy-path integration tests. Human: ~1 day / CC: ~30min. Risk: Low."
|
||||
},
|
||||
{
|
||||
"label": "C) Full Production Grade \u2014 B plus async email + full test suite",
|
||||
"description": "Everything in B, plus async email queue (Stripe retry-safe), full unit/integration test suite, metrics and alerting. Human: ~2-3 days / CC: ~1h. Risk: Low but introduces async job queue dependency."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D1 \u2014 Which implementation approach should this plan use?\n\nProject/branch: Payment Processing Integration on main branch.\nELI10: The plan describes adding a Stripe webhook handler, but it has three serious problems baked in: it injects user IDs directly into raw SQL (classic SQL injection vulnerability), it calls the notification email inline with zero error handling (an email failure will cause Stripe to retry the payment event and potentially double-notify), and it has no tests. We have three paths: patch just the SQL hole, fix all three P1 issues together, or fix everything plus build a production-grade test suite and async email system. Which level of correctness do we target?\nStakes if we pick wrong: Approach A ships with broken email error handling \u2014 a transient SMTP failure causes Stripe to retry the payment event, potentially confusing the dedup guard. Approach C introduces an async job queue that may not be available yet, delaying the feature.\nRecommendation: B because it fixes all three P1 issues (SQL injection, email error handling, test coverage) without introducing new infrastructure dependencies.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-approach>": "A) Minimal Patch \u2014 fix SQL injection only"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:30:06.979Z"
|
||||
}
|
||||
+32
@@ -0,0 +1,32 @@
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_01RgdrdtYEuN2DGi3372EaTU",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D3 \u2014 Which implementation approach for the processPayment() tests?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan calls for 2 unit tests. The question is how precise those tests should be. A loose test says \u201cassert a receipt came back\u201d; a tight test says \u201cassert receipt.amount == 50, receipt.charge_id == mock_id, and Stripe was called exactly once.\u201d Tight tests catch more regressions but take slightly more time to write.\nStakes if we pick wrong: Loose assertions let partial regressions ship undetected \u2014 the payment goes through but the receipt is wrong. Missing call-count assertions mean the retry path could silently stop retrying and the test would still pass.\nRecommendation: B because the mock call history and virtual sleeper already exist to assert exact behavior \u2014 not using them wastes the infrastructure and leaves the retry proof hollow.\nCompleteness: A=5/10, B=7/10, C=9/10\n<gstack-qid:plan-ceo-review-impl-approach>",
|
||||
"header": "Approach",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Minimal as Described",
|
||||
"description": "Add exactly 2 tests per the plan: happy path + 502. Basic assertions only (\u201ccorrect receipt generated\u201d, \u201cfails clean\u201d). Smallest diff. Effort: human ~1 hr / CC ~5 min. Completeness: 5/10."
|
||||
},
|
||||
{
|
||||
"label": "B) 2 Tests + Explicit Assertions (Recommended)",
|
||||
"description": "Same 2 tests but with concrete assertions: receipt field values, Stripe call count = 2, virtual sleeper backoff invocation recorded. Proves retry mechanism actually fired. Effort: human ~1.5 hr / CC ~8 min. Completeness: 7/10."
|
||||
},
|
||||
{
|
||||
"label": "C) Coverage Sweep",
|
||||
"description": "2 primary tests + nil/empty input cases + explicit out-of-scope comment while already in the file. Closes more gaps in one PR. Effort: human ~3 hr / CC ~15 min. Completeness: 9/10."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D3 \u2014 Which implementation approach for the processPayment() tests?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan calls for 2 unit tests. The question is how precise those tests should be. A loose test says \u201cassert a receipt came back\u201d; a tight test says \u201cassert receipt.amount == 50, receipt.charge_id == mock_id, and Stripe was called exactly once.\u201d Tight tests catch more regressions but take slightly more time to write.\nStakes if we pick wrong: Loose assertions let partial regressions ship undetected \u2014 the payment goes through but the receipt is wrong. Missing call-count assertions mean the retry path could silently stop retrying and the test would still pass.\nRecommendation: B because the mock call history and virtual sleeper already exist to assert exact behavior \u2014 not using them wastes the infrastructure and leaves the retry proof hollow.\nCompleteness: A=5/10, B=7/10, C=9/10\n<gstack-qid:plan-ceo-review-impl-approach>": "A) Minimal as Described"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:46.283Z"
|
||||
}
|
||||
+270
@@ -0,0 +1,270 @@
|
||||
{
|
||||
"source": "Q paired first native session81402884; raw timeout remains failed.",
|
||||
"calls": [
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_01Jig6XQTh6w7RPArTTftYU6",
|
||||
"questions": [
|
||||
{
|
||||
"question": "gstack works best when your project's CLAUDE.md includes skill routing rules. Add them? (Note: in plan mode, the CLAUDE.md edit + commit will happen after ExitPlanMode.)",
|
||||
"header": "Skill routing",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add routing rules (Recommended)",
|
||||
"description": "Append standard gstack skill routing rules to CLAUDE.md and commit. This lets gstack auto-invoke the right skill for tasks like bugs, QA, design review, and more."
|
||||
},
|
||||
{
|
||||
"label": "No thanks, manual only",
|
||||
"description": "Skip routing rules. You'll invoke skills manually. This only prompts once per project."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"gstack works best when your project's CLAUDE.md includes skill routing rules. Add them? (Note: in plan mode, the CLAUDE.md edit + commit will happen after ExitPlanMode.)": "Add routing rules (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:08.170Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_017Ba4F6EKQpWtZAww6SRxht",
|
||||
"questions": [
|
||||
{
|
||||
"question": "gstack can search learnings from your other projects on this machine to find patterns that apply here. Enable cross-project learnings?",
|
||||
"header": "Cross-project",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Enable cross-project learnings (Recommended)",
|
||||
"description": "Search learnings across all projects on this machine. Stays local \u2014 no data leaves. Best for solo developers."
|
||||
},
|
||||
{
|
||||
"label": "Keep learnings project-scoped only",
|
||||
"description": "Only search learnings from this project. Safer if you work across multiple client codebases."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"gstack can search learnings from your other projects on this machine to find patterns that apply here. Enable cross-project learnings?": "Enable cross-project learnings (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:20.202Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_01RgdrdtYEuN2DGi3372EaTU",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D3 \u2014 Which implementation approach for the processPayment() tests?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan calls for 2 unit tests. The question is how precise those tests should be. A loose test says \u201cassert a receipt came back\u201d; a tight test says \u201cassert receipt.amount == 50, receipt.charge_id == mock_id, and Stripe was called exactly once.\u201d Tight tests catch more regressions but take slightly more time to write.\nStakes if we pick wrong: Loose assertions let partial regressions ship undetected \u2014 the payment goes through but the receipt is wrong. Missing call-count assertions mean the retry path could silently stop retrying and the test would still pass.\nRecommendation: B because the mock call history and virtual sleeper already exist to assert exact behavior \u2014 not using them wastes the infrastructure and leaves the retry proof hollow.\nCompleteness: A=5/10, B=7/10, C=9/10\n<gstack-qid:plan-ceo-review-impl-approach>",
|
||||
"header": "Approach",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Minimal as Described",
|
||||
"description": "Add exactly 2 tests per the plan: happy path + 502. Basic assertions only (\u201ccorrect receipt generated\u201d, \u201cfails clean\u201d). Smallest diff. Effort: human ~1 hr / CC ~5 min. Completeness: 5/10."
|
||||
},
|
||||
{
|
||||
"label": "B) 2 Tests + Explicit Assertions (Recommended)",
|
||||
"description": "Same 2 tests but with concrete assertions: receipt field values, Stripe call count = 2, virtual sleeper backoff invocation recorded. Proves retry mechanism actually fired. Effort: human ~1.5 hr / CC ~8 min. Completeness: 7/10."
|
||||
},
|
||||
{
|
||||
"label": "C) Coverage Sweep",
|
||||
"description": "2 primary tests + nil/empty input cases + explicit out-of-scope comment while already in the file. Closes more gaps in one PR. Effort: human ~3 hr / CC ~15 min. Completeness: 9/10."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D3 \u2014 Which implementation approach for the processPayment() tests?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan calls for 2 unit tests. The question is how precise those tests should be. A loose test says \u201cassert a receipt came back\u201d; a tight test says \u201cassert receipt.amount == 50, receipt.charge_id == mock_id, and Stripe was called exactly once.\u201d Tight tests catch more regressions but take slightly more time to write.\nStakes if we pick wrong: Loose assertions let partial regressions ship undetected \u2014 the payment goes through but the receipt is wrong. Missing call-count assertions mean the retry path could silently stop retrying and the test would still pass.\nRecommendation: B because the mock call history and virtual sleeper already exist to assert exact behavior \u2014 not using them wastes the infrastructure and leaves the retry proof hollow.\nCompleteness: A=5/10, B=7/10, C=9/10\n<gstack-qid:plan-ceo-review-impl-approach>": "A) Minimal as Described"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:46.283Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_019Q72RAptHiRFcmesFoy4BT",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D4 \u2014 Which review mode for this plan?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: Four review postures are available. This is a targeted test-coverage plan with no production code changes, and you\u2019ve already chosen the minimal approach (2 tests, basic assertions). HOLD SCOPE means we audit those 2 tests rigorously for correctness, edge cases, and assertion gaps. SELECTIVE EXPANSION means we also surface cherry-pick opportunities (more tests, tighter assertions) without automatically adding them.\nStakes if we pick wrong: HOLD SCOPE with a vague plan = ship tests that won\u2019t catch the regressions they\u2019re supposed to catch. EXPANSION on a minimal plan = scope drift when the user wants tight focus.\nRecommendation: HOLD SCOPE because you chose the minimal approach \u2014 maximum rigor on the 2 tests, no scope additions surfaced.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-mode-select>",
|
||||
"header": "Review mode",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "HOLD SCOPE (Recommended)",
|
||||
"description": "Plan scope accepted. Audit the 2 tests with maximum rigor: assertion correctness, failure modes, edge cases, test isolation. No expansions surfaced."
|
||||
},
|
||||
{
|
||||
"label": "SELECTIVE EXPANSION",
|
||||
"description": "Hold the 2-test scope as baseline, but surface cherry-pick opportunities (tighter assertions, additional edge-case tests). You opt in to each one individually. Neutral recommendation posture."
|
||||
},
|
||||
{
|
||||
"label": "SCOPE EXPANSION",
|
||||
"description": "Dream bigger. Propose a more ambitious test suite. Every expansion presented individually for approval. Good if you want to think about what comprehensive coverage looks like."
|
||||
},
|
||||
{
|
||||
"label": "SCOPE REDUCTION",
|
||||
"description": "Find the minimum version that ships value. Use if the plan feels overbuilt or you want to ship faster."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D4 \u2014 Which review mode for this plan?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: Four review postures are available. This is a targeted test-coverage plan with no production code changes, and you\u2019ve already chosen the minimal approach (2 tests, basic assertions). HOLD SCOPE means we audit those 2 tests rigorously for correctness, edge cases, and assertion gaps. SELECTIVE EXPANSION means we also surface cherry-pick opportunities (more tests, tighter assertions) without automatically adding them.\nStakes if we pick wrong: HOLD SCOPE with a vague plan = ship tests that won\u2019t catch the regressions they\u2019re supposed to catch. EXPANSION on a minimal plan = scope drift when the user wants tight focus.\nRecommendation: HOLD SCOPE because you chose the minimal approach \u2014 maximum rigor on the 2 tests, no scope additions surfaced.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-mode-select>": "HOLD SCOPE (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:32:18.385Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_016z6mJWZf6UQei8W95B8zRv",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D5 \u2014 What does processPayment() do when 502 retries are exhausted?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The 502 test asserts the function \u201cfails clean\u201d after the retry is exhausted. But a test is code \u2014 it needs to call something (assert_raises, assert result == :error, etc). The plan doesn\u2019t say what processPayment() actually does when retries run out. Without this, the test author can\u2019t write the assertion.\nStakes if we pick wrong: The test passes vacuously \u2014 it doesn\u2019t assert the failure at all, or asserts the wrong thing, letting silent partial failures through.\nRecommendation: A because naming the exception class makes the assertion concrete and self-documenting.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-failure-contract>",
|
||||
"header": "Failure contract",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Raises an exception (Recommended)",
|
||||
"description": "processPayment() raises a specific exception class (e.g. PaymentGatewayError, StripeTimeoutError) when retries are exhausted. Test asserts: assert_raises(PaymentGatewayError) { processPayment(\u2026) }. Name the class in the plan."
|
||||
},
|
||||
{
|
||||
"label": "B) Returns an error value",
|
||||
"description": "processPayment() returns an error object or nil on failure (e.g. Result.err(:gateway_timeout), nil). Test asserts the return value. Name the value in the plan."
|
||||
},
|
||||
{
|
||||
"label": "C) It\u2019s already defined \u2014 look at the existing code",
|
||||
"description": "The failure contract is already documented or clear from context. The plan implementer knows where to look. No change needed to the plan."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D5 \u2014 What does processPayment() do when 502 retries are exhausted?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The 502 test asserts the function \u201cfails clean\u201d after the retry is exhausted. But a test is code \u2014 it needs to call something (assert_raises, assert result == :error, etc). The plan doesn\u2019t say what processPayment() actually does when retries run out. Without this, the test author can\u2019t write the assertion.\nStakes if we pick wrong: The test passes vacuously \u2014 it doesn\u2019t assert the failure at all, or asserts the wrong thing, letting silent partial failures through.\nRecommendation: A because naming the exception class makes the assertion concrete and self-documenting.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-failure-contract>": "A) Raises an exception (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:33:34.669Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_014fMNmBpU7sYU8bugf2zK8J",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D6 \u2014 How does the 502 test prove retry-with-backoff actually fired?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan\u2019s purpose for the 502 test is to verify that the retry mechanism fires once before failing. But if we only assert that processPayment() raises an exception, a broken implementation that raises immediately on the first 502 (skipping the retry entirely) would also pass. The mock call history and virtual sleeper were built to catch exactly this. Choosing C means the test verifies failure but not retry.\nStakes if we pick wrong: A refactor that accidentally deletes the retry loop would not be caught by this test. The retry mechanism would silently stop working.\nRecommendation: A because it directly proves the retry fired and is the minimum addition needed to validate the spec\u2019s stated requirement.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-retry-proof>",
|
||||
"header": "Retry proof",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Assert Stripe mock called exactly twice (Recommended)",
|
||||
"description": "Add: assert Stripe.mock.call_count == 2. This is a one-line addition to the 502 test using the existing mock call history. Proves both retry fired AND that it stopped after one retry (max_retries=1). Effort: ~2 min."
|
||||
},
|
||||
{
|
||||
"label": "B) Assert virtual sleeper invoked once",
|
||||
"description": "Add: assert virtual_sleeper.call_count == 1. This proves the backoff delay ran, which implies the retry loop executed. Also a one-line addition. Slightly weaker than A (doesn\u2019t verify the second Stripe call happened)."
|
||||
},
|
||||
{
|
||||
"label": "C) Accept the gap \u2014 exception assertion is enough",
|
||||
"description": "The test still has value even without a retry proof. A future refactor that breaks retry would surface in the adapter suite. Document the gap explicitly in the plan."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D6 \u2014 How does the 502 test prove retry-with-backoff actually fired?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: The plan\u2019s purpose for the 502 test is to verify that the retry mechanism fires once before failing. But if we only assert that processPayment() raises an exception, a broken implementation that raises immediately on the first 502 (skipping the retry entirely) would also pass. The mock call history and virtual sleeper were built to catch exactly this. Choosing C means the test verifies failure but not retry.\nStakes if we pick wrong: A refactor that accidentally deletes the retry loop would not be caught by this test. The retry mechanism would silently stop working.\nRecommendation: A because it directly proves the retry fired and is the minimum addition needed to validate the spec\u2019s stated requirement.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-retry-proof>": "A) Assert Stripe mock called exactly twice (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:34:20.869Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_018NcGmJzX3naAi6hHcHY9Cj",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D7 \u2014 TODO: Nil/empty input coverage for processPayment()\nProject: Payment Processing \u2014 Test Coverage (main)\nWhat: Add a test for processPayment(nil) and processPayment({}) \u2014 neither is currently covered by the adapter suite, regression suite, or this plan.\nWhy: processPayment() is called with payment data from user input or upstream services. Nil/empty input is a real production scenario (missing required field, serialization bug). The current plan leaves this path untested anywhere.\nELI10: The happy path and 502 path are getting tests. What happens if the payment data itself is nil or empty? Right now, nobody knows what processPayment() does in that case, and no test would catch a regression there.\nStakes if we pick wrong: A serialization bug that passes nil to processPayment() silently misbehaves in production.\nRecommendation: B because nil/empty input testing is valuable but separate scope from the 2 tests in this PR. It should live in a follow-up.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-todo-nil-input>",
|
||||
"header": "TODO: nil input",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Add to TODOS.md",
|
||||
"description": "Track as a P2 follow-up. Effort: human ~1 hr / CC ~5 min. Scope: 1-2 new test cases for nil and empty input."
|
||||
},
|
||||
{
|
||||
"label": "B) Skip \u2014 not in scope for this PR (Recommended)",
|
||||
"description": "This is clean deferred scope. The current plan is focused on happy path + 502. Document the gap in the plan\u2019s \u2018NOT in scope\u2019 section but don\u2019t create a TODO."
|
||||
},
|
||||
{
|
||||
"label": "C) Add to this PR",
|
||||
"description": "Include nil/empty input test cases in this PR alongside the other two tests. Slight scope expansion."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D7 \u2014 TODO: Nil/empty input coverage for processPayment()\nProject: Payment Processing \u2014 Test Coverage (main)\nWhat: Add a test for processPayment(nil) and processPayment({}) \u2014 neither is currently covered by the adapter suite, regression suite, or this plan.\nWhy: processPayment() is called with payment data from user input or upstream services. Nil/empty input is a real production scenario (missing required field, serialization bug). The current plan leaves this path untested anywhere.\nELI10: The happy path and 502 path are getting tests. What happens if the payment data itself is nil or empty? Right now, nobody knows what processPayment() does in that case, and no test would catch a regression there.\nStakes if we pick wrong: A serialization bug that passes nil to processPayment() silently misbehaves in production.\nRecommendation: B because nil/empty input testing is valuable but separate scope from the 2 tests in this PR. It should live in a follow-up.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-todo-nil-input>": "A) Add to TODOS.md"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:34:50.993Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_01RxLLr5huJVqrxQpYXG299k",
|
||||
"questions": [
|
||||
{
|
||||
"question": "D8 \u2014 What's next after this CEO review?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: CEO review is done and clear. Eng Review is the required shipping gate \u2014 it covers architecture, code quality, tests, and performance at the diff level. Since this is a tests-only PR with 0 critical gaps from the CEO review, it\u2019s a lightweight eng review.\nRecommendation: A because Eng Review is required before shipping and this PR is small enough that it should be fast.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-next-step>",
|
||||
"header": "Next step",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "A) Run /plan-eng-review next (Recommended)",
|
||||
"description": "Eng Review is the required shipping gate. Covers architecture, code quality, tests, performance at the diff level. Fast for a tests-only PR."
|
||||
},
|
||||
{
|
||||
"label": "B) Skip \u2014 I\u2019ll handle reviews manually",
|
||||
"description": "Proceed to implementation. Run /plan-eng-review manually later or skip it (set skip_eng_review=true to suppress the prompt)."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"D8 \u2014 What's next after this CEO review?\nProject: Payment Processing \u2014 Test Coverage (main)\nELI10: CEO review is done and clear. Eng Review is the required shipping gate \u2014 it covers architecture, code quality, tests, and performance at the diff level. Since this is a tests-only PR with 0 critical gaps from the CEO review, it\u2019s a lightweight eng review.\nRecommendation: A because Eng Review is required before shipping and this PR is small enough that it should be fast.\nNote: options differ in kind, not coverage \u2014 no completeness score.\n<gstack-qid:plan-ceo-review-next-step>": "A) Run /plan-eng-review next (Recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:41:08.653Z"
|
||||
}
|
||||
],
|
||||
"planReadyRequests": [
|
||||
{
|
||||
"sessionId": "81402884-47d1-4487-b313-43154021bd52",
|
||||
"toolUseId": "toolu_01MzvNML9K14bHKguLY4YKT5",
|
||||
"timestamp": "2026-09-09T03:41:20.539Z",
|
||||
"failed": false
|
||||
}
|
||||
],
|
||||
"reportContent": "# Plan Review: Payment Processing \u2014 Test Coverage\n\n## Context\n\nThe team identified a test coverage gap in `processPayment()`: neither the happy path (successful charge \u2192 receipt) nor the 502 error path (retry-with-backoff \u2192 clean failure) has a unit test. The plan proposes exactly two targeted tests reusing existing test infrastructure. This review evaluates the plan with HOLD SCOPE rigor, surfacing concrete gaps in assertion specificity and failure contract definition.\n\n## System Audit\n\n- Fixture repo: single seed commit, no production source files visible\n- No design doc, no handoff note, no prior learnings\n- Plan is clean-scope: unit tests only, no production code changes\n- Existing infrastructure correctly identified: payment test factory (max_retries=1, Stripe mock call history), virtual sleeper, Stripe adapter suite, receipt-builder regression tests\n\n## Step 0: Scope Analysis\n\n### 0A. Premise Challenge\n\nRight problem: YES. Untested payment code is a genuine risk. The 502 retry path is hard to catch manually and fragile in refactors. The business outcome is confidence that `processPayment()` is correct on the happy path and degrades gracefully on infrastructure errors. The \"do nothing\" cost is silent regressions in critical payment flows.\n\n### 0B. Existing Code Leverage\n\nThe plan already identifies reusable infrastructure correctly. No parallel implementations needed. The mock call history and virtual sleeper are available and should be used for retry-proof assertions.\n\n### 0C. Dream State\n\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\nNo unit tests --> 2 targeted tests: --> Full processPayment()\nfor processPayment - happy path coverage including:\n - 502 error path - nil/invalid inputs\n - all error codes (402, 429, 503)\n - receipt field contract\n - idempotency key behavior\n```\n\n### 0C-bis. Implementation Approach Selected\n\n**User chose Approach A: Minimal as Described** (Completeness: 5/10)\n- 2 tests as specified, basic assertions\n- gstack-shortcut(dec-impl-approach-1): ceiling 5/10, upgrade when another developer touches processPayment() tests and needs stronger regression guarantees\n\n*Note: This review adds two specific assertions beyond \"minimal\" through D5 and D6 (exception class + call count) that are required for the tests to verify the stated behaviors.*\n\n### 0F. Mode Selected: HOLD SCOPE\n\nTest-coverage plan for existing code, no production changes. Maximum rigor on the 2 tests, no scope additions.\n\n## Key Decisions Made During Review\n\n| ID | Question | Decision |\n|----|----------|----------|\n| D5 | Failure contract for 502 path | Raises exception (specific class TBD from codebase) |\n| D6 | Retry proof assertion | Assert Stripe.mock.call_count == 2 |\n| D7 | Nil/empty input coverage | Add to TODOS.md (P2 follow-up) |\n\n## 11-Section Deep Review\n\n### Section 1: Architecture\n\nTests-only change. No new components, coupling changes, state machines, or integrations. Rollback: git revert of the test file. **No issues.**\n\n### Section 2: Error & Rescue Map\n\n**Finding (resolved via D5):** The plan states \"fails clean\" after exhausted retries but does not define the failure contract. A test cannot assert \"fails clean\" without knowing whether processPayment() raises an exception or returns an error value.\n\n**Resolution (D5):** `processPayment()` raises an exception when 502 retries are exhausted. The plan must name the specific exception class (e.g., `PaymentGatewayError`, `StripeTimeoutError` \u2014 look up in production code).\n\n```\nMETHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS\n--------------------------|----------------------------|------------------\nprocessPayment() | Stripe returns 502 | [named in codebase]\n | Retries exhausted (2 calls) | PaymentGatewayError (TBD)\n\nEXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES\n--------------------------|----------|---------------------|----------\n502 StripeError | YES | Retry once (via | Exception raised\n | | max_retries=1), | to caller\n | | then raise\n```\n\n### Section 3: Security\n\nTests only. No new attack surface, user input, data access, secrets, or dependencies. **No issues.**\n\n### Section 4: Data Flow & Interaction Edge Cases\n\nShadow paths for the happy path (nil input, empty input) are explicitly out of scope per HOLD SCOPE. The receipt_builder integration (mocked vs real) is an implementation detail; the implementer should use the same pattern as adjacent tests.\n\n```\nHAPPY PATH:\n processPayment(valid_payment) \u2192 Stripe mock (200) \u2192 receipt Line truncated
|
||||
"reportAt": "2026-09-09T03:40:08.873Z",
|
||||
"sourceProofSha256": "1119067252a5308498a10a2728692ef99fd75464506983f99eaee9fe3a358454",
|
||||
"reportWrite": {
|
||||
"toolUseId": "toolu_01JBdaKiZgYNaxb2hHS42fBD",
|
||||
"calledAt": "2026-09-09T03:40:07.366Z",
|
||||
"resultAt": "2026-09-09T03:40:08.873Z",
|
||||
"failed": false
|
||||
}
|
||||
}
|
||||
+33
@@ -0,0 +1,33 @@
|
||||
{
|
||||
"case": "hold-prerequisite-q",
|
||||
"nativeSourceSha256": "572f4d37176faa79f5eb44524cd5cd838c0e7b84a648c2b5092b9effe4e2a185",
|
||||
"requestLine": 56,
|
||||
"resultLine": 57,
|
||||
"call": {
|
||||
"sessionId": "ce0d0a04-8358-47fe-97c7-fae5207e3980",
|
||||
"toolUseId": "toolu_01UGxtFGAXCKRLwP6RErA4km",
|
||||
"requestedAt": "2026-09-09T03:30:34.803Z",
|
||||
"answered": true,
|
||||
"answeredAt": "2026-09-09T03:30:34.847Z",
|
||||
"questions": [
|
||||
{
|
||||
"question": "No design doc found for this branch. Run /office-hours first for sharper review input, or proceed directly with the CEO review?",
|
||||
"header": "Design doc",
|
||||
"options": [
|
||||
{
|
||||
"label": "Run /office-hours first",
|
||||
"description": "Produces a structured problem statement, premise challenge, and explored alternatives \u2014 gives this review much sharper input. Takes ~10 minutes."
|
||||
},
|
||||
{
|
||||
"label": "Proceed with standard review",
|
||||
"description": "Skip /office-hours and review the plan as-is. Recommended when the problem statement is already clear and the plan is ready to interrogate. (recommended)"
|
||||
}
|
||||
],
|
||||
"multiSelect": false
|
||||
}
|
||||
],
|
||||
"answers": {
|
||||
"No design doc found for this branch. Run /office-hours first for sharper review input, or proceed directly with the CEO review?": "Run /office-hours first"
|
||||
}
|
||||
}
|
||||
}
|
||||
+866
@@ -0,0 +1,866 @@
|
||||
# Plan: cache profile summaries in one process
|
||||
|
||||
## Measured problem and accepted scope
|
||||
The existing profile-summary service has one active process. A one-week trace
|
||||
shows repeated reads of about 900 hot keys: DB CPU is 70%, with read p95 120 ms.
|
||||
Add a process-local LRU wrapper to the existing repository. Acceptance targets
|
||||
are at least 60% cache hits, DB CPU below 50%, and read p95 below 60 ms, with the
|
||||
existing error-rate and correctness SLOs unchanged. This is an internal backend
|
||||
change with no UI, API, schema, pricing, or developer onboarding change.
|
||||
|
||||
## Existing contracts retained
|
||||
- All reads and writes use this repository in the same process; there are no
|
||||
external DB writers. Multi-process operation remains unsupported and startup
|
||||
rejects that configuration while caching is enabled.
|
||||
- Authentication and authorization run before repository access. Keys encode
|
||||
the authenticated tenant ID and validated profile ID without ambiguity.
|
||||
Values are immutable profile-summary DTOs; secrets and cache keys are never
|
||||
logged. Cached results cannot bypass authorization.
|
||||
- The existing LRU adapter supports 1000 entries, a 16 MiB byte cap, and a
|
||||
30-second TTL. Recorded hot data fits those limits. Absent records use a
|
||||
distinct sentinel with a 10-second TTL; undefined means a cache miss.
|
||||
- Cache operations are synchronous and atomic in the single JS event loop.
|
||||
On any cache failure the existing adapter bypasses the cache until an empty
|
||||
cache is reinitialized; repository errors keep the current typed API error
|
||||
mapping. The existing per-key
|
||||
single-flight wrapper coalesces simultaneous misses and releases on failure.
|
||||
- **AMENDED (stale-fill race):** A read already in progress when a write commits
|
||||
may return its earlier DB snapshot to that caller AND may fill the cache with
|
||||
that stale snapshot. The stale entry remains visible to subsequent reads until
|
||||
TTL expiry — at most 30 seconds for a present record, 10 seconds for an absent
|
||||
sentinel. Every read begun after the cached stale entry expires must observe the
|
||||
committed version. This bounded inconsistency window is the accepted consistency
|
||||
model for this in-process cache; TTL is the consistency deadline.
|
||||
|
||||
## Proposed wrapper integration
|
||||
Keep the current read-through repository interface and shared adapters. These
|
||||
are the complete new read/write ordering rules. Cache population and invalidation
|
||||
are best-effort: errors in cache operations are caught, logged, and trigger bypass
|
||||
mode; they never propagate to the caller when the underlying repository operation
|
||||
succeeded.
|
||||
|
||||
```javascript
|
||||
async function readProfile(key) {
|
||||
const cached = cache.get(key);
|
||||
if (cached !== undefined) return cached;
|
||||
const value = await repository.read(key);
|
||||
// best-effort: a cache.set failure must not fail a successful read
|
||||
try { cache.set(key, value); } catch (e) { log.warn('cache.set failed', { err: e }); }
|
||||
return value;
|
||||
}
|
||||
|
||||
async function writeProfile(key, update) {
|
||||
const saved = await repository.write(key, update);
|
||||
// best-effort: a cache.delete failure is logged; bypass mode activates until cache reinits
|
||||
try { cache.delete(key); } catch (e) { log.error('cache.delete failed after write', { err: e }); }
|
||||
return saved;
|
||||
}
|
||||
```
|
||||
|
||||
## Verification and rollout
|
||||
Existing repository contract tests cover tenant isolation, key validation,
|
||||
absence, DB failures, and authorization. New wrapper tests cover hit/miss,
|
||||
eviction and byte limits, TTL, adapter-failure fallback, successful-write
|
||||
invalidation, failed-write preservation, concurrent-miss coalescing,
|
||||
**cache.set failure during a successful read (read must still return value)**,
|
||||
**and cache.delete failure during a successful write (write must still return saved)**.
|
||||
The rollout uses the existing runtime feature flag: enable for 10% of keys,
|
||||
then 50%, then all keys after one healthy hour at each stage. "Healthy" means:
|
||||
hit rate ≥ 60%, fallback-error rate < 0.1%, read p95 < 80 ms (relaxed during
|
||||
ramp; final target is 60 ms at full rollout). Monitor hit/miss, eviction, cache
|
||||
bytes, fallback errors, DB CPU, and read p95 without raw IDs.
|
||||
On error-rate or latency regression, disable the flag immediately; both reads
|
||||
and writes bypass the cache while disabled, and enabling creates an empty cache.
|
||||
Cold starts remain within the existing DB capacity. The service owner monitors
|
||||
the rollout and records the results against the acceptance targets.
|
||||
|
||||
## Out of scope
|
||||
Distributed caching, cross-process coherence, prewarming, changing consistency
|
||||
semantics, or adding new product surfaces. The repository interface preserves a
|
||||
future replacement path without introducing a general cache framework now.
|
||||
|
||||
---
|
||||
|
||||
## CEO Review — HOLD SCOPE
|
||||
|
||||
**Mode:** HOLD SCOPE (auto-selected: well-scoped performance fix, backend-only,
|
||||
no UI, minimal file surface, feature-flag rollback available)
|
||||
|
||||
**Approach selected (auto):** The plan as written — wrap the existing repository
|
||||
interface with a thin LRU layer using existing adapters. Minimal viable and ideal
|
||||
architecture converge here; there is no meaningful tradeoff between them for this
|
||||
scope.
|
||||
|
||||
---
|
||||
|
||||
### Step 0: Scope Challenge
|
||||
|
||||
#### 0A. Premise Challenge
|
||||
|
||||
Right problem: yes. The one-week trace with measured DB CPU at 70% and p95 at
|
||||
120 ms on 900 stable hot keys is real signal. A process-local LRU read-through
|
||||
cache is the direct solution.
|
||||
|
||||
Most direct path: yes. The plan reuses existing LRU adapter, existing feature
|
||||
flag, existing single-flight wrapper. No new infrastructure is introduced.
|
||||
|
||||
Do-nothing cost: DB continues at 70% CPU with 120 ms reads. If query volume grows,
|
||||
this becomes a blocking constraint. Real pain.
|
||||
|
||||
#### 0B. Existing Code Leverage
|
||||
|
||||
Everything substantial is reused:
|
||||
- LRU adapter (1000 entries, 16 MiB cap, TTL) — pre-built
|
||||
- Per-key single-flight wrapper — pre-built, handles concurrent-miss thundering herd
|
||||
- Runtime feature flag — pre-built, enables phased rollout and instant rollback
|
||||
- Existing repository contract tests — cover key validation, tenant isolation, absence, DB failures
|
||||
|
||||
The plan does not rebuild anything that already exists.
|
||||
|
||||
#### 0C. Dream State Mapping
|
||||
|
||||
```
|
||||
CURRENT STATE THIS PLAN 12-MONTH IDEAL
|
||||
DB CPU 70% ---> DB CPU <50% ---> DB CPU stable <40%
|
||||
read p95 120ms read p95 <60ms read p95 <40ms (with
|
||||
900 hot keys, 900 hot keys cached warmer hit rate)
|
||||
no caching ~60%+ hit rate Clear path to
|
||||
Feature flag rollback distributed cache
|
||||
when multi-process
|
||||
is needed
|
||||
```
|
||||
|
||||
This plan moves in the right direction. It does not close the multi-process gap
|
||||
(correctly out of scope) but explicitly preserves the replacement path.
|
||||
|
||||
#### 0C-bis. Implementation Alternatives (HOLD SCOPE, informational)
|
||||
|
||||
**APPROACH A: Thin wrapper (this plan)**
|
||||
Summary: Wrap existing repository in a read-through LRU cache using existing adapter.
|
||||
Effort: S | Risk: Low
|
||||
Pros: Reuses existing adapter, zero new infrastructure, instant rollback via flag
|
||||
Cons: Single-process only, stale-fill race (documented in amended plan)
|
||||
Reuses: LRU adapter, single-flight wrapper, feature flag, contract tests
|
||||
|
||||
**APPROACH B: Precomputed projection layer**
|
||||
Summary: Compute and store profile summaries at write time, read from a summary
|
||||
table instead of computing on the fly.
|
||||
Effort: L | Risk: High
|
||||
Pros: No cache coherence issues, reads always fresh, better for multi-process later
|
||||
Cons: Schema migration, write-path complexity, out of scope by definition
|
||||
Not selected — correct exclusion.
|
||||
|
||||
**APPROACH C: External cache (Redis)**
|
||||
Summary: Use Redis as a shared cache across processes.
|
||||
Effort: L | Risk: Med
|
||||
Pros: Multi-process capable, no single-process constraint
|
||||
Cons: New infrastructure dependency, distributed consistency complexity, out of scope
|
||||
Not selected — correct exclusion for single-process constraint.
|
||||
|
||||
**RECOMMENDATION:** Approach A. The plan correctly identifies that the single-process
|
||||
constraint makes in-process caching optimal for now. Approaches B and C solve problems
|
||||
the team does not have yet.
|
||||
|
||||
#### 0D. HOLD SCOPE Analysis
|
||||
|
||||
Complexity check: The plan touches ≤3 files (repository wrapper, tests, possibly
|
||||
a config check for startup rejection). Does not introduce new classes beyond the
|
||||
wrapper. Correctly scoped.
|
||||
|
||||
Minimum change: The read/write pseudocode plus test additions is the minimum. No
|
||||
over-engineering detected.
|
||||
|
||||
#### 0E. Temporal Interrogation
|
||||
|
||||
Key decisions for the implementer:
|
||||
|
||||
```
|
||||
HOUR 1 (foundations):
|
||||
- What is the SENTINEL value? Symbol, null, or a typed object?
|
||||
Must be serializable by the LRU adapter but distinguishable from valid DTOs
|
||||
and from undefined (cache miss). Define it before writing the wrapper.
|
||||
|
||||
HOUR 2-3 (core logic):
|
||||
- How does the LRU adapter signal bypass mode? Does cache.set throw or return false?
|
||||
Does cache.get return undefined in bypass mode (which readProfile treats as a miss)?
|
||||
This must be verified against the adapter's actual API before writing error handling.
|
||||
|
||||
HOUR 4-5 (integration):
|
||||
- The stale-fill race: a concurrent write completing while a read is in-flight
|
||||
(at the `await repository.read()` suspension point) will cause cache.set to
|
||||
store a stale value AFTER cache.delete ran. Verify the amended invariant in
|
||||
the plan matches what the team accepts.
|
||||
|
||||
HOUR 6+ (polish/tests):
|
||||
- Concurrent-miss coalescing tests need controlled pause/release points — real
|
||||
async timing makes them flaky. Use a mock adapter that can be paused mid-read.
|
||||
- The multi-process startup rejection test needs to verify the exact error thrown
|
||||
and that it is unrecoverable (no retry loop).
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Section 1: Architecture Review
|
||||
|
||||
**System architecture:**
|
||||
|
||||
```
|
||||
┌─────────────────────────────────────────────────────────────┐
|
||||
│ Single Process │
|
||||
│ │
|
||||
│ Caller ──▶ ProfileRepository (wrapper) │
|
||||
│ │ │
|
||||
│ ┌─────┴───────────────────────┐ │
|
||||
│ │ LRU Cache Layer │ │
|
||||
│ │ ┌───────────────────────┐ │ │
|
||||
│ │ │ LRU Adapter │ │ │
|
||||
│ │ │ 1000 entries / 16 MiB │ │ │
|
||||
│ │ │ 30s TTL (present) │ │ │
|
||||
│ │ │ 10s TTL (sentinel) │ │ │
|
||||
│ │ └───────────────────────┘ │ │
|
||||
│ │ Single-flight per key │ │
|
||||
│ │ Bypass mode on adapter fail │ │
|
||||
│ └─────────────────────────────┘ │
|
||||
│ │ │
|
||||
│ Existing Repository Layer │
|
||||
│ │ │
|
||||
│ ▼ │
|
||||
│ ┌──────────┐ │
|
||||
│ │ Database │ │
|
||||
│ └──────────┘ │
|
||||
└─────────────────────────────────────────────────────────────┘
|
||||
EXCLUDED: multi-process, external cache, cross-process coherence
|
||||
```
|
||||
|
||||
**Data flow — read path (all four paths):**
|
||||
|
||||
```
|
||||
readProfile(key)
|
||||
│
|
||||
├─▶ cache.get(key)
|
||||
│ │
|
||||
│ ┌────┴──────────────────────────────────────────────┐
|
||||
│ │ HIT (value !== undefined) MISS (undefined) │
|
||||
│ │ │ │ │
|
||||
│ │ return value await repo.read(key) │
|
||||
│ │ [happy path] │ │
|
||||
│ │ ┌────────────────┤ │
|
||||
│ │ │ Success │ Error │
|
||||
│ │ │ │ │
|
||||
│ │ try cache.set propagate err │
|
||||
│ │ catch → log (cache unchanged) │
|
||||
│ │ return value │
|
||||
│ └────────────────────────────────────────────────── ┘
|
||||
│
|
||||
│ nil key → existing key validation rejects before cache
|
||||
│ empty key → same
|
||||
│ cache in bypass mode → cache.get returns undefined → DB read
|
||||
```
|
||||
|
||||
**Data flow — write path (all four paths):**
|
||||
|
||||
```
|
||||
writeProfile(key, update)
|
||||
│
|
||||
├─▶ await repo.write(key, update)
|
||||
│ │
|
||||
│ ┌────┴─────────────────────────────────────┐
|
||||
│ │ Success Failure │
|
||||
│ │ │ │ │
|
||||
│ │ try cache.delete(key) propagate err │
|
||||
│ │ catch → log + bypass (cache unchanged) │
|
||||
│ │ return saved │
|
||||
│ └────────────────────────────────────────── ┘
|
||||
│
|
||||
│ nil/invalid key → repo.write validates, rejects before cache
|
||||
```
|
||||
|
||||
**Cache entry state machine:**
|
||||
|
||||
```
|
||||
[ABSENT/EXPIRED] ──read+DB hit──▶ [CACHED present, 30s TTL]
|
||||
│ │
|
||||
│ write invalidate
|
||||
│◀────────────────────────────────── │
|
||||
│ TTL expiry
|
||||
│◀────────────────────────────────── ┘
|
||||
│
|
||||
├──read+DB absent──▶ [SENTINEL cached, 10s TTL]
|
||||
│ │
|
||||
│ write invalidate / TTL expiry
|
||||
│◀─────────────────────────────┘
|
||||
│
|
||||
└──cache adapter fail──▶ [BYPASS MODE] ──reinit──▶ [ABSENT/EXPIRED]
|
||||
```
|
||||
|
||||
**Coupling:** The cache couples read freshness to write ordering. This coupling is the design's purpose and is justified. No new coupling is introduced between otherwise-independent components.
|
||||
|
||||
**Scaling:**
|
||||
- 10x read load: cache absorbs; hit rate improves; DB load decreases. LRU adapter is synchronous, no lock contention in a single event loop.
|
||||
- 10x write load: cache invalidation pressure grows. With 900 hot keys and a 1000-entry limit, high write throughput could increase effective miss rate. Not a correctness issue, but a performance consideration worth monitoring during rollout.
|
||||
- 100x: Multi-process operation required — explicitly out of scope and blocked at startup.
|
||||
|
||||
**Single points of failure:** The process itself. Cache failure degrades to bypass (existing DB path) — no new SPOF introduced.
|
||||
|
||||
**Security:** Auth boundary is at the repository interface (pre-wrapper). Keys encode tenant ID + profile ID. Cache cannot bypass authorization because cached results are scoped to the same authenticated call path. Secrets not logged.
|
||||
|
||||
**Rollback:** Feature flag → instant disable → reads and writes go directly to DB. No DB migration, no schema change. Rollback is a configuration toggle, not a code deploy. **5/5 reversibility.**
|
||||
|
||||
**Production failure scenarios:**
|
||||
- LRU adapter failure: bypass mode, reads go to DB. DB returns to pre-cache load. Monitor fallback-error rate.
|
||||
- DB failure during cache miss: error propagates, cache not updated. Correct.
|
||||
- Byte-limit eviction under write pressure: LRU evicts, misses go to DB. Graceful degradation, no correctness impact.
|
||||
|
||||
No issues. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 2: Error & Rescue Map
|
||||
|
||||
```
|
||||
METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS
|
||||
--------------------------|----------------------------------|-----------------------
|
||||
readProfile(key) | cache.get: adapter failure | CACHE_ADAPTER_ERROR
|
||||
| repo.read: DB timeout | DB_TIMEOUT
|
||||
| repo.read: DB connection lost | DB_CONNECTION_ERROR
|
||||
| repo.read: key validation fail | INVALID_KEY (existing)
|
||||
| cache.set: OOM / byte-limit full | CACHE_SET_ERROR
|
||||
| cache.set: adapter failure | CACHE_ADAPTER_ERROR
|
||||
--------------------------|----------------------------------|-----------------------
|
||||
writeProfile(key, update) | repo.write: DB timeout | DB_TIMEOUT
|
||||
| repo.write: DB write conflict | DB_CONFLICT (existing)
|
||||
| cache.delete: adapter failure | CACHE_ADAPTER_ERROR
|
||||
--------------------------|----------------------------------|-----------------------
|
||||
startup | multi-process config detected | CONFIG_ERROR (unrecoverable)
|
||||
```
|
||||
|
||||
```
|
||||
EXCEPTION CLASS | RESCUED? | RESCUE ACTION | CALLER SEES
|
||||
----------------------|----------|---------------------------------|---------------------
|
||||
CACHE_ADAPTER_ERROR | Y | Enter bypass mode, log warn | Transparent (DB path)
|
||||
(on cache.get) | | |
|
||||
CACHE_ADAPTER_ERROR | Y | Log error, enter bypass mode | Transparent (DB path)
|
||||
(on cache.delete) | | |
|
||||
CACHE_SET_ERROR | Y | Log warn, return DB value | Correct value returned
|
||||
DB_TIMEOUT | N | Propagate (existing handler) | Existing typed error
|
||||
DB_CONNECTION_ERROR | N | Propagate (existing handler) | Existing typed error
|
||||
INVALID_KEY | N | Propagate (existing handler) | Existing typed error
|
||||
DB_CONFLICT | N | Propagate (existing handler) | Existing typed error
|
||||
CONFIG_ERROR | N | Reject startup (unrecoverable) | Process won't start
|
||||
```
|
||||
|
||||
No catch-all error handling. Every cache operation error is caught specifically,
|
||||
logged, and either triggers bypass mode or allows the caller's successful result
|
||||
through. DB errors propagate via the existing typed error mapping unchanged.
|
||||
|
||||
No critical gaps. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 3: Security & Threat Model
|
||||
|
||||
| Threat | Likelihood | Impact | Mitigated? |
|
||||
|--------|-----------|--------|-----------|
|
||||
| Cache bypass to access another tenant's data | Low | High | YES — keys encode authenticated tenant ID; auth runs before cache |
|
||||
| Stale read exposing a deleted/updated record | Med | Low | PARTIAL — TTL bounds to 30s; documented as accepted |
|
||||
| Cache key collision between tenants | Low | High | YES — key encoding is unambiguous per plan contract |
|
||||
| Memory exhaustion via cache growth | Low | Med | YES — 16 MiB byte cap + LRU eviction |
|
||||
| Secret leakage through cache key logging | Low | High | YES — secrets and raw IDs never logged |
|
||||
| Multi-process coherence bypass | Low | High | YES — startup rejects multi-process config |
|
||||
| Side-channel timing (hit vs miss latency) | Low | Low | Acceptable — attacker needs valid credentials anyway |
|
||||
|
||||
No new endpoints, no new params, no new file paths, no new external-facing API surface.
|
||||
No new dependencies. Values are immutable DTOs — no mutation risk from cached reads.
|
||||
|
||||
No issues. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 4: Data Flow & Interaction Edge Cases
|
||||
|
||||
**Full read-path trace with shadow nodes:**
|
||||
|
||||
```
|
||||
readProfile(key)
|
||||
│
|
||||
├─[nil key]────▶ key validation → INVALID_KEY error
|
||||
├─[empty key]──▶ key validation → INVALID_KEY error
|
||||
│
|
||||
▼
|
||||
cache.get(key)
|
||||
│
|
||||
├─[cache hit: value]──▶ return value (happy path, no DB)
|
||||
├─[cache hit: SENTINEL]─▶ return SENTINEL (DB absent, still valid)
|
||||
├─[cache miss: bypass]──▶ falls through to repo.read
|
||||
│
|
||||
▼ (cache miss or bypass)
|
||||
repo.read(key) ← SUSPENSION POINT (await)
|
||||
│
|
||||
│ ← concurrent write can complete here (cache.delete runs)
|
||||
│
|
||||
├─[DB hit]────▶ try cache.set(key, value)
|
||||
│ ├─[set succeeds]────▶ return value ✓
|
||||
│ └─[set fails]────▶ log warn, return value ✓
|
||||
│ (stale-fill race: write may have deleted this key
|
||||
│ between suspension and set; set re-populates with
|
||||
│ stale value; expires in ≤30s per amended invariant)
|
||||
│
|
||||
├─[DB absent]─▶ try cache.set(key, SENTINEL)
|
||||
│ same stale-fill race applies; expires in ≤10s
|
||||
│
|
||||
└─[DB error]──▶ propagate; cache unchanged ✓
|
||||
```
|
||||
|
||||
**Async ordering at suspension points:**
|
||||
|
||||
At `await repository.read(key)`, the event loop is free to process other tasks.
|
||||
Specifically, a `writeProfile(key, update)` can complete in this window, including
|
||||
its `cache.delete(key)`. When the read resumes, its `cache.set(key, stale_value)`
|
||||
then overwrites the (now-absent) cache entry with a stale value.
|
||||
|
||||
The single-flight wrapper coalesces concurrent reads for the same key — it does
|
||||
NOT prevent the read/write race. Per the amended invariant, this is accepted with
|
||||
the TTL as the consistency deadline.
|
||||
|
||||
**Backend-only plan — no UI interaction edge cases to map.**
|
||||
|
||||
No unhandled edge cases beyond the stale-fill race, which is now documented as
|
||||
accepted behavior with a bounded window.
|
||||
|
||||
---
|
||||
|
||||
### Section 5: Code Quality Review
|
||||
|
||||
**Original pseudocode issue — cache.set and cache.delete not wrapped:**
|
||||
The original wrapper pseudocode allowed cache.set to throw and propagate to the
|
||||
caller (failing a successful DB read), and allowed cache.delete to throw and
|
||||
propagate (failing a successful DB write). Both are now corrected in the amended
|
||||
wrapper (see Proposed wrapper integration above).
|
||||
|
||||
**SENTINEL definition:** The plan describes the sentinel abstractly. Implementation
|
||||
must define it concretely — a Symbol or typed singleton is idiomatic in JS. It must
|
||||
be: not undefined (or cache.get would treat it as a miss), not a valid profile DTO
|
||||
(or callers would misinterpret it), and serializable by the LRU adapter if the
|
||||
adapter persists to disk. This decision belongs in the implementation, not the plan,
|
||||
but the implementer must make it at hour 1.
|
||||
|
||||
**DRY:** No repetition. The cache wrapper is the single point of change. Existing
|
||||
repository and adapter code unchanged.
|
||||
|
||||
**Naming:** readProfile / writeProfile are clear and action-oriented.
|
||||
|
||||
**Cyclomatic complexity:** Both functions branch ≤2 times. No refactor needed.
|
||||
|
||||
**Over/under-engineering:** Correctly scoped. The try/catch additions are not
|
||||
over-engineering — they prevent a cache failure from breaking an otherwise-successful
|
||||
repository operation.
|
||||
|
||||
No further issues. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 6: Test Review
|
||||
|
||||
**New codepaths introduced:**
|
||||
|
||||
```
|
||||
NEW CODEPATHS:
|
||||
Read path: cache hit (present value)
|
||||
Read path: cache hit (absent sentinel)
|
||||
Read path: cache miss → DB hit → cache.set → return value
|
||||
Read path: cache miss → DB absent → sentinel fill → return sentinel
|
||||
Read path: cache miss → DB error → propagate
|
||||
Read path: cache.set failure → log, return DB value (NEW REQUIREMENT)
|
||||
Read path: cache.get in bypass mode → falls through to DB
|
||||
Write path: DB success → cache.delete → return saved
|
||||
Write path: DB failure → cache unchanged → propagate
|
||||
Write path: cache.delete failure → log, return saved (NEW REQUIREMENT)
|
||||
Concurrent miss on same key → single-flight coalesces to one DB read
|
||||
TTL expiry (30s present, 10s sentinel) → subsequent read hits DB
|
||||
Byte-limit eviction → LRU evicts least-recently-used, subsequent miss hits DB
|
||||
Feature flag: enabled → cache mode active
|
||||
Feature flag: disabled → full bypass, reads and writes go to DB
|
||||
Startup: multi-process config + caching enabled → reject with CONFIG_ERROR
|
||||
|
||||
NEW ERROR/RESCUE PATHS:
|
||||
CACHE_ADAPTER_ERROR on cache.get → bypass mode (see Section 2)
|
||||
CACHE_ADAPTER_ERROR on cache.delete → log + bypass (see Section 2)
|
||||
CACHE_SET_ERROR on cache.set → log + return value (see Section 2)
|
||||
```
|
||||
|
||||
**Test coverage map:**
|
||||
|
||||
| Codepath | Type | Exists in plan? | Happy path | Failure path | Edge case |
|
||||
|----------|------|----------------|------------|--------------|-----------|
|
||||
| cache hit (present) | Unit | YES | return value | — | hit after eviction |
|
||||
| cache hit (sentinel) | Unit | YES | return sentinel | — | sentinel after 10s expiry |
|
||||
| cache miss → DB hit → set | Unit | YES | fill + return | set failure (NEW) | concurrent miss coalesced |
|
||||
| cache miss → DB error | Unit | YES | — | propagate | — |
|
||||
| write → cache.delete | Unit | YES | invalidate | delete failure (NEW) | write during in-flight read |
|
||||
| eviction / byte limits | Unit | YES | evict LRU | — | at exactly 1000 entries |
|
||||
| TTL expiry | Unit | YES | re-fetch | — | expiry during in-flight |
|
||||
| adapter fallback | Unit | YES | bypass mode | — | bypass then reinit |
|
||||
| concurrent miss coalescing | Integration | YES | single DB read | — | controlled pause/release |
|
||||
| feature flag disabled | Integration | YES | full bypass | — | mid-rollout disable |
|
||||
| startup rejection (multi-process) | Integration | NOT IN PLAN | reject startup | — | — |
|
||||
| cache.set failure (read returns value) | Unit | NOT IN PLAN | — | value returned | — |
|
||||
| cache.delete failure (write returns saved) | Unit | NOT IN PLAN | — | saved returned | — |
|
||||
|
||||
**Three gaps require new test specs:**
|
||||
|
||||
1. `cache.set failure during successful read`:
|
||||
```
|
||||
GIVEN: DB read succeeds AND cache.set throws CACHE_SET_ERROR
|
||||
THEN: readProfile returns the DB value (not the cache error)
|
||||
AND: the error is logged at warn level
|
||||
AND: bypass mode is entered
|
||||
```
|
||||
|
||||
2. `cache.delete failure during successful write`:
|
||||
```
|
||||
GIVEN: DB write succeeds AND cache.delete throws CACHE_ADAPTER_ERROR
|
||||
THEN: writeProfile returns the saved value (not the cache error)
|
||||
AND: the error is logged at error level
|
||||
AND: bypass mode is entered
|
||||
```
|
||||
|
||||
3. `multi-process startup rejection`:
|
||||
```
|
||||
GIVEN: service config detects multi-process + caching enabled at startup
|
||||
THEN: process startup throws CONFIG_ERROR
|
||||
AND: the error message names the conflicting configuration
|
||||
AND: no partial initialization occurs
|
||||
```
|
||||
|
||||
**Flakiness risk:** Concurrent-miss coalescing tests must use controlled
|
||||
pause/release points on the mock repository adapter — real async timing is
|
||||
non-deterministic and will produce flaky CI results.
|
||||
|
||||
**Test pyramid:** Unit-heavy (correct for a wrapper), integration for flag and
|
||||
concurrent scenarios, no E2E needed (internal backend, no user-facing surface).
|
||||
|
||||
**Chaos test:** Feature flag disabled mid-rollout while reads and writes are
|
||||
in flight. All in-flight reads complete via DB; no stale cache entry is observed
|
||||
after flag-off. This validates the bypass semantics.
|
||||
|
||||
**Friday-night confidence test:** A write followed immediately by a read on the
|
||||
same key returns the committed version — either because the cache was invalidated
|
||||
before the read ran, or because the stale entry expired within 30 seconds. The
|
||||
latter is an accepted window per the amended invariant.
|
||||
|
||||
---
|
||||
|
||||
### Section 7: Performance Review
|
||||
|
||||
**Hit rate math:**
|
||||
900 hot keys, 1000-entry limit, 30s TTL. Read-to-write ratio assumed high (hot
|
||||
keys are frequently read, less often written). Hit rate well above 60% target
|
||||
under these conditions.
|
||||
|
||||
**Memory:** 1000 entries × average DTO size ≤ 16 MiB cap. Confirmed by "recorded
|
||||
hot data fits those limits." No unbounded growth.
|
||||
|
||||
**Eviction pressure under write load:** If writes on hot keys exceed 1000/30s
|
||||
(one full rotation per TTL window), the effective miss rate rises. At the current
|
||||
write volume this is not a concern, but it should be measured in rollout metrics
|
||||
as a leading indicator.
|
||||
|
||||
**N+1:** No ActiveRecord-style traversal. Cache is key-based. If callers
|
||||
batch-read multiple keys, each is handled individually — no batch optimization.
|
||||
This is acceptable for the current read pattern.
|
||||
|
||||
**Connection pool:** Cache reduces DB read connections. No new connections.
|
||||
No Redis, no new HTTP pool.
|
||||
|
||||
**p99 new latency for cache hit:** Synchronous LRU get is O(1). Cache hit cost
|
||||
is nanoseconds. p99 for hits will be bound by the event loop, not the cache.
|
||||
|
||||
**Slow path:** Cache miss → DB read → cache.set. Same as pre-cache p99 for misses.
|
||||
Target hit rate of 60%+ means most requests avoid this path.
|
||||
|
||||
No issues. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 8: Observability & Debuggability Review
|
||||
|
||||
**Plan already specifies monitoring:** hit/miss rate, eviction, cache bytes,
|
||||
fallback errors, DB CPU, read p95.
|
||||
|
||||
**Gaps:**
|
||||
|
||||
1. **Alert thresholds not defined.** "Monitor" is not an alert. Define:
|
||||
- Alert: hit rate drops below 50% (sustained 5 min) → investigate rollout
|
||||
- Alert: fallback error rate > 0.5% → cache adapter degraded, check bypass mode
|
||||
- Alert: read p95 > 90 ms at any rollout stage → consider rollback
|
||||
|
||||
2. **Runbooks missing for key failure modes:**
|
||||
- *Cache adapter failure:* bypass mode active → DB handles load → check adapter
|
||||
init log → reinitialize (restart or flag cycle) → verify hit rate recovers
|
||||
- *Hit rate not meeting targets:* check eviction count (byte-limit or 1000-entry
|
||||
pressure) → check write rate on hot keys → consider extending TTL if write
|
||||
rate is low
|
||||
|
||||
3. **Post-deploy verification checklist (first 5 min):**
|
||||
- hit rate > 0% within 30s of enabling flag (cold start filling)
|
||||
- fallback error rate == 0
|
||||
- DB CPU trending down
|
||||
- read p95 stable or improving
|
||||
- no INVALID_KEY errors (key encoding regression)
|
||||
|
||||
4. **Debuggability:** If a stale-read bug is reported 3 weeks post-ship, logs
|
||||
must allow reconstruction. Keys should be logged as anonymized hashes (not raw
|
||||
IDs). Cache hit/miss events should carry a log line at debug level with key hash
|
||||
and TTL remaining. This allows timeline reconstruction without exposing IDs.
|
||||
|
||||
These gaps are captured as implementation tasks. The plan's rollout section is
|
||||
amended to reference the post-deploy checklist.
|
||||
|
||||
---
|
||||
|
||||
### Section 9: Deployment & Rollout Review
|
||||
|
||||
**No DB migration.** No schema change. No API change. Deployment is code-only.
|
||||
|
||||
**Feature flag rollout is correct:** 10% → 50% → 100% with a healthy-hour gate.
|
||||
The amended plan now defines "healthy" concretely (hit rate ≥ 60%, fallback-error
|
||||
rate < 0.1%, read p95 < 80 ms at ramp, < 60 ms at full).
|
||||
|
||||
**Old/new code simultaneously:** Not applicable — this is a flag-gated change.
|
||||
With flag off, behavior is identical to pre-cache. No dual-code-path window.
|
||||
|
||||
**Rollback plan:**
|
||||
1. Disable feature flag → cache bypassed immediately (zero-downtime)
|
||||
2. Verify: DB CPU returns to ~70%, p95 returns to ~120 ms (confirms cache was active)
|
||||
3. No DB migration to reverse. No data to clean up.
|
||||
4. If flag disable isn't immediate: deploy flag-off config → restart service
|
||||
|
||||
**Deploy-time risk window:** None. Flag starts at 0%, so first deploy has no effect
|
||||
until flag is explicitly enabled.
|
||||
|
||||
**Staging parity:** Plan does not mention staging. Should be tested in staging with
|
||||
a smaller key set before production rollout.
|
||||
|
||||
**Smoke tests (post-enable at each stage):**
|
||||
- read p95 sampled over 60 seconds < 80 ms
|
||||
- cache hit rate > 0% (any hits confirm adapter is live)
|
||||
- fallback error rate == 0
|
||||
- no increase in error-rate SLO
|
||||
|
||||
No critical deployment risks. The staging gap is a minor process note.
|
||||
|
||||
---
|
||||
|
||||
### Section 10: Long-Term Trajectory Review
|
||||
|
||||
**Technical debt introduced:**
|
||||
- Stale-fill race: documented as accepted, bounded by TTL. This is design debt
|
||||
(accepted inconsistency window) that would need a version guard if consistency
|
||||
requirements tighten. Low risk at current write rate.
|
||||
- SENTINEL definition: must be defined in implementation; not a plan gap but
|
||||
worth noting for the replacement path (any future cache must agree on sentinel
|
||||
encoding).
|
||||
|
||||
**Path dependency:**
|
||||
- Multi-process operation requires disabling or replacing this cache. The startup
|
||||
guard prevents accidental multi-process + in-process cache. The replacement path
|
||||
is explicitly preserved. Low lock-in.
|
||||
|
||||
**Reversibility: 5/5** — Feature flag toggle. No data, schema, or API changes.
|
||||
|
||||
**Ecosystem fit:** Node.js single-process event loop + synchronous LRU cache is
|
||||
standard and well-understood. No framework divergence.
|
||||
|
||||
**1-year question:** A new engineer reading the amended plan would understand:
|
||||
what the cache does, what invariants it guarantees, what invariants it does not
|
||||
(stale-fill window), and how to disable it. The startup guard documents the
|
||||
multi-process constraint explicitly.
|
||||
|
||||
**Phase 2:** If multi-process operation is needed, the repository interface's
|
||||
"future replacement path" (per the plan) allows swapping in a Redis-backed adapter
|
||||
without touching callers. This is a two-way door.
|
||||
|
||||
No issues. Moving on.
|
||||
|
||||
---
|
||||
|
||||
### Section 11: Design & UX Review
|
||||
|
||||
SKIPPED — no UI scope. "This is an internal backend change with no UI, API, schema,
|
||||
pricing, or developer onboarding change."
|
||||
|
||||
---
|
||||
|
||||
## Required Outputs
|
||||
|
||||
### NOT in scope
|
||||
- Distributed caching (Redis, Memcached) — preserves future replacement path
|
||||
- Cross-process coherence — would require a distributed cache
|
||||
- Cache prewarming — not needed; cold start within existing DB capacity
|
||||
- Consistency model changes beyond TTL-bounded staleness
|
||||
- Batch read optimization — current read pattern is key-by-key
|
||||
- Any new product surface, API, UI, or schema change
|
||||
|
||||
### What already exists
|
||||
| Sub-problem | Existing code | Plan reuses? |
|
||||
|-------------|--------------|-------------|
|
||||
| LRU eviction, byte cap, TTL | LRU adapter | YES |
|
||||
| Concurrent-miss thundering herd | Per-key single-flight wrapper | YES |
|
||||
| Feature-gated rollout | Runtime feature flag | YES |
|
||||
| Key validation, tenant isolation, auth | Existing repository contract tests | YES (retained) |
|
||||
| Error mapping for DB errors | Existing typed API error mapping | YES (unchanged) |
|
||||
|
||||
### Dream state delta
|
||||
After this plan ships:
|
||||
- DB CPU: 70% → <50% (target)
|
||||
- read p95: 120 ms → <60 ms (target)
|
||||
- Cache hit rate: 0% → ≥60%
|
||||
|
||||
Remaining gap to 12-month ideal:
|
||||
- DB CPU floor is ~40% (some reads always miss); closing to <40% requires higher
|
||||
hit rate, which needs either a larger key set or lower write churn
|
||||
- Multi-process operation gap remains — reserved for a future phase with distributed
|
||||
cache when operationally justified
|
||||
|
||||
### Error & Rescue Registry
|
||||
|
||||
| Method | Exception Class | Rescued? | Rescue Action | Caller Sees |
|
||||
|--------|----------------|----------|--------------|-------------|
|
||||
| readProfile: cache.get | CACHE_ADAPTER_ERROR | Y | Bypass mode, log warn | Transparent |
|
||||
| readProfile: cache.set | CACHE_SET_ERROR | Y | Log warn, return DB value | Correct value |
|
||||
| readProfile: repo.read | DB_TIMEOUT | N | Propagate (existing) | Existing typed error |
|
||||
| readProfile: repo.read | DB_CONNECTION_ERROR | N | Propagate (existing) | Existing typed error |
|
||||
| readProfile: key validation | INVALID_KEY | N | Propagate (existing) | Existing typed error |
|
||||
| writeProfile: repo.write | DB_TIMEOUT | N | Propagate (existing) | Existing typed error |
|
||||
| writeProfile: repo.write | DB_CONFLICT | N | Propagate (existing) | Existing typed error |
|
||||
| writeProfile: cache.delete | CACHE_ADAPTER_ERROR | Y | Log error, bypass mode | Correct saved value |
|
||||
| startup | CONFIG_ERROR (multi-proc) | N | Reject startup (unrecoverable) | Process won't start |
|
||||
|
||||
### Failure Modes Registry
|
||||
|
||||
| Codepath | Failure Mode | Rescued? | Test? | User Sees | Logged? |
|
||||
|----------|-------------|----------|-------|-----------|---------|
|
||||
| readProfile | cache.get adapter fail | Y | Y (existing) | Nothing (bypass) | Y (warn) |
|
||||
| readProfile | cache.set fail after read | Y | **GAP→add** | Nothing | Y (warn) |
|
||||
| readProfile | DB timeout | N | Y (existing) | Typed error | Y (existing) |
|
||||
| readProfile | stale-fill race (stale cache.set) | Y (TTL) | **GAP→add** | Stale ≤30s | N (accepted) |
|
||||
| writeProfile | DB write fail | N | Y (existing) | Typed error | Y (existing) |
|
||||
| writeProfile | cache.delete fail | Y | **GAP→add** | Nothing | Y (error) |
|
||||
| startup | multi-process + caching | N | **GAP→add** | Process won't start | Y (error) |
|
||||
|
||||
Critical gaps: 0 after amendments. The stale-fill race is accepted behavior with
|
||||
a bounded window (not silent — it is now documented). Test gaps are captured as
|
||||
tasks.
|
||||
|
||||
### TODOS.md updates
|
||||
No TODOS proposed in HOLD SCOPE mode. The staging verification note and alert
|
||||
threshold definitions are captured as implementation tasks (T4, T5) rather than
|
||||
deferred work.
|
||||
|
||||
### Diagrams produced
|
||||
1. System architecture — Section 1 ✓
|
||||
2. Data flow (read path with shadow nodes) — Section 1 ✓
|
||||
3. Data flow (write path) — Section 1 ✓
|
||||
4. Cache entry state machine — Section 1 ✓
|
||||
5. Read-path shadow trace — Section 4 ✓
|
||||
6. Rollback plan: feature flag disable → verify DB metrics. No separate diagram needed (2-step procedure).
|
||||
|
||||
Stale diagram audit: No ASCII diagrams exist in the repository yet (new feature).
|
||||
No stale diagrams to audit.
|
||||
|
||||
---
|
||||
|
||||
## Implementation Tasks
|
||||
|
||||
Synthesized from this review's findings. Each task derives from a specific finding.
|
||||
|
||||
- [ ] **T1 (P1, human: ~1h / CC: ~5min)** — Wrapper — Add try/catch to cache.set in readProfile
|
||||
- Surfaced by: Section 2 / Section 5 — cache.set can throw; currently propagates to caller despite successful DB read
|
||||
- Files: repository wrapper module
|
||||
- Verify: new unit test (cache.set failure, read returns DB value)
|
||||
|
||||
- [ ] **T2 (P1, human: ~1h / CC: ~5min)** — Wrapper — Add try/catch to cache.delete in writeProfile
|
||||
- Surfaced by: Section 2 / Section 5 — cache.delete can throw; currently propagates to caller despite successful DB write
|
||||
- Files: repository wrapper module
|
||||
- Verify: new unit test (cache.delete failure, write returns saved)
|
||||
|
||||
- [ ] **T3 (P1, human: ~2h / CC: ~10min)** — Tests — Add three missing test specs
|
||||
- Surfaced by: Section 6 — cache.set failure path, cache.delete failure path, multi-process startup rejection
|
||||
- Files: wrapper test file
|
||||
- Verify: all three specs pass; concurrent-miss test uses controlled pause/release (not real async timing)
|
||||
|
||||
- [ ] **T4 (P2, human: ~1h / CC: ~5min)** — Observability — Define alert thresholds and runbooks
|
||||
- Surfaced by: Section 8 — "monitor" is not an alert; no runbooks for cache adapter failure or hit-rate regression
|
||||
- Files: runbook/ops doc, monitoring config
|
||||
- Verify: alerts fire in staging simulation; runbooks reviewed by on-call engineer
|
||||
|
||||
- [ ] **T5 (P2, human: ~30min / CC: ~5min)** — Rollout — Add post-deploy verification checklist and staging step
|
||||
- Surfaced by: Section 9 — checklist for first-5-min post-enable and staging parity
|
||||
- Files: rollout runbook / deployment doc
|
||||
- Verify: checklist is in the deployment guide; staging test run passes before production enable
|
||||
|
||||
- [ ] **T6 (P2, human: ~30min / CC: ~5min)** — Consistency — Define SENTINEL value concretely
|
||||
- Surfaced by: Section 5 / Step 0E — sentinel is described abstractly; implementer must define it before writing the wrapper
|
||||
- Files: wrapper module, cache adapter interface
|
||||
- Verify: SENTINEL is a typed singleton not assignable to a valid profile DTO; unit test confirms cache.get returns it correctly
|
||||
|
||||
---
|
||||
|
||||
## Completion Summary
|
||||
|
||||
```
|
||||
+====================================================================+
|
||||
| MEGA PLAN REVIEW — COMPLETION SUMMARY |
|
||||
+====================================================================+
|
||||
| Mode selected | HOLD SCOPE |
|
||||
| System Audit | Skipped per run rules |
|
||||
| Step 0 | HOLD SCOPE confirmed; single approach wins |
|
||||
| Section 1 (Arch) | 0 issues — architecture sound |
|
||||
| Section 2 (Errors) | 3 error paths mapped (cache ops); 0 GAPS |
|
||||
| | after amendments |
|
||||
| Section 3 (Security)| 0 issues — auth boundary correct |
|
||||
| Section 4 (Data/UX) | Stale-fill race documented; 0 unhandled |
|
||||
| Section 5 (Quality) | 2 issues (cache.set/delete error handling) |
|
||||
| | → amended in plan |
|
||||
| Section 6 (Tests) | Diagram produced; 3 test gaps |
|
||||
| Section 7 (Perf) | 0 issues |
|
||||
| Section 8 (Observ) | 3 gaps (alerts, runbooks, checklist) |
|
||||
| Section 9 (Deploy) | 1 gap (staging parity, checklist) |
|
||||
| Section 10 (Future) | Reversibility: 5/5; debt: stale-fill |
|
||||
| | (accepted, bounded) |
|
||||
| Section 11 (Design) | SKIPPED (no UI scope) |
|
||||
+--------------------------------------------------------------------+
|
||||
| NOT in scope | written (6 items) |
|
||||
| What already exists | written (5 items reused) |
|
||||
| Dream state delta | written |
|
||||
| Error/rescue registry| 9 methods, 0 CRITICAL GAPS after amendments |
|
||||
| Failure modes | 7 total, 0 CRITICAL GAPS after amendments |
|
||||
| TODOS.md updates | 0 items (HOLD SCOPE; gaps → tasks) |
|
||||
| Scope proposals | 0 proposed, 0 accepted (HOLD SCOPE) |
|
||||
| CEO plan | skipped (HOLD SCOPE) |
|
||||
| Outside voice | disabled (codex_reviews=disabled) |
|
||||
| Lake Score | 6/6 recommendations chose complete option |
|
||||
| Diagrams produced | 5 (arch, read flow, write flow, state |
|
||||
| | machine, data flow shadow trace) |
|
||||
| Stale diagrams found | 0 (new feature, no prior diagrams) |
|
||||
| Unresolved decisions | 0 (all auto-resolved, headless run) |
|
||||
+====================================================================+
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## GSTACK REVIEW REPORT
|
||||
|
||||
| Review | Trigger | Why | Runs | Status | Findings |
|
||||
|--------|---------|-----|------|--------|----------|
|
||||
| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open | mode: HOLD_SCOPE, 0 critical gaps (stale-fill race documented as accepted; 3 test gaps → T3) |
|
||||
| Outside Review | disabled | Independent 2nd opinion | 0 | disabled | codex_reviews=disabled per config |
|
||||
| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | — | not yet run |
|
||||
| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | no UI scope |
|
||||
| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | not yet run |
|
||||
|
||||
**OUTSIDE COVERAGE:** disabled. `codex_reviews: disabled` in config. Re-enable: `gstack-config set codex_reviews enabled`. Native review completed in full.
|
||||
|
||||
**VERDICT:** CEO review completed (HOLD SCOPE). Eng review required before shipping.
|
||||
|
||||
NO UNRESOLVED DECISIONS
|
||||
+246
@@ -0,0 +1,246 @@
|
||||
{
|
||||
"sourceRun": "ship-source-q-full-paid-20260909-0325",
|
||||
"nativeSha256": "7efda14792d1e26a713915f86548f7bdb11f9802b57a8e1e61aa0d9b5282646b",
|
||||
"sourceProof": "/home/vercel-sandbox/gstack/.context/ship-source-q-full-paid-20260909-0325/design-closure-observation-v1/proof.json",
|
||||
"historicalOutcome": "Still pending at capture; no historical pass is asserted.",
|
||||
"calls": [
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_01TBHyJ3rHbngqPoeedfgmYq",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Pass 1 \u2014 Visual Hierarchy: The plan lists this gap but has no fix. The Save button renders with the same size, weight, and color as Reset, Cancel, and Export. DESIGN.md already specifies the remedy: Save gets #1d4ed8 fill with white text; the other three are ghost neutral buttons. Should I add this fix specification to the plan? <gstack-qid:plan-design-review-gap1-visual-hierarchy>",
|
||||
"header": "Save button",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add DESIGN.MD fix to plan (recommended)",
|
||||
"description": "Add a plan task specifying: Save = filled #1d4ed8/white; Reset, Cancel, Export = ghost neutral. This is a pre-approved token \u2014 no new design call needed. Implementer has unambiguous spec."
|
||||
},
|
||||
{
|
||||
"label": "Defer to implementer",
|
||||
"description": "Leave the gap description in place without a specified remedy. Risk: engineer may invent their own solution that doesn't match the design system."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Pass 1 \u2014 Visual Hierarchy: The plan lists this gap but has no fix. The Save button renders with the same size, weight, and color as Reset, Cancel, and Export. DESIGN.md already specifies the remedy: Save gets #1d4ed8 fill with white text; the other three are ghost neutral buttons. Should I add this fix specification to the plan? <gstack-qid:plan-design-review-gap1-visual-hierarchy>": "Add DESIGN.MD fix to plan (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:30:35.365Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_013o6wpww76uhGbvtuuheSad",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Pass 2 \u2014 Interaction States: The Save action takes 2\u20135 seconds with no visible indicator. Users see a frozen page and may double-click or assume failure. DESIGN.md already specifies the remedy: an inline spinner beside \u201cSaving\u2026\u201d inside the disabled Save button, aria-busy=true, with reduced-motion support. Should I add this pending-state specification to the plan? <gstack-qid:plan-design-review-gap5-motion>",
|
||||
"header": "Save pending",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add DESIGN.MD pending-state spec to plan (recommended)",
|
||||
"description": "Add a task specifying: spinner + \u201cSaving\u2026\u201d label inside disabled button, aria-busy=true, prefers-reduced-motion respected. Closes the frozen-UI gap and the a11y pending-state gap."
|
||||
},
|
||||
{
|
||||
"label": "Defer to implementer",
|
||||
"description": "Leave the motion gap described but unspecified. Risk: implementer ships a toast, a full-page overlay, or nothing, diverging from the design system\u2019s established pending pattern."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Pass 2 \u2014 Interaction States: The Save action takes 2\u20135 seconds with no visible indicator. Users see a frozen page and may double-click or assume failure. DESIGN.md already specifies the remedy: an inline spinner beside \u201cSaving\u2026\u201d inside the disabled Save button, aria-busy=true, with reduced-motion support. Should I add this pending-state specification to the plan? <gstack-qid:plan-design-review-gap5-motion>": "Add DESIGN.MD pending-state spec to plan (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:30:45.386Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_014HrXmHX33zRzu2A89Z5jzA",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Pass 5 \u2014 Design System: Spacing is inconsistent \u2014 24px in some places, 32px in others, 16px in a third. DESIGN.md specifies a clear 8px-base scale: sections 32px apart, field groups 24px, label-to-input 8px. Should I add this spacing specification to the plan? <gstack-qid:plan-design-review-gap2-spacing>",
|
||||
"header": "Spacing scale",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add DESIGN.MD spacing scale to plan (recommended)",
|
||||
"description": "Document the 8px-base values (32px/24px/8px) as implementation requirements. Gives the engineer exact values instead of a problem description. Uneven rhythm ships unnoticed otherwise."
|
||||
},
|
||||
{
|
||||
"label": "Defer to implementer",
|
||||
"description": "Leave the spacing gap named but without specified values. Risk: engineer picks values that feel right locally but don\u2019t match the design system, and the inconsistency persists."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Pass 5 \u2014 Design System: Spacing is inconsistent \u2014 24px in some places, 32px in others, 16px in a third. DESIGN.md specifies a clear 8px-base scale: sections 32px apart, field groups 24px, label-to-input 8px. Should I add this spacing specification to the plan? <gstack-qid:plan-design-review-gap2-spacing>": "Add DESIGN.MD spacing scale to plan (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:30:53.401Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_016jvUA2qsEoHfK2NiMj9REw",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Pass 5/6 \u2014 Color & Accessibility: The error message uses red text on a light pink background at ~3:1 contrast \u2014 below WCAG AA (requires 4.5:1). DESIGN.md already specifies the fix: #991b1b text on #fef2f2 background, which computes to ~6.9:1 (verified). Should I add these exact color tokens as the required remedy? <gstack-qid:plan-design-review-gap3-error-contrast>",
|
||||
"header": "Error contrast",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add #991b1b/#fef2f2 tokens to plan (recommended)",
|
||||
"description": "Specify the exact DESIGN.MD error token values as the fix. Resolves the WCAG AA violation. The computed contrast (~6.9:1) well exceeds the 4.5:1 requirement."
|
||||
},
|
||||
{
|
||||
"label": "Defer to implementer",
|
||||
"description": "Leave the contrast gap described without specifying the correct colors. Risk: engineer picks a different red-on-pink combination that may or may not meet WCAG AA."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Pass 5/6 \u2014 Color & Accessibility: The error message uses red text on a light pink background at ~3:1 contrast \u2014 below WCAG AA (requires 4.5:1). DESIGN.md already specifies the fix: #991b1b text on #fef2f2 background, which computes to ~6.9:1 (verified). Should I add these exact color tokens as the required remedy? <gstack-qid:plan-design-review-gap3-error-contrast>": "Add #991b1b/#fef2f2 tokens to plan (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:03.427Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_01HxoveAbVUy1V6DpmH9yNwD",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Pass 5 \u2014 Typography: The form uses three font sizes \u2014 14px, 16px, and 18px. DESIGN.md specifies two roles only: 16px for body/form labels/helper text, 20px for section headings. Should I add this two-size constraint to the plan, specifying which elements map to which size? <gstack-qid:plan-design-review-gap4-typography>",
|
||||
"header": "Type scale",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Add 16px/20px type scale to plan (recommended)",
|
||||
"description": "Specify that all body text, form labels, and helper text use 16px; section headings use 20px. Gives the engineer a clear mapping and eliminates the third size."
|
||||
},
|
||||
{
|
||||
"label": "Defer to implementer",
|
||||
"description": "Leave the three-size problem named but unresolved. Risk: engineer removes one size but doesn\u2019t know which, or defaults to a system that doesn\u2019t match the type scale."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Pass 5 \u2014 Typography: The form uses three font sizes \u2014 14px, 16px, and 18px. DESIGN.md specifies two roles only: 16px for body/form labels/helper text, 20px for section headings. Should I add this two-size constraint to the plan, specifying which elements map to which size? <gstack-qid:plan-design-review-gap4-typography>": "Add 16px/20px type scale to plan (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:31:09.436Z"
|
||||
},
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_01JTzc9a8SMq3mxCQSt3pC8c",
|
||||
"questions": [
|
||||
{
|
||||
"question": "Design review is complete (5/10 \u2192 9/10, 5 decisions added). What\u2019s next? <gstack-qid:plan-design-review-next-steps>",
|
||||
"header": "Next steps",
|
||||
"multiSelect": false,
|
||||
"options": [
|
||||
{
|
||||
"label": "Run /plan-eng-review (recommended)",
|
||||
"description": "Required gate before shipping. This design review added specific interaction specs (pending states, color tokens, spacing values) that need architectural validation."
|
||||
},
|
||||
{
|
||||
"label": "Run /design-shotgun",
|
||||
"description": "Explore visual variants for the settings form. Useful if you want to see the approved design decisions rendered before implementation."
|
||||
},
|
||||
{
|
||||
"label": "Skip \u2014 handle next steps manually",
|
||||
"description": "Close out the review here. You can run /plan-eng-review or other reviews manually when ready."
|
||||
}
|
||||
]
|
||||
}
|
||||
],
|
||||
"answered": true,
|
||||
"failed": false,
|
||||
"answers": {
|
||||
"Design review is complete (5/10 \u2192 9/10, 5 decisions added). What\u2019s next? <gstack-qid:plan-design-review-next-steps>": "Run /plan-eng-review (recommended)"
|
||||
},
|
||||
"unansweredQuestionIndices": [],
|
||||
"answeredAt": "2026-09-09T03:33:57.456Z"
|
||||
}
|
||||
],
|
||||
"planReadyRequests": [
|
||||
{
|
||||
"sessionId": "d783a750-f024-47d5-8b67-77213e37e618",
|
||||
"toolUseId": "toolu_015unza5DGEz6ugXgPYcJxBe",
|
||||
"timestamp": "2026-09-09T03:34:09.253Z",
|
||||
"failed": false,
|
||||
"source": "pre_tool_use"
|
||||
}
|
||||
],
|
||||
"report": {
|
||||
"content": "# Design Review: Settings Page UI Redesign\n\n## Context\n\nThis is a designer's-eye review of the Settings Page UI redesign plan. The plan updates an existing account-settings form against a checked-in DESIGN.md. The shell and components exist; the plan identifies five implementation gaps where the current form diverges from the approved design. This review's job is to ensure those gaps have specified, DESIGN.md-aligned remedies before implementation begins.\n\n---\n\n## PRE-REVIEW SYSTEM AUDIT\n\n- **DESIGN.md**: Present and comprehensive. All design decisions calibrate against it.\n- **UI scope**: Account settings form \u2014 action group, inline status, two fieldsets (Profile + Notifications). Single-column layout with responsive breakpoint at 640px.\n- **Existing components**: Button, Field, InlineStatus, ErrorSummary, ConfirmationDialog \u2014 no new component family needed.\n- **Prior design reviews**: None in review log (first run on this plan).\n- **Retrospective check**: No prior flagged areas to escalate.\n\n---\n\n## Step 0: Design Scope Assessment\n\n### 0A. Initial Rating: 5/10\n\nThe plan is thorough on **accepted behavior** (DOM order, state machine, responsive specs, a11y, confirmation dialogs, focus management). That foundation is solid. But the \"Planned implementation gaps\" section lists five bugs without fixes \u2014 naming the disease without prescribing treatment. A 10/10 plan for this scope would have: approved remedies for each gap, a journey storyboard, and an explicit implementation task list.\n\n### 0B. DESIGN.md Status\n\nDESIGN.md exists and is detailed. It prescribes specific colors, spacing values, type sizes, and interaction patterns. Every gap in the plan has a corresponding DESIGN.md token. The fix path is clear \u2014 apply what's already approved.\n\n### 0C. Existing Design Leverage\n\n| Asset | Detail |\n|---|---|\n| Button component | Save = filled #1d4ed8/white; Reset, Cancel, Export = ghost neutral |\n| Spacing scale | 8px base: sections 32px, field groups 24px, label-to-input 8px |\n| Type scale | 16px body/labels/helper; 20px section headings |\n| Error colors | `#991b1b` on `#fef2f2` (computed contrast ~6.9:1, exceeds WCAG AA) |\n| Pending pattern | Spinner + \"Saving\u2026\" inside disabled Save button, `aria-busy=true`, reduced-motion support |\n| Focus ring | 2px solid `#1d4ed8`, offset 2px; measured contrast above 3:1 |\n\n### 0D. Focus Areas\n\nUser requested all 7 dimensions. Mockups skipped (text-only review). Proceeding to passes.\n\n---\n\n## Pass 1: Information Architecture \u2014 7/10\n\n**What works:** DOM and visual order are explicitly defined. Heading hierarchy (h1 \u2192 h2 \u2192 h2) is correct and semantic. The fieldsets are labeled via `aria-labelledby`. Action group position (above InlineStatus, above fieldsets) is specified.\n\n**Gap 1 \u2014 Save button has no visual primacy (PENDING)**\n\n> The plan lists this as a known gap but provides no fix: \"The 'Save' button is rendered with the same size, weight, and color as three other buttons.\"\n\nDESIGN.md specifies: *\"Save is the only filled primary action (#1d4ed8 with white text). Reset, Cancel and Export are neutral ghost buttons.\"*\n\nWithout this distinction, users can't scan the page and know what the primary action is. All four buttons compete equally. This violates the hierarchy principle: **what should the user see/do first is visually unclear.**\n\n**Proposed remedy:** Apply `#1d4ed8` fill with white text to Save only; render Reset, Cancel, Export as ghost buttons per DESIGN.md. This is already an approved DESIGN.md token \u2014 no new decision needed, only application.\n\n**Decision: APPROVED** \u2014 Apply `#1d4ed8` fill with white text to Save; render Reset, Cancel, Export as ghost neutral buttons per DESIGN.MD.\n\n**Re-rated: 9/10** (structure solid; action group hierarchy now specified).\n\n---\n\n## Pass 2: Interaction State Coverage \u2014 7/10\n\n**State inventory vs. plan coverage:**\n\n| Feature | Loading | Empty | Error | Success | Pending | Dirty | Partial |\n|---|---|---|---|---|---|---|---|\n| Form load | Skeleton \u2713 | Defaults \u2713 | Retry \u2713 | \u2014 | \u2014 | \u2014 | \u2014 |\n| Save | **MISSING** | \u2014 | Retain edits + error \u2713 | \"Saved at HH:mm\" \u2713 | **MISSING** | \"Unsaved changes\" \u2713 | Atomic \u2713 |\n| Export | \u2014 | \u2014 | Inline error + Retry \u2713 | Clears export error \u2713 | \u2014 | \u2014 | \u2014 |\n| Reset | \u2014 | \u2014 | \u2014 | Restores save timestamp \u2713 | \u2014 | \u2014 | \u2014 |\n\n**Gap 5 \u2014 No loading/pending indicator for Save (PENDING)**\n\n> The plan states: \"The 'Save' action takes 2-5 seconds with no loading indicator. Users see a frozen page.\"\n\nDESIGN.md specifies: *\"The established pending-action pattern is an inline spinner beside 'Saving\u2026' inside the disabled Save button, aria-busy=true, with reduced-motion support.\"*\n\nWithout this: users see a frozen UI Line truncated
|
||||
"successfulUpdateAt": "2026-09-09T03:33:19.861Z",
|
||||
"nativeModifications": [
|
||||
{
|
||||
"toolUseId": "toolu_01Vdf8ALCNVDNAXRf59ygKui",
|
||||
"calledAt": "2026-09-09T03:30:20.268Z",
|
||||
"succeededAt": "2026-09-09T03:30:21.930Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01N2JauknBNhPqxazCrzdHon",
|
||||
"calledAt": "2026-09-09T03:31:26.243Z",
|
||||
"succeededAt": "2026-09-09T03:31:27.520Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_013wUGUX1gxiGxKtGFP7tRrr",
|
||||
"calledAt": "2026-09-09T03:31:31.653Z",
|
||||
"succeededAt": "2026-09-09T03:31:33.036Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01GUcB6gkCKfHdj7oqC4Kuae",
|
||||
"calledAt": "2026-09-09T03:31:37.421Z",
|
||||
"succeededAt": "2026-09-09T03:31:38.569Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01VKbXVApCrE127TCj74fpV1",
|
||||
"calledAt": "2026-09-09T03:31:42.830Z",
|
||||
"succeededAt": "2026-09-09T03:31:44.069Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01QxFKJCB6BMGa4miwg9c74B",
|
||||
"calledAt": "2026-09-09T03:31:47.833Z",
|
||||
"succeededAt": "2026-09-09T03:31:49.579Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01XjKgUbKMiSDXRv6SJ8LukY",
|
||||
"calledAt": "2026-09-09T03:31:58.766Z",
|
||||
"succeededAt": "2026-09-09T03:31:59.098Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_014HJ5Q8ncq64PgkG3nxpj7w",
|
||||
"calledAt": "2026-09-09T03:32:09.979Z",
|
||||
"succeededAt": "2026-09-09T03:32:10.625Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_013egZir3S1N16cEFVSjzXNX",
|
||||
"calledAt": "2026-09-09T03:32:25.091Z",
|
||||
"succeededAt": "2026-09-09T03:32:26.207Z"
|
||||
},
|
||||
{
|
||||
"toolUseId": "toolu_01MqwaBg4uLpa5s4iCMtir4r",
|
||||
"calledAt": "2026-09-09T03:33:18.271Z",
|
||||
"succeededAt": "2026-09-09T03:33:19.861Z"
|
||||
}
|
||||
]
|
||||
},
|
||||
"screen": " 319 +| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | \u2014 | \n 320 + \n 321 +**VERDICT:** Design Review CLEAR \u2014 eng review required before shipping. \n 322 + \n 323 +NO UNRESOLVED DECISIONS \n\n\u25cf Now let me verify the plan file ends correctly with ## GSTACK REVIEW REPORT as the last heading and NO UNRESOLVED\n DECISIONS as the final non-whitespace line.\n\n Read 1 file (ctrl+o to expand)\n\n\u25cf Gate checks pass. Logging learnings and running final steps.\n\n\u25cf Bash(~/.claude/skills/gstack/bin/gstack-learnings-log\n '{\"skill\":\"plan-design-review\",\"type\":\"operational\",\"key\":\"design-md-error-colors-contrast\",\"insight\":\"DESIGN.m\u2026)\n \u23bf \u00a0LEARNING_LOGGED\n \u23bf \u00a0Allowed by auto mode classifier\n\n\u25cf\u00a0User answered Claude's questions:\n \u23bf \u00a0\u00b7 Design review is complete (5/10 \u2192 9/10, 5 decisions added). What\u2019s next?\n <gstack-qid:plan-design-review-next-steps> \u2192 Run /plan-eng-review (recommended)\n \n\u25cf Bash(~/.claude/skills/gstack/bin/gstack-skill-end --skill \"plan-design-review\" --outcome success \\\n --session-id \"2661353-1788924381-77a97d17\" --tel-start \"17889243\u2026)\n \u23bf \u00a0SKILL_END: recorded outcome=success duration_s=462\n TEL_OK\n \u23bf \u00a0Allowed by auto mode classifier\n \n\u25cf All done. The review report is written to\n /tmp/gstack-paid-shard-fORfDd/tmp/gstack-e2e-plan-design-VZoWd7/gstack-test-plan-design.md.\n\n\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\n Exit plan mode?\n\n Claude wants to exit plan mode\n\n \u276f 1. Yes, and switch to default (ask each time) for this session\n 2. No\n\n"
|
||||
}
|
||||
@@ -0,0 +1,38 @@
|
||||
import type { AskUserQuestionFingerprint } from './claude-pty-runner';
|
||||
import { pickCeoCompletionHandoff } from './ceo-completion-handoff';
|
||||
|
||||
/** Follow the offered recommendation only in the native pre-review approach menu. */
|
||||
export function pickCeoRecommendedApproach(fp: AskUserQuestionFingerprint): number | null {
|
||||
const call = fp.nativeCall;
|
||||
if (!fp.preReview || !call || call.answered !== false || call.failed !== false ||
|
||||
fp.signature !== `${call.sessionId}:${call.toolUseId}` || call.questions.length !== 1 ||
|
||||
(fp.nativeQuestionIndex !== undefined && fp.nativeQuestionIndex !== 0)) return null;
|
||||
const q = call.questions[0]!;
|
||||
if (q.multiSelect || q.options.length < 2 || !/^Approach$/i.test(q.header.trim())) return null;
|
||||
const ids = [...q.question.matchAll(/<gstack-qid:\s*([a-z0-9-]+)\s*>/gi)];
|
||||
if (ids.length !== 1 || (q.question.match(/<gstack-qid/gi)?.length ?? 0) !== 1) return null;
|
||||
const qid = ids[0]![1]!.toLowerCase();
|
||||
const question = q.question.trim().replace(/^D\s*\d+\s*[—–:-]\s*/i, '');
|
||||
const planApproach = qid === 'plan-ceo-approach' &&
|
||||
/^Which implementation approach should this plan use\?(?:\s|$)/i.test(question);
|
||||
const testApproach = qid === 'plan-ceo-review-impl-approach' &&
|
||||
/^Which implementation approach for the [a-z_$][\w$]*(?:\.[a-z_$][\w$]*)*\(\) tests\?(?:\s|$)/i.test(question);
|
||||
if (!planApproach && !testApproach) return null;
|
||||
if (fp.options.length !== q.options.length || !fp.options.every((option, i) =>
|
||||
option.index === i + 1 && option.label === q.options[i]!.label)) return null;
|
||||
const labels = q.options.map(option => option.label.trim());
|
||||
if (new Set(labels).size !== labels.length) return null;
|
||||
const recommended = labels.map((label, i) => ({ label, index: i + 1 })).filter(({ label }) =>
|
||||
/\s\(Recommended\)\s*$/i.test(label) &&
|
||||
(label.match(/\brecommended\b/gi)?.length ?? 0) === 1 &&
|
||||
!/\b(?:not|never)\s*\(recommended\)/i.test(label));
|
||||
return recommended.length === 1 ? recommended[0]!.index : null;
|
||||
}
|
||||
|
||||
/** Preserve the existing manual handoff and all other caller/default choices. */
|
||||
export function pickCeoCountQuestion(
|
||||
fp: AskUserQuestionFingerprint,
|
||||
activeCapture: AskUserQuestionFingerprint = fp,
|
||||
): number | null {
|
||||
return pickCeoRecommendedApproach(activeCapture) ?? pickCeoCompletionHandoff(fp, activeCapture);
|
||||
}
|
||||
@@ -16,15 +16,34 @@ function resolvedCeoRecap(description: string): boolean {
|
||||
/** A closed-review declaration plus one direct navigation query, even when its recap follows it. */
|
||||
function closedReviewNavigation(declaration: string, context: string): boolean {
|
||||
const question = declaration.replace(/<gstack-qid:[^>]+>/gi, '');
|
||||
return /^CEO review (?:is )?(?:complete|done|cleared|clean)[.!](?:\s|$)/i.test(question) &&
|
||||
/(?:^|[.!]\s+)What(?:['’]s)? next\?(?:\s|$)/i.test(question) && closedNavigationContext(context);
|
||||
}
|
||||
|
||||
/** The metadata recap is native question text, not a new substantive choice. */
|
||||
function isMetadataNavigationQuestion(declaration: string): boolean {
|
||||
return /^What(?:['’]s|\s+is)\s+(?:the\s+)?next(?:\s+(?:step|review))?\s+after\s+(?:this|the)\s+CEO\s+review\?\s*$/i.test(declaration.trim().split('\n')[0]!);
|
||||
}
|
||||
|
||||
function metadataClosedReviewNavigation(declaration: string, context: string): boolean {
|
||||
const question = declaration.replace(/<gstack-qid:[^>]+>/gi, '').trim();
|
||||
return isMetadataNavigationQuestion(question) &&
|
||||
/^[ \t]{0,3}ELI10:\s*(?:The\s+)?CEO\s+review\s+(?:is\s+)?(?:complete|cleared|clean|done(?:\s+and\s+clear(?:ed)?)?)[.!](?:\s|$)/im.test(question) &&
|
||||
!/`{3}|~{3}|(?:^|[.!?]\s+)[ \t]*>|\b(?:example|quoted source)\s*:/im.test(context) &&
|
||||
!/\b(?:incomplete|unfinished)\b|\b(?:review|decisions|findings|issues|gaps)\b[^.!?\n]{0,60}\b(?:not|never)\b|\b(?:isn['’]t|aren['’]t|wasn['’]t|weren['’]t)\b/i.test(context) &&
|
||||
!/(?:^|[.!?;:]\s+|\b(?:proceed to|continue to|should|must|will|need to|can|could|would|may|might)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|repair|implement|resolve|decide)\b/im.test(context) &&
|
||||
closedNavigationContext(context);
|
||||
}
|
||||
|
||||
/** Shared closed-review guards; next-review sequencing is still navigation. */
|
||||
function closedNavigationContext(context: string): boolean {
|
||||
const unfinished = context.replace(/\b(?:no|0)\s+unresolved\s+(?:decisions|gaps|issues|findings)\b/gi, '');
|
||||
// Conditional closure of this review is unfinished work. Sequencing the
|
||||
// next review after implementation does not reopen the completed CEO review.
|
||||
const stateVerb = String.raw`(?:is|are|was|were|becomes?|became|(?:will|would|can|could|may|might)\s+(?:be|become))`;
|
||||
const closure = String.raw`(?:(?:all\s+)?(?:decisions|gaps|issues|findings)\s+(?:${stateVerb}\s+)?resolved|(?:the\s+)?CEO\s+review\s+(?:${stateVerb}\s+)?(?:complete|done|cleared|clean)|the\s+review\s+(?:${stateVerb}\s+)?(?:complete|done|cleared|clean))`;
|
||||
const conditionalClosure = new RegExp(String.raw`\b(?:once|when|after)\b[^.!?]{0,180}\b${closure}\b|\b${closure}\b[^.!?]{0,100}\b(?:once|when|after)\b`, 'i');
|
||||
return /^CEO review (?:is )?(?:complete|done|cleared|clean)[.!](?:\s|$)/i.test(question) &&
|
||||
/(?:^|[.!]\s+)What(?:['’]s)? next\?(?:\s|$)/i.test(question) &&
|
||||
(context.match(/\?/g)?.length ?? 0) === 1 &&
|
||||
return (context.match(/\?/g)?.length ?? 0) === 1 &&
|
||||
!/\b(?:unresolved|outstanding|remaining|pending|if|unless|until)\b|\b(?:gap|issue|finding|decision)s?\s+(?:still\s+)?remains?\b|\bstill\s+open\b/i.test(unfinished) &&
|
||||
!/\bnot\s+(?:all|no|0)\b/i.test(context) &&
|
||||
!conditionalClosure.test(context) &&
|
||||
@@ -56,7 +75,9 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null {
|
||||
const describedCompletion = (recappedNavigation || (genericCompletion && q.options.some(option => closedCeoRecap(option.description ?? '')))) &&
|
||||
!/\b(?:unresolved|outstanding|remains?|remaining|pending)\b/i.test(unfinished) &&
|
||||
!/(?:^|[.!?;]\s+|\b(?:please|must|need\s+to)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|implement|resolve|decide)\b/im.test(gateContext);
|
||||
const completion = explicitCompletion || describedCompletion;
|
||||
const metadataCompletion = Boolean(id) && metadataClosedReviewNavigation(declaration, gateContext);
|
||||
if (isMetadataNavigationQuestion(questionText) && /\n[ \t]*ELI10:/i.test(questionText) && !metadataCompletion) return null;
|
||||
const completion = explicitCompletion || describedCompletion || metadataCompletion;
|
||||
const requiredEng = /(?:\bEng(?:ineering)?\s+review|\/plan-eng-review)\b[^.!?]{0,180}\brequired(?:\s+shipping)?\s+gate\b/i.test(gateContext) ||
|
||||
/\brequired(?:\s+shipping)?\s+gate\s+is\s+(?:an?\s+)?(?:Eng(?:ineering)?\s+review|\/plan-eng-review)\b/i.test(gateContext);
|
||||
// These native next-review identities share a closed navigation contract;
|
||||
|
||||
@@ -154,7 +154,7 @@ export function hasStaleFillRaceFinding(report: string): boolean {
|
||||
const claims = context.split(/(?:[.!?]\s+|\b(?:but|however|nevertheless|yet)\s*[:,]?\s+)/i);
|
||||
const violation = claims.some(claim => /\b(?:violat\w*|break\w*)\b[^.!?]*\b(?:contract|guarantee|consistency|rule)\b/i.test(claim)
|
||||
&& !/\b(?:not|no|never)\b/i.test(claim));
|
||||
for (const claim of claims) {
|
||||
for (const [claimIndex, claim] of claims.entries()) {
|
||||
// "Not permitted" is a violation assertion, not permission. Scope a
|
||||
// permitted old result to its original caller; it cannot justify a
|
||||
// cache fill or a later reader observing that same old version.
|
||||
@@ -164,8 +164,21 @@ export function hasStaleFillRaceFinding(report: string): boolean {
|
||||
const proposedPrevention = /^(?:guard|serialize|serialise|coordinate|prevent|reject|skip)\b/i.test(claim.trim())
|
||||
&& /\b(?:cache|fill|refill|write|mutation|invalidation)\w*\b/i.test(claim)
|
||||
&& /\b(?:so(?:\s+that)?|to\s+ensure)\b/i.test(claim);
|
||||
// The model declaration must accept the stale consequence itself.
|
||||
// A normative freshness requirement called an accepted model is not a
|
||||
// dismissal. Bare "This" can refer only to the preceding stale claim.
|
||||
const modelDeclaration = /^(.+?)\s+(?:is|remains)\s+(?:(?:the|an?)\s+)?(?:accepted|expected|intentional|documented)\s+consistency\s+(?:model|contract|policy|semantics)\b/i.exec(allowanceText.trim());
|
||||
const subject = modelDeclaration?.[1] ?? '';
|
||||
const previousClaim = claims[claimIndex - 1] ?? '';
|
||||
const explicitStaleSubject = /^(?:this|the|an?)\s+(?:bounded\s+)?(?:inconsistency|staleness|stale[- ](?:read|fill)|stale\s+(?:read|fill|refill))(?:\s+(?:window|behavior|behaviour|race|consequence))?$/i.test(subject);
|
||||
const impliedStaleSubject = /^this$/i.test(subject)
|
||||
&& /\b(?:stale|outdated|old(?:er)?\s+(?:value|snapshot|data))\b/i.test(previousClaim)
|
||||
&& /\b(?:read|fetch|fill|refill|repopulat)\w*\b/i.test(previousClaim)
|
||||
&& !/\b(?:must|shall|requires?|violat\w*|not|cannot|can't)\b/i.test(previousClaim);
|
||||
const acceptedStaleModel = Boolean(modelDeclaration) && (explicitStaleSubject || impliedStaleSubject);
|
||||
const dismissal = /\b(?:not|isn't)\s+(?:a\s+|an\s+)?(?:gap|bug|defect|issue|violation|problem)\b|\bno\s+(?:gap|bug|defect|issue|violation|race)\b/i.test(claim)
|
||||
|| /\b(?:accepted|expected|intentional|documented)\s+(?:invariant|behavior|trade[- ]off|stale[- ]read\s+window)\b|\b(?:allowed|permitted|acceptable)\b/i.test(allowanceText)
|
||||
|| acceptedStaleModel
|
||||
|| (!proposedPrevention && /\b(?:cannot|can't|never|does not|will not)\s+(?:\w+\s+){0,3}(?:refill|repopulate|populate|insert|store|cache|set|violate)\b/i.test(claim))
|
||||
|| /\bno\s+(?:fix|change|coordination|guard)\s+(?:is\s+)?(?:needed|required)\b/i.test(claim);
|
||||
if (!dismissal) continue;
|
||||
|
||||
@@ -1460,10 +1460,11 @@ export function planCountPrerequisitePick(fp: AskUserQuestionFingerprint): numbe
|
||||
const skip = fp.options.filter(({ label }) =>
|
||||
/^Skip\s*[—–-]\s*(?:proceed\s*with\s*)?standard\s*review(?:\s*\(recommended\))?$/i.test(label));
|
||||
if (run.length === 1 && skip.length === 1) return skip[0].index;
|
||||
// The same prerequisite also offers "Skip — review now". Require exactly
|
||||
// The same prerequisite also offers "Skip — review now" or a direct
|
||||
// "Proceed with standard review". Require exactly
|
||||
// the two opposed actions so this wording cannot skip a mixed finding.
|
||||
const choices = fp.options.filter(({ label }) => !/^(?:Typesomething\.|Chataboutthis)$/i.test(label.replace(/\s+/g, '')));
|
||||
const reviewNow = choices.filter(({ label }) => /^Skip\s*[—–-]\s*review\s*now(?:\s*\(recommended\))?$/i.test(label));
|
||||
const reviewNow = choices.filter(({ label }) => /^(?:Skip\s*[—–-]\s*review\s*now|Proceed\s*with\s*standard\s*review)(?:\s*\(recommended\))?$/i.test(label));
|
||||
if (choices.length !== 2 || run.length !== 1 || reviewNow.length !== 1 || run[0].index === reviewNow[0].index ||
|
||||
!/^Run\s*\/office-hours\s*(?:now|first)(?:\s*\(recommended\))?$/i.test(run[0].label) ||
|
||||
(fp.nativeCall && (fp.nativeCall.failed || fp.nativeCall.questions.length !== 1 || fp.nativeCall.questions[0]?.multiSelect))) return null;
|
||||
@@ -2824,16 +2825,32 @@ export async function runPlanSkillCounting(opts: {
|
||||
* fixture plan content, not the git diff.
|
||||
*/
|
||||
firstAUQPick?: (fp: AskUserQuestionFingerprint) => number;
|
||||
/** Total budget for skill to reach a terminal outcome. Default 1_500_000 (25 min). */
|
||||
/** Total budget including startup and cleanup. Must exceed the 5s cleanup reserve. Default 1_500_000. */
|
||||
timeoutMs?: number;
|
||||
/** Extra env merged into the spawned `claude` process. */
|
||||
env?: Record<string, string>;
|
||||
/** Override the spawned model. Defaults via launchClaudePty's chain. */
|
||||
model?: string;
|
||||
}): Promise<PlanSkillCountObservation> {
|
||||
const budgetStarted = performance.now();
|
||||
const startedAt = Date.now();
|
||||
const defaultPick = opts.defaultPick ?? 1;
|
||||
const timeoutMs = opts.timeoutMs ?? 1_500_000;
|
||||
// The caller may use this same limit as its Bun timeout. Leave room for
|
||||
// close()'s 2s graceful + 1s forced exit waits and artifact/fixture cleanup.
|
||||
// A second work window after boot lets Bun retry while this body is alive.
|
||||
const cleanupReserveMs = 5_000;
|
||||
if (!Number.isFinite(timeoutMs) || timeoutMs <= cleanupReserveMs) {
|
||||
throw new RangeError('Plan counting timeout must exceed the 5000ms cleanup reserve');
|
||||
}
|
||||
const workDeadline = budgetStarted + timeoutMs - cleanupReserveMs;
|
||||
const remainingWork = () => Math.max(0, workDeadline - performance.now());
|
||||
async function waitForWork(ms: number): Promise<boolean> {
|
||||
const remaining = remainingWork();
|
||||
if (remaining <= 0) return false;
|
||||
await Bun.sleep(Math.min(ms, remaining));
|
||||
return remainingWork() > 0;
|
||||
}
|
||||
|
||||
const fixture = createPlanCountFixture(opts.followUpPrompt, { nativeReviewOnly: true, files: opts.fixtureFiles });
|
||||
let session: ClaudePtySession;
|
||||
@@ -2841,7 +2858,9 @@ export async function runPlanSkillCounting(opts: {
|
||||
session = await launchClaudePty({
|
||||
permissionMode: 'plan',
|
||||
cwd: fixture.cwd,
|
||||
timeoutMs: timeoutMs + 60_000,
|
||||
// Stop new output at the work cutoff so screen drain cannot consume
|
||||
// the reserve while the CLI continues streaming.
|
||||
timeoutMs: Math.max(1, remainingWork()),
|
||||
env: { ...opts.env, ...fixture.env },
|
||||
model: opts.model,
|
||||
seedSkills: true,
|
||||
@@ -2903,14 +2922,15 @@ export async function runPlanSkillCounting(opts: {
|
||||
}
|
||||
|
||||
try {
|
||||
await Bun.sleep(8000); // boot grace + auto-trust handler window
|
||||
session.mark();
|
||||
session.send(`${opts.slashCommand}\r`);
|
||||
if (await waitForWork(8000)) { // boot grace is part of the total budget
|
||||
session.mark();
|
||||
session.send(`${opts.slashCommand}\r`);
|
||||
}
|
||||
|
||||
const budgetStart = Date.now();
|
||||
while (Date.now() - budgetStart < timeoutMs) {
|
||||
await Bun.sleep(2000);
|
||||
while (remainingWork() > 0) {
|
||||
if (!await waitForWork(2000)) break;
|
||||
const visible = viewport = await session.currentScreen();
|
||||
if (remainingWork() <= 0) break;
|
||||
transcript = session.hermeticConfigDir
|
||||
? readPlanCountTranscript(session.hermeticConfigDir, fixture.cwd)
|
||||
: { status: 'error', calls: [], assistantMessages: [], error: 'Claude count session has no isolated transcript directory' };
|
||||
@@ -3009,15 +3029,17 @@ export async function runPlanSkillCounting(opts: {
|
||||
const permission = nativeQuestionVisible || terminalHint === 'plan_ready'
|
||||
? null : filePermission(visible, session.visibleText());
|
||||
if (frame === 'permission' || (permission === 'grant' && frame === null)) {
|
||||
if (remainingWork() <= 0) break;
|
||||
if (permission !== 'handled') session.send(`${defaultPick}\r`);
|
||||
await Bun.sleep(1500);
|
||||
await waitForWork(1500);
|
||||
continue;
|
||||
}
|
||||
|
||||
const submissionInput = frame === null ? planCountSubmissionInput(visible) : null;
|
||||
if (submissionInput !== null) {
|
||||
if (remainingWork() <= 0) break;
|
||||
session.send(submissionInput);
|
||||
await Bun.sleep(1500);
|
||||
await waitForWork(1500);
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -3096,17 +3118,23 @@ export async function runPlanSkillCounting(opts: {
|
||||
(isFirstAUQ && opts.firstAUQPick ? opts.firstAUQPick(routing) : defaultPick);
|
||||
isFirstAUQ = false;
|
||||
const questionInput = planCountQuestionInput(visible, fp, pickIdx);
|
||||
if (questionInput.includes('\r') && (prerequisitePick !== null || callerPick !== null)) await selectPtyNumberedOption(session, pickIdx);
|
||||
else session.send(questionInput);
|
||||
if (remainingWork() <= 0) break;
|
||||
if (questionInput.includes('\r') && (prerequisitePick !== null || callerPick !== null)) {
|
||||
// The helper separates digit and Enter by 500ms. Do not let that
|
||||
// delayed confirmation send input after this counting window closes.
|
||||
await selectPtyNumberedOption({ send: input => {
|
||||
if (remainingWork() > 0) session.send(input);
|
||||
} }, pickIdx);
|
||||
} else session.send(questionInput);
|
||||
|
||||
// Give the agent a beat to advance to the next state.
|
||||
await Bun.sleep(2000);
|
||||
await waitForWork(2000);
|
||||
}
|
||||
|
||||
return snapshot(
|
||||
'timeout',
|
||||
`no terminal outcome within ${timeoutMs}ms (step0=${step0Count}, review=${reviewCount})`,
|
||||
viewport = await session.currentScreen(),
|
||||
`no terminal outcome within ${timeoutMs}ms total budget (including startup and ${cleanupReserveMs}ms cleanup reserve; step0=${step0Count}, review=${reviewCount})`,
|
||||
viewport,
|
||||
);
|
||||
} finally {
|
||||
try {
|
||||
|
||||
@@ -57,24 +57,33 @@ function designHandoff(fp: AskUserQuestionFingerprint): { manualIndex: number |
|
||||
.replace(/^next\s+steps?\s*:\s*/i, '');
|
||||
// Scores and a completed decision count describe a closed review. A
|
||||
// condition or unresolved gap cannot masquerade as its next-step menu.
|
||||
const completed = /^Design\s+review\s+(?:is\s+)?complete(?:[.!]|\s+\((?:\d+(?:\.\d+)?(?:\/10)?\s*(?:→|->|to)\s*)?\d+(?:\.\d+)?\/10(?:,\s*\d+\s+decisions?(?:\s+made)?)?\)[.!])(?:\s|$)/i.exec(declaration);
|
||||
const completed = /^Design\s+review\s+(?:is\s+)?complete(?:[.!]|\s+\((?:\d+(?:\.\d+)?(?:\/10)?\s*(?:→|->|to)\s*)?\d+(?:\.\d+)?\/10(?:,\s*\d+\s+decisions?(?:\s+(?:made|added))?)?\)[.!])(?:\s|$)/i.exec(declaration);
|
||||
if (!completed) return null;
|
||||
const requiredGateOffer = /^The required next gate is Eng(?:ineering)? Review\s*[—–-]\s*want me to run it now\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim());
|
||||
const requiredGateQuestion = requiredGateOffer || /^(?:\d+ implementation tasks ready\.\s*)?Eng(?:ineering)? Review is the required shipping gate\.\s*What next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim());
|
||||
// The offered Eng action can carry the required-gate declaration while the
|
||||
// closed question asks only what is next. Its descriptions remain part of
|
||||
// the decision, so they cannot conceal a new repair or conditional closure.
|
||||
const describedRequiredGate = /^What['’]s\s+next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration.slice(completed[0].length).trim()) &&
|
||||
q.options.some(option => /^Run \/plan-eng-review(?:\s*\(recommended\))?$/i.test(option.label.trim()) &&
|
||||
/^Required gate before shipping[.!]/i.test(option.description ?? ''));
|
||||
const guardedNavigation = requiredGateQuestion || describedRequiredGate;
|
||||
if (!requiredGateQuestion && !/\bWhat['’]s\s+next\?\s*<gstack-qid:[a-z0-9-]+>\s*$/i.test(declaration)) return null;
|
||||
// A routing label cannot conceal a new repair in its description.
|
||||
if (requiredGateQuestion && q.options.some(option =>
|
||||
/(?:^|[.!?;]\s+|\b(?:proceed to|continue to|must|need to)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|implement|resolve|decide)\b/i.test(option.description ?? ''))) return null;
|
||||
if (guardedNavigation && q.options.some(option =>
|
||||
/(?:^|[.!?;]\s+|\b(?:proceed to|continue to|must|need to)\s+)(?:(?:please|first|then|also)\s+)*(?:add|fix|repair|implement|resolve|decide)\b|\b(?:(?:should|could|can|would)\s+(?:we|I)|(?:we|I)\s+(?:should|could|can|would))\s+(?:add|fix|repair|implement|resolve|decide)\b/i.test(option.description ?? '') ||
|
||||
/\b(?:Design|the|this)\s+review\s+(?:(?:is|remains)\s+)?(?:not\s+(?:complete|done|resolved)|incomplete|unfinished)\b|\bnot\s+all\s+(?:decisions|findings|issues|gaps)\s+(?:are\s+)?(?:resolved|complete|done)\b|\b(?:decisions|findings|issues|gaps)\s+(?:are\s+)?not\s+(?:resolved|complete|done)\b/i.test(option.description ?? '') ||
|
||||
/\b(?:once|after|when|if|unless|until)\b[^.!?]*\b(?:review|decisions?|findings?|issues?|gaps?)\b[^.!?]*\b(?:complete|done|resolved)\b|\b(?:review|decisions?|findings?|issues?|gaps?)\b[^.!?]*\b(?:complete|done|resolved)\b[^.!?]*\b(?:once|after|when|if|unless|until)\b/i.test(option.description ?? ''))) return null;
|
||||
// A closed heading does not override an affirmative outstanding-work claim
|
||||
// in its recap. Zero/no outstanding work is a compatible completion claim.
|
||||
const outstanding = (requiredGateQuestion ? [declaration, ...q.options.map(o => o.description ?? '')].join('\n') : declaration)
|
||||
const outstanding = (guardedNavigation ? [declaration, ...q.options.map(o => o.description ?? '')].join('\n') : declaration)
|
||||
.replace(/\b(?:no|zero|0)\s+(?:unresolved|open|pending|unaddressed|remaining|outstanding)\s+(?:[a-z-]+\s+){0,3}(?:gaps?|issues?|decisions?|requirements?|work)\b/gi, '')
|
||||
.replace(/\bno\s+(?:gaps?|issues?|decisions?|requirements?|work)\s+remains?\b/gi, '');
|
||||
if (/\b(?:unresolved|open|pending|unaddressed|remaining|outstanding)\s+(?:[a-z-]+\s+){0,3}(?:gaps?|issues?|decisions?|requirements?|work)\b|\b(?:gaps?|issues?|decisions?|requirements?|work)\s+(?:still\s+)?remains?\b|\b(?:gaps?|issues?|decisions?|requirements?|work)\s+(?:is|are)\s+still\s+(?:unresolved|open|pending|unaddressed)\b/i.test(outstanding)) return null;
|
||||
const labels = q.options.map(o => o.label.trim().replace(/^[A-Z][).]\s*/i, '')
|
||||
.replace(/\s*\(recommended\)\s*$/i, '').trim());
|
||||
const manual = labels.map(label => /^(?:Handle next steps manually|Skip\s*[—–-]\s*I['’]ll handle next steps manually)$/i.test(label) ||
|
||||
(requiredGateQuestion && /^Skip\s*[—–-]\s*handle (?:next steps )?manually$/i.test(label)));
|
||||
(guardedNavigation && /^Skip\s*[—–-]\s*handle (?:next steps )?manually$/i.test(label)));
|
||||
const review = labels.map(label => /^Run \/plan-eng-review(?: next)?(?: \(required gate\))?$/i.test(label));
|
||||
const navigation = labels.map(label => /^(?:Skip to implementation|Run \/plan-ceo-review(?: first)?|Run \/design-(?:shotgun|html))$/i.test(label));
|
||||
if (manual.filter(Boolean).length > 1 || !review.some(Boolean) ||
|
||||
|
||||
@@ -139,7 +139,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
// Each one tests behavior the SDK harness can't observe (rendered TTY,
|
||||
// numbered-option lists, multi-phase ordering, idempotency state echo).
|
||||
'auq-format-gate': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completeness-section.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/helpers/llm-judge.ts', 'test/skill-e2e-ask-user-question-format-compliance.test.ts'],
|
||||
'plan-ceo-mode-routing': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/ceo-mode-option.ts', 'test/ceo-mode-option.test.ts', 'test/pty-option-selection.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-mode-routing.test.ts', 'test/ceo-mode-posture-native.test.ts', 'test/fixtures/ceo-hold-posture-l.json', 'test/ceo-mode-labels-native.test.ts', 'test/fixtures/ceo-mode-labels-l.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/ceo-mode-prerequisite.test.ts', 'test/fixtures/ceo-mode-prerequisite-o-calls.json'],
|
||||
'plan-ceo-mode-routing': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/ceo-mode-option.ts', 'test/ceo-mode-option.test.ts', 'test/pty-option-selection.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-mode-routing.test.ts', 'test/ceo-mode-posture-native.test.ts', 'test/fixtures/ceo-hold-posture-l.json', 'test/ceo-mode-labels-native.test.ts', 'test/fixtures/ceo-mode-labels-l.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/ceo-mode-prerequisite.test.ts', 'test/fixtures/ceo-mode-prerequisite-o-calls.json', 'test/fixtures/ceo-mode-prerequisite-q-call.json'],
|
||||
'plan-design-with-ui-scope': ['plan-design-review/**', 'test/fixtures/plans/ui-heavy-feature.md', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/skill-e2e-plan-design-with-ui.test.ts'],
|
||||
'ship-idempotency-pty': ['ship/**', 'bin/gstack-next-version', 'bin/gstack-version-bump', 'scripts/resolvers/sections.ts', 'lib/worktree.ts', 'test/helpers/claude-pty-runner.ts', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/skill-e2e-ship-idempotency.test.ts'],
|
||||
'tpa-present': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'],
|
||||
@@ -148,7 +148,7 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
'tpa-absent-darwin': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'],
|
||||
'tpa-apple-ban': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'],
|
||||
'ship-section-loading': ['ship/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/skill-e2e-ship-section-loading.test.ts'],
|
||||
'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md'],
|
||||
'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md', 'test/fixtures/ceo-section-loading-q-report.md'],
|
||||
// Data-driven behavioral guard for the 'plan'/'prompt' carves (eng, design,
|
||||
// devex, office-hours + future PR2 carves). One file iterating CARVE_GUARDS;
|
||||
// the selector sets GSTACK_CARVE_SKILL=<name> to scope cost to the changed
|
||||
@@ -160,10 +160,10 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
// 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/**', 'test/skill-ceo-section-ordering.test.ts', '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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/fixtures/ceo-completion-handoff-calls.json', 'test/fixtures/ceo-completion-handoff-j-calls.json', 'test/fixtures/ceo-completion-handoff-k-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-finding-count.test.ts', 'test/ceo-completion-handoff-l.test.ts', 'test/fixtures/ceo-completion-handoff-l-calls.json', 'test/ceo-completion-handoff-m.test.ts', 'test/fixtures/ceo-completion-handoff-m-call.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/ceo-handoff-n-calls.json', 'test/ceo-completion-handoff-o.test.ts', 'test/fixtures/ceo-completion-handoff-o-call.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-eng-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/design-count-review.ts', 'test/design-count-review.test.ts', 'test/fixtures/design-review-j-calls.json', 'test/helpers/design-count-outside.ts', 'test/design-count-outside.test.ts', 'test/skill-e2e-plan-design-finding-count.test.ts', 'test/fixtures/design-review-l-calls.json', 'test/design-completion-handoff.test.ts', 'test/fixtures/design-handoff-l-calls.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/design-review-n-calls.json', 'test/design-completion-handoff-scored.test.ts', 'test/fixtures/design-handoff-n-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/devex-review-n-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/devex-output-o.test.ts', 'test/fixtures/devex-review-o-calls.json', 'test/fixtures/devex-output-o-retry-call.json', 'test/devex-setup-remedy-o.test.ts', 'test/fixtures/devex-review-o-retry-calls.json', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', '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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/fixtures/ceo-completion-handoff-calls.json', 'test/fixtures/ceo-completion-handoff-j-calls.json', 'test/fixtures/ceo-completion-handoff-k-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-ceo-finding-count.test.ts', 'test/ceo-completion-handoff-l.test.ts', 'test/fixtures/ceo-completion-handoff-l-calls.json', 'test/ceo-completion-handoff-m.test.ts', 'test/fixtures/ceo-completion-handoff-m-call.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/ceo-handoff-n-calls.json', 'test/ceo-completion-handoff-o.test.ts', 'test/fixtures/ceo-completion-handoff-o-call.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/helpers/ceo-approach-pick.ts', 'test/ceo-approach-pick.test.ts', 'test/fixtures/ceo-approach-q-call.json', 'test/fixtures/ceo-approach-q-paired-call.json', 'test/fixtures/ceo-completion-handoff-q-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/skill-e2e-plan-eng-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/design-count-review.ts', 'test/design-count-review.test.ts', 'test/fixtures/design-review-j-calls.json', 'test/helpers/design-count-outside.ts', 'test/design-count-outside.test.ts', 'test/skill-e2e-plan-design-finding-count.test.ts', 'test/fixtures/design-review-l-calls.json', 'test/design-completion-handoff.test.ts', 'test/fixtures/design-handoff-l-calls.json', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/design-review-n-calls.json', 'test/design-completion-handoff-scored.test.ts', 'test/fixtures/design-handoff-n-calls.json', 'test/fixtures/design-handoff-q-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
'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/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/helpers/devex-count-fixture.ts', 'test/devex-count-fixture.test.ts', 'test/skill-e2e-plan-devex-finding-count.test.ts', 'test/fixtures/devex-review-l-calls.json', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt', 'test/fixtures/devex-review-n-calls.json', 'test/plan-count-empty-review.test.ts', 'test/fixtures/plan-count-design-questionless-report.md', 'test/devex-output-o.test.ts', 'test/fixtures/devex-review-o-calls.json', 'test/fixtures/devex-output-o-retry-call.json', 'test/devex-setup-remedy-o.test.ts', 'test/fixtures/devex-review-o-retry-calls.json', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'],
|
||||
|
||||
// Gate-tier reviewCount-floor counterparts. Catch the May 2026 transcript
|
||||
// bug (model wrote a plan-mode plan and ExitPlanMode'd without firing any
|
||||
@@ -180,8 +180,8 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
// a model fires one AUQ then batches the rest into a "## Decisions to
|
||||
// confirm" plan write. runPlanSkillFloorCheck cannot detect that shape
|
||||
// (it exits on first AUQ); runPlanSkillCounting can.
|
||||
'plan-eng-multi-finding-batching': ['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', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-multi-finding-batching.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'],
|
||||
'plan-ceo-split-overflow': ['bin/gstack-config', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'bin/gstack-question-preference', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-ceo-split-overflow.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'],
|
||||
'plan-eng-multi-finding-batching': ['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', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-eng-multi-finding-batching.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'],
|
||||
'plan-ceo-split-overflow': ['bin/gstack-config', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'bin/gstack-question-preference', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', '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/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.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/plan-count-timeout.test.ts', 'test/plan-count-prerequisite-n.test.ts', 'test/fixtures/ceo-prerequisite-n-call.json', 'test/fixtures/forcing-finding-seeds.ts', 'test/skill-e2e-plan-ceo-split-overflow.test.ts', 'test/plan-count-checkbox.test.ts', 'test/fixtures/ceo-checkbox-l.screen.txt'],
|
||||
'brain-privacy-gate': ['bin/gstack-skill-start', 'bin/gstack-skill-end', 'scripts/resolvers/preamble/generate-brain-sync-block.ts', 'scripts/resolvers/preamble.ts', 'bin/gstack-brain-sync', 'bin/gstack-artifacts-init', 'bin/gstack-config', 'test/helpers/agent-sdk-runner.ts', 'test/skill-e2e-brain-privacy-gate.test.ts'],
|
||||
|
||||
// /setup-gbrain Path 4 (Remote MCP) — happy + bad-token end-to-end via
|
||||
|
||||
@@ -102,7 +102,7 @@ process.stdin.resume();
|
||||
fs.chmodSync(fake, 0o755);
|
||||
fs.writeFileSync(worker, `import {runPlanSkillCounting} from ${JSON.stringify(pathToFileURL(path.resolve(import.meta.dir, 'helpers/claude-pty-runner.ts')).href)};
|
||||
const cases=${JSON.stringify(cases)};
|
||||
const results=await Promise.all(cases.map(async item=>({late:item.late,observation:await runPlanSkillCounting({skillName:'plan-eng-review',slashCommand:'/plan-eng-review',followUpPrompt:'Review this fixture.',isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:35000,expectedPlanPath:item.report,env:{CHECKBOX_CASE:JSON.stringify(item)}})})));
|
||||
const results=await Promise.all(cases.map(async item=>({late:item.late,observation:await runPlanSkillCounting({skillName:'plan-eng-review',slashCommand:'/plan-eng-review',followUpPrompt:'Review this fixture.',isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:48000,expectedPlanPath:item.report,env:{CHECKBOX_CASE:JSON.stringify(item)}})})));
|
||||
await Bun.write(${JSON.stringify(output)},JSON.stringify(results));`);
|
||||
const child = Bun.spawn([process.execPath, worker], {
|
||||
env: { ...process.env, BROWSE_TERMINAL_BINARY: fake, EVALS_HERMETIC: '1' }, stdout: 'pipe', stderr: 'pipe',
|
||||
|
||||
@@ -219,7 +219,7 @@ 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` +
|
||||
`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:33000,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 },
|
||||
@@ -471,7 +471,7 @@ process.stdin.resume();
|
||||
`);
|
||||
fs.chmodSync(fake, 0o755);
|
||||
fs.writeFileSync(worker, `import { runPlanSkillCounting } from ${JSON.stringify(runner)};\n` +
|
||||
`const result = await runPlanSkillCounting({skillName:'plan-eng-review',slashCommand:'/plan-eng-review',followUpPrompt:'# Completion fixture',expectedPlanPath:${JSON.stringify(output)},isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:30000,env:${JSON.stringify({PROBE_PLAN:output,PROBE_INPUTS:record,PROBE_EVENTS:events,PROBE_REPORT:REPORT,PROBE_TERMINAL:terminal})}});\n` +
|
||||
`const result = await runPlanSkillCounting({skillName:'plan-eng-review',slashCommand:'/plan-eng-review',followUpPrompt:'# Completion fixture',expectedPlanPath:${JSON.stringify(output)},isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:43000,env:${JSON.stringify({PROBE_PLAN:output,PROBE_INPUTS:record,PROBE_EVENTS:events,PROBE_REPORT:REPORT,PROBE_TERMINAL:terminal})}});\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' });
|
||||
|
||||
@@ -51,7 +51,7 @@ process.stdin.resume();
|
||||
fs.chmodSync(fake, 0o755);
|
||||
fs.writeFileSync(worker, `import { runPlanSkillCounting } from ${JSON.stringify(runner)};\n` +
|
||||
`if (process.env.BROWSE_TERMINAL_BINARY !== ${JSON.stringify(fake)}) throw new Error('fake CLI not selected');\n` +
|
||||
`const result = await runPlanSkillCounting({skillName:'plan-design-review',slashCommand:'/plan-design-review',followUpPrompt:'# Empty review fixture',expectedPlanPath:${JSON.stringify(report)},isLastStep0AUQ:()=>false,reviewCountCeiling:8,timeoutMs:20000,env:${JSON.stringify({PROBE_PLAN:report,PROBE_INPUTS:inputs,PROBE_PID:pidFile,PROBE_REPORT:REPORT})}});\n` +
|
||||
`const result = await runPlanSkillCounting({skillName:'plan-design-review',slashCommand:'/plan-design-review',followUpPrompt:'# Empty review fixture',expectedPlanPath:${JSON.stringify(report)},isLastStep0AUQ:()=>false,reviewCountCeiling:8,timeoutMs:33000,env:${JSON.stringify({PROBE_PLAN:report,PROBE_INPUTS:inputs,PROBE_PID:pidFile,PROBE_REPORT:REPORT})}});\n` +
|
||||
`await Bun.write(${JSON.stringify(resultFile)},JSON.stringify(result));\n`);
|
||||
const child = Bun.spawn([process.execPath, worker], {
|
||||
env: { ...process.env, EVALS_HERMETIC:'1', EVALS_RUN_ID:'', BROWSE_TERMINAL_BINARY:fake },
|
||||
|
||||
@@ -511,7 +511,7 @@ const results = await Promise.all(cases.map(async (item) => ({
|
||||
: ['late-mode', 'batched-mode'].includes(item.mode) ? devexReviewModePick
|
||||
: item.custom ? fp => fp.promptSnippet.includes('routing-proof-after-240') ? 1 : null : undefined,
|
||||
reviewCountCeiling: 8,
|
||||
timeoutMs: item.mode === 'permission-lifecycle' ? 22000 : 15000,
|
||||
timeoutMs: item.mode === 'permission-lifecycle' ? 35000 : 28000,
|
||||
firstAUQPick: () => ['late-mode', 'batched-mode'].includes(item.mode) ? 1 : 2,
|
||||
env: {
|
||||
FIXTURE_RECORD: item.record, FIXTURE_SKILL: item.skillName, FIXTURE_MODE: item.mode,
|
||||
|
||||
@@ -464,7 +464,7 @@ const results = await Promise.all(cases.map(async item => ({
|
||||
firstAUQPick: item.designQuestions ? undefined : () => 2,
|
||||
pickAUQ: item.designQuestions ? pickDesignCountOutsideVoices : undefined,
|
||||
reviewCountCeiling: 8,
|
||||
timeoutMs: 22000,
|
||||
timeoutMs: 35000,
|
||||
env: { NATIVE_INPUT_CASE: JSON.stringify(item) },
|
||||
}),
|
||||
})));
|
||||
|
||||
@@ -0,0 +1,199 @@
|
||||
/** Free real-Bun/PTY regression: the counting body must settle before its equal outer timeout. */
|
||||
import { 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 { execFileSync } from 'node:child_process';
|
||||
|
||||
const ROOT = path.resolve(import.meta.dir, '..');
|
||||
type Event = { event: string; at: number; invocation: number; pid?: number; start?: string; cwd?: string; data?: string; elapsed?: number; outcome?: string; fixtureGone?: boolean };
|
||||
|
||||
function ownedFake(event: Event, fake: string): boolean {
|
||||
if (!event.pid) return false;
|
||||
try {
|
||||
if (process.platform === 'linux') {
|
||||
const args = fs.readFileSync(`/proc/${event.pid}/cmdline`, 'utf8').split('\0');
|
||||
const start = fs.readFileSync(`/proc/${event.pid}/stat`, 'utf8').split(') ')[1]!.split(' ')[19];
|
||||
return args.includes(fake) && start === event.start;
|
||||
}
|
||||
return execFileSync('ps', ['-p', String(event.pid), '-o', 'command='], { encoding: 'utf8', timeout: 5000 }).includes(fake);
|
||||
} catch { return false; }
|
||||
}
|
||||
|
||||
test.skipIf(process.platform === 'win32')('count timeout settles and cleans its real PTY before Bun retries the equal outer limit', async () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'plan-count-timeout-'));
|
||||
const fake = path.join(dir, 'fake-claude');
|
||||
const worker = path.join(dir, 'worker.test.ts');
|
||||
const eventsPath = path.join(dir, 'events.jsonl');
|
||||
const fixtureTmp = path.join(dir, 'tmp');
|
||||
fs.mkdirSync(fixtureTmp);
|
||||
const helper = pathToFileURL(path.join(ROOT, 'test/helpers/claude-pty-runner.ts')).href;
|
||||
fs.writeFileSync(fake, `#!${process.execPath}\n` + String.raw`
|
||||
import * as fs from 'node:fs';
|
||||
const invocation = Number(process.env.TIMEOUT_INVOCATION);
|
||||
const log = (event, extra = {}) => fs.appendFileSync(process.env.TIMEOUT_EVENTS, JSON.stringify({event, at:Date.now(), invocation, ...extra})+'\n');
|
||||
const start = process.platform === 'linux' ? fs.readFileSync('/proc/self/stat','utf8').split(') ')[1].split(' ')[19] : '';
|
||||
log('ready', {pid:process.pid, start, cwd:process.cwd()});
|
||||
process.stdin.setRawMode(true);
|
||||
process.stdin.resume();
|
||||
process.stdin.on('data', bytes => log('input', {data:bytes.toString()}));
|
||||
process.on('SIGINT', () => log('sigint')); // exercise the owned forced-exit fallback
|
||||
process.stdout.write('Counting lifecycle fixture is ready.\n');
|
||||
setInterval(() => {}, 1000);
|
||||
`, { mode: 0o755 });
|
||||
fs.writeFileSync(worker, `import {test} from 'bun:test';\nimport * as fs from 'node:fs';\nimport {runPlanSkillCounting} from ${JSON.stringify(helper)};\n` + String.raw`
|
||||
let starts = 0;
|
||||
const log = (event, invocation, extra={}) => fs.appendFileSync(process.env.TIMEOUT_EVENTS, JSON.stringify({event,at:Date.now(),invocation,...extra})+'\n');
|
||||
test('owned counting timeout', async () => {
|
||||
const invocation = ++starts;
|
||||
const start = Date.now();
|
||||
log('body', invocation);
|
||||
try {
|
||||
const observation = await runPlanSkillCounting({skillName:'plan-design-review', slashCommand:'/plan-design-review',
|
||||
followUpPrompt:'# Timeout lifecycle fixture\nReview this plan.', isLastStep0AUQ:()=>false,
|
||||
reviewCountCeiling:8, timeoutMs:18000, env:{TIMEOUT_INVOCATION:String(invocation),TIMEOUT_EVENTS:process.env.TIMEOUT_EVENTS}});
|
||||
const ready = fs.readFileSync(process.env.TIMEOUT_EVENTS,'utf8').trim().split('\n').map(line=>JSON.parse(line)).find(e=>e.event==='ready'&&e.invocation===invocation);
|
||||
log('returned', invocation, {elapsed:Date.now()-start,outcome:observation.outcome,fixtureGone:!fs.existsSync(ready.cwd)});
|
||||
throw new Error('HELPER_TIMEOUT_'+invocation);
|
||||
} finally { log('finally', invocation); }
|
||||
}, 18000);
|
||||
`);
|
||||
let rows: Event[] = [];
|
||||
let child: ReturnType<typeof Bun.spawn> | undefined;
|
||||
let output = '', error = '';
|
||||
let status: number | undefined;
|
||||
let watchdog: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
child = Bun.spawn([process.execPath, 'test', '--retry', '1', worker], {
|
||||
cwd: ROOT, env: { ...process.env, BROWSE_TERMINAL_BINARY: fake, EVALS_HERMETIC: '1', EVALS_RUN_ID: '',
|
||||
EVALS: '', GSTACK_EVAL_DIR: path.join(dir, 'evals'), TIMEOUT_EVENTS: eventsPath, TMPDIR: fixtureTmp },
|
||||
stdout: 'pipe', stderr: 'pipe',
|
||||
});
|
||||
watchdog = setTimeout(() => child?.kill('SIGKILL'), 42000);
|
||||
[status, output, error] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]);
|
||||
rows = fs.readFileSync(eventsPath, 'utf8').trim().split('\n').map(line => JSON.parse(line));
|
||||
expect(status, output + error).toBe(1); // each retry retains its explicit timeout failure
|
||||
expect(error).toContain('HELPER_TIMEOUT_1');
|
||||
expect(error).toContain('HELPER_TIMEOUT_2');
|
||||
const starts = rows.filter(e => e.event === 'body');
|
||||
const ready = rows.filter(e => e.event === 'ready');
|
||||
const returned = rows.filter(e => e.event === 'returned');
|
||||
const finished = rows.filter(e => e.event === 'finally');
|
||||
expect(starts).toHaveLength(2);
|
||||
expect(ready).toHaveLength(2);
|
||||
expect(returned).toHaveLength(2);
|
||||
expect(finished).toHaveLength(2);
|
||||
expect(ready[0]!.pid).not.toBe(ready[1]!.pid);
|
||||
expect(ready[0]!.cwd).not.toBe(ready[1]!.cwd);
|
||||
expect(finished[0]!.at).toBeLessThanOrEqual(starts[1]!.at);
|
||||
for (const event of returned) {
|
||||
expect(event.outcome).toBe('timeout');
|
||||
expect(event.elapsed).toBeLessThan(18000);
|
||||
expect(event.fixtureGone).toBe(true);
|
||||
}
|
||||
for (const event of ready) {
|
||||
expect(ownedFake(event, fake)).toBe(false);
|
||||
const body = starts.find(e => e.invocation === event.invocation)!;
|
||||
const inputs = rows.filter(e => e.event === 'input' && e.invocation === event.invocation);
|
||||
expect(inputs.map(e => e.data).join('')).toBe('/plan-design-review\r');
|
||||
expect(inputs.every(e => e.at - body.at < 13000)).toBe(true);
|
||||
}
|
||||
} finally {
|
||||
if (watchdog) clearTimeout(watchdog);
|
||||
if (child && child.exitCode === null) { child.kill('SIGKILL'); await child.exited; }
|
||||
if (fs.existsSync(eventsPath)) rows = fs.readFileSync(eventsPath, 'utf8').trim().split('\n').filter(Boolean).map(line => JSON.parse(line));
|
||||
const cleanup: object[] = [];
|
||||
for (const event of rows.filter(e => e.event === 'ready')) {
|
||||
const owned = ownedFake(event, fake);
|
||||
if (owned) { process.kill(event.pid!, 'SIGKILL'); }
|
||||
cleanup.push({pid:event.pid, start:event.start, wasOwned:owned, signal:owned?'SIGKILL':null});
|
||||
}
|
||||
// Optional immutable owner proof; it never changes the test's assertions.
|
||||
if (process.env.PLAN_COUNT_TIMEOUT_PROOF_DIR) {
|
||||
const dest = process.env.PLAN_COUNT_TIMEOUT_PROOF_DIR;
|
||||
fs.mkdirSync(dest, { recursive: false });
|
||||
for (const name of ['events.jsonl','worker.test.ts','fake-claude']) if (fs.existsSync(path.join(dir,name))) fs.copyFileSync(path.join(dir,name),path.join(dest,name));
|
||||
fs.writeFileSync(path.join(dest,'worker.log'), output + error);
|
||||
fs.writeFileSync(path.join(dest,'result.json'), JSON.stringify({status,cleanup,rows},null,2)+'\n');
|
||||
}
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
}, 48000);
|
||||
|
||||
test.skipIf(process.platform === 'win32')('deadline boundaries stop boot, late screen results and delayed Enter without abandoning cleanup', async () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'plan-count-deadline-'));
|
||||
const fake = path.join(dir, 'fake-claude');
|
||||
const worker = path.join(dir, 'worker.ts');
|
||||
const helper = pathToFileURL(path.join(ROOT, 'test/helpers/claude-pty-runner.ts')).href;
|
||||
const screenModule = pathToFileURL(path.join(ROOT, 'test/helpers/pty-screen.ts')).href;
|
||||
fs.writeFileSync(fake, `#!${process.execPath}\n` + String.raw`
|
||||
import * as fs from 'node:fs';
|
||||
const log = (event, extra={}) => fs.appendFileSync(process.env.BOUNDARY_EVENTS,JSON.stringify({event,at:Date.now(),invocation:1,...extra})+'\n');
|
||||
const start = process.platform === 'linux' ? fs.readFileSync('/proc/self/stat','utf8').split(') ')[1].split(' ')[19] : '';
|
||||
log('ready',{pid:process.pid,start,cwd:process.cwd()});
|
||||
process.stdin.setRawMode(true); process.stdin.resume();
|
||||
process.stdin.on('data',data=>{
|
||||
log('input',{data:data.toString()});
|
||||
process.stdout.write('\x1b[2J\x1b[HWhich remedy should be used?\r\n❯1.First remedy\r\n2.Second remedy\r\n');
|
||||
});
|
||||
process.on('SIGINT',()=>{log('sigint');process.exit(0)});
|
||||
setInterval(()=>{},1000);
|
||||
`, { mode: 0o755 });
|
||||
fs.writeFileSync(worker, `import {mock} from 'bun:test';\nimport * as fs from 'node:fs';\n` +
|
||||
`const screenModule=${JSON.stringify(screenModule)};\nconst originalScreen=(await import(screenModule)).createPtyScreen;\n` + String.raw`
|
||||
const mode=process.env.BOUNDARY_MODE;
|
||||
let start=0, reads=0;
|
||||
const log=(event,extra={})=>fs.appendFileSync(process.env.BOUNDARY_EVENTS,JSON.stringify({event,at:Date.now(),invocation:1,...extra})+'\n');
|
||||
if(mode==='screen') mock.module(screenModule,()=>({createPtyScreen:async(...args)=>{
|
||||
const screen=await originalScreen(...args);
|
||||
return {...screen,read:async()=>{reads++; await Bun.sleep(Math.max(0,start+13200-Date.now()));return screen.read();}};
|
||||
}}));
|
||||
` + `const {runPlanSkillCounting}=await import(${JSON.stringify(helper)});\n` + String.raw`
|
||||
start=Date.now();log('body');
|
||||
const observation=await runPlanSkillCounting({skillName:'plan-design-review',slashCommand:'/plan-design-review',
|
||||
followUpPrompt:'Review the deadline fixture.',isLastStep0AUQ:()=>false,reviewCountCeiling:8,timeoutMs:mode==='boot'?12000:18000,
|
||||
pickAUQ:()=>{log('picker');while(Date.now()-start<12800){};return 2;},
|
||||
env:{BOUNDARY_EVENTS:process.env.BOUNDARY_EVENTS}});
|
||||
const events=fs.readFileSync(process.env.BOUNDARY_EVENTS,'utf8').trim().split('\n').map(line=>JSON.parse(line));
|
||||
log('returned',{outcome:observation.outcome,elapsed:Date.now()-start,reads,fixtureGone:!fs.existsSync(events.find(e=>e.event==='ready').cwd)});
|
||||
`);
|
||||
const children: ReturnType<typeof Bun.spawn>[] = [];
|
||||
const timers: ReturnType<typeof setTimeout>[] = [];
|
||||
try {
|
||||
const results = await Promise.allSettled(['boot','screen','enter'].map(async mode => {
|
||||
const eventPath=path.join(dir,mode+'.jsonl');
|
||||
const fixtureTmp=path.join(dir,mode); fs.mkdirSync(fixtureTmp);
|
||||
const child=Bun.spawn([process.execPath,worker], {cwd:ROOT,
|
||||
env:{...process.env,BROWSE_TERMINAL_BINARY:fake,EVALS_HERMETIC:'1',EVALS:'',EVALS_RUN_ID:'',
|
||||
GSTACK_EVAL_DIR:path.join(dir,'evals-'+mode),TMPDIR:fixtureTmp,BOUNDARY_MODE:mode,BOUNDARY_EVENTS:eventPath},stdout:'pipe',stderr:'pipe'});
|
||||
children.push(child);timers.push(setTimeout(()=>child.kill('SIGKILL'),25000));
|
||||
const [code,out,err]=await Promise.all([child.exited,new Response(child.stdout).text(),new Response(child.stderr).text()]);
|
||||
expect(code,out+err).toBe(0);
|
||||
const events=fs.readFileSync(eventPath,'utf8').trim().split('\n').map(line=>JSON.parse(line));
|
||||
const returned=events.find(e=>e.event==='returned');
|
||||
expect(returned.outcome).toBe('timeout'); expect(returned.fixtureGone).toBe(true);
|
||||
expect(returned.elapsed).toBeLessThan(mode==='boot'?12000:18000);
|
||||
const input=events.filter(e=>e.event==='input').map(e=>e.data).join('');
|
||||
expect(input).toBe(mode==='boot'?'':mode==='screen'?'/plan-design-review\r':'/plan-design-review\r2');
|
||||
if(mode==='screen') { expect(returned.reads).toBe(1);expect(events.some(e=>e.event==='picker')).toBe(false); }
|
||||
expect(ownedFake(events.find(e=>e.event==='ready'),fake)).toBe(false);
|
||||
}));
|
||||
for (const result of results) if (result.status === 'rejected') throw result.reason;
|
||||
} finally {
|
||||
timers.forEach(clearTimeout);
|
||||
for(const child of children) if(child.exitCode===null){child.kill('SIGKILL');await child.exited;}
|
||||
for(const mode of ['boot','screen','enter']) {
|
||||
const eventPath=path.join(dir,mode+'.jsonl');
|
||||
if(!fs.existsSync(eventPath))continue;
|
||||
for(const event of fs.readFileSync(eventPath,'utf8').trim().split('\n').filter(Boolean).map(line=>JSON.parse(line))) {
|
||||
if(event.event==='ready'&&ownedFake(event,fake))process.kill(event.pid,'SIGKILL');
|
||||
}
|
||||
}
|
||||
if(process.env.PLAN_COUNT_TIMEOUT_PROOF_DIR) {
|
||||
const dest=process.env.PLAN_COUNT_TIMEOUT_PROOF_DIR+'-boundaries';fs.mkdirSync(dest,{recursive:false});
|
||||
for(const name of ['worker.ts','fake-claude','boot.jsonl','screen.jsonl','enter.jsonl'])if(fs.existsSync(path.join(dir,name)))fs.copyFileSync(path.join(dir,name),path.join(dest,name));
|
||||
}
|
||||
fs.rmSync(dir,{recursive:true,force:true});
|
||||
}
|
||||
},30000);
|
||||
@@ -109,7 +109,7 @@ process.stdin.on('data',()=>{if(sent)return;sent=true;
|
||||
`, {mode:0o755});
|
||||
const worker = path.join(f.dir, 'long-summary-worker.ts');
|
||||
fs.writeFileSync(worker, `import {runPlanSkillCounting} from ${JSON.stringify(runner)};
|
||||
const result=await runPlanSkillCounting({skillName:'plan-ceo-review',slashCommand:'/plan-ceo-review',followUpPrompt:'# Long summary fixture',expectedPlanPath:${JSON.stringify(output)},isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:15000});
|
||||
const result=await runPlanSkillCounting({skillName:'plan-ceo-review',slashCommand:'/plan-ceo-review',followUpPrompt:'# Long summary fixture',expectedPlanPath:${JSON.stringify(output)},isLastStep0AUQ:()=>false,isReviewAUQ:()=>true,reviewCountCeiling:8,timeoutMs:28000});
|
||||
if(result.outcome!=='completion_summary'||result.reviewCount!==1||result.evidence.includes('## Completion Summary'))throw new Error(JSON.stringify(result));
|
||||
process.stdout.write('native-summary-complete-after-heading-scrolled');`);
|
||||
const child = Bun.spawn([process.execPath, worker], {cwd:ROOT,env:{...process.env,EVALS_HERMETIC:'1',EVALS_RUN_ID:'',BROWSE_TERMINAL_BINARY:f.fake},stdout:'pipe',stderr:'pipe'});
|
||||
|
||||
@@ -17,7 +17,8 @@
|
||||
|
||||
import { test } from 'bun:test';
|
||||
import { describeE2ETier } from './helpers/e2e-gate';
|
||||
import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff';
|
||||
import { isCeoCompletionHandoff } from './helpers/ceo-completion-handoff';
|
||||
import { pickCeoCountQuestion } from './helpers/ceo-approach-pick';
|
||||
import * as fs from 'node:fs';
|
||||
import * as os from 'node:os';
|
||||
import * as path from 'node:path';
|
||||
@@ -184,7 +185,7 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', ()
|
||||
isLastStep0AUQ: ceoStep0Boundary,
|
||||
isFirstReviewAUQ: ceoFirstReviewAUQ,
|
||||
isCompletionHandoffAUQ: isCeoCompletionHandoff,
|
||||
pickAUQ: pickCeoCompletionHandoff,
|
||||
pickAUQ: pickCeoCountQuestion,
|
||||
reviewCountCeiling: CEILING_DISTINCT + 1, // hard cap above assertion ceiling
|
||||
firstAUQPick: pickSkipInterview, // bypass scope-selection, route to review
|
||||
timeoutMs: 1_500_000, // 25 min
|
||||
@@ -271,7 +272,7 @@ describeE2E('/plan-ceo-review per-finding AskUserQuestion count (periodic)', ()
|
||||
isLastStep0AUQ: ceoStep0Boundary,
|
||||
isFirstReviewAUQ: ceoFirstReviewAUQ,
|
||||
isCompletionHandoffAUQ: isCeoCompletionHandoff,
|
||||
pickAUQ: pickCeoCompletionHandoff,
|
||||
pickAUQ: pickCeoCountQuestion,
|
||||
reviewCountCeiling: CEILING_PAIRED + 1,
|
||||
timeoutMs: 1_500_000,
|
||||
env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' },
|
||||
|
||||
Reference in new issue
Block a user