diff --git a/CHANGELOG.md b/CHANGELOG.md index 8aa6b996a..6a04a32c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ - Failed, refused, empty, or malformed outside reviews can no longer count as a clean pass. Claude runner failures include authentication, timeout, and output overflow diagnoses, and stale skills stop before invoking their own harness. - Spec review stops when redaction fails, before sending the spec to a reviewer or saving it downstream. - Planning reviews request each unresolved decision before editing and carry approved remedies across sections without asking again. Choosing a scope or approach does not approve every finding. Reviews preserve stated requirements unless you authorize changing them. -- Autoplan preserves the original plan and gives each reviewer the complete current phase input. It reads the review skills installed for the current harness, waits for its reviewers, and reports completion after saving the required outputs. Disabling extra plan or documentation review also skips replacement reviewers. +- Autoplan preserves the original plan and carries every approved obligation into the next phase’s input. It reads the review skills installed for the current harness, waits for each phase’s reviewers and verified plan updates before starting the next phase, and reports completion after saving the required outputs. Disabling extra plan or documentation review also skips replacement reviewers. ## [1.83.0.0] - 2026-09-09 diff --git a/autoplan/SKILL.md b/autoplan/SKILL.md index 228d56bff..f4504d6ab 100644 --- a/autoplan/SKILL.md +++ b/autoplan/SKILL.md @@ -661,7 +661,7 @@ context models lack. See Decision Classification above. - PRODUCE every output the section requires (diagrams, tables, registries, artifacts) - IDENTIFY every issue the section is designed to catch - DECIDE each issue using the 6 principles (instead of asking the user) -- LOG each decision in the audit trail +- LOG each decision and amend ALL accepted obligations in `Implementation plan` in the SAME edit - WRITE all required artifacts to disk **You MUST NOT:** diff --git a/autoplan/SKILL.md.tmpl b/autoplan/SKILL.md.tmpl index d67dba94e..5018d201d 100644 --- a/autoplan/SKILL.md.tmpl +++ b/autoplan/SKILL.md.tmpl @@ -146,7 +146,7 @@ context models lack. See Decision Classification above. - PRODUCE every output the section requires (diagrams, tables, registries, artifacts) - IDENTIFY every issue the section is designed to catch - DECIDE each issue using the 6 principles (instead of asking the user) -- LOG each decision in the audit trail +- LOG each decision and amend ALL accepted obligations in `Implementation plan` in the SAME edit - WRITE all required artifacts to disk **You MUST NOT:** diff --git a/autoplan/sections/ceo-phase.md b/autoplan/sections/ceo-phase.md index 8a5ea928d..7ebf752a8 100644 --- a/autoplan/sections/ceo-phase.md +++ b/autoplan/sections/ceo-phase.md @@ -1,6 +1,6 @@ -Read `~/.claude/skills/gstack/plan-ceo-review/SKILL.md` and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-ceo-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Mode selection: SELECTIVE EXPANSION @@ -26,19 +26,17 @@ bun "" create ceo "" "" **Claude CEO subagent** (via Agent tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **Codex CEO voice** (via Bash): @@ -137,8 +135,8 @@ CEO DUAL VOICES — CONSENSUS TABLE: 5. Competitive/market risks covered? — — — 6. 6-month trajectory sound? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` Sections 1-10 — for EACH section, run the evaluation criteria from the loaded skill file: @@ -155,17 +153,18 @@ Sections 1-10 — for EACH section, run the evaluation criteria from the loaded - Dream state delta (where this plan leaves us vs 12-month ideal) - Completion Summary (the full summary table from the CEO skill) -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check ceo "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 1 complete.** Codex: [completed: N concerns / unavailable / disabled]. Claude subagent: [completed: N issues / unavailable]. diff --git a/autoplan/sections/ceo-phase.md.tmpl b/autoplan/sections/ceo-phase.md.tmpl index 9cae27295..88284c7bb 100644 --- a/autoplan/sections/ceo-phase.md.tmpl +++ b/autoplan/sections/ceo-phase.md.tmpl @@ -1,4 +1,4 @@ -Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review}} and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-ceo-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Mode selection: SELECTIVE EXPANSION @@ -24,19 +24,17 @@ bun "" create ceo "" "" **{{NATIVE_LABEL}} CEO subagent** (via Agent tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **{{OUTSIDE_LABEL}} CEO voice** (via Bash): @@ -96,8 +94,8 @@ CEO DUAL VOICES — CONSENSUS TABLE: 5. Competitive/market risks covered? — — — 6. 6-month trajectory sound? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` Sections 1-10 — for EACH section, run the evaluation criteria from the loaded skill file: @@ -114,17 +112,18 @@ Sections 1-10 — for EACH section, run the evaluation criteria from the loaded - Dream state delta (where this plan leaves us vs 12-month ideal) - Completion Summary (the full summary table from the CEO skill) -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check ceo "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 1 complete.** {{OUTSIDE_LABEL}}: [completed: N concerns / unavailable / disabled]. {{NATIVE_LABEL}} subagent: [completed: N issues / unavailable]. diff --git a/autoplan/sections/design-phase.md b/autoplan/sections/design-phase.md index 2c9c2c928..0c3dacd5c 100644 --- a/autoplan/sections/design-phase.md +++ b/autoplan/sections/design-phase.md @@ -1,6 +1,6 @@ -Read `~/.claude/skills/gstack/plan-design-review/SKILL.md` and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-design-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Focus areas: all relevant dimensions (P1) @@ -17,19 +17,17 @@ bun "" create design "" "" **Claude design subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **Codex design voice** (via Bash): @@ -109,21 +107,23 @@ For this phase (design), retain the historical review-log skill identifier. Add and Claude SUBAGENT (design — independent review). Produce the design litmus scorecard from plan-design-review. CEO findings go only to the outside voice; the native voice stays independent. + Missing/disabled outside = N/A, not CONFIRMED; primary cannot replace it. 3. Passes 1-7: Run each from loaded skill. Rate 0-10. Auto-decide each issue. DISAGREE items from scorecard → raised in the relevant pass with both perspectives. -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check design "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 2 complete.** Codex: [completed: N concerns / unavailable / disabled]. Claude subagent: [completed: N issues / unavailable]. diff --git a/autoplan/sections/design-phase.md.tmpl b/autoplan/sections/design-phase.md.tmpl index 309c41876..4de660b9e 100644 --- a/autoplan/sections/design-phase.md.tmpl +++ b/autoplan/sections/design-phase.md.tmpl @@ -1,4 +1,4 @@ -Read {{AUTOPLAN_REVIEW_FILE:plan-design-review}} and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-design-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Focus areas: all relevant dimensions (P1) @@ -15,19 +15,17 @@ bun "" create design "" "" **{{NATIVE_LABEL}} design subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **{{OUTSIDE_LABEL}} design voice** (via Bash): @@ -68,21 +66,23 @@ Outer tool timeout: 720000ms. Failed/incomplete outside review → unavailable; and {{NATIVE_LABEL}} SUBAGENT (design — independent review). Produce the design litmus scorecard from plan-design-review. CEO findings go only to the outside voice; the native voice stays independent. + Missing/disabled outside = N/A, not CONFIRMED; primary cannot replace it. 3. Passes 1-7: Run each from loaded skill. Rate 0-10. Auto-decide each issue. DISAGREE items from scorecard → raised in the relevant pass with both perspectives. -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check design "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 2 complete.** {{OUTSIDE_LABEL}}: [completed: N concerns / unavailable / disabled]. {{NATIVE_LABEL}} subagent: [completed: N issues / unavailable]. diff --git a/autoplan/sections/dx-phase.md b/autoplan/sections/dx-phase.md index d214c8d4f..e9e2c9c15 100644 --- a/autoplan/sections/dx-phase.md +++ b/autoplan/sections/dx-phase.md @@ -1,6 +1,6 @@ -Read `~/.claude/skills/gstack/plan-devex-review/SKILL.md` and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-devex-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Mode selection: DX POLISH @@ -21,19 +21,17 @@ bun "" create dx "" "" **Claude DX subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **Codex DX voice** (via Bash): @@ -126,8 +124,8 @@ DX DUAL VOICES — CONSENSUS TABLE: 5. Upgrade path safe? — — — 6. Dev environment friction-free? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` 3. Passes 1-8: Run each from loaded skill. Rate 0-10. Auto-decide each issue. @@ -142,17 +140,18 @@ Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = fl - DX Implementation Checklist - TTHW assessment with target -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check dx "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 2.5 complete.** DX overall: [N]/10. TTHW: [N] min → [target] min. diff --git a/autoplan/sections/dx-phase.md.tmpl b/autoplan/sections/dx-phase.md.tmpl index 1f9cc5f76..d76b336cb 100644 --- a/autoplan/sections/dx-phase.md.tmpl +++ b/autoplan/sections/dx-phase.md.tmpl @@ -1,4 +1,4 @@ -Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review}} and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-devex-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Mode selection: DX POLISH @@ -19,19 +19,17 @@ bun "" create dx "" "" **{{NATIVE_LABEL}} DX subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **{{OUTSIDE_LABEL}} DX voice** (via Bash): @@ -85,8 +83,8 @@ DX DUAL VOICES — CONSENSUS TABLE: 5. Upgrade path safe? — — — 6. Dev environment friction-free? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` 3. Passes 1-8: Run each from loaded skill. Rate 0-10. Auto-decide each issue. @@ -101,17 +99,18 @@ Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = fl - DX Implementation Checklist - TTHW assessment with target -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check dx "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion AND load/create/dispatch the next phase: **Phase 2.5 complete.** DX overall: [N]/10. TTHW: [N] min → [target] min. diff --git a/autoplan/sections/eng-phase.md b/autoplan/sections/eng-phase.md index e243b93a6..60c3149ed 100644 --- a/autoplan/sections/eng-phase.md +++ b/autoplan/sections/eng-phase.md @@ -1,6 +1,6 @@ -Read `~/.claude/skills/gstack/plan-eng-review/SKILL.md` and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read `~/.claude/skills/gstack/plan-eng-review/SKILL.md` + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Scope challenge: never reduce (P2) @@ -14,19 +14,17 @@ bun "" create eng "" "" **Claude eng subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **Codex eng voice** (via Bash): @@ -117,8 +115,8 @@ ENG DUAL VOICES — CONSENSUS TABLE: 5. Error paths handled? — — — 6. Deployment risk manageable? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` 3. Section 1 (Architecture): Produce ASCII dependency graph showing new components @@ -150,17 +148,18 @@ Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = fl - Completion Summary (the full summary from the Eng skill) - TODOS.md updates (collected from all phases) -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check eng "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion and proceed to final synthesis/approval: **Phase 3 complete.** Codex: [completed: N concerns / unavailable / disabled]. Claude subagent: [completed: N issues / unavailable]. diff --git a/autoplan/sections/eng-phase.md.tmpl b/autoplan/sections/eng-phase.md.tmpl index 32f90c1ff..6842ae0cd 100644 --- a/autoplan/sections/eng-phase.md.tmpl +++ b/autoplan/sections/eng-phase.md.tmpl @@ -1,4 +1,4 @@ -Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review}} and its triggered sections in full. Before dispatch, record successful Read start/end/total ranges; fetch gaps to EOF. Load skip-listed sections; skip execution. +Before dispatch, fully Read {{AUTOPLAN_REVIEW_FILE:plan-eng-review}} + triggered sections; record successful start/end/total ranges; fill gaps to EOF. Load skip-listed sections; skip execution. **Override rules:** - Scope challenge: never reduce (P2) @@ -12,19 +12,17 @@ bun "" create eng "" "" **{{NATIVE_LABEL}} eng subagent** (native tool): Claude Code: set Agent `run_in_background: false` if its schema exposes it. - Other hosts: use foreground dispatch and await completion when supported. + Other hosts: foreground dispatch; await completion when supported. Send `nativeDispatchPrompt` verbatim: ONLY/FINAL tool call this response. Keep native Reads enabled. Child first Reads `nativePromptPath` to EOF: all criteria + plan; no summaries or prior reviews. - **Native completion barrier:** If `isAsync: true` / `status: "async_launched"`, - Claude Code: immediately end this response with "Waiting for ." - Do no more tool calls or review work until that ID's terminal notification is - delivered. Other hosts: await that ID. Then outside → this phase's review ONLY. - For completed native reviews, match INPUT phase/hash to this snapshot. Missing - or mismatched INPUT: retry this dispatch once, then use failure policy if - still invalid. + **Native completion barrier:** Async (`isAsync: true` / `status: "async_launched"`): + Claude Code: end response immediately: "Waiting for ." + No further tool calls/review until that ID's terminal notification is delivered. + Other hosts await that ID. Then outside → this phase's review ONLY. + Completed-native INPUT must match snapshot phase/hash. Retry invalid input once; then failure policy if still invalid. No inline substitute; apply failure policy. **{{OUTSIDE_LABEL}} eng voice** (via Bash): @@ -76,8 +74,8 @@ ENG DUAL VOICES — CONSENSUS TABLE: 5. Error paths handled? — — — 6. Deployment risk manageable? — — — ═══════════════════════════════════════════════════════════════ -CONFIRMED = both agree. DISAGREE = models differ (→ taste decision). -Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = flagged regardless. +CONFIRMED = native + outside agree; primary cannot replace outside. DISAGREE → taste. +Missing/disabled voice = N/A, never CONFIRMED. Flag any single-voice critical finding. ``` 3. Section 1 (Architecture): Produce ASCII dependency graph showing new components @@ -109,17 +107,18 @@ Missing voice = N/A (not CONFIRMED). Single critical finding from one voice = fl - Completion Summary (the full summary from the Eng skill) - TODOS.md updates (collected from all phases) -**Close this phase:** Edit accepted changes into `Implementation plan` (taste: -pending final approval; unresolved User Challenges: retain original direction). -Report/task edits do not count. After successful Edit: +**Close this phase:** Reread full phase `Review record`; record EVERY accepted +obligation (split bundles) → exact `Implementation plan` text. Edit omissions. +Taste: provisional; unresolved User Challenges: original direction. +Report/task edits do not count. ```bash bun "" check eng "" "" changed ``` -Use `unchanged` only if no implementation changes were accepted; explain why. -Verify returned text against decisions; hashes prove bytes, not correctness. -Require full load ranges, matched INPUT for completed native reviews, consumed -terminal reviewers (unavailable/disabled allowed), successful writes and this check result. -Only then emit this actual assistant message: +No accepted implementation change: `unchanged` + reason. +Verify each mapping in readback; bytes prove neither completeness nor correctness. +Require full skill/section Read ranges, matched INPUT for completed native reviews, consumed +terminal reviewers (unavailable/disabled allowed), successful writes/check. +Only then announce completion and proceed to final synthesis/approval: **Phase 3 complete.** {{OUTSIDE_LABEL}}: [completed: N concerns / unavailable / disabled]. {{NATIVE_LABEL}} subagent: [completed: N issues / unavailable]. diff --git a/plan-ceo-review/SKILL.md b/plan-ceo-review/SKILL.md index 44daa0b1e..058219a72 100644 --- a/plan-ceo-review/SKILL.md +++ b/plan-ceo-review/SKILL.md @@ -1119,27 +1119,30 @@ Once selected, commit fully. Do not silently drift. Present these mode options via AskUserQuestion using the preamble's AskUserQuestion Format section: include RECOMMENDATION. These options differ in kind (review posture), not coverage — do NOT emit `Completeness: N/10` per option. Include the one-line note from step 4 of the preamble format rule instead: `Note: options differ in kind, not coverage — no completeness score.` -**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds. -**Reminder: Do NOT make any code changes. Review only.** +**STOP.** AskUserQuestion: one tool_use per issue, no batching, even obvious fixes. Recommend + WHY; wait for approval before changing the plan. Zero findings: state "No issues, moving on" and proceed. No code changes; review only. > **STOP.** Before running the 11-section deep review, required outputs, and review report (only after Step 0 scope and mode are agreed), Read `~/.claude/skills/gstack/plan-ceo-review/sections/review-sections.md` and execute it > in full. Do not work from memory — that section is the source of truth for this step. ## Section self-check (before you finish) -You ran a carved skill. The Section index above named `sections/review-sections.md` -as the source of truth for the 11-section deep review, the required outputs, and the -review report. Confirm you issued a Read for it and executed every section from the -file, not from memory. If you produced the Completion Summary or wrote the review -report without Reading that section, STOP, Read it now, and redo the review from the -source of truth. +Read and execute every section and output in `sections/review-sections.md`. +If summaries/reports came first, STOP, Read it and redo the review. +Before summaries, review logs or next-step menus, run approval check 0 below. ## EXIT PLAN MODE GATE (BLOCKING) Before calling ExitPlanMode, run this self-check. If any item fails, do the missing work — do NOT call ExitPlanMode: +0. Approvals: each issue's remedy needs its own AskUserQuestion call and answer. + Never group distinct issues. Setup, mode, approach and navigation are not approval. + Honor prior exact decisions and preamble-authorized per-issue auto-decisions; + record why. Deferrals remain unresolved. + If missing, reset drafts to pending, ask and wait. After answers or resets, + refresh the plan, report and review log; rerun this gate. + 1. Read the plan file with the Read tool (after your most recent write to it). 2. Confirm the LAST `## ` heading in the file is `## GSTACK REVIEW REPORT`. In-body prose that mentions "outside voice", "codex findings", or similar diff --git a/plan-ceo-review/SKILL.md.tmpl b/plan-ceo-review/SKILL.md.tmpl index 3981ef955..a73292c0d 100644 --- a/plan-ceo-review/SKILL.md.tmpl +++ b/plan-ceo-review/SKILL.md.tmpl @@ -415,19 +415,15 @@ Once selected, commit fully. Do not silently drift. Present these mode options via AskUserQuestion using the preamble's AskUserQuestion Format section: include RECOMMENDATION. These options differ in kind (review posture), not coverage — do NOT emit `Completeness: N/10` per option. Include the one-line note from step 4 of the preamble format rule instead: `Note: options differ in kind, not coverage — no completeness score.` -**STOP.** AskUserQuestion once per issue. Do NOT batch. Recommend + WHY. If this section turned up zero findings, state "No issues, moving on" and proceed. If the section has findings, you MUST call AskUserQuestion as a tool_use — a finding with an "obvious fix" is still a finding and still needs user approval before any change lands in the plan. Do NOT proceed until the user responds. -**Reminder: Do NOT make any code changes. Review only.** +**STOP.** AskUserQuestion: one tool_use per issue, no batching, even obvious fixes. Recommend + WHY; wait for approval before changing the plan. Zero findings: state "No issues, moving on" and proceed. No code changes; review only. {{SECTION:review-sections}} ## Section self-check (before you finish) -You ran a carved skill. The Section index above named `sections/review-sections.md` -as the source of truth for the 11-section deep review, the required outputs, and the -review report. Confirm you issued a Read for it and executed every section from the -file, not from memory. If you produced the Completion Summary or wrote the review -report without Reading that section, STOP, Read it now, and redo the review from the -source of truth. +Read and execute every section and output in `sections/review-sections.md`. +If summaries/reports came first, STOP, Read it and redo the review. +Before summaries, review logs or next-step menus, run approval check 0 below. {{EXIT_PLAN_MODE_GATE}} diff --git a/plan-design-review/SKILL.md b/plan-design-review/SKILL.md index 401ac7dc9..794bb4687 100644 --- a/plan-design-review/SKILL.md +++ b/plan-design-review/SKILL.md @@ -1179,19 +1179,20 @@ descriptions of what 10/10 looks like. Confirm you Read the review section the Section index named, and executed all 7 design passes, the required outputs, and the review report in full. If you produced findings or the review report from memory without Reading `sections/review-sections.md`, stop and Read it now. -Before the completion summary, review log, next-step menu, or ExitPlanMode, -reconcile every proposed fix with the exact user decision for that issue. A new -accepted task, raised score, or "decision made" needs its individual approval; -an explicit deferral stays unresolved. Existing accepted requirements need no repeat approval. -Keep the preamble's authorized auto-decisions distinct and record their per-issue basis. -If an issue has no such decision, ask about it now and wait; a next-step answer -cannot approve it. If a draft applied it, return that change to pending before continuing. +Before summaries, review logs or next-step menus, run approval check 0 below. ## EXIT PLAN MODE GATE (BLOCKING) Before calling ExitPlanMode, run this self-check. If any item fails, do the missing work — do NOT call ExitPlanMode: +0. Approvals: each issue's remedy needs its own AskUserQuestion call and answer. + Never group distinct issues. DESIGN.md tokens and navigation are not approval. + Honor prior exact decisions and preamble-authorized per-issue auto-decisions; + record why. Deferrals remain unresolved. + If missing, reset drafts to pending, ask and wait. After answers or resets, + refresh the plan, report and review log; rerun this gate. + 1. Read the plan file with the Read tool (after your most recent write to it). 2. Confirm the LAST `## ` heading in the file is `## GSTACK REVIEW REPORT`. In-body prose that mentions "outside voice", "codex findings", or similar diff --git a/plan-design-review/SKILL.md.tmpl b/plan-design-review/SKILL.md.tmpl index 39ce92905..e772a4877 100644 --- a/plan-design-review/SKILL.md.tmpl +++ b/plan-design-review/SKILL.md.tmpl @@ -314,12 +314,6 @@ descriptions of what 10/10 looks like. Confirm you Read the review section the Section index named, and executed all 7 design passes, the required outputs, and the review report in full. If you produced findings or the review report from memory without Reading `sections/review-sections.md`, stop and Read it now. -Before the completion summary, review log, next-step menu, or ExitPlanMode, -reconcile every proposed fix with the exact user decision for that issue. A new -accepted task, raised score, or "decision made" needs its individual approval; -an explicit deferral stays unresolved. Existing accepted requirements need no repeat approval. -Keep the preamble's authorized auto-decisions distinct and record their per-issue basis. -If an issue has no such decision, ask about it now and wait; a next-step answer -cannot approve it. If a draft applied it, return that change to pending before continuing. +Before summaries, review logs or next-step menus, run approval check 0 below. {{EXIT_PLAN_MODE_GATE}} diff --git a/scripts/resolvers/review.ts b/scripts/resolvers/review.ts index 7c181396a..78d91474e 100644 --- a/scripts/resolvers/review.ts +++ b/scripts/resolvers/review.ts @@ -177,13 +177,25 @@ there — the user then sees a plan whose review report is not at the bottom and (correctly) rejects it.`; } -export function generateExitPlanModeGate(_ctx: TemplateContext): string { +export function generateExitPlanModeGate(ctx: TemplateContext): string { + // These reviews reconcile issue decisions before summaries and logging. + // Writing a report or choosing the review's approach cannot supply approval. + const noApproval = ctx.skillName === 'plan-design-review' + ? 'DESIGN.md tokens and navigation' : 'Setup, mode, approach and navigation'; + const approvals = ['plan-design-review', 'plan-ceo-review'].includes(ctx.skillName) ? `0. Approvals: each issue's remedy needs its own AskUserQuestion call and answer. + Never group distinct issues. ${noApproval} are not approval. + Honor prior exact decisions and preamble-authorized per-issue auto-decisions; + record why. Deferrals remain unresolved. + If missing, reset drafts to pending, ask and wait. After answers or resets, + refresh the plan, report and review log; rerun this gate. + +` : ''; return `## EXIT PLAN MODE GATE (BLOCKING) Before calling ExitPlanMode, run this self-check. If any item fails, do the missing work — do NOT call ExitPlanMode: -1. Read the plan file with the Read tool (after your most recent write to it). +${approvals}1. Read the plan file with the Read tool (after your most recent write to it). 2. Confirm the LAST \`## \` heading in the file is \`## GSTACK REVIEW REPORT\`. In-body prose that mentions "outside voice", "codex findings", or similar does NOT count — only the structured \`## GSTACK REVIEW REPORT\` section diff --git a/test/autoplan-phase-order.test.ts b/test/autoplan-phase-order.test.ts index cca904bac..ef506f600 100644 --- a/test/autoplan-phase-order.test.ts +++ b/test/autoplan-phase-order.test.ts @@ -79,10 +79,10 @@ describe('autoplan phase execution checkpoints', () => { expect(intake).toContain('Do not prefetch future phase sections or review skills'); for (const phase of phases) { const section = read(`autoplan/sections/${phase}-phase.md.tmpl`); - expect(section).toMatch(/^Read \{\{AUTOPLAN_REVIEW_FILE:plan-[a-z-]+\}\} and its triggered sections in full/); + expect(section).toMatch(/^Before dispatch, fully Read \{\{AUTOPLAN_REVIEW_FILE:plan-[a-z-]+\}\} \+ triggered sections/); const load = section.split('**Override rules:**')[0]!; - expect(load).toContain('Before dispatch, record successful Read start/end/total ranges'); - expect(load).toContain('fetch gaps to EOF'); + expect(load).toContain('record successful start/end/total ranges'); + expect(load).toContain('fill gaps to EOF'); expect(load).toContain('Load skip-listed sections; skip execution'); } }); @@ -102,17 +102,21 @@ describe('autoplan phase execution checkpoints', () => { expect(dispatch).toContain('all criteria + plan'); expect(dispatch).toContain('if its schema exposes it'); expect(dispatch).toContain('isAsync: true'); - expect(dispatch).toContain('Claude Code: immediately end this response with'); - expect(dispatch).toContain('Do no more tool calls or review work until that ID'); - expect(dispatch).toContain('For completed native reviews, match INPUT phase/hash'); - expect(dispatch).toContain('retry this dispatch once, then use failure policy'); - expect(dispatch).toContain('Other hosts: await that ID'); + expect(dispatch).toContain('Claude Code: end response immediately'); + expect(dispatch).toContain('No further tool calls/review until'); + expect(dispatch).toContain('Completed-native INPUT must match snapshot phase/hash'); + expect(dispatch).toContain('Retry invalid input once; then failure policy if still invalid'); + expect(dispatch).toContain('Other hosts await that ID'); expect(dispatch).toContain("Then outside → this phase's review ONLY"); expect(dispatch).toContain('No inline substitute; apply failure policy'); // Provider preflight, timeout and native fallback remain at every call. expect(section).toContain('Outer tool timeout: 720000ms'); expect(section).toContain('disabled → skip outside. Both retain the native pass.'); expect(section).toContain(`{{OUTSIDE_PROVENANCE:${phase}}}`); + expect(section).toContain('Missing/disabled'); + expect(section).toContain('N/A'); + expect(section).toContain('primary cannot replace'); + expect(section).toContain(phase === 'design' ? 'not CONFIRMED' : 'never CONFIRMED'); }); } @@ -131,6 +135,7 @@ describe('autoplan phase execution checkpoints', () => { expect(contract).toContain('Pending is not unavailable'); expect(contract).toContain('Time/context pressure or your own review never permits\nskipping native passes or required sections'); expect(contract).toContain('Never read raw agent transcripts'); + expect(tmpl).toContain('LOG each decision and amend ALL accepted obligations in `Implementation plan` in the SAME edit'); }); test('each completed phase announces only after persisted full outputs and settled reviewers', () => { @@ -141,18 +146,23 @@ describe('autoplan phase execution checkpoints', () => { expect(barrier).toBeGreaterThan(-1); expect(barrier).toBeLessThan(announcement); const checkpoint = section.slice(barrier, announcement); - expect(checkpoint).toContain('Require full load ranges'); + expect(checkpoint).toContain('Require full skill/section Read ranges'); expect(checkpoint).toContain('successful writes'); expect(checkpoint).toContain('terminal reviewers'); expect(checkpoint).toContain('matched INPUT for completed native reviews'); expect(checkpoint).toContain('(unavailable/disabled allowed)'); - expect(checkpoint).toContain('this check result'); - expect(checkpoint).toContain('actual assistant message'); - expect(checkpoint).toContain('Edit accepted changes into `Implementation plan`'); - expect(checkpoint).toContain('Verify returned text against decisions'); - expect(checkpoint).toContain('taste:\npending final approval'); - expect(checkpoint).toContain('unresolved User Challenges: retain original direction'); - expect(checkpoint).toContain('Only then emit'); + expect(checkpoint).toContain('successful writes/check'); + expect(checkpoint).toContain(phase === 'eng' + ? 'announce completion and proceed to final synthesis/approval' + : 'announce completion AND load/create/dispatch the next phase'); + expect(checkpoint).toContain('record EVERY accepted'); + expect(checkpoint).toContain('obligation (split bundles) → exact `Implementation plan` text. Edit omissions'); + expect(checkpoint).toContain('Reread full phase `Review record`'); + expect(checkpoint).toContain('Verify each mapping in readback'); + expect(checkpoint).toContain('bytes prove neither completeness nor correctness'); + expect(checkpoint).toContain('Taste: provisional'); + expect(checkpoint).toContain('unresolved User Challenges: original direction'); + expect(checkpoint).toContain('Only then announce'); } }); @@ -209,7 +219,7 @@ describe('autoplan current implementation-plan identity', () => { expect(section).toContain('Reads `nativePromptPath` to EOF'); expect(section).toContain(`Outside prompt: inline the full contents of <${phase.toUpperCase()}_INPUT>`); expect(section).toContain(`check ${phase} "" "<${phase.toUpperCase()}_INPUT>" changed`); - expect(section).toContain('Use `unchanged` only if no implementation changes were accepted'); + expect(section).toContain('No accepted implementation change: `unchanged` + reason'); expect(section).toContain('Report/task edits do not count'); expect(section).not.toContain(''); expect(section).not.toContain(''); diff --git a/test/ceo-count-s-terminals.test.ts b/test/ceo-count-s-terminals.test.ts new file mode 100644 index 000000000..75720e43e --- /dev/null +++ b/test/ceo-count-s-terminals.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { ceoFirstReviewAUQ, ceoStep0Boundary, hasNativePlanTerminal, isQuestionlessNativePlanExit, nativePlanCallFingerprint, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import { isCeoCompletionHandoff, pickCeoCompletionHandoff } from './helpers/ceo-completion-handoff'; +import type { NativePlanQuestionCall, PlanCountTranscript } from './helpers/plan-count-transcript'; +import distinct from './fixtures/ceo-count-s-distinct.json'; +import paired from './fixtures/ceo-count-s-paired.json'; + +const fp = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, false); +function replay(calls: NativePlanQuestionCall[]) { + let started = false; + const setup = new Set(); const administrative = new Set(); let review = 0; + for (const call of calls) { + const fingerprint = fp(call); + const phase = planCountQuestionPhase(fingerprint, started, ceoStep0Boundary, ceoFirstReviewAUQ, undefined, isCeoCompletionHandoff); + if (phase.administrative) administrative.add(fingerprint.signature); + else if (phase.preReview) setup.add(fingerprint.signature); + else review++; + started = phase.reviewStarted; + } + return { setup, administrative, review }; +} +function transcript(capture: typeof distinct | typeof paired): PlanCountTranscript { + return { status: 'ready', calls: structuredClone(capture.calls) as NativePlanQuestionCall[], + assistantMessages: [], planReadyRequests: structuredClone(capture.planReadyRequests) }; +} +function withReport(capture: typeof distinct | typeof paired, run: (file: string, start: number) => void) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-s-terminal-')); + const file = path.join(dir, 'report.md'); + fs.writeFileSync(file, capture.report); + fs.utimesSync(file, capture.reportMtimeMs / 1000, capture.reportMtimeMs / 1000); + try { run(file, capture.reportMtimeMs - 1000); } + finally { fs.rmSync(dir, { recursive: true, force: true }); } +} + +describe('captured S native CEO completion gates', () => { + test('setup-only exit fails promptly without relaxing report freshness for positive coverage', () => { + const t = transcript(distinct); const result = replay(t.calls); + expect(result.setup.size).toBe(4); expect(result.review).toBe(0); + withReport(distinct, (file, start) => { + expect(isQuestionlessNativePlanExit(t, file, start, distinct.screen, result.setup)).toBe(true); + expect(isQuestionlessNativePlanExit(t, file, start, distinct.screen)).toBe(false); + expect(hasNativePlanTerminal(t, file, start, 'plan_ready')).toBe(false); + for (const mutate of [ + (v: PlanCountTranscript) => { v.calls[0]!.answered = false; }, + (v: PlanCountTranscript) => { v.calls[0]!.failed = true; }, + (v: PlanCountTranscript) => { v.calls[0]!.sessionId = 'foreign'; }, + (v: PlanCountTranscript) => { v.calls[0]!.answeredAt = 'invalid'; }, + (v: PlanCountTranscript) => { v.calls[0]!.answeredAt = v.planReadyRequests![0]!.timestamp; }, + (v: PlanCountTranscript) => { v.calls[0]!.answers = {}; }, + (v: PlanCountTranscript) => { v.calls[0]!.unansweredQuestionIndices = [0]; }, + ]) { + const changed = structuredClone(t); mutate(changed); + expect(isQuestionlessNativePlanExit(changed, file, start, distinct.screen, result.setup)).toBe(false); + } + const incomplete = new Set(result.setup); incomplete.delete(fp(t.calls[0]!).signature); + expect(isQuestionlessNativePlanExit(t, file, start, distinct.screen, incomplete)).toBe(false); + }); + }); + + test('paired review retains two issue approvals and excludes only the completed Eng menu', () => { + const t = transcript(paired); const before = structuredClone(t); const result = replay(t.calls); + expect(result.setup.size).toBe(2); expect(result.review).toBe(2); expect(result.administrative.size).toBe(1); + const pending = structuredClone(t.calls.at(-1)!); pending.answered = false; delete pending.answers; + expect(pickCeoCompletionHandoff(fp(pending))).toBe(2); + pending.questions[0]!.options.reverse(); expect(pickCeoCompletionHandoff(fp(pending))).toBe(1); + expect(t).toEqual(before); + withReport(paired, (file, start) => { + expect(hasNativePlanTerminal(t, file, start, 'plan_ready', result.administrative)).toBe(true); + expect(hasNativePlanTerminal(t, file, start, 'plan_ready')).toBe(false); + expect(isQuestionlessNativePlanExit(t, file, start, paired.screen, result.setup)).toBe(false); + }); + }); + + test('the same menu cannot hide a new obligation, ambiguous gate, or unverified answer', () => { + const base = transcript(paired).calls.at(-1)!; + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('It\'s', 'That might become'); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question += ' First repair authorization.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = 'Example: ' + c.questions[0]!.question; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.question = c.questions[0]!.question.replace('CLEAN', 'CLEAN once tests pass'); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.description += ' Remove the owner check.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.description += ' Change the guarantee to permit old results.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.description += ' Tests remain unresolved.'; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options[1]!.label = 'Fix the missing assertion'; }, + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { c.answered = false; }, + ]) { + const call = structuredClone(base); mutate(call); + call.answers = { [call.questions[0]!.question]: call.questions[0]!.options[0]!.label }; + expect(isCeoCompletionHandoff(fp(call))).toBe(false); + } + const call = structuredClone(base); call.answers = { [call.questions[0]!.question]: 'First repair the missing test' }; + expect(isCeoCompletionHandoff(fp(call))).toBe(false); + const pending = structuredClone(base); pending.answered = false; + expect(pickCeoCompletionHandoff({ ...fp(pending), signature: 'foreign' })).toBeNull(); + }); +}); diff --git a/test/ceo-section-loading-fixture.test.ts b/test/ceo-section-loading-fixture.test.ts index 59a2bb284..b4ac3bad9 100644 --- a/test/ceo-section-loading-fixture.test.ts +++ b/test/ceo-section-loading-fixture.test.ts @@ -207,3 +207,56 @@ describe('CEO R report requirement weakening remains rejected', () => { expect(hasStaleFillRaceFinding(report)).toBe(false); }); }); + +// Exact S output names the original-contract defect but proposes an ineffective +// guard. Detection and remedy correctness remain separate assertions. +describe('S native same-finding ordered trace', () => { + const report = require('node:fs').readFileSync(require('node:path').join(import.meta.dir, 'fixtures/ceo-section-s-trace-report.md'), 'utf8'); + const finding = report.slice(report.indexOf('### Critical Finding: Stale Re-insertion After Write Invalidation'), report.indexOf('### State Machine: Cache Entry')); + test('recognizes the exact delivered S finding without certifying its remedy', () => { + expect(hasStaleFillRaceFinding(report)).toBe(true); + expect(hasStaleFillRaceFinding(finding)).toBe(true); + expect(report).toContain('if (cache.get(key) === undefined)'); + }); + test('the same ordered evidence tolerates whitespace and a consistent key name', () => { + expect(hasStaleFillRaceFinding(finding.replaceAll('(key', '(accountKey').replaceAll(' t', ' t'))).toBe(true); + expect(hasStaleFillRaceFinding(finding.replaceAll('(key', '($key'))).toBe(true); + }); + test.each([ + ['optional single-flight label', finding.replace('single-flight → ', '')], + ['old/stale value vocabulary', finding.replace('OLD snapshot', 'stale value').replace('OLD_VALUE', 'STALE_VALUE').replace('stale value re-inserted', 'old snapshot refilled').replace('next readProfile', 'subsequent readProfile')], + ['prose payload vocabulary', finding.replace('OLD_VALUE', 'old value').replace('returns stale value', 'returns old snapshot')], + ['ASCII arrows and compact spacing', finding.replaceAll(' → ', '->').replaceAll(' ← ', '<-')], + ['trace keyword case', finding.replace(/t[1-6]:[^\n]*/g, (event: string) => event.toLowerCase())], + ['call whitespace and optional suspension annotation', finding.replaceAll('(key)', '( key )').replace('(key, OLD_VALUE)', '( key , OLD_VALUE )').replaceAll(' (suspends)', '')], + ])('accepts equivalent %s', (_name, text) => expect(hasStaleFillRaceFinding(text)).toBe(true)); + test.each([ + ['only the trace', finding.slice(finding.indexOf('```'), finding.indexOf('```', finding.indexOf('```') + 3) + 3)], + ['quoted whole finding', finding.split('\n').map((line: string) => '> ' + line).join('\n')], + ['indented whole finding', finding.split('\n').map((line: string) => ' ' + line).join('\n')], + ['source format preface', 'Example of report format:\n' + finding], + ['outer fenced source', '````text\n' + finding + '\n````'], + ['missing independent violation', finding.replace('The proposed wrapper violates this invariant.', '')], + ['negated independent violation', finding.replace('The proposed wrapper violates this invariant.', 'The proposed wrapper does not violate this invariant.')], + ['unrelated heading', finding.replace('### Critical Finding:', '### Example:')], + ['separate named finding', finding.replace('Race sequence', '### Another finding\nRace sequence')], + ['separate bold finding', finding.replace('Race sequence', '**HIGH FINDING — unrelated issue**\nRace sequence')], + ['missing write completion', finding.replace('DB write completes', 'DB write remains pending')], + ['no invalidation', finding.replace('cache.delete(key) → writeProfile returns', 'cache.get(key) → writeProfile returns')], + ['missing late old fill', finding.replace('cache.set(key, OLD_VALUE)', 'cache.set(key, NEW_VALUE)')], + ['different filled key', finding.replace('cache.set(key, OLD_VALUE)', 'cache.set(otherKey, OLD_VALUE)')], + ['case-distinct filled key', finding.replace('cache.set(key, OLD_VALUE)', 'cache.set(KEY, OLD_VALUE)')], + ['different later key', finding.replace('next readProfile(key)', 'next readProfile(otherKey)')], + ['only original pending reader', finding.replace('next readProfile(key)', 'original pending readProfile(key)')], + ['later reader misses', finding.replace('cache HIT → returns stale value', 'cache MISS → returns committed value')], + ['nonviolating trace', finding.replace('← INVARIANT VIOLATED', '← INVARIANT PRESERVED')], + ['reverse order labels', finding.replace('t3:', 't4:').replace('t4: DB read', 't3: DB read')], + ['missing event', finding.replace(/^.*t4:.*\n/m, '')], + ['unclosed trace', finding.replace('```\n\nThe plan says', '\nThe plan says')], + ['source-code trace fence', finding.replace('```\n t1:', '```javascript\n t1:')], + ['split traces', finding.replace(' t4:', '```\n\n```\n t4:')], + ['accepted stale trace', finding + '\nThis stale-read behavior is accepted; no guard is required.\n'], + ['allowed new-reader consequence', finding + '\nA subsequent stale read is permitted by the amended contract.\n'], + ['explicit defect dismissal', finding + '\nThis is not a defect; no fix is required.\n'], + ])('rejects %s', (_name, text) => expect(hasStaleFillRaceFinding(text)).toBe(false)); +}); diff --git a/test/eng-devex-s-count.test.ts b/test/eng-devex-s-count.test.ts new file mode 100644 index 000000000..d1c3b769d --- /dev/null +++ b/test/eng-devex-s-count.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, test } from 'bun:test'; +import actual from './fixtures/eng-devex-s-first-calls.json'; +import retry from './fixtures/eng-devex-s-retry-calls.json'; +import { nativePlanCallFingerprint, engFirstReviewAUQ, engStep0Boundary, engSetupAUQ, planCountQuestionPhase } from './helpers/claude-pty-runner'; +import { isDevexReviewIssue } from './helpers/devex-count-fixture'; +import type { NativePlanQuestionCall } from './helpers/plan-count-transcript'; +const fp = (call: NativePlanQuestionCall) => nativePlanCallFingerprint(call, 0, true); +const copy = (call: unknown) => structuredClone(call) as NativePlanQuestionCall; +function changeQuestion(call: NativePlanQuestionCall, transform: (s: string) => string) { + const q = call.questions[0]!; const answer = call.answers![q.question]!; + q.question = transform(q.question); call.answers = { [q.question]: answer }; return call; +} + +describe('S completed native review accounting', () => { + test('DX keeps all five real approvals including unnamed package file and CI/TTHW contradiction', () => { + expect(actual.devex.calls.map(call => isDevexReviewIssue(fp(copy(call))))).toEqual([true, true, true, true, true]); + }); + test('retry keeps empathy/scope setup and all distinct issues/TODOs', () => { + expect(retry.devex.calls.map(call => isDevexReviewIssue(fp(copy(call))))).toEqual([false, true, true, true, true, true]); + let started = false; + expect(retry.eng.calls.map(call => { const phase = planCountQuestionPhase(fp(copy(call)), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); started = phase.reviewStarted; return phase.preReview; })).toEqual([true, false, false, false, false, false, false]); + }); + test('Eng scope remains setup; first architecture remedy opens review including the later TODO', () => { + let started = false; + const phases = actual.eng.calls.map(call => { + const phase = planCountQuestionPhase(fp(copy(call)), started, engStep0Boundary, engFirstReviewAUQ, engSetupAUQ); + started = phase.reviewStarted; return phase.preReview; + }); + expect(phases).toEqual([true, false, false, false, false, false]); + expect(engFirstReviewAUQ(fp(copy(actual.eng.calls[0])))).toBe(false); + expect(engFirstReviewAUQ(fp(copy(actual.eng.calls[1])))).toBe(true); + }); + for (const [name, original, predicate] of [ + ['DX quickstart', actual.devex.calls[0], isDevexReviewIssue], + ['DX CI/TTHW', actual.devex.calls[1], isDevexReviewIssue], + ['Eng architecture', actual.eng.calls[1], engFirstReviewAUQ], + ['DX retry CI repair', retry.devex.calls[2], isDevexReviewIssue], + ] as const) { + test(`${name} requires complete native offered-answer identity`, () => { + for (const mutate of [ + (c: NativePlanQuestionCall) => { c.failed = true; }, + (c: NativePlanQuestionCall) => { c.answered = false; }, + (c: NativePlanQuestionCall) => { c.unansweredQuestionIndices = [0]; }, + (c: NativePlanQuestionCall) => { delete c.unansweredQuestionIndices; }, + (c: NativePlanQuestionCall) => { c.answers = {}; }, + (c: NativePlanQuestionCall) => { c.answers = { [c.questions[0]!.question]: 'Unrelated answer' }; }, + (c: NativePlanQuestionCall) => { c.questions[0]!.multiSelect = true; }, + (c: NativePlanQuestionCall) => { c.questions.push(copy(original).questions[0]!); }, + (c: NativePlanQuestionCall) => { c.questions[0]!.options.push({ ...c.questions[0]!.options[0]! }); }, + ]) { const c = copy(original); mutate(c); expect(predicate(fp(c))).toBe(false); } + const duplicateId = changeQuestion(copy(original), s => s + ' '); expect(predicate(fp(duplicateId))).toBe(false); + const foreign = fp(copy(original)); foreign.signature = 'foreign:call'; expect(predicate(foreign)).toBe(false); + const screen = fp(copy(original)); delete screen.nativeCall; + // The legacy text-only API already recognizes the retry's full CI prose. + // This change adds no text-only credit; the counter consumes native calls. + expect(predicate(screen)).toBe(name === 'DX retry CI repair'); + }); + test(`${name} does not convert setup or navigation into an approval`, () => { + for (const suffix of ['mode', 'setup', 'next-steps', 'scope', 'routing', 'prerequisite']) { + const c = changeQuestion(copy(original), s => s.replace(/]+>/, ``)); + expect(predicate(fp(c))).toBe(false); + } + const c = changeQuestion(copy(original), s => name === 'DX retry CI repair' + ? s.replace(/^.*$/m, 'Which benchmark tier should we use? ') + : s.replace(/How should [^?]+\?/i, 'Should we begin the review?')); + expect(predicate(fp(c))).toBe(false); + }); + } + test('DX does not count resolved or generic benchmark recaps', () => { + expect(isDevexReviewIssue(fp(changeQuestion(copy(actual.devex.calls[0]), s => s.replace("doesn't exist", 'already exists'))))).toBe(false); + expect(isDevexReviewIssue(fp(changeQuestion(copy(actual.devex.calls[0]), s => s.replace('quickstart points to', 'quickstart no longer points to'))))).toBe(false); + expect(isDevexReviewIssue(fp(changeQuestion(copy(actual.devex.calls[1]), s => s.replace('these are mutually exclusive', 'these are not mutually exclusive'))))).toBe(false); + expect(isDevexReviewIssue(fp(changeQuestion(copy(actual.devex.calls[1]), s => s.replace('with no skip path', 'with a working skip path'))))).toBe(false); + }); + test('retry benchmark confirmation or resolved target cannot count as a new repair', () => { + expect(isDevexReviewIssue(fp(changeQuestion(copy(retry.devex.calls[2]), s => s.replace('target unreachable', 'target reachable'))))).toBe(false); + const c = copy(retry.devex.calls[2]); c.questions[0]!.options[0]!.label = 'Keep the existing target (Recommended)'; c.answers = { [c.questions[0]!.question]: c.questions[0]!.options[0]!.label }; + expect(isDevexReviewIssue(fp(c))).toBe(false); + }); + test('Eng requires a concrete asserted defect and a direct repair decision', () => { + const c = copy(actual.eng.calls[1]); c.questions[0]!.header = 'Scope'; expect(engFirstReviewAUQ(fp(c))).toBe(false); + expect(engFirstReviewAUQ(fp(changeQuestion(copy(actual.eng.calls[1]), s => s.replace('is a race condition', 'is not a race condition'))))).toBe(false); + expect(engFirstReviewAUQ(fp(changeQuestion(copy(actual.eng.calls[1]), s => s.replace('Architecture: ', 'Architecture: It is false that '))))).toBe(false); + expect(engFirstReviewAUQ(fp(changeQuestion(copy(actual.eng.calls[1]), s => s.replace('is a race condition', 'is a design preference'))))).toBe(false); + expect(engFirstReviewAUQ(fp(changeQuestion(copy(actual.eng.calls[1]), s => s.replace('shared mutable AuthCache via module-level export', 'a mutable RequestRegistry'))))).toBe(true); + }); +}); diff --git a/test/fixtures/ceo-count-s-distinct.json b/test/fixtures/ceo-count-s-distinct.json new file mode 100644 index 000000000..eacf4efbc --- /dev/null +++ b/test/fixtures/ceo-count-s-distinct.json @@ -0,0 +1,137 @@ +{ + "provenance": "Exact S native calls/report, output-only captured audit; raw paid outcome remains unchanged", + "calls": [ + { + "sessionId": "904d2ca6-ef47-4207-9d96-a0574a3c1e8c", + "toolUseId": "toolu_01AjVd74DRgeL7fJd7ANoViE", + "questions": [ + { + "question": "D1 \u2014 Add gstack skill routing rules to CLAUDE.md? ", + "header": "Skill routing", + "multiSelect": false, + "options": [ + { + "label": "Add routing rules (Recommended)", + "description": "Append skill routing rules to CLAUDE.md so gstack auto-invokes the right skill for product brainstorming, architecture review, QA, ship, etc. One-time setup; commits the change." + }, + { + "label": "No thanks, skip", + "description": "Invoke skills manually. You can re-enable later with gstack-config set routing_declined false." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 \u2014 Add gstack skill routing rules to CLAUDE.md? ": "Add routing rules (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:10.961Z" + }, + { + "sessionId": "904d2ca6-ef47-4207-9d96-a0574a3c1e8c", + "toolUseId": "toolu_01RZt9CzeEn1uHb3ysCMSBYT", + "questions": [ + { + "question": "D2 \u2014 No design doc found for this branch. Run /office-hours first, or proceed with standard review? ", + "header": "Design doc", + "multiSelect": false, + "options": [ + { + "label": "Skip \u2014 proceed with standard review (Recommended)", + "description": "The plan has already been reviewed in full (HOLD SCOPE, 6 findings including 1 CRITICAL SQL injection). No design doc needed to proceed." + }, + { + "label": "Run /office-hours first", + "description": "Produces a structured problem statement and premise challenge. Takes ~10 min. Recommended if the problem statement is still unclear." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 \u2014 No design doc found for this branch. Run /office-hours first, or proceed with standard review? ": "Skip \u2014 proceed with standard review (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:23.004Z" + }, + { + "sessionId": "904d2ca6-ef47-4207-9d96-a0574a3c1e8c", + "toolUseId": "toolu_01UrJPhu4G8sP2fqqrXemPqo", + "questions": [ + { + "question": "D3 \u2014 Which implementation approach should this plan adopt? ", + "header": "Approach", + "multiSelect": false, + "options": [ + { + "label": "Approach B: Proper Integration (Recommended)", + "description": "Fix SQL injection (parameterized query), register in WebhookDispatcher, wrap email in try/rescue, batch order fetch, add 5 unit tests. Effort: human ~1 day / CC ~20 min. Completeness: 9/10." + }, + { + "label": "Approach A: Minimal Patch", + "description": "Fix ONLY the SQL injection; leave N+1, no email error handling, no tests, WebhookDispatcher bypass intact. Effort: human ~2h / CC ~5 min. Completeness: 5/10 \u2014 requires TODOS.md entries for deferred items." + }, + { + "label": "Approach C: Async Job Queue", + "description": "Scrap inline handler in favor of an async job queue. Decouples payment update from email. Adds infrastructure dependency. Effort: human ~1 week / CC ~2h. Major scope increase. Completeness: 10/10." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 \u2014 Which implementation approach should this plan adopt? ": "Approach B: Proper Integration (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:31.030Z" + }, + { + "sessionId": "904d2ca6-ef47-4207-9d96-a0574a3c1e8c", + "toolUseId": "toolu_01AGy4HT1yXxFMDVLHLh68s9", + "questions": [ + { + "question": "D4 \u2014 Which review mode should govern the plan? ", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "HOLD SCOPE \u2014 make it bulletproof (Recommended)", + "description": "The plan\u2019s scope is right (webhook handler, DB update, email notification). Review with maximum rigor: fix the SQL injection, error handling, tests, N+1, and architecture smell. No expansions surfaced. Note: options differ in kind, not coverage \u2014 no completeness score." + }, + { + "label": "SELECTIVE EXPANSION \u2014 hold scope + cherry-pick", + "description": "Same rigorous baseline fixes, but also surface expansion opportunities (async email queue, retry UI, payment analytics) as individual opt-in decisions." + }, + { + "label": "SCOPE REDUCTION \u2014 strip to essentials", + "description": "Find the minimum viable version: SQL injection fix + DB update only. Defer email notification to a follow-up PR." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 \u2014 Which review mode should govern the plan? ": "HOLD SCOPE \u2014 make it bulletproof (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:39.049Z" + } + ], + "planReadyRequests": [ + { + "sessionId": "904d2ca6-ef47-4207-9d96-a0574a3c1e8c", + "toolUseId": "toolu_01GDdwqBxo2MXzptfCkrWVD9", + "timestamp": "2026-09-09T05:45:06.139Z", + "failed": false + } + ], + "report": "# CEO Plan Review: Payment Processing Integration\n\n**Branch:** main | **Mode:** HOLD SCOPE | **Date:** 2026-09-09\n**Reviewer:** /plan-ceo-review\n\n---\n\n## Context\n\nThis plan adds a new `StripePaymentWebhookHandler` class to handle Stripe\n`payment_intent.succeeded` webhook events. The handler updates a user's\npayment status in the database and fires a notification email. Existing\ncontracts (signature verification, event deduplication, per-user transaction\nlocking, feature flag/rollback path) are retained unchanged.\n\n**Review verdict up front:** The plan contains a confirmed SQL injection\nvulnerability and is not safe to ship in its current form. HOLD SCOPE\nreview below maps every defect and required fix.\n\n---\n\n## PRE-REVIEW SYSTEM AUDIT\n\n- **Repo:** gstack-plan-count-CoF0hZ (fixture \u2014 no remote, single commit)\n- **Branch:** main, 1 commit (`ac99131 Seed review plan`)\n- **Open stash:** none\n- **TODOs/FIXMEs in codebase:** none (fixture repo, no implementation files)\n- **Design doc:** none found\n- **Handoff note:** none found\n- **Prior learnings:** none (fresh environment)\n- **Brain context:** none (fresh project)\n\n---\n\n## Step 0: Nuclear Scope Challenge\n\n### 0A. Premise Challenge\n\n**Is this the right problem to solve?**\nYes \u2014 Stripe webhook handling for payment confirmation is a legitimate,\nnecessary integration. The plan correctly scopes to `payment_intent.succeeded`\nonly. The problem is real; the implementation has critical defects.\n\n**Actual user/business outcome:**\nWhen a user's payment succeeds, their account should be marked paid and they\nshould receive a confirmation email. The plan is the right shape; the\nimplementation details are wrong.\n\n**What happens if we do nothing?**\nPayment status updates don't happen automatically. Real pain point, not\nhypothetical.\n\n### 0B. Existing Code Leverage\n\nThe plan identifies these existing contracts:\n- Ingress middleware: signature verification \u2014 RETAINED\n- WebhookDispatcher: event routing \u2014 BYPASSED (architectural smell)\n- Payload adapter: userId extraction \u2014 RETAINED\n- Event-ID guard: deduplication \u2014 RETAINED\n- Per-user transaction lock \u2014 RETAINED\n- DB client with tracing \u2014 RETAINED\n- Mail client with tracing \u2014 RETAINED\n- Feature flag + rollback path \u2014 RETAINED\n\n**Key gap:** The plan bypasses `WebhookDispatcher` for \"namespace separation.\"\nThis creates a parallel webhook infrastructure path. The existing dispatcher\nis the natural registration point for this handler.\n\n### 0C. Dream State Mapping\n\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\nNo payment webhook --> New handler class --> Handler registered in\nhandler bypassing Dispatcher WebhookDispatcher,\n + raw SQL userId lookup parameterized queries,\n + inline email with async email queue,\n no error handling full test coverage,\n + N+1 query loop batch order fetch\n```\n\nThis plan moves toward the right outcome but in the wrong direction\narchitecturally \u2014 fragmented infrastructure, SQL injection, no tests.\n\n### 0C-bis. Implementation Approaches\n\n```\nAPPROACH A: Minimal Patch (as-written minus SQL injection fix)\n Summary: Add handler class as planned, fix ONLY the SQL injection;\n leave everything else (no tests, N+1, no email error handling).\n Effort: S (human: ~2h / CC: ~5 min)\n Risk: High \u2014 no tests, N+1 in production, email errors silently fail\n Pros: - Smallest diff, fastest to ship\n - SQL injection fixed (the one hard blocker)\n Cons: - No regression coverage for a payment flow\n - Email errors can cause lost notifications silently\n - N+1 hits DB hard at scale\n Reuses: Existing DB/mail clients, existing guards\n\nAPPROACH B: Proper Integration (RECOMMENDED)\n Summary: Fix SQL injection (parameterized query), register handler in\n WebhookDispatcher instead of bypassing it, wrap email in\n try/rescue with correlated error log, batch order fetch,\n add minimal handler unit tests.\n Effort: M (human: ~1 day / CC: ~20 min)\n Risk: Low\n Pros: - SQL injection eliminated via parameterized query\n - WebhookDispatcher registration keeps infrastructure unified\n - Email failures caught, logged, surfaced to existing mail alert\n - N+1 eliminated with single batch query\n - Tests catch regressions in future\n Cons: - Slightly larger diff than Approach A\n - Requires understanding WebhookDispatcher registration pattern\n Reuses: WebhookDispatcher, existing DB/mail clients, existing guards\n\nAPPROACH C: Scrap and Redesign\n Summary: Scrap the inline handler in favor of an async job queue:\n webhook handler enqueues a job, worker processes payment update\n and email separately with retries.\n Effort: XL (human: ~1 week / CC: ~2h)\n Risk: Med (introduces new infrastructure dependency)\n Pros: - Decouples payment update from email failure completely\n - Scales to high event volume without blocking webhook threads\n - Email retries handled by job queue, not manual runbook\n Cons: - Major scope increase vs the stated plan\n - Adds infrastructure dependency (job queue)\n - Overkill if event volume is low\n Reuses: Existing job queue if one exists; otherwise new dependency\n```\n\n**RECOMMENDATION:** Approach B. It fixes all critical defects without\narchitectural overreach. Approach A is the floor (SQL injection is a\nhard blocker). Approach C is the ceiling for a high-volume production\nsystem, but is out of scope for this plan.\n\n### 0F. Mode Selection\n\n**Recommended mode: HOLD SCOPE.**\n\nThis is not a greenfield feature where expansion would help \u2014 it is an\nintegration with confirmed security vulnerabilities. The plan's scope\nis correct (webhook handler, DB update, email notification). The\nimplementation has critical defects that must be fixed. Expanding scope\nwhile the foundation is broken would be counterproductive.\n\nMode: **HOLD SCOPE \u2014 make it bulletproof.**\n\n---\n\n## Section 1: Architecture & System Design\n\n### Finding 1.1 \u2014 SQL Injection (CRITICAL SECURITY)\n\nThe plan states explicitly:\n> \"The adapter forwards that external string unchanged. It does not cast,\n> escape, or SQL-sanitize it; a valid signature does not make it safe for SQL.\"\n> AND\n> \"The new endpoint reads `request.params.userId` directly into a raw SQL\n> fragment for the lookup query.\"\n\nThis is a confirmed SQL injection vulnerability. `userId` comes from Stripe\nevent metadata \u2014 it is an externally controlled string. Inserting it into a\nraw SQL fragment allows an attacker who can control Stripe event metadata\n(e.g., a rogue Stripe webhook sender who somehow forges a valid signature,\nOR a compromised upstream that populates metadata) to execute arbitrary SQL.\n\n**Fix required:** Use a parameterized query or ORM lookup. The userId should\nbe bound as a parameter, never interpolated into a SQL string.\n\n```\n# WRONG (as planned):\ndb.query(\"SELECT * FROM users WHERE id = '#{request.params.userId}'\")\n\n# RIGHT:\ndb.query(\"SELECT * FROM users WHERE id = $1\", [request.params.userId])\n# or ORM: User.find_by(id: request.params.userId)\n```\n\nThe plan's note that \"the adapter acknowledges missing, nil, or empty user_id\nwith HTTP 200 and an event-correlated warning before invoking this handler\"\ndoes NOT protect against SQL injection \u2014 it only gates on presence, not on\ncontent. A nonempty malicious value passes through.\n\n**Status:** BLOCKING. This plan cannot ship with a raw SQL fragment.\n\n### Finding 1.2 \u2014 Architectural Fragmentation (WebhookDispatcher Bypass)\n\nThe plan bypasses `WebhookDispatcher` for \"namespace separation.\" This creates\ntwo parallel webhook infrastructure paths: one through the dispatcher (all\nother event types), one through the new handler (payment_intent.succeeded).\n\nProblems:\n- Future engineers will look in WebhookDispatcher for payment handling and\n find nothing \u2014 operational confusion during incidents.\n- Namespace separation is achievable within WebhookDispatcher by registering\n the handler under a `Payments` namespace or module.\n- If the dispatcher has cross-cutting concerns (rate limiting, telemetry hooks,\n retry logic), the new handler bypasses them.\n\n**Fix required:** Register `StripePaymentWebhookHandler` in WebhookDispatcher\nrather than bypassing it. If WebhookDispatcher is the wrong pattern for this\nhandler's lifecycle, document why explicitly and mark the bypass as intentional.\n\n---\n\n## Section 2: Error Handling & Rescue Map\n\n### Finding 2.1 \u2014 Unhandled Email Failure (HIGH)\n\nThe plan states: \"Both happen inline; no error handling on the email leg.\"\n\nThe existing mail client \"rethrows exceptions unchanged.\" This means an email\ndelivery failure will throw an exception from the inline email call. Given that\nthe existing DB/mail clients rethrow, and the ingress wrapper catches DB\nexceptions and returns HTTP 500 (causing Stripe to retry), an email failure\nwill cause:\n\n```\n1. DB update succeeds (payment_status=paid, payment intent ID written)\n2. Email throws exception\n3. Exception propagates to ingress wrapper\n4. Ingress returns HTTP 500\n5. Stripe retries the event\n6. Event-ID dedup guard: if dedup is recorded AFTER the transaction commits,\n the event is now marked complete \u2014 Stripe retry gets HTTP 200 but no retry.\n IF the dedup guard records completion ONLY after the full handler returns\n successfully, then a retry would re-run the DB update (idempotent, same\n values) AND try the email again \u2014 could double-send.\n```\n\nThe plan says \"The existing event-ID dedup guard records completion only after\nthe database transaction commits; failed or rolled-back database attempts remain\nretryable.\" An email failure AFTER a DB commit means: DB is committed, dedup\nis NOT yet recorded (the commit happened, but the handler hasn't returned\nsuccessfully), Stripe retries, dedup check passes (not yet recorded), DB update\nruns again (idempotent), email attempted again. This could result in double\nemail delivery.\n\n**Fix required:** Wrap the email call in a try/rescue block. Log the failure\nwith a correlated event ID and user ID. Do NOT re-raise \u2014 return HTTP 200 so\nStripe does not retry. The existing mail client publishes failure rate to the\ndashboard and alert; catching the exception does NOT suppress that metric.\nThe incident runbook already handles failed notifications via the retry\nprocedure.\n\n```\n# Pattern:\nbegin\n mail_client.send_payment_confirmation(user_id: user.id, event_id: event.id)\nrescue MailDeliveryError => e\n logger.error(\"payment_confirmation_email_failed\", user_id: user.id,\n event_id: event.id, error: e.message)\n # Return success \u2014 Stripe should not retry for an email failure.\n # On-call will see this in the mail failure rate alert.\nend\n```\n\n**Status:** HIGH \u2014 can cause double email delivery and confusing Stripe retry\nbehavior.\n\n### Finding 2.2 \u2014 Error/Rescue Map (Complete)\n\n```\n\u250c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2510\n\u2502 Stripe \u2192 ingress \u2192 handler \u2502\n\u2502 \u2502\n\u2502 1. Signature invalid \u2192 HTTP 400 (existing) \u2502\n\u2502 2. Event type != succeeded \u2192 HTTP 200 (existing) \u2502\n\u2502 3. userId nil/empty \u2192 HTTP 200 + warning (existing) \u2502\n\u2502 4. Duplicate event ID \u2192 HTTP 200 (existing dedup) \u2502\n\u2502 5. DB lookup fails \u2192 HTTP 500 \u2192 Stripe retries \u2713 \u2502\n\u2502 6. User not found/deleted \u2192 HTTP 200 + log (existing) \u2502\n\u2502 7. DB update fails \u2192 HTTP 500 \u2192 Stripe retries \u2713 \u2502\n\u2502 8. Email fails (CURRENT) \u2192 HTTP 500 \u2192 Stripe retries \u2717 \u2502\n\u2502 (double-send risk) \u2502\n\u2502 8. Email fails (FIXED) \u2192 HTTP 200 + log \u2192 mail alert \u2713 \u2502\n\u2502 9. SQL injection (CURRENT) \u2192 arbitrary SQL execution \u2717 \u2502\n\u2502 9. Parameterized query (FIX)\u2192 safe lookup \u2713 \u2502\n\u2514\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2518\n```\n\n---\n\n## Section 3: Security\n\n### Finding 3.1 \u2014 SQL Injection (see Section 1.1, CRITICAL)\n\nNo additional mitigations exist in the plan. The plan explicitly documents\nthe vulnerability. This is the highest-priority finding.\n\n### Finding 3.2 \u2014 userId Trust Boundary\n\nThe plan correctly identifies that Stripe signature verification does not\nmake the `userId` metadata value safe for SQL. The same logic applies to\nany other use of `userId` downstream: logging, email templates, audit records.\nEnsure `userId` is logged as an opaque string, not interpolated into\nstructured formats that could be interpreted (e.g., log injection via newlines\nin `userId`).\n\n**Recommendation:** After parameterizing the SQL query, strip newlines from\n`userId` before logging. The ORM/parameterized approach handles SQL; log\nformatting should handle log injection separately.\n\n---\n\n## Section 4: Test Strategy\n\n### Finding 4.1 \u2014 No Tests (HIGH)\n\nThe plan states: \"None planned. We'll rely on the existing integration suite\ncatching regressions.\"\n\nThis is inadequate for a payment flow. The integration suite catches\nregressions in existing code paths \u2014 it will not cover the new handler's\nspecific behaviors:\n- What happens when `userId` is present but the user is deleted?\n- What happens when the DB update succeeds but email fails?\n- What does the handler do with a malformed `userId`?\n- Is the parameterized query correctly applied?\n\n**Fix required:** At minimum, add handler-level unit tests covering:\n1. Happy path: known userId, DB update succeeds, email succeeds \u2192 HTTP 200\n2. Unknown user: DB lookup returns nil \u2192 HTTP 200 + log, no email\n3. Email failure: DB update succeeds, email throws \u2192 HTTP 200 + log (not 500)\n4. DB failure: DB lookup throws \u2192 HTTP 500 (Stripe retries)\n5. SQL injection probe: `userId = \"' OR '1'='1\"` \u2192 parameterized query binds\n it safely, no injection\n\nThe plan notes that the manual staging checklist verifies the happy path.\nThat is not a substitute for automated tests that prevent regressions.\n\n---\n\n## Section 5: Performance\n\n### Finding 5.1 \u2014 N+1 Query (MEDIUM)\n\nThe plan states: \"Each webhook lookup hits the database for the user, then\nfetches each order in a loop.\"\n\nThis is a classic N+1 pattern. For a user with N orders, this fires N+1\nqueries per webhook event.\n\n**Fix required:** Batch-fetch orders in a single query with the user lookup,\nor use a JOIN. The exact fix depends on the ORM/query layer in use:\n\n```\n# N+1 (as planned):\nuser = db.find_user(user_id)\nuser.orders.each { |order_id| db.find_order(order_id) }\n\n# Batch (fixed):\nuser = db.find_user_with_orders(user_id)\n# Single query: SELECT users.*, orders.* FROM users\n# LEFT JOIN orders ON orders.user_id = users.id\n# WHERE users.id = $1\n```\n\nAt low webhook volume this is tolerable. At high volume (flash sale, billing\ncycle), this pattern hits the DB hard. Fix it now \u2014 it costs 5 minutes with\nCC and saves an incident later.\n\n---\n\n## Section 6: Data Flow Diagram\n\n```\n\u250c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2510\n\u2502 WEBHOOK DATA FLOW \u2502\n\u2502 \u2502\n\u2502 Stripe \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 Ingress Middleware \u2502\n\u2502 \u251c\u2500 Verify Stripe signature (HMAC) \u2502\n\u2502 \u2502 invalid \u2192 HTTP 400 \u2502\n\u2502 \u251c\u2500 Filter: payment_intent.succeeded only \u2502\n\u2502 \u2502 other types \u2192 HTTP 200 \u2502\n\u2502 \u2514\u2500 Log event_id, start timer \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 Payload Adapter \u2502\n\u2502 \u251c\u2500 Extract userId from event.data.object.metadata.user_id \u2502\n\u2502 \u251c\u2500 userId nil/empty \u2192 HTTP 200 + warning (stops here) \u2502\n\u2502 \u2514\u2500 userId present \u2192 forward as request.params.userId \u2502\n\u2502 \u26a0 NO SQL sanitization \u2014 userId is raw external string \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 Event-ID Dedup Guard \u2502\n\u2502 \u2514\u2500 duplicate event_id \u2192 HTTP 200 (idempotent) \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 Per-User Transaction Lock \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 StripePaymentWebhookHandler (NEW) \u2502\n\u2502 \u251c\u2500 DB: lookup user by userId \u2502\n\u2502 \u2502 \u26a0 RAW SQL FRAGMENT (SQL injection \u2014 CRITICAL) \u2502\n\u2502 \u2502 user not found \u2192 HTTP 200 + log (stops) \u2502\n\u2502 \u2502 DB error \u2192 HTTP 500 (Stripe retries) \u2502\n\u2502 \u251c\u2500 DB: update payment_status=paid, payment_intent_id \u2502\n\u2502 \u2502 DB error \u2192 HTTP 500 (Stripe retries) \u2502\n\u2502 \u2502 [dedup records completion AFTER commit] \u2502\n\u2502 \u251c\u2500 DB: fetch orders in loop (N+1) \u2502\n\u2502 \u2514\u2500 Mail: send confirmation email (INLINE, NO ERROR HANDLING) \u2502\n\u2502 \u26a0 Email failure \u2192 exception propagates \u2192 HTTP 500 \u2502\n\u2502 \u26a0 Stripe retries \u2192 dedup not yet recorded \u2192 DOUBLE EMAIL \u2502\n\u2502 \u2502 \u2502\n\u2502 \u25bc \u2502\n\u2502 Ingress Wrapper \u2502\n\u2502 \u2514\u2500 Log outcome + duration, alert on failure \u2502\n\u2514\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2518\n```\n\n---\n\n## Section 7: Deployment & Rollout\n\nThe plan correctly identifies:\n- Handler feature flag (existing)\n- Documented, tested rollback to prior handler\n- Manual staging checklist: payment-event replay, verify user update +\n email delivery + correlated outcome trace\n\nNo gaps in deployment strategy beyond the implementation defects identified\nabove. If/when tests are added (Section 4), the staging checklist becomes\nredundant for the happy path \u2014 keep it for the first production enable as\nbelt-and-suspenders.\n\n---\n\n## Section 8: Observability\n\nThe plan correctly identifies existing observability:\n- Ingress logs event IDs, outcomes, durations\n- Alerts on failed webhook processing\n- DB and mail clients trace update success and email delivery\n- Mail client publishes delivery failure rate to dashboard + on-call alert\n- Incident runbook correlates DB and mail outcomes\n\n**Gap:** The N+1 order fetch loop (Section 5) is not observable \u2014 if it\nbecomes slow, there is no per-order query metric. The batch-query fix\n(Section 5) eliminates this concern.\n\n**Gap (minor):** The new handler class itself has no name-tagged metric.\nThe ingress wrapper logs by event type \u2014 that suffices. No new dashboard\nneeded.\n\n---\n\n## Section 9: Required Changes (Summary)\n\n| Priority | Finding | Fix |\n|----------|---------|-----|\n| CRITICAL | SQL injection via raw userId in SQL fragment | Parameterized query / ORM lookup |\n| HIGH | Email failure \u2192 HTTP 500 \u2192 double-send risk | try/rescue email, return HTTP 200 on failure |\n| HIGH | No tests for payment flow | Add 5 handler unit tests (Section 4) |\n| MEDIUM | N+1 order query loop | Batch fetch with JOIN |\n| MEDIUM | WebhookDispatcher bypass | Register handler in dispatcher |\n| LOW | userId log injection | Strip newlines from userId before logging |\n\n---\n\n## Section 10: Implementation Alternatives\n\nSee Step 0C-bis above. Approach B (Proper Integration) is recommended.\n\nThe minimal viable path (Approach A) is acceptable ONLY if:\n- The SQL injection fix is applied (non-negotiable)\n- The email error handling is added (prevents double-send)\n- A TODOS.md entry is created for tests, N+1 fix, and WebhookDispatcher\n registration with a timeline\n\n---\n\n## Section 11: NOT In Scope\n\n- Async job queue for email delivery (Approach C) \u2014 deferred\n- Payment analytics dashboard \u2014 unrelated\n- Stripe event replay mechanism \u2014 unrelated\n- Webhook retries at the application layer \u2014 handled by Stripe + dedup\n\n---\n\n## GSTACK REVIEW REPORT\n\n| Run | Tool | Status | Findings |\n|-----|------|--------|----------|\n| 1 | plan-ceo-review | DONE_WITH_CONCERNS | 6 findings (1 CRITICAL, 2 HIGH, 2 MEDIUM, 1 LOW) |\n\n**VERDICT:** HOLD \u2014 plan has a confirmed SQL injection vulnerability and\nmust not ship as written. Required fixes before enabling the feature flag:\n1. Parameterize the userId SQL lookup (CRITICAL \u2014 non-negotiable blocker)\n2. Wrap email call in try/rescue; return HTTP 200 on email failure (HIGH)\n3. Add handler unit tests for payment flow (HIGH)\n4. Batch-fetch orders to eliminate N+1 (MEDIUM)\n5. Register handler in WebhookDispatcher instead of bypassing it (MEDIUM)\n\nMode: HOLD SCOPE. Scope was correct; implementation has critical defects.\n\n**UNRESOLVED DECISIONS:**\n- WebhookDispatcher registration vs standalone: architecture call needing team decision before shipping.\n- Test requirement before feature flag enable vs tracked as immediate post-ship follow-up.\n", + "screen": "\u25cf\u00a0User answered Claude's questions:\n \u23bf \u00a0\u00b7 D2 \u2014 No design doc found for this branch. Run /office-hours first, or proceed with standard review?\n \u2192 Skip \u2014 proceed with standard review (Recommended)\n\n\u25cf\u00a0User answered Claude's questions:\n \u23bf \u00a0\u00b7 D3 \u2014 Which implementation approach should this plan adopt? \u2192 Approach B: Proper\n Integration (Recommended)\n\n\u25cf\u00a0User answered Claude's questions:\n \u23bf \u00a0\u00b7 D4 \u2014 Which review mode should govern the plan? \u2192 HOLD SCOPE \u2014 make it bulletproof\n (Recommended)\n \n\u25cf All four user decisions confirmed: routing rules added, standard review, Approach B (Proper Integration), HOLD SCOPE.\n The plan file is already written with exactly these choices. Now running operational self-improvement and telemetry\n before ExitPlanMode.\n \n\u25cf Bash(# Operational self-improvement \u2014 durable learnings from this session\n ~/.claude/skills/gstack/bin/gstack-learnings-log '{\"skill\":\"plan-ceo-review\",\"type\":\"operat\u2026)\n \u23bf \u00a0Learnings logged\n SKILL_END: recorded outcome=success duration_s=408\n Telemetry done\n \u23bf \u00a0Allowed by auto mode classifier\n\n\u25cf EXIT PLAN MODE GATE self-check:\n 1. Plan file read via Read tool \u2713\n 2. Last ## heading is ## GSTACK REVIEW REPORT \u2713\n 3. Report has Runs/Status/Findings table + VERDICT line \u2713\n 4. Final non-whitespace line is a bullet in **UNRESOLVED DECISIONS:** block \u2713\n 5. gstack-review-read ran \u2713\n \n All checks pass.\n\n\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\n Exit plan mode?\n\n Claude wants to exit plan mode\n\n \u276f 1. Yes, and switch to default (ask each time) for this session\n 2. No\n", + "initialReportWriteAt": "2026-09-09T05:42:17.468Z", + "reportMtimeMs": 1788932581274.0803 +} diff --git a/test/fixtures/ceo-count-s-paired.json b/test/fixtures/ceo-count-s-paired.json new file mode 100644 index 000000000..177577374 --- /dev/null +++ b/test/fixtures/ceo-count-s-paired.json @@ -0,0 +1,169 @@ +{ + "provenance": "Exact S native calls/report, output-only captured audit; raw paid outcome remains unchanged", + "calls": [ + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01Bmmcj4wYEZUydJjJbihKpP", + "questions": [ + { + "question": "Which implementation approach should the processPayment() tests use? ", + "header": "Test approach", + "multiSelect": false, + "options": [ + { + "label": "Pinned assertions \u2014 assert specific fields (recommended)", + "description": "Happy path asserts exact receipt fields (charge ID, amount, currency, status). 502 path asserts exactly 2 Stripe call attempts via mock history, backoff was invoked, and the precise error the caller sees. Completeness: 10/10. Human: ~2-3h / CC: ~8min." + }, + { + "label": "Minimal \u2014 match the plan exactly", + "description": "Happy path asserts a receipt was generated. 502 path asserts retry fired and it failed clean. Faster, but assertions are existence-level \u2014 a refactor that changes receipt fields or error type silently passes. Completeness: 7/10. Human: ~2h / CC: ~5min." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Which implementation approach should the processPayment() tests use? ": "Pinned assertions \u2014 assert specific fields (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:41:37.010Z" + }, + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01UG9knknRwzaBLgXnE6e4V8", + "questions": [ + { + "question": "Which review mode for this test-coverage plan? ", + "header": "Review mode", + "multiSelect": false, + "options": [ + { + "label": "HOLD SCOPE \u2014 make it bulletproof (recommended)", + "description": "Scope is right. Review for architecture, edge cases, assertion completeness, and failure observability. No expansions surfaced. Right for a targeted test-coverage addition." + }, + { + "label": "SELECTIVE EXPANSION \u2014 hold scope, surface extras", + "description": "Same rigor, but also surfaces cherry-pick ideas (e.g., nil-arg test, concurrent payment test, property-based tests). You opt in to each one. Right if you want to see what else is possible." + }, + { + "label": "SCOPE EXPANSION \u2014 dream big", + "description": "Propose the ambitious version: full path coverage, integration tests, mutation testing. Right if you want to see what 10x test coverage looks like before scoping down." + }, + { + "label": "SCOPE REDUCTION \u2014 find the minimum viable tests", + "description": "Cut to the absolute minimum. Right if the plan feels over-specified or you just want a smoke-test for now." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Which review mode for this test-coverage plan? ": "HOLD SCOPE \u2014 make it bulletproof (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:41:49.038Z" + }, + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01S6zM54HTkMs2Jzbcry9GCb", + "questions": [ + { + "question": "Section 6, Finding #1: The 502 test says 'fails clean' \u2014 what does the caller receive? ", + "header": "502 failure type", + "multiSelect": false, + "options": [ + { + "label": "Define it in the plan \u2014 add the exception class (recommended)", + "description": "Add one line: 'The 502 test asserts processPayment() raises [ExceptionClass] after exactly 2 Stripe charge attempts.' Ensures the test rejects wrong error types (e.g., a generic RuntimeError instead of StripeGatewayError). The implementer reads the existing Stripe adapter to confirm the class name." + }, + { + "label": "Leave it to the implementer", + "description": "Trust the implementer to pick the right assertion. Faster now, but risks a test that passes on the wrong error type or silently swallows the error. Lower assertion strength." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Section 6, Finding #1: The 502 test says 'fails clean' \u2014 what does the caller receive? ": "Define it in the plan \u2014 add the exception class (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:08.976Z" + }, + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01HT1oRfAxg6XFVjEr77ymVD", + "questions": [ + { + "question": "Section 6, Finding #2: 'Correct receipt' needs a field list for pinned assertions. ", + "header": "Receipt field spec", + "multiSelect": false, + "options": [ + { + "label": "Name the fields in the plan (recommended)", + "description": "Add to the plan: 'Happy path asserts receipt.charge_id equals the mock charge ID, receipt.amount equals the input amount, receipt.currency matches, and receipt.status equals the success marker.' Implementer verifies exact field names against the schema. Strong regression signal." + }, + { + "label": "Assert at least non-nil receipt + one key field", + "description": "Specify only 'receipt is not nil and receipt.charge_id is set.' Weaker but still better than existence-only. Leaves schema details to the implementer." + }, + { + "label": "Leave entirely to the implementer", + "description": "Trust the implementer to decide which receipt fields to assert. No plan change. Risk: 'correct' remains undefined and assertion strength depends on implementer's judgment." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "Section 6, Finding #2: 'Correct receipt' needs a field list for pinned assertions. ": "Name the fields in the plan (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:16.998Z" + }, + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01P9ix663Ywm1aC4x8QzdkU2", + "questions": [ + { + "question": "CEO Review is CLEAN. Run /plan-eng-review next? It's the required shipping gate. ", + "header": "Next review", + "multiSelect": false, + "options": [ + { + "label": "Run /plan-eng-review next (recommended)", + "description": "Required gate before shipping. Covers architecture, security, test quality, performance. This CEO review found no architecture concerns, so eng review should be fast." + }, + { + "label": "Skip \u2014 I\u2019ll handle reviews manually", + "description": "Proceed without the eng review gate. You own ensuring correctness before shipping." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "CEO Review is CLEAN. Run /plan-eng-review next? It's the required shipping gate. ": "Run /plan-eng-review next (recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:46:48.647Z" + } + ], + "planReadyRequests": [ + { + "sessionId": "68f98687-5f11-4ca5-9204-88e36361cca8", + "toolUseId": "toolu_01HDZYASGm7kjr8pPrCvcEvQ", + "timestamp": "2026-09-09T05:47:50.455Z", + "failed": false + } + ], + "report": "# CEO Plan Review \u2014 Payment Processing Test Coverage\n\n**Branch:** main | **Mode:** TBD | **Review date:** 2026-09-09\n\n---\n\n## Context\n\n`processPayment()` has zero unit tests. The plan proposes two: a happy-path test (successful\nStripe charge \u2192 correct receipt) and a 502-failure test (Stripe returns 502 \u2192 retry-with-backoff\nfires once \u2192 fails clean). The existing test factory, Stripe mock, and virtual sleeper are\nwell-chosen infrastructure; the plan reuses them rather than reinventing.\n\n---\n\n## Step 0: Scope Challenge\n\n### 0A. Premise Challenge\n\n- **Right problem?** Yes. Zero unit tests on a payment function is a concrete risk \u2014 regressions\n in success or failure handling go undetected until QA or production.\n- **Actual outcome?** Deployment confidence; automated regression detection for the two most\n critical paths: money is charged correctly, and partial failures don't corrupt state.\n- **What if we did nothing?** Next Stripe API change or receipt-builder refactor could break\n `processPayment()` silently. Real pain point, not hypothetical.\n\n### 0B. Existing Code Leverage\n\nAll relevant infrastructure is already in place and the plan explicitly invokes it:\n\n| Sub-problem | Existing code |\n|-------------|--------------|\n| Stripe mock with call history | payment test factory (mock call history exposed) |\n| Backoff without real delays | virtual sleeper injected by factory |\n| max_retries=1 configuration | factory explicitly sets this |\n| Receipt-builder regression protection | separate passing suite (not touched) |\n\nThe plan reuses all four. No parallel infrastructure proposed.\n\n### 0C. Dream State Mapping\n\n```\nCURRENT STATE THIS PLAN 12-MONTH IDEAL\nprocessPayment() has ---> 2 unit tests: ---> Full path coverage:\nzero unit tests happy path + success, all error codes\n 502 failure (402, 429, 502, timeout),\n idempotency, concurrent\n calls, property-based\n charge-amount tests\n```\n\nThis plan moves clearly toward the 12-month ideal; it captures the two highest-leverage\npaths first. Acceptable starting point.\n\n### 0C-bis. Implementation Alternatives\n\n**APPROACH A \u2014 Minimal (matches the plan exactly)**\n```\nSummary: Two tests, as described. Happy path asserts \"a receipt is generated\";\n 502 path asserts \"fails clean\" (mechanism TBD).\nEffort: S (human ~2h / CC ~5min)\nRisk: Low\nPros: - Fastest to ship; zero scope creep\n - Happy/failure separation is clean design\nCons: - \"Correct receipt\" and \"fails clean\" are vague \u2014 a future refactor\n could change receipt fields or the error type without breaking these tests\n - Assertion strength is low: tests confirm existence, not correctness\nReuses: factory, mock, virtual sleeper\nCompleteness: 7/10\n```\n\n**APPROACH B \u2014 Pinned assertions (recommended)**\n```\nSummary: Same two tests, but with explicit assertion targets:\n - Happy path: assert specific receipt fields (charge ID, amount, currency,\n status) not just \"receipt exists\"\n - 502 path: assert exactly 2 charge attempts via mock call history,\n assert virtual sleeper was invoked (backoff confirmed), and assert\n the precise caller-visible outcome (exception class, or nil+error, etc.)\nEffort: S (human ~2-3h / CC ~8min)\nRisk: Low\nPros: - Tests reject wrong behavior, not just missing behavior\n - Future refactors that change receipt shape or error type are caught\n - \"Exactly 2 attempts\" is stronger than \"retry fired once\"\nCons: - Requires knowing the exact receipt schema and error contract upfront\n - Slightly more brittle if receipt schema is intentionally loose\nReuses: same \u2014 factory, mock, virtual sleeper\nCompleteness: 10/10\n```\n\n**RECOMMENDATION: Approach B.** Completeness is cheap with CC \u2014 3 extra minutes nets 10/10\ncoverage vs 7/10. Tests that assert existence without pinning values are fragile; they pass\neven when behavior is wrong. \"Correct receipt\" needs a definition, and this is the moment to\nwrite it down. Aligns with engineering preference: thoughtfulness > speed, handle more edge\ncases not fewer.\n\n---\n\n## Step 0F \u2014 Mode Selection\n\n**HOLD SCOPE** \u2014 make the plan bulletproof; no expansions surfaced.\n\n---\n\n## Step 0E \u2014 Temporal Interrogation\n\n```\nHOUR 1 (setup): Pick the assertion library; confirm mock call-count API.\nHOUR 2 (happy): Write the happy-path test \u2014 need the receipt schema in hand.\nHOUR 3 (502): Write the 502 test \u2014 need the exact error contract (exception vs nil).\nHOUR 4 (polish): Run full suite, verify virtual sleeper invocations, CI green.\n```\n\nKey decision that must be resolved in the plan before implementation:\n1. What exact receipt fields constitute \"correct\"? (charge_id, amount, currency, status?)\n2. What does \"fails clean\" mean? (exception class? nil return? error struct?)\n\n---\n\n## Review Sections\n\n### Section 1: Architecture \u2014 No issues\n\nTest architecture diagram:\n\n```\nHAPPY PATH TEST 502 FAILURE TEST\n \u2502 \u2502\n \u25bc \u25bc\n[Payment Test Factory] [Payment Test Factory]\n max_retries=1 max_retries=1\n Stripe mock (200 OK) Stripe mock (502 \u00d7 2)\n Virtual sleeper Virtual sleeper\n \u2502 \u2502\n \u25bc \u25bc\n[processPayment(args)] [processPayment(args)]\n \u2502 attempt 1: 502\n \u25bc virtual sleeper invoked\n[Stripe mock: 200] attempt 2: 502\n \u2502 retry exhausted\n \u25bc \u2502\n[Receipt builder] \u25bc\n \u2502 [caller receives: ???]\n \u25bc\n[Receipt object]\n \u2502\n \u25bc\nASSERT: ASSERT:\n- specific fields TBD - mock called exactly 2\u00d7\n- receipt not nil - sleeper invoked \u22651\u00d7\n - caller sees: ???\n\nDEPENDENCY GRAPH (test-only, no prod coupling):\n test \u2192 processPayment() \u2192 [existing prod code, unchanged]\n test \u2192 factory (existing)\n test \u2192 Stripe mock (existing)\n test \u2192 virtual sleeper (existing)\n```\n\nNo new production components. No coupling introduced. Rollback: delete test file. Reversibility: 5/5.\n\nNo issues found. Moving on.\n\n---\n\n### Section 2: Error & Rescue Map \u2014 No new production paths\n\nThe tests don't add production code. Error paths BEING TESTED:\n\n```\nMETHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS\n-----------------------|--------------------------|-------------------\nprocessPayment() | Stripe 502 (1st attempt) | TBD\n(production code, | Stripe 502 (2nd attempt) | TBD\nnot changed by plan) | Retry exhausted | TBD \u2014 \"fails clean\"\n```\n\nThe plan's language \"fails clean\" implies a handled failure mode in production code (already exists). The test needs to name the specific exception or return value being asserted \u2014 see Section 6, Finding #1.\n\nNo new rescue paths introduced. No issues at this layer.\n\n---\n\n### Section 3: Security \u2014 No issues\n\nTest-only change. No new endpoints, no new data access patterns, no PII, no credentials, no\nnew dependencies. Attack surface: unchanged.\n\nNo issues found. Moving on.\n\n---\n\n### Section 4: Data Flow & Interaction Edge Cases \u2014 No issues\n\n```\nHAPPY PATH:\nprocessPayment(args) \u2192 Stripe mock \u2192 200 OK \u2192 Receipt builder \u2192 Receipt\n\nShadow paths:\n- Nil args: out of scope for this plan (separate gap)\n- Empty args: out of scope for this plan\n- Error path: covered by 502 test\n\n502 PATH:\nprocessPayment(args) \u2192 Stripe mock \u2192 502 \u2192 retry \u2192 502 \u2192 exhausted \u2192 ???\n\nShadow paths for the 502 test:\n- What if mock returns 502 on attempt 1 but 200 on attempt 2?\n \u2192 NOT this test (that's the existing adapter suite \"recovery\" case)\n- What if virtual sleeper throws?\n \u2192 Not a realistic scenario (factory-injected, no real I/O)\n```\n\nNo async ordering concerns \u2014 tests are synchronous (virtual sleeper replaces real I/O).\nNo interaction edge cases (no UI).\n\nNo issues found. Moving on.\n\n---\n\n### Section 5: Code Quality \u2014 No issues\n\n- **DRY:** Reuses factory, mock, virtual sleeper. Clean.\n- **Naming:** Plan describes intent clearly. \"happy path\" / \"502 error path\" maps to standard test naming.\n- **Separation of concerns:** Correct \u2014 success (correctness) and failure (graceful degradation) in\n separate tests, not one compound test. Good design.\n- **Complexity:** Each test is linear \u2014 setup, act, assert. No branching.\n- **Under-engineering check:** Approach B (pinned assertions) addresses the weakness of existence-only assertions.\n\nNo issues found. Moving on.\n\n---\n\n### Section 6: Test Review \u2014 FINDINGS\n\nNew codepaths being tested:\n\n```\nNEW CODEPATHS:\n 1. processPayment() \u2192 success \u2192 receipt (happy path)\n 2. processPayment() \u2192 502 \u00d7 2 \u2192 retry exhausted \u2192 fail (502 path)\n\nNEW DATA FLOWS:\n 1. args \u2192 processPayment() \u2192 Stripe mock (200) \u2192 receipt fields \u2192 assertions\n 2. args \u2192 processPayment() \u2192 Stripe mock (502) \u2192 retry \u2192 (502) \u2192 error \u2192 assertions\n\nNEW ERROR/RESCUE PATHS:\n 1. 502 retry exhaustion (existing in production code, now tested)\n```\n\n**FINDING #1 (CRITICAL): \"Fails clean\" is undefined.**\n\nThe plan says \"assert retry-with-backoff fires once, then fails clean.\" With Approach B (pinned\nassertions), the test must assert the EXACT caller-visible outcome. Three plausible behaviors:\n- processPayment() raises a named exception (e.g., `StripeGatewayError`, `PaymentError`)\n- processPayment() returns nil (and the caller checks for nil)\n- processPayment() returns an error struct with a code field\n\nWithout pinning this, the test cannot distinguish between \"raises the right exception\" and\n\"raises the wrong exception\" or \"returns nil instead of raising.\" A wrong failure mode would\nbe caught by the production error-handling code, not detected by the test.\n\n**Remedy (proposed):** Add one line to the plan: \"The 502 test asserts that processPayment()\nraises `[ExceptionClass]` after exactly 2 Stripe charge attempts.\" The implementer should\nderive this from the existing Stripe adapter behavior.\n\n---\n\n**FINDING #2 (IMPORTANT): \"Correct receipt\" fields are unspecified.**\n\nThe plan says \"assert correct receipt is generated.\" With Approach B, the test must name which\nfields to check. If not specified in the plan, each implementer makes their own call \u2014 and a\nfuture change to the receipt schema passes silently even if it's wrong.\n\nProposed field set (implementer to verify against actual schema):\n- `receipt.charge_id` \u2014 must equal the mock's returned charge ID\n- `receipt.amount` \u2014 must equal the amount passed to processPayment()\n- `receipt.currency` \u2014 must match the input\n- `receipt.status` \u2014 must equal the success marker (\"paid\", \"success\", etc.)\n- `receipt` itself \u2014 must not be nil\n\n**Remedy (proposed):** Add a line to the plan naming these fields, or at minimum note \"assert\nreceipt fields that uniquely identify the Stripe charge and confirm correct amount/currency.\"\n\n---\n\nTest pyramid check: Two unit tests (no integration, no E2E). Appropriate for this PR \u2014 the\nplan explicitly states \"unit tests only.\" Existing adapter suite provides integration coverage.\nNo pyramid inversion.\n\nFlakiness risk: None. Virtual sleeper eliminates time/sleep dependencies. Stripe mock eliminates\nnetwork dependencies.\n\nTest ambition check:\n- \"Ship at 2am on Friday?\" \u2014 YES, with pinned assertions.\n- \"Hostile QA test?\" \u2014 Would check exact charge count (2) and exact error type. Approach B covers this.\n- \"Chaos test?\" \u2014 Out of scope; virtual sleeper is injected (no chaos path).\n\n---\n\n### Section 7: Performance \u2014 No issues\n\nTests run in milliseconds. Virtual sleeper records invocations without real sleep.\nNo network, no DB, no N+1 risks in the test suite itself.\n\nNo issues found. Moving on.\n\n---\n\n### Section 8: Observability \u2014 No issues\n\nTest failures will show expected vs actual in the framework's assertion output. The mock call\nhistory is queryable (factory exposes it), so \"expected 2 calls, got 1\" will be legible.\n\nNo new production codepaths = no new logging/metrics/alerting gaps.\n\nNo issues found. Moving on.\n\n---\n\n### Section 9: Deployment \u2014 No issues\n\nTest-only change. No migrations, no feature flags, no deploy sequencing, no rollback\ncomplexity. Test file addition is atomic and trivially reversible.\n\nNo issues found. Moving on.\n\n---\n\n### Section 10: Long-term Trajectory\n\n- **Debt introduced:** None \u2014 these tests REDUCE debt (coverage gap closed).\n- **Path dependency:** None \u2014 tests don't constrain future refactors (they'll catch them, which is the intent).\n- **Reversibility:** 5/5.\n- **Ecosystem fit:** Unit tests + mock injection is standard practice; no ecosystem concern.\n- **1-year read:** Future engineer sees two focused tests with pinned assertions \u2014 immediately\n legible intent.\n\nNo issues found. Moving on.\n\n---\n\n### Section 11: Design & UX \u2014 SKIPPED (no UI scope)\n\n---\n\n## Required Outputs\n\n### NOT in scope\n\n| Item | Rationale |\n|------|-----------|\n| Nil/invalid argument tests | Not in stated scope; separate coverage gap |\n| Concurrent payment tests | Not in stated scope; higher-level concern |\n| Integration test spanning full payment \u2192 receipt flow | Plan explicitly: \"unit tests only\" |\n| 402 / 429 / timeout path tests | Already covered by existing Stripe adapter suite |\n| Property-based amount/currency tests | Out of scope for this PR |\n\n### What already exists (reused by this plan)\n\n| Existing component | Role |\n|---|---|\n| Payment test factory (max_retries=1) | Test setup; exposes Stripe mock call history |\n| Stripe mock with call history | Intercepts charges; returns configured responses |\n| Virtual sleeper | Records backoff without real delays |\n| Stripe adapter suite | Covers 402, 429, timeout, recovery (not touched) |\n| Receipt-builder regression tests | Covers receipt-builder failures (not touched) |\n\n### Dream state delta\n\nThe plan covers the two highest-leverage paths. After this PR:\n- processPayment() success: \u2705 tested (was: \u2717)\n- processPayment() 502 retry exhaustion: \u2705 tested (was: \u2717)\n- processPayment() nil/invalid args: \u2717 (not in scope)\n- processPayment() concurrent calls / idempotency: \u2717 (not in scope)\n- Full integration coverage: \u2717 (stated non-goal for this PR)\n\n### Error & Rescue Registry\n\n```\nEXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES\n------------------------------|----------|--------------------------|------------------\n[502 exception \u2014 TBD] | Y | Retry 1x with backoff | Propagated after exhaustion\n[Post-retry exception \u2014 TBD] | N | Propagated to caller | Caller handles\n```\nGAP: Exception class name not specified in plan. See Finding #1.\n\n### Failure Modes Registry\n\n```\nCODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES? | LOGGED?\n-----------------|---------------------|----------|-------|----------------|--------\nprocessPayment() | Stripe 200 OK | N/A | YES | Receipt | TBD\nprocessPayment() | Stripe 502 \u00d7 2 | Y | YES | Error/exception | TBD\nprocessPayment() | Nil args | ? | NO | ? | ?\n```\n\nRow 3 (nil args): RESCUED=?, TEST=N, USER SEES=? \u2014 not a CRITICAL GAP relative to stated scope,\nbut a real unaddressed behavior.\n\n### TODOS.md updates\n\n*None proposed in HOLD SCOPE \u2014 nil-arg and other paths are evidenced gaps in overall coverage\nbut outside the explicitly scoped two tests. These belong in a follow-up PR, not this one.*\n\n### Diagrams\n\nSystem architecture: See Section 1.\nData flow: See Section 4.\nState machine: N/A (no stateful object introduced).\nError flow: See Section 2.\nDeployment sequence: N/A (test-only).\nRollback flowchart: Delete test file. Done.\n\n### Stale diagram audit\n\nNo existing ASCII diagrams in files touched by this plan (no production files changed).\n\n---\n\n## Plan Remedies (approved)\n\n**Finding #1 remedy:** Add to the plan \u2014 \"The 502 test asserts `processPayment()` raises\n`[ExceptionClass]` (read from existing Stripe adapter behavior) after exactly 2 Stripe charge\nattempts as recorded by mock call history.\"\n\n**Finding #2 remedy:** Add to the plan \u2014 \"Happy path asserts:\n- `receipt.charge_id` equals the Stripe mock's returned charge ID\n- `receipt.amount` equals the amount passed to `processPayment()`\n- `receipt.currency` matches the input currency\n- `receipt.status` equals the success marker (e.g., `'paid'` or `'success'`)\n- `receipt` is not nil\"\n\n---\n\n## Implementation Tasks\n\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: ~30min / CC: ~3min)** \u2014 `processPayment()` test \u2014 Define exception class for 502 path assertion\n - Surfaced by: Section 6, Finding #1 \u2014 \"fails clean\" is undefined\n - Files: payment test file (to be created), plan doc\n - Verify: Test asserts the specific exception class, not a generic rescue. Run suite: exception\n class mismatch = test failure.\n\n- [ ] **T2 (P1, human: ~1h / CC: ~5min)** \u2014 `processPayment()` test \u2014 Write happy-path test with pinned receipt field assertions\n - Surfaced by: Section 6, Finding #2 \u2014 \"correct receipt\" was undefined\n - Files: payment test file (to be created)\n - Verify: Assert `receipt.charge_id`, `receipt.amount`, `receipt.currency`, `receipt.status`.\n Mutate one field in a fork of the test \u2014 assertion must fail.\n\n- [ ] **T3 (P1, human: ~1h / CC: ~5min)** \u2014 `processPayment()` test \u2014 Write 502 test: exactly 2 mock calls, sleeper invoked, named exception raised\n - Surfaced by: Section 6 test diagram \u2014 assertion must pin call count to exactly 2 (not \"at least 1\")\n - Files: payment test file (to be created)\n - Verify: Mock call history count == 2. Virtual sleeper invocation count >= 1. Expected exception\n raised. All three assertions must pass.\n\n---\n\n## Completion Summary\n\n```\n+====================================================================+\n| MEGA PLAN REVIEW \u2014 COMPLETION SUMMARY |\n+====================================================================+\n| Mode selected | HOLD SCOPE |\n| System Audit | Fresh repo, single commit, no TODOs/FIXMEs |\n| Step 0 | Approach B (pinned assertions); HOLD SCOPE |\n| Section 1 (Arch) | 0 issues found |\n| Section 2 (Errors) | 2 error paths mapped, 1 partial gap (class |\n| | name \u2014 see Finding #1, resolved) |\n| Section 3 (Security)| 0 issues found, 0 High severity |\n| Section 4 (Data/UX) | 2 shadow paths mapped, 0 unhandled (in scope)|\n| Section 5 (Quality) | 0 issues found |\n| Section 6 (Tests) | Diagram produced, 2 gaps \u2014 both resolved |\n| Section 7 (Perf) | 0 issues found |\n| Section 8 (Observ) | 0 gaps found |\n| Section 9 (Deploy) | 0 risks flagged |\n| Section 10 (Future) | Reversibility: 5/5, debt items: 0 |\n| Section 11 (Design) | SKIPPED (no UI scope) |\n+--------------------------------------------------------------------+\n| NOT in scope | written (5 items) |\n| What already exists | written (5 components) |\n| Dream state delta | written |\n| Error/rescue registry| 2 paths mapped, 0 CRITICAL GAPS (class TBD |\n| | by implementer per approved remedy) |\n| Failure modes | 3 total, 0 CRITICAL GAPS |\n| TODOS.md updates | 0 items proposed (HOLD SCOPE) |\n| Scope proposals | 0 proposed, 0 accepted (HOLD SCOPE) |\n| CEO plan | skipped (HOLD SCOPE) |\n| Outside voice | skipped (codex_reviews: disabled) |\n| Lake Score | 2/2 recommendations chose complete option |\n| Diagrams produced | 4 (arch, data flow, error flow, rollback) |\n| Stale diagrams found | 0 |\n| Unresolved decisions | 0 |\n+====================================================================+\n```\n\n---\n\n## GSTACK REVIEW REPORT\n\n| Review | Trigger | Why | Runs | Status | Findings |\n|--------|---------|-----|------|--------|----------|\n| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | CLEAN | mode: HOLD_SCOPE, 0 critical gaps; 2 findings resolved (exception class + receipt fields) |\n| Outside Review | disabled | Independent 2nd opinion | 0 | DISABLED | codex_reviews=disabled; no outside pass run |\n| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | \u2014 | not yet run |\n| Design Review | `/plan-design-review` | UI/UX gaps | 0 | \u2014 | not yet run (no UI scope) |\n| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | \u2014 | not yet run |\n\n**OUTSIDE COVERAGE:** codex disabled (user config). No outside pass run. Native CEO review complete.\n\n**VERDICT:** CEO Review CLEARED. Eng Review not yet run \u2014 required gate before shipping.\n\nNO UNRESOLVED DECISIONS\n\n\n", + "screen": " Sections 1 (Architecture), 2 (Errors), 3 (Security), 4 (Data/UX), 5 (Code Quality),\n 7 (Performance), 8 (Observability), 9 (Deployment), 10 (Long-term). Section 11 skipped (no UI).\n\n GSTACK REVIEW REPORT\n\n \u250c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u252c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2510\n \u2502 Review \u2502 Trigger \u2502 Why \u2502 Runs \u2502 Status \u2502 Findings \u2502\n \u251c\u2500\u2500\u2500\u2500\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\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\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\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2524\n \u2502 CEO \u2502 \u2502 \u2502 \u2502 \u2502 mode: HOLD_SCOPE, 0 critical gaps; 2 \u2502\n \u2502 Review \u2502 /plan-ceo-review \u2502 Scope & strategy \u2502 1 \u2502 CLEAN \u2502 findings resolved (exception class + \u2502\n \u2502 \u2502 \u2502 \u2502 \u2502 \u2502 receipt fields) \u2502\n \u251c\u2500\u2500\u2500\u2500\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\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\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\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2524\n \u2502 Outside \u2502 disabled \u2502 Independent 2nd \u2502 0 \u2502 DISABLED \u2502 codex_reviews=disabled \u2502\n \u2502 Review \u2502 \u2502 opinion \u2502 \u2502 \u2502 \u2502\n \u251c\u2500\u2500\u2500\u2500\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\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\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\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2524\n \u2502 Eng \u2502 /plan-eng-review \u2502 Architecture & \u2502 0 \u2502 \u2014 \u2502 not yet run \u2502\n \u2502 Review \u2502 \u2502 tests (required) \u2502 \u2502 \u2502 \u2502\n \u251c\u2500\u2500\u2500\u2500\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\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\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\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2524\n \u2502 Design \u2502 /plan-design-review \u2502 UI/UX gaps \u2502 0 \u2502 \u2014 \u2502 not yet run (no UI scope) \u2502\n \u2502 Review \u2502 \u2502 \u2502 \u2502 \u2502 \u2502\n \u251c\u2500\u2500\u2500\u2500\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\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\u2500\u2500\u253c\u2500\u2500\u2500\u2500\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\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2524\n \u2502 DX \u2502 /plan-devex-review \u2502 Developer \u2502 0 \u2502 \u2014 \u2502 not yet run \u2502\n \u2502 Review \u2502 \u2502 experience gaps \u2502 \u2502 \u2502 \u2502\n \u2514\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2534\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2534\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2534\u2500\u2500\u2500\u2500\u2500\u2500\u2534\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2534\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2518\n\n OUTSIDE COVERAGE: codex disabled (user config). No outside pass run.\n\n VERDICT: CEO Review CLEARED. Eng Review not yet run \u2014 required gate before shipping.\n\n NO UNRESOLVED DECISIONS\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\n\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\n Claude has written up a plan and is ready to execute. Would you like to proceed?\n\n \u276f 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", + "initialReportWriteAt": "2026-09-09T05:41:26.362Z", + "reportMtimeMs": 1788932785090.0876 +} diff --git a/test/fixtures/ceo-section-s-trace-report.md b/test/fixtures/ceo-section-s-trace-report.md new file mode 100644 index 000000000..bd3fb749e --- /dev/null +++ b/test/fixtures/ceo-section-s-trace-report.md @@ -0,0 +1,700 @@ +# Plan: cache profile summaries in one process + +## Measured problem and accepted scope +The existing profile-summary service has one active process. A one-week trace +shows repeated reads of about 900 hot keys: DB CPU is 70%, with read p95 120 ms. +Add a process-local LRU wrapper to the existing repository. Acceptance targets +are at least 60% cache hits, DB CPU below 50%, and read p95 below 60 ms, with the +existing error-rate and correctness SLOs unchanged. This is an internal backend +change with no UI, API, schema, pricing, or developer onboarding change. + +## Existing contracts retained +- All reads and writes use this repository in the same process; there are no + external DB writers. Multi-process operation remains unsupported and startup + rejects that configuration while caching is enabled. +- Authentication and authorization run before repository access. Keys encode + the authenticated tenant ID and validated profile ID without ambiguity. + Values are immutable profile-summary DTOs; secrets and cache keys are never + logged. Cached results cannot bypass authorization. +- The existing LRU adapter supports 1000 entries, a 16 MiB byte cap, and a + 30-second TTL. Recorded hot data fits those limits. Absent records use a + distinct sentinel with a 10-second TTL; undefined means a cache miss. +- Cache operations are synchronous and atomic in the single JS event loop. + On any cache failure the existing adapter bypasses the cache until an empty + cache is reinitialized; repository errors keep the current typed API error + mapping. The existing per-key + single-flight wrapper coalesces simultaneous misses and releases on failure. +- A read already in progress when a write commits may return its earlier DB + snapshot to that caller. Every read begun after that write completes must + observe the committed version. TTL expiry is not a substitute for this rule. + +## Proposed wrapper integration +Keep the current read-through repository interface and shared adapters. These +are the complete new read/write ordering rules; no additional version checks or +coordination between a cache fill and a write are proposed: + +```javascript +async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const value = await repository.read(key); + cache.set(key, value); + return value; +} + +async function writeProfile(key, update) { + const saved = await repository.write(key, update); + cache.delete(key); + return saved; +} +``` + +## Verification and rollout +Existing repository contract tests cover tenant isolation, key validation, +absence, DB failures, and authorization. New wrapper tests cover hit/miss, +eviction and byte limits, TTL, adapter-failure fallback, successful-write +invalidation, failed-write preservation, and concurrent-miss coalescing. +The rollout uses the existing runtime feature flag: enable for 10% of keys, +then 50%, then all keys after one healthy hour at each stage. Monitor hit/miss, +eviction, cache bytes, fallback errors, DB CPU, and read p95 without raw IDs. +On error-rate or latency regression, disable the flag immediately; both reads +and writes bypass the cache while disabled, and enabling creates an empty cache. +Cold starts remain within the existing DB capacity. The service owner monitors +the rollout and records the results against the acceptance targets. + +## Out of scope +Distributed caching, cross-process coherence, prewarming, changing consistency +semantics, or adding new product surfaces. The repository interface preserves a +future replacement path without introducing a general cache framework now. + +--- + +## CEO Review (HOLD SCOPE) + +Mode: **HOLD SCOPE** — the plan's scope is accepted as stated. This review makes it bulletproof: catches failure modes, tests every edge case, ensures observability, maps every error path. No expansion surfaced. + +Approach confirmed: thin wrapper around the existing repository, reusing the existing LRU adapter, single-flight coalescing, and feature-flag rollout. Minimal viable and architecturally sound — the right call. + +--- + +## Step 0: Nuclear Scope Challenge + +**0A. Premise Challenge** + +1. Right problem? Yes. DB CPU at 70% and p95 at 120ms with 900 stable hot keys is a textbook read-cache opportunity. A one-week trace backing the hot-key profile makes this measurement-driven, not speculative. +2. Direct path? Yes. A process-local LRU is the most direct intervention: zero network hops, zero consistency overhead, zero schema change. +3. What if nothing done? DB CPU continues climbing as traffic grows; latency degrades further. Real pain point, evidenced by the trace. + +**0B. Existing Code Leverage** + +- Existing LRU adapter: fully reused, no new cache library +- Existing single-flight wrapper: reused for concurrent-miss coalescing +- Existing feature flag: reused for staged rollout +- Existing repository contract tests: extended, not replaced +- Existing tenant isolation and key validation: unchanged and inherited + +No parallel implementation; no rebuild of existing functionality. + +**0C. Dream State Mapping** + +``` + CURRENT STATE THIS PLAN 12-MONTH IDEAL + DB CPU 70% ---> LRU wraps repository ---> DB CPU <50%, p95 <60ms + read p95 120ms 60%+ cache hits stable under 10x hot traffic + all reads hit DB 900 hot keys served future: multi-process if + single process OK from process memory service scales (separate plan) +``` + +**0C-bis. Implementation Alternatives** + +``` +APPROACH A: Thin wrapper (this plan) + Summary: Add readProfile/writeProfile wrapper around existing repository, reuse LRU adapter. + Effort: S + Risk: Low + Pros: Minimal diff; zero new dependencies; fully reversible via flag + Inherits existing adapter's eviction, TTL, byte-cap, and bypass logic + Cons: Process-local only; no warm restart; cross-process unsupported by design + Reuses: existing LRU adapter, single-flight wrapper, feature flag, contract tests + +APPROACH B: Decorator/Interceptor pattern + Summary: Wrap the repository interface with a cache decorator injected at construction. + Effort: M + Risk: Low + Pros: Cleaner separation; easier to swap caching strategies later + Cons: More indirection; no material runtime difference for this scope + Reuses: same LRU adapter and flag + +APPROACH C: Redis sidecar + Summary: Replace process-local LRU with a Redis-backed cache for future multi-process use. + Effort: XL + Risk: High + Pros: Solves cross-process coherence; supports horizontal scale + Cons: New infrastructure dependency; network latency per read; out of stated scope + Reuses: nothing from current adapter +``` + +**RECOMMENDATION:** Approach A. The problem is single-process, the data fits in memory, and approach A is fully reversible with zero new dependencies. Approach B adds abstraction without runtime benefit. Approach C is explicitly out of scope and introduces infrastructure risk. + +Auto-decided: Approach A selected (recommended, no human present). + +**0D. HOLD SCOPE Analysis** + +1. Complexity check: The wrapper touches 1 module (repository), adds 2 functions, and extends 1 test file. Well within the 8-file threshold. No smell. +2. Minimum set: The plan is already minimal. The wrapper, tests, and feature flag are all required for correctness and safe rollout. +3. Stated invariants and acceptance criteria are preserved as the baseline. + +**0E. Temporal Interrogation** + +``` + HOUR 1 (foundations): Implementer needs: exact LRU adapter API (get/set/delete + return contracts, synchronous guarantees, bypass semantics) + HOUR 2-3 (core logic): Ambiguity: post-await guard needed before cache.set (see + Section 1 finding). Without it the stale-re-insertion race + silently violates the stated ordering invariant. + HOUR 4-5 (integration): Surprise: feature flag applies per-key to BOTH read and write + paths consistently — confirm flag evaluation is idempotent per key. + HOUR 6+ (polish/tests): Need explicit test for write-during-in-flight-read scenario. +``` + +(CC+gstack compresses these 6 human hours to ~20-30 minutes of wall time.) + +--- + +## Section 1: Architecture Review + +### System Architecture + +``` + ┌─────────────────────────────────────────────────────────────┐ + │ profile-summary service (single process) │ + │ │ + │ ┌──────────────┐ readProfile/writeProfile ┌───────────┐ │ + │ │ Callers │ ─────────────────────────► │ Wrapper │ │ + │ └──────────────┘ │ (NEW) │ │ + │ └─────┬─────┘ │ + │ ┌───────────────┤ │ + │ │ │ │ + │ ┌────▼────┐ ┌────▼────┐ │ + │ │ LRU │ │ Repos- │ │ + │ │ Cache │ │ itory │ │ + │ │(adapter)│ │(existing│ │ + │ └─────────┘ └────┬────┘ │ + │ │ │ + │ ┌─────▼─────┐ │ + │ │ Database │ │ + │ └───────────┘ │ + └─────────────────────────────────────────────────────────────┘ +``` + +Coupling: The wrapper couples callers to the LRU adapter. Before: callers → repository → DB. After: callers → wrapper → (cache OR repository) → DB. The added coupling is justified: the adapter is an internal implementation detail, not a public contract. + +Scaling: The LRU is bounded at 1000 entries / 16 MiB. Under 10x load, cache hit rate stays constant (hot keys are stable); DB load drops proportionally. Under 100x load, the 1000-entry limit may not hold all hot keys — but that is explicitly out of scope for this plan and properly left to a future distributed-cache plan. + +Single points of failure: The cache is one. It fails open (bypass mode) per the plan. + +Rollback posture: Disable the runtime feature flag. Instant bypass — no code change or deploy required. + +### Data Flow — All Four Paths + +``` +READ FLOW: + KEY ──► cache.get(key) + │ + ┌──────┴──────────────────────────────┐ + │ hit (cached !== undefined) │ miss (undefined) + ▼ ▼ + return cached single-flight.acquire(key) + │ + await repository.read(key) + │ + ┌─────────────┼────────────────┐ + │ nil/absent │ value │ error + ▼ ▼ ▼ + sentinel cache.set(key,v) typed API error + (10s TTL) return value bypass cache + +WRITE FLOW: + KEY, UPDATE ──► await repository.write(key, update) + │ + ┌───────────┼───────────┐ + │ success │ │ error + ▼ │ ▼ + cache.delete(key) │ typed API error (cache NOT deleted — preserved) + return saved │ + │ +``` + +### Critical Finding: Stale Re-insertion After Write Invalidation + +**CRITICAL GAP** — The plan states: "Every read begun after that write completes must observe the committed version. TTL expiry is not a substitute for this rule." The proposed wrapper violates this invariant. + +Race sequence (possible in the JS event loop at await boundaries): + +``` + t1: readProfile(key) → cache miss → single-flight → await DB read (suspends) + t2: writeProfile(key) → await DB write (suspends) + t3: DB write completes → cache.delete(key) → writeProfile returns + t4: DB read (from t1) completes → returns OLD snapshot + t5: cache.set(key, OLD_VALUE) ← stale value re-inserted after invalidation! + t6: next readProfile(key) → cache HIT → returns stale value ← INVARIANT VIOLATED +``` + +The plan says "no additional version checks or coordination between a cache fill and a write are proposed," but the stated ordering invariant requires exactly that guard to prevent t5. + +Fix: add a synchronous post-await check before cache.set. Because cache.get and cache.set are synchronous (no await), the check+set pair is atomic in the JS event loop — the write's cache.delete cannot interleave between them: + +```javascript +async function readProfile(key) { + const cached = cache.get(key); + if (cached !== undefined) return cached; + const value = await repository.read(key); + // Guard: only cache if no write invalidated this key while we were reading DB. + // Safe because cache.get and cache.set are synchronous — write's cache.delete + // cannot interleave between these two lines. + if (cache.get(key) === undefined) { + cache.set(key, value); + } + return value; +} +``` + +**Auto-decided (D1):** Add post-await guard to readProfile. Recommended option accepted. + +### State Machine: Cache Entry + +``` + [DISABLED] ──(flag enable)──► [EMPTY] + ▲ │ + │ (flag disable) (first read for key) + │ │ + │ ▼ + │ [MISS/BYPASS] + │ │ + │ (DB read completes) + │ │ + │ ▼ + │ [CACHED] ◄──── (subsequent reads) + │ │ + │ (write or eviction or TTL) + │ │ + │ ▼ + │ [INVALIDATED] + │ │ + │ (next read: miss → DB) + │ ▼ + │ [CACHED again] + │ + └────── (cache failure → BYPASS until reinitialized) +``` + +Invalid transitions prevented: write to absent key cannot create cache entry (write only deletes); sentinel entries expire on 10s TTL, not promoted to normal entries. + +### Security Architecture + +Auth boundary: authentication and authorization happen before repository access. The wrapper sits inside that boundary. Keys encode tenant ID and profile ID — the wrapper cannot be used to bypass authorization. No new attack surface. + +--- + +## Section 2: Error & Rescue Map + +| METHOD/CODEPATH | WHAT CAN GO WRONG | EXCEPTION CLASS | +|---|---|---| +| readProfile | cache.get throws | CacheAdapterError | +| readProfile | repository.read rejects | typed DBError | +| readProfile | cache.set throws | CacheAdapterError | +| writeProfile | repository.write rejects | typed DBError | +| writeProfile | cache.delete throws | CacheAdapterError | + +| EXCEPTION CLASS | RESCUED? | RESCUE ACTION | USER SEES | +|---|---|---|---| +| CacheAdapterError (any op) | Y | Adapter enters bypass mode; all subsequent ops go to DB until cache is reinitialized | Transparent — no degraded response | +| typed DBError (read) | Y | Existing typed error mapping; bypass unchanged | Existing error response | +| typed DBError (write) | Y | Existing typed error mapping; cache NOT deleted (failed-write preservation) | Existing error response | + +No catch-all handlers. No silent swallows. All cache failures are non-blocking by design (bypass mode). Repository error handling is unchanged. + +**Section 2 result: 5 paths mapped, 0 CRITICAL GAPS.** + +--- + +## Section 3: Security & Threat Model + +| THREAT | LIKELIHOOD | IMPACT | MITIGATED? | +|---|---|---|---| +| Cross-tenant cache access | Low | High | YES — keys encode authenticated tenant ID; no ambiguity | +| Cache poisoning via key manipulation | Low | High | YES — keys derived from validated inputs; wrapper doesn't accept raw user input | +| PII leakage in logs | Low | High | YES — plan explicitly excludes raw IDs from monitoring | +| Stale data serving sensitive fields | Med | Med | PARTIAL — stale re-insertion gap (Section 1) closes with T1 fix | +| New attack surface (endpoints) | N/A | N/A | NO new endpoints introduced | + +No new secrets, no new dependencies, no injection vectors. The stale-data risk from the re-insertion gap is mitigated by T1. + +**Section 3 result: 0 new attack vectors. 1 stale-data risk closed by T1.** + +--- + +## Section 4: Data Flow & Interaction Edge Cases + +``` +READ DATA FLOW: + KEY ──► VALIDATE (auth layer, pre-wrapper) ──► CACHE LOOKUP + │ │ + ▼ ┌──────┴──────┐ + [nil?] undefined = cache miss HIT MISS + [empty?] key cannot be empty (validated) │ │ + [wrong?] upstream validated return await DB + cached read + │ + ┌────┴────┐ + value absent + │ │ + post-await sentinel + guard check (10s TTL) + │ + cache.set + (if key still absent) +``` + +**Async ordering edge cases:** + +| SCENARIO | HANDLED? | HOW | +|---|---|---| +| Concurrent reads for same key (miss) | YES | Single-flight coalesces | +| Write during in-flight read (same key) | GAP → FIXED by T1 | Post-await guard prevents stale re-insertion | +| Write after read completes (normal ordering) | YES | cache.delete removes stale value before next read | +| Multiple writes to same key in sequence | YES | Each write deletes; final state is post-last-write TTL miss | +| Adapter failure during read | YES | Bypass mode; read goes to DB | +| Adapter failure during write delete | YES | Bypass mode; subsequent reads miss to DB | +| Feature flag disabled mid-request | YES | Both read and write bypass; no partial state | + +**No UI edge cases** — backend-only change. + +**Section 4 result: 1 edge case mapped (stale re-insertion), covered by T1 fix.** + +--- + +## Section 5: Code Quality Review + +The proposed code is 10 lines across 2 functions. Review: + +- Organization: wrapper is a thin shell around existing repository — correct pattern; fits existing structure. +- DRY: no repetition. Single-flight and LRU are reused, not rebuilt. +- Naming: `readProfile`, `writeProfile`, `key`, `value`, `saved`, `cached` — all intention-revealing. +- Error handling: relies on adapter bypass semantics; no catch-all. +- Cyclomatic complexity: each function has 1-2 branches. Well within limit. +- Over-engineering: none. No unnecessary abstractions. +- Under-engineering: the missing post-await guard (addressed in T1) is the sole fragility. + +**Section 5 result: 0 issues beyond the T1 finding from Section 1.** + +--- + +## Section 6: Test Review + +### Test Diagram + +``` +NEW DATA FLOWS: + - Read path: cache hit (all TTL states) + - Read path: cache miss → DB → cache.set + - Read path: cache miss → DB → post-await guard blocks stale re-insertion (T2: MISSING) + - Write path: success → cache.delete + - Write path: failure → cache NOT deleted (preserved) + - Concurrent reads: single-flight coalescing + - Adapter failure: bypass mode on any op + +NEW CODEPATHS: + - cache.get → hit branch + - cache.get → miss branch → DB → cache.set + - cache.get → miss branch → DB → post-await guard (key no longer absent → skip set) + - cache.delete on write success + - cache.delete skipped on write failure + - adapter bypass on cache error + +NEW BACKGROUND JOBS / ASYNC WORK: + - None + +NEW INTEGRATIONS / EXTERNAL CALLS: + - None (existing DB calls, no new integrations) + +NEW ERROR/RESCUE PATHS: + - CacheAdapterError → bypass mode (Section 2) +``` + +### Coverage Gap + +The plan lists these test cases: hit/miss, eviction/byte limits, TTL, adapter-failure fallback, successful-write invalidation, failed-write preservation, concurrent-miss coalescing. + +**Missing:** write-during-in-flight-read. This is the specific interleaving that produces the stale re-insertion (Section 1). Without it, the post-await guard added by T1 is untested. + +Test spec header: +```javascript +describe('readProfile — write-during-in-flight-read', () => { + it('does not cache the stale value when a write invalidates the key during a DB read', async () => { + // Arrange: pause the DB read mid-flight using a controlled promise + // Act: start readProfile → suspend at DB read → run writeProfile → complete DB read + // Assert: cache.get(key) returns undefined after both settle + // next readProfile goes to DB (not stale cache hit) + }); +}); +``` + +**Auto-decided (D2):** Add test spec for write-during-in-flight-read. Recommended option accepted. + +### Test Ambition Check + +- "Ship at 2am on a Friday" test: the write-during-in-flight-read test (T2) is that test — it catches the silent stale-read failure. +- "Hostile QA" test: force adapter failure mid-sequence; verify bypass, then re-enable and verify empty cache. +- Chaos test: cycle the feature flag rapidly under concurrent read/write load; assert no stale reads escape. + +**Section 6 result: 1 gap (write-during-in-flight-read test). Addressed by T2.** + +--- + +## Section 7: Performance Review + +| DIMENSION | ASSESSMENT | +|---|---| +| N+1 queries | None introduced. Cache reduces DB calls, not adds. | +| Memory | 16 MiB cap is explicit; 900 hot key DTOs are small; bounded. | +| DB indexes | No new queries; existing indexes unchanged. | +| Caching opportunities | This plan IS the caching. All hot reads covered. | +| Slow paths | Cold start: 900 cache misses on restart. Spread over time by single-flight coalescing and rolling traffic. Plan claims within existing DB capacity — reasonable given 900 reads over 30s TTL window. | +| Connection pool | No new connections. Cache eliminates DB round-trips. | + +Cold-start analysis: 900 hot keys × average read latency ≈ 900 DB reads over the first ~30 seconds of traffic. Current DB handles ~N reads/s at 70% CPU. If N >> 30, cold start is within capacity. Plan states this explicitly — the service owner should verify against the current read rate during rollout monitoring. + +**Section 7 result: 0 issues. Cold start assertion is a monitoring checkpoint, not a plan gap.** + +--- + +## Section 8: Observability & Debuggability Review + +**Metrics specified in plan:** hit/miss rate, eviction rate, cache bytes, fallback errors, DB CPU, read p95. This is complete for the acceptance criteria. + +**Gap: alert thresholds not specified.** + +The plan says "monitor" but doesn't define when the service owner should be paged vs. just watching. For a staged rollout with an active owner, manual monitoring is acceptable. But for ongoing operation post-100% rollout, the missing thresholds become an operational gap. + +Suggested thresholds (P3 TODO, not blocking ship): +- Page if: hit rate < 40% sustained for 5 minutes (cache not working) +- Page if: fallback error rate > 1% (adapter instability) +- Warn if: cache bytes > 14 MiB (near 16 MiB cap, possible eviction pressure) + +**Logging:** secrets and raw IDs excluded per plan. Structured log lines at bypass entry/exit and eviction spikes would aid post-incident debugging. Not required for ship. + +**Debuggability at 3 weeks post-ship:** With hit/miss, eviction, and fallback error metrics, you can reconstruct most incident scenarios. The stale-read scenario (before T1 fix) would have been invisible — this is why T1 is P1. + +**Runbook for fallback errors:** Disable the feature flag → service reverts to direct DB → investigate adapter state → re-enable creates empty cache. + +**Auto-decided (D3):** Alert thresholds as P3 TODO. Non-blocking. + +**Section 8 result: 1 minor gap (alert thresholds). Addressed as P3 TODO (T3).** + +--- + +## Section 9: Deployment & Rollout Review + +| CHECK | STATUS | +|---|---| +| DB migration | None required. | +| Feature flag | YES — existing runtime flag; staged rollout 10% → 50% → 100% | +| Rollout order | Flag enable is the deploy; no schema changes, no migration sequencing needed. | +| Rollback plan | Disable flag → instant bypass → empty cache on re-enable. Full rollback < 1 minute. | +| Deploy-time risk window | Flag-gated; old and new behavior coexist cleanly. 10% of keys test the wrapper while 90% go direct. | +| Environment parity | Staging test assumed; plan says "existing DB capacity" — verify staging has representative load. | +| Post-deploy verification | Hit rate, fallback errors, DB CPU at each stage; one healthy hour before advancing. | +| Smoke tests | Existing contract tests; new wrapper tests (T1+T2) must pass before flag enable. | + +**"10% of keys" flag semantics:** The rollout applies consistently to both read and write paths for a given key. This is the only correct interpretation — partial application would create inconsistency (reads cache, writes don't invalidate). Recommend confirming flag implementation applies to both paths per key in the same call. + +**Section 9 result: 0 risks. Flag semantics worth verifying in implementation.** + +--- + +## Section 10: Long-Term Trajectory Review + +| DIMENSION | ASSESSMENT | +|---|---| +| Technical debt | Minimal. Thin wrapper, no new abstraction layer. | +| Path dependency | None. Plan explicitly preserves future replacement path ("without introducing a general cache framework now"). | +| Knowledge concentration | Plan is self-contained and clear. A new engineer reading it in 12 months will understand the problem, the invariant, and the fix. | +| Reversibility | **5/5** — feature flag disable reverts instantly to baseline. One-hour rollback, zero data risk. | +| Ecosystem fit | Standard read-through LRU pattern in JS. No unusual choices. | +| The 1-year question | Reads cleanly. Acceptance targets, invariants, and out-of-scope items are all explicit. | + +Future trajectory: if the service scales to multiple processes, this plan's design makes that upgrade path clear — the "Out of scope" section names distributed caching and cross-process coherence as the next tier. The current interface can be replaced without changing callers. + +**Section 10 result: Reversibility 5/5, 0 debt items.** + +--- + +## Section 11: Design & UX Review + +**SKIPPED** — no UI scope detected. This is a backend-only change with no user-visible interactions, screens, or state changes. + +--- + +## Outside Voice + +Codex review skipped (codex_reviews disabled). Re-enable: `gstack-config set codex_reviews enabled`. + +--- + +## Required Outputs + +### NOT in scope (considered and deferred) + +| Item | Rationale | +|---|---| +| Distributed caching | Out of stated scope; single-process design is the constraint | +| Cross-process coherence | Explicitly excluded; multi-process startup is rejected by design | +| Cache prewarming | Not needed; hot keys warm within first TTL window via normal traffic | +| Changing consistency semantics | No requirement; existing read-through model is sufficient | +| New product surfaces (API, UI, schema) | Backend-only change; no new surfaces planned | +| General cache framework | Would over-engineer; specific adapter is the right size for this problem | +| Alert threshold automation | P3 TODO; manual monitoring sufficient during rollout and initial operation | + +### What already exists + +| Component | Status | +|---|---| +| LRU adapter (1000 entries, 16 MiB, 30s TTL) | Reused unchanged | +| Single-flight miss coalescing | Reused unchanged | +| Runtime feature flag | Reused for staged rollout | +| Repository contract tests (tenant isolation, key validation, absence, DB failures, authorization) | Extended with wrapper tests | +| Tenant ID + profile ID key encoding | Unchanged; wrapper inherits this property | +| Typed API error mapping | Unchanged; wrapper preserves these error responses | + +### Dream state delta + +``` + CURRENT STATE THIS PLAN DELTA 12-MONTH IDEAL + DB CPU 70% ---> DB CPU < 50% ---> Stable at < 50% under 10x traffic growth + read p95 120ms read p95 < 60ms read p95 < 60ms sustained + all 900 hot reads 60%+ hot reads served full hot-key coverage; optional + hit DB directly from process memory distributed cache if multi-process + needed (separate plan) +``` + +This plan closes the gap from current state to 12-month ideal for the single-process constraint. The distributed-cache path is preserved, not foreclosed. + +### Error & Rescue Registry + +| METHOD | FAILURE | EXCEPTION | RESCUED? | RESCUE ACTION | USER SEES | +|---|---|---|---|---|---| +| readProfile | cache.get throws | CacheAdapterError | YES | Adapter enters bypass; DB read proceeds | Transparent | +| readProfile | DB read rejects | typed DBError | YES | Existing typed error mapping | Existing error response | +| readProfile | cache.set throws | CacheAdapterError | YES | Adapter enters bypass; read returns DB value | Transparent | +| writeProfile | DB write rejects | typed DBError | YES | Existing typed error mapping; cache NOT deleted | Existing error response | +| writeProfile | cache.delete throws | CacheAdapterError | YES | Adapter enters bypass; subsequent reads go to DB | Transparent | + +### Failure Modes Registry + +| CODEPATH | FAILURE MODE | RESCUED? | TEST? | USER SEES | LOGGED? | +|---|---|---|---|---|---| +| readProfile | Cache adapter failure | YES | YES (adapter-failure fallback) | Nothing (transparent) | YES (bypass entry) | +| readProfile | DB read error | YES | YES (existing contract tests) | Existing error response | YES (existing) | +| readProfile | Stale re-insertion after write | NO (gap) → FIXED T1 | NO (gap) → ADDED T2 | Silent stale read | Not without T1+T2 | +| writeProfile | DB write error | YES | YES (failed-write preservation) | Existing error response | YES (existing) | +| writeProfile | Cache delete failure | YES | YES (adapter-failure fallback) | Nothing (transparent) | YES (bypass entry) | +| Feature flag disable | All cache ops bypassed | N/A | YES (existing flag tests) | Nothing (transparent) | YES (flag events) | + +**CRITICAL GAP before T1+T2:** Stale re-insertion after write is RESCUED=N, TEST=N, USER SEES=Silent. T1 and T2 close this gap. + +### Diagrams Summary + +Architecture, data flow, state machine, and error flow produced in sections above. Deployment sequence and rollback are feature-flag based and described in Section 9 prose (no sequence diagram needed — disable flag = rollback, one step). + +### Stale Diagram Audit + +No existing ASCII diagrams found in the plan or referenced files. This plan introduces 4 new diagrams (system architecture, read flow, state machine, cross-reference in Section 4). All current and consistent with the proposed implementation. + +--- + +## Implementation Tasks + +Synthesized from this review's findings. Each task derives from a specific finding above. Run with Claude Code; checkbox as you ship. + +- [ ] **T1 (P1, human: ~30min / CC: ~5min)** — readProfile wrapper — Add post-await guard to prevent stale re-insertion after write invalidation + - Surfaced by: Section 1 (Architecture) — stale re-insertion race; write's cache.delete can execute between the DB read's await and cache.set, allowing an old snapshot back into cache + - Files: repository wrapper module (readProfile function) + - Verify: T2 test passes; run existing contract tests; confirm cache.get returns undefined for a key that was written while its read was in-flight + - Change: after `await repository.read(key)`, replace unconditional `cache.set(key, value)` with `if (cache.get(key) === undefined) { cache.set(key, value); }` + +- [ ] **T2 (P2, human: ~1h / CC: ~10min)** — wrapper tests — Add test for write-during-in-flight-read scenario + - Surfaced by: Section 6 (Tests) — the post-await guard (T1) is untested without a controlled interleaving test + - Files: wrapper test file + - Verify: test fails without T1 fix; passes with it; is deterministic (no timing dependency — use controlled promise resolution) + - Test: pause readProfile at the DB read using a deferred promise; run writeProfile to completion; resolve DB read; assert cache.get(key) === undefined after both settle + +- [ ] **T3 (P3, human: ~30min / CC: ~5min)** — observability — Define alert thresholds for ongoing cache operation post-rollout + - Surfaced by: Section 8 (Observability) — monitoring metrics are specified but alert thresholds are not; fine during active-owner rollout, operational gap post-stabilization + - Files: alerting config / runbook + - Thresholds: hit rate < 40% sustained 5 min → page; fallback error rate > 1% → page; cache bytes > 14 MiB → warn + +_No new tasks from Sections 3, 5, 7, 9, 10, 11._ + +--- + +## Completion Summary + +``` ++====================================================================+ +| MEGA PLAN REVIEW — COMPLETION SUMMARY | ++====================================================================+ +| Mode selected | HOLD SCOPE | +| System Audit | SKIPPED (per run configuration) | +| Step 0 | HOLD SCOPE confirmed; Approach A selected | +| Section 1 (Arch) | 1 CRITICAL GAP (stale re-insertion) → T1 | +| Section 2 (Errors) | 5 error paths mapped, 0 GAPS | +| Section 3 (Security)| 0 issues found, 0 High severity | +| Section 4 (Data/UX) | 1 edge case mapped (stale re-insertion) | +| Section 5 (Quality) | 0 issues found | +| Section 6 (Tests) | Diagram produced, 1 gap (T2) | +| Section 7 (Perf) | 0 issues found | +| Section 8 (Observ) | 1 minor gap (alert thresholds → T3) | +| Section 9 (Deploy) | 0 risks flagged | +| Section 10 (Future) | Reversibility: 5/5, debt items: 0 | +| Section 11 (Design) | SKIPPED (no UI scope) | ++--------------------------------------------------------------------+ +| NOT in scope | written (7 items) | +| What already exists | written (6 components) | +| Dream state delta | written | +| Error/rescue registry| 5 methods, 0 CRITICAL GAPS (post-T1) | +| Failure modes | 6 total, 1 CRITICAL GAP (closed by T1+T2) | +| TODOS.md updates | 1 item (T3, P3, alert thresholds) | +| Scope proposals | 0 proposed, 0 accepted (HOLD SCOPE) | +| CEO plan | skipped (HOLD SCOPE) | +| Outside voice | DISABLED (codex_reviews=disabled) | +| Lake Score | 3/3 recommendations chose complete option | +| Diagrams produced | 4 (architecture, read flow, state machine, | +| | data flow with shadow paths) | +| Stale diagrams found | 0 | +| Unresolved decisions | 0 (D1, D2, D3 all auto-decided) | ++====================================================================+ +``` + +**Auto-decided decisions (no human present):** +- D1: Add post-await guard (stale re-insertion fix) → recommended option accepted → T1 +- D2: Add write-during-in-flight-read test → recommended option accepted → T2 +- D3: Alert thresholds as P3 TODO → recommended option accepted → T3 + +--- + +## GSTACK REVIEW REPORT + +| Review | Trigger | Why | Runs | Status | Findings | +|--------|---------|-----|------|--------|----------| +| CEO Review | `/plan-ceo-review` | Scope & strategy | 1 | issues_open | mode: HOLD_SCOPE, 1 critical gap (stale re-insertion → T1+T2) | +| Outside Review | disabled | Independent 2nd opinion | 0 | DISABLED | codex_reviews=disabled per config | +| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 0 | — | — | +| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — | +| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — | + +**OUTSIDE COVERAGE:** codex_reviews=disabled; outside review step skipped per isolated config. Re-enable: `gstack-config set codex_reviews enabled`. + +**VERDICT:** CEO Review complete — 1 critical gap found and addressed (T1: post-await guard; T2: interleaving test). Eng review required before shipping. + +NO UNRESOLVED DECISIONS diff --git a/test/fixtures/eng-devex-s-first-calls.json b/test/fixtures/eng-devex-s-first-calls.json new file mode 100644 index 000000000..9034f84c6 --- /dev/null +++ b/test/fixtures/eng-devex-s-first-calls.json @@ -0,0 +1,346 @@ +{ + "devex": { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "calls": [ + { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "toolUseId": "toolu_01TVxf9HJmMFvih7VVcDvVJq", + "questions": [ + { + "question": "D1 — Pass 1 (Getting Started): The README quickstart points to a file that doesn't exist in the published package — how should the plan fix it? ", + "header": "Quickstart fix", + "multiSelect": false, + "options": [ + { + "label": "Remove the broken reference (Recommended)", + "description": "Delete the 'python examples/first_eval.py' line from README. The demo command ('python -m evalkit.demo') already works and is the documented magical moment. No new files needed. (human: ~15min / CC: ~2min) ELI10: The README tells a dev to run a file that isn't in the package. Simplest fix: stop mentioning the file that doesn't exist. Completeness: A=9/10" + }, + { + "label": "Create and ship examples/first_eval.py", + "description": "Write a minimal working example script and add it to the package. README reference stays accurate. Developers can read runnable source. (human: ~2h / CC: ~20min) ELI10: Create the missing file so the README is true. Completeness: B=10/10 — both paths work." + }, + { + "label": "Keep reference, add 'see python -m evalkit.demo' note", + "description": "Leave the broken reference but append a note that the demo command is the primary first-run path. Does not fix the FileNotFoundError — just softens the confusion. Completeness: C=4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Pass 1 (Getting Started): The README quickstart points to a file that doesn't exist in the published package — how should the plan fix it? ": "Remove the broken reference (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:43:13.108Z" + }, + { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "toolUseId": "toolu_01L4MrPBSKnZSkQwF6adbR2d", + "questions": [ + { + "question": "D2 — Pass 1 (Getting Started): The plan targets TTHW < 2 min but retains a mandatory 5-minute CI gate with no skip path — these are mutually exclusive. How should the plan resolve this? ", + "header": "CI gate vs TTHW", + "multiSelect": false, + "options": [ + { + "label": "Add --offline / --skip-ci flag for local first-run (Recommended)", + "description": "The demo and first eval skip the remote CI gate when a flag is set. Lets the ML engineer see results in <2 min locally. CI gate still enforces in production CI (noninteractive mode). Matches the persona's stated goal: 'wants a local result before connecting to production CI.' (human: ~1 day / CC: ~30min) Completeness: A=9/10" + }, + { + "label": "Move CI gate to non-blocking background check", + "description": "Return eval results immediately; run the CI check in background and warn on failure. First-run TTHW drops to ~1 min. Risk: developers may act on results before the check completes. (human: ~2 days / CC: ~1h) Completeness: B=8/10" + }, + { + "label": "Revise the TTHW target to reflect the CI wait", + "description": "Change the documented target from <2 min to <7 min (inclusive of the 5-min gate). No code change needed. Honest, but EvalKit stays last-place in the benchmark set and loses the champion-tier positioning. Completeness: C=7/10 — accurate docs, poor competitive position." + }, + { + "label": "Accept current trajectory (6 min, gate retained)", + "description": "Plan makes no change to the CI gate or the TTHW target. The contradiction between the <2 min target and the 5-min gate stays in the docs. Completeness: D=3/10 — unresolved." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — Pass 1 (Getting Started): The plan targets TTHW < 2 min but retains a mandatory 5-minute CI gate with no skip path — these are mutually exclusive. How should the plan resolve this? ": "Add --offline / --skip-ci flag for local first-run (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:43:27.146Z" + }, + { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "toolUseId": "toolu_01HBiwr8VgCqu7erLaUR9RLV", + "questions": [ + { + "question": "D3 — Pass 3 (Error Messages): AuthError(\"request failed\") has no error code, no cause, and no fix. An ML engineer with a typo in their API key sees only 'request failed' and has no path to resolution. Every other SDK error already identifies cause + fix. How should this be repaired? ", + "header": "Auth error fix", + "multiSelect": false, + "options": [ + { + "label": "Full structured error with cause and fix (Recommended)", + "description": "Raise AuthError with message: 'Authentication failed: invalid or missing API key. Check EVALKIT_API_KEY environment variable or pass api_key= explicitly.' Include an error code (e.g. auth/invalid-key) and a link to the key management docs. (human: ~1h / CC: ~5min) Tier 3 quality — matches every other SDK error. Completeness: A=10/10" + }, + { + "label": "Minimal improvement: add cause only", + "description": "Change message to 'Authentication failed: check your EVALKIT_API_KEY.' No error code, no doc link. Fixes the blank-screen problem; still below the SDK's own error standard. (human: ~15min / CC: ~2min) Completeness: B=6/10" + }, + { + "label": "Leave as-is (plan retains this message)", + "description": "Keep AuthError('request failed'). The auth error remains worse than every other SDK error. New developers with a wrong key have no self-service path. Completeness: C=2/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — Pass 3 (Error Messages): AuthError(\"request failed\") has no error code, no cause, and no fix. An ML engineer with a typo in their API key sees only 'request failed' and has no path to resolution. Every other SDK error already identifies cause + fix. How should this be repaired? ": "Full structured error with cause and fix (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:43:41.180Z" + }, + { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "toolUseId": "toolu_018xEouM6cTknAHaPebLNgwP", + "questions": [ + { + "question": "D4 — Pass 2 (API Design): run_eval(dataset, evaluator) and run_batch(evaluator, dataset) accept the same two arguments in reversed order. An ML engineer who copies from one example to the other will silently pass arguments backward. The plan retains this as intentional. How should the plan handle it? ", + "header": "Arg order fix", + "multiSelect": false, + "options": [ + { + "label": "Standardize both to (dataset, evaluator) (Recommended)", + "description": "Fix run_batch to match run_eval's order. Dataset-first is the natural reading order: 'evaluate this dataset with this evaluator.' Add keyword-argument enforcement (Python *) so future callers can't silently pass in the wrong order. (human: ~2h / CC: ~10min) Completeness: A=10/10. Breaking for any run_batch callers passing positional args — changelog entry required." + }, + { + "label": "Keep reversed order; add *-enforcement to both", + "description": "Force keyword arguments for both functions now. Developers can't accidentally swap them. Doesn't fix the asymmetry, but prevents silent bugs going forward. (human: ~1h / CC: ~5min) Completeness: B=7/10" + }, + { + "label": "Retain intentional reversal, document it prominently", + "description": "Add a callout in API docs: 'Note: run_batch takes (evaluator, dataset), opposite of run_eval.' No code change. The asymmetry remains a cognitive trap but is at least disclosed. Completeness: C=4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Pass 2 (API Design): run_eval(dataset, evaluator) and run_batch(evaluator, dataset) accept the same two arguments in reversed order. An ML engineer who copies from one example to the other will silently pass arguments backward. The plan retains this as intentional. How should the plan handle it? ": "Standardize both to (dataset, evaluator) (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:43:53.212Z" + }, + { + "sessionId": "2cd7f591-cb50-4731-bdc0-6378013ab022", + "toolUseId": "toolu_01Y6hhmTxcEspvHnNYHEB6th", + "questions": [ + { + "question": "D5 — Pass 5 (Upgrade Path): Client.evaluate() (v1) is removed immediately in v2 with no compat alias, no deprecation warning, no migration guide, and no codemod. An ML engineer upgrading pip sees an AttributeError on their next run with no explanation of what changed or how to fix it. How should the plan address this? ", + "header": "v1→v2 migration", + "multiSelect": false, + "options": [ + { + "label": "Add alias + deprecation warning + changelog entry (Recommended)", + "description": "Ship Client.evaluate() as a compatibility alias that calls Client.run() and emits a DeprecationWarning with: 'evaluate() is deprecated in v2; use run() instead. This alias will be removed in v3.' Add a migration guide section to the changelog. Remove the alias in v3. (human: ~2h / CC: ~10min) Completeness: A=9/10 — zero surprise upgrades." + }, + { + "label": "Migration guide only, no alias", + "description": "Add a MIGRATION.md with a one-page v1→v2 guide and a changelog entry. No alias. Developers must manually update call sites but have a clear path. (human: ~1h / CC: ~5min) Completeness: B=7/10 — better than nothing, but still breaks silently on upgrade." + }, + { + "label": "Retain immediate removal (plan unchanged)", + "description": "No alias, no warning, no guide. Every v1 codebase breaks silently on pip upgrade. Completeness: C=2/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Pass 5 (Upgrade Path): Client.evaluate() (v1) is removed immediately in v2 with no compat alias, no deprecation warning, no migration guide, and no codemod. An ML engineer upgrading pip sees an AttributeError on their next run with no explanation of what changed or how to fix it. How should the plan address this? ": "Add alias + deprecation warning + changelog entry (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:44:05.242Z" + } + ] + }, + "eng": { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "calls": [ + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01Bzc6ugNvRmNcgALuNBX7Mt", + "questions": [ + { + "question": "D1 — Step 0 scope challenge: 12 files + 4 new classes is overbuilt. Should we reduce scope before reviewing? ", + "header": "Scope", + "multiSelect": false, + "options": [ + { + "label": "Reduce scope (Recommended)", + "description": "ELI10: The plan introduces 4 new classes (TokenStore, SessionMint, AuthCache, RequestPolicy) and touches 12 files. That is a large surface area for a refactor. The 80% solution is: one MultiTenantAuth service wrapping the existing cache adapter, replacing legacyAuthFlow() directly — no parallel class hierarchy. This achieves multi-tenancy without the complexity tax. Completeness: A=8/10 (full multi-tenant auth, smaller blast radius). Note: options differ in kind for the extra classes — see below. Recommend A because engineering preferences favor minimal diff and the additional classes (TokenStore, RequestPolicy) appear to be new abstractions over existing adapter behavior. ✅ Smaller blast radius — fewer files means less merge conflict surface ✅ Each class boundary needs test coverage; fewer classes = faster to complete ❌ May need a follow-up PR to add TokenStore/RequestPolicy if they turn out necessary ❌ If the scope is right-sized already, this cuts work that will have to be re-done" + }, + { + "label": "Proceed as-is", + "description": "Accept the 12-file / 4-class scope and review it thoroughly as written. Completeness: B=10/10 (reviews the full plan). ✅ No scope decisions deferred — all pieces reviewed together ✅ Can catch cross-class coupling issues before implementation begins ❌ Large scope means more findings and more interactive questions ❌ If some classes turn out unnecessary, the review work is partially wasted" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Step 0 scope challenge: 12 files + 4 new classes is overbuilt. Should we reduce scope before reviewing? ": "Reduce scope (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:47:15.523Z" + }, + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01N7vnMMyiD4Mf3QesrrsZRX", + "questions": [ + { + "question": "D2 — Architecture: shared mutable AuthCache via module-level export is a race condition. How should it be fixed? ", + "header": "Architecture", + "multiSelect": false, + "options": [ + { + "label": "Inject AuthCache via constructor (Recommended)", + "description": "[P1] (confidence: 9/10) The plan states: 'Two new services (AuthBroker and SessionMint) share a global mutable AuthCache instance via module-level export. Both services mutate it.' Module-level mutable singletons cannot be isolated in tests and create implicit ordering dependencies between callers. Fix: pass AuthCache as a constructor argument to both services. This also solves the scope-reduction goal — a single MultiTenantAuth service gets one injected cache. Effort: human ~1h / CC ~5min. ✅ Eliminates implicit shared-state coupling between services ✅ Makes both services testable in isolation without module mock tricks ✅ Aligns with the existing cache adapter pattern (the adapter already exists; this is Layer 1 reuse) ❌ Callers must wire the dependency (minimal — typically one DI container line). Completeness: A=9/10", + "preview": "// Before (plan as written)\nimport { authCache } from './cache' // module-level export\n\nclass AuthBroker {\n validate() { authCache.get(...) }\n}\nclass SessionMint {\n mint() { authCache.set(...) }\n}\n\n// After (recommended)\nclass MultiTenantAuth {\n constructor(private cache: AuthCache) {}\n validate() { this.cache.get(...) }\n mint() { this.cache.set(...) }\n}" + }, + { + "label": "Add a mutex/lock around mutations", + "description": "[P1] (confidence: 9/10) Keep the global export but wrap mutations in a lock. Effort: human ~2h / CC ~10min. ✅ Avoids refactoring DI wiring across callers ❌ Adds accidental complexity (Brooks): the lock is solving a problem the module-level export created ❌ Doesn't help test isolation ❌ Still violates the user's 'explicit over clever' preference. Completeness: B=6/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — Architecture: shared mutable AuthCache via module-level export is a race condition. How should it be fixed? ": "Inject AuthCache via constructor (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:47:37.557Z" + }, + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01R1oK3MC2e64Nku3E9JhcCx", + "questions": [ + { + "question": "D3 — Code quality: validateAndDispatch() swallows errors instead of surfacing them. How should it be fixed? ", + "header": "Code quality", + "multiSelect": false, + "options": [ + { + "label": "Refactor to typed error returns (Recommended)", + "description": "[P1] (confidence: 9/10) The plan states: 'validateAndDispatch() is 60 lines with three nested try/catch blocks; each catch swallows a different error class.' Swallowed errors in auth code mean callers can't distinguish token invalid / IDP down / policy denied — all look the same. Fix: decompose into three sub-functions (validateToken, checkPolicy, dispatchSession), each returning a typed Result rather than swallowing. Aligns with 'explicit over clever' preference. Effort: human ~3h / CC ~15min. ✅ Callers can handle each failure class differently (retry vs reject vs audit) ✅ Eliminates 60-line god function ✅ Three focused functions are each unit-testable in isolation ❌ Slightly more code surface (three functions vs one). Completeness: A=9/10" + }, + { + "label": "Add structured logging and re-throw", + "description": "[P2] (confidence: 7/10) Keep the three catch blocks but log the error class and re-throw to the caller. Effort: human ~1h / CC ~5min. ✅ Minimal diff ❌ Doesn't fix the 60-line function or enable typed error handling ❌ Callers still need try/catch; error discrimination moves to instanceof checks at call site ❌ The underlying design stays fragile. Completeness: B=5/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — Code quality: validateAndDispatch() swallows errors instead of surfacing them. How should it be fixed? ": "Refactor to typed error returns (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:48:01.599Z" + }, + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01Ft7tfHP7qakDZgTF5g61jd", + "questions": [ + { + "question": "D4 — Tests: multi-tenant isolation is not explicitly covered in the test plan. Should it be added? ", + "header": "Tests", + "multiSelect": false, + "options": [ + { + "label": "Add tenant isolation tests (Recommended)", + "description": "[P1] (confidence: 9/10) The plan says coverage is planned for 'success/error paths' but does not mention cross-tenant isolation. The cache keys on tenantId+issuer+audience+policyVersion, but tests must verify that an operation on tenant A's key cannot read or evict tenant B's entry. Auth systems where this goes wrong produce privilege escalation bugs. These are integration tests, not unit tests. Effort: human ~2h / CC ~10min. ✅ Catches cache key collision bugs (keying error would silently allow cross-tenant access) ✅ Documents the security invariant as executable specification ❌ Two additional test files; slightly more maintenance surface. Completeness: A=9/10" + }, + { + "label": "Defer — existing adapter tests are sufficient", + "description": "[P2] (confidence: 5/10) The adapter tests already verify the key schema. If the multi-tenant key logic doesn't change, the existing tests transitively cover it. Effort: human ~0h / CC ~0min. ✅ No additional test code ❌ A new service wrapping the adapter can still introduce isolation bugs at the facade layer ❌ Relies on the plan's claim that the adapter is unchanged — unverifiable without code. Completeness: B=4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Tests: multi-tenant isolation is not explicitly covered in the test plan. Should it be added? ": "Add tenant isolation tests (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:48:45.699Z" + }, + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01GdAJYqktTENRvfYPcyt2iT", + "questions": [ + { + "question": "D5 — Performance: 5 sequential IDP calls during token validation could be parallelized. Fix now or defer? ", + "header": "Performance", + "multiSelect": false, + "options": [ + { + "label": "Parallelize in this PR (Recommended)", + "description": "[P2] (confidence: 9/10) The plan explicitly notes: 'Token validation issues 5 sequential API calls to the IDP; they could be parallelized via Promise.all trivially (calls are independent).' With CC, the fix is ~5min. Sequential IDP calls multiply p99 latency by 5x for every auth check. Wrapping independent calls in Promise.all is a Layer 2 standard pattern with no meaningful risk. Effort: human ~30min / CC ~5min. ✅ Reduces auth latency proportionally to IDP round-trip time ✅ The plan already identified this; fixing it now costs essentially nothing with CC ❌ A Promise.all failure mode change: first failure short-circuits; need to verify all 5 calls are truly independent and any single failure should abort. Completeness: A=9/10" + }, + { + "label": "Defer to a follow-up PR", + "description": "[P3] (confidence: 7/10) Ship the auth refactor first, optimize separately. Effort: human ~0h now / ~30min later. ✅ Smaller initial diff ❌ The plan already identified this gap; it will be immediately stale on merge ❌ Auth latency regresses for every user on every request. Completeness: B=4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Performance: 5 sequential IDP calls during token validation could be parallelized. Fix now or defer? ": "Parallelize in this PR (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:48:59.732Z" + }, + { + "sessionId": "0b335784-93fa-4b0a-8248-7d26077f2b4b", + "toolUseId": "toolu_01AEoFpBiADH4oLKs6jkSBDR", + "questions": [ + { + "question": "D6 — TODO: Deferred classes TokenStore and RequestPolicy may be needed for advanced policy scenarios. Capture as TODO? ", + "header": "TODO", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md (Recommended)", + "description": "What: Extract TokenStore and RequestPolicy as standalone components if policy complexity grows. Why: The scope reduction deferred them; if auth rules expand (RBAC, attribute-based policies), the single MultiTenantAuth service will bloat without these boundaries. Pros: captures the intent so future devs know it was considered. Cons: adds a TODOS entry that may never be needed. Completeness: A=8/10", + "preview": "## TODO: TokenStore + RequestPolicy (deferred from multi-tenant auth refactor)\n\n**What:** Extract TokenStore and RequestPolicy as separate\nclasses when policy complexity justifies it.\n\n**Why:** Scope reduction kept them merged into MultiTenantAuth.\nIf RBAC or attribute-based policy rules are added, that class\nwill need these boundaries to stay maintainable.\n\n**Context:** Agreed in D1 scope review. Upgrade when any single\nauth rule set exceeds ~30 lines of logic.\n\n**Blocked by:** Nothing. Independent of current PR." + }, + { + "label": "Skip — not valuable enough", + "description": "If the plan’s scope reduction already made the right call, no need to track these as future work. ✅ Less TODO clutter ❌ If policy complexity does grow, the decision context is lost. Completeness: B=5/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 — TODO: Deferred classes TokenStore and RequestPolicy may be needed for advanced policy scenarios. Capture as TODO? ": "Add to TODOS.md (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:49:43.838Z" + } + ] + } +} diff --git a/test/fixtures/eng-devex-s-retry-calls.json b/test/fixtures/eng-devex-s-retry-calls.json new file mode 100644 index 000000000..fec90b495 --- /dev/null +++ b/test/fixtures/eng-devex-s-retry-calls.json @@ -0,0 +1,412 @@ +{ + "devex": { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "calls": [ + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_01PF1kGiAidK2jLcnwa63hkH", + "questions": [ + { + "question": "D1 — Does this empathy narrative match the real EvalKit onboarding experience? \n\nI traced the actual path from README and docs. Your ML engineer persona today:\n\nT+0:00 — Runs `python -m pip install evalkit==2.0.0b1`. Installs fine.\nT+1:00 — Sets EVALKIT_API_KEY.\nT+1:30 — README says: \"follow the quickstart's command: python examples/first_eval.py\". Runs it.\nT+1:35 — Error: \"can't open file 'examples/first_eval.py': No such file or directory\"\nT+1:40 — Scrolls README, no explanation. Spots python -m evalkit.demo and tries it.\nT+1:45 — stderr: \"Waiting for CI check: 0s elapsed of 300s\"\nT+6:45 — \"CI check completed.\"\nT+6:50 — Finally: example 1: score=0.80 / example 2: score=1.00 / overall: score=0.90\n\nResult: ~7 minutes. The quickstart pointed at a file absent from the package. The demo blocked 5 minutes on an unexplained network hold. Target is under 2 minutes. Does this match reality?", + "header": "Empathy check", + "multiSelect": false, + "options": [ + { + "label": "Accurate — proceed (Recommended)", + "description": "This is correct. Use this as the empathy baseline for all 8 review passes." + }, + { + "label": "Partly wrong — correct it", + "description": "Some details are off. You'll clarify and I'll update before proceeding." + }, + { + "label": "Wrong — actual experience differs", + "description": "The real onboarding path is substantially different. Describe the actual flow." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Does this empathy narrative match the real EvalKit onboarding experience? \n\nI traced the actual path from README and docs. Your ML engineer persona today:\n\nT+0:00 — Runs `python -m pip install evalkit==2.0.0b1`. Installs fine.\nT+1:00 — Sets EVALKIT_API_KEY.\nT+1:30 — README says: \"follow the quickstart's command: python examples/first_eval.py\". Runs it.\nT+1:35 — Error: \"can't open file 'examples/first_eval.py': No such file or directory\"\nT+1:40 — Scrolls README, no explanation. Spots python -m evalkit.demo and tries it.\nT+1:45 — stderr: \"Waiting for CI check: 0s elapsed of 300s\"\nT+6:45 — \"CI check completed.\"\nT+6:50 — Finally: example 1: score=0.80 / example 2: score=1.00 / overall: score=0.90\n\nResult: ~7 minutes. The quickstart pointed at a file absent from the package. The demo blocked 5 minutes on an unexplained network hold. Target is under 2 minutes. Does this match reality?": "Accurate — proceed (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:56:14.770Z" + }, + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_01BcgUMDvFU4UsB9GrrdsVmY", + "questions": [ + { + "question": "D2 — Journey Stage HELLO WORLD: The quickstart file is absent from the published package. \n\nThe README says the first command after install is `python examples/first_eval.py`. But docs/package-contents.txt confirms: \"The quickstart references examples/first_eval.py, but that file is absent from both the published package and the release examples archive.\"\n\nEvery developer who follows the README gets a file-not-found error as their first interaction with EvalKit. This is a P0 blocker — the primary onboarding path is broken before any evaluation runs. The demo module (python -m evalkit.demo) works, but the README leads here first.\n\nStakes if we don't fix: First impression is a crash. The developer doesn't know if it's their fault or the SDK's.\n\nRecommendation: A — replace the quickstart reference in README with the working demo command, because the demo is already packaged and produces the right output.\n\nCompleteness: A=9/10, B=7/10, C=4/10", + "header": "Missing file", + "multiSelect": false, + "options": [ + { + "label": "Fix README to point to demo (Recommended)", + "description": "Replace `python examples/first_eval.py` with `python -m evalkit.demo` as the primary quickstart command. One-line README edit. Completeness: 9/10" + }, + { + "label": "Add first_eval.py to package", + "description": "Write and include examples/first_eval.py in the published package so the README stays correct. Requires authoring the file and updating the package manifest. Completeness: 7/10" + }, + { + "label": "Leave both, add a note", + "description": "Keep the README reference but add a note that the file is not included and point to the demo as an alternative. Lowest effort but perpetuates the confusion. Completeness: 4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — Journey Stage HELLO WORLD: The quickstart file is absent from the published package. \n\nThe README says the first command after install is `python examples/first_eval.py`. But docs/package-contents.txt confirms: \"The quickstart references examples/first_eval.py, but that file is absent from both the published package and the release examples archive.\"\n\nEvery developer who follows the README gets a file-not-found error as their first interaction with EvalKit. This is a P0 blocker — the primary onboarding path is broken before any evaluation runs. The demo module (python -m evalkit.demo) works, but the README leads here first.\n\nStakes if we don't fix: First impression is a crash. The developer doesn't know if it's their fault or the SDK's.\n\nRecommendation: A — replace the quickstart reference in README with the working demo command, because the demo is already packaged and produces the right output.\n\nCompleteness: A=9/10, B=7/10, C=4/10": "Fix README to point to demo (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:56:28.814Z" + }, + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_018Xrf5Na2k8ebPUaMS6Hj3G", + "questions": [ + { + "question": "D3 — Journey Stage HELLO WORLD: The 5-minute mandatory CI block makes the under-2-minute TTHW target unreachable. \n\nThe plan retains the first-run CI gate (docs/current-contracts.md): \"blocks for five minutes before returning an evaluation result. There is no skip flag or offline first-run path.\" The README confirms: \"Like every first evaluation, it currently waits for the mandatory CI check.\"\n\ndocs/benchmarks.md shows the agreed target is under 2 minutes. Competitor A already hits 2 minutes. The 5-minute block applies to EVERY first-time developer, including the demo (`python -m evalkit.demo`). No matter how fast install + auth runs, the gate adds 5 minutes.\n\nThe plan retains this gate. That makes the under-2-minute target a documented goal the plan cannot achieve as written.\n\nStakes if we don't fix: The benchmarks.md target is false. Developers wait 5x longer than the stated goal. The plan ships with a known broken promise.\n\nRecommendation: A — add a demo-mode skip flag that bypasses the CI check for bundled sample data, so `python -m evalkit.demo` is fast while real evaluations keep the gate. The magical moment should not cost 5 minutes.\n\nCompleteness: A=9/10, B=6/10, C=3/10", + "header": "TTHW target", + "multiSelect": false, + "options": [ + { + "label": "Add demo-mode CI skip flag (Recommended)", + "description": "Exempt `python -m evalkit.demo` (bundled sample data only) from the mandatory CI check. Real evaluations keep the gate. TTHW for onboarding becomes ~1 minute. Completeness: 9/10" + }, + { + "label": "Revise target in benchmarks.md", + "description": "Update the documented target to reflect reality: 6-7 minutes. Honest, but accepts losing to all three peer SDKs and abandons the competitive goal. Completeness: 6/10" + }, + { + "label": "Leave as-is, document the wait", + "description": "Keep the gate, keep the target, and add a note in README that first-run takes 5 minutes. Developers are warned but still wait. Target remains broken. Completeness: 3/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — Journey Stage HELLO WORLD: The 5-minute mandatory CI block makes the under-2-minute TTHW target unreachable. \n\nThe plan retains the first-run CI gate (docs/current-contracts.md): \"blocks for five minutes before returning an evaluation result. There is no skip flag or offline first-run path.\" The README confirms: \"Like every first evaluation, it currently waits for the mandatory CI check.\"\n\ndocs/benchmarks.md shows the agreed target is under 2 minutes. Competitor A already hits 2 minutes. The 5-minute block applies to EVERY first-time developer, including the demo (`python -m evalkit.demo`). No matter how fast install + auth runs, the gate adds 5 minutes.\n\nThe plan retains this gate. That makes the under-2-minute target a documented goal the plan cannot achieve as written.\n\nStakes if we don't fix: The benchmarks.md target is false. Developers wait 5x longer than the stated goal. The plan ships with a known broken promise.\n\nRecommendation: A — add a demo-mode skip flag that bypasses the CI check for bundled sample data, so `python -m evalkit.demo` is fast while real evaluations keep the gate. The magical moment should not cost 5 minutes.\n\nCompleteness: A=9/10, B=6/10, C=3/10": "Add demo-mode CI skip flag (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:56:54.414Z" + }, + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_01XoZ2ZdG3rGW5Ssh8ZttgU3", + "questions": [ + { + "question": "D4 — Pass 2 API Design: `run_eval` and `run_batch` take the same two arguments in opposite order. \n\ndocs/api.md shows:\n- `run_eval(dataset, evaluator)` — dataset first\n- `run_batch(evaluator, dataset)` — evaluator first\n\nBoth describe the same concepts. The plan notes \"The reversed positional order is intentional in the current draft; neither function requires keyword arguments.\"\n\nFor an ML engineer using both functions in the same codebase, this is a muscle memory trap. A dev who writes `run_eval(my_dataset, my_eval)` will write `run_batch(my_dataset, my_eval)` by reflex — silently passing the wrong values in the wrong order, with no type error because both arguments are the same type.\n\nThe Pit of Success test: make the right thing easy, the wrong thing hard. This API makes a silent wrong call easy.\n\nStakes: Logic bugs that don't surface at runtime, only in evaluation results. \"Why are my batch scores wrong?\" is a hard debug.\n\nRecommendation: A — standardize on `(dataset, evaluator)` order in both functions, because dataset is the primary noun (what you're evaluating), evaluator is the tool. Both should require keyword arguments or be consistently ordered.\n\nCompleteness: A=10/10, B=7/10, C=4/10", + "header": "Arg order", + "multiSelect": false, + "options": [ + { + "label": "Standardize order to (dataset, evaluator) (Recommended)", + "description": "Fix run_batch to match run_eval's order. Breaking change for v2, so ideal to ship before beta release. Add keyword-argument requirement or deprecation shim for the old order. Completeness: 10/10" + }, + { + "label": "Require keyword arguments in both", + "description": "Leave the positional order inconsistency but require callers to use keyword arguments (`run_batch(evaluator=e, dataset=d)`). Eliminates silent wrong-order calls without changing argument order. Completeness: 7/10" + }, + { + "label": "Leave intentional, add docs warning", + "description": "Keep the reversed order as-is but add a prominent note in the API reference. Preserves current behavior, relies on developers reading docs carefully. Completeness: 4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Pass 2 API Design: `run_eval` and `run_batch` take the same two arguments in opposite order. \n\ndocs/api.md shows:\n- `run_eval(dataset, evaluator)` — dataset first\n- `run_batch(evaluator, dataset)` — evaluator first\n\nBoth describe the same concepts. The plan notes \"The reversed positional order is intentional in the current draft; neither function requires keyword arguments.\"\n\nFor an ML engineer using both functions in the same codebase, this is a muscle memory trap. A dev who writes `run_eval(my_dataset, my_eval)` will write `run_batch(my_dataset, my_eval)` by reflex — silently passing the wrong values in the wrong order, with no type error because both arguments are the same type.\n\nThe Pit of Success test: make the right thing easy, the wrong thing hard. This API makes a silent wrong call easy.\n\nStakes: Logic bugs that don't surface at runtime, only in evaluation results. \"Why are my batch scores wrong?\" is a hard debug.\n\nRecommendation: A — standardize on `(dataset, evaluator)` order in both functions, because dataset is the primary noun (what you're evaluating), evaluator is the tool. Both should require keyword arguments or be consistently ordered.\n\nCompleteness: A=10/10, B=7/10, C=4/10": "Standardize order to (dataset, evaluator) (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:57:22.039Z" + }, + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_01Y1XAbDXc99z6tzdt5eXBXG", + "questions": [ + { + "question": "D5 — Pass 3 Error Messages: The auth error gives developers nothing to act on. \n\ndocs/api.md states: \"For an invalid API key, the SDK raises `AuthError('request failed')`.\nThere is no authentication error code, explanation of the cause, or instruction for replacing the key. The plan retains this message.\"\n\nAuth errors are the single most common first-run failure mode for SDK developers. \"request failed\" tells the developer nothing: not what request, not why it failed, not what to check. They'll grep their code looking for what they broke.\n\nThe DX First Principle: every error = problem + cause + fix. Current message delivers zero of three.\n\nComparison: A Stripe auth failure returns `{\"error\": {\"code\": \"api_key_invalid\", \"message\": \"No API key provided.\", \"type\": \"invalid_request_error\"}}`.\n\nStakes: Developers with a wrong or missing key get a useless message. They try again, check env vars three times, maybe open a support ticket. The fix is one line.\n\nRecommendation: A — change the error message to include cause and fix, because DX POLISH means bulletproofing every touchpoint and this is the highest-traffic error path.\n\nCompleteness: A=10/10, B=6/10", + "header": "Auth error", + "multiSelect": false, + "options": [ + { + "label": "Fix error message to include cause + fix (Recommended)", + "description": "Change AuthError message to something like: \"Authentication failed: API key invalid or missing. Check that EVALKIT_API_KEY is set and valid. See docs/api.md for key setup.\" Completeness: 10/10" + }, + { + "label": "Add error code only, keep existing message", + "description": "Add a machine-readable error code (e.g. AuthError code='auth_key_invalid') without rewriting the human message. Useful for programmatic handling but still unhelpful for the developer reading it. Completeness: 6/10" + }, + { + "label": "Leave as-is (plan's current position)", + "description": "Keep AuthError('request failed'). The plan explicitly retains this. Requires accepting that auth failures are invisible to developers. Completeness: 3/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Pass 3 Error Messages: The auth error gives developers nothing to act on. \n\ndocs/api.md states: \"For an invalid API key, the SDK raises `AuthError('request failed')`.\nThere is no authentication error code, explanation of the cause, or instruction for replacing the key. The plan retains this message.\"\n\nAuth errors are the single most common first-run failure mode for SDK developers. \"request failed\" tells the developer nothing: not what request, not why it failed, not what to check. They'll grep their code looking for what they broke.\n\nThe DX First Principle: every error = problem + cause + fix. Current message delivers zero of three.\n\nComparison: A Stripe auth failure returns `{\"error\": {\"code\": \"api_key_invalid\", \"message\": \"No API key provided.\", \"type\": \"invalid_request_error\"}}`.\n\nStakes: Developers with a wrong or missing key get a useless message. They try again, check env vars three times, maybe open a support ticket. The fix is one line.\n\nRecommendation: A — change the error message to include cause and fix, because DX POLISH means bulletproofing every touchpoint and this is the highest-traffic error path.\n\nCompleteness: A=10/10, B=6/10": "Fix error message to include cause + fix (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:57:45.639Z" + }, + { + "sessionId": "306e0de6-6557-43cf-8502-f320c689b3ad", + "toolUseId": "toolu_016eEvDGhGseLrg8LRLyAnDN", + "questions": [ + { + "question": "D6 — Pass 5 Upgrade Path: `Client.evaluate()` is removed in v2 with no migration path. \n\ndocs/api.md: \"Version 1 exposes `Client.evaluate()`. Version 2 replaces it with `Client.run()` and removes the old name immediately. No compatibility alias, deprecation warning, migration guide, or codemod is provided.\"\n\nAny v1 user who upgrades gets `AttributeError: 'Client' object has no attribute 'evaluate'` with no context — the error tells them nothing about `Client.run()` or why the method disappeared.\n\nThis contradicts the SDK's own stated values: docs/current-contracts.md says \"All other errors already identify the cause, relevant argument or file, and an actionable fix.\" The upgrade path is the one place where this standard is violated.\n\nFor a beta SDK, a breaking rename without a shim is acceptable — but a helpful error message at the call site is not optional. Upgrade fear is a leading cause of SDK abandonment.\n\nStakes: v1 users hit an opaque error. They file issues instead of upgrading. Migration time grows from \"5 minutes\" to \"I need to figure out what changed.\"\n\nRecommendation: A — add a deprecated-method shim that raises a helpful AttributeError, because it's one function and it makes the upgrade self-documenting.\n\nCompleteness: A=9/10, B=7/10, C=4/10", + "header": "v1→v2 upgrade", + "multiSelect": false, + "options": [ + { + "label": "Add deprecated shim with helpful error (Recommended)", + "description": "Keep `Client.evaluate()` but have it raise AttributeError: \"evaluate() was renamed to run() in v2. Replace: client.evaluate(data) -> client.run(data). See changelog for details.\" No behavior, just a self-documenting error. Completeness: 9/10" + }, + { + "label": "Add migration guide only", + "description": "Write a v1-to-v2 migration guide in docs/ that lists the rename. Developers who read changelogs find it. Developers who don't still hit the opaque AttributeError. Completeness: 7/10" + }, + { + "label": "Leave as-is (plan's current position)", + "description": "Keep the silent removal. Beta SDK, breaking changes expected. Developers can read the changelog. Completeness: 4/10" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 — Pass 5 Upgrade Path: `Client.evaluate()` is removed in v2 with no migration path. \n\ndocs/api.md: \"Version 1 exposes `Client.evaluate()`. Version 2 replaces it with `Client.run()` and removes the old name immediately. No compatibility alias, deprecation warning, migration guide, or codemod is provided.\"\n\nAny v1 user who upgrades gets `AttributeError: 'Client' object has no attribute 'evaluate'` with no context — the error tells them nothing about `Client.run()` or why the method disappeared.\n\nThis contradicts the SDK's own stated values: docs/current-contracts.md says \"All other errors already identify the cause, relevant argument or file, and an actionable fix.\" The upgrade path is the one place where this standard is violated.\n\nFor a beta SDK, a breaking rename without a shim is acceptable — but a helpful error message at the call site is not optional. Upgrade fear is a leading cause of SDK abandonment.\n\nStakes: v1 users hit an opaque error. They file issues instead of upgrading. Migration time grows from \"5 minutes\" to \"I need to figure out what changed.\"\n\nRecommendation: A — add a deprecated-method shim that raises a helpful AttributeError, because it's one function and it makes the upgrade self-documenting.\n\nCompleteness: A=9/10, B=7/10, C=4/10": "Add deprecated shim with helpful error (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:58:11.250Z" + } + ] + }, + "eng": { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "calls": [ + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_01YJ9RYv2tb9dKBYui3SMbnN", + "questions": [ + { + "question": "D1 — Complexity check triggered: 12 files, 4 new classes. Reduce scope or proceed as-is? ", + "header": "Scope check", + "multiSelect": false, + "options": [ + { + "label": "Proceed as-is (Recommended)", + "description": "Accept the full scope: AuthBroker, SessionMint, AuthCache facade, TokenStore, RequestPolicy across 12 files. Review all four classes for correctness and coupling, then implement the complete refactor." + }, + { + "label": "Reduce scope first", + "description": "Propose a leaner version: keep AuthBroker + SessionMint as the core services, collapse AuthCache into direct adapter usage (the adapter already exists with all eviction/invalidation logic), and inline RequestPolicy and TokenStore logic rather than introducing standalone classes. Target: 7-8 files, 2 classes." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D1 — Complexity check triggered: 12 files, 4 new classes. Reduce scope or proceed as-is? ": "Proceed as-is (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:56:31.324Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_018Pp3EC7wSd6EHLjWc58EDu", + "questions": [ + { + "question": "D2 — Architecture Issue 1: Global mutable AuthCache shared between two services with no mutation serialization. How should we fix this? ", + "header": "Shared cache", + "multiSelect": false, + "options": [ + { + "label": "Inject AuthCache via constructor (Recommended)", + "description": "Pass the AuthCache instance as a constructor argument to AuthBroker and SessionMint. No module-level singleton. Add a mutex or async queue (e.g. a simple promise-chain lock) to serialize writes. This makes the coupling explicit, testable, and prevents interleaved mutations from concurrent async flows." + }, + { + "label": "Keep global export, add write serialization", + "description": "Retain the module-level singleton but wrap all mutating methods in an async mutex (e.g. using the 'async-mutex' package or a hand-rolled promise queue). Lower refactor surface, but the hidden global dependency remains, making unit testing harder and creating a subtle import-order hazard." + }, + { + "label": "Keep as-is, document the risk", + "description": "Accept the race condition as acceptable for the current scale and add a code comment flagging it. Not recommended: the plan explicitly notes mutations are not serialized, and a cache holding auth tokens corrupted at write time is a security surface, not just a correctness issue." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D2 — Architecture Issue 1: Global mutable AuthCache shared between two services with no mutation serialization. How should we fix this? ": "Inject AuthCache via constructor (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:56:53.399Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_01VwxtKsLQumQ7Ag869jNbBk", + "questions": [ + { + "question": "D3 — Code Quality Issue 1: validateAndDispatch() swallows three distinct error classes across nested try/catch blocks. How should we address this? ", + "header": "Error handling", + "multiSelect": false, + "options": [ + { + "label": "Refactor into typed error propagation (Recommended)", + "description": "Break validateAndDispatch() into 3-4 focused sub-functions (validateToken, dispatchPolicy, recordAudit etc.), each throwing or returning a typed Result. Replace catch-and-swallow with catch-log-rethrow at the entry point boundary only. This aligns with 'explicit over clever' and gives callers actionable error types. (human: ~3h / CC: ~20min)" + }, + { + "label": "Log and rethrow in each catch, keep structure", + "description": "Minimal change: add logger.error() in each catch, then rethrow (or return a discriminated error enum). Keeps the 60-line function intact but stops silently swallowing failures. Faster, but doesn't fix the structural debt. (human: ~30min / CC: ~5min)" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D3 — Code Quality Issue 1: validateAndDispatch() swallows three distinct error classes across nested try/catch blocks. How should we address this? ": "Refactor into typed error propagation (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:57:51.609Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_011quLsKZRH26s88RTtqZNgp", + "questions": [ + { + "question": "D4 — Test Issue 1: No E2E or integration test planned for the cross-service auth flow (AuthBroker → IDP → AuthCache → SessionMint). Add one? ", + "header": "E2E coverage", + "multiSelect": false, + "options": [ + { + "label": "Add E2E integration test for full auth flow (Recommended)", + "description": "Add one E2E test file (auth-flow.e2e.ts or similar) covering: (1) full token validation round-trip against a stubbed IDP, (2) concurrent requests from two tenants (validates mutex correctness), (3) logout triggers cache invalidation. This is the flow the plan's unit tests cannot adequately mock. (human: ~4h / CC: ~25min)" + }, + { + "label": "Unit tests only, skip E2E", + "description": "Accept that unit tests with mocked IDP and mocked cache cover the plan's scope. The integration surface is small enough to trust. Risk: mutex correctness and IDP retry behavior are not verified until production. (human: 0 / CC: 0)" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D4 — Test Issue 1: No E2E or integration test planned for the cross-service auth flow (AuthBroker → IDP → AuthCache → SessionMint). Add one? ": "Add E2E integration test for full auth flow (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:58:43.808Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_01ETKhG4zvAW4PURcXzHVMwh", + "questions": [ + { + "question": "D5 — Performance Issue 1: 5 sequential IDP calls per token validation. Fix with Promise.all in this PR or defer? ", + "header": "IDP calls", + "multiSelect": false, + "options": [ + { + "label": "Fix with Promise.all in this PR (Recommended)", + "description": "Wrap the 5 independent IDP calls in Promise.all(). The plan itself calls this 'trivial' and 'calls are independent.' With CC, this is a ~5-minute change. Leaving it sequential while doing a full auth refactor is the wrong time to ignore a known bottleneck. (human: ~30min / CC: ~5min)" + }, + { + "label": "Defer to a follow-up PR", + "description": "Add a TODO and ship the refactor without the optimization. Acceptable if the IDP calls are already fast enough at current scale and you want to keep the diff minimal. (human: 0 now / CC: 0 now, but requires a follow-up task)" + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D5 — Performance Issue 1: 5 sequential IDP calls per token validation. Fix with Promise.all in this PR or defer? ": "Fix with Promise.all in this PR (Recommended)" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T05:58:59.871Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_01Pfq8t8yzTVsZUGQZdukwii", + "questions": [ + { + "question": "D6 — TODO: Add IDP circuit breaker or retry strategy? The plan parallelizes IDP calls but does not address IDP unavailability. ", + "header": "TODO: IDP CB", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md for later", + "description": "Capture it now so it is not forgotten, but do not scope it into this PR. Context: Promise.all failing on first IDP error means auth is non-functional during IDP brownouts. A circuit breaker (e.g. opossum library) or exponential-backoff retry would isolate failures. Depends on: T5 (IDP parallelization) being merged first." + }, + { + "label": "Build it now in this PR", + "description": "Add a circuit breaker or retry wrapper around the IDP calls as part of this PR. Most natural time to add it since we are already refactoring the IDP call path. (human: ~3h / CC: ~20min)" + }, + { + "label": "Skip — not valuable enough", + "description": "Acceptable if the IDP has its own HA/retry guarantees and application-level retries are not required by SLA." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D6 — TODO: Add IDP circuit breaker or retry strategy? The plan parallelizes IDP calls but does not address IDP unavailability. ": "Add to TODOS.md for later" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T06:01:14.441Z" + }, + { + "sessionId": "4fc2a716-4122-4cd3-bc0e-1128cd9b9819", + "toolUseId": "toolu_01Fc4jMYA8Nxj9ToEEp6ZjF9", + "questions": [ + { + "question": "D7 — TODO: Distributed locking for AuthCache if running multi-instance? The in-process mutex from T1 only protects within a single process. ", + "header": "TODO: dist lock", + "multiSelect": false, + "options": [ + { + "label": "Add to TODOS.md for later", + "description": "Capture the gap: if AuthBroker + SessionMint run as multiple replicas, the mutex from T1 only serializes writes within one process. Cross-process writes to the shared cache can still race. A Redis lock or optimistic concurrency on the underlying adapter would fix it. Depends on: knowing the deployment topology." + }, + { + "label": "Skip — not valuable enough", + "description": "Acceptable if the service runs as a single process or the underlying cache adapter already handles concurrent writes atomically (e.g. Redis SET NX). If the adapter is thread-safe and atomic, the in-process mutex is redundant anyway." + }, + { + "label": "Investigate now before deciding", + "description": "Check if the existing cache adapter (mentioned in the plan) already provides atomic write semantics. If it does, neither the in-process mutex nor a distributed lock are needed for correctness." + } + ] + } + ], + "answered": true, + "failed": false, + "answers": { + "D7 — TODO: Distributed locking for AuthCache if running multi-instance? The in-process mutex from T1 only protects within a single process. ": "Add to TODOS.md for later" + }, + "unansweredQuestionIndices": [], + "answeredAt": "2026-09-09T06:01:24.472Z" + } + ] + } +} diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index dcf0c684b..9ec0931c7 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -1823,17 +1823,54 @@ describe('Design approval reconciliation', () => { expect(section).toContain('With no unresolved findings, no issue question is required.'); }); - test('finish-time check requires exact issue decisions and repairs prematurely accepted drafts', () => { + test('Design blocking Exit checklist reconciles approvals and refreshes stale output after a decision', () => { const main = fs.readFileSync(path.join(ROOT, 'plan-design-review/SKILL.md'), 'utf8'); const check = extractMarkdownSection(main, '## Section self-check'); - expect(check).toContain('reconcile every proposed fix with the exact user decision for that issue'); - expect(check).toContain('an explicit deferral stays unresolved'); - expect(check).toContain('Existing accepted requirements need no repeat approval'); - expect(check).toContain("preamble's authorized auto-decisions"); - expect(check).toContain('return that change to pending before continuing'); - const exitGate = main.indexOf('\n## EXIT PLAN MODE GATE (BLOCKING)'); - expect(exitGate).toBeGreaterThan(0); - expect(main.indexOf('reconcile every proposed fix')).toBeLessThan(exitGate); + expect(check).toContain('Before summaries, review logs or next-step menus, run approval check 0 below.'); + const gate = extractMarkdownSection(main, '## EXIT PLAN MODE GATE (BLOCKING)'); + expect(gate.indexOf('0. Approvals:')).toBeGreaterThanOrEqual(0); + expect(gate.indexOf('0. Approvals:')).toBeLessThan(gate.indexOf('1. Read the plan file')); + expect(gate).toContain("each issue's remedy needs its own AskUserQuestion call and answer."); + expect(gate).toContain("Never group distinct issues."); + expect(gate).toContain('Never group distinct issues. DESIGN.md tokens and navigation are not approval.'); + expect(gate).toContain('Honor prior exact decisions and preamble-authorized per-issue auto-decisions;'); + expect(gate).toContain('preamble-authorized'); + expect(gate).toContain('record why. Deferrals remain unresolved.'); + expect(gate).toContain('If missing, reset drafts to pending, ask and wait.'); + expect(gate).toContain('refresh the plan, report and review log; rerun this gate.'); + expect(gate).toContain('after your most recent write to it'); + }); + + test('CEO blocking Exit checklist rejects setup and approach as issue approval', () => { + const main = fs.readFileSync(path.join(ROOT, 'plan-ceo-review/SKILL.md'), 'utf8'); + const check = extractMarkdownSection(main, '## Section self-check'); + expect(check).toContain('Before summaries, review logs or next-step menus, run approval check 0 below.'); + const gate = extractMarkdownSection(main, '## EXIT PLAN MODE GATE (BLOCKING)'); + expect(gate.indexOf('0. Approvals:')).toBeGreaterThanOrEqual(0); + expect(gate.indexOf('0. Approvals:')).toBeLessThan(gate.indexOf('1. Read the plan file')); + expect(gate).toContain("each issue's remedy needs its own AskUserQuestion call and answer."); + expect(gate).toContain("Never group distinct issues."); + expect(gate).toContain('Never group distinct issues. Setup, mode, approach and navigation are not approval.'); + expect(gate).toContain('Honor prior exact decisions and preamble-authorized per-issue auto-decisions;'); + expect(gate).toContain('preamble-authorized'); + expect(gate).toContain('record why. Deferrals remain unresolved.'); + expect(gate).toContain('If missing, reset drafts to pending, ask and wait.'); + expect(gate).toContain('refresh the plan, report and review log; rerun this gate.'); + expect(check).toContain('Read and execute every section and output'); + expect(check).toContain('STOP, Read it and redo the review'); + expect(main).toContain('one tool_use per issue, no batching'); + expect(main).toContain('even obvious fixes'); + expect(main).toContain('wait for approval before changing the plan'); + expect(main).toContain('Zero findings: state "No issues, moving on"'); + }); + + test('approval entry does not alter other review Exit checklists', () => { + for (const skill of ['plan-eng-review', 'plan-devex-review']) { + const gate = extractMarkdownSection(readSkillUnion(skill), '## EXIT PLAN MODE GATE (BLOCKING)'); + expect(gate).not.toContain('0. Approvals:'); + expect(gate).toContain('1. Read the plan file'); + expect(gate).toContain('5. If a plan file is in context'); + } }); test('task and decision reporting require approval while preserving unanswered findings', () => { diff --git a/test/helpers/ceo-completion-handoff.ts b/test/helpers/ceo-completion-handoff.ts index aeadee446..78480eda3 100644 --- a/test/helpers/ceo-completion-handoff.ts +++ b/test/helpers/ceo-completion-handoff.ts @@ -59,6 +59,21 @@ function describedEngNavigation(question: string, descriptions: string[], contex closedNavigationContext(context); } +/** The next sentence may name the required gate with "it" after the Eng query. */ +function pronounEngGate(question: string, descriptions: string[], context: string): boolean { + if (!/^(?:The\s+)?CEO\s+review\s+is\s+(?:complete|cleared|clean|done)[.!]\s+Run\s+\/plan-eng-review\s+next\?\s+It(?:['’]s|\s+is)\s+the\s+required(?:\s+shipping)?\s+gate\.$/i.test(question)) return false; + const topics = String.raw`(?:architecture|security|test quality|performance)`; + const covers = new RegExp(String.raw`^Covers\s+${topics}(?:,\s+${topics})*(?:,?\s+and\s+${topics})?$`, 'i'); + const sentences = descriptions.flatMap(description => description.trim().split(/[.!](?:\s+|$)/) + .map(sentence => sentence.trim()).filter(Boolean)); + return sentences.every(sentence => covers.test(sentence) || + /^Required\s+gate\s+before\s+shipping$/i.test(sentence) || + /^This\s+CEO\s+review\s+found\s+no\s+architecture\s+concerns,\s+so\s+eng\s+review\s+should\s+be\s+fast$/i.test(sentence) || + /^Proceed\s+without\s+the\s+eng\s+review\s+gate$/i.test(sentence) || + /^You\s+own\s+ensuring\s+correctness\s+before\s+shipping$/i.test(sentence)) && + closedNavigationContext(context); +} + /** Shared closed-review guards; next-review sequencing is still navigation. */ function closedNavigationContext(context: string): boolean { const unfinished = context.replace(/\b(?:no|0)\s+unresolved\s+(?:decisions|gaps|issues|findings)\b/gi, ''); @@ -105,7 +120,8 @@ function manualHandoffIndex(fp: AskUserQuestionFingerprint): number | null { describedEngNavigation(questionText, q.options.map(option => option.description ?? ''), gateContext); const completion = explicitCompletion || describedCompletion || metadataCompletion || describedEngCompletion; const requiredEng = /(?:\bEng(?:ineering)?\s+review|\/plan-eng-review)\b[^.!?]{0,180}\brequired(?:\s+shipping)?\s+gate\b/i.test(gateContext) || - /\brequired(?:\s+shipping)?\s+gate\s+is\s+(?:an?\s+)?(?:Eng(?:ineering)?\s+review|\/plan-eng-review)\b/i.test(gateContext); + /\brequired(?:\s+shipping)?\s+gate\s+is\s+(?:an?\s+)?(?:Eng(?:ineering)?\s+review|\/plan-eng-review)\b/i.test(gateContext) || + pronounEngGate(questionText, q.options.map(option => option.description ?? ''), gateContext); // These native next-review identities share a closed navigation contract; // the question or a following recap cannot hide a new repair obligation. if (id && /^(?:ceo-plan-next-steps|ceo-review-next-(?:steps?|review))$/.test(id) && diff --git a/test/helpers/ceo-section-loading-fixture.ts b/test/helpers/ceo-section-loading-fixture.ts index 788c257e7..fc4746878 100644 --- a/test/helpers/ceo-section-loading-fixture.ts +++ b/test/helpers/ceo-section-loading-fixture.ts @@ -77,6 +77,26 @@ semantics, or adding new product surfaces. The repository interface preserves a future replacement path without introducing a general cache framework now. `; +/** All six events must form one ordered, same-key, post-write reader trace. */ +function hasNumberedStaleFillTrace(text: string): boolean { + const events = text.split('\n').map(line => line.trim().replace(/\s+/g, ' ')).filter(Boolean); + if (events.length !== 6) return false; + const arrow = String.raw`\s*(?:→|->)\s*`; + const backArrow = String.raw`\s*(?:←|<-)\s*`; + const identifier = String.raw`([A-Za-z_$][\w$]*)`; + const read = String.raw`readProfile\(\s*${identifier}\s*\)`; + const write = String.raw`writeProfile\(\s*${identifier}\s*\)`; + const first = new RegExp(String.raw`^t1:\s*${read}${arrow}cache miss${arrow}(?:single-flight${arrow})?await DB read(?: \(suspends\))?$`, 'i').exec(events[0]!); + if (!first) return false; + // Prose keywords are case-insensitive; identifiers remain case-sensitive. + const sameKey = (event: string, pattern: string) => new RegExp(pattern, 'i').exec(event)?.[1] === first[1]; + return sameKey(events[1]!, String.raw`^t2:\s*${write}${arrow}await DB write(?: \(suspends\))?$`) + && sameKey(events[2]!, String.raw`^t3:\s*DB write completes${arrow}cache\.delete\(\s*${identifier}\s*\)${arrow}writeProfile returns$`) + && new RegExp(String.raw`^t4:\s*DB read \(from t1\) completes${arrow}returns (?:old|stale) (?:snapshot|value)$`, 'i').test(events[3]!) + && sameKey(events[4]!, String.raw`^t5:\s*cache\.set\(\s*${identifier}\s*,\s*(?:(?:OLD|STALE)_VALUE|(?:old|stale) (?:snapshot|value))\s*\)${backArrow}(?:old|stale) (?:value|snapshot) (?:re-inserted|refilled) after invalidation!?$`) + && sameKey(events[5]!, String.raw`^t6:\s*(?:next|new|subsequent) ${read}${arrow}cache HIT${arrow}returns (?:old|stale) (?:value|snapshot)${backArrow}(?:INVARIANT|CONTRACT) (?:VIOLATED|VIOLATION)!?$`); +} + /** A named finding may put its ordering evidence in a trace, not one paragraph. */ function hasStructuredStaleFillFinding(report: string): boolean { const lines = report.split('\n'); @@ -100,12 +120,30 @@ function hasStructuredStaleFillFinding(report: string): boolean { if (!fence && !/^\s*>/.test(line) && !/^(?: {4}|\t)/.test(line)) prose[i] = line; } for (let i = 0; i < lines.length; i++) { - if (!/^\*\*CRITICAL FINDING\s*[—–:-].*\*\*\s*$/.test(prose[i]!)) continue; - const previous = prose.slice(0, i).filter(value => value.trim()).at(-1) ?? ''; + const legacy = /^\*\*CRITICAL FINDING\s*[—–:-].*\*\*\s*$/.test(prose[i]!); + let previousIndex = i - 1; + while (previousIndex >= 0 && !prose[previousIndex]!.trim()) previousIndex--; + const numbered = /^\*\*CRITICAL GAP\*\*\s*[—–:-]/.test(prose[i]!) + && /^#{1,6}\s+Critical Finding:\s+\S.*$/i.test(prose[previousIndex] ?? ''); + if (!legacy && !numbered) continue; + const previous = prose.slice(0, numbered ? previousIndex : i).filter(value => value.trim()).at(-1) ?? ''; if (/\b(?:example|template|source|quoted|format)\b[^.]*:\s*$/i.test(previous)) continue; let end = i + 1; while (end < lines.length && !/^(?:#{1,6}\s|\*\*(?:(?:CRITICAL|HIGH|MEDIUM|LOW)\s+)?(?:FINDING|GAP)\b)/i.test(prose[end]!)) end++; - const claim = prose.slice(i + 1, end).join('\n'); + const claim = prose.slice(numbered ? i : i + 1, end).join('\n'); + if (numbered) { + // A quoted requirement alone is insufficient: the same finding must + // independently assert that the current wrapper violates it. + if (!/^\*\*CRITICAL GAP\*\*\s*[—–:-]\s*The plan states: "Every read begun after that write completes must observe the committed version\.(?: TTL expiry is not a substitute for this rule\.)?" The proposed wrapper violates this invariant\.\s*$/m.test(claim)) continue; + for (const trace of traces.filter(trace => trace.start > i && trace.end < end)) { + if (!hasNumberedStaleFillTrace(trace.text)) continue; + // Only this validated same-finding trace becomes prose evidence. + // Reuse all existing dismissal/accepted-staleness checks unchanged; + // this recognizes a finding, not the correctness of its proposed fix. + if (hasProseStaleFillFinding((claim + '\n' + trace.text).replace(/\s+/g, ' '))) return true; + } + continue; + } if (!/^This\s+violates\s+the\s+stated\s+(?:invariant|contract):/m.test(claim) || !/Every read begun after that write\s+completes must observe the committed version/.test(claim)) continue; if (/\b(?:not\s+(?:a\s+)?(?:gap|bug|defect|issue)|no\s+(?:fix|change|guard)\s+(?:is\s+)?(?:needed|required))\b/i.test(claim)) continue; @@ -121,7 +159,10 @@ function hasStructuredStaleFillFinding(report: string): boolean { /** Require an unresolved late-fill defect, not a keyword-bearing dismissal. */ export function hasStaleFillRaceFinding(report: string): boolean { - if (hasStructuredStaleFillFinding(report)) return true; + return hasStructuredStaleFillFinding(report) || hasProseStaleFillFinding(report); +} + +function hasProseStaleFillFinding(report: string): boolean { // Copied source, diagrams and quoted examples cannot supply a finding. const prose = report.replace(/```[\s\S]*?```/g, '').replace(/^\s*>.*$/gm, ''); // Independent list items and table rows cannot borrow each other's words. diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index ce624ac4d..f26eb4823 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -1710,19 +1710,32 @@ function hasCompletePlanReport(expectedPlanPath: string, minimumMtime: number, m } catch { return false; } } -/** A final owned gate with no recorded questions is missing coverage, never success. */ +/** A final owned gate with no review questions is missing coverage, never success. + * The caller may identify completed setup/navigation calls; unclassified, + * pending, or failed questions cannot be dismissed by this failure diagnostic. + */ export function isQuestionlessNativePlanExit( transcript: PlanCountTranscript, expectedPlanPath: string, startedAt: number, screen: string, + nonReviewCalls: ReadonlySet = new Set(), ): boolean { if (!isCurrentPlanApprovalScreen(screen)) return false; - if (transcript.status !== 'ready' || transcript.calls.length) return false; + if (transcript.status !== 'ready' || transcript.calls.some(call => + !call.sessionId || !call.toolUseId || !call.answered || call.failed || + !nonReviewCalls.has(`${call.sessionId}:${call.toolUseId}`) || + !call.questions.length || call.questions.some(q => !call.answers?.[q.question]) || + !Array.isArray(call.unansweredQuestionIndices) || call.unansweredQuestionIndices.length)) return false; const ready = [...(transcript.planReadyRequests ?? [])] .sort((a, b) => Date.parse(a.timestamp) - Date.parse(b.timestamp)).at(-1); if (!ready || ready.failed || !ready.sessionId || !ready.toolUseId) return false; const at = Date.parse(ready.timestamp); - const sessions = new Set([...transcript.assistantMessages.map(m => m.sessionId), + const sessions = new Set([...transcript.calls.map(call => call.sessionId), + ...transcript.assistantMessages.map(m => m.sessionId), ...(transcript.planReadyRequests ?? []).map(r => r.sessionId)]); return sessions.size === 1 && Number.isFinite(at) && at >= startedAt && at <= Date.now() && + transcript.calls.every(call => { + const answerAt = Date.parse(call.answeredAt ?? ''); + return Number.isFinite(answerAt) && answerAt >= startedAt && answerAt < at; + }) && hasCompletePlanReport(expectedPlanPath, startedAt, at, true); } @@ -2002,8 +2015,27 @@ export const engSetupAUQ: Step0BoundaryPredicate = (fp) => { }); }; +/** A native architecture repair may state the defect without an "issue" label. */ +function engExplicitRepairAUQ(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (!call?.answered || call.failed || call.questions.length !== 1 || + !Array.isArray(call.unansweredQuestionIndices) || call.unansweredQuestionIndices.length || fp.signature !== `${call.sessionId}:${call.toolUseId}`) return false; + const q = call.questions[0]!; + if (q.multiSelect || !/^Architecture$/i.test(q.header.trim()) || q.options.length < 2 || + new Set(q.options.map(o => o.label)).size !== q.options.length || + q.options.filter(o => o.label === call.answers?.[q.question]).length !== 1) return false; + const ids = [...q.question.matchAll(/]+)>/gi)]; + if (ids.length !== 1 || (q.question.match(/]+>/i, '').trim().replace(/\s+/g, ' '); + const issue = /^D\s*\d+\s*[—–:-]\s*Architecture:\s+((?:shared|a|an|the)\s+[^.!?]+)\s+is a race condition\.\s+How should (?:it be fixed|we fix it)\?$/i.exec(body); + return Boolean(issue && !/\b(?:false|not|never|no longer|denies?|claim|assertion|example)\b/i.test(issue[1]!)); +} + /** An answered substantive finding can start review even in a mixed setup packet. */ export const engFirstReviewAUQ: Step0BoundaryPredicate = (fp) => { + if (engExplicitRepairAUQ(fp)) return true; const call = fp.nativeCall; if (!call?.answered || call.failed) return false; return call.questions.some(q => { @@ -3041,10 +3073,12 @@ export async function runPlanSkillCounting(opts: { // its report/native evidence before input routing; verified gates still // pass the existing silent-write check below. A positively matched // native question above takes precedence over gate text. - if (opts.expectedPlanPath && terminalHint === 'plan_ready' && countedCalls.size === 0 && - isQuestionlessNativePlanExit(transcript, opts.expectedPlanPath, startedAt, visible)) { + const nonReviewCalls = new Set(fingerprints.filter(fp => fp.preReview || fp.administrative) + .map(fp => fp.signature)); + if (opts.expectedPlanPath && terminalHint === 'plan_ready' && reviewCount === 0 && + isQuestionlessNativePlanExit(transcript, opts.expectedPlanPath, startedAt, visible, nonReviewCalls)) { return snapshot('no_review_questions', - 'Native plan approval reached with zero recorded AskUserQuestion calls; review coverage is missing', visible); + 'Native plan approval reached with zero review-phase AskUserQuestion calls; review coverage is missing', visible); } if (opts.expectedPlanPath && terminalHint === 'plan_ready' && !verifiedTerminal) continue; const permission = nativeQuestionVisible || terminalHint === 'plan_ready' diff --git a/test/helpers/devex-count-fixture.ts b/test/helpers/devex-count-fixture.ts index b35c18427..2a202d816 100644 --- a/test/helpers/devex-count-fixture.ts +++ b/test/helpers/devex-count-fixture.ts @@ -219,6 +219,14 @@ function answeredSetupRepair(fp: AskUserQuestionFingerprint): boolean { /\b(?:mandatory|blocks?|retains? the CI block)\b/i.test(text) && /(?:^|[—–:]\s*)add\s+(?:an?\s+)?(?:skip flag|--skip-ci|offline(?:[- ]first[- ]run)? path)\b/i.test(label); } + if (id === 'plan-devex-review-tthw-ci-block' && /^TTHW target$/i.test(header)) { + // A confirmed benchmark does not approve a new CI bypass. This captured + // menu asserts the broken target and selects an explicit repair. + const headline = question.question.split('\n')[0]!.replace(/]+>/i, '').trim(); + return Array.isArray(call.unansweredQuestionIndices) && call.unansweredQuestionIndices.length === 0 && + /^D\s*\d+\s*[—–:-]\s*Journey Stage HELLO WORLD:\s*The \d+[- ]minute mandatory CI block makes the under-\d+[- ]minute TTHW target unreachable\.\s*$/i.test(headline) && + /^Add (?:a )?demo-mode CI skip flag(?:\s*\(Recommended\))?$/i.test(label); + } if (id === 'plan-devex-review-magical-moment' && /^Magical moment$/i.test(header)) { // The selected option adds progress feedback beyond the already chosen // demo vehicle and prior CI-bypass decision. An unselected remedy or @@ -229,9 +237,33 @@ function answeredSetupRepair(fp: AskUserQuestionFingerprint): boolean { return false; } +/** A current first-pass repair can name the broken contract without its file path. */ +function answeredContractRepair(fp: AskUserQuestionFingerprint): boolean { + const call = fp.nativeCall; + if (!call?.answered || call.failed || call.questions.length !== 1 || + !Array.isArray(call.unansweredQuestionIndices) || call.unansweredQuestionIndices.length || fp.signature !== `${call.sessionId}:${call.toolUseId}`) return false; + const q = call.questions[0]!; + if (q.multiSelect || q.options.length < 2 || new Set(q.options.map(o => o.label)).size !== q.options.length || + q.options.filter(o => o.label === call.answers?.[q.question]).length !== 1 || + administrativeQuestion(q.header, q.question, q.options)) return false; + const ids = [...q.question.matchAll(/]+)>/gi)]; + if (ids.length !== 1 || (q.question.match(/]+>/i, '').trim().replace(/\s+/g, ' '); + if (!/^D\s*\d+\s*[—–:-]\s*Pass\s+1\s*\(Getting Started\):/i.test(body)) return false; + const statement = body.replace(/^D\s*\d+\s*[—–:-]\s*Pass\s+1\s*\(Getting Started\):\s*/i, ''); + const absentPackageFile = /^(?:The )?(?:README )?quickstart points to a file that doesn['’]t exist in the (?:published )?package\b/i.test(statement) && + /\bhow should (?:the plan|we) fix (?:it|this)\?$/i.test(body); + const conflictingGate = /^(?:The )?plan targets TTHW\b[^.!?]*\bbut retains a mandatory\b[^.!?]*\bCI gate with no skip path\b/i.test(statement) && + /\b(?:these are mutually exclusive|these contradict each other)\b/i.test(body) && + /\bhow should (?:the plan|we) resolve (?:this|it)\?$/i.test(body); + return absentPackageFile || conflictingGate; +} + /** A batched native call remains one decision; the caller owns call-ID deduplication. */ export function isDevexReviewIssue(fp: AskUserQuestionFingerprint): boolean { - return answeredSetupRepair(fp) || questionRecords(fp, true).some(substantiveIssue); + return answeredSetupRepair(fp) || answeredContractRepair(fp) || questionRecords(fp, true).some(substantiveIssue); } /** Select POLISH only on the recognized mode menu; leave all other answers unchanged. */ diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 20d0c2245..2dca3e928 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'], 'tpa-apple-ban': ['scripts/resolvers/third-party-actions.ts', 'ship/SKILL.md.tmpl', 'ship/sections/apple-release.md.tmpl', 'scripts/gen-skill-docs.ts', 'test/helpers/session-runner.ts', 'test/skill-e2e-third-party-actions.test.ts'], 'ship-section-loading': ['ship/**', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/skill-e2e-ship-section-loading.test.ts'], - 'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md', 'test/fixtures/ceo-section-loading-q-report.md', 'test/fixtures/ceo-section-r-rejected-report.md'], + 'plan-ceo-section-loading': ['plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/sections.ts', 'scripts/gen-skill-docs.ts', 'test/helpers/auq-sdk-capture.ts', 'test/helpers/session-runner.ts', 'test/session-runner-tools.test.ts', 'test/helpers/ceo-section-loading-fixture.ts', 'test/ceo-section-loading-fixture.test.ts', 'test/skill-e2e-plan-ceo-review-section-loading.test.ts', 'test/fixtures/ceo-section-loading-l-report.md', 'test/fixtures/ceo-section-loading-q-report.md', 'test/fixtures/ceo-section-r-rejected-report.md', 'test/fixtures/ceo-section-s-trace-report.md'], // Data-driven behavioral guard for the 'plan'/'prompt' carves (eng, design, // devex, office-hours + future PR2 carves). One file iterating CARVE_GUARDS; // the selector sets GSTACK_CARVE_SKILL= 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/fixtures/ceo-completion-handoff-calls.json', 'test/fixtures/ceo-completion-handoff-j-calls.json', 'test/fixtures/ceo-completion-handoff-k-calls.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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/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'], - 'plan-eng-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-eng-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'], - 'plan-design-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-design-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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/plan-count-design-questionless-report.md', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'], - 'plan-devex-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-devex-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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/plan-count-design-questionless-report.md', 'test/devex-output-o.test.ts', 'test/fixtures/devex-review-o-calls.json', 'test/fixtures/devex-output-o-retry-call.json', 'test/devex-setup-remedy-o.test.ts', 'test/fixtures/devex-review-o-retry-calls.json', 'test/plan-count-dx-handoff-o.test.ts', 'test/fixtures/devex-handoff-o-call.json'], + 'plan-ceo-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-ceo-review/**', 'test/skill-ceo-section-ordering.test.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/ceo-completion-handoff.ts', 'test/ceo-completion-handoff.test.ts', 'test/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/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'], + 'plan-eng-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-eng-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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'], + 'plan-design-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-design-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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'], + 'plan-devex-finding-count': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-devex-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/plan-count-completion.test.ts', 'test/plan-count-dx-handoff.test.ts', 'test/fixtures/devex-handoff-n-call.json', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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'], // Gate-tier reviewCount-floor counterparts. Catch the May 2026 transcript // bug (model wrote a plan-mode plan and ExitPlanMode'd without firing any @@ -180,7 +180,7 @@ 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/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'], + 'plan-eng-multi-finding-batching': ['bin/gstack-config', 'bin/gstack-skill-start', 'bin/gstack-skill-end', 'plan-eng-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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'], 'plan-ceo-split-overflow': ['bin/gstack-config', 'plan-ceo-review/**', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'bin/gstack-question-preference', 'test/helpers/claude-pty-runner.ts', 'test/plan-count-native-input.test.ts', 'test/helpers/pty-screen.ts', 'test/pty-screen.test.ts', 'test/pty-screen-session.test.ts', 'test/fixtures/pty-screen/**', 'test/helpers/hermetic-skill-runtime.ts', 'test/hermetic-skill-runtime.test.ts', 'test/helpers/claude-pty-runner.unit.test.ts', 'test/helpers/pty-trust-dialog.ts', 'test/pty-trust-dialog.test.ts', 'test/helpers/plan-count-transcript.ts', 'test/helpers/plan-count-pending-exit.ts', 'test/plan-count-pending-exit.test.ts', 'test/plan-count-transcript.test.ts', 'test/helpers/plan-count-artifacts.ts', 'test/plan-count-artifacts.test.ts', 'test/helpers/eval-store.ts', 'test/helpers/plan-count-fixture.ts', 'test/plan-count-fixture.test.ts', 'test/plan-count-timeout.test.ts', 'test/plan-count-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'], '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'], diff --git a/test/plan-count-empty-review.test.ts b/test/plan-count-empty-review.test.ts index d0f91d16d..c2cb70004 100644 --- a/test/plan-count-empty-review.test.ts +++ b/test/plan-count-empty-review.test.ts @@ -7,13 +7,14 @@ import { pathToFileURL } from 'node:url'; import { execFileSync } from 'node:child_process'; import { isQuestionlessNativePlanExit, hasNativePlanTerminal } from './helpers/claude-pty-runner'; import type { PlanCountTranscript } from './helpers/plan-count-transcript'; +import setupCapture from './fixtures/ceo-count-s-distinct.json'; // Exact caller-owned report from a native zero-question run; the Run header is // intentional. Diagnostic failure must not depend on the positive coverage header. const GATE = 'Exit plan mode?\n\nClaude wants to exit plan mode\n❯ 1. Yes, and switch to default (ask each time) for this session\n 2. No\n'; const REPORT = fs.readFileSync(path.join(import.meta.dir, 'fixtures/plan-count-design-questionless-report.md'), 'utf8'); -test.skipIf(process.platform === 'win32')('real PTY fails promptly at an owned native exit with no recorded questions', async () => { +test.skipIf(process.platform === 'win32').each(['none', 'setup'] as const)('real PTY fails promptly at an owned native exit with %s questions', async (questions) => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'empty-native-review-')); const fake = path.join(dir, 'fake-claude'); const worker = path.join(dir, 'worker.ts'); @@ -39,7 +40,12 @@ process.stdin.on('data', data => { if (sent) return; sent = true; fs.writeFileSync(process.env.PROBE_PLAN, process.env.PROBE_REPORT); setTimeout(() => { - fs.writeFileSync(transcript, JSON.stringify({cwd:process.cwd(), sessionId, isSidechain:false, + const records = JSON.parse(process.env.PROBE_SETUP_CALLS || '[]').flatMap((call, index) => { + const timestamp = new Date(Date.now() - 20 + index * 2).toISOString(); + return [{cwd:process.cwd(), sessionId, isSidechain:false, timestamp, message:{role:'assistant',content:[{type:'tool_use', id:call.toolUseId,name:'AskUserQuestion',input:{questions:call.questions}}]}}, + {cwd:process.cwd(), sessionId,isSidechain:false,timestamp:new Date(Date.parse(timestamp)+1).toISOString(),toolUseResult:{answers:call.answers},message:{role:'user',content:[{type:'tool_result',tool_use_id:call.toolUseId,content:'Your questions have been answered: '+Object.entries(call.answers).map(([q,a])=>JSON.stringify(q)+'='+JSON.stringify(a)).join(', ')+'. You can now continue with these answers in mind.'}]}}]; + }); + fs.writeFileSync(transcript, records.map(record=>JSON.stringify(record)+'\n').join('') + JSON.stringify({cwd:process.cwd(), sessionId, isSidechain:false, timestamp:new Date().toISOString(), message:{role:'assistant', content:[ {type:'tool_use', id:'empty-review-exit', name:'ExitPlanMode', input:{}} ]}}) + '\n'); @@ -49,9 +55,9 @@ process.stdin.on('data', data => { process.stdin.resume(); `); fs.chmodSync(fake, 0o755); - fs.writeFileSync(worker, `import { runPlanSkillCounting } from ${JSON.stringify(runner)};\n` + + fs.writeFileSync(worker, `import { runPlanSkillCounting, ceoStep0Boundary, ceoFirstReviewAUQ } from ${JSON.stringify(runner)};\n` + `if (process.env.BROWSE_TERMINAL_BINARY !== ${JSON.stringify(fake)}) throw new Error('fake CLI not selected');\n` + - `const result = await runPlanSkillCounting({skillName:'plan-design-review',slashCommand:'/plan-design-review',followUpPrompt:'# Empty review fixture',expectedPlanPath:${JSON.stringify(report)},isLastStep0AUQ:()=>false,reviewCountCeiling:8,timeoutMs:33000,env:${JSON.stringify({PROBE_PLAN:report,PROBE_INPUTS:inputs,PROBE_PID:pidFile,PROBE_REPORT:REPORT})}});\n` + + `const result = await runPlanSkillCounting({skillName:'plan-design-review',slashCommand:'/plan-design-review',followUpPrompt:'# Empty review fixture',expectedPlanPath:${JSON.stringify(report)},isLastStep0AUQ:ceoStep0Boundary,isFirstReviewAUQ:ceoFirstReviewAUQ,reviewCountCeiling:8,timeoutMs:33000,env:${JSON.stringify({PROBE_PLAN:report,PROBE_INPUTS:inputs,PROBE_PID:pidFile,PROBE_REPORT:REPORT,PROBE_SETUP_CALLS:JSON.stringify(questions === 'setup' ? setupCapture.calls : [])})}});\n` + `await Bun.write(${JSON.stringify(resultFile)},JSON.stringify(result));\n`); const child = Bun.spawn([process.execPath, worker], { env: { ...process.env, EVALS_HERMETIC:'1', EVALS_RUN_ID:'', BROWSE_TERMINAL_BINARY:fake }, @@ -62,10 +68,10 @@ process.stdin.resume(); const [code, out, err] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]); expect(code, out + err).toBe(0); const result = JSON.parse(fs.readFileSync(resultFile, 'utf8')); - expect(result.outcome).toBe('no_review_questions'); + expect(result.outcome, JSON.stringify({summary:result.summary,calls:result.transcript.calls})).toBe('no_review_questions'); expect(result.reviewCount).toBe(0); - expect(result.step0Count).toBe(0); - expect(result.transcript.calls).toHaveLength(0); + expect(result.step0Count).toBe(questions === 'setup' ? 4 : 0); + expect(result.transcript.calls).toHaveLength(questions === 'setup' ? 4 : 0); expect(result.transcript.planReadyRequests).toHaveLength(1); expect(fs.readFileSync(inputs,'utf8').trim().split('\n').map(line => JSON.parse(line))).toEqual(['/plan-design-review\r']); expect(() => process.kill(JSON.parse(fs.readFileSync(pidFile,'utf8')).pid, 0)).toThrow();