From 33d7b26710dc43c7473b4c91b460dc602128e923 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 5 Aug 2026 19:16:15 +0200 Subject: [PATCH] refactor(ship): ChatGPT verifies Fable's own findings, no Sonnet pass The ChatGPT-author plan loop fanned out a parallel Sonnet read-only verifier per Fable finding before folding accepted ones into a revision context for ChatGPT -- mirroring the default (Fable-authors) loop's pattern, where that separate fact-check makes sense because Fable there is reviewing ChatGPT's own PR/plan work. But in this loop ChatGPT is the plan's sole author and reviser, and it already performs exactly this kind of live verification unprompted (e.g. looking up an exact npm package version while incorporating a finding about it, observed live on issue #585 phase 0). Fable's raw findings -- explicitly labelled unverified, since Fable is read-only and cannot confirm exact repository/registry state -- now go straight to ChatGPT's revision context instead. ChatGPT verifies each one itself before folding it in, and records anything it rejects under a `## Review responses` section in the plan the same way the default loop's revise agent already does, so nothing is silently dropped even though there's no separate accepted/rejected return value anymore. Removes one Sonnet agent call per finding per pass. Updates review-loops.md and SKILL.md, which documented the removed pattern as a hard rule ("neither ChatGPT nor Fable is a source of truth" / "the workflows return accepted and rejected lists") -- both now describe the ownership-based split: whichever party owns the plan/PR being revised verifies findings against it, never the critic who raised them. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- skills/ship/SKILL.md | 37 +++++++++++++------ .../chatgpt-plan-author-loop.workflow.mjs | 31 +++++++--------- skills/ship/references/review-loops.md | 30 ++++++++++----- skills/ship/tests/workflow-contract.test.mjs | 10 +++-- 4 files changed, 66 insertions(+), 42 deletions(-) diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index ea62d644..2e0add17 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -65,7 +65,11 @@ creation, every `chatgpt-review` invocation, and the merge. code-review loop's fix-accepted-findings agent) uses `sonnet`. Planning/plan-authoring work uses `fable` at `effort: "high"` in the default planner mode. With `--planner chatgpt`, ChatGPT owns every plan draft and revision while Fable/high owns -the read-only approval decision; Sonnet still verifies every substantive finding. +the read-only approval decision. ChatGPT verifies Fable's own findings itself while +revising — it is the plan's sole author and reviser, and it already performs this kind +of live check unprompted (e.g. confirming an exact npm package version) — so this loop +has no separate Sonnet fact-check pass on Fable's findings the way the default plan +loop and the code review loop do on theirs. This split is wired into `references/plan-review-loop.workflow.mjs`, `references/chatgpt-plan-author-loop.workflow.mjs`, and `references/code-review-pass.workflow.mjs`; keep it there when editing those scripts. @@ -89,10 +93,16 @@ This split is wired into `references/plan-review-loop.workflow.mjs`, authoring protocol plus schema-constrained Fable verdicts. Every loop's pass still counts against its cap. - **Every substantive finding is verified against the real repository before it is - trusted** — the loop workflows fan out one read-only verifier per finding; neither - ChatGPT nor Fable is a source of truth. Every finding ends in exactly one state: - accepted-and-fixed, rejected-with-reason, or unresolved. Never silently drop one — - the workflows return `accepted` and `rejected` lists; record them. + trusted, by whichever party owns the plan/PR being revised** — never the critic who + raised it. In the default plan loop and the code review loop that's a separate + read-only Sonnet verifier per finding, since Fable there is the reviewer of ChatGPT's + own PR/plan review; in the ChatGPT-author loop, ChatGPT itself (the plan's sole + author/reviser) verifies each Fable finding while revising, since Fable is read-only + and cannot confirm exact repository state. Either way, every finding ends in exactly + one state — accepted-and-fixed, rejected-with-reason, or unresolved — never silently + dropped: the default/code loops return `accepted` and `rejected` lists to record; the + ChatGPT-author loop's rejections land as `## Review responses` entries in the plan + itself, and a `needs_human` outcome there carries the round's raw `findings`. ### Output capture @@ -250,18 +260,21 @@ mid-loop (footguns). ``` ChatGPT privately authors a complete standalone plan through `plan-author`; Fable - at high effort reviews it read-only against the actual repository. Sonnet read-only - agents verify every substantive Fable finding. Accepted findings and evidence-backed - rebuttals are passed back to ChatGPT, which atomically replaces the canonical plan - with a complete revision in the same conversation. The workflow performs at most - five Fable review passes. ChatGPT alone owns drafts and revisions; Fable/high alone - owns approval. + at high effort reviews it read-only against the actual repository. Fable's raw + findings — labelled unverified, since Fable cannot confirm exact repository state — + are passed straight back to ChatGPT, which verifies each one itself (it already does + this kind of live check unprompted) and atomically replaces the canonical plan with + a complete revision in the same conversation, recording anything it rejects under + `## Review responses`. The workflow performs at most five Fable review passes. + ChatGPT alone owns drafts, revisions, and verifying findings against them; Fable/high + alone owns approval. 3. `status: "approved"` → record the pass count and conversation URL for the ship log; proceed to 2.3. 4. `status: "blocked"` → skip the unit and report the concrete missing decision; do not guess and do not treat this as a review-loop exhaustion. 5. `status: "needs_human"` → **FULL STOP — human decision needed.** Present the latest - plan, the returned `contested` findings, and the conversation URL, and ask the + plan, the last pass's returned findings (`contested` from the default loop, + `findings` from the ChatGPT-author loop), and the conversation URL, and ask the human: approve the latest plan, redirect, or skip the unit. Write no code for this unit before that decision. (`status: "error"` → read the workflow journal, then re-invoke or stop.) diff --git a/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs b/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs index 3ab456d5..5b8c44d0 100644 --- a/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs +++ b/skills/ship/references/chatgpt-plan-author-loop.workflow.mjs @@ -5,7 +5,7 @@ export const meta = { phases: [ { title: 'Author', detail: 'ChatGPT writes or replaces the canonical plan privately' }, { title: 'Review', detail: 'Fable/high reviews the plan read-only against the repository' }, - { title: 'Verify', detail: 'one Sonnet read-only verifier per substantive finding' }, + { title: 'Prepare', detail: 'fold Fable\'s raw findings into revision context for ChatGPT to verify and incorporate itself' }, ], } @@ -37,10 +37,6 @@ const REVIEW_SCHEMA = { } } }, }, } -const VERIFY_SCHEMA = { - type: 'object', additionalProperties: false, required: ['accepted', 'reason'], - properties: { accepted: { type: 'boolean' }, reason: { type: 'string' } }, -} const CONTEXT_SCHEMA = { type: 'object', additionalProperties: false, required: ['written'], properties: { written: { type: 'boolean' } }, @@ -51,7 +47,7 @@ const label = runArgs.unitLabel ?? runArgs.issueUrl let session = runArgs.session ?? null let conversationUrl = runArgs.conversationUrl ?? null let authorContextFile = runArgs.contextFile -let lastContested = { accepted: [], rejected: [] } +let lastFindings = [] for (let pass = 1; pass <= 5; pass++) { log(`ChatGPT plan authoring / Fable review pass ${pass}/5 — ${label}`) @@ -80,26 +76,25 @@ for (let pass = 1; pass <= 5; pass++) { } if (review.verdict === 'APPROVED') return { status: 'approved', passes: pass, session, conversationUrl } - const verified = (await parallel(review.findings.map((finding, index) => () => - agent( - `Adversarially verify this Fable plan-review finding against the ACTUAL repository and canonical plan.\nFinding: ${finding.claim}\nTarget: ${finding.where}\nPlan: ${runArgs.planFile}\nContext: ${runArgs.contextFile}\naccepted=true only when concrete repository, contract, or plan evidence supports it; cite file:line or plan-section evidence either way. ${READ_ONLY}`, - { label: `verify finding ${index + 1}`, phase: 'Verify', schema: VERIFY_SCHEMA, model: 'sonnet' }, - ).then(result => ({ finding, accepted: result.accepted, reason: result.reason })), - ))).filter(Boolean) - const accepted = verified.filter(item => item.accepted) - const rejected = verified.filter(item => !item.accepted) - lastContested = { accepted, rejected } + lastFindings = review.findings if (pass === 5) break + // Fable is read-only and does not have live access to confirm exact repository state, + // registry contents, or line numbers — its findings are unverified claims, not settled + // fact. ChatGPT is the plan's sole author and reviser; rather than a separate Sonnet + // pass fact-checking Fable before handing ChatGPT a pre-filtered accept/reject list, + // ChatGPT verifies each finding itself (it already does this kind of live check when + // revising — e.g. looking up an exact npm package version) and decides whether to fold + // it in or reject it, recording either outcome so nothing is silently dropped. const nextContext = `${runArgs.contextFile}.chatgpt-revision-${pass + 1}.md` const contextWrite = await agent( - `Create ${nextContext} as a complete revision context. Copy the full original delivery contract from ${runArgs.contextFile}, then append a section "Fable review pass ${pass}" containing these accepted findings to incorporate: ${JSON.stringify(accepted)} and these evidence-backed rebuttals to rejected findings: ${JSON.stringify(rejected)}. If neither list has entries, explicitly require a complete reassessment with concrete findings. Mutation boundary: Write ${nextContext} only; no other file, git, gh, task, memory, or chatgpt-review mutation.`, - { label: `prepare revision context ${pass + 1}`, phase: 'Verify', schema: CONTEXT_SCHEMA, model: 'sonnet' }, + `Create ${nextContext} as a complete revision context. Copy the full original delivery contract from ${runArgs.contextFile}, then append a section "Fable review pass ${pass} — unverified findings" containing these raw findings from an independent read-only reviewer: ${JSON.stringify(review.findings)}. Precede them with this instruction verbatim: "These are UNVERIFIED claims from a reviewer with no live access to confirm exact repository state, registry contents, or line numbers. Before incorporating any finding, verify it yourself against the actual issue, the real repository, and current external sources (e.g. package registries) as needed. Fold in only what you confirm is correct and material. For any finding you determine is wrong, outdated, or already addressed, do not incorporate it — instead add a one-or-two-line entry under a '##' + ' Review responses' section at the end of the plan explaining why, citing your own verification evidence." Mutation boundary: Write ${nextContext} only; no other file, git, gh, task, memory, or chatgpt-review mutation.`, + { label: `prepare revision context ${pass + 1}`, phase: 'Prepare', schema: CONTEXT_SCHEMA, model: 'sonnet' }, ) if (!contextWrite?.written) return { status: 'error', reason: 'could not prepare ChatGPT revision context', pass, session, conversationUrl } authorContextFile = nextContext } -return { status: 'needs_human', reason: 'no Fable APPROVED verdict after 5 passes', passes: 5, session, conversationUrl, contested: lastContested } +return { status: 'needs_human', reason: 'no Fable APPROVED verdict after 5 passes', passes: 5, session, conversationUrl, findings: lastFindings } function shellQuote(value) { return `'${String(value).replaceAll("'", "'\\''")}'` } diff --git a/skills/ship/references/review-loops.md b/skills/ship/references/review-loops.md index 24e3aa4f..e722c32e 100644 --- a/skills/ship/references/review-loops.md +++ b/skills/ship/references/review-loops.md @@ -29,9 +29,14 @@ its task notification; do not poll and do not start other review work meanwhile. PR review contexts include their `VERDICT:` protocol. ChatGPT authoring context instead carries the complete delivery contract; the CLI supplies its strict READY/BLOCKED protocol. -- **Findings are never silently dropped.** Every return carries `accepted` and - `rejected` (with per-finding evidence); the coordinator records them in the ship log - and final report. `rejected` entries become rebuttals, not deletions. +- **Findings are never silently dropped.** In the default plan loop and the code + review loop, every return carries `accepted` and `rejected` (with per-finding + evidence) from an independent Sonnet fact-check; the coordinator records them in the + ship log and final report, and `rejected` entries become rebuttals, not deletions. In + the ChatGPT-author plan loop there is no separate fact-check pass — ChatGPT verifies + Fable's findings itself while revising, and a rejected one must still land as a `## + Review responses` entry in the plan (not a return field); a `needs_human` outcome + there carries the last round's raw `findings` for the coordinator to present. - **Verify the tree after every workflow** (`git diff`, `git log`, `gh pr list`) — the fix and revise agents carry stated mutation boundaries, but a prompt is not an enforced restriction. @@ -74,12 +79,19 @@ Workflow { The canonical plan path never changes. Each pass calls private `plan-author`, has Fable/high review the resulting complete plan read-only against the repository, and -fans out one Sonnet read-only verifier per substantive finding. Before the next pass, -accepted findings and evidence-backed rebuttals are placed in revision context for -ChatGPT, which returns a complete atomic replacement. The loop stops at the first -Fable `APPROVED`, skips the unit on a concrete `BLOCKED`, and returns `needs_human` -after five non-approved Fable passes or an authoring response that remains incomplete -after bounded same-session retries. +passes Fable's raw findings — explicitly labelled unverified — straight into the next +revision's context. Unlike the default loop, there is no separate Sonnet fact-check +pass here: ChatGPT is the plan's sole author and reviser, and it verifies each finding +itself against the actual issue/repository/external sources (it already does this kind +of live check unprompted, e.g. looking up an exact npm package version) before folding +it in. A finding it determines is wrong or already addressed gets rejected the same way +the default loop's revise agent does — a one-or-two-line entry under `## Review +responses` at the end of the plan, not a silent drop; that section is where a rejected +finding's evidence lives for this loop, since there is no `rejected` return value. The +loop stops at the first Fable `APPROVED`, skips the unit on a concrete `BLOCKED`, and +returns `needs_human` (with the last round's raw `findings`, not an accepted/rejected +split) after five non-approved Fable passes or an authoring response that remains +incomplete after bounded same-session retries. ## Code review pass — `code-review-pass.workflow.mjs` diff --git a/skills/ship/tests/workflow-contract.test.mjs b/skills/ship/tests/workflow-contract.test.mjs index 3aba242c..17d24e18 100644 --- a/skills/ship/tests/workflow-contract.test.mjs +++ b/skills/ship/tests/workflow-contract.test.mjs @@ -6,15 +6,19 @@ import { fileURLToPath } from 'node:url'; const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); -test('ChatGPT planner workflow enforces ownership, verification, and the five-pass stop', async () => { +test('ChatGPT planner workflow enforces ownership, ChatGPT-self-verification, and the five-pass stop', async () => { const source = await fs.readFile(path.join(root, 'references/chatgpt-plan-author-loop.workflow.mjs'), 'utf8'); assert.doesNotThrow(() => new Function(`return async function workflowSyntaxCheck() {\n${source.replace('export const meta', 'const meta')}\n}`)); assert.match(source, /for \(let pass = 1; pass <= 5; pass\+\+\)/); assert.match(source, /plan-author/); assert.match(source, /model: 'fable', effort: 'high'/); assert.match(source, /model: 'sonnet'/); - assert.match(source, /accepted = verified\.filter/); - assert.match(source, /rejected = verified\.filter/); + // Fable's findings go to ChatGPT unverified by a separate pass — ChatGPT (the plan's + // sole author/reviser) checks them itself against the real repository before folding + // any in, per the caller instruction embedded in the revision context. + assert.match(source, /UNVERIFIED claims/); + assert.match(source, /lastFindings = review\.findings/); + assert.doesNotMatch(source, /verified\.filter/); assert.match(source, /no Fable APPROVED verdict after 5 passes/); assert.match(source, /status: 'blocked'/); assert.match(source, /status: 'needs_human'/);