diff --git a/test/ceo-parenthesized-issue-ah.test.ts b/test/ceo-parenthesized-issue-ah.test.ts new file mode 100644 index 000000000..cbb641649 --- /dev/null +++ b/test/ceo-parenthesized-issue-ah.test.ts @@ -0,0 +1,160 @@ +import { expect, test } from 'bun:test'; +import fixture from './fixtures/ceo-parenthesized-issue-ah.json'; +import { ceoFirstReviewAUQ, ceoStep0Boundary, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +import { E2E_TOUCHFILES, selectTests } from './helpers/touchfiles'; + +const calls = () => structuredClone(fixture.calls) as NativePlanQuestionCall[]; +const fp = (c: NativePlanQuestionCall) => nativePlanCallFingerprint(c, 0, true); +function reanswer(c: NativePlanQuestionCall) { + c.answers = { [c.questions[0]!.question]: c.questions[0]!.options[0]!.label }; + return c; +} +function changed(original: NativePlanQuestionCall, mutate: (c: NativePlanQuestionCall) => void) { + const c = structuredClone(original); mutate(c); return reanswer(c); +} + +test('both exact completed Issue questions start review with descriptive headers', () => { + for (const c of calls()) expect(ceoFirstReviewAUQ(fp(c))).toBe(true); + let started = false; + const counts = { setup: 0, review: 0 }; + for (const c of [...fixture.setupCalls, ...calls()] as NativePlanQuestionCall[]) { + const phase = planCountQuestionPhase(fp(c), started, ceoStep0Boundary, ceoFirstReviewAUQ); + started = phase.reviewStarted; + counts[phase.preReview ? 'setup' : 'review']++; + } + expect(counts).toEqual({ setup: 2, review: 2 }); + expect(fixture.historicalOutcome).toBe('no_review_questions'); +}); + +test('fixture questions and selected answers are exact owned public request/result projections', () => { + for (const c of calls()) { + const requests = fixture.publicEvents.filter(e => e.record.message.content.some(b => 'id' in b && b.id === c.toolUseId)); + const results = fixture.publicEvents.filter(e => e.record.message.content.some(b => 'tool_use_id' in b && b.tool_use_id === c.toolUseId)); + expect(requests).toHaveLength(1); expect(results).toHaveLength(1); + expect(requests[0]!.record.sessionId).toBe(c.sessionId); + expect(results[0]!.record.sessionId).toBe(c.sessionId); + const request = requests[0]!.record.message.content.find(b => 'id' in b && b.id === c.toolUseId) as any; + expect(request.input.questions).toEqual(c.questions); + const result = results[0]!.record.message.content.find(b => 'tool_use_id' in b && b.tool_use_id === c.toolUseId) as any; + expect(result.is_error).not.toBe(true); + expect(result.content).toContain(`"${c.questions[0]!.question}"="${c.answers![c.questions[0]!.question]}"`); + expect(c.answeredAt).toBe(results[0]!.record.timestamp); + } +}); + +test('complete current native identity and actual offered answer remain required', () => { + for (const original of calls()) { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { c.sessionId = ''; }, + (c: NativePlanQuestionCall) => { c.toolUseId = ''; }, + (c: NativePlanQuestionCall) => { c.answers = {}; }, + (c: NativePlanQuestionCall) => { c.answers = { 'prior question': c.questions[0]!.options[0]!.label }; }, + (c: NativePlanQuestionCall) => { c.answers![c.questions[0]!.question] = 'foreign answer'; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + (c: NativePlanQuestionCall) => { c.questions.push(structuredClone(c.questions[0]!)); }, + ]) { + const c = structuredClone(original); mutate(c); + expect(ceoFirstReviewAUQ(fp(c))).toBe(false); + } + expect(ceoFirstReviewAUQ({ ...fp(original), signature: 'foreign:call' })).toBe(false); + expect(ceoFirstReviewAUQ({ ...fp(original), nativeCall: undefined })).toBe(false); + expect(ceoFirstReviewAUQ({ ...fp(original), options: [] })).toBe(false); + } +}); + +test('title, recommendation, every option and any numbered header share one issue identity', () => { + for (const original of calls()) { + const n = /\(Issue (\d+)\)/.exec(original.questions[0]!.question)![1]!; + for (const header of [`Finding ${n}`, `Issue ${n}`, `F${n}`]) { + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.header = header; })))).toBe(true); + } + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/^Recommendation:.*\n/m, ''); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/^Recommendation: \d+A/m, 'Recommendation: 99A'); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/^Recommendation: \d+A/m, `Recommendation: ${n}Z`); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = '99B) Different issue'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = c.questions[0]!.options[0]!.label; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.description = ''; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Finding 99'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Finding'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/D\d+ \(Issue \d+\) — /, ''); c.questions[0]!.header = `Issue ${n}`; }, + ]) expect(ceoFirstReviewAUQ(fp(changed(original, mutate)))).toBe(false); + } +}); + +test('source, conditional and stale or withdrawn briefs do not start review', () => { + for (const original of calls()) { + for (const prefix of ['Example: ', 'If requested: ', '> ', ' ', '```text\n']) { + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.question = prefix + c.questions[0]!.question; })))).toBe(false); + } + for (const framing of ['If this hypothetical plan were adopted, ', 'Example: ', 'Historical example only. ', 'Quoted assessment: ']) { + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.question = c.questions[0]!.question.replace('ELI10: ', `ELI10: ${framing}`); })))).toBe(false); + } + for (const tail of ['No current defect exists.', 'Correction: this issue is already resolved.', 'This question is only an example.', 'I withdraw this finding.']) { + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.question += '\n' + tail; })))).toBe(false); + } + for (const prefix of ['> ', ' ', '```\n']) { + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.question = c.questions[0]!.question.replace(/^ELI10:/m, prefix + 'ELI10:'); })))).toBe(false); + } + // Later attributed source text does not withdraw a present decision. + expect(ceoFirstReviewAUQ(fp(changed(original, c => { c.questions[0]!.question += '\nAn old note said: "No current defect exists."'; })))).toBe(true); + } +}); + +test('qid and setup exclusions apply before the new identity form', () => { + for (const original of calls()) { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.question += ' '; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/]+>/, ''); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/]+>/, ''); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace(/]+>/, ''); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.header = 'Approach'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.label = 'HOLD SCOPE'; }, + ]) expect(ceoFirstReviewAUQ(fp(changed(original, mutate)))).toBe(false); + } +}); + +test('identity recognition is independent of observed component and numbering', () => { + for (const original of calls()) { + const c = changed(original, c => { + const q = c.questions[0]!; const n = /\(Issue (\d+)\)/.exec(q.question)![1]!; + q.header = 'Notification state'; + q.question = q.question.replace(/^D\d+/, 'D24').replace(`(Issue ${n})`, '(Issue 17)') + .replace(new RegExp(`\\b${n}([ABC])\\b`, 'g'), '17$1') + .replace(/Stripe/g, 'PaymentProvider').replace(/email/g, 'notification').replace(/userId/g, 'accountKey'); + q.options.forEach(o => { o.label = o.label.replace(new RegExp(`^${n}`), '17'); }); + }); + expect(ceoFirstReviewAUQ(fp(c))).toBe(true); + c.answers = { [c.questions[0]!.question]: c.questions[0]!.options.at(-1)!.label }; + expect(ceoFirstReviewAUQ(fp(c))).toBe(true); + } +}); + +test('new fixture and controls select only their existing CEO count owner', () => { + for (const file of ['test/ceo-parenthesized-issue-ah.test.ts', 'test/fixtures/ceo-parenthesized-issue-ah.json']) { + expect(selectTests([file], E2E_TOUCHFILES).selected).toEqual(['plan-ceo-finding-count']); + } +}); + +test('numbered administrative, literal-only, hypothetical and withdrawn briefs are not defects', () => { + for (const original of calls()) { + for (const mutate of [ + (q: NativePlanQuestionCall['questions'][number]) => { + const n = /\(Issue (\d+)\)/.exec(q.question)![1]!; + q.question = q.question.replace(/^(D\d+ \(Issue \d+\) — ).*/, '$1How should we archive this completed review?') + .replace(/^ELI10:.*$/m, 'ELI10: The review is complete. This choice only saves the finished report.'); + q.options.forEach((o, i) => { o.label = `${n}${String.fromCharCode(65 + i)}) Save report format ${i}`; o.description = 'Store the completed review report.'; }); + }, + (q: NativePlanQuestionCall['questions'][number]) => { q.question += '\nThere is no defect or unresolved issue; this is a historical example.'; }, + (q: NativePlanQuestionCall['questions'][number]) => { q.question = q.question.replace(/^ELI10:.*$/m, 'ELI10: `The plan has no error handling.`'); }, + (q: NativePlanQuestionCall['questions'][number]) => { q.question = q.question.replace(/^(D\d+ \(Issue \d+\) — ).*/, '$1What should happen if a hypothetical future handler lacked error handling?'); }, + ]) { + const c = changed(original, c => mutate(c.questions[0]!)); + expect(ceoFirstReviewAUQ(fp(c))).toBe(false); + } + } +}); diff --git a/test/eng-next-handoff-ah.test.ts b/test/eng-next-handoff-ah.test.ts new file mode 100644 index 000000000..d6d676f7d --- /dev/null +++ b/test/eng-next-handoff-ah.test.ts @@ -0,0 +1,125 @@ +import { expect, test } from 'bun:test'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import actual from './fixtures/eng-next-handoff-ah.json'; +import { isEngCompletionHandoff } from './helpers/eng-completion-handoff'; +import { hasNativePlanTerminal, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import type { NativePlanQuestionCall, PlanCountTranscript } from './helpers/plan-count-transcript'; +import { isCurrentPlanApprovalScreen } from './helpers/plan-count-pending-exit'; +import { E2E_TOUCHFILES, matchGlob } from './helpers/touchfiles'; + +const call = () => structuredClone(actual.fingerprint.nativeCall) as NativePlanQuestionCall; +const fp = (c = call()) => nativePlanCallFingerprint(c, 0, false); +const accepts = (c = call(), plan = actual.plan) => isEngCompletionHandoff(fp(c), plan); +function question(c: NativePlanQuestionCall, f: (s: string) => string) { + const q = c.questions[0]!, answer = c.answers![q.question]; + q.question = f(q.question); c.answers = { [q.question]: answer! }; return c; +} + +test('actual completed Next navigation is administrative and never starts review', () => { + expect(accepts()).toBe(true); + for (const started of [false, true]) { + expect(planCountQuestionPhase(fp(), started, () => false, undefined, undefined, + f => isEngCompletionHandoff(f, actual.plan))).toEqual({ preReview: false, reviewStarted: started, administrative: 'completion-handoff' }); + } +}); + +test('published confirmation and characterization references do not introduce work', () => { + expect(actual.source.stat.mtimeMs).toBeLessThan(Date.parse(call().answeredAt!)); + expect(accepts(call(), actual.plan.replaceAll('P0', 'P7'))).toBe(true); + const c = call(); c.questions[0]!.options.reverse(); + expect(accepts(c)).toBe(true); + c.answers![c.questions[0]!.question] = c.questions[0]!.options[0]!.label; + expect(accepts(c)).toBe(true); + expect(accepts(call(), actual.plan.replace(' - Surfaced by: Architecture issue 3 (D7)', ' - Correction: T2 is cancelled.\n - Surfaced by: Architecture issue 3 (D7)'))).toBe(true); + expect(accepts(call(), actual.plan.replace('Write characterization tests for `legacyAuthFlow()` before any rewrite', 'Write characterization tests for `legacyAuthFlow()` before any rewrite\nVerify expired and revoked tokens are rejected.'))).toBe(true); + expect(accepts(call(), actual.plan.replace('Invariants and Latency target above.', 'Invariants and Latency target above.\nKeep a record of rejected alternatives after the author confirms Context.'))).toBe(true); +}); + +test('incomplete, foreign, ambiguous and changed choices cannot be administrative', () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { c.answeredAt = 'invalid'; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { c.answers = {}; }, + (c: NativePlanQuestionCall) => { c.answers![c.questions[0]!.question] = 'unoffered'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + (c: NativePlanQuestionCall) => { c.questions.push(structuredClone(c.questions[0]!)); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options.push({ label: 'Add another requirement' }); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.description += ' Add a new datastore first.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.description = 'Change the implementation architecture first.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[0]!.description = c.questions[0]!.options[0]!.description!.replace('T1', 'T99'); }, + ]) { const c = call(); mutate(c); expect(accepts(c)).toBe(false); } + expect(isEngCompletionHandoff({ ...fp(), signature: 'foreign:call' }, actual.plan)).toBe(false); + expect(isEngCompletionHandoff({ ...fp(), nativeQuestionIndex: 1 }, actual.plan)).toBe(false); + expect(isEngCompletionHandoff({ ...fp(), options: [] }, actual.plan)).toBe(false); +}); + +test('nonasserted, prospective, conditional and reopened navigation stays substantive', () => { + for (const text of [ + '> ', 'Example: ', 'An unproven hypothesis. ', '```text\n', + ]) expect(accepts(question(call(), s => text + s))).toBe(false); + for (const change of [ + (s: string) => s.replace('all required reviews are complete', 'all required reviews will be complete'), + (s: string) => s.replace('all required reviews are complete', 'all required reviews are not complete'), + (s: string) => s.replace('all required reviews are complete', 'all required reviews are complete if more tests pass'), + (s: string) => s + '\nA new implementation prerequisite is required.', + (s: string) => s.replace('Recommendation: A', 'Recommendation: C'), + ]) expect(accepts(question(call(), change))).toBe(false); +}); + +test('missing, refuted or quoted published prerequisites/tasks cannot be borrowed', () => { + for (const plan of [ + '', '```markdown\n' + actual.plan + '\n```', actual.plan.split('\n').map(s => '> ' + s).join('\n'), + actual.plan.replace('## Context', '## Example context'), + actual.plan.replace('Implementation does not start until the author confirms', 'Implementation starts without the author confirming'), + actual.plan.replace('### Prerequisite P0', '### Example prerequisite P0'), + actual.plan.replace('## Implementation Tasks', '## Historical Tasks'), + actual.plan.replace('Write characterization tests for `legacyAuthFlow()` before any rewrite', 'Write characterization tests after rewriting `legacyAuthFlow()`'), + actual.plan.replace('**T1 (P1', '**T99 (P1'), + actual.plan.replace('## Context', 'Example only:\n## Context'), + actual.plan.replace('## Implementation Tasks', 'Example only:\n## Implementation Tasks'), + actual.plan.replace('Invariants and Latency target above.', 'Invariants and Latency target above.\nCorrection: Prerequisite P0 is cancelled; the author no longer needs to confirm Context.'), + actual.plan.replace('Write characterization tests for `legacyAuthFlow()` before any rewrite', 'Write characterization tests for `legacyAuthFlow()` before any rewrite\nCorrection: T1 is cancelled; no characterization tests are required.'), + ]) expect(accepts(call(), plan)).toBe(false); +}); + +test('exact final exit/report replay retains all freshness, identity and answer gates', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-eng-next-ah-')); + const file = path.join(dir, 'reviewed.md'); + const now = Date.now; + try { + fs.writeFileSync(file, actual.plan); + fs.utimesSync(file, actual.source.stat.mtimeMs / 1000, actual.source.stat.mtimeMs / 1000); + Date.now = () => Date.parse(actual.captureAt); + const t = structuredClone(actual.transcript) as PlanCountTranscript; + const id = actual.fingerprint.signature; + const admin = new Set(accepts() ? [id] : []); + const check = (v = t, a = admin) => hasNativePlanTerminal(v, file, actual.startedAt, 'plan_ready', a); + expect(isCurrentPlanApprovalScreen(actual.screen)).toBe(true); + expect(check()).toBe(true); + expect(check(t, new Set())).toBe(false); + expect(check(t, new Set(['foreign:call']))).toBe(false); + for (const mutate of [ + (v: PlanCountTranscript) => { v.planReadyRequests = []; }, + (v: PlanCountTranscript) => { v.planReadyRequests!.at(-1)!.failed = true; }, + (v: PlanCountTranscript) => { v.planReadyRequests!.at(-1)!.sessionId = 'foreign'; }, + (v: PlanCountTranscript) => { v.planReadyRequests!.at(-1)!.timestamp = '2026-09-10T03:29:40.000Z'; }, + (v: PlanCountTranscript) => { v.planReadyRequests!.at(-1)!.timestamp = new Date(Date.now() + 1).toISOString(); }, + (v: PlanCountTranscript) => { v.calls.at(-1)!.answered = false; }, + (v: PlanCountTranscript) => { v.calls.at(-2)!.answeredAt = '2026-09-10T03:29:00.000Z'; }, + ]) { const v = structuredClone(t); mutate(v); expect(check(v)).toBe(false); } + fs.writeFileSync(file, actual.plan.replace('NO UNRESOLVED DECISIONS', 'Report still pending')); + fs.utimesSync(file, actual.source.stat.mtimeMs / 1000, actual.source.stat.mtimeMs / 1000); + expect(check()).toBe(false); + } finally { Date.now = now; fs.rmSync(dir, { recursive: true, force: true }); } +}); + +test('new handoff evidence belongs to its existing paid caller', () => { + for (const file of ['test/eng-next-handoff-ah.test.ts', 'test/fixtures/eng-next-handoff-ah.json']) { + const owners = Object.entries(E2E_TOUCHFILES).filter(([, globs]) => globs.some(glob => matchGlob(file, glob))).map(([name]) => name); + expect(owners).toEqual(['plan-eng-finding-count']); + } +}); diff --git a/test/fixtures/ceo-parenthesized-issue-ah.json b/test/fixtures/ceo-parenthesized-issue-ah.json new file mode 100644 index 000000000..5198af303 --- /dev/null +++ b/test/fixtures/ceo-parenthesized-issue-ah.json @@ -0,0 +1,293 @@ +{ + "sourceHead": "c9e7a422be47b69d855f48cfce9024457e0dd4fc", + "sourceSnapshot": { + "path": ".context/ship-source-ah-delta-paid-20260910-v1/ceo-first-finding-checkpoint-ledger-v1/public-snapshots.json", + "sha256": "b60ad4921c649276c52d8f09caf45178d91d028b56560e49610165faede4eed9" + }, + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "cwd": "/tmp/gstack-paid-shard-zcm490/tmp/gstack-plan-count-q8859d", + "publicEvents": [ + { + "line": 107, + "record": { + "type": "assistant", + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "cwd": "/tmp/gstack-paid-shard-zcm490/tmp/gstack-plan-count-q8859d", + "isSidechain": false, + "timestamp": "2026-09-10T03:17:25.755Z", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_01M2hWu9BLPXb7e1wjfF23L5", + "name": "AskUserQuestion", + "input": { + "questions": [ + { + "header": "Email leg", + "question": "D4 (Issue 1) \u2014 What should the handler do when the notification email fails after the payment update commits?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Webhook fan-out\": update + email inline, no error handling on the email leg.\nELI10: The database write says \"this user paid\" and commits. Then the email call throws. Today that exception turns into a 500, which tells Stripe \"try again\". But the payment is already saved, so the retry either gets swallowed by the dedup guard (no email ever goes out) or re-runs the handler and can send a second email. Stripe's failed-webhook alert fires for a payment that actually succeeded, which the runbook says must never trigger a blind replay.\nStakes if we pick wrong: paid users get zero or two receipts, on-call gets paged for successful payments, and a permanently rejected address retries for 72 hours.\nRecommendation: 1A because it is the only option where a committed payment returns 200 and the failure is still fully visible through the mail client's existing metric, alert and runbook. Maps to your preference: every error has a name, zero silent failures.\nCompleteness: 1A=9/10, 1B=4/10, 1C=2/10\nPros / cons:\n1A) Order: commit update -> record dedup completion -> send email. Rescue only the mail client's named classes (MailDeliveryError, MailTimeoutError, MailRejectedError). Log at error with event ID, user ID, payment intent ID, exception class. Return 200. If the existing notification retry procedure has a programmatic entry point, call it in the rescue; otherwise the logged correlation feeds the runbook. Tests: one per rescued class asserting 200, update persisted, error log emitted; one asserting DB errors still propagate as 500. (recommended) (human ~3h / CC ~15 min)\n \u2705 Committed payments never report failure to Stripe, so no false retries or alert noise\n \u2705 Named rescues only; DB failures keep their retry-via-500 contract untouched\n \u2705 Failure remains visible via the mail client's failure-rate metric, alert, and correlated trace\n \u274c Adds a rescue block and ordering rule the implementer must get right; needs the tests listed\n1B) Keep no rescue, but move the email after commit and dedup record so a retry cannot double-send (human ~30 min / CC ~5 min)\n \u2705 Tiny change; no new rescue code in the handler\n \u2705 Double email is prevented because the retry hits the dedup guard\n \u274c A mail failure still returns 500 and pages on-call for a successful payment; the retry acks 200 and the notification is silently never sent\n1C) Do nothing; keep the plan as written (human 0 / CC 0)\n \u2705 No implementation work\n \u2705 Behavior is at least consistent with the prior handler if it did the same\n \u274c Ordering is unspecified, so a mail outage can either roll back a real payment or send duplicate emails, and every mail failure looks like a webhook failure\nNet: a rescue with a name and a 200, versus letting Stripe retry a payment that already committed. ", + "options": [ + { + "label": "1A) Named rescue, 200, log+trace (recommended)", + "description": "Completeness 9/10. Commit -> dedup record -> email; rescue only mail classes; log correlated; return 200; tests per class." + }, + { + "label": "1B) Reorder only, no rescue", + "description": "Completeness 4/10. Email after commit + dedup record; failures still 500 and page on-call." + }, + { + "label": "1C) Do nothing", + "description": "Completeness 2/10. Keep inline email with no rescue and unspecified ordering." + } + ], + "multiSelect": false + } + ] + }, + "caller": { + "type": "direct" + } + } + ] + } + } + }, + { + "line": 108, + "record": { + "type": "user", + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "cwd": "/tmp/gstack-paid-shard-zcm490/tmp/gstack-plan-count-q8859d", + "isSidechain": false, + "timestamp": "2026-09-10T03:17:27.593Z", + "message": { + "role": "user", + "content": [ + { + "type": "tool_result", + "content": "Your questions have been answered: \"D4 (Issue 1) \u2014 What should the handler do when the notification email fails after the payment update commits?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Webhook fan-out\": update + email inline, no error handling on the email leg.\nELI10: The database write says \"this user paid\" and commits. Then the email call throws. Today that exception turns into a 500, which tells Stripe \"try again\". But the payment is already saved, so the retry either gets swallowed by the dedup guard (no email ever goes out) or re-runs the handler and can send a second email. Stripe's failed-webhook alert fires for a payment that actually succeeded, which the runbook says must never trigger a blind replay.\nStakes if we pick wrong: paid users get zero or two receipts, on-call gets paged for successful payments, and a permanently rejected address retries for 72 hours.\nRecommendation: 1A because it is the only option where a committed payment returns 200 and the failure is still fully visible through the mail client's existing metric, alert and runbook. Maps to your preference: every error has a name, zero silent failures.\nCompleteness: 1A=9/10, 1B=4/10, 1C=2/10\nPros / cons:\n1A) Order: commit update -> record dedup completion -> send email. Rescue only the mail client's named classes (MailDeliveryError, MailTimeoutError, MailRejectedError). Log at error with event ID, user ID, payment intent ID, exception class. Return 200. If the existing notification retry procedure has a programmatic entry point, call it in the rescue; otherwise the logged correlation feeds the runbook. Tests: one per rescued class asserting 200, update persisted, error log emitted; one asserting DB errors still propagate as 500. (recommended) (human ~3h / CC ~15 min)\n \u2705 Committed payments never report failure to Stripe, so no false retries or alert noise\n \u2705 Named rescues only; DB failures keep their retry-via-500 contract untouched\n \u2705 Failure remains visible via the mail client's failure-rate metric, alert, and correlated trace\n \u274c Adds a rescue block and ordering rule the implementer must get right; needs the tests listed\n1B) Keep no rescue, but move the email after commit and dedup record so a retry cannot double-send (human ~30 min / CC ~5 min)\n \u2705 Tiny change; no new rescue code in the handler\n \u2705 Double email is prevented because the retry hits the dedup guard\n \u274c A mail failure still returns 500 and pages on-call for a successful payment; the retry acks 200 and the notification is silently never sent\n1C) Do nothing; keep the plan as written (human 0 / CC 0)\n \u2705 No implementation work\n \u2705 Behavior is at least consistent with the prior handler if it did the same\n \u274c Ordering is unspecified, so a mail outage can either roll back a real payment or send duplicate emails, and every mail failure looks like a webhook failure\nNet: a rescue with a name and a 200, versus letting Stripe retry a payment that already committed. \"=\"1A) Named rescue, 200, log+trace (recommended)\". You can now continue with these answers in mind.", + "tool_use_id": "toolu_01M2hWu9BLPXb7e1wjfF23L5" + } + ] + } + } + }, + { + "line": 122, + "record": { + "type": "assistant", + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "cwd": "/tmp/gstack-paid-shard-zcm490/tmp/gstack-plan-count-q8859d", + "isSidechain": false, + "timestamp": "2026-09-10T03:18:20.407Z", + "message": { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_019pkzdAQt72RfCswrXXvvyc", + "name": "AskUserQuestion", + "input": { + "questions": [ + { + "header": "SQL lookup", + "question": "D5 (Issue 2) \u2014 How should the handler look up the user from `request.params.userId`?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Database access\": userId read directly into a raw SQL fragment.\nELI10: `userId` is a string copied out of Stripe metadata. Our own server usually writes it, but the plan itself says nothing casts, escapes or validates it, and a valid Stripe signature only proves Stripe sent it, not that the string is safe. Pasting it into SQL means anyone who can set that metadata (a leaked API key, a dashboard edit, a bug in intent creation) can run their own SQL against the users table. Separately, a garbage string that fails the ID column's type cast raises a database error, which becomes a 500, which makes Stripe retry an unfixable event for three days.\nStakes if we pick wrong: data exfiltration or destruction through the payment path, plus 72 hours of retries and alerts for one malformed event.\nRecommendation: 2A because it closes injection completely and gives a malformed ID the same 200-and-warn treatment the adapter already gives a missing one. Maps to your preferences: security is not optional, explicit over clever, DRY (reuse the existing lookup helper).\nCompleteness: 2A=10/10, 2B=6/10, 2C=0/10\nPros / cons:\n2A) Validate then bind: parse userId to the users primary-key type (integer or UUID, whichever the schema uses; reject anything else including oversized or unicode strings). On parse failure log a warning with event ID and the rejected value length (not the value), ack 200 (retry cannot fix it). On success call the existing lookup helper, which uses a bound parameter; never build SQL text from the value. Tests: injection payload never reaches the DB and returns 200 + warning; wrong-type string returns 200 + warning; valid ID hits the existing lookup guard paths. (recommended) (human ~2h / CC ~10 min)\n \u2705 No SQL text is ever built from external input, so injection is impossible by construction\n \u2705 Malformed IDs return 200 with a correlated warning instead of 72h of retries\n \u2705 Reuses the existing lookup helper and its not-found guard; no duplicated query\n \u274c Needs the primary-key type confirmed and one small validation step with its tests\n2B) Bound parameter only, no shape validation (human ~1h / CC ~5 min)\n \u2705 Closes the injection vector with a single change to how the query is built\n \u2705 Reuses the existing lookup helper as-is\n \u274c A non-ID string still raises a type-cast DB error, which the wrapper turns into a 500 and Stripe retries for 72 hours; the log says \"DB failure\" not \"bad input\"\n2C) Keep the raw SQL fragment as written (human 0 / CC 0)\n \u2705 No work\n \u2705 Matches the sketch\n \u274c Live SQL injection on the payment path; the plan's own contract text says the input is unsanitized\nNet: reject bad input loudly and bind good input, versus trusting a string because Stripe signed the envelope. ", + "options": [ + { + "label": "2A) Validate + bound lookup (recommended)", + "description": "Completeness 10/10. Parse to PK type, 200+warn on failure, existing helper with bound param, injection + type tests." + }, + { + "label": "2B) Bound parameter only", + "description": "Completeness 6/10. Parameterize the query; malformed IDs still cause 500 and 72h retries." + }, + { + "label": "2C) Keep raw SQL fragment", + "description": "Completeness 0/10. Plan as written; injection remains open." + } + ], + "multiSelect": false + } + ] + }, + "caller": { + "type": "direct" + } + } + ] + } + } + }, + { + "line": 123, + "record": { + "type": "user", + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "cwd": "/tmp/gstack-paid-shard-zcm490/tmp/gstack-plan-count-q8859d", + "isSidechain": false, + "timestamp": "2026-09-10T03:18:21.399Z", + "message": { + "role": "user", + "content": [ + { + "type": "tool_result", + "content": "Your questions have been answered: \"D5 (Issue 2) \u2014 How should the handler look up the user from `request.params.userId`?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Database access\": userId read directly into a raw SQL fragment.\nELI10: `userId` is a string copied out of Stripe metadata. Our own server usually writes it, but the plan itself says nothing casts, escapes or validates it, and a valid Stripe signature only proves Stripe sent it, not that the string is safe. Pasting it into SQL means anyone who can set that metadata (a leaked API key, a dashboard edit, a bug in intent creation) can run their own SQL against the users table. Separately, a garbage string that fails the ID column's type cast raises a database error, which becomes a 500, which makes Stripe retry an unfixable event for three days.\nStakes if we pick wrong: data exfiltration or destruction through the payment path, plus 72 hours of retries and alerts for one malformed event.\nRecommendation: 2A because it closes injection completely and gives a malformed ID the same 200-and-warn treatment the adapter already gives a missing one. Maps to your preferences: security is not optional, explicit over clever, DRY (reuse the existing lookup helper).\nCompleteness: 2A=10/10, 2B=6/10, 2C=0/10\nPros / cons:\n2A) Validate then bind: parse userId to the users primary-key type (integer or UUID, whichever the schema uses; reject anything else including oversized or unicode strings). On parse failure log a warning with event ID and the rejected value length (not the value), ack 200 (retry cannot fix it). On success call the existing lookup helper, which uses a bound parameter; never build SQL text from the value. Tests: injection payload never reaches the DB and returns 200 + warning; wrong-type string returns 200 + warning; valid ID hits the existing lookup guard paths. (recommended) (human ~2h / CC ~10 min)\n \u2705 No SQL text is ever built from external input, so injection is impossible by construction\n \u2705 Malformed IDs return 200 with a correlated warning instead of 72h of retries\n \u2705 Reuses the existing lookup helper and its not-found guard; no duplicated query\n \u274c Needs the primary-key type confirmed and one small validation step with its tests\n2B) Bound parameter only, no shape validation (human ~1h / CC ~5 min)\n \u2705 Closes the injection vector with a single change to how the query is built\n \u2705 Reuses the existing lookup helper as-is\n \u274c A non-ID string still raises a type-cast DB error, which the wrapper turns into a 500 and Stripe retries for 72 hours; the log says \"DB failure\" not \"bad input\"\n2C) Keep the raw SQL fragment as written (human 0 / CC 0)\n \u2705 No work\n \u2705 Matches the sketch\n \u274c Live SQL injection on the payment path; the plan's own contract text says the input is unsanitized\nNet: reject bad input loudly and bind good input, versus trusting a string because Stripe signed the envelope. \"=\"2A) Validate + bound lookup (recommended)\". You can now continue with these answers in mind.", + "tool_use_id": "toolu_019pkzdAQt72RfCswrXXvvyc" + } + ] + } + } + } + ], + "calls": [ + { + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "toolUseId": "toolu_01M2hWu9BLPXb7e1wjfF23L5", + "questions": [ + { + "header": "Email leg", + "question": "D4 (Issue 1) \u2014 What should the handler do when the notification email fails after the payment update commits?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Webhook fan-out\": update + email inline, no error handling on the email leg.\nELI10: The database write says \"this user paid\" and commits. Then the email call throws. Today that exception turns into a 500, which tells Stripe \"try again\". But the payment is already saved, so the retry either gets swallowed by the dedup guard (no email ever goes out) or re-runs the handler and can send a second email. Stripe's failed-webhook alert fires for a payment that actually succeeded, which the runbook says must never trigger a blind replay.\nStakes if we pick wrong: paid users get zero or two receipts, on-call gets paged for successful payments, and a permanently rejected address retries for 72 hours.\nRecommendation: 1A because it is the only option where a committed payment returns 200 and the failure is still fully visible through the mail client's existing metric, alert and runbook. Maps to your preference: every error has a name, zero silent failures.\nCompleteness: 1A=9/10, 1B=4/10, 1C=2/10\nPros / cons:\n1A) Order: commit update -> record dedup completion -> send email. Rescue only the mail client's named classes (MailDeliveryError, MailTimeoutError, MailRejectedError). Log at error with event ID, user ID, payment intent ID, exception class. Return 200. If the existing notification retry procedure has a programmatic entry point, call it in the rescue; otherwise the logged correlation feeds the runbook. Tests: one per rescued class asserting 200, update persisted, error log emitted; one asserting DB errors still propagate as 500. (recommended) (human ~3h / CC ~15 min)\n \u2705 Committed payments never report failure to Stripe, so no false retries or alert noise\n \u2705 Named rescues only; DB failures keep their retry-via-500 contract untouched\n \u2705 Failure remains visible via the mail client's failure-rate metric, alert, and correlated trace\n \u274c Adds a rescue block and ordering rule the implementer must get right; needs the tests listed\n1B) Keep no rescue, but move the email after commit and dedup record so a retry cannot double-send (human ~30 min / CC ~5 min)\n \u2705 Tiny change; no new rescue code in the handler\n \u2705 Double email is prevented because the retry hits the dedup guard\n \u274c A mail failure still returns 500 and pages on-call for a successful payment; the retry acks 200 and the notification is silently never sent\n1C) Do nothing; keep the plan as written (human 0 / CC 0)\n \u2705 No implementation work\n \u2705 Behavior is at least consistent with the prior handler if it did the same\n \u274c Ordering is unspecified, so a mail outage can either roll back a real payment or send duplicate emails, and every mail failure looks like a webhook failure\nNet: a rescue with a name and a 200, versus letting Stripe retry a payment that already committed. ", + "options": [ + { + "label": "1A) Named rescue, 200, log+trace (recommended)", + "description": "Completeness 9/10. Commit -> dedup record -> email; rescue only mail classes; log correlated; return 200; tests per class." + }, + { + "label": "1B) Reorder only, no rescue", + "description": "Completeness 4/10. Email after commit + dedup record; failures still 500 and page on-call." + }, + { + "label": "1C) Do nothing", + "description": "Completeness 2/10. Keep inline email with no rescue and unspecified ordering." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 (Issue 1) \u2014 What should the handler do when the notification email fails after the payment update commits?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Webhook fan-out\": update + email inline, no error handling on the email leg.\nELI10: The database write says \"this user paid\" and commits. Then the email call throws. Today that exception turns into a 500, which tells Stripe \"try again\". But the payment is already saved, so the retry either gets swallowed by the dedup guard (no email ever goes out) or re-runs the handler and can send a second email. Stripe's failed-webhook alert fires for a payment that actually succeeded, which the runbook says must never trigger a blind replay.\nStakes if we pick wrong: paid users get zero or two receipts, on-call gets paged for successful payments, and a permanently rejected address retries for 72 hours.\nRecommendation: 1A because it is the only option where a committed payment returns 200 and the failure is still fully visible through the mail client's existing metric, alert and runbook. Maps to your preference: every error has a name, zero silent failures.\nCompleteness: 1A=9/10, 1B=4/10, 1C=2/10\nPros / cons:\n1A) Order: commit update -> record dedup completion -> send email. Rescue only the mail client's named classes (MailDeliveryError, MailTimeoutError, MailRejectedError). Log at error with event ID, user ID, payment intent ID, exception class. Return 200. If the existing notification retry procedure has a programmatic entry point, call it in the rescue; otherwise the logged correlation feeds the runbook. Tests: one per rescued class asserting 200, update persisted, error log emitted; one asserting DB errors still propagate as 500. (recommended) (human ~3h / CC ~15 min)\n \u2705 Committed payments never report failure to Stripe, so no false retries or alert noise\n \u2705 Named rescues only; DB failures keep their retry-via-500 contract untouched\n \u2705 Failure remains visible via the mail client's failure-rate metric, alert, and correlated trace\n \u274c Adds a rescue block and ordering rule the implementer must get right; needs the tests listed\n1B) Keep no rescue, but move the email after commit and dedup record so a retry cannot double-send (human ~30 min / CC ~5 min)\n \u2705 Tiny change; no new rescue code in the handler\n \u2705 Double email is prevented because the retry hits the dedup guard\n \u274c A mail failure still returns 500 and pages on-call for a successful payment; the retry acks 200 and the notification is silently never sent\n1C) Do nothing; keep the plan as written (human 0 / CC 0)\n \u2705 No implementation work\n \u2705 Behavior is at least consistent with the prior handler if it did the same\n \u274c Ordering is unspecified, so a mail outage can either roll back a real payment or send duplicate emails, and every mail failure looks like a webhook failure\nNet: a rescue with a name and a 200, versus letting Stripe retry a payment that already committed. ": "1A) Named rescue, 200, log+trace (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:17:27.593Z" + }, + { + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "toolUseId": "toolu_019pkzdAQt72RfCswrXXvvyc", + "questions": [ + { + "header": "SQL lookup", + "question": "D5 (Issue 2) \u2014 How should the handler look up the user from `request.params.userId`?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Database access\": userId read directly into a raw SQL fragment.\nELI10: `userId` is a string copied out of Stripe metadata. Our own server usually writes it, but the plan itself says nothing casts, escapes or validates it, and a valid Stripe signature only proves Stripe sent it, not that the string is safe. Pasting it into SQL means anyone who can set that metadata (a leaked API key, a dashboard edit, a bug in intent creation) can run their own SQL against the users table. Separately, a garbage string that fails the ID column's type cast raises a database error, which becomes a 500, which makes Stripe retry an unfixable event for three days.\nStakes if we pick wrong: data exfiltration or destruction through the payment path, plus 72 hours of retries and alerts for one malformed event.\nRecommendation: 2A because it closes injection completely and gives a malformed ID the same 200-and-warn treatment the adapter already gives a missing one. Maps to your preferences: security is not optional, explicit over clever, DRY (reuse the existing lookup helper).\nCompleteness: 2A=10/10, 2B=6/10, 2C=0/10\nPros / cons:\n2A) Validate then bind: parse userId to the users primary-key type (integer or UUID, whichever the schema uses; reject anything else including oversized or unicode strings). On parse failure log a warning with event ID and the rejected value length (not the value), ack 200 (retry cannot fix it). On success call the existing lookup helper, which uses a bound parameter; never build SQL text from the value. Tests: injection payload never reaches the DB and returns 200 + warning; wrong-type string returns 200 + warning; valid ID hits the existing lookup guard paths. (recommended) (human ~2h / CC ~10 min)\n \u2705 No SQL text is ever built from external input, so injection is impossible by construction\n \u2705 Malformed IDs return 200 with a correlated warning instead of 72h of retries\n \u2705 Reuses the existing lookup helper and its not-found guard; no duplicated query\n \u274c Needs the primary-key type confirmed and one small validation step with its tests\n2B) Bound parameter only, no shape validation (human ~1h / CC ~5 min)\n \u2705 Closes the injection vector with a single change to how the query is built\n \u2705 Reuses the existing lookup helper as-is\n \u274c A non-ID string still raises a type-cast DB error, which the wrapper turns into a 500 and Stripe retries for 72 hours; the log says \"DB failure\" not \"bad input\"\n2C) Keep the raw SQL fragment as written (human 0 / CC 0)\n \u2705 No work\n \u2705 Matches the sketch\n \u274c Live SQL injection on the payment path; the plan's own contract text says the input is unsanitized\nNet: reject bad input loudly and bind good input, versus trusting a string because Stripe signed the envelope. ", + "options": [ + { + "label": "2A) Validate + bound lookup (recommended)", + "description": "Completeness 10/10. Parse to PK type, 200+warn on failure, existing helper with bound param, injection + type tests." + }, + { + "label": "2B) Bound parameter only", + "description": "Completeness 6/10. Parameterize the query; malformed IDs still cause 500 and 72h retries." + }, + { + "label": "2C) Keep raw SQL fragment", + "description": "Completeness 0/10. Plan as written; injection remains open." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 (Issue 2) \u2014 How should the handler look up the user from `request.params.userId`?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md \"Database access\": userId read directly into a raw SQL fragment.\nELI10: `userId` is a string copied out of Stripe metadata. Our own server usually writes it, but the plan itself says nothing casts, escapes or validates it, and a valid Stripe signature only proves Stripe sent it, not that the string is safe. Pasting it into SQL means anyone who can set that metadata (a leaked API key, a dashboard edit, a bug in intent creation) can run their own SQL against the users table. Separately, a garbage string that fails the ID column's type cast raises a database error, which becomes a 500, which makes Stripe retry an unfixable event for three days.\nStakes if we pick wrong: data exfiltration or destruction through the payment path, plus 72 hours of retries and alerts for one malformed event.\nRecommendation: 2A because it closes injection completely and gives a malformed ID the same 200-and-warn treatment the adapter already gives a missing one. Maps to your preferences: security is not optional, explicit over clever, DRY (reuse the existing lookup helper).\nCompleteness: 2A=10/10, 2B=6/10, 2C=0/10\nPros / cons:\n2A) Validate then bind: parse userId to the users primary-key type (integer or UUID, whichever the schema uses; reject anything else including oversized or unicode strings). On parse failure log a warning with event ID and the rejected value length (not the value), ack 200 (retry cannot fix it). On success call the existing lookup helper, which uses a bound parameter; never build SQL text from the value. Tests: injection payload never reaches the DB and returns 200 + warning; wrong-type string returns 200 + warning; valid ID hits the existing lookup guard paths. (recommended) (human ~2h / CC ~10 min)\n \u2705 No SQL text is ever built from external input, so injection is impossible by construction\n \u2705 Malformed IDs return 200 with a correlated warning instead of 72h of retries\n \u2705 Reuses the existing lookup helper and its not-found guard; no duplicated query\n \u274c Needs the primary-key type confirmed and one small validation step with its tests\n2B) Bound parameter only, no shape validation (human ~1h / CC ~5 min)\n \u2705 Closes the injection vector with a single change to how the query is built\n \u2705 Reuses the existing lookup helper as-is\n \u274c A non-ID string still raises a type-cast DB error, which the wrapper turns into a 500 and Stripe retries for 72 hours; the log says \"DB failure\" not \"bad input\"\n2C) Keep the raw SQL fragment as written (human 0 / CC 0)\n \u2705 No work\n \u2705 Matches the sketch\n \u274c Live SQL injection on the payment path; the plan's own contract text says the input is unsanitized\nNet: reject bad input loudly and bind good input, versus trusting a string because Stripe signed the envelope. ": "2A) Validate + bound lookup (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:18:21.399Z" + } + ], + "setupCalls": [ + { + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "toolUseId": "toolu_01EY1KHHBh5ps97s9uSQP4h2", + "questions": [ + { + "header": "Routing", + "question": "D1 \u2014 Add gstack skill routing rules to this project's CLAUDE.md?\nProject/branch/task: gstack-plan-count-q8859d on main, reviewing PLAN.md (Stripe payment webhook handler).\nELI10: gstack works best when CLAUDE.md tells the agent which skill to reach for (bugs \u2192 /investigate, scope \u2192 /plan-ceo-review, ship \u2192 /ship). Without it you invoke skills by hand every time. This is a one-time prompt per project.\nStakes if we pick wrong: nothing breaks either way; you just lose automatic skill routing or gain a CLAUDE.md section you did not want.\nRecommendation: A because routing rules are the documented default and cost one small append. Note: plan mode is active, so the CLAUDE.md edit and commit are deferred until this review exits plan mode.\nNote: options differ in kind, not coverage \u2014 no completeness score.\nPros / cons:\nA) Add routing rules (recommended)\n \u2705 Future sessions pick the right gstack skill without you naming it each time\n \u2705 One appended section, committed separately, easy to revert if unwanted\n \u274c Adds ~20 lines to CLAUDE.md that teammates will also see and may not want\nB) No thanks, invoke skills manually\n \u2705 CLAUDE.md stays exactly as the fixture seeded it, no repo change from setup\n \u2705 Re-enable later with one gstack-config command if you change your mind\n \u274c Every future skill run needs an explicit slash command from you\nNet: convenience later versus zero repo churn now.", + "options": [ + { + "label": "A) Add routing rules (recommended)", + "description": "Append the routing section to CLAUDE.md and commit it after plan mode exits." + }, + { + "label": "B) No thanks, manual", + "description": "Set routing_declined=true; re-enable any time via gstack-config." + } + ], + "multiSelect": false + }, + { + "header": "Learnings", + "question": "D2 \u2014 Let gstack search learnings from your other projects on this machine?\nProject/branch/task: gstack-plan-count-q8859d on main, CEO plan review.\nELI10: gstack records small lessons per project. With cross-project search on, a lesson learned in another repo on this machine can inform this review. Nothing leaves the machine. Recommended for solo developers; skip if you work across client codebases where mixing context is a concern.\nStakes if we pick wrong: on by mistake means learnings from unrelated client work could surface here; off by mistake means you miss patterns you already learned elsewhere.\nRecommendation: A because this is a local-only lookup and the recorded default for solo work.\nNote: options differ in kind, not coverage \u2014 no completeness score.\nPros / cons:\nA) Enable cross-project learnings (recommended)\n \u2705 Past pitfalls from any project on this machine can surface as prior-learning notes\n \u2705 Stays local; a single config flag you can flip back off at any time\n \u274c Learnings from unrelated codebases may show up where they do not apply\nB) Keep learnings project-scoped\n \u2705 No cross-contamination between client or team codebases on this machine\n \u2705 Simplest privacy posture for shared or multi-client machines\n \u274c Each project starts cold and relearns the same pitfalls independently\nNet: shared memory across repos versus strict per-repo isolation.", + "options": [ + { + "label": "A) Enable cross-project (recommended)", + "description": "gstack-config set cross_project_learnings true (local only)." + }, + { + "label": "B) Project-scoped only", + "description": "gstack-config set cross_project_learnings false." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Add gstack skill routing rules to this project's CLAUDE.md?\nProject/branch/task: gstack-plan-count-q8859d on main, reviewing PLAN.md (Stripe payment webhook handler).\nELI10: gstack works best when CLAUDE.md tells the agent which skill to reach for (bugs \u2192 /investigate, scope \u2192 /plan-ceo-review, ship \u2192 /ship). Without it you invoke skills by hand every time. This is a one-time prompt per project.\nStakes if we pick wrong: nothing breaks either way; you just lose automatic skill routing or gain a CLAUDE.md section you did not want.\nRecommendation: A because routing rules are the documented default and cost one small append. Note: plan mode is active, so the CLAUDE.md edit and commit are deferred until this review exits plan mode.\nNote: options differ in kind, not coverage \u2014 no completeness score.\nPros / cons:\nA) Add routing rules (recommended)\n \u2705 Future sessions pick the right gstack skill without you naming it each time\n \u2705 One appended section, committed separately, easy to revert if unwanted\n \u274c Adds ~20 lines to CLAUDE.md that teammates will also see and may not want\nB) No thanks, invoke skills manually\n \u2705 CLAUDE.md stays exactly as the fixture seeded it, no repo change from setup\n \u2705 Re-enable later with one gstack-config command if you change your mind\n \u274c Every future skill run needs an explicit slash command from you\nNet: convenience later versus zero repo churn now.": "A) Add routing rules (recommended)", + "D2 \u2014 Let gstack search learnings from your other projects on this machine?\nProject/branch/task: gstack-plan-count-q8859d on main, CEO plan review.\nELI10: gstack records small lessons per project. With cross-project search on, a lesson learned in another repo on this machine can inform this review. Nothing leaves the machine. Recommended for solo developers; skip if you work across client codebases where mixing context is a concern.\nStakes if we pick wrong: on by mistake means learnings from unrelated client work could surface here; off by mistake means you miss patterns you already learned elsewhere.\nRecommendation: A because this is a local-only lookup and the recorded default for solo work.\nNote: options differ in kind, not coverage \u2014 no completeness score.\nPros / cons:\nA) Enable cross-project learnings (recommended)\n \u2705 Past pitfalls from any project on this machine can surface as prior-learning notes\n \u2705 Stays local; a single config flag you can flip back off at any time\n \u274c Learnings from unrelated codebases may show up where they do not apply\nB) Keep learnings project-scoped\n \u2705 No cross-contamination between client or team codebases on this machine\n \u2705 Simplest privacy posture for shared or multi-client machines\n \u274c Each project starts cold and relearns the same pitfalls independently\nNet: shared memory across repos versus strict per-repo isolation.": "A) Enable cross-project (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:14:18.643Z" + }, + { + "sessionId": "d87d468e-9058-4a52-8dc8-f7d4246dc13a", + "toolUseId": "toolu_01TopSkSasMW3J5dU3bKLTWz", + "questions": [ + { + "header": "Approach", + "question": "D3 \u2014 Which implementation structure should the plan adopt for the Stripe payment handler?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md Payment Processing Integration, HOLD SCOPE review.\nELI10: The plan says the new handler skips the existing WebhookDispatcher for a \"clean namespace\", yet also promises the existing dedup, per-user lock and unknown-user guards still protect it. Those guards sit on the dispatcher path. You can register the handler with the dispatcher (guards apply automatically), keep the bypass and re-wire each guard by hand, or ship the bypass as sketched.\nStakes if we pick wrong: a paid user could be processed twice, or reach an unguarded path where a Stripe retry re-runs the update and email with no dedup.\nRecommendation: B because the plan's retained contracts only hold if the handler sits behind the dispatcher, and it is a one-line registration.\nCompleteness: A=3/10, B=9/10, C=6/10\nPros / cons:\nA) Standalone handler, bypass dispatcher (plan as written) (human ~1 day / CC ~20 min)\n \u2705 Smallest diff and no edits to the shared dispatcher module at all\n \u2705 Handler file stands alone, easy to read in isolation\n \u274c Dedup, lock and lookup guards are claimed but nothing on this path applies them\nB) Register handler inside WebhookDispatcher (recommended) (human ~1.5 days / CC ~30 min)\n \u2705 Dedup, per-user lock, unknown-user ack and tracing apply by construction\n \u2705 One dispatch path; namespace separation comes from module layout, not a bypass\n \u274c Requires one registration change in the dispatcher and giving up the literal bypass\nC) Standalone handler with guards re-wired as explicit middleware (human ~2 days / CC ~45 min)\n \u2705 Keeps the bypass the plan asked for while making each guard visible\n \u2705 No change to the dispatcher module\n \u274c Two dispatch paths forever; every guard change must land twice or drifts silently\nNet: one shared pipeline versus a second pipeline you maintain by hand. ", + "options": [ + { + "label": "B) Register in dispatcher (recommended)", + "description": "Completeness 9/10. Same class, registered for payment_intent.succeeded; reuse existing lookup helper." + }, + { + "label": "A) Standalone bypass (as written)", + "description": "Completeness 3/10. Keep the plan's structure; guards must be re-verified for this path." + }, + { + "label": "C) Standalone + explicit guards", + "description": "Completeness 6/10. Compose dedup, lock and lookup guards around the handler by hand." + } + ], + "multiSelect": false + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Which implementation structure should the plan adopt for the Stripe payment handler?\nProject/branch/task: gstack-plan-count-q8859d on main, PLAN.md Payment Processing Integration, HOLD SCOPE review.\nELI10: The plan says the new handler skips the existing WebhookDispatcher for a \"clean namespace\", yet also promises the existing dedup, per-user lock and unknown-user guards still protect it. Those guards sit on the dispatcher path. You can register the handler with the dispatcher (guards apply automatically), keep the bypass and re-wire each guard by hand, or ship the bypass as sketched.\nStakes if we pick wrong: a paid user could be processed twice, or reach an unguarded path where a Stripe retry re-runs the update and email with no dedup.\nRecommendation: B because the plan's retained contracts only hold if the handler sits behind the dispatcher, and it is a one-line registration.\nCompleteness: A=3/10, B=9/10, C=6/10\nPros / cons:\nA) Standalone handler, bypass dispatcher (plan as written) (human ~1 day / CC ~20 min)\n \u2705 Smallest diff and no edits to the shared dispatcher module at all\n \u2705 Handler file stands alone, easy to read in isolation\n \u274c Dedup, lock and lookup guards are claimed but nothing on this path applies them\nB) Register handler inside WebhookDispatcher (recommended) (human ~1.5 days / CC ~30 min)\n \u2705 Dedup, per-user lock, unknown-user ack and tracing apply by construction\n \u2705 One dispatch path; namespace separation comes from module layout, not a bypass\n \u274c Requires one registration change in the dispatcher and giving up the literal bypass\nC) Standalone handler with guards re-wired as explicit middleware (human ~2 days / CC ~45 min)\n \u2705 Keeps the bypass the plan asked for while making each guard visible\n \u2705 No change to the dispatcher module\n \u274c Two dispatch paths forever; every guard change must land twice or drifts silently\nNet: one shared pipeline versus a second pipeline you maintain by hand. ": "B) Register in dispatcher (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:15:39.519Z" + } + ], + "observedFirstReviewClassifications": [ + false, + false + ], + "historicalOutcome": "no_review_questions", + "scope": "First completed distinct attempt; whole paid job and retry were still active when fixture authored." +} diff --git a/test/fixtures/eng-next-handoff-ah.json b/test/fixtures/eng-next-handoff-ah.json new file mode 100644 index 000000000..2a6c3b9b6 --- /dev/null +++ b/test/fixtures/eng-next-handoff-ah.json @@ -0,0 +1,543 @@ +{ + "schemaVersion": 1, + "source": { + "observationSha256": "49e9b1d794889b356cd7c67974acd89c9a84e20bde35fdab13881f35abafb8d1", + "planSha256": "d333e40fa1925ac6e07a78ff64757393cd5aa66fddc394b31eb3a26434bb6e06", + "screenSha256": "fe599c919bd7d0469dba8985f43b34941dacba94be3d50a88d9364e2d61c4309", + "capture": { + "skill": "plan-eng-review", + "runId": "ship-source-ah-delta-paid-20260910-v1-8", + "cwd": "/tmp/gstack-paid-shard-oLsoWY/tmp/gstack-plan-count-b3qdhZ", + "claudeConfigDir": "/tmp/gstack-paid-shard-oLsoWY/tmp/gstack-hermetic-340461-9Ex6MJ/with-skills/.claude", + "at": "2026-09-10T03:36:31.588Z" + }, + "stat": { + "path": "/tmp/gstack-paid-shard-oLsoWY/tmp/gstack-e2e-plan-eng-K1VV4m/gstack-test-plan-eng.md", + "sha256": "d333e40fa1925ac6e07a78ff64757393cd5aa66fddc394b31eb3a26434bb6e06", + "mtimeMs": 1789010935798.159, + "mtimeUTC": "2026-09-10T03:28:55.798159+00:00", + "size": 23521 + }, + "limitations": "Exact retained public observation and saved plan, not private native blocks. Administrative projection is prospective only." + }, + "fingerprint": { + "signature": "e5829792-573f-4de0-8cbc-97379840c953:toolu_01Je2eR9CH4vWsDYmEdCnsAP", + "promptSnippet": "Next D14 — Next step after this eng review? Project/branch/task: gstack-plan-count-b3qdhZ on main, reviewed plan written to gstack-test-plan-eng.md. ELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touch", + "options": [ + { + "index": 1, + "label": "A) Ready to implement (recommended)" + }, + { + "index": 2, + "label": "B) Run /plan-ceo-review" + } + ], + "observedAtMs": 685701, + "preReview": false, + "nativeCall": { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01Je2eR9CH4vWsDYmEdCnsAP", + "questions": [ + { + "question": "D14 — Next step after this eng review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewed plan written to gstack-test-plan-eng.md.\nELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touched, so a design review does not apply. A CEO review is optional and normally for product-direction changes; this is a backend auth refactor, so it is a soft mention only.\nStakes if we pick wrong: Low either way; the CEO review would cost time on a change with no product-facing scope decision left open.\nRecommendation: A because all required reviews are complete and the plan's remaining blocker is the author confirming the Context section, not another review.\nNote: options differ in kind, not coverage — no completeness score.\nNet: start implementing versus an optional strategy pass on a backend refactor.", + "header": "Next", + "multiSelect": false, + "options": [ + { + "label": "A) Ready to implement (recommended)", + "description": "✅ All relevant reviews complete; run /ship when the work is done. ✅ The first task is the author confirming Context, then T1 characterization tests. ❌ No second strategic opinion on whether the refactor is the right thing to build now." + }, + { + "label": "B) Run /plan-ceo-review", + "description": "✅ Adds a scope-and-strategy pass before any code is written. ✅ Useful if the refactor's business motivation is contested. ❌ Backend-only refactor with no product-direction choice; likely low yield for the time." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D14 — Next step after this eng review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewed plan written to gstack-test-plan-eng.md.\nELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touched, so a design review does not apply. A CEO review is optional and normally for product-direction changes; this is a backend auth refactor, so it is a soft mention only.\nStakes if we pick wrong: Low either way; the CEO review would cost time on a change with no product-facing scope decision left open.\nRecommendation: A because all required reviews are complete and the plan's remaining blocker is the author confirming the Context section, not another review.\nNote: options differ in kind, not coverage — no completeness score.\nNet: start implementing versus an optional strategy pass on a backend refactor.": "A) Ready to implement (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:29:41.500Z" + } + }, + "plan": "# Plan: Multi-tenant Auth Refactor (reviewed)\n\nReviewed by /plan-eng-review on 2026-09-10 against PLAN.md at commit 7007835.\nScope was reduced (D4). Six findings, all folded into this plan with the complete remedy.\n\n## Context\n\n**Problem (inferred, author must confirm — see Prerequisite P0 below).** The current auth path\nvalidates tokens with 5 sequential IDP calls, swallows three classes of error inside\n`validateAndDispatch()`, and mixes tenant-scoped session logic into a single `legacyAuthFlow()`.\nThe refactor introduces two focused services, `AuthBroker` (mints and stores tokens) and\n`SessionMint` (establishes sessions from cached or freshly minted tokens), on top of the\nexisting tenant-keyed cache adapter.\n\n**Goal (draft).** Every tenant's login and token validation runs through one explicit,\nobservable path with bounded latency and no silent failures, while the two invariants below\nhold at all times.\n\n**Invariants (draft, must be asserted by tests).**\n1. No cross-tenant reads: a token stored under tenant A's key is never returned for tenant B.\n2. No resurrection: once a token is invalidated (logout, revocation, tenant suspension, policy\n version bump), no in-flight write can put it back.\n\n**Latency target (draft).** Token validation p95 bounded by the slowest single IDP call plus\ncache lookup, not by the sum of five calls. Author to fill in the number.\n\n### Prerequisite P0 (decision D7 → 3A)\nImplementation does not start until the author confirms or edits the Problem, Goal,\nInvariants and Latency target above. Estimated: human ~30 min / CC ~5 min.\n\n## Existing contracts retained (unchanged from PLAN.md)\n\nThe existing cache adapter keys entries by tenant ID, issuer, audience, and policy version.\nIt evicts expired tokens and invalidates entries on logout, token revocation, or tenant\nsuspension. It does not serialize mutations. The adapter, its invalidation hooks, and their\nexisting tests remain in use unchanged. The new services receive the adapter by constructor\ninjection; nothing new wraps it.\n\n## Architecture (as reviewed)\n\n### Scope (decision D4 → A)\nPLAN.md proposed 12 files and five new types (`AuthBroker`, `SessionMint`, `TokenStore`,\n`AuthCache`, `RequestPolicy`). Reduced to **two new types**:\n\n| Proposed type | Outcome | Reason |\n|---|---|---|\n| `AuthBroker` | **Keep** | The only writer to the cache; mints tokens via the IDP. |\n| `SessionMint` | **Keep** | Reads the cache; asks `AuthBroker` to mint on miss. |\n| `AuthCache` | **Drop** | PLAN.md:11-13 describes a pass-through facade over the existing adapter with one backing cache. The adapter is injected directly instead. |\n| `TokenStore` | **Drop** | Overlaps the adapter's token storage role. |\n| `RequestPolicy` | **Defer** | No consumer named anywhere in the plan. Captured as TODO 1. |\n\nEstimated diff: ~7 files.\n\n### Component and data flow\n\n```\n per-tenant flag (D6)\n request ──────────┬─────────────────────────────┐\n │ flag OFF │ flag ON\n ▼ ▼\n legacyAuthFlow() validateAndDispatch() (typed pipeline, D8)\n (unchanged, kept ├─ parse token → Result\n until TODO 2) ├─ verify signature → Result (JWKS cache, D10)\n │ ├─ check tenant policy→ Result\n │ └─ dispatch → Result\n │ │\n │ ▼\n │ ┌──── error boundary ────┐\n │ │ maps each error class │\n │ │ → outcome + log + metric│\n │ └────────────────────────┘\n │ │\n ▼ ▼\n existing adapter ◄──read── SessionMint ──mint?──► AuthBroker ──write(gen)──► existing adapter\n (tenant-keyed, │ (same instance,\n invalidation hooks) ▼ injected)\n IDP calls (parallel, shared AbortSignal,\n per-call timeout; discovery + JWKS cached)\n```\n\n### Issue 1 — Shared cache mutated by two services with no ordering (decision D5 → 1A)\n`[P1] (confidence: 8/10) PLAN.md:19-20, PLAN.md:10` — \"Both services mutate it\" and \"they do\nnot serialize mutations.\" Failure scenario: `SessionMint` begins minting for tenant A; tenant A\nis suspended and the existing hook invalidates A's entries; the in-flight write lands and\nrestores a valid token. Suspended tenant keeps access silently until expiry.\n\n**Remedy (approved):**\n- `AuthBroker` is the **single writer**. `SessionMint` only reads and calls\n `AuthBroker.mint()` on a miss.\n- Every write carries the **invalidation generation** it observed at read time. The adapter\n wrapper call in `AuthBroker` compares the generation; a stale write is dropped and logged\n with tenant ID and reason.\n- The generation counter is bumped by the existing invalidation hooks (logout, revocation,\n suspension, policy bump). This is one small guard where all writes route through, not a\n guard in every caller.\n- Test: deterministic interleaving test (read → invalidate → write) proves the write is dropped.\n\n### Issue 2 — In-place rewrite of legacyAuthFlow() with no rollback (decision D6 → 2A)\n`[P1] (confidence: 9/10) PLAN.md:27-28` — \"legacyAuthFlow() will get rewritten.\"\n\n**Remedy (approved):** strangler-fig rollout.\n- Keep `legacyAuthFlow()` callable. A **per-tenant flag** routes each tenant to the legacy or\n the `AuthBroker` path.\n- Add a counter of requests served by the legacy path (feeds TODO 2's deletion trigger).\n- Migrate tenants in waves. Rollback for one tenant is a flag flip, not a revert.\n- Deletion of the legacy path and the flag is TODO 2, a separate follow-up.\n\n### Issue 3 — No stated goal or invariants (decision D7 → 3A)\n`[P1] (confidence: 9/10) PLAN.md:4-36` — every section describes a smell; none states what\ndone means. Remedy: the Context section above, confirmed by the author before implementation\n(Prerequisite P0).\n\n### Inline ASCII diagrams to add in code\n- `AuthBroker`: the read-generation → mint → write-if-current sequence, with the invalidation\n hook bumping the generation drawn alongside.\n- `validateAndDispatch()`: the four-step pipeline and the error-boundary mapping table.\n- The flag router: legacy vs new path decision.\n- Interleaving test file: a timeline comment showing which step of which actor runs when.\nDiagram maintenance is part of every later change to these files.\n\n## Code quality (as reviewed)\n\n### Issue 4 — validateAndDispatch() swallows three error classes (decision D8 → 4A)\n`[P1] (confidence: 9/10) PLAN.md:23-24` — \"three nested try/catch blocks; each catch swallows\na different error class.\"\n\n**Remedy (approved):**\n- Flatten into a straight-line pipeline of small steps (parse, verify signature, check tenant\n policy, dispatch). Each step returns a typed `Result` (reuse ladder: check the repo for an\n existing Result/Either helper first, then an installed dependency, before adding one).\n- **One error boundary** maps each error class to a distinct outcome, log line and metric.\n Unknown errors fail closed (reject), never pass.\n- Each step gets its own unit test; the boundary gets one table-driven test per error class.\n- DRY: cache write logic exists in exactly one place (`AuthBroker`, Issue 1). `SessionMint`\n never duplicates it.\n\n## Tests (as reviewed)\n\n### CRITICAL — Regression: legacyAuthFlow() characterization tests (mandatory, no decision needed)\nPLAN.md:27-28 rewrites existing behavior; PLAN.md:15-16 explicitly excludes it from coverage.\nUnder the regression rule this is a critical requirement:\n- **Before** any rewrite, write characterization tests for `legacyAuthFlow()` covering: valid\n token, expired token, revoked token, wrong-tenant token, IDP unavailable, logout then replay,\n tenant suspended then request.\n- Run the same fixtures against the `AuthBroker` path under the flag (D6) and assert parity\n where behavior must match, and assert the intended difference where it must not.\n- These tests stay until TODO 2 deletes the legacy path, then become new-path-only tests.\n\n### Coverage map (decision D9 → 5A: full map)\n\n```\nCODE PATHS USER FLOWS\n[~] legacyAuthFlow() (kept behind flag) [+] Tenant login via new path\n └── [CRITICAL] characterization: valid / expired ├── [→E2E] login → mint → validated request\n / revoked / wrong-tenant / IDP down / logout replay ├── [→E2E] tenant suspended mid-session → rejected\n[+] AuthBroker (single writer, versioned write) └── flag OFF → legacy path, parity\n ├── write with current generation → stored [+] Error states (user-visible)\n ├── write with stale generation → dropped + logged ├── IDP timeout → explicit, retryable error\n ├── mint failure (IDP error) → typed error, no write ├── malformed token → explicit reject\n └── tenant A key never readable via tenant B key └── policy miss → explicit reject, never pass\n[+] SessionMint (reads, requests mint)\n ├── cache hit → no IDP call\n ├── cache miss → broker mint → stored\n ├── invalidation during mint → no resurrection (interleaving test)\n └── two concurrent mints, same tenant/user → one entry, no corruption\n[+] validateAndDispatch() pipeline\n ├── each step happy path\n ├── each error class → distinct outcome + log (table test)\n └── unknown error → fail closed\n[+] IDP validation (parallel)\n ├── all calls succeed\n ├── one rejects → others cancelled (assert abort), typed error\n ├── one hangs → per-call timeout fires\n ├── discovery/JWKS served from cache within TTL\n └── unknown kid → JWKS refresh → success (key rotation)\n[+] Flag router\n ├── flag ON → new path\n └── flag OFF → legacy path; legacy counter increments\n\nCOVERAGE TODAY: 0/23 paths tested (0%) | GAPS: 23 (1 CRITICAL regression, 2 E2E)\nTARGET AT MERGE: 23/23\n```\nLegend: [→E2E] needs integration test. No LLM paths, no evals.\n\n### Test requirements (write alongside the code, not after)\n| # | Test | Kind | Asserts |\n|---|---|---|---|\n| 1 | `legacyAuthFlow` characterization | unit + integration | Existing behavior for 7 fixtures listed above (CRITICAL) |\n| 2 | `AuthBroker` write current generation | unit | Entry stored under full tenant key |\n| 3 | `AuthBroker` write stale generation | unit | Write dropped; log emitted with tenant and reason |\n| 4 | `AuthBroker` mint failure | unit | Typed error returned; adapter untouched |\n| 5 | Cross-tenant isolation | unit | Store under tenant A; read as tenant B returns miss |\n| 6 | `SessionMint` hit / miss | unit | Hit makes zero IDP calls; miss calls broker once |\n| 7 | Invalidation during mint | unit (controlled async) | read → invalidate → write; final state is empty |\n| 8 | Concurrent mints | unit | One stored entry; both callers get a valid result or a clean error |\n| 9 | Pipeline steps | unit | Each step's Result on valid and invalid input |\n| 10 | Error boundary | table-driven unit | Each error class → its outcome, log, metric; unknown → reject |\n| 11 | IDP all succeed | unit (stubbed IDP) | Result assembled; call count = expected |\n| 12 | IDP one rejects | unit | Other calls' AbortSignal aborted; typed error |\n| 13 | IDP one hangs | unit (fake timers) | Timeout error within the bound |\n| 14 | Discovery/JWKS cache | unit | Second validation makes no discovery/JWKS call |\n| 15 | Unknown kid rotation | unit | Refresh once, then verify succeeds |\n| 16 | Flag routing | unit | ON → new path; OFF → legacy path + counter |\n| 17 | Login → validated request | E2E | Full flow succeeds; second request is a cache hit |\n| 18 | Suspend mid-session | E2E | Next request rejected; no token resurrection |\n\nTest framework: none detected in this fixture repo (no `package.json`, zero test files). Match\nthe real repo's existing convention (`*.test.ts` or `*.spec.ts`) when implementing.\n\n## Performance (as reviewed)\n\n### Issue 6 — Bare Promise.all over 5 IDP calls (decision D10 → 6A)\n`[P2] (confidence: 7/10) PLAN.md:31-32` — \"parallelized via Promise.all trivially.\"\nPromise.all is fail-fast but does not cancel: the first rejection leaves four calls running\nagainst an already-degraded IDP. 1→5 concurrent calls per request multiplies burst against\nrate-limited per-tenant IDPs.\n\n**Remedy (approved):**\n- Run the independent calls with `Promise.all`, all sharing one `AbortController` signal;\n the first rejection aborts the rest.\n- Each call gets `AbortSignal.timeout(...)` combined with the shared signal, so a hung IDP\n bounds the tail.\n- **Cache the discovery document (TTL hours) and JWKS (TTL minutes)** per issuer; on an\n unknown `kid`, refresh once and retry the verification. Hot path drops to 3 or fewer network\n calls. **[Layer 1]** — standard OIDC practice, no new dependency needed.\n- Memory: the discovery/JWKS cache is bounded by issuer count, not request count.\n\nNo N+1 or database access patterns in scope.\n\n## What already exists\n| Existing piece | Plan's use | Verdict |\n|---|---|---|\n| Tenant-keyed cache adapter with eviction and invalidation hooks + tests | Reused unchanged, injected | Correct reuse |\n| `legacyAuthFlow()` | Was to be rewritten in place | Now kept behind a flag with characterization tests until TODO 2 |\n| Module cache singleton semantics | Was relied on via module-level export | Replaced by constructor injection; the global bought nothing |\n| `AuthCache` facade (proposed) | Wrapped the adapter | Dropped as duplicate |\n\n## NOT in scope\n- `RequestPolicy` — no consumer named; deferred to TODO 1.\n- `TokenStore` and `AuthCache` — dropped; the adapter already does this job.\n- Changing the adapter's key schema or serializing mutations inside the adapter — the\n generation check in `AuthBroker` closes the race without touching the adapter contract.\n- Deleting `legacyAuthFlow()` and the flag — TODO 2, after all tenants migrate.\n- Module-wide audit of swallow-and-continue catch blocks — TODO 3, separate diff.\n- IDP-side changes, multi-region cache, new artifacts or distribution — none introduced.\n\n## Failure modes\n| New codepath | Realistic failure | Test | Handling | User sees |\n|---|---|---|---|---|\n| `AuthBroker` write | Stale write after invalidation | #7 | Generation check drops + logs | Rejected on next request, log for on-call |\n| `AuthBroker` mint | IDP 5xx | #4, #12 | Typed error via boundary | Clear retryable error |\n| `SessionMint` read | Wrong tenant key | #5 | Adapter key includes tenant ID | Miss, then mint for own tenant |\n| Pipeline boundary | Unmapped error class | #10 | Fail closed | Explicit reject |\n| IDP parallel | One call hangs | #13 | Per-call timeout | Timeout error, no spinner |\n| JWKS cache | Key rotation | #15 | Refresh on unknown kid | Transparent |\n| Flag router | Flag store unavailable | #16 (add case) | Default to legacy path, log | No change in behavior |\n| Legacy path | Behavior drift during refactor | #1 (CRITICAL) | Parity assertions | None if tests hold |\n\n**Critical gaps: 0.** The stale-write resurrection race would have been a critical gap (no\ntest, no handling, silent) under the original plan; decision D5 closes it with test #7.\nFlag-store unavailability is added to test #16 as a case.\n\n## Worktree parallelization strategy\n\n| Step | Modules touched | Depends on |\n|---|---|---|\n| S0 Author confirms Context (P0) | PLAN.md | — |\n| S1 Characterization tests for legacy path | auth/__tests__ | S0 |\n| S2 `AuthBroker` + `SessionMint` + generation check | auth/ (broker, mint) | S0 |\n| S3 `validateAndDispatch()` pipeline + boundary | auth/ (validate) | S0 |\n| S4 IDP parallel calls + discovery/JWKS cache | auth/idp | S0 |\n| S5 Flag router + legacy counter | auth/ (routing), config/ | S1, S2 |\n| S6 Interleaving, isolation, concurrency tests | auth/__tests__ | S2 |\n| S7 E2E flows | e2e/ | S2, S3, S4, S5 |\n\nLanes:\n- Lane A: S1 (independent, test-only, must land before any rewrite)\n- Lane B: S2 → S6 (sequential, shared broker/mint module)\n- Lane C: S3 (independent)\n- Lane D: S4 (independent)\n- Lane E: S5 → S7 (after A, B, C, D merge)\n\nExecution: after S0, launch A + B + C + D in parallel worktrees. Merge all four. Then E.\nConflict flag: Lanes B and C both live under `auth/`; keep them in separate files (broker/mint\nvs validate) and expect a small import-level merge in the module index.\n\n## TODOs (approved; write to TODOS.md when plan mode exits)\n\n### RequestPolicy seam (D11)\n**What:** Introduce a `RequestPolicy` type once a concrete consumer needs per-request policy\ndecisions beyond the adapter's policy-version key.\n**Why:** Dropped from scope because no plan section names a caller; keeps the author's intent.\n**Context:** PLAN.md listed it with no description. After the refactor lands, grep call sites\nthat branch on tenant policy; 2+ sites is the consumer.\n**Effort:** M **Priority:** P3 **Depends on:** Refactor merged.\n\n### Delete legacyAuthFlow() and the per-tenant flag (D12)\n**What:** Remove the legacy path, the routing flag, and the parity assertions once every tenant\nis on the `AuthBroker` path.\n**Why:** A strangler without scheduled demolition is two auth code paths forever.\n**Context:** Watch the legacy-path counter added in S5; when it reads zero for a full cycle,\ndelete. Characterization tests become new-path-only tests.\n**Effort:** S **Priority:** P2 **Depends on:** All tenants flagged onto the new path.\n\n### Audit auth module for swallow-and-continue catch blocks (D13)\n**What:** Find catch blocks that neither rethrow, return an explicit failure, nor log, and route\nthem through the D8 error boundary.\n**Why:** `validateAndDispatch()` is unlikely to be the only site; same silent-failure class.\n**Context:** Grep `catch` in the auth directory for bodies with no throw/return-error/log.\n**Effort:** M **Priority:** P2 **Depends on:** D8 pipeline merged.\n\n## Implementation Tasks\nSynthesized from this review's findings. Each task derives from a specific finding above.\nRun with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~1 day / CC: ~20 min)** — auth/legacy — Write characterization tests for `legacyAuthFlow()` before any rewrite\n - Surfaced by: Test review, REGRESSION RULE — PLAN.md:27-28, PLAN.md:15-16\n - Files: auth/__tests__/ (match repo convention)\n - Verify: tests pass against the current legacy path; fixtures cover the 7 listed cases\n- [ ] **T2 (P1, human: ~30 min / CC: ~5 min)** — plan — Author confirms goal, latency target and two invariants in Context\n - Surfaced by: Architecture issue 3 (D7)\n - Files: this plan's Context section\n - Verify: Context has no \"(draft)\" markers left\n- [ ] **T3 (P1, human: ~1 day / CC: ~20 min)** — auth/broker — `AuthBroker` sole writer with invalidation-generation check; `SessionMint` reads; adapter constructor-injected\n - Surfaced by: Architecture issue 1 (D5) — PLAN.md:19-20, :10\n - Files: auth/ (broker, mint), existing invalidation hooks bump the generation\n - Verify: tests #2-#8\n- [ ] **T4 (P1, human: ~1 day / CC: ~15 min)** — auth/routing — Per-tenant flag between legacy and new path, plus legacy-path counter\n - Surfaced by: Architecture issue 2 (D6)\n - Files: auth/ (routing), config/\n - Verify: test #16 incl. flag-store-unavailable → legacy\n- [ ] **T5 (P1, human: ~1 day / CC: ~20 min)** — auth/validate — Flatten `validateAndDispatch()` into typed pipeline + one error boundary\n - Surfaced by: Code quality issue 4 (D8) — PLAN.md:23-24\n - Files: auth/ (validate); reuse an existing Result helper if one exists\n - Verify: tests #9-#10\n- [ ] **T6 (P1, human: ~3 days / CC: ~45 min)** — auth/tests — Full test map: race, isolation, concurrency, IDP failure modes, 2 E2E\n - Surfaced by: Test review issue 5 (D9)\n - Files: auth/__tests__/, e2e/\n - Verify: coverage map reads 23/23\n- [ ] **T7 (P2, human: ~1.5 days / CC: ~25 min)** — auth/idp — Parallel IDP calls with shared abort + per-call timeout; cache discovery + JWKS with unknown-kid refresh\n - Surfaced by: Performance issue 6 (D10) — PLAN.md:31-32\n - Files: auth/idp/\n - Verify: tests #11-#15; hot path makes ≤3 network calls\n- [ ] **T8 (P3, human: ~15 min / CC: ~3 min)** — docs — Create TODOS.md with the three approved entries\n - Surfaced by: TODOS.md updates (D11-D13)\n - Files: TODOS.md\n - Verify: file exists with three H3 entries in the TODOS-format shape\n\n## Suppressed findings\n- `[P3] (confidence: 5/10) PLAN.md:7-9` — Cache keys omit the IDP client ID; if a tenant runs two\n clients against one issuer/audience, entries could collide. Medium confidence, verify this\n is actually an issue against the real adapter key before acting. No code was available to\n quote, so this stays in the appendix.\n\n## Review completion summary\n- Step 0: Scope Challenge — scope reduced per recommendation (5 types/12 files → 2 types/~7 files)\n- Architecture Review: 3 issues found (all resolved: 1A, 2A, 3A)\n- Code Quality Review: 1 issue found (resolved: 4A)\n- Test Review: diagram produced, 23 gaps identified (1 CRITICAL regression added by rule; rest resolved: 5A)\n- Performance Review: 1 issue found (resolved: 6A)\n- NOT in scope: written\n- What already exists: written\n- TODOS.md updates: 3 items proposed to user, 3 accepted\n- Failure modes: 0 critical gaps remaining (1 would-be critical gap closed by D5)\n- Outside voice: skipped (codex_reviews disabled)\n- Parallelization: 5 lanes, 4 parallel / 1 sequential after merge\n- Lake Score: 6/6 recommendations chose the complete option\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 0 | — | — |\n| Outside Review | codex via `/plan-eng-review` (disabled) | Independent 2nd opinion | 1 | disabled | 0 (skipped, host=claude, provider=codex, phase=plan-review) |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 1 | clean (PLAN) | 24 issues, 0 critical gaps, mode SCOPE_REDUCED |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — |\n\n**OUTSIDE COVERAGE:** provider codex, phase plan-review, outside_status disabled (user opt-out via `codex_reviews=disabled`); no native fallback dispatched; 0 findings. Outside coverage is absent, not clean.\n\n**VERDICT:** ENG CLEARED — ready to implement once Prerequisite P0 (author confirms Context) is done. Outside review disabled by config.\n\nNO UNRESOLVED DECISIONS\n", + "transcript": { + "status": "ready", + "calls": [ + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01PB4xuaMCGz1CRsiJYoirn5", + "questions": [ + { + "question": "D1 — Add gstack skill routing rules to this project's CLAUDE.md?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, about to run /plan-eng-review on PLAN.md.\nELI10: gstack works best when your project's CLAUDE.md includes skill routing rules, so that requests like \"review the architecture\" or \"ship this\" automatically route to the right skill instead of you naming it each time. This is a one-time setup prompt per project. Note: we are in plan mode right now, so if you pick A I will record the choice and append/commit the section only after plan mode exits.\nStakes if we pick wrong: Without routing, skills only fire when you name them explicitly; with routing, nothing breaks, you just get one extra section in CLAUDE.md.\nRecommendation: A because routing rules make the skill suite self-serve and cost one small commit.\nNote: options differ in kind, not coverage — no completeness score.\nNet: a small CLAUDE.md append versus invoking skills by name forever.", + "header": "Routing", + "multiSelect": false, + "options": [ + { + "label": "Add routing rules (recommended)", + "description": "✅ Requests auto-route to the matching skill without naming it each time. ✅ One short section appended to CLAUDE.md, committed once (human: ~2 min / CC: ~10 sec). ❌ Adds a gstack-specific block to a project file teammates also read." + }, + { + "label": "No thanks, invoke manually", + "description": "✅ CLAUDE.md stays exactly as it is today, nothing extra committed. ✅ You keep full explicit control over when skills run. ❌ You must type the /skill name every time, and this prompt will not re-offer." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Add gstack skill routing rules to this project's CLAUDE.md?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, about to run /plan-eng-review on PLAN.md.\nELI10: gstack works best when your project's CLAUDE.md includes skill routing rules, so that requests like \"review the architecture\" or \"ship this\" automatically route to the right skill instead of you naming it each time. This is a one-time setup prompt per project. Note: we are in plan mode right now, so if you pick A I will record the choice and append/commit the section only after plan mode exits.\nStakes if we pick wrong: Without routing, skills only fire when you name them explicitly; with routing, nothing breaks, you just get one extra section in CLAUDE.md.\nRecommendation: A because routing rules make the skill suite self-serve and cost one small commit.\nNote: options differ in kind, not coverage — no completeness score.\nNet: a small CLAUDE.md append versus invoking skills by name forever.": "Add routing rules (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:18:59.984Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01DCxScs8A4buRspLArYkfSR", + "questions": [ + { + "question": "D2 — Run /office-hours first, or proceed with the standard review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewing PLAN.md (Multi-tenant Auth Refactor).\nELI10: No design doc found for this branch. /office-hours produces a structured problem statement, premise challenge, and explored alternatives, which gives this review sharper input. It takes about 10 minutes. The design doc is per-feature, not per-product. PLAN.md today reads as a list of known smells with no stated goal, so the problem statement is genuinely missing.\nStakes if we pick wrong: Skipping means the review must infer the goal from the smells and flag the missing problem statement as a finding; running it delays the requested review deliverable by a full extra workflow.\nRecommendation: B because you asked for the reviewed plan deliverable now, and the missing goal statement is better captured as an explicit finding in this review than by detouring.\nNote: options differ in kind, not coverage — no completeness score.\nNet: sharper input versus the deliverable you asked for, sooner.", + "header": "Prereq", + "multiSelect": false, + "options": [ + { + "label": "Skip, standard review (recommended)", + "description": "✅ Delivers the reviewed plan now, as requested, with the missing goal flagged as a finding. ✅ No extra workflow or context spent before the review starts (human: 0 / CC: 0). ❌ The review infers the refactor's goal from the listed smells rather than a stated problem." + }, + { + "label": "Run /office-hours now", + "description": "✅ Produces a real problem statement and alternatives before architecture gets locked in. ✅ The review then grounds every recommendation in a stated goal (human: ~10 min / CC: ~10 min). ❌ Delays the requested deliverable and adds a second interactive workflow to this session." + } + ] + }, + { + "question": "D3 — Enable cross-project learnings search?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, one-time gstack setting.\nELI10: gstack can search learnings from your other projects on this machine to find patterns that might apply here. This stays local, no data leaves your machine. Recommended for solo developers. Skip if you work on multiple client codebases where cross-contamination would be a concern.\nStakes if we pick wrong: Enabling on a multi-client machine could surface one client's patterns while reviewing another's; disabling loses reuse of pitfalls you already paid for elsewhere.\nRecommendation: A because it is local-only and the learnings store is currently empty here, so the only upside comes from other projects.\nNote: options differ in kind, not coverage — no completeness score.\nNet: reuse of past lessons versus strict per-project isolation.", + "header": "Learnings", + "multiSelect": false, + "options": [ + { + "label": "Enable cross-project (recommended)", + "description": "✅ Past pitfalls and patterns from your other repos inform this and future reviews. ✅ Entirely local; one config flag you can flip back any time. ❌ On a shared or multi-client machine, learnings from unrelated codebases can leak into recommendations." + }, + { + "label": "Project-scoped only", + "description": "✅ Guarantees no cross-codebase contamination of review advice. ✅ Simplest mental model: what this repo taught gstack stays with this repo. ❌ This repo has zero learnings today, so the search adds nothing until the store grows." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — Run /office-hours first, or proceed with the standard review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewing PLAN.md (Multi-tenant Auth Refactor).\nELI10: No design doc found for this branch. /office-hours produces a structured problem statement, premise challenge, and explored alternatives, which gives this review sharper input. It takes about 10 minutes. The design doc is per-feature, not per-product. PLAN.md today reads as a list of known smells with no stated goal, so the problem statement is genuinely missing.\nStakes if we pick wrong: Skipping means the review must infer the goal from the smells and flag the missing problem statement as a finding; running it delays the requested review deliverable by a full extra workflow.\nRecommendation: B because you asked for the reviewed plan deliverable now, and the missing goal statement is better captured as an explicit finding in this review than by detouring.\nNote: options differ in kind, not coverage — no completeness score.\nNet: sharper input versus the deliverable you asked for, sooner.": "Skip, standard review (recommended)", + "D3 — Enable cross-project learnings search?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, one-time gstack setting.\nELI10: gstack can search learnings from your other projects on this machine to find patterns that might apply here. This stays local, no data leaves your machine. Recommended for solo developers. Skip if you work on multiple client codebases where cross-contamination would be a concern.\nStakes if we pick wrong: Enabling on a multi-client machine could surface one client's patterns while reviewing another's; disabling loses reuse of pitfalls you already paid for elsewhere.\nRecommendation: A because it is local-only and the learnings store is currently empty here, so the only upside comes from other projects.\nNote: options differ in kind, not coverage — no completeness score.\nNet: reuse of past lessons versus strict per-project isolation.": "Enable cross-project (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:20:58.274Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01TKTdk9cTrnYbRqZeLaqaUd", + "questions": [ + { + "question": "D4 — Scope: 12 files and 5 new types for the auth refactor. Reduce, or proceed as-is?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md lines 19-20 and 35-36.\nELI10: The plan adds AuthBroker, SessionMint, TokenStore, AuthCache and RequestPolicy. But PLAN.md:11-13 says AuthCache is just a facade over the existing adapter with one backing cache and the adapter stays unchanged, so AuthCache is a second name for something you already have. TokenStore overlaps the adapter's token storage job, and RequestPolicy has no stated consumer anywhere in the plan. Every extra type is another place a 3am on-call engineer has to look when auth breaks.\nStakes if we pick wrong: Over-scoped, you ship a 12-file auth change with two layers doing one job and a harder rollback. Under-scoped, a real seam (say, RequestPolicy) gets cut and gets bolted on later in a worse spot.\nRecommendation: A because the existing adapter already owns tenant keys, eviction and invalidation, so handing it to AuthBroker and SessionMint by constructor injection achieves the goal with two new types and roughly 7 files.\nNote: options differ in kind, not coverage — no completeness score.\nNet: two focused services on the proven adapter versus five types that split one cache across two layers.", + "header": "Scope", + "multiSelect": false, + "options": [ + { + "label": "A) Reduce to 2 new types (recommended)", + "description": "✅ AuthBroker and SessionMint take the existing adapter via constructor; AuthCache and TokenStore dropped, RequestPolicy deferred until a consumer exists (human: ~3 days / CC: ~1 hr). ✅ Fewer files in the auth diff means a smaller blast radius and an easier revert. ❌ If RequestPolicy turns out to be load-bearing, it lands as a follow-up rather than in this design pass." + }, + { + "label": "B) Proceed as-is, 5 types / 12 files", + "description": "✅ Every seam the author envisioned is present from day one; no follow-up structural PR. ✅ No re-planning cost right now (human: ~1 week / CC: ~2 hr). ❌ Two cache layers (adapter + AuthCache) over one backing store, plus two types with no stated job, in an auth path." + }, + { + "label": "C) Drop AuthCache only, keep TokenStore + RequestPolicy", + "description": "✅ Removes the clearest duplicate while keeping the author's other seams intact. ✅ Moderate diff, roughly 9-10 files (human: ~4 days / CC: ~1.5 hr). ❌ TokenStore still overlaps the adapter's storage role, and RequestPolicy still has no consumer in the plan." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Scope: 12 files and 5 new types for the auth refactor. Reduce, or proceed as-is?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md lines 19-20 and 35-36.\nELI10: The plan adds AuthBroker, SessionMint, TokenStore, AuthCache and RequestPolicy. But PLAN.md:11-13 says AuthCache is just a facade over the existing adapter with one backing cache and the adapter stays unchanged, so AuthCache is a second name for something you already have. TokenStore overlaps the adapter's token storage job, and RequestPolicy has no stated consumer anywhere in the plan. Every extra type is another place a 3am on-call engineer has to look when auth breaks.\nStakes if we pick wrong: Over-scoped, you ship a 12-file auth change with two layers doing one job and a harder rollback. Under-scoped, a real seam (say, RequestPolicy) gets cut and gets bolted on later in a worse spot.\nRecommendation: A because the existing adapter already owns tenant keys, eviction and invalidation, so handing it to AuthBroker and SessionMint by constructor injection achieves the goal with two new types and roughly 7 files.\nNote: options differ in kind, not coverage — no completeness score.\nNet: two focused services on the proven adapter versus five types that split one cache across two layers.": "A) Reduce to 2 new types (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:22:14.017Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01MaBGFFWUhpt2tpUYCffX3Z", + "questions": [ + { + "question": "D5 — Issue 1: AuthBroker and SessionMint both mutate one shared cache with no ordering guard. Who owns writes?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:19-20 (\"share a global mutable AuthCache ... Both services mutate it\") and PLAN.md:10 (\"they do not serialize mutations\").\nELI10: Two services writing the same tenant-keyed cache with nothing ordering them is a race. Realistic failure: SessionMint starts minting a session for tenant A, the tenant gets suspended and the existing hook invalidates A's entries, then SessionMint's in-flight write lands and puts a fresh token back. A suspended tenant keeps working until the token expires, silently. The existing adapter guarantees validity and tenant keys, but PLAN.md:10 says outright that it does not serialize mutations, so nothing today prevents this. Even with constructor injection (D4), two writers still race.\nStakes if we pick wrong: Revoked or suspended tenants can resurrect access with no error and no log line; that is a security incident, not a bug.\nRecommendation: A because a single writer plus a version check on write is explicit, testable, and costs one small guard where all writes route through, which matches your explicit-over-clever and root-cause preferences.\nCompleteness: A=10/10, B=6/10, C=3/10\nNet: one guarded write path versus two unguarded writers that only fail under the timing you never see in tests.", + "header": "Arch 1", + "multiSelect": false, + "options": [ + { + "label": "1A) Single writer + versioned write (recommended)", + "description": "✅ AuthBroker is the only component that writes tokens; SessionMint reads and asks the broker to mint. Writes carry the invalidation generation they read, and a write with a stale generation is dropped and logged (human: ~1 day / CC: ~20 min). ✅ Testable deterministically: one interleaving test proves a suspended tenant cannot resurrect a token. ❌ Adds a generation counter to the adapter wrapper call path and one more concept to explain." + }, + { + "label": "1B) Inject the adapter into both, both still write", + "description": "✅ Removes the module-level global, so tests can pass a fake adapter (human: ~2 hr / CC: ~5 min). ✅ Smallest diff on top of the D4 scope reduction. ❌ The invalidate-then-stale-write race stays; only the failure's discoverability improves." + }, + { + "label": "1C) Keep the module-level export as planned", + "description": "✅ Zero additional work beyond what PLAN.md already describes. ✅ Matches the module-cache singleton idiom some TypeScript codebases already use. ❌ Hidden global coupling, untestable without module mocking, and the resurrection race ships unaddressed." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Issue 1: AuthBroker and SessionMint both mutate one shared cache with no ordering guard. Who owns writes?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:19-20 (\"share a global mutable AuthCache ... Both services mutate it\") and PLAN.md:10 (\"they do not serialize mutations\").\nELI10: Two services writing the same tenant-keyed cache with nothing ordering them is a race. Realistic failure: SessionMint starts minting a session for tenant A, the tenant gets suspended and the existing hook invalidates A's entries, then SessionMint's in-flight write lands and puts a fresh token back. A suspended tenant keeps working until the token expires, silently. The existing adapter guarantees validity and tenant keys, but PLAN.md:10 says outright that it does not serialize mutations, so nothing today prevents this. Even with constructor injection (D4), two writers still race.\nStakes if we pick wrong: Revoked or suspended tenants can resurrect access with no error and no log line; that is a security incident, not a bug.\nRecommendation: A because a single writer plus a version check on write is explicit, testable, and costs one small guard where all writes route through, which matches your explicit-over-clever and root-cause preferences.\nCompleteness: A=10/10, B=6/10, C=3/10\nNet: one guarded write path versus two unguarded writers that only fail under the timing you never see in tests.": "1A) Single writer + versioned write (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:22:50.133Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01FpJ6XEt4Yh3nyovGp5qWts", + "questions": [ + { + "question": "D6 — Issue 2: legacyAuthFlow() is rewritten in place with no way to roll back. Gate the new path behind a flag?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:27-28 (\"legacyAuthFlow() will get rewritten as part of this work\").\nELI10: The plan replaces the existing login path outright. If the new AuthBroker path has a bug for one tenant's IDP quirk, the only fix is a revert of the whole 7-file change while users are locked out. A strangler-fig rollout keeps legacyAuthFlow() callable, routes traffic by a per-tenant flag, and lets you move tenants over in waves, then delete the old path in a follow-up commit once every tenant is on the new one.\nStakes if we pick wrong: Without a flag, the first production auth regression is a full revert under lockout pressure; with one, it is a flag flip for one tenant.\nRecommendation: A because auth is the one path where reversibility is worth a temporary second branch, and the deletion is a trivial follow-up once tenants are migrated.\nCompleteness: A=10/10, B=7/10, C=3/10\nNet: a short-lived flag and one extra branch versus a big-bang cutover on the login path.", + "header": "Arch 2", + "multiSelect": false, + "options": [ + { + "label": "2A) Per-tenant flag, strangler rollout (recommended)", + "description": "✅ New path enabled per tenant; any regression is a flag flip, not a revert (human: ~1 day / CC: ~15 min). ✅ Old and new paths run side by side, so the characterization tests can assert parity on the same inputs. ❌ Two live code paths until the cleanup commit deletes legacyAuthFlow()." + }, + { + "label": "2B) Global on/off flag only", + "description": "✅ Still a one-line rollback instead of a revert (human: ~2 hr / CC: ~5 min). ✅ Less flag plumbing than per-tenant routing. ❌ A problem for one tenant's IDP forces every tenant back to the old path." + }, + { + "label": "2C) Rewrite in place, no flag", + "description": "✅ Smallest diff and no temporary dual path to maintain. ✅ Nothing to clean up afterwards. ❌ Rollback is a full revert under active lockout, on the auth path." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 — Issue 2: legacyAuthFlow() is rewritten in place with no way to roll back. Gate the new path behind a flag?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:27-28 (\"legacyAuthFlow() will get rewritten as part of this work\").\nELI10: The plan replaces the existing login path outright. If the new AuthBroker path has a bug for one tenant's IDP quirk, the only fix is a revert of the whole 7-file change while users are locked out. A strangler-fig rollout keeps legacyAuthFlow() callable, routes traffic by a per-tenant flag, and lets you move tenants over in waves, then delete the old path in a follow-up commit once every tenant is on the new one.\nStakes if we pick wrong: Without a flag, the first production auth regression is a full revert under lockout pressure; with one, it is a flag flip for one tenant.\nRecommendation: A because auth is the one path where reversibility is worth a temporary second branch, and the deletion is a trivial follow-up once tenants are migrated.\nCompleteness: A=10/10, B=7/10, C=3/10\nNet: a short-lived flag and one extra branch versus a big-bang cutover on the login path.": "2A) Per-tenant flag, strangler rollout (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:23:08.211Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_0147cpYN9qq37APE6SWMwcUX", + "questions": [ + { + "question": "D7 — Issue 3: PLAN.md never states the refactor's goal or its invariants. Require them before implementation?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:4-36 (every section describes a smell; none states what users or operators gain).\nELI10: The plan says what is wrong with the code but not what \"done\" means: which tenant-isolation guarantees must hold, which latency target token validation must hit, and what the multi-tenant change unlocks. Without that, every implementer picks their own definition, and the tests in Section 3 have nothing to assert against beyond \"it runs.\" The reviewed plan will carry a Context section with the goal I can infer and two invariants (no cross-tenant reads, no token resurrection after revocation), but the author must confirm or correct them.\nStakes if we pick wrong: Code that passes its own tests but solves a different problem than the one that prompted the refactor.\nRecommendation: A because writing three sentences now is far cheaper than discovering the goal mismatch in review of a 7-file auth diff, matching your thoughtfulness-over-speed preference.\nCompleteness: A=10/10, B=6/10\nNet: a short, explicit definition of done versus an inferred one that nobody signed off on.", + "header": "Arch 3", + "multiSelect": false, + "options": [ + { + "label": "3A) Author confirms goal + invariants first (recommended)", + "description": "✅ Implementation starts from a stated goal, a latency target, and two named security invariants that tests assert directly (human: ~30 min / CC: ~5 min to draft). ✅ The reviewed plan ships with the inferred draft filled in, so confirming is a read-and-edit, not a blank page. ❌ Implementation is blocked until someone with product context confirms the draft." + }, + { + "label": "3B) Proceed on the inferred goal", + "description": "✅ No blocking step; the review's inferred Context section stands as written. ✅ Fine if the author is you and the inference matches your intent. ❌ If the inference is wrong, tests and architecture were tuned to the wrong target." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 — Issue 3: PLAN.md never states the refactor's goal or its invariants. Require them before implementation?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:4-36 (every section describes a smell; none states what users or operators gain).\nELI10: The plan says what is wrong with the code but not what \"done\" means: which tenant-isolation guarantees must hold, which latency target token validation must hit, and what the multi-tenant change unlocks. Without that, every implementer picks their own definition, and the tests in Section 3 have nothing to assert against beyond \"it runs.\" The reviewed plan will carry a Context section with the goal I can infer and two invariants (no cross-tenant reads, no token resurrection after revocation), but the author must confirm or correct them.\nStakes if we pick wrong: Code that passes its own tests but solves a different problem than the one that prompted the refactor.\nRecommendation: A because writing three sentences now is far cheaper than discovering the goal mismatch in review of a 7-file auth diff, matching your thoughtfulness-over-speed preference.\nCompleteness: A=10/10, B=6/10\nNet: a short, explicit definition of done versus an inferred one that nobody signed off on.": "3A) Author confirms goal + invariants first (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:23:26.274Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01AYqEFiSbnVFUrhPY3WGjA8", + "questions": [ + { + "question": "D8 — Issue 4: validateAndDispatch() is 60 lines with three nested try/catch blocks that each swallow an error class. Flatten it?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:23-24 (\"three nested try/catch blocks; each catch swallows a different error class\").\nELI10: A swallowed error means the function keeps going as if nothing happened. In an auth path, that is how an IDP timeout, a malformed token, and a policy lookup miss all turn into the same silent \"validation passed\" or \"validation failed\" with no log, no metric and no way for on-call to tell them apart at 3am. The fix is a straight-line pipeline of small steps, each returning a typed result, with one boundary that maps each error class to an explicit outcome and a log line.\nStakes if we pick wrong: Production auth failures stay invisible; the first signal is a customer ticket rather than an alert.\nRecommendation: A because a typed pipeline with a single error boundary is explicit over clever, each step gets its own unit test, and the diff is the same size as fixing the swallows in place.\nCompleteness: A=10/10, B=7/10, C=2/10\nNet: three observable, testable failure modes versus three invisible ones.", + "header": "Code 4", + "multiSelect": false, + "options": [ + { + "label": "4A) Typed step pipeline + one error boundary (recommended)", + "description": "✅ Steps (parse, verify signature, check tenant policy, dispatch) each return a typed Result; one boundary maps every error class to a distinct outcome, log line and metric (human: ~1 day / CC: ~20 min). ✅ Each step is unit-testable in isolation; the error boundary gets one table-driven test per class. ❌ Introduces a small Result type or reuses one already in the repo; implementer must check the reuse ladder first." + }, + { + "label": "4B) Keep the structure, stop swallowing", + "description": "✅ Minimal diff: each catch now rethrows a typed error or returns an explicit failure (human: ~2 hr / CC: ~5 min). ✅ Errors become visible and loggable without restructuring. ❌ Three nested blocks stay, so the 60-line function remains hard to test branch by branch." + }, + { + "label": "4C) Leave as-is", + "description": "✅ No change, no risk of altering current behavior. ✅ Zero effort. ❌ Silent failures on the auth path ship into the refactor untouched." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D8 — Issue 4: validateAndDispatch() is 60 lines with three nested try/catch blocks that each swallow an error class. Flatten it?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:23-24 (\"three nested try/catch blocks; each catch swallows a different error class\").\nELI10: A swallowed error means the function keeps going as if nothing happened. In an auth path, that is how an IDP timeout, a malformed token, and a policy lookup miss all turn into the same silent \"validation passed\" or \"validation failed\" with no log, no metric and no way for on-call to tell them apart at 3am. The fix is a straight-line pipeline of small steps, each returning a typed result, with one boundary that maps each error class to an explicit outcome and a log line.\nStakes if we pick wrong: Production auth failures stay invisible; the first signal is a customer ticket rather than an alert.\nRecommendation: A because a typed pipeline with a single error boundary is explicit over clever, each step gets its own unit test, and the diff is the same size as fixing the swallows in place.\nCompleteness: A=10/10, B=7/10, C=2/10\nNet: three observable, testable failure modes versus three invisible ones.": "4A) Typed step pipeline + one error boundary (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:23:48.343Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01J1JWDEDDrabRKK9YwUNauA", + "questions": [ + { + "question": "D9 — Issue 5: PLAN.md:14-15 promises success/error coverage for new components only. How far does test coverage go?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:14-16 (\"coverage is planned for the new components and their success/error paths\").\nELI10: The legacyAuthFlow() characterization tests are already mandatory under the regression rule and are not part of this question. What is open is the rest of the map above: the invalidation-during-mint race, the tenant-isolation check (tenant A's token never visible through tenant B's key), IDP partial failure and timeout, the error-boundary table, and the two end-to-end flows (login through a validated request, and tenant suspended mid-session). These are the tests that prove the two security invariants from D7, and unit tests alone cannot show the suspended-tenant flow works across broker, cache and hooks.\nStakes if we pick wrong: The two invariants that justify the refactor ship unproven; the race in D5 is exactly the kind of bug that only a deliberate interleaving test ever catches.\nRecommendation: A because with CC the full set costs minutes, not days, and you have said you would rather have too many tests than too few.\nCompleteness: A=10/10, B=7/10, C=4/10\nNet: every branch and both invariants asserted versus happy-path confidence on an auth path.", + "header": "Tests 5", + "multiSelect": false, + "options": [ + { + "label": "5A) Full map: units + race + isolation + 2 E2E (recommended)", + "description": "✅ All 18 non-regression gaps covered, including a deterministic interleaving test for the stale-write race and a cross-tenant isolation test (human: ~3 days / CC: ~45 min). ✅ Two E2E flows prove the invariants end to end where mocking would hide hook wiring mistakes. ❌ The interleaving test needs controllable async ordering in the fake adapter; slightly more test scaffolding." + }, + { + "label": "5B) Units + race + isolation, no E2E", + "description": "✅ Every unit branch and both invariant tests land at the unit level (human: ~2 days / CC: ~30 min). ✅ No E2E harness or IDP stub to stand up. ❌ Hook wiring between adapter, broker and suspension path is only proven by mocks, which is where real failures hide." + }, + { + "label": "5C) Success/error paths as the plan states", + "description": "✅ Matches PLAN.md as written, no added test scope. ✅ Fastest to land (human: ~1 day / CC: ~15 min). ❌ The race, tenant isolation, IDP timeout and both E2E flows stay untested; the invariants are asserted nowhere." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D9 — Issue 5: PLAN.md:14-15 promises success/error coverage for new components only. How far does test coverage go?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:14-16 (\"coverage is planned for the new components and their success/error paths\").\nELI10: The legacyAuthFlow() characterization tests are already mandatory under the regression rule and are not part of this question. What is open is the rest of the map above: the invalidation-during-mint race, the tenant-isolation check (tenant A's token never visible through tenant B's key), IDP partial failure and timeout, the error-boundary table, and the two end-to-end flows (login through a validated request, and tenant suspended mid-session). These are the tests that prove the two security invariants from D7, and unit tests alone cannot show the suspended-tenant flow works across broker, cache and hooks.\nStakes if we pick wrong: The two invariants that justify the refactor ship unproven; the race in D5 is exactly the kind of bug that only a deliberate interleaving test ever catches.\nRecommendation: A because with CC the full set costs minutes, not days, and you have said you would rather have too many tests than too few.\nCompleteness: A=10/10, B=7/10, C=4/10\nNet: every branch and both invariants asserted versus happy-path confidence on an auth path.": "5A) Full map: units + race + isolation + 2 E2E (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:24:24.490Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_016wNkSKGArZFFzxvxVN3B7i", + "questions": [ + { + "question": "D10 — Issue 6: 5 sequential IDP calls per validation. Promise.all as planned, or bounded parallelism plus caching?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:31-32 (\"5 sequential API calls to the IDP; they could be parallelized via Promise.all trivially\").\nELI10: Parallelizing is right, but \"trivially\" hides two problems. Promise.all is fail-fast: when one call rejects, the other four keep running against the IDP with nobody listening, so a flaky IDP now costs 5 wasted calls per failed validation instead of 1. And per-tenant IDPs usually rate-limit; going from 1 to 5 concurrent calls per request multiplies your burst. Separately, at least two of the five calls (discovery document, JWKS signing keys) are static-ish and standard practice is to cache them with a TTL and refresh on unknown key id, which removes those calls entirely rather than speeding them up.\nStakes if we pick wrong: Under IDP degradation the naive version amplifies load on the thing that is already failing, and validation latency stays bound by the slowest of five network calls.\nRecommendation: A because AbortController plus a shared signal makes fail-fast actually cancel, per-call timeouts bound the tail, and caching discovery and JWKS is Layer 1 practice that turns 5 calls into 3 or fewer on the hot path.\nCompleteness: A=10/10, B=6/10, C=3/10\nNet: fewer, bounded, cancellable IDP calls versus five unbounded calls that pile onto a degraded IDP.", + "header": "Perf 6", + "multiSelect": false, + "options": [ + { + "label": "6A) Cancel-on-fail + timeouts + cache discovery/JWKS (recommended)", + "description": "✅ Promise.all over calls sharing one AbortSignal, per-call AbortSignal.timeout, and a TTL cache for discovery and JWKS with refresh on unknown kid (human: ~1.5 days / CC: ~25 min). ✅ Hot path drops to 3 or fewer network calls; failures are typed and bounded; IDP burst per request is capped. ❌ JWKS caching needs the unknown-kid refresh path and its test, or a key rotation locks tenants out." + }, + { + "label": "6B) Promise.all + per-call timeouts only", + "description": "✅ Latency drops from sum-of-5 to max-of-5 with bounded tails (human: ~3 hr / CC: ~8 min). ✅ No new cache surface to reason about. ❌ First rejection still leaves four calls running; still 5 calls per validation and 5x burst against rate-limited IDPs." + }, + { + "label": "6C) Bare Promise.all as PLAN.md states", + "description": "✅ One-line change, exactly what the plan proposes. ✅ Immediate latency win on the happy path. ❌ No timeouts, no cancellation, no caching: a hung IDP call hangs validation, and failures amplify IDP load." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D10 — Issue 6: 5 sequential IDP calls per validation. Promise.all as planned, or bounded parallelism plus caching?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:31-32 (\"5 sequential API calls to the IDP; they could be parallelized via Promise.all trivially\").\nELI10: Parallelizing is right, but \"trivially\" hides two problems. Promise.all is fail-fast: when one call rejects, the other four keep running against the IDP with nobody listening, so a flaky IDP now costs 5 wasted calls per failed validation instead of 1. And per-tenant IDPs usually rate-limit; going from 1 to 5 concurrent calls per request multiplies your burst. Separately, at least two of the five calls (discovery document, JWKS signing keys) are static-ish and standard practice is to cache them with a TTL and refresh on unknown key id, which removes those calls entirely rather than speeding them up.\nStakes if we pick wrong: Under IDP degradation the naive version amplifies load on the thing that is already failing, and validation latency stays bound by the slowest of five network calls.\nRecommendation: A because AbortController plus a shared signal makes fail-fast actually cancel, per-call timeouts bound the tail, and caching discovery and JWKS is Layer 1 practice that turns 5 calls into 3 or fewer on the hot path.\nCompleteness: A=10/10, B=6/10, C=3/10\nNet: fewer, bounded, cancellable IDP calls versus five unbounded calls that pile onto a degraded IDP.": "6A) Cancel-on-fail + timeouts + cache discovery/JWKS (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:24:46.590Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01EaGbiNdd6gqQMbuwLcUT2o", + "questions": [ + { + "question": "D11 — TODO 1: RequestPolicy (deferred in D4).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:35-36.\nWhat: Introduce a RequestPolicy type once a concrete consumer needs per-request policy decisions beyond what the adapter's policy-version key already provides.\nWhy: The scope reduction dropped it because no plan section names who calls it; capturing it keeps the author's intent from being lost.\nPros: Preserves the design idea with the reasoning; a future implementer starts from the seam the author saw. Cons: Risk of building a type nobody calls if the need never materialises.\nContext: PLAN.md listed RequestPolicy among 4 new classes with no description. The adapter already keys on policy version. Start by grepping call sites that branch on tenant policy after the refactor lands; if there are 2+, that is the consumer.\nDepends on: Refactor merged (D4 scope).\nRecommendation: A because the idea had an author behind it and a TODO with context costs nothing.\nNote: options differ in kind, not coverage — no completeness score.\nNet: keep the idea with its reasoning versus let it vanish with the scope cut.", + "header": "TODO 1", + "multiSelect": false, + "options": [ + { + "label": "A) Add to TODOS.md (recommended)", + "description": "✅ The seam and its trigger condition are recorded for whoever hits the need (P3, Effort M). ✅ Nothing is built speculatively now. ❌ One more backlog item that may never be picked up." + }, + { + "label": "B) Skip, not valuable enough", + "description": "✅ Backlog stays clean; the idea can be re-derived if the need appears. ✅ Zero bookkeeping. ❌ The author's reasoning for wanting it is lost with the scope cut." + }, + { + "label": "C) Build it now in this PR", + "description": "✅ Every seam the author envisioned ships together. ✅ No follow-up structural change. ❌ Reverses D4 and adds a type with no stated consumer to an auth diff." + } + ] + }, + { + "question": "D12 — TODO 2: Delete legacyAuthFlow() and the per-tenant flag after migration (from D6).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:27-28.\nWhat: Once every tenant runs on the AuthBroker path, remove legacyAuthFlow(), the routing flag, and the parity tests that compare the two paths.\nWhy: A strangler flag left in place forever becomes two auth code paths to maintain and an accidental rollback nobody remembers exists.\nPros: The refactor actually finishes; dead auth code is removed. Cons: Requires confirming 100% tenant migration first, which needs a metric or flag audit.\nContext: D6 chose per-tenant flag routing. Start by adding a counter of requests served by the legacy path; when it reads zero for a full cycle, delete. The characterization tests move from parity assertions to asserting the new path alone.\nDepends on: All tenants flagged onto the new path.\nRecommendation: A because a strangler without a scheduled demolition is just two code paths.\nNote: options differ in kind, not coverage — no completeness score.\nNet: a planned cleanup versus a permanent dual path.", + "header": "TODO 2", + "multiSelect": false, + "options": [ + { + "label": "A) Add to TODOS.md (recommended)", + "description": "✅ The cleanup has an owner condition and a start point (P2, Effort S). ✅ Prevents the flag from becoming permanent architecture. ❌ Requires the legacy-path counter to be part of the flag work now." + }, + { + "label": "B) Skip, not valuable enough", + "description": "✅ No backlog entry to track. ✅ Nothing changes in this PR. ❌ The dual path has no scheduled end and will outlive everyone's memory of why it exists." + }, + { + "label": "C) Build it now in this PR", + "description": "✅ No dual path ever exists. ✅ Smallest final codebase immediately. ❌ Contradicts D6; deleting the legacy path now removes the rollback the flag exists to provide." + } + ] + }, + { + "question": "D13 — TODO 3: Audit the rest of the auth module for the swallow-and-continue try/catch pattern (from D8).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:23-24.\nWhat: Grep the auth module for catch blocks that neither rethrow nor return an explicit failure, and route them through the error boundary introduced in D8.\nWhy: validateAndDispatch() is unlikely to be the only place the pattern appears; the same silent-failure class probably exists in siblings.\nPros: One error boundary becomes the module convention; on-call sees every auth failure. Cons: Touches files outside this refactor's diff; each site needs its own test.\nContext: D8 introduces a typed Result pipeline and a single boundary. Start with a grep for `catch` blocks in the auth directory whose body has no throw, no return of an error, and no log call.\nDepends on: D8 pipeline merged so there is a boundary to route to.\nRecommendation: A because it is separate scope (other files) but the same root cause, and root-cause fixes belong where all callers route through.\nNote: options differ in kind, not coverage — no completeness score.\nNet: module-wide visibility of auth failures versus fixing one function and hoping.", + "header": "TODO 3", + "multiSelect": false, + "options": [ + { + "label": "A) Add to TODOS.md (recommended)", + "description": "✅ The sibling sites get fixed with the same boundary once it exists (P2, Effort M). ✅ Keeps this PR's diff scoped to the plan while not losing the finding. ❌ Silent failures elsewhere persist until the follow-up lands." + }, + { + "label": "B) Skip, not valuable enough", + "description": "✅ No extra backlog. ✅ Assumes validateAndDispatch() was the outlier. ❌ If it was not, the same 3am invisibility remains in neighbouring code." + }, + { + "label": "C) Build it now in this PR", + "description": "✅ Whole module converges on one error convention in a single change. ✅ No follow-up needed. ❌ Widens an auth diff past the agreed 7 files and mixes unrelated behavior changes into the refactor." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D11 — TODO 1: RequestPolicy (deferred in D4).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:35-36.\nWhat: Introduce a RequestPolicy type once a concrete consumer needs per-request policy decisions beyond what the adapter's policy-version key already provides.\nWhy: The scope reduction dropped it because no plan section names who calls it; capturing it keeps the author's intent from being lost.\nPros: Preserves the design idea with the reasoning; a future implementer starts from the seam the author saw. Cons: Risk of building a type nobody calls if the need never materialises.\nContext: PLAN.md listed RequestPolicy among 4 new classes with no description. The adapter already keys on policy version. Start by grepping call sites that branch on tenant policy after the refactor lands; if there are 2+, that is the consumer.\nDepends on: Refactor merged (D4 scope).\nRecommendation: A because the idea had an author behind it and a TODO with context costs nothing.\nNote: options differ in kind, not coverage — no completeness score.\nNet: keep the idea with its reasoning versus let it vanish with the scope cut.": "A) Add to TODOS.md (recommended)", + "D12 — TODO 2: Delete legacyAuthFlow() and the per-tenant flag after migration (from D6).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:27-28.\nWhat: Once every tenant runs on the AuthBroker path, remove legacyAuthFlow(), the routing flag, and the parity tests that compare the two paths.\nWhy: A strangler flag left in place forever becomes two auth code paths to maintain and an accidental rollback nobody remembers exists.\nPros: The refactor actually finishes; dead auth code is removed. Cons: Requires confirming 100% tenant migration first, which needs a metric or flag audit.\nContext: D6 chose per-tenant flag routing. Start by adding a counter of requests served by the legacy path; when it reads zero for a full cycle, delete. The characterization tests move from parity assertions to asserting the new path alone.\nDepends on: All tenants flagged onto the new path.\nRecommendation: A because a strangler without a scheduled demolition is just two code paths.\nNote: options differ in kind, not coverage — no completeness score.\nNet: a planned cleanup versus a permanent dual path.": "A) Add to TODOS.md (recommended)", + "D13 — TODO 3: Audit the rest of the auth module for the swallow-and-continue try/catch pattern (from D8).\nProject/branch/task: gstack-plan-count-b3qdhZ on main, PLAN.md:23-24.\nWhat: Grep the auth module for catch blocks that neither rethrow nor return an explicit failure, and route them through the error boundary introduced in D8.\nWhy: validateAndDispatch() is unlikely to be the only place the pattern appears; the same silent-failure class probably exists in siblings.\nPros: One error boundary becomes the module convention; on-call sees every auth failure. Cons: Touches files outside this refactor's diff; each site needs its own test.\nContext: D8 introduces a typed Result pipeline and a single boundary. Start with a grep for `catch` blocks in the auth directory whose body has no throw, no return of an error, and no log call.\nDepends on: D8 pipeline merged so there is a boundary to route to.\nRecommendation: A because it is separate scope (other files) but the same root cause, and root-cause fixes belong where all callers route through.\nNote: options differ in kind, not coverage — no completeness score.\nNet: module-wide visibility of auth failures versus fixing one function and hoping.": "A) Add to TODOS.md (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:25:54.870Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_01Je2eR9CH4vWsDYmEdCnsAP", + "questions": [ + { + "question": "D14 — Next step after this eng review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewed plan written to gstack-test-plan-eng.md.\nELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touched, so a design review does not apply. A CEO review is optional and normally for product-direction changes; this is a backend auth refactor, so it is a soft mention only.\nStakes if we pick wrong: Low either way; the CEO review would cost time on a change with no product-facing scope decision left open.\nRecommendation: A because all required reviews are complete and the plan's remaining blocker is the author confirming the Context section, not another review.\nNote: options differ in kind, not coverage — no completeness score.\nNet: start implementing versus an optional strategy pass on a backend refactor.", + "header": "Next", + "multiSelect": false, + "options": [ + { + "label": "A) Ready to implement (recommended)", + "description": "✅ All relevant reviews complete; run /ship when the work is done. ✅ The first task is the author confirming Context, then T1 characterization tests. ❌ No second strategic opinion on whether the refactor is the right thing to build now." + }, + { + "label": "B) Run /plan-ceo-review", + "description": "✅ Adds a scope-and-strategy pass before any code is written. ✅ Useful if the refactor's business motivation is contested. ❌ Backend-only refactor with no product-direction choice; likely low yield for the time." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D14 — Next step after this eng review?\nProject/branch/task: gstack-plan-count-b3qdhZ on main, reviewed plan written to gstack-test-plan-eng.md.\nELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touched, so a design review does not apply. A CEO review is optional and normally for product-direction changes; this is a backend auth refactor, so it is a soft mention only.\nStakes if we pick wrong: Low either way; the CEO review would cost time on a change with no product-facing scope decision left open.\nRecommendation: A because all required reviews are complete and the plan's remaining blocker is the author confirming the Context section, not another review.\nNote: options differ in kind, not coverage — no completeness score.\nNet: start implementing versus an optional strategy pass on a backend refactor.": "A) Ready to implement (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-10T03:29:41.500Z" + } + ], + "assistantMessages": [ + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Running the plan-eng-review preamble, then reviewing PLAN.md as the review target.", + "timestamp": "2026-09-10T03:18:33.274Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "The preamble fired a one-time onboarding directive about CLAUDE.md skill routing. Asking that first, as instructed.", + "timestamp": "2026-09-10T03:18:52.415Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "I've noted the routing rules addition, but the CLAUDE.md update and commit will wait until plan mode ends. Now I'm checking the design doc, recovering context, and reading PLAN.md along with the review-sections file.\n\n", + "timestamp": "2026-09-10T03:19:13.106Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Preflight results: no design doc, Codex outside voice is disabled in config, no learnings on file, and cross-project learnings is unset. Two setup questions before Step 0.", + "timestamp": "2026-09-10T03:20:33.830Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "I've completed step 0: the plan wraps an existing tenant-keyed cache in a redundant facade, rewrites `legacyAuthFlow()` instead of reusing it, and never states the refactor's user-facing goal. It also trips complexity thresholds (12 files, 5 new types), and my research shows the global singleton export adds nothing over constructor injection, `Promise.all` risks leaving IDP calls in flight on failure, and JWKS/discovery caching could eliminate those calls entirely — I'll carry these as findings.", + "timestamp": "2026-09-10T03:22:02.107Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Scope reduced per D4. Two new types, existing adapter injected. Committing to that; no more scope re-argument. Now Section 1, Architecture. Three issues, one question each.", + "timestamp": "2026-09-10T03:22:34.819Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Architecture section wrapped up with all three issues resolved. Moving to Code Quality—found one potential issue there, but the duplicate cache-write logic concern is already addressed by the D5 single-writer decision, so I'll skip re-raising it.", + "timestamp": "2026-09-10T03:23:35.656Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Code Quality done, one issue, complete option chosen. Section 3, Tests.\n\nThe regression rule applies with no question: `legacyAuthFlow()` is existing behavior being rewritten with zero regression coverage (PLAN.md:27-28, and PLAN.md:15-16 explicitly excludes it). Characterization tests for `legacyAuthFlow()` go into the plan as a CRITICAL requirement, written before the rewrite starts, and run against both paths under the D6 flag.\n\nCoverage map for the planned code (no test framework or source is present in this fixture repo, so every path is a GAP today):\n\n```\nCODE PATHS USER FLOWS\n[~] legacyAuthFlow() (rewritten) [+] Tenant login via new path\n └── [GAP] [CRITICAL] characterization: valid / expired ├── [GAP] [→E2E] login → mint → validated request\n / revoked / wrong-tenant / IDP down ├── [GAP] [→E2E] tenant suspended mid-session\n[+] AuthBroker (single writer, versioned write) └── [GAP] flag off → legacy path, parity\n ├── [GAP] write with current generation → stored [+] Error states\n ├── [GAP] write with stale generation → dropped + logged ├── [GAP] IDP timeout → explicit error, retryable\n └── [GAP] tenant A key never readable via tenant B ├── [GAP] malformed token → explicit reject\n[+] SessionMint (reads, requests mint) └── [GAP] policy miss → explicit reject, not pass\n ├── [GAP] cache hit → no IDP call\n ├── [GAP] cache miss → broker mint → stored\n └── [GAP] invalidation during mint → no resurrection\n[+] validateAndDispatch() pipeline\n ├── [GAP] each step happy path\n ├── [GAP] each error class → distinct outcome + log\n └── [GAP] boundary table test: unknown error → fail closed\n[+] IDP validation (parallel calls)\n ├── [GAP] all 5 succeed\n ├── [GAP] one rejects → others cancelled, typed error\n └── [GAP] one hangs → per-call timeout fires\n\nCOVERAGE: 0/19 paths tested (0%) | GAPS: 19 (1 CRITICAL regression, 2 E2E)\n```", + "timestamp": "2026-09-10T03:24:07.566Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "All four sections are done, and since codex_reviews is disabled, I'm skipping the Codex review (re-enable with `gstack-config set codex_reviews enabled`). I'll note that and check the TODO format before moving to the TODO questions.\n\n", + "timestamp": "2026-09-10T03:25:05.174Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Moving to the TODO candidates now, one question each. Since TODOS.md doesn't exist and plan mode blocks file creation, I'll record these items in the plan's TODO section and write them to TODOS.md once plan mode exits.", + "timestamp": "2026-09-10T03:25:13.361Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "All decisions are in: D4's scope was reduced, D5-D10 went with the complete remedy, and three TODOs were accepted. Now I'll generate the tasks JSONL, review log, decision log, test-plan artifact, and the reviewed plan with its report.", + "timestamp": "2026-09-10T03:26:25.388Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "Exit gate checks all pass — Eng Review is clear, both log entries match HEAD with no staleness, and the review readiness dashboard confirms CLEARED status.\n\n", + "timestamp": "2026-09-10T03:29:34.661Z" + }, + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "text": "All relevant reviews complete. Run /ship when ready. Logging telemetry and the background brain refresh, then exiting plan mode.", + "timestamp": "2026-09-10T03:29:46.388Z" + } + ], + "planReadyRequests": [ + { + "sessionId": "e5829792-573f-4de0-8cbc-97379840c953", + "toolUseId": "toolu_019TVvkeXoLv89EKxXSXZHjD", + "timestamp": "2026-09-10T03:29:49.951Z", + "failed": false + } + ] + }, + "screen": "\n GSTACK REVIEW REPORT\n\n ┌───────────┬──────────────────────────┬────────────────────┬──────┬───────────┬─────────────────────────────────┐\n │ Review │ Trigger │ Why │ Runs │ Status │ Findings │\n ├───────────┼──────────────────────────┼────────────────────┼──────┼───────────┼─────────────────────────────────┤\n │ CEO │ /plan-ceo-review │ Scope & strategy │ 0 │ — │ — │\n │ Review │ │ │ │ │ │\n ├───────────┼──────────────────────────┼────────────────────┼──────┼───────────┼─────────────────────────────────┤\n │ Outside │ codex via │ Independent 2nd │ │ │ 0 (skipped, host=claude, │\n │ Review │ /plan-eng-review │ opinion │ 1 │ disabled │ provider=codex, │\n │ │ (disabled) │ │ │ │ phase=plan-review) │\n ├───────────┼──────────────────────────┼────────────────────┼──────┼───────────┼─────────────────────────────────┤\n │ Eng │ /plan-eng-review │ Architecture & │ 1 │ clean │ 24 issues, 0 critical gaps, │\n │ Review │ │ tests (required) │ │ (PLAN) │ mode SCOPE_REDUCED │\n ├───────────┼──────────────────────────┼────────────────────┼──────┼───────────┼─────────────────────────────────┤\n │ Design │ /plan-design-review │ UI/UX gaps │ 0 │ — │ — │\n │ Review │ │ │ │ │ │\n ├───────────┼──────────────────────────┼────────────────────┼──────┼───────────┼─────────────────────────────────┤\n │ DX Review │ /plan-devex-review │ Developer │ 0 │ — │ — │\n │ │ │ experience gaps │ │ │ │\n └───────────┴──────────────────────────┴────────────────────┴──────┴───────────┴─────────────────────────────────┘\n\n OUTSIDE COVERAGE: provider codex, phase plan-review, outside_status disabled (user opt-out via\n codex_reviews=disabled); no native fallback dispatched; 0 findings. Outside coverage is absent, not clean.\n\n VERDICT: ENG CLEARED — ready to implement once Prerequisite P0 (author confirms Context) is done. Outside review\n disabled by config.\n\n NO UNRESOLVED DECISIONS\n╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌\n\n────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────\n Claude has written up a plan and is ready to execute. Would you like to proceed?\n\n ❯ 1. Yes, and use auto mode\n 2. Yes, manually approve edits\n 3. Tell Claude what to change\n shift+tab to approve with this feedback\n", + "startedAt": 1789010299798.0, + "captureAt": "2026-09-10T03:36:31.588Z" +} diff --git a/test/fixtures/plan-count-permission-ah.json b/test/fixtures/plan-count-permission-ah.json new file mode 100644 index 000000000..8c9742379 --- /dev/null +++ b/test/fixtures/plan-count-permission-ah.json @@ -0,0 +1,42 @@ +{ + "runLabel": "ship-source-ah-delta-paid-20260910-v1", + "sourceHead": "c9e7a422be47b69d855f48cfce9024457e0dd4fc", + "pid": 321153, + "startUnix": 1789009886.02, + "cwd": "/tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-plan-count-7c0HlE", + "config": "/tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-hermetic-321113-CTsKll/with-skills/.claude", + "screen": " + text + sibling Retry (aria-label \"Retry save\") | \n + | \n 79 +Export | Spinner + \"Exporting\u2026\" inside disabled | n/a (exports last saved preferences) | Same \n +error/retry area, fields untouched, Retry (aria-label \"Retry | JSON download; clears only the Export error \n + | n/a \n 80 + | button, aria-busy=true, reduced motion | | expor\n +t\") | \n + | \n 81 +Reset | Disabled while Save/Export pending | Not dirty: no-op (nothing to restore) | n/a \n + | Dialog \"Discard unsaved changes?\" \u2192 confirm \n + | n/a \n 82 + | | | \n + | restores saved values and last status text \n + | \n 83 +Cancel | Disabled while Save/Export pending | Not dirty: returns immediately | n/a \n + | Dialog \u2192 \"Discard and leave\" \u2192 previous page, \n + | n/a \n 84 + | | | \n + | focus destination main heading \n + | \n 85 +InlineStatus | unchanged during pending | Blank before first successful save | \"Unsa\n +ved changes\" retained beside error | \"Saved at HH:mm\" \n + | n/a \n 86 +Dirty tracking | n/a | Unchanged values: blank / last saved text | n/a \n + | Edit \u2192 \"Unsaved changes\"; revert \u2192 prior text \n + | n/a \n 87 +``` \n 88 + \n 89 ## Review findings (pending until each is individually approved)\n 90 \n 91 Each finding below is a documented violation of DESIGN.md in the current\n\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\u254c\n Do you want to make this edit to gstack-test-plan-design.md?\n \u276f 1. Yes\n 2. Yes, and switch to accept edits (auto-approve file edits and common file commands) for this session; Yes, and\n always allow access to /tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-e2e-plan-design-fUi1gF for this session\n 3. Nohift+tab)\n\n Esc to cancel \u00b7 Tab to amend\n", + "state": { + "cwd": "/tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-plan-count-7c0HlE", + "expected": "/tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-e2e-plan-design-fUi1gF/gstack-test-plan-design.md", + "sessionId": "72749819-b356-4fdd-af06-b075dadec9ff", + "transcriptPath": "/tmp/gstack-paid-shard-H8EbsZ/tmp/gstack-hermetic-321113-CTsKll/with-skills/.claude/projects/-tmp-gstack-paid-shard-H8EbsZ-tmp-gstack-plan-count-7c0HlE/72749819-b356-4fdd-af06-b075dadec9ff.jsonl", + "seenIds": [ + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01P5SHThBn2wpcArmnBDJFtf", + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01VWPw6KhWQz5LD1buvv87Us", + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01JhN4DTKCBkwaNhgWRo9wG7", + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01Y75biat54XcRZpusrUtWuL" + ], + "pendingId": "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01Y75biat54XcRZpusrUtWuL", + "completedId": "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01JhN4DTKCBkwaNhgWRo9wG7", + "completedIds": [ + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01P5SHThBn2wpcArmnBDJFtf", + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01VWPw6KhWQz5LD1buvv87Us", + "72749819-b356-4fdd-af06-b075dadec9ff:toolu_01JhN4DTKCBkwaNhgWRo9wG7" + ], + "timestamp": "2026-09-10T03:15:34.900Z" + }, + "transcriptStatus": "ready", + "transcriptSessions": [ + "72749819-b356-4fdd-af06-b075dadec9ff" + ], + "provenance": { + "proof": ".context/ship-source-ah-delta-paid-20260910-v1/design-permission-checkpoint-v1/proof.json", + "proofSha256": "6195102842dc224e4778ceee654ab4266053680430dfa294783413b624b0d735", + "binding": ".context/ship-source-ah-delta-paid-20260910-v1/design-permission-checkpoint-v1/binding.json", + "bindingSha256": "378ac2789131c42873536f21afa8e8948e2d63bddef26f4fcde0951e8bcb9d89", + "pendingPublished": false, + "noLaterMutationEvent": true, + "note": "Exact captured screen and current content-free native hook identity; the pending Edit body was unpublished, and no input was sent." + } +} diff --git a/test/fixtures/sdk-schedule-continuation-ah.json b/test/fixtures/sdk-schedule-continuation-ah.json new file mode 100644 index 000000000..a7517825f --- /dev/null +++ b/test/fixtures/sdk-schedule-continuation-ah.json @@ -0,0 +1,9 @@ +{ + "sourceHead": "c9e7a422be47b69d855f48cfce9024457e0dd4fc", + "sessionId": "6ce43790-8ac2-45d1-a7ef-a764737578e3", + "publicReportSha256": "4be3eeaaef4f9a8f28c09d0d86305a900902aa774bb840150f130875d70450ac", + "writeLine": 2717, + "ackLine": 2721, + "report": "# Plan: cache profile summaries in one process\n\n> Reviewed by `/plan-ceo-review` on 2026-09-10, mode HOLD SCOPE. Original plan\n> text is preserved below. Accepted amendments are marked `[A1]`..`[A5]` inline\n> and defined in the review's Decision Registry. Nothing else in scope changed.\n\n## Measured problem and accepted scope\nThe existing profile-summary service has one active process. A one-week trace\nshows repeated reads of about 900 hot keys: DB CPU is 70%, with read p95 120 ms.\nAdd a process-local LRU wrapper to the existing repository. Acceptance targets\nare at least 60% cache hits, DB CPU below 50%, and read p95 below 60 ms, with the\nexisting error-rate and correctness SLOs unchanged. This is an internal backend\nchange with no UI, API, schema, pricing, or developer onboarding change.\n\n## Existing contracts retained\n- All reads and writes use this repository in the same process; there are no\n external DB writers. Multi-process operation remains unsupported and startup\n rejects that configuration while caching is enabled.\n- Authentication and authorization run before repository access. Keys encode\n the authenticated tenant ID and validated profile ID without ambiguity.\n Values are immutable profile-summary DTOs; secrets and cache keys are never\n logged. Cached results cannot bypass authorization.\n- The existing LRU adapter supports 1000 entries, a 16 MiB byte cap, and a\n 30-second TTL. Recorded hot data fits those limits. Absent records use a\n distinct sentinel with a 10-second TTL; undefined means a cache miss.\n- Cache operations are synchronous and atomic in the single JS event loop.\n On any cache failure the existing adapter bypasses the cache until an empty\n cache is reinitialized; repository errors keep the current typed API error\n mapping. The existing per-key\n single-flight wrapper coalesces simultaneous misses and releases on failure.\n- A read already in progress when a write commits may return its earlier DB\n snapshot to that caller. Every read begun after that write completes must\n observe the committed version. TTL expiry is not a substitute for this rule.\n **(Consistency invariant, unchanged. Review finding F1 shows the original\n sketch below did not meet it; amendment [A1] repairs the sketch.)**\n\n## Proposed wrapper integration\nKeep the current read-through repository interface and shared adapters.\n\n**Original sketch (superseded by [A1]/[A2], retained for the record).** It\nstated: \"no additional version checks or coordination between a cache fill and\na write are proposed.\"\n\n```javascript\n// ORIGINAL \u2014 violates the consistency invariant (see F1 schedule S2)\nasync function readProfile(key) {\n const cached = cache.get(key);\n if (cached !== undefined) return cached;\n const value = await repository.read(key);\n cache.set(key, value);\n return value;\n}\nasync function writeProfile(key, update) {\n const saved = await repository.write(key, update);\n cache.delete(key);\n return saved;\n}\n```\n\n**Amended ordering rules [A1][A2][A4].** A fill may land only if no write for\nthat key completed after the fill's DB read began. Write completion (success or\nfailure) invalidates, in one synchronous tick: the cache entry, the in-flight\nfill slot, and the single-flight slot, so later callers start a fresh DB read.\n\n```javascript\n/*\n * Fill/write ordering (keep this diagram current with the code):\n *\n * R1 miss \u2500\u2510 await read (V0) \u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2510 resolve: slot gone \u2192 DROP fill\n * W \u2502 await write \u2500\u2500 commit V1 \u2500\u2500 delete entry + forget slot (sync)\n * R2 \u2500\u2500\u2500\u2500\u2500\u2500\u253c\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\u2534\u2500 begins after W: miss \u2192 fresh read \u2192 V1 \u2713\n */\nasync function readProfile(key) {\n if (!flag.enabledFor(key)) return repository.read(key); // [A4] deterministic per-key bucket\n const cached = cache.get(key);\n if (cached !== undefined) return cached; // includes ABSENT sentinel\n return inflight.run(key, async (slot) => { // existing single-flight, now slot-aware\n const value = await repository.read(key); // may resolve after a commit\n if (inflight.current(key) === slot) cache.set(key, value); // else stale fill dropped, metric++\n else metrics.increment('profile_cache.stale_fill_dropped');\n return value;\n });\n}\n\nasync function writeProfile(key, update) {\n if (!flag.enabledFor(key)) return repository.write(key, update);\n try {\n return await repository.write(key, update);\n } finally { // [A2] success, typed failure, or indeterminate\n cache.delete(key); // same tick: entry,\n inflight.forget(key); // fill slot + single-flight slot\n }\n}\n```\n\n`inflight.forget(key)` is a new method on the existing single-flight wrapper.\nIt drops the current slot so joiners arriving after the write start a fresh\nread; callers already joined keep the in-progress promise (they began before\nthe write completed, which the invariant permits).\n\n## Verification and rollout\nExisting repository contract tests cover tenant isolation, key validation,\nabsence, DB failures, and authorization. New wrapper tests cover hit/miss,\neviction and byte limits, TTL, adapter-failure fallback, successful-write\ninvalidation, ~~failed-write preservation~~ **failed-write invalidation [A2]**,\nand concurrent-miss coalescing.\n\nAdded by review (each is a required assertion, see Section 6):\n- **[A1] Stale-fill ordering tests** with controlled pause/release points for\n schedules S2, S3 and S4 in the Async Ordering record.\n- **[A2] Indeterminate write test**: write rejects after the DB has committed;\n the next read returns the committed value.\n- **[A4] Flag transition tests**: read/write bucket agreement per key; any\n flag change clears entries and in-flight slots; flag evaluation error\n resolves to bypass.\n- **[A5] Clock injection**: TTL and sentinel-TTL tests use an injected clock.\n\nThe rollout uses the existing runtime feature flag: enable for 10% of keys,\nthen 50%, then all keys after one healthy hour at each stage. Bucketing is a\ndeterministic hash of the key, evaluated identically in read and write [A4].\nMonitor hit/miss, eviction, cache bytes, fallback errors, DB CPU, and read p95\nwithout raw IDs. **[A3]** Also emit `stale_fill_dropped`, `set_rejected_oversize`,\n`flag_eval_error`, and the gauge `cache_bypass_active`; alert when bypass is\nactive for more than 5 minutes or hit rate is below 60% for 15 minutes after\nthe 100% stage. Bypass entry logs at WARN with the adapter error class only.\nOn error-rate or latency regression, disable the flag immediately; both reads\nand writes bypass the cache while disabled, and enabling creates an empty cache.\n**[A4]** Any change to the flag value or bucket percentage reinitializes an empty\ncache and clears in-flight slots; a flag evaluation failure resolves to bypass.\nCold starts remain within the existing DB capacity. The service owner monitors\nthe rollout and records the results against the acceptance targets.\n\n## Out of scope\nDistributed caching, cross-process coherence, prewarming, changing consistency\nsemantics, or adding new product surfaces. The repository interface preserves a\nfuture replacement path without introducing a general cache framework now.\n\n---\n\n## CEO Review Record (HOLD SCOPE)\n\nRun conditions: automated execution, no human present. System audit, preamble,\ntelemetry, codebase exploration, learnings search, and brain context were\nskipped per run instructions (no shell available). Every decision point was\nauto-resolved to the skill's recommended option and is recorded below.\nNo repository code was read or changed.\n\n### Step 0\n\n**0A Premise.** Problem is measured, not hypothetical: one week of traces, ~900\nhot keys, DB CPU 70%, p95 120 ms. Doing nothing means the DB saturates with\ngrowth. Acceptance targets measure the real outcome (DB CPU, p95), not a proxy.\nRight problem, most direct path. No change.\n\n**0B Existing code leverage.** Sub-problem to existing code map:\n\n| Sub-problem | Existing | Reused? |\n|---|---|---|\n| Bounded storage, TTL, byte cap | LRU adapter | Yes |\n| Miss stampede | Per-key single-flight wrapper | Yes, extended with `forget` [A1] |\n| Gradual rollout / kill switch | Runtime feature flag | Yes |\n| Error surface | Typed API error mapping | Yes |\n| Tenant isolation, authz | Contract tests, pre-repo authz | Yes |\n\nNothing is rebuilt.\n\n**0C Dream state.**\n```\n CURRENT STATE THIS PLAN 12-MONTH IDEAL\n every read hits DB ---> read-through LRU in-process, ---> same interface, adapter\n DB CPU 70%, p95 120ms invariant-safe fills, swappable to shared cache\n flagged rollout if multi-process arrives\n```\nPlan moves toward the ideal; the retained repository interface is the seam.\n\n**0C-bis Implementation alternatives (D1).**\n\n| | A: Minimal (original sketch) | B: Sketch + fill/slot invalidation | C: Row-version check on fill |\n|---|---|---|---|\n| Effort | S | S | M |\n| Risk | High: violates invariant (F1) | Low | Med |\n| Completeness | 4/10 | 10/10 | 10/10 but needs schema version column |\n| Reuses | adapter, flag, single-flight | same + single-flight extension | same + DB change |\n\nD1 auto-decided: **B**. C is eliminated by the plan's \"no schema change\"\nconstraint. A fails the stated invariant.\n\n**0F Mode (D2).** HOLD SCOPE, set by the user for this run. Not auto-decided.\n\n**0D HOLD analysis.** Files touched: wrapper, single-flight adapter (one\nmethod), flag helper, tests. Under the 8-file smell threshold. Minimum set is\nthe wrapper plus the repairs required to meet the retained invariant; nothing\ndeferrable without breaking a stated requirement.\n\n**0E Temporal interrogation.** Resolved now: where the flag check lives (both\nentry points, per key); fill/write ordering (F1); slot invalidation hook (F1);\nindeterminate write handling (F2); flag transition semantics (F5); test clock\n(F8). Human ~6h / CC ~40 min for the full amended scope.\n\n### Decision Registry (all auto-resolved to recommended option)\n\n| ID | Decision | Options | Chosen | Why |\n|---|---|---|---|---|\n| D1 | Approach | A 4/10, B 10/10, C 10/10 blocked | B | Meets invariant, no schema change |\n| D2 | Mode | 4 modes | HOLD SCOPE | User-specified |\n| D3 | F1 remedy | A drop-stale-fill + slot forget 10/10; B per-key read lock 8/10; C do nothing 3/10 | A \u2192 [A1] | Explicit, bounded state, no added latency |\n| D4 | F2 remedy | A invalidate in `finally` 10/10; B allowlist pre-commit errors 9/10; C keep preservation 5/10 | A \u2192 [A2] | Smallest diff, no error taxonomy coupling |\n| D5 | F4 remedy | A metrics + alerts + WARN log 10/10; B metrics only 6/10 | A \u2192 [A3] | Zero silent failures |\n| D6 | F5 remedy | A clear on any flag change + bypass on eval error 10/10; B document only 4/10 | A \u2192 [A4] | Edge cases over documentation |\n| D7 | F8 remedy | A injected clock 10/10; B real sleeps 5/10 | A \u2192 [A5] | Flakiness is a test defect |\n| D8 | TODOS.md | none proposed | 0 items | All evidenced gaps fixed in scope |\n| D9 | Next review | A eng review; C manual | A recommended | Required gate; not runnable here |\n\nLake Score: 7/7 recommendations chose the complete option.\n\n### Findings Registry\n\n| ID | Sev | Section | Evidence | Remedy | Residual risk | Verification |\n|---|---|---|---|---|---|---|\n| F1 | CRITICAL GAP | 1, 2, 4, 5, 6 | Original sketch: fill after `await repository.read` has no guard; plan text says no fill/write coordination. Schedule S2 makes a post-write reader see V0 for 30 s; S3 does the same via single-flight joiners; S4 caches ABSENT after a create. Violates retained invariant. | [A1] drop fills whose slot was forgotten; write forgets slot + entry in one tick | Lost fill on race costs one extra DB read | Pause/release tests for S2, S3, S4; `stale_fill_dropped` metric |\n| F2 | WARNING | 2 | \"failed-write preservation\" keeps entry when `repository.write` rejects; a timeout after DB commit leaves V0 cached up to 30 s. Plan does not state failures are pre-commit. | [A2] invalidate in `finally` on every outcome | Failed writes evict one entry | Test: write rejects post-commit, next read returns V1 |\n| F3 | OK (noted) | 3 | Sentinel thrash: probing absent IDs fills 1000-entry LRU with sentinels; worst case is today's baseline, bounded by LRU size | None; hit-rate alert [A3] observes it | Degradation to baseline only | Hit-rate alert |\n| F4 | WARNING | 2, 8 | \"Any cache failure\" bypass is a catch-all with no named alert or reinit trigger; \"fallback errors\" monitored but no threshold | [A3] gauge + alerts + WARN log with error class | Bypass reinit policy stays adapter-owned | Alert fires in staging fault-injection |\n| F5 | WARNING | 9 | Rollout by % of keys; plan says enable clears cache but is silent on % changes and flag evaluation failure | [A4] deterministic per-key bucket, clear on any change, eval error \u2192 bypass | None material | Flag transition tests |\n| F6 | OK | 3 | Authz precedes repository; keys tenant-scoped; DTOs immutable; no keys logged | None | \u2014 | Existing contract tests |\n| F7 | OK | 7 | 1000 \u00d7 DTO within 16 MiB; O(1) sync ops; cold start within DB capacity; single-flight caps stampede | None | 100x load is a multi-process problem, out of scope | Rollout metrics |\n| F8 | WARNING | 6 | TTL and sentinel-TTL tests depend on wall clock | [A5] injected clock | None | Tests deterministic |\n\n### Async Ordering Record (Section 4)\n\nShared state: `cache[key]`, `inflight[key]`. Invariant boundary: a read that\n*begins* after `writeProfile` resolves must return the committed version.\n\n```\n Sched | R1 (miss, reads V0) | W (commits V1) | R2 (begins after W) | cache[key] | Result\n ------|----------------------------|-----------------------------|---------------------|------------|-------\n S1 | read\u2192set V0 | write\u2192delete | miss\u2192read V1\u2192set | V1 | OK\n S2* | await read ... | write commits, delete(noop) | | - |\n | resolves V0 \u2192 set V0 | | hit \u2192 V0 | V0 (30 s) | VIOLATION\n S2 A1 | resolves V0, slot gone\u2192drop| delete + forget (one tick) | miss\u2192read V1\u2192set | V1 | OK\n S3* | await read ... | write, delete | joins R1 \u2192 V0 | - | VIOLATION\n S3 A1 | await read ... | delete + forget | new slot\u2192read V1 | V1 | OK\n S4* | miss on absent \u2192 await | create commits | hit ABSENT (10 s) | ABSENT | VIOLATION\n S4 A1 | slot gone \u2192 drop ABSENT | delete + forget | miss\u2192read V1 | V1 | OK\n S5 | begins before W, joins | delete + forget | \u2014 | \u2014 | OK: R1 began before W completed (permitted clause)\n```\n`*` = original sketch. Mechanism preventing S2\u2013S4 after [A1]: the forget and\nthe slot comparison run in synchronous ticks; no await separates `delete` from\n`forget`, and no await separates the slot check from `cache.set`.\n\nShadow paths for the new flow (nil key, empty key, upstream error): nil/empty\nkeys are rejected by existing key validation before the cache; upstream read\nerror releases the slot and maps to the typed API error with no fill; upstream\nwrite error invalidates per [A2]. All three covered by existing contract tests\nplus [A2].\n\nNo user-visible interactions: interaction edge-case table not applicable.\n\n### Section Outcomes\n\n| # | Section | Outcome |\n|---|---|---|\n| 1 | Architecture | 1 issue (F1). Coupling added: wrapper \u2192 single-flight `forget`. Justified. SPOF unchanged (one process, DB). Rollback: flag off, seconds. Diagram below. |\n| 2 | Error & Rescue | 9 paths mapped, 2 GAPS (F1 stale fill, F2 indeterminate write), both remedied. F4 catch-all named and instrumented. Registry below. |\n| 3 | Security | 0 issues High. F3 and F6 evaluated, no change. No new endpoints, params, secrets, or dependencies. |\n| 4 | Data flow / ordering | 5 schedules mapped, 3 violating (S2\u2013S4), 0 unhandled after [A1]. |\n| 5 | Code quality | 1 issue (F1 sketch under-engineered). Sketch amended. Both functions branch \u22643 times. No DRY violation. |\n| 6 | Tests | Diagram below. 4 gaps (F1 ordering, F2 indeterminate, F5 flag transitions, F8 clock), all amended. Test pyramid: unit-heavy, correct. |\n| 7 | Performance | No issues (F7). Slowest new path: miss + fill, p99 \u2248 existing DB read. |\n| 8 | Observability | 1 gap (F4), remedied by [A3]. Runbook: bypass alert \u2192 inspect WARN error class \u2192 reinit or disable flag. |\n| 9 | Deployment | 1 risk (F5), remedied by [A4]. No migration. Old/new code overlap: none (single process). Post-deploy: hit rate rising, bypass gauge 0, p95 falling within 5 min. |\n| 10 | Trajectory | Reversibility 5/5. Debt items: 0 (ordering diagram lives in code comment, maintained with the change). Obvious to a new engineer in 12 months given the comment. |\n| 11 | Design & UX | SKIPPED, justified: no UI scope; plan states no UI, API, or onboarding change. |\n\n### Error & Rescue Registry\n\n| Codepath | What goes wrong | Exception | Rescued | Action | User sees |\n|---|---|---|---|---|---|\n| readProfile / cache.get,set | adapter internal failure | adapter error (named in log) | Y (adapter) | bypass mode, WARN, gauge=1 [A3] | Nothing (DB serves) |\n| readProfile / repository.read | DB timeout, connection | TimeoutError, ConnectionError | Y (existing) | slot released, typed API error | Existing error |\n| readProfile / repository.read | record absent | none (ABSENT sentinel) | n/a | cache sentinel 10 s | Existing not-found |\n| readProfile / fill | write completed during read | none | Y [A1] | drop fill, metric | Nothing |\n| readProfile / cache.set | DTO > 16 MiB | adapter reject | Y (adapter) | no fill, `set_rejected_oversize` [A3] | Nothing |\n| writeProfile / repository.write | validation, authz | ValidationError, AuthorizationError | Y (existing) | invalidate [A2], typed error | Existing error |\n| writeProfile / repository.write | timeout after commit | TimeoutError | Y [A2] | invalidate in `finally` | Existing error |\n| writeProfile / cache.delete | adapter failure | adapter error | Y (adapter) | bypass, WARN [A3] | Nothing |\n| flag.enabledFor | flag store unavailable | flag client error | Y [A4] | bypass, `flag_eval_error` | Nothing |\n| startup | multi-process + cache on | ConfigurationError | Y (existing) | refuse start, log | Deploy fails loudly |\n\n### Failure Modes Registry\n\n```\n CODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED?\n --------------------|---------------------------|----------|-----------|-----------------|--------\n read fill | stale fill after write | Y [A1] | Y [A1] | Nothing | metric\n read via joiner | joins pre-write read | Y [A1] | Y [A1] | Nothing | metric\n read fill | ABSENT cached after create| Y [A1] | Y [A1] | Nothing | metric\n write | indeterminate outcome | Y [A2] | Y [A2] | Existing error | existing\n write | typed pre-commit failure | Y | Y (exist) | Existing error | existing\n cache adapter | internal failure \u2192 bypass | Y | Y (exist) | Nothing | WARN [A3]\n cache adapter | bypass persists silently | Y [A3] | staging | Nothing | alert\n cache set | oversize DTO | Y | Y (exist) | Nothing | metric [A3]\n flag | eval error | Y [A4] | Y [A4] | Nothing | metric\n flag | % change leaves stale | Y [A4] | Y [A4] | Nothing | n/a\n startup | multi-process config | Y | Y (exist) | Deploy fails | Y\n```\nCRITICAL GAPS before review: 1 (F1). Open after accepted remedies: 0.\n\n### Test Diagram (Section 6)\n\n```\n NEW UX FLOWS: none\n NEW DATA FLOWS: read-through fill; write-through invalidate; flag gate\n NEW CODEPATHS: hit; miss+fill; miss+dropped fill; joiner; forget; finally-invalidate;\n flag off; flag eval error; flag change clear\n NEW ASYNC WORK: none (in-loop promises only)\n NEW INTEGRATIONS: none\n NEW ERROR/RESCUE PATHS: see Error & Rescue Registry\n```\nAssertions (unit unless noted): hit returns cached DTO and skips DB (rejects a\nDB call count of 1); miss fills once (rejects 2 DB calls under N concurrent\nmisses); S2/S3/S4 with paused read return V1 to the post-write reader (rejects\nV0 or ABSENT); post-commit write rejection leaves no entry (rejects a hit);\nadapter throw yields DB value and gauge=1 (rejects a thrown error); eviction at\n1001 entries and at 16 MiB + 1 byte; TTL 30 s and sentinel 10 s via injected\nclock; flag off skips both cache calls; flag change empties cache and slots.\n2am test: S2 under load harness. Hostile QA: S3 joiner after write. Chaos:\nadapter throws mid-fill while a write commits. Load test: 900-key replay at\n10x, assert hit rate \u2265 60% and no DB call amplification.\n\n### Diagrams\n\n**1. System architecture**\n```\n caller \u2500\u25b6 authn/authz \u2500\u25b6 readProfile/writeProfile (wrapper, NEW)\n \u2502 \u2502 \u2502\n \u25bc \u25bc \u25bc\n flag.enabledFor LRU adapter single-flight (+forget, NEW)\n \u2502 \u2502\n \u2514\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2518\n \u25bc\n repository \u2500\u25b6 DB\n```\n\n**2. Data flow with shadow paths**\n```\n key \u2500\u25b6 validate \u2500\u25b6 flag? \u2500\u25b6 cache.get \u2500\u25b6 [miss] slot \u2500\u25b6 repository.read \u2500\u25b6 slot check \u2500\u25b6 set \u2500\u25b6 DTO\n \u2502 \u2502 \u2502 \u2502 \u2502 \u2502\n nil/empty reject error\u2192bypass hit\u2192return error\u2192release, typed forgotten\u2192drop\n```\n\n**3. State machines**\n```\n entry: (none) \u2500\u2500miss\u2500\u2500\u25b6 FILLING \u2500\u2500resolve & slot ok\u2500\u2500\u25b6 PRESENT|ABSENT \u2500\u2500TTL/evict/write\u2500\u2500\u25b6 (none)\n \u2514\u2500write forget\u2500\u25b6 (none) [set after forget: impossible, slot check]\n adapter: ACTIVE \u2500\u2500any adapter error\u2500\u2500\u25b6 BYPASS \u2500\u2500reinit empty\u2500\u2500\u25b6 ACTIVE\n flag: OFF \u2500\u2500enable\u2500\u2500\u25b6 ON(p%) \u2500\u2500change p\u2500\u2500\u25b6 ON(p'%) (cache cleared) \u2500\u2500disable\u2500\u2500\u25b6 OFF\n```\n\n**4. Error flow**\n```\n adapter error \u2500\u25b6 BYPASS + WARN + gauge \u2500\u25b6 alert >5 min \u2500\u25b6 operator: reinit or flag off\n repo error \u2500\u25b6 release slot \u2500\u25b6 typed API error (unchanged) \u2500\u25b6 write: finally invalidate\n```\n\n**5. Deployment sequence**\n```\n deploy (flag OFF) \u2500\u25b6 10% keys \u25001h healthy\u2500\u25b6 50% \u25001h healthy\u2500\u25b6 100% \u2500\u25b6 record vs targets\n each step: watch hit rate, bypass gauge, stale_fill_dropped, DB CPU, p95\n```\n\n**6. Rollback flowchart**\n```\n regression? \u2500yes\u2500\u25b6 flag OFF (reads+writes bypass, seconds) \u2500\u25b6 still bad? \u2500yes\u2500\u25b6 git revert deploy\n \u2502no \u2502no\n \u25bc \u25bc\n continue ramp investigate WARN error class\n```\n\n### Required Prose Sections\n\n**NOT in scope** (all pre-existing, restated with rationale):\n- Distributed or cross-process cache: single process today; interface seam retained.\n- Prewarming: cold start fits DB capacity per trace.\n- Consistency semantics change: invariant retained as written.\n- Row-version fill check (Approach C): needs schema change, prohibited.\n\n**What already exists.** LRU adapter, single-flight wrapper, runtime flag,\ntyped error mapping, contract tests. All reused; single-flight gains one method.\n\n**Dream state delta.** After this plan the service has a correct, observable,\nflag-controlled local cache behind the unchanged repository interface. The\nremaining gap to the 12-month ideal is only the adapter swap if multi-process\never arrives, which the interface already permits.\n\n**TODOS.md updates.** 0 items. Every evidenced gap was repaired in scope (D8).\n\n**Stale diagram audit.** No existing ASCII diagrams in the plan or named\nfiles. One new diagram is added as a code comment with [A1] and must be kept\ncurrent with the wrapper.\n\n**Outside voice.** `codex_reviews: disabled` in the isolated config. The extra\noutside-review step was skipped entirely, including its native fallback.\nOutside coverage: disabled. The guarded review-log persistence for the disabled\nrecord was not run (no shell in this run).\n\n## Implementation Tasks\nSynthesized from this review's findings. Each task derives from a specific\nfinding above. Run with Claude Code or Codex; checkbox as you ship.\n\n- [ ] **T1 (P1, human: ~2h / CC: ~15min)** \u2014 wrapper + single-flight \u2014 Implement slot-aware fill drop and `forget` on write completion [A1]\n - Surfaced by: Section 4 \u2014 F1, schedules S2\u2013S4\n - Files: profile wrapper, single-flight adapter, ordering diagram comment\n - Verify: pause/release tests for S2, S3, S4 pass; `stale_fill_dropped` increments in S2\n- [ ] **T2 (P1, human: ~30min / CC: ~5min)** \u2014 wrapper \u2014 Invalidate in `finally` on every write outcome [A2]\n - Surfaced by: Section 2 \u2014 F2 indeterminate write\n - Files: profile wrapper, tests (replace failed-write preservation test)\n - Verify: post-commit rejection test returns V1 on next read\n- [ ] **T3 (P1, human: ~1h / CC: ~10min)** \u2014 flag helper \u2014 Deterministic per-key bucket, clear on any flag change, bypass on eval error [A4]\n - Surfaced by: Section 9 \u2014 F5\n - Files: flag helper, wrapper, tests\n - Verify: flag transition tests; `flag_eval_error` metric on injected failure\n- [ ] **T4 (P2, human: ~1h / CC: ~10min)** \u2014 observability \u2014 Metrics, gauge, WARN log with error class, two alerts [A3]\n - Surfaced by: Section 8 \u2014 F4\n - Files: wrapper, adapter bypass hook, alert config\n - Verify: staging fault injection fires bypass alert within 5 min\n- [ ] **T5 (P2, human: ~30min / CC: ~5min)** \u2014 tests \u2014 Injected clock for TTL and sentinel-TTL tests [A5]\n - Surfaced by: Section 6 \u2014 F8\n - Files: wrapper tests\n - Verify: tests pass with no sleeps; run 20x without flake\n- [ ] **T6 (P2, human: ~1h / CC: ~10min)** \u2014 tests \u2014 Load replay of 900 hot keys at 10x asserting hit rate \u2265 60% and no DB amplification\n - Surfaced by: Section 6 \u2014 test ambition check\n - Files: load test harness\n - Verify: harness report against acceptance targets\n\n_No new tasks from Section 3 (Security), Section 7 (Performance), Section 10 (Trajectory), Section 11 (Design, skipped)._\n\nJSONL task artifact: not written (no shell/jq in this run). Re-run the JSONL\nstep from the review sections when a shell is available.\n\n### Completion Summary\n```\n +====================================================================+\n | MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n +====================================================================+\n | Mode selected | HOLD SCOPE (user-specified) |\n | System Audit | SKIPPED per run instructions |\n | Step 0 | Approach B; premise holds; 0 scope changes |\n | Section 1 (Arch) | 1 issue found (F1) |\n | Section 2 (Errors) | 9 error paths mapped, 2 GAPS (remedied) |\n | Section 3 (Security)| 0 issues found, 0 High severity |\n | Section 4 (Data/UX) | 5 schedules mapped, 0 unhandled after A1 |\n | Section 5 (Quality) | 1 issue found (F1 sketch) |\n | Section 6 (Tests) | Diagram produced, 4 gaps (all amended) |\n | Section 7 (Perf) | 0 issues found |\n | Section 8 (Observ) | 1 gap found (F4, remedied) |\n | Section 9 (Deploy) | 1 risk flagged (F5, remedied) |\n | Section 10 (Future) | Reversibility: 5/5, debt items: 0 |\n | Section 11 (Design) | SKIPPED (no UI scope) |\n +--------------------------------------------------------------------+\n | NOT in scope | written (4 items) |\n | What already exists | written |\n | Dream state delta | written |\n | Error/rescue registry| 10 rows, 0 CRITICAL GAPS open |\n | Failure modes | 11 total, 1 CRITICAL GAP found, 0 open |\n | TODOS.md updates | 0 items proposed |\n | Scope proposals | 0 proposed, 0 accepted (HOLD) |\n | CEO plan | skipped (HOLD) |\n | Outside voice | codex: disabled (config), no fallback |\n | Lake Score | 7/7 recommendations chose complete option |\n | Diagrams produced | 7 (arch, data flow, state, error, deploy, |\n | | rollback, async schedule) |\n | Stale diagrams found | 0 |\n | Unresolved decisions | 0 |\n +====================================================================+\n```\n\nSkipped persistence (no shell in this run): `gstack-review-log`,\n`gstack-decision-log`, `gstack-review-read`, telemetry, learnings log,\nhandoff cleanup. No durable learnings this session beyond the plan itself.\n\nNext review: run `/plan-eng-review` (required gate) against this amended plan.\nDesign review not applicable.\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | clean (HOLD_SCOPE) | mode: HOLD_SCOPE, 1 critical gap found and remedied, 0 open; 5 amendments A1\u2013A5 |\n| Outside Review | codex via `/plan-ceo-review` (codex_reviews: disabled) | Independent 2nd opinion | 0 | disabled | \u2014 |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | \u2014 |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | \u2014 |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | \u2014 |\n\n- **OUTSIDE COVERAGE:** provider codex, phase plan-review, status disabled by `codex_reviews: disabled`; no outside CLI invoked, no native fallback dispatched; disabled record not persisted (no shell this run).\n- **VERDICT:** CEO CLEARED (HOLD SCOPE, amendments A1\u2013A5 accepted) \u2014 eng review required\n\nNO UNRESOLVED DECISIONS\n", + "compact": "### Findings Registry\n\n| ID | Sev | Section | Evidence | Remedy | Residual risk | Verification |\n|---|---|---|---|---|---|---|\n| F1 | CRITICAL GAP | 1, 2, 4, 5, 6 | Original sketch: fill after `await repository.read` has no guard; plan text says no fill/write coordination. Schedule S2 makes a post-write reader see V0 for 30 s; S3 does the same via single-flight joiners; S4 caches ABSENT after a create. Violates retained invariant. | [A1] drop fills whose slot was forgotten; write forgets slot + entry in one tick | Lost fill on race costs one extra DB read | Pause/release tests for S2, S3, S4; `stale_fill_dropped` metric |\n| F2 | WARNING | 2 | \"failed-write preservation\" keeps entry when `repository.write` rejects; a timeout after DB commit leaves V0 cached up to 30 s. Plan does not state failures are pre-commit. | [A2] invalidate in `finally` on every outcome | Failed writes evict one entry | Test: write rejects post-commit, next read returns V1 |\n| F3 | OK (noted) | 3 | Sentinel thrash: probing absent IDs fills 1000-entry LRU with sentinels; worst case is today's baseline, bounded by LRU size | None; hit-rate alert [A3] observes it | Degradation to baseline only | Hit-rate alert |\n| F4 | WARNING | 2, 8 | \"Any cache failure\" bypass is a catch-all with no named alert or reinit trigger; \"fallback errors\" monitored but no threshold | [A3] gauge + alerts + WARN log with error class | Bypass reinit policy stays adapter-owned | Alert fires in staging fault-injection |\n| F5 | WARNING | 9 | Rollout by % of keys; plan says enable clears cache but is silent on % changes and flag evaluation failure | [A4] deterministic per-key bucket, clear on any change, eval error \u2192 bypass | None material | Flag transition tests |\n| F6 | OK | 3 | Authz precedes repository; keys tenant-scoped; DTOs immutable; no keys logged | None | \u2014 | Existing contract tests |\n| F7 | OK | 7 | 1000 \u00d7 DTO within 16 MiB; O(1) sync ops; cold start within DB capacity; single-flight caps stampede | None | 100x load is a multi-process problem, out of scope | Rollout metrics |\n| F8 | WARNING | 6 | TTL and sentinel-TTL tests depend on wall clock | [A5] injected clock | None | Tests deterministic |\n\n### Async Ordering Record (Section 4)\n\nShared state: `cache[key]`, `inflight[key]`. Invariant boundary: a read that\n*begins* after `writeProfile` resolves must return the committed version.\n\n```\n Sched | R1 (miss, reads V0) | W (commits V1) | R2 (begins after W) | cache[key] | Result\n ------|----------------------------|-----------------------------|---------------------|------------|-------\n S1 | read\u2192set V0 | write\u2192delete | miss\u2192read V1\u2192set | V1 | OK\n S2* | await read ... | write commits, delete(noop) | | - |\n | resolves V0 \u2192 set V0 | | hit \u2192 V0 | V0 (30 s) | VIOLATION\n S2 A1 | resolves V0, slot gone\u2192drop| delete + forget (one tick) | miss\u2192read V1\u2192set | V1 | OK\n S3* | await read ... | write, delete | joins R1 \u2192 V0 | - | VIOLATION\n S3 A1 | await read ... | delete + forget | new slot\u2192read V1 | V1 | OK\n S4* | miss on absent \u2192 await | create commits | hit ABSENT (10 s) | ABSENT | VIOLATION\n S4 A1 | slot gone \u2192 drop ABSENT | delete + forget | miss\u2192read V1 | V1 | OK\n S5 | begins before W, joins | delete + forget | \u2014 | \u2014 | OK: R1 began before W completed (permitted clause)\n```\n`*` = original sketch. Mechanism preventing S2\u2013S4 after [A1]: the forget and\nthe slot comparison run in synchronous ticks; no await separates `delete` from\n`forget`, and no await separates the slot check from `cache.set`.\n\n" +} diff --git a/test/helpers/ceo-section-loading-fixture.ts b/test/helpers/ceo-section-loading-fixture.ts index 7cd55308f..9c7870a16 100644 --- a/test/helpers/ceo-section-loading-fixture.ts +++ b/test/helpers/ceo-section-loading-fixture.ts @@ -224,6 +224,34 @@ function originalSketchStaleFillTrace(text: string): { assessment: string; summa }; } +/** Two adjacent original-schedule rows keep each operation in its named column. */ +function continuationStaleFillTrace(text: string, schedule: string, old: string, ttl: string): { key: string; writer: string } | undefined { + const lines = text.split('\n').map(line => line.trim().replace(/\s+/g, ' ').replace(/→/g, '->')).filter(Boolean); + const cells = (line: string) => line.split('|').map(cell => cell.trim()); + const header = cells(lines[0] ?? ''); + if (header.length !== 6 || header[0] !== 'Sched' || header[5] !== 'Result') return; + const first = /^(R[1-9]\d*) \(miss, reads ([A-Za-z_$][\w$]*)\)$/.exec(header[1]!); + const writer = /^(W[1-9]\d*|W) \(commits ([A-Za-z_$][\w$]*)\)$/.exec(header[2]!); + const later = /^(R[1-9]\d*) \(begins after (W[1-9]\d*|W)\)$/.exec(header[3]!); + const cache = /^cache\[([A-Za-z_$][\w$]*)\]$/.exec(header[4]!); + if (!first || !writer || !later || !cache || first[1] === later[1] || writer[1] !== later[2] + || first[2] !== old || writer[2] === old) return; + const rows = lines.slice(2).map(cells); + if (cells(lines[1] ?? '').length !== 6 || !/^[-| ]+$/.test(lines[1] ?? '') || rows.some(row => row.length !== 6)) return; + const matches = rows.flatMap((row, i) => row[0] === `${schedule}*` ? [i] : []); + if (matches.length !== 1 || rows.some(row => row[0] === schedule)) return; + const i = matches[0]!; + const expected = [ + [`${schedule}*`, 'await read ...', 'write commits, delete(noop)', '', '-', ''], + ['', `resolves ${old} -> set ${old}`, '', `hit -> ${old}`, `${old} (${ttl} s)`, 'VIOLATION'], + ]; + if (expected.some((row, n) => row.some((cell, c) => rows[i + n]?.[c] !== cell))) return; + // A third unlabeled row would still belong to this schedule and could + // contradict the claimed late fill or later cache hit. + if (rows[i + 2]?.[0] === '') return; + return { key: cache[1]!, writer: writer[1]! }; +} + /** A named finding may put its ordering evidence in a trace, not one paragraph. */ function hasStructuredStaleFillFinding(report: string): boolean { const lines = report.split('\n'); @@ -246,6 +274,55 @@ function hasStructuredStaleFillFinding(report: string): boolean { // closing delimiter. They cannot supply an asserted prose violation. if (!fence && !/^\s*>/.test(line) && !/^(?: {4}|\t)/.test(line)) prose[i] = line; } + for (const trace of traces) { + let heading = trace.start - 1; + while (heading >= 0 && !/^#{1,6}\s/.test(prose[heading]!)) heading--; + const section = /^#{1,6} Async Ordering Record \(Section ([1-9]\d*)\)$/.exec(prose[heading] ?? ''); + if (!section) continue; + const framed = (prefix: string) => !prefix || /^(?:\||#{1,6}\s)/.test(prefix); + if (!framed(prose.slice(0, heading).filter(line => line.trim()).at(-1) ?? '')) continue; + const ownership = prose.slice(heading + 1, trace.start).join(' ').replace(/\s+/g, ' ').trim(); + const shared = /^Shared state: `cache\[([A-Za-z_$][\w$]*)\]`, `inflight\[([A-Za-z_$][\w$]*)\]`\. Invariant boundary: a read that \*begins\* after `writeProfile` resolves must return the committed version\.$/.exec(ownership); + if (!shared || shared[1] !== shared[2]) continue; + const findings = prose.slice(0, heading).map((line, index) => ({ index, cells: line.split('|').map(cell => cell.trim()) })) + .filter(({ cells }) => cells[0] === '' && /^F[1-9]\d*$/.test(cells[1] ?? '') && cells[2] === 'CRITICAL GAP'); + for (const finding of findings) { + let registryHeading = finding.index - 1; + while (registryHeading >= 0 && !/^#{1,6}\s/.test(prose[registryHeading]!)) registryHeading--; + const prefix = prose.slice(0, registryHeading).filter(line => line.trim()).at(-1) ?? ''; + const previousSection = prose.slice(0, registryHeading).filter(line => /^#{1,6}\s/.test(line)).at(-1) ?? ''; + // A completed decision registry's score closes that earlier section; + // an unheaded example/hypothesis cannot introduce the current finding. + const closesDecisions = /^Lake Score: [0-9]+\/[0-9]+ recommendations chose the complete option\.$/.test(prefix) + && /^#{1,6} Decision Registry(?: \(all auto-resolved to recommended option\))?$/.test(previousSection); + if (!/^#{1,6} Findings Registry$/.test(prose[registryHeading] ?? '') + || (!framed(prefix) && !closesDecisions) + || prose.slice(registryHeading + 1, finding.index).some(line => line.trim() && !/^\|/.test(line))) continue; + if (!finding.cells[3]?.split(/,\s*/).includes(section[1]!)) continue; + if (!framed(prose.slice(0, finding.index).filter(line => line.trim()).at(-1) ?? '')) continue; + const evidence = (finding.cells[4] ?? '').replace(/`/g, ''); + const citation = /^Original sketch: fill after await repository\.read has no guard; plan text says no fill\/write coordination\. Schedule (S[1-9]\d*) makes a post-write reader see ([A-Za-z_$][\w$]*) for ([1-9]\d*) s[;.]/.exec(evidence); + if (!citation || !/(?:^|[.;]\s+)Violates retained invariant\.$/.test(evidence)) continue; + if (findings.filter(other => other.cells[1] === finding.cells[1]).length !== 1) continue; + const ordered = continuationStaleFillTrace(trace.text, citation[1]!, citation[2]!, citation[3]!); + if (!ordered || ordered.key !== shared[1]) continue; + const assessment: string[] = []; + for (let i = trace.end + 1; i < prose.length; i++) { + const named = /^([FSDA][1-9]\d*)\b/.exec(prose[i]!); + if (/^(?:#{1,6}\s|\|)/.test(prose[i]!) || (named && named[1] !== citation[1] && named[1] !== finding.cells[1])) break; + assessment.push(prose[i]!); + } + const tail = assessment.join(' ').replace(/\s+/g, ' ').trim(); + if (!/^`\*` = original sketch\.(?:\s|$)/.test(tail)) continue; + const withdrawn = new RegExp(`\\b(?:${citation[1]}|${finding.cells[1]})\\s+(?:is|was|remains)\\s+(?:impossible|rejected|dismissed|withdrawn)\\b`, 'i'); + if (withdrawn.test(tail)) continue; + if (/\b(?:(?:this|that|the|original)\s+(?:trace|schedule|scenario|execution|sequence)|this|that|it)\s+(?:is|was|remains)\s+impossible\b/i.test(tail)) continue; + // Only the validated original rows establish the race. The registry and + // following assessment still govern whether the report dismisses it. + const claim = `Concurrent cache read fills an old value after the write committed and invalidated the same key. A new reader receives that stale value, which violates the read-after-write contract. ${finding.cells.join(' | ')} ${tail}`; + if (hasProseStaleFillFinding(claim)) return true; + } + } for (const trace of traces) { let heading = trace.start - 1; while (heading >= 0 && !prose[heading]!.trim()) heading--; diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index 95cdbe770..737a3bd66 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -2211,6 +2211,43 @@ function ceoNumberedBriefDecision(q: NativePlanQuestionCall['questions'][number] return currentProblem && amendment; } +/** A descriptive menu header can accompany a fully numbered issue brief. */ +function ceoParenthesizedIssueBrief(q: NativePlanQuestionCall['questions'][number], number: string): boolean { + const title = q.question.split('\n')[0]!; + if (!/^D[1-9]\d*\s+\(Issue [1-9]\d*\)\s*[—–-]\s*(?:What|How|Which|Should)\b[^\n?]+\?$/i.test(title) || + /\b(?:hypothetical|example|template)\b/i.test(title)) return false; + const prose = q.question + .replace(/```[\s\S]*?(?:```|$)|~~~[\s\S]*?(?:~~~|$)/g, '') + .replace(/^(?:\s*>| {4}|\t).*$/gm, '') + .replace(/"(?:[^"\\]|\\.)*"|“[^”]*”|`[^`]*`/g, '""'); + const explanations = [...prose.matchAll(/^ELI10:\s*(.+)$/gm)]; + const recommendations = [...prose.matchAll(/^Recommendation:\s*([1-9]\d*)([A-Z])\b/gim)]; + if (explanations.length !== 1 || recommendations.length !== 1 || + recommendations[0]![1] !== number || !/\w/.test(explanations[0]![1]!) || + /^(?:if|unless|whether|example|template|hypothetical|historical|quoted)\b/i.test(explanations[0]![1]!.trim())) return false; + // Current prose may explicitly withdraw an earlier issue. Literal examples + // and attributed quotations cannot supply either the brief or its withdrawal. + if (/\b(?:no (?:(?:current|unresolved) )?(?:defect|gap|issue|problem)|(?:this|that|the) (?:issue|finding|gap|problem|defect)\s+(?:is|was|has been)\s+(?:(?:already|now)\s+)?(?:resolved|fixed|closed)|(?:this|that|the) (?:question|finding|issue)\s+is\s+(?:only\s+)?(?:an?\s+)?(?:example|hypothetical)|(?:I|we)\s+(?:withdraw|retract)\s+(?:this|that|the)\s+(?:finding|issue|question))\b/i.test(prose)) return false; + // The number is identity, not evidence of a defect. Require a current + // missing contract in the assessment and a concrete offered amendment. + const assessment = [prose.split('\n')[0], /^Project\/branch\/task:\s*(.+)$/m.exec(prose)?.[1] ?? '', explanations[0]![1]!].join(' '); + const missingContract = /\b(?:no|without)\s+(?:error handling|tests?|checks?|validation|coordination)\b|\b(?:the|this) plan(?: itself)?\s+(?:(?:says|states|defines|specifies)\s+nothing\b|(?:does not|doesn't)\s+(?:define|specify|cover|mention|handle)\b)/i.test(assessment); + const amendment = q.options.some(option => [option.label.replace(/^[1-9]\d*[A-Z][):.]\s*/i, ''), option.description ?? ''].some(text => + text.replace(/"(?:[^"\\]|\\.)*"|“[^”]*”|`[^`]*`/g, '""') + .replace(/^Completeness\s+\d+\/10\.\s*/i, '').split(/[.;]\s+/).some(clause => + /^(?:add|remove|replace|send|rescue|handle|validate|check|assert|pin|require|define|specify|guard|serialize|parameterize|use|implement|write)\b/i.test(clause.trim()) && + !/^[a-z]+\s+(?:(?:the|a|this|prior|previous|completed|reviewed|current|saved|stored)\s+)*(?:review\s+)?(?:plan|report|summary|note|record|document|log)\b/i.test(clause.trim())))); + if (!missingContract || !amendment) return false; + const headerNumber = /^(?:(?:Finding|Issue)\s+F?|F)([1-9]\d*)(?:\s+[a-z][a-z -]*)?$/i.exec(q.header.trim()); + if (/^(?:finding|issue)\b|^f\d/i.test(q.header.trim()) && !headerNumber) return false; + if (headerNumber && headerNumber[1] !== number) return false; + const labels = q.options.map(option => /^([1-9]\d*)([A-Z])[):.]\s*\S/i.exec(option.label)); + return q.options.length >= 2 && q.options.every((option, i) => + Boolean(option.description?.trim()) && labels[i]?.[1] === number) && + new Set(labels.map(label => label![2]!.toUpperCase())).size === labels.length && + labels.some(label => label![2]!.toUpperCase() === recommendations[0]![2]!.toUpperCase()); +} + function nativeExplicitCeoFinding(fp: AskUserQuestionFingerprint, allowQuestionId = false): boolean { const call = fp.nativeCall; // QUESTION_TUNING=false omits qid injection. Accept an explicit Finding @@ -2254,6 +2291,7 @@ function nativeExplicitCeoFinding(fp: AskUserQuestionFingerprint, allowQuestionI const identity = /^(Finding|Issue)\s+F?([1-9]\d*(?:\.[1-9]\d*)*)(?:\s+\(Section\s+[1-9]\d*\))?\s*:\s*([^\n]+)$/i.exec(normalized); const numberedSubject = /^([1-9]\d*(?:\.[1-9]\d*)+)\s+([^:\n]+):\s*([^\n]+)$/.exec(normalized); const parenthesized = /^D[1-9]\d*\s+\(issue\s+([1-9]\d*(?:\.[1-9]\d*)*)\)\s*[—–-]\s*[^\n?]+\?$/i.exec(title); + if (parenthesized && allowQuestionId) return ceoParenthesizedIssueBrief(q, parenthesized[1]!); if (identity && !/^[^\n?]+\?$/.test(identity[3]!) && !ceoNumberedBriefDecision(q, identity[3]!)) return false; if (numberedSubject && (numberedSubject[2]!.trim().toLowerCase() !== q.header.trim().toLowerCase() || !ceoNumberedBriefDecision(q, numberedSubject[3]!))) return false; @@ -2281,6 +2319,7 @@ export const ceoFirstReviewAUQ: Step0BoundaryPredicate = (fp) => const id = / o.index === i + 1 && o.label === q.options[i]!.label) || @@ -34,3 +35,74 @@ export function isEngCompletionHandoff(fp: AskUserQuestionFingerprint, reviewedP if (!/ [—–] Record regression(?: characterization)? fixtures before\b/i.test(regression)) return false; return ceo.description === 'Optional scope and strategy pass before implementing. ✅ Catches product-level questions the eng review does not ask. ✅ Adds a CEO row to the dashboard. ❌ Little product surface here; likely confirms the current scope.'; } + +/** A finished backend review may recap an already-published author prerequisite. + * This classifies only its completed navigation; the runner still independently + * requires the owned report, fresh modifying decisions and a later native exit. + */ +function isPublishedPrerequisiteHandoff(fp: AskUserQuestionFingerprint, reviewedPlan: string): boolean { + const call = fp.nativeCall!, q = call.questions[0]!; + if (q.multiSelect || q.header.trim() !== 'Next' || q.options.length !== 2 || fp.options.length !== 2 || + !fp.options.every((o, i) => o.index === i + 1 && o.label === q.options[i]!.label) || + !q.options.some(o => o.label === call.answers?.[q.question]) || / s.trim()).filter(Boolean); + if (lines.length !== 7 || !/^D[1-9]\d* [—–-] Next step after this eng review\?$/.test(lines[0]!) || + !/^Project\/branch\/task: [\w.-]+ on [\w./-]+, reviewed plan written to gstack-test-plan-eng\.md\.$/.test(lines[1]!) || + lines[2] !== 'ELI10: The eng review is the only gate that blocks shipping and it is clear. No UI is touched, so a design review does not apply. A CEO review is optional and normally for product-direction changes; this is a backend auth refactor, so it is a soft mention only.' || + lines[3] !== 'Stakes if we pick wrong: Low either way; the CEO review would cost time on a change with no product-facing scope decision left open.' || + lines[5] !== 'Note: options differ in kind, not coverage — no completeness score.' || + lines[6] !== 'Net: start implementing versus an optional strategy pass on a backend refactor.') return false; + const recommendation = /^Recommendation: ([A-Z]) because all required reviews are complete and the plan's remaining blocker is the author confirming the Context section, not another review\.$/.exec(lines[4]!); + const ready = q.options.find(o => /^[A-Z]\) Ready to implement \(recommended\)$/.test(o.label)); + const ceo = q.options.find(o => /^[A-Z]\) Run \/plan-ceo-review$/.test(o.label)); + if (!recommendation || !ready || !ceo || ready.label[0] !== recommendation[1] || ready.label[0] === ceo.label[0]) return false; + const task = /^✅ All relevant reviews complete; run \/ship when the work is done\. ✅ The first task is the author confirming Context, then T([1-9]\d*) characterization tests\. ❌ No second strategic opinion on whether the refactor is the right thing to build now\.$/.exec(ready.description ?? ''); + if (!task || ceo.description !== '✅ Adds a scope-and-strategy pass before any code is written. ✅ Useful if the refactor\'s business motivation is contested. ❌ Backend-only refactor with no product-direction choice; likely low yield for the time.') return false; + + // Only the published Context and Implementation Tasks sections own the + // references. Quoted or fenced examples cannot supply a prerequisite/task. + const published: string[] = []; + let fence: { marker: string; length: number } | undefined; + for (const line of reviewedPlan.split(/\r?\n/)) { + if (fence) { + const close = /^ {0,3}(`{3,}|~{3,})[ \t]*$/.exec(line); + if (close && close[1]![0] === fence.marker && close[1]!.length >= fence.length) fence = undefined; + continue; + } + const open = /^ {0,3}(`{3,}|~{3,})(.*)$/.exec(line); + if (open && (open[1]![0] !== '`' || !open[2]!.includes('`'))) { + fence = { marker: open[1]![0]!, length: open[1]!.length }; continue; + } + if (!/^(?: {4}|\t| {0,3}>)/.test(line)) published.push(line); + } + if (fence) return false; + const section = (heading: string): string | undefined => { + const hits = published.flatMap((line, i) => line === `## ${heading}` ? [i] : []); + if (hits.length !== 1) return undefined; + const start = hits[0]!; + const preceding = published.slice(0, start).filter(s => s.trim()).at(-1) ?? ''; + if (/[:：]$|\b(?:example|sample|hypothetical|template|quoted)\b/i.test(preceding)) return undefined; + const end = published.findIndex((line, i) => i > start && /^#{1,2} /.test(line)); + return published.slice(start + 1, end < 0 ? undefined : end).join('\n'); + }; + const context = section('Context'), tasks = section('Implementation Tasks'); + if (!context || !tasks) return false; + const prerequisite = /^### Prerequisite P(\d+) \(decision D[1-9]\d* → [1-9]\d*[A-Z]\)\nImplementation does not start until the author confirms or edits the Problem, Goal,\nInvariants and Latency target above\./m.exec(context); + if (!prerequisite || !context.includes(`author must confirm — see Prerequisite P${prerequisite[1]} below`)) return false; + const entries = [...tasks.matchAll(/^- \[ \] \*\*T([1-9]\d*)\b[^\n]+$/gm)]; + const referenced = entries.filter(t => t[1] === task[1]); + if (referenced.length !== 1 || !/ — auth\/legacy — Write characterization tests for `legacyAuthFlow\(\)` before any rewrite$/.test(referenced[0]![0])) return false; + const prerequisiteTail = context.slice(prerequisite.index!); + const nextPrerequisiteHeading = prerequisiteTail.indexOf('\n### ', 1); + const prerequisiteOwner = nextPrerequisiteHeading < 0 ? prerequisiteTail : prerequisiteTail.slice(0, nextPrerequisiteHeading); + const taskStart = referenced[0]!.index!; + const nextTask = entries.find(entry => entry.index! > taskStart)?.index ?? tasks.length; + // A later correction inside the same owner can withdraw its earlier rule. + // Unrelated prerequisite/task bodies cannot supply or revoke this reference. + const withdrawn = (owner: string, id: string) => new RegExp( + `\\b(?:Prerequisite )?${id}\\b(?: (?:requirement|task))? (?:is |was |has been )?(?:cancelled|canceled|withdrawn|rejected|not (?:required|needed|necessary))\\b`, 'i').test(owner); + return !withdrawn(prerequisiteOwner, `P${prerequisite[1]}`) && + !/\b(?:the )?author no longer needs to confirm Context\b|\bContext confirmation is (?:not required|cancelled|withdrawn)\b/i.test(prerequisiteOwner) && + !withdrawn(tasks.slice(taskStart, nextTask), `T${task[1]}`) && + !/\bno characterization tests (?:are )?required\b|\bcharacterization tests are (?:not required|cancelled|withdrawn)\b/i.test(tasks.slice(taskStart, nextTask)); +} diff --git a/test/helpers/plan-count-file-permission.ts b/test/helpers/plan-count-file-permission.ts index 403a528e6..3eea1a4f3 100644 --- a/test/helpers/plan-count-file-permission.ts +++ b/test/helpers/plan-count-file-permission.ts @@ -73,7 +73,7 @@ export function recordFilePermission(input: string, file: string, cwd: string, c /** A long diff can crop its path header; the native access choice repeats the directory. */ function croppedEditTarget(screen: string, cwd: string): string | undefined { const text = screen.replace(/\r+\n?/g, '\n'); - // Cropping may begin inside a wrapped added/deleted diff row (five-space gutter). + // Cropping may begin inside a wrapped added/deleted diff row (four/five-space gutter). // Still require numbered rows below and the full native footer; never a quoted AUQ. // The heading can be cropped one row earlier, leaving the complete path. // Keep it only when the existing menu independently identifies that target. @@ -82,7 +82,7 @@ function croppedEditTarget(screen: string, cwd: string): string | undefined { const pathOnly = headerPath && (path.isAbsolute(headerPath) || /^\.\.?[/\\]/.test(headerPath)); // A crop can start on the single native rule immediately above the diff. const diff = pathOnly ? text.slice(header![0].length) : text.replace(/^[╌─━]{3,}[ \t]*\n/, ''); - if (!/^(?:\s*\d+\s+[ +\-]?| {5}[+\-])/.test(diff) || /[☐□]|^\s*(?:>|`{3}|~{3})/m.test(text)) return undefined; + if (!/^(?:\s*\d+\s+[ +\-]?| {4,5}[+\-])/.test(diff) || /[☐□]|^\s*(?:>|`{3}|~{3})/m.test(text)) return undefined; const prompt = [...text.matchAll(/^ {0,3}Do you want to make this edit to ([^\n?\/\\]+)\?[ \t]*\n([\s\S]*)$/gm)].at(-1); if (!prompt || (text.slice(0, prompt.index).match(/^\s*\d+\s+/gm)?.length ?? 0) < 2) return undefined; // The unselected option supplies path identity only. Input remains one-time Yes. diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 934e737f9..22ac7cbd8 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -148,7 +148,7 @@ export const E2E_TOUCHFILES: Record = { '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', 'test/helpers/third-party-actions.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', 'test/helpers/third-party-actions.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', 'test/fixtures/ceo-section-loading-q-report.md', 'test/fixtures/ceo-section-r-rejected-report.md', 'test/fixtures/ceo-section-s-trace-report.md', 'test/fixtures/ceo-section-u-report.md', 'test/fixtures/ceo-section-y-report.md', 'test/fixtures/ceo-section-aa-report.md', 'test/sdk-stale-table-ad-v3.test.ts', 'test/fixtures/sdk-stale-table-ad-v3.json', 'test/sdk-ordering-ae.test.ts', 'test/fixtures/sdk-ordering-ae.json', 'test/sdk-columnar-af.test.ts', 'test/fixtures/sdk-columnar-af.json', 'test/sdk-order-b-ag.test.ts', 'test/fixtures/sdk-order-b-ag.json'], + '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', 'test/fixtures/ceo-section-r-rejected-report.md', 'test/fixtures/ceo-section-s-trace-report.md', 'test/fixtures/ceo-section-u-report.md', 'test/fixtures/ceo-section-y-report.md', 'test/fixtures/ceo-section-aa-report.md', 'test/sdk-stale-table-ad-v3.test.ts', 'test/fixtures/sdk-stale-table-ad-v3.json', 'test/sdk-ordering-ae.test.ts', 'test/fixtures/sdk-ordering-ae.json', 'test/sdk-columnar-af.test.ts', 'test/fixtures/sdk-columnar-af.json', 'test/sdk-order-b-ag.test.ts', 'test/fixtures/sdk-order-b-ag.json', 'test/sdk-schedule-continuation-ah.test.ts', 'test/fixtures/sdk-schedule-continuation-ah.json'], // 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= to scope cost to the changed @@ -160,10 +160,10 @@ export const E2E_TOUCHFILES: Record = { // Each test drives its skill end-to-end; touchfiles include preamble + // completion-status resolvers because they affect question cadence and // terminal output (the regression surface this test catches). - 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', '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/ceo-count-s-terminals.test.ts', 'test/fixtures/ceo-count-s-paired.json', '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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', '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-r-call.json', 'test/fixtures/ceo-approach-r-distinct-call.json', 'test/fixtures/ceo-approach-q-paired-call.json', 'test/fixtures/ceo-completion-handoff-q-call.json', 'test/fixtures/ceo-completion-handoff-r-calls.json', 'test/fixtures/ceo-completion-handoff-t-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/ceo-completion-handoff-u-call.json', 'test/fixtures/ceo-completion-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-ceo-body-finding.test.ts', 'test/fixtures/ceo-count-w-paired.json', 'test/fixtures/ceo-completion-handoff-w-call.json', 'test/fixtures/ceo-approach-y-call.json', 'test/fixtures/ceo-approach-y-screen.txt', 'test/ceo-handoff-y.test.ts', 'test/fixtures/ceo-handoff-y-call.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/ceo-handoff-z-call.json', 'test/fixtures/ceo-approach-aa-call.json', 'test/review-handoffs-aa.test.ts', 'test/fixtures/review-handoff-aa-ceo.json', 'test/fixtures/review-handoff-aa-dx.json', 'test/helpers/ceo-mode-option.ts', 'test/ceo-count-mode.test.ts', 'test/fixtures/ceo-count-mode-ab-call.json', 'test/ceo-count-ac.test.ts', 'test/fixtures/ceo-count-ac-calls.json', 'test/fixtures/ceo-count-ac-later-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/ceo-count-ad-v2.test.ts', 'test/fixtures/ceo-count-ad-v2.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/fixtures/ceo-finding-alias-af.json', 'test/fixtures/ceo-numbered-brief-af.json', 'test/ceo-contract-assertions-ag.test.ts', 'test/fixtures/ceo-contract-assertions-ag.json', 'test/fixtures/ceo-contract-assertions-ag-retry.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/eng-binding-z.test.ts', 'test/fixtures/eng-binding-z-calls.json', 'test/eng-binding-retry-z.test.ts', 'test/fixtures/eng-binding-retry-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/helpers/eng-completion-handoff.ts', 'test/eng-count-ad-v2.test.ts', 'test/fixtures/eng-count-ad-v2.json', 'test/helpers/eng-seeded-coverage.ts', 'test/eng-seeded-coverage.test.ts', 'test/eng-seeded-packet-ae.test.ts', 'test/fixtures/eng-seeded-packet-ae.json', 'test/eng-first-category-af.test.ts', 'test/fixtures/eng-first-category-af.json', 'test/eng-regression-pinning-ag.test.ts', 'test/fixtures/eng-regression-pinning-ag.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', '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/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/design-completion-handoff-u.test.ts', 'test/fixtures/design-handoff-u-calls.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/helpers/design-artifact-question.ts', 'test/design-artifact-question.test.ts', 'test/fixtures/design-artifacts-w-calls.json', 'test/fixtures/design-outside-y-calls.json', 'test/fixtures/design-boundaries-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/design-gap-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/design-count-ad-v2.test.ts', 'test/fixtures/design-count-ad-v2.json', 'test/design-first-decision-af.test.ts', 'test/fixtures/design-first-decision-af.json', 'test/fixtures/design-first-decision-af-retry.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/dx-prerequisite-r-call.json', '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/ceo-count-s-distinct.json', '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', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/fixtures/devex-review-t-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/devex-count-u-calls.json', 'test/fixtures/devex-count-u-retry-calls.json', 'test/fixtures/devex-empathy-v-calls.json', 'test/fixtures/devex-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/fixtures/devex-count-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/devex-count-z-calls.json', 'test/fixtures/devex-handoff-z-call.json', 'test/review-handoffs-aa.test.ts', 'test/fixtures/review-handoff-aa-ceo.json', 'test/fixtures/review-handoff-aa-dx.json', 'test/devex-empathy-ab.test.ts', 'test/fixtures/devex-empathy-ab-calls.json', 'test/devex-ac-accounting.test.ts', 'test/fixtures/devex-ac-first-attempt-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/devex-reconfirmation-ad-v2.test.ts', 'test/fixtures/devex-reconfirmation-ad-v2.json', 'test/plan-count-history.test.ts', 'test/helpers/devex-seed-coverage.ts', 'test/devex-seed-coverage.test.ts', 'test/fixtures/devex-seed-coverage-ad-v3.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/ceo-count-s-terminals.test.ts', 'test/fixtures/ceo-count-s-paired.json', '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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', '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-r-call.json', 'test/fixtures/ceo-approach-r-distinct-call.json', 'test/fixtures/ceo-approach-q-paired-call.json', 'test/fixtures/ceo-completion-handoff-q-call.json', 'test/fixtures/ceo-completion-handoff-r-calls.json', 'test/fixtures/ceo-completion-handoff-t-call.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/ceo-completion-handoff-u-call.json', 'test/fixtures/ceo-completion-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-ceo-body-finding.test.ts', 'test/fixtures/ceo-count-w-paired.json', 'test/fixtures/ceo-completion-handoff-w-call.json', 'test/fixtures/ceo-approach-y-call.json', 'test/fixtures/ceo-approach-y-screen.txt', 'test/ceo-handoff-y.test.ts', 'test/fixtures/ceo-handoff-y-call.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/ceo-handoff-z-call.json', 'test/fixtures/ceo-approach-aa-call.json', 'test/review-handoffs-aa.test.ts', 'test/fixtures/review-handoff-aa-ceo.json', 'test/fixtures/review-handoff-aa-dx.json', 'test/helpers/ceo-mode-option.ts', 'test/ceo-count-mode.test.ts', 'test/fixtures/ceo-count-mode-ab-call.json', 'test/ceo-count-ac.test.ts', 'test/fixtures/ceo-count-ac-calls.json', 'test/fixtures/ceo-count-ac-later-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/ceo-count-ad-v2.test.ts', 'test/fixtures/ceo-count-ad-v2.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/fixtures/ceo-finding-alias-af.json', 'test/fixtures/ceo-numbered-brief-af.json', 'test/ceo-contract-assertions-ag.test.ts', 'test/fixtures/ceo-contract-assertions-ag.json', 'test/fixtures/ceo-contract-assertions-ag-retry.json', 'test/fixtures/plan-count-permission-ah.json', 'test/ceo-parenthesized-issue-ah.test.ts', 'test/fixtures/ceo-parenthesized-issue-ah.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', 'test/fixtures/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/eng-binding-z.test.ts', 'test/fixtures/eng-binding-z-calls.json', 'test/eng-binding-retry-z.test.ts', 'test/fixtures/eng-binding-retry-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/helpers/eng-completion-handoff.ts', 'test/eng-count-ad-v2.test.ts', 'test/fixtures/eng-count-ad-v2.json', 'test/helpers/eng-seeded-coverage.ts', 'test/eng-seeded-coverage.test.ts', 'test/eng-seeded-packet-ae.test.ts', 'test/fixtures/eng-seeded-packet-ae.json', 'test/eng-first-category-af.test.ts', 'test/fixtures/eng-first-category-af.json', 'test/eng-regression-pinning-ag.test.ts', 'test/fixtures/eng-regression-pinning-ag.json', 'test/fixtures/plan-count-permission-ah.json', 'test/eng-next-handoff-ah.test.ts', 'test/fixtures/eng-next-handoff-ah.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/ceo-count-s-distinct.json', '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/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/design-completion-handoff-u.test.ts', 'test/fixtures/design-handoff-u-calls.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/helpers/design-artifact-question.ts', 'test/design-artifact-question.test.ts', 'test/fixtures/design-artifacts-w-calls.json', 'test/fixtures/design-outside-y-calls.json', 'test/fixtures/design-boundaries-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/design-gap-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/design-count-ad-v2.test.ts', 'test/fixtures/design-count-ad-v2.json', 'test/design-first-decision-af.test.ts', 'test/fixtures/design-first-decision-af.json', 'test/fixtures/design-first-decision-af-retry.json', 'test/fixtures/plan-count-permission-ah.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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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/dx-prerequisite-r-call.json', '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/ceo-count-s-distinct.json', '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', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/fixtures/devex-review-t-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/devex-count-u-calls.json', 'test/fixtures/devex-count-u-retry-calls.json', 'test/fixtures/devex-empathy-v-calls.json', 'test/fixtures/devex-handoff-v-call.json', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/fixtures/devex-count-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/fixtures/devex-count-z-calls.json', 'test/fixtures/devex-handoff-z-call.json', 'test/review-handoffs-aa.test.ts', 'test/fixtures/review-handoff-aa-ceo.json', 'test/fixtures/review-handoff-aa-dx.json', 'test/devex-empathy-ab.test.ts', 'test/fixtures/devex-empathy-ab-calls.json', 'test/devex-ac-accounting.test.ts', 'test/fixtures/devex-ac-first-attempt-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/devex-reconfirmation-ad-v2.test.ts', 'test/fixtures/devex-reconfirmation-ad-v2.json', 'test/plan-count-history.test.ts', 'test/helpers/devex-seed-coverage.ts', 'test/devex-seed-coverage.test.ts', 'test/fixtures/devex-seed-coverage-ad-v3.json', 'test/fixtures/plan-count-permission-ah.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 = { // 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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/eng-binding-z.test.ts', 'test/fixtures/eng-binding-z-calls.json', 'test/eng-binding-retry-z.test.ts', 'test/fixtures/eng-binding-retry-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/eng-count-ad-v2.test.ts', 'test/fixtures/eng-count-ad-v2.json', 'test/eng-first-category-af.test.ts', 'test/fixtures/eng-first-category-af.json', 'test/eng-regression-pinning-ag.test.ts', 'test/fixtures/eng-regression-pinning-ag.json'], - '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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json'], + '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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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', 'test/eng-devex-s-count.test.ts', 'test/fixtures/eng-devex-s-first-calls.json', 'test/fixtures/eng-devex-s-retry-calls.json', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/eng-first-review-t.test.ts', 'test/fixtures/eng-batching-t-calls.json', 'test/plan-count-preview-footer.test.ts', 'test/fixtures/ceo-preview-u-call.json', 'test/fixtures/ceo-preview-u-screen.txt', 'test/fixtures/design-preview-v-screen.txt', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/eng-scope-y.test.ts', 'test/fixtures/eng-scope-y-calls.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/eng-binding-z.test.ts', 'test/fixtures/eng-binding-z-calls.json', 'test/eng-binding-retry-z.test.ts', 'test/fixtures/eng-binding-retry-z-calls.json', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/eng-count-ad-v2.test.ts', 'test/fixtures/eng-count-ad-v2.json', 'test/eng-first-category-af.test.ts', 'test/fixtures/eng-first-category-af.json', 'test/eng-regression-pinning-ag.test.ts', 'test/fixtures/eng-regression-pinning-ag.json', 'test/fixtures/plan-count-permission-ah.json'], + '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/autoplan-public-narration.test.ts', 'test/fixtures/autoplan-public-narration-ad.json', '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-navigation-r.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', 'test/helpers/plan-count-file-permission.ts', 'test/plan-count-file-permission.test.ts', 'test/fixtures/plan-count-edit-permission-t.json', 'test/plan-count-owned-permission.test.ts', 'test/fixtures/plan-count-owned-permission-v.json', 'test/fixtures/ceo-questionless-w-native.json', 'test/plan-count-truncated-question.test.ts', 'test/fixtures/ceo-approach-z-call.json', 'test/fixtures/ceo-approach-z-screen.txt', 'test/plan-count-permission-ac.test.ts', 'test/fixtures/plan-count-permission-ac.json', 'test/fixtures/plan-count-permission-ad.json', 'test/fixtures/plan-count-permission-ae.json', 'test/fixtures/plan-count-permission-target-ad-v2.json', 'test/fixtures/plan-count-permission-ah.json'], '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 diff --git a/test/hermetic-skill-runtime.test.ts b/test/hermetic-skill-runtime.test.ts index ebf495b56..3e66d26c8 100644 --- a/test/hermetic-skill-runtime.test.ts +++ b/test/hermetic-skill-runtime.test.ts @@ -48,6 +48,8 @@ const root = path.join(process.env.HOME,'.claude','skills','gstack'); const args = process.argv.slice(2); const addDirs = args.flatMap((arg,index) => arg === '--add-dir' ? [args[index+1]] : []); const addDir = addDirs[0] || null; +const registryDirectory = path.join(process.env.CLAUDE_CONFIG_DIR || path.join(process.env.HOME,'.claude'),'skills'); +const registeredSection = path.join(registryDirectory,'autoplan','sections','design-phase.md'); const stateDirectory = path.join(process.env.HOME,'.gstack'); const methodologyPath = path.join(stateDirectory,'projects','probe','autoplan-design-methodology-owned','methodology.md'); const inside = (file,directory) => { const relative=path.relative(directory,file); return relative==='' || (!relative.startsWith('..'+path.sep) && relative!=='..' && !path.isAbsolute(relative)); }; @@ -60,6 +62,10 @@ const record = { pid:process.pid, home:process.env.HOME, root:fs.realpathSync(root), args, addDirs, stateDirectory, stateDirectoryExists:fs.existsSync(stateDirectory), methodologyPath, methodologyAllowed:addDirs.some(directory => inside(methodologyPath,directory)), + registryDirectory, registeredSection, + registeredSectionHash:fs.existsSync(registeredSection) ? createHash('sha256').update(fs.readFileSync(registeredSection)).digest('hex') : null, + registeredSectionAllowed:addDirs.some(directory => inside(registeredSection,directory)), + registryNeighborsAllowed:['../settings.json','../plans/unrelated.md','../skills-other/unrelated.md'].map(relative => addDirs.some(directory => inside(path.resolve(registryDirectory,relative),directory))), phaseExit:phase.status, phaseHash:createHash('sha256').update(phase.stdout).digest('hex'), configExit:config.status, configValue:config.stdout.toString().trim(), canonicalConfigExit:canonicalConfig?.status, canonicalConfigValue:canonicalConfig?.stdout.toString().trim(), @@ -145,9 +151,12 @@ try { // the runtime checkout. Approve only that owned state tree, not HOME, // an inherited/explicit GSTACK_HOME, or a broad policy-setting answer. expect(result.methodologyAllowed, item.name + ': owned methodology Read is covered').toBe(true); + expect(result.registeredSectionAllowed, item.name + ': installed registry section Read is covered').toBe(true); + expect(result.registeredSectionHash).toBe(digest(path.join(ROOT,'autoplan/sections/design-phase.md'))); + expect(result.registryNeighborsAllowed).toEqual([false,false,false]); expect(result.stateDirectoryExists).toBe(true); expect(result.ownedStateRoot).toBe(result.stateDirectory); - expect(result.addDirs).toEqual([path.join(result.home,'.claude','skills','gstack'),path.join(result.home,'.gstack')]); + expect(result.addDirs).toEqual([path.join(result.home,'.claude','skills','gstack'),path.join(result.home,'.gstack'),path.join(result.configDir,'skills')]); for (const external of [operatorHome,state,path.dirname(result.home),result.codexHome,result.browserCache]) expect(result.addDirs).not.toContain(external); expect(result.args).not.toContain('--dangerously-skip-permissions'); @@ -168,6 +177,8 @@ try { expect(result.home).toBe(operatorHome); expect(result.phaseHash).toBe(originalSection); expect(result.configValue).toBe('stale-runtime'); expect(result.runtimeAllowed).toBe(false); expect(result.addDirs).toEqual([]); expect(result.methodologyAllowed).toBe(false); + expect(result.registeredSectionAllowed).toBe(false); + expect(result.registryNeighborsAllowed).toEqual([false,false,false]); expect(result.ownedStateRoot).toBeNull(); } if (item.name === 'explicit-config') expect(result.configDir).toBe(customConfig); diff --git a/test/plan-count-permission-ac.test.ts b/test/plan-count-permission-ac.test.ts index bbffb7daf..834d2bb97 100644 --- a/test/plan-count-permission-ac.test.ts +++ b/test/plan-count-permission-ac.test.ts @@ -5,6 +5,7 @@ import * as path from 'node:path'; import captured from './fixtures/plan-count-permission-ac.json'; import capturedAd from './fixtures/plan-count-permission-ad.json'; import capturedAe from './fixtures/plan-count-permission-ae.json'; +import capturedAh from './fixtures/plan-count-permission-ah.json'; import { classifyPlanCountFrame, createPlanCountPermissionGuard } from './helpers/claude-pty-runner'; import { recordFilePermission, currentFilePermissionEpoch, currentFilePermissionBinding } from './helpers/plan-count-file-permission'; import { E2E_TOUCHFILES, selectTests } from './helpers/touchfiles'; @@ -152,7 +153,7 @@ test('a later exact owned binding wins over an earlier same-basename block', () // Exact current screens plus content-free native identity from full AD/AE runs. // The replay projections do not assert these pending writes ever completed. -const cases = [...capturedAd.rows, capturedAe].map(row => ({ +const cases = [...capturedAd.rows, capturedAe, capturedAh].map(row => ({ p: row, screen: row.screen, binding: {expected: row.state.expected, state: row.state}, observation: {transcript: {status: row.transcriptStatus, calls: [], assistantMessages: row.transcriptSessions.map(sessionId => ({sessionId}))}}, @@ -179,7 +180,7 @@ for (const c of cases) { test(`AD isolating the rejected rendering guard ${c.p.pid} preserves native identity`, () => { // These are explicitly normalized controls; the actual captured screen is unchanged above. const normalized = c.p.pid === capturedAe.pid ? c.screen.replace(/^[╌─━]{3,}[ \t]*\n/, '') - : c.p.pid === 1332470 ? c.screen.replace('3. Nohift+tab)', '3. No') : c.screen.replace(/^ \+/, ' 99 +'); + : c.p.pid === 1332470 ? c.screen.replace('3. Nohift+tab)', '3. No') : c.screen.replace(/^ {4,5}\+/, ' 99 +'); expect(normalized).not.toBe(c.screen); expect(adEpoch(c, normalized)?.pendingId).toBe(c.binding.state.pendingId); }); @@ -249,3 +250,37 @@ test('AE crop admits one native divider only and preserves its exact existing ca expect(selectTests(['test/fixtures/plan-count-permission-ae.json'], E2E_TOUCHFILES).selected.sort()).toEqual( selectTests(['test/fixtures/plan-count-permission-ad.json'], E2E_TOUCHFILES).selected.sort()); }); + +test('AH wrapped crop admits four or five spaces with the same owned native epoch', () => { + const c = cases.find(item => item.p.pid === capturedAh.pid)!; + expect(c.screen.startsWith(' + ')).toBe(true); + for (const screen of [c.screen, ` ${c.screen}`, c.screen.replace(/^ \+/, ' -')]) { + expect(adEpoch(c, screen)?.pendingId).toBe(c.binding.state.pendingId); + const guard = createPlanCountPermissionGuard(); + expect(guard(screen, '', adEpoch(c, screen))).toBe('grant'); + expect(guard(screen, '', adEpoch(c, screen))).toBe('handled'); + } + // Cleaning the unselected No paint residue does not establish missing identity. + const noOnly = c.screen.replace('3. Nohift+tab)', '3. No'); + expect(noOnly).not.toBe(c.screen); + expect(adEpoch(c, noOnly)?.pendingId).toBe(c.binding.state.pendingId); +}); + +test('AH continuation crop rejects prose, unsupported gutters and malformed numbered context', () => { + const c = cases.find(item => item.p.pid === capturedAh.pid)!; + for (const [name, screen] of [ + ['four-space prose', c.screen.replace(/^.*\n/, ' Apply this edit now\n')], + ['four-space quoted prose', c.screen.replace(/^.*\n/, ' > Example\n')], + ['three-space gutter', c.screen.slice(1)], + ['six-space gutter', ` ${c.screen}`], + ['no numbered rows', c.screen.replace(/^\s*\d+\s+(?=[+\- ])/gm, ' +')], + ['one numbered row', c.screen.replace(/^(\s*\d+\s+)(?=[+\- ])/gm, + (prefix, _group, offset) => offset === c.screen.indexOf(' 79 ') ? prefix : ' +')], + ]) { + expect(screen, name).not.toBe(c.screen); + expect(adEpoch(c, screen), name).toBeNull(); + expect(createPlanCountPermissionGuard()(screen, '', adEpoch(c, screen)), name).not.toBe('grant'); + } + expect(selectTests(['test/fixtures/plan-count-permission-ah.json'], E2E_TOUCHFILES).selected.sort()).toEqual( + selectTests(['test/fixtures/plan-count-permission-ae.json'], E2E_TOUCHFILES).selected.sort()); +}); diff --git a/test/sdk-schedule-continuation-ah.test.ts b/test/sdk-schedule-continuation-ah.test.ts new file mode 100644 index 000000000..6a3e3a8c5 --- /dev/null +++ b/test/sdk-schedule-continuation-ah.test.ts @@ -0,0 +1,121 @@ +import { expect, test } from 'bun:test'; +import fixture from './fixtures/sdk-schedule-continuation-ah.json'; +import { hasStaleFillRaceFinding } from './helpers/ceo-section-loading-fixture'; +import { E2E_TOUCHFILES } from './helpers/touchfiles-data'; +import { selectTests } from './helpers/touchfiles'; + +const frame = fixture.compact; +function replace(from: string, to: string, input = frame): string { + expect(input.includes(from)).toBe(true); + return input.replace(from, to); +} +const originalRows = ' S2* | await read ... | write commits, delete(noop) | | - |\n' + + ' | resolves V0 → set V0 | | hit → V0 | V0 (30 s) | VIOLATION\n'; + +test('retains both exact public report forms as affirmative original-race findings', () => { + expect(hasStaleFillRaceFinding(fixture.report)).toBe(true); + expect(hasStaleFillRaceFinding(frame)).toBe(true); + expect(fixture.report.includes(frame.trim())).toBe(true); +}); + +test('binds consistently renamed actors, shared key, versions and finding/schedule IDs', () => { + const renamed = frame.replace(/\bR1\b/g, 'R7').replace(/\bR2\b/g, 'R8').replace(/\bW\b/g, 'W9') + .replace(/\bV0\b/g, 'oldValue').replace(/\bV1\b/g, 'freshValue') + .replace(/\bkey\b/g, 'profile_key').replace(/\bF1\b/g, 'F9').replace(/\bS2\b/g, 'S9'); + expect(hasStaleFillRaceFinding(renamed)).toBe(true); + expect(hasStaleFillRaceFinding(frame.replace(/→/g, '->'))).toBe(true); + const unrelated = '## Historical example\nAn unrelated old example.\n\n## Current findings\n\n'; + expect(hasStaleFillRaceFinding(unrelated + frame)).toBe(true); +}); + +test('amendments, permitted earlier readers and missing continuation do not supply the original race', () => { + for (const changed of [ + replace(originalRows, ''), + replace('S2* | await read', 'S2 A1 | await read'), + replace(originalRows, ' S2* | begins before W, joins | delete + forget | — | — | OK: R1 began before W completed (permitted clause)\n'), + replace('resolves V0 → set V0', 'resolves V0, slot gone→drop'), + replace('hit → V0', 'miss→read V1→set'), + replace('hit → V0', ''), + replace('VIOLATION\n S2 A1', 'OK (permitted earlier return)\n S2 A1'), + replace('VIOLATION\n S2 A1', 'VIOLATION\n | already guarded | | | | OK\n S2 A1'), + ]) expect(hasStaleFillRaceFinding(changed)).toBe(false); +}); + +test('requires the original schedule citation, legend and explicit post-completion boundary', () => { + for (const changed of [ + replace('Schedule S2 makes', 'Schedule S9 makes'), + replace('`*` = original sketch.', '`*` = amended sketch.'), + replace('`*` = original sketch.', ''), + replace('`*` = original sketch.', 'Hypothetically, `*` = original sketch.'), + replace('CRITICAL GAP | 1, 2, 4, 5, 6', 'CRITICAL GAP | 1, 2, 5, 6'), + replace('Violates retained invariant.', 'No defect in the retained invariant.'), + replace('R2 (begins after W)', 'R2 (begins before W)'), + replace('after `writeProfile` resolves', 'before `writeProfile` resolves'), + replace('after `writeProfile` resolves', 'after `writeProfile` begins'), + replace('after `writeProfile` resolves', 'after `readProfile` resolves'), + ]) expect(hasStaleFillRaceFinding(changed)).toBe(false); +}); + +test('rejects actor, key, value, invalidation and ordering mismatches', () => { + for (const changed of [ + replace('R2 (begins after W)', 'R1 (begins after W)'), + replace('R2 (begins after W)', 'R2 (begins after W9)'), + replace('`inflight[key]`', '`inflight[other_key]`'), + replace('| cache[key] | Result', '| cache[other_key] | Result'), + replace('W (commits V1)', 'W (commits V0)'), + replace('resolves V0 → set V0', 'resolves V1 → set V0'), + replace('resolves V0 → set V0', 'resolves V0 → set V1'), + replace('hit → V0', 'hit → V1'), + replace('write commits, delete(noop)', 'write begins, delete(noop)'), + replace('write commits, delete(noop)', 'write commits'), + replace('await read ...', 'await write ...'), + replace(originalRows, originalRows.split('\n').slice(0, 2).reverse().join('\n') + '\n'), + replace('V0 (30 s) | VIOLATION', 'V1 (30 s) | VIOLATION'), + replace('see V0 for 30 s;', 'see V1 for 30 s;'), + ]) expect(hasStaleFillRaceFinding(changed)).toBe(false); +}); + +test('quotes, source introductions and withdrawn findings remain negative', () => { + for (const prefix of ['An unproven hypothesis.', 'Historical example only.', 'The following is a hypothetical example.']) { + expect(hasStaleFillRaceFinding(prefix + '\n\n' + frame)).toBe(false); + expect(hasStaleFillRaceFinding(replace('### Async Ordering Record', prefix + '\n\n### Async Ordering Record'))).toBe(false); + expect(hasStaleFillRaceFinding(replace('### Findings Registry\n', '### Findings Registry\n\n' + prefix))).toBe(false); + } + expect(hasStaleFillRaceFinding(frame.split('\n').map(line => '> ' + line).join('\n'))).toBe(false); + expect(hasStaleFillRaceFinding('````text\n' + frame + '\n````')).toBe(false); + expect(hasStaleFillRaceFinding(replace('```\n Sched', '```javascript\n Sched'))).toBe(false); + for (const dismissal of [ + 'The original trace is impossible.', 'This schedule is not a bug.', + 'The original race is permitted.', 'The stale fill is accepted.', + 'No coordination is required.', + ]) { + expect(hasStaleFillRaceFinding(frame + '\n' + dismissal)).toBe(false); + expect(hasStaleFillRaceFinding(replace('Violates retained invariant.', 'Violates retained invariant. ' + dismissal))).toBe(false); + } +}); + + +test('completed prior decision section is independent; spoofed or withdrawn framing is not', () => { + const close = '### Decision Registry (all auto-resolved to recommended option)\n\n| D1 | A | B |\n\nLake Score: 7/7 recommendations chose the complete option.\n\n'; + expect(hasStaleFillRaceFinding(close + frame)).toBe(true); + expect(hasStaleFillRaceFinding(close.replace('### Decision Registry (all auto-resolved to recommended option)', '### Historical example') + frame)).toBe(false); + expect(hasStaleFillRaceFinding(close.replace('Lake Score: 7/7 recommendations chose the complete option.', 'An unproven hypothesis.') + frame)).toBe(false); + const row = frame.split('\n').find(line => line.startsWith('| F1 |'))!; + expect(hasStaleFillRaceFinding(replace(row, row + '\n' + row))).toBe(false); +}); + +test('only the existing SDK paid owner selects the added fixture and controls', () => { + for (const file of ['test/sdk-schedule-continuation-ah.test.ts', 'test/fixtures/sdk-schedule-continuation-ah.json']) { + expect(Object.entries(E2E_TOUCHFILES).filter(([, files]) => files.includes(file)).map(([name]) => name)).toEqual(['plan-ceo-section-loading']); + expect(selectTests([file], E2E_TOUCHFILES, []).selected).toEqual(['plan-ceo-section-loading']); + } +}); + + +test('same finding or schedule tail withdrawals remain authoritative', () => { + for (const tail of ['S2 is impossible.', 'F1 is rejected. The original trace is impossible.', 'F1 is rejected.', 'S2 is withdrawn.']) { + expect(hasStaleFillRaceFinding(frame + '\n' + tail)).toBe(false); + } + expect(hasStaleFillRaceFinding(frame + '\nF2 is rejected. The original trace is impossible.')).toBe(true); + expect(hasStaleFillRaceFinding(frame + '\nS3 is impossible.')).toBe(true); +}); diff --git a/test/skill-e2e-plan-ceo-finding-count.test.ts b/test/skill-e2e-plan-ceo-finding-count.test.ts index a002d8d86..3706ad264 100644 --- a/test/skill-e2e-plan-ceo-finding-count.test.ts +++ b/test/skill-e2e-plan-ceo-finding-count.test.ts @@ -90,8 +90,20 @@ const planCeo5Findings = (planPath: string) => [ 'For every nonempty external string it performs no SQL-format validation.', 'The adapter forwards that external string unchanged. It does not cast,', 'escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.', + 'User IDs are opaque TEXT values, including punctuation and Unicode. The', + 'lookup has no integer/UUID cast or ID-format restriction; every nonempty', + 'string is a valid identifier representation.', + 'An existing ingress ownership guard checks the PaymentIntent ID against', + 'its stored opaque user-ID binding before invoking the handler. A mismatch', + 'is acknowledged with HTTP 200 and an event-correlated warning. This is an', + 'identity comparison, not SQL-format validation; the adapter still forwards', + 'the original string unchanged.', 'The existing webhook event guard deduplicates deliveries by Stripe event ID,', - 'and an existing per-user transaction lock serializes payment updates.', + 'and an existing per-user lock serializes payment updates.', + 'The event guard acquires the existing per-user lock before checking the', + 'committed completion marker, and rechecks after any lock wait. It holds', + 'that lock through the handler and completion bookkeeping; an overlapping', + 'completed duplicate does not invoke the handler.', 'The new handler runs inside those unchanged guards; this plan does not', 'replace signature verification, event deduplication, or update locking.', 'The existing user update assigns payment_status=paid and the payment intent', @@ -99,6 +111,10 @@ const planCeo5Findings = (planPath: string) => [ 'intent assigns the same values, independently of the event-ID guard.', 'The existing lookup-result guard acknowledges unknown/deleted users with', 'HTTP 200, logs the event, and stops before user updates or email fan-out.', + 'Account deletion uses the same per-user lock. The handler holds it from', + 'lookup through update and inline email, so deletion either precedes lookup', + '(the existing unknown/deleted-user path) or follows the handler; it cannot', + 'remove the user between lookup and update.', 'The ingress wrapper already logs event IDs, outcomes, and durations, with', 'alerts for failed webhook processing. Those controls remain in place.', 'The existing DB and mail clients attach the adapter user ID and event ID',