Skip to content

feat(plan-ceo-review): every approach states whether its phases are independently mergeable - #2819

Open
frosimanuel wants to merge 1 commit into
garrytan:mainfrom
frosimanuel:pr/plan-ceo-review-phasing-check
Open

feat(plan-ceo-review): every approach states whether its phases are independently mergeable#2819
frosimanuel wants to merge 1 commit into
garrytan:mainfrom
frosimanuel:pr/plan-ceo-review-phasing-check

Conversation

@frosimanuel

Copy link
Copy Markdown

Why (in your own words)

0C-bis makes the CEO review produce 2-3 approaches with effort, risk, pros and cons. It never asks whether the chosen approach can land in pieces. That question is cheap at approach-selection time and expensive later: a plan where nothing is observable until the last phase merges cannot be validated incrementally, and every defect surfaces at once at the end.

Our own closest incident (private KB): a fan-out where six agents built pinned-contract lanes in parallel, each lane green on its own, and the post-integration /code-review found 10 correctness-grade defects, the three worst being cross-lane integration bugs that no single lane could see. The lesson we took from it is the same one that applies to phased plans: if the plan only works once everything has landed, say so before choosing it.

This adds one line to the APPROACH template (Phasing: ships as independently mergeable phases? if not, why) and one rule: each phase of the chosen approach must be mergeable on its own, and a plan where nothing works until every phase lands is a red flag the review calls out.

Live evidence

Setup: a fixture repo with a PLAN.md for migrating an invoices module to event sourcing in four phases (schema, dead code behind a hard-coded-off flag, behaviour-neutral refactor, then flip flag + backfill + DROP TABLE in one phase). An earlier draft of the plan named the problem itself ("phases 1-3 land with zero user-visible change and cannot be validated until phase 4"); two commits labelled docs: tidy removed those lines, so the plan reads clean. The skill directory was copied into .claude/skills/plan-ceo-review/ (upstream main vs this branch), and each copy was run with GSTACK_SESSION_KIND=spawned claude -p "/plan-ceo-review PLAN.md" --setting-sources project, same prompt, same fixture, same model.

Being honest about what changed: both versions found the defect and led with it. Unpatched main already wrote "Phases 1-3 produce zero validation signal … All correctness risk for a 40-call-site ledger rewrite lands in one instant" and recommended re-sequencing into 7 phases where "each phase ships alone and reverts alone." So this PR does not make the review catch something it missed. What it changes is where the phasing question is answered.

Before (upstream main): the transcript contains no 0C-bis approach comparison at all. The approaches are only visible as a one-line auto-decision at the end ("auto-selected Approach B (expand/dual-write/shadow/contract) as recommended"). Phasing is discussed in the findings, after the approach has already been chosen. Occurrences of Phasing: in the transcript: 0.

After (this branch): the 0C-bis block is rendered with the new line on every approach, so mergeability is on the table at the moment the approach is picked:

APPROACH A: Append-only history table (minimal viable)
  ...
  Phasing: Single PR. Trivially revertible.

APPROACH B: Event-sourced repository with a real coexistence period (ideal architecture)
  ...
  Pros:    - Every phase is independently mergeable AND independently revertible
  ...
  Phasing: 7 phases, each shippable alone. This is the whole point.

APPROACH C: The plan exactly as written
  ...
  Phasing: Phases 1-3 mergeable alone. Phase 4 is not — it bundles 4 risk classes.

Occurrences of Phasing: in the transcript: 3 (one per approach). The final verdict, mode and recommendation were the same in both runs (NOT CLEARED, HOLD SCOPE, Approach B, 7-phase re-sequence).

Full claude -p transcripts for both runs are attached as a comment on this PR.

Scope

  • Changed: plan-ceo-review/SKILL.md.tmpl (one template line in 0C-bis, one rule), regenerated plan-ceo-review/SKILL.md.
  • Verified live by: two claude -p runs (upstream main vs this branch) of the skill on the same fixture plan; see Live evidence. Tier 1 bun run test on this branch.
  • Did NOT test: Tier 2 E2E (bun run test:e2e).

Tier 1 note. bun run test on this branch on my machine (macOS, no Aside) fails only in files unrelated to plan-ceo-review/ (test/codex-hardening.test.ts watchdog timing, test/aside-render.test.ts browse binary, occasionally test/gstack-slug-parity.test.ts under load); the same files fail on upstream main (0530392) on the same machine, and the load-related ones pass when re-run in isolation (68 pass, 0 fail across the five files that had failed). test/parity-suite.test.ts and the skill budget tests pass. A per-branch comparison against main is in a comment below.

Liveness proof (required)

Checklist

  • Liveness screenshot attached: GSTACK PR typed live into a real surface (not edited onto the image)
  • This is not a generated-file-only diff (I edited the source/template and regenerated)
  • No ETHOS.md edits, and no changes to voice / founder perspective / YC references
  • New public command / external service / host adapter has an accepted issue linked (or N/A) — N/A
  • Linked issue or reproduction: see Live evidence

🤖 Generated with Claude Code

https://claude.ai/code/session_01EYQXGocbEnQvhMgpPFhVZu

@trunk-io

trunk-io Bot commented Sep 6, 2026

Copy link
Copy Markdown

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@frosimanuel

Copy link
Copy Markdown
Author

Full claude -p transcripts (assistant text + tool calls, extracted from the session .jsonl): https://gist.github.com/frosimanuel/0395c411f3701d29900c9fbd73c16f67

Files 03/04 are the runs quoted in Live evidence (plan with the self-incriminating lines removed). 01/02 are an earlier fixture where the plan still named its own flaw; both versions flagged it, included for completeness.

@frosimanuel

Copy link
Copy Markdown
Author

Tier 1 comparison against main (promised in the PR body)

Short version: I could not get a fully green Tier 1 on this machine on any ref, including upstream main at 0530392. Every failure on the PR branches is a timing/watchdog test in a file the PR does not touch, and the failure set shifts from run to run. The parity/skeleton-budget tests (test/parity-suite.test.ts) passed on every branch and never appeared in a failing list. Please treat CI as the authoritative gate; this comment exists so the numbers are on the record rather than hand-waved.

Method. bun run scripts/test-free-shards.ts --wall-timeout 1800, one branch at a time, nothing else of mine running. Each branch is a single commit on top of upstream main 0530392. Same macOS box for all runs.

Ref Commit Failing tests / files Failing files
upstream main 0530392 10 / 9 (+1 unhandled error between tests) aside-render, busy-daemon-iron-rule, daemon, diff-scope, e2e/diagram-gate, eval-cli-family, hook-scripts ×3, relink, server-no-import-side-effects
pr/plan-reviews-design-doc-is-data (#2818) 0016f67 3 / 3 e2e/combined-gate, gbrain-local-status, gstack-memory-helpers
pr/plan-ceo-review-phasing-check (#2819), run 1 793d010 60 / 23 see note below
pr/plan-ceo-review-phasing-check (#2819), run 2 793d010 6 / 6 e2e/diagram-gate, e2e/landscape-gate, gbrain-local-status, gstack-memory-helpers, pair-agent-e2e, telemetry
pr/review-false-positives (#2820) 332f360 3 / 2 aside-render ×2, codex-hardening

What the PRs touch. #2818: plan-ceo-review/SKILL.md{,.tmpl}, plan-eng-review/SKILL.md{,.tmpl}. #2819: plan-ceo-review/SKILL.md{,.tmpl}. #2820: review/checklist.md only. None of the failing files above reads those artifacts.

About the 60-failure run on #2819. That run coincided with the machine swapping hard (swap 8.1 GB used of 9.2 GB, ~16 MB of free pages). Individual tests reported 400+ second durations and spawnSync /bin/sh ETIMEDOUT, across 23 unrelated files (brain-sync, relink, hook-scripts, gstack-skill-start, slug-parity…). The immediate re-run of the same commit, same command, dropped to 6 timing-flavoured failures. I am reporting the bad run rather than hiding it, but I don't think it says anything about the two-line change in the branch.

Why the sets don't overlap. aside-render needs a browse binary that this box doesn't have; codex-hardening is a watchdog test; the rest are cooldown/timeout/e2e-render gates that flip under memory pressure. The same kind of test fails on main here.

What I did verify deterministically on each branch: bun run gen:skill-docs is a no-op after the commit (generated SKILL.md matches the template), and the skeleton byte budget holds (56489 / 78735 on #2818 vs 56500 / 79000 caps; #2820 leaves review/SKILL.md byte-identical to main at 61314).

If a maintainer wants a specific failing file re-run in isolation on this branch, say which and I'll attach the log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant