From a46ad6e6ec3d0cb8fa0cb5b2935de46376a479e8 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 8 Aug 2026 11:07:52 +0200 Subject: [PATCH 1/2] feat(ship): thread one ChatGPT conversation across a unit's plan and code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chatgpt-review.mjs scopes each session record to one CLI mode+target, so a plan-author-mode session and a pr-mode session were always separate conversations by construction — the "one unit, one ChatGPT session" rule added after #630 phase 5 was broader than the tool actually supports. Add --seed-from-session : starts a brand-new session for the current mode (its own pass counter) but reopens an existing session's conversation instead of a fresh chat, copying conversationUrl and lastResponseFingerprint so the "recover an uncollected response" check doesn't mistake the seed conversation's last message for this mode's own answer. --session and --seed-from-session are mutually exclusive. Wire it into code-review-pass.workflow.mjs (seedFromSession arg, used only on pass 1) and correct review-loops.md/SKILL.md to describe the real mechanism: same-mode resumes always use --session; crossing modes (plan authoring -> PR code review) uses --seed-from-session once, then --session from there. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- skills/chatgpt-review/README.md | 11 ++- skills/chatgpt-review/SKILL.md | 2 + .../chatgpt-review/scripts/chatgpt-review.mjs | 21 +++++- skills/chatgpt-review/scripts/lib/cli.mjs | 19 +++-- skills/chatgpt-review/tests/core.test.mjs | 45 +++++++++++- skills/ship/SKILL.md | 12 +++- .../references/code-review-pass.workflow.mjs | 16 +++-- skills/ship/references/review-loops.md | 70 +++++++++++++------ 8 files changed, 156 insertions(+), 40 deletions(-) diff --git a/skills/chatgpt-review/README.md b/skills/chatgpt-review/README.md index acaab2bc..7adbdbae 100644 --- a/skills/chatgpt-review/README.md +++ b/skills/chatgpt-review/README.md @@ -187,7 +187,10 @@ The generated upload is stored in a permission-restricted temporary directory an ```text --question-file Add focused project and acceptance context --output-file Absolute canonical plan path (plan-author only) ---session Continue the exact saved ChatGPT conversation +--session Continue the exact saved ChatGPT conversation (same mode+target) +--seed-from-session Start a NEW mode's session, but reopen an existing conversation + from a DIFFERENT mode/target's session instead of a fresh + chat. Mutually exclusive with --session. --timeout Completion timeout; default 1800 (30 minutes) --format json|text Output format; default json --cdp-url Chrome CDP endpoint @@ -237,7 +240,11 @@ Only `completed` means a complete response. `timed_out` may include partial resp ## Browser and permission behavior -- Starting without `--session` creates a fresh ChatGPT conversation. +- Starting without `--session` or `--seed-from-session` creates a fresh ChatGPT conversation. +- `--seed-from-session ` creates this mode's own new session (its own pass counter) + but reopens ``'s conversation instead of a fresh one — for threading one unit of + work (e.g. plan authoring, then that plan's PR review) through DIFFERENT CLI modes without + losing conversational context. Once that new session exists, resume it with `--session`. - A matching open tab is reused for a session; otherwise the saved conversation URL is reopened. - Completion requires a new non-empty assistant response, no active generation control, and stable text for seven seconds. - A visible **Continue generating** control is handled automatically. diff --git a/skills/chatgpt-review/SKILL.md b/skills/chatgpt-review/SKILL.md index 5e1a2f4a..8f54643c 100644 --- a/skills/chatgpt-review/SKILL.md +++ b/skills/chatgpt-review/SKILL.md @@ -27,6 +27,8 @@ Leave ChatGPT's predefined model and effort unchanged; the script must not open For a PR fix review, retain the returned `session` handle and invoke the same PR with `--session `. The script reuses that conversation and permits at most three total passes. Ask it only after accepted findings have been fixed and pushed. +To keep one whole unit of work (e.g. plan authoring, then its PR's code review) in a single ChatGPT conversation across DIFFERENT modes, pass `--seed-from-session ` — naming a session from any earlier mode/target — the FIRST time the new mode starts, instead of `--session`. This creates that mode's own new session (its own pass counter) but reopens the same browser tab/conversation rather than starting a fresh chat. `--session` and `--seed-from-session` are mutually exclusive; once the new mode's own session exists, resume it with `--session` like any other, never re-seed. + If a run ends after submission with an incomplete typed status, retry with its returned `session` handle. The script resumes an active or already-finished uncollected response instead of sending the prompt twice. For `plan-author`, use the canonical issue URL and keep the absolute output path diff --git a/skills/chatgpt-review/scripts/chatgpt-review.mjs b/skills/chatgpt-review/scripts/chatgpt-review.mjs index 5b7ae355..576b48f2 100755 --- a/skills/chatgpt-review/scripts/chatgpt-review.mjs +++ b/skills/chatgpt-review/scripts/chatgpt-review.mjs @@ -41,7 +41,19 @@ export async function run(argv, dependencies = {}) { session = await store.load(options.session); if (session.mode !== options.mode || session.targetIdentity !== prepared.targetIdentity) throw new CliError('Session does not match this mode and target'); } else { - session = await store.create({ mode: options.mode, targetIdentity: prepared.targetIdentity, canonicalUrl: prepared.target?.canonicalUrl }); + // --seed-from-session threads an EXISTING ChatGPT conversation (from a different mode, + // e.g. this unit's own plan-author session) into a brand-new session record for this + // mode+target, so pageFor() reopens that conversation instead of a fresh chat. Copying + // lastResponseFingerprint too is required, not cosmetic: without it, review()'s + // hasUncollected check would see the seed conversation's last (unrelated) assistant + // message as an "uncollected response" and return it without ever sending this mode's + // prompt. + let seed = {}; + if (options.seedFromSession) { + const seedSession = await store.load(options.seedFromSession); + seed = { conversationUrl: seedSession.conversationUrl, lastResponseFingerprint: seedSession.lastResponseFingerprint }; + } + session = await store.create({ mode: options.mode, targetIdentity: prepared.targetIdentity, canonicalUrl: prepared.target?.canonicalUrl, ...seed }); } passNumber = (session.passCount ?? 0) + 1; if (options.mode === 'pr' && passNumber > 3) throw new CliError('PR review sessions permit at most three total passes'); @@ -72,8 +84,13 @@ export async function run(argv, dependencies = {}) { previousSha: session.reportedReviewedSha, uploadName: uploadPath ? path.basename(uploadPath) : null, }); + // A genuinely fresh conversation (neither --session nor --seed-from-session) passes + // null so pageFor() opens chatgpt.com from scratch. Both --session (resuming this exact + // mode+target, even if an earlier pass never got far enough to record a conversationUrl) + // and --seed-from-session (a brand-new session record pre-populated with a prior, + // different-mode conversation) must pass the real session through. const review = await driver.review({ - session: options.session ? session : null, + session: (options.session || options.seedFromSession) ? session : null, prompt, uploadPath, timeoutMs: options.timeoutMs, diff --git a/skills/chatgpt-review/scripts/lib/cli.mjs b/skills/chatgpt-review/scripts/lib/cli.mjs index acc18ddd..5b0b2ec6 100644 --- a/skills/chatgpt-review/scripts/lib/cli.mjs +++ b/skills/chatgpt-review/scripts/lib/cli.mjs @@ -14,7 +14,7 @@ export const EXIT_CODES = Object.freeze({ }); const VALUE_FLAGS = new Set([ - '--question-file', '--session', '--timeout', '--format', '--repo', '--base', + '--question-file', '--session', '--seed-from-session', '--timeout', '--format', '--repo', '--base', '--cdp-url', '--diagnostics-dir', '--output-file', ]); const BOOL_FLAGS = new Set(['--publish', '--no-publish', '--working-tree', '--include-untracked']); @@ -22,11 +22,17 @@ const BOOL_FLAGS = new Set(['--publish', '--no-publish', '--working-tree', '--in export function usage() { return `Usage: chatgpt-review.mjs doctor [--cdp-url ] [--format json|text] - chatgpt-review.mjs pr [--question-file ] [--session ] [--no-publish] [--timeout 1800] - chatgpt-review.mjs issue [--question-file ] [--session ] [--publish] [--timeout 1800] - chatgpt-review.mjs plan [--question-file ] [--session ] [--timeout 1800] - chatgpt-review.mjs plan-author --output-file --question-file [--session ] [--timeout 1800] - chatgpt-review.mjs local [--repo ] [--base ] [--working-tree] [--include-untracked] [--question-file ] [--session ] [--timeout 1800]`; + chatgpt-review.mjs pr [--question-file ] [--session |--seed-from-session ] [--no-publish] [--timeout 1800] + chatgpt-review.mjs issue [--question-file ] [--session |--seed-from-session ] [--publish] [--timeout 1800] + chatgpt-review.mjs plan [--question-file ] [--session |--seed-from-session ] [--timeout 1800] + chatgpt-review.mjs plan-author --output-file --question-file [--session |--seed-from-session ] [--timeout 1800] + chatgpt-review.mjs local [--repo ] [--base ] [--working-tree] [--include-untracked] [--question-file ] [--session |--seed-from-session ] [--timeout 1800] + + --session : resume THIS exact mode+target's own prior session (same conversation, same pass counter). + --seed-from-session : start a NEW session for this mode+target, but continue an EXISTING + ChatGPT conversation from a prior session of a DIFFERENT mode (e.g. thread a plan-author + conversation into this PR's own pr-mode review) instead of opening a fresh chat. The new + session gets its own pass counter; only one of --session/--seed-from-session may be given.`; } export function parseArgs(argv, env = process.env) { @@ -50,6 +56,7 @@ export function parseArgs(argv, env = process.env) { } } if (options.publish && options.noPublish) throw new CliError('Use only one of --publish and --no-publish'); + if (options.session && options.seedFromSession) throw new CliError('Use only one of --session and --seed-from-session'); if (options.format && !['json', 'text'].includes(options.format)) throw new CliError('--format must be json or text'); const timeout = Number(options.timeout ?? 1800); if (!Number.isFinite(timeout) || timeout <= 0) throw new CliError('--timeout must be a positive number of seconds'); diff --git a/skills/chatgpt-review/tests/core.test.mjs b/skills/chatgpt-review/tests/core.test.mjs index f16a69de..ed52ad9b 100644 --- a/skills/chatgpt-review/tests/core.test.mjs +++ b/skills/chatgpt-review/tests/core.test.mjs @@ -35,11 +35,16 @@ test('CLI parses documented modes, defaults, environment, and publication rules' }); test('CLI rejects invalid combinations', () => { - for (const args of [[], ['wat'], ['pr'], ['doctor', 'x'], ['local', 'x'], ['issue', 'x', '--publish', '--no-publish'], ['plan', 'x', '--timeout', 'nope'], ['doctor', '--wat'], ['plan-author', 'https://github.com/o/r/issues/1'], ['plan-author', 'https://github.com/o/r/issues/1', '--output-file', 'relative.md', '--question-file', '/tmp/q'], ['plan-author', 'https://github.com/o/r/issues/1', '--output-file', '/tmp/p', '--question-file', '/tmp/q', '--publish']]) { + for (const args of [[], ['wat'], ['pr'], ['doctor', 'x'], ['local', 'x'], ['issue', 'x', '--publish', '--no-publish'], ['plan', 'x', '--timeout', 'nope'], ['doctor', '--wat'], ['plan-author', 'https://github.com/o/r/issues/1'], ['plan-author', 'https://github.com/o/r/issues/1', '--output-file', 'relative.md', '--question-file', '/tmp/q'], ['plan-author', 'https://github.com/o/r/issues/1', '--output-file', '/tmp/p', '--question-file', '/tmp/q', '--publish'], ['pr', 'https://github.com/o/r/pull/1', '--session', 'a', '--seed-from-session', 'b']]) { assert.throws(() => parseArgs(args), CliError); } }); +test('CLI accepts --seed-from-session as a documented value flag', () => { + const parsed = parseArgs(['pr', 'https://github.com/o/r/pull/7', '--seed-from-session', '00000000-0000-4000-8000-000000000009'], {}); + assert.equal(parsed.seedFromSession, '00000000-0000-4000-8000-000000000009'); +}); + test('GitHub targets are canonical and kind checked', () => { assert.deepEqual(normalizeGithubTarget('https://github.com/Owner/repo/pull/007', 'pr'), { kind: 'pr', owner: 'Owner', repo: 'repo', number: 7, @@ -230,6 +235,44 @@ test('run retains a session and enforces three PR passes', async () => { assert.match(result.error, /at most three/); }); +test('--seed-from-session threads a prior, different-mode conversation into a brand-new session', async (t) => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'chatgpt-review-seed-')); + t.after(() => fs.rm(dir, { recursive: true, force: true })); + const store = new SessionStore(dir); + // A prior plan-author session for this SAME unit, already carrying a real conversation + // and the fingerprint of ChatGPT's last message in it (as a completed plan-author pass + // would leave behind). + const seed = await store.create({ + mode: 'plan-author', targetIdentity: 'plan-author:o/r#8:/tmp/plan.md', + conversationUrl: 'https://chatgpt.com/c/seed-conversation', lastResponseFingerprint: 'seed-fingerprint', + }); + const seenSessions = []; + const driver = { + async review({ session }) { + seenSessions.push(session ? { conversationUrl: session.conversationUrl, lastResponseFingerprint: session.lastResponseFingerprint } : null); + return { responseText: `VERDICT: SHIP\nReviewed head ${'c'.repeat(40)}`, conversationUrl: session?.conversationUrl ?? 'https://chatgpt.com/c/new' }; + }, + }; + const result = await run(['pr', 'https://github.com/o/r/pull/1', '--seed-from-session', seed.handle, '--no-publish'], { store, driver }); + assert.equal(result.status, 'completed'); + assert.equal(result.pass_number, 1); + assert.notEqual(result.session, seed.handle); + assert.equal(result.conversation_url, 'https://chatgpt.com/c/seed-conversation'); + // review() must have been called with a session whose conversationUrl/lastResponseFingerprint + // were already the seed's — proving the new pr-mode session reopened that exact conversation + // (rather than opening a fresh chat) and was seeded with the fingerprint needed so the + // "uncollected response" recovery path does not mistake the seed's last message for this + // mode's own answer. + assert.deepEqual(seenSessions, [{ conversationUrl: 'https://chatgpt.com/c/seed-conversation', lastResponseFingerprint: 'seed-fingerprint' }]); + const newSession = await store.load(result.session); + assert.equal(newSession.mode, 'pr'); + assert.equal(newSession.passCount, 1); + // The seed session itself is untouched — seeding copies its conversation, it does not + // consume or mutate the original record. + const reloadedSeed = await store.load(seed.handle); + assert.equal(reloadedSeed.passCount, 0); +}); + test('plan mode uploads a pass-numbered copy (never the literal session-identity path) and never authorizes publication', async (t) => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'chatgpt-review-plan-')); t.after(() => fs.rm(dir, { recursive: true, force: true })); diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index 69e5ae88..b6f275de 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -471,12 +471,20 @@ in the main tree, per pass: Workflow { scriptPath: "skills/ship/references/code-review-pass.workflow.mjs", args: { prUrl, questionFile, session: , pass: , - integrationBranch: "", issueRef: "" } + integrationBranch: "", issueRef: "", + seedFromSession: } } ``` (`integrationBranch` is the script's existing argument name — pass this unit's own -branch into it; no script edit is required.) +branch into it; no script edit is required.) **On pass 1 only** (`session: null`), pass +`seedFromSession` as the session handle returned by this unit's plan loop (step 2.2) — +`chatgpt-review.mjs pr`'s own `--seed-from-session` flag threads that existing ChatGPT +conversation into the brand-new pr-mode session instead of opening a fresh chat, so +planning and code review land in the one conversation `references/review-loops.md`'s +"one unit, one ChatGPT session" rule requires. On pass 2/3 (`session` is now the +code-review loop's own returned handle from pass 1), omit `seedFromSession` — `session` +alone resumes that conversation. The pass reviews (publishing a PR comment), verifies every finding with parallel read-only agents, and — when findings are accepted — applies the fixes with tests, diff --git a/skills/ship/references/code-review-pass.workflow.mjs b/skills/ship/references/code-review-pass.workflow.mjs index 5bf1ef22..4478d69a 100644 --- a/skills/ship/references/code-review-pass.workflow.mjs +++ b/skills/ship/references/code-review-pass.workflow.mjs @@ -9,14 +9,20 @@ export const meta = { ], } -// args: { prUrl, questionFile, session, pass, integrationBranch, issueRef } +// args: { prUrl, questionFile, session, pass, integrationBranch, issueRef, seedFromSession } // The coordinator MUST have the integration branch checked out in the main tree before invoking. +// seedFromSession (only meaningful when session is null, i.e. pass 1): the chatgpt-review +// session handle from this SAME unit's plan-authoring/plan-review loop. Passing it threads +// that existing ChatGPT conversation into this brand-new pr-mode session (--seed-from-session) +// instead of opening a fresh chat, so the whole unit — planning through code review — stays +// one conversation. Omit it only when no prior session exists for this unit (should not happen +// in normal /ship operation) or the coordinator has a specific reason not to thread it. // The workflow runtime has been observed delivering `args` JSON-encoded as a string rather // than parsed, even when the caller passes a real object — normalize defensively so a // well-formed argument is never rejected. const runArgs = typeof args === 'string' ? JSON.parse(args) : args if (!runArgs || !runArgs.prUrl || !runArgs.questionFile || !runArgs.pass || !runArgs.integrationBranch) { - throw new Error('args {prUrl, questionFile, session|null, pass, integrationBranch, issueRef} required') + throw new Error('args {prUrl, questionFile, session|null, pass, integrationBranch, issueRef, seedFromSession} required') } const PASS_SCHEMA = { @@ -69,13 +75,15 @@ const FIX_SCHEMA = { const READ_ONLY = 'Strictly read-only beyond your stated deliverable: no Edit or Write, no git or gh mutations, no task or memory writes, no chatgpt-review invocations beyond the one command given.' log(`Code review pass ${runArgs.pass}/3 — ${runArgs.prUrl}`) -const sessionFlag = runArgs.session ? ` --session ${runArgs.session}` : '' +const sessionFlag = runArgs.session + ? ` --session ${runArgs.session}` + : runArgs.seedFromSession ? ` --seed-from-session ${runArgs.seedFromSession}` : '' const review = await agent( 'A repo-grounded ChatGPT review pass commonly takes 10-25 minutes. Two things you MUST NOT do: (1) run_in_background — a background wait inside this kind of agent call has been observed getting force-terminated (structured-output-enforce) under two minutes in, before any response can exist, regardless of effort; (2) omit --timeout — the script defaults to a 1800s internal wait, but the Bash tool itself hard-kills any FOREGROUND command at 10 minutes with no output flushed, so an uncapped call dies with nothing to read.\n\n' + 'Instead, run this command with Bash IN THE FOREGROUND, with the Bash call\'s own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to a file under $TMPDIR (e.g. `> $TMPDIR/chatgpt-review-pr.json`) — it publishes a PR comment:\n\n' + `node skills/chatgpt-review/scripts/chatgpt-review.mjs pr ${runArgs.prUrl} --question-file ${runArgs.questionFile} --timeout 540${sessionFlag}\n\n` + '--timeout 540 caps the script\'s OWN internal wait at 9 minutes — safely inside the Bash tool\'s 10-minute ceiling — so the process exits cleanly with valid JSON instead of being killed. A "status" of "timed_out" is EXPECTED and NORMAL here, not a failure: the script persists its session handle and conversation URL even on a timeout.\n' + - 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: SHIP" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status, retry immediately. Either way, retry the SAME chatgpt-review command, adding/updating `--session ` from the JSON (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt (it may already have published the comment). Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + + 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: SHIP" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status, retry immediately. Either way, retry the SAME chatgpt-review command, but REPLACE whatever `--session`/`--seed-from-session` flag it had with `--session ` using the "session" field from the JSON (never keep --seed-from-session, and never pass both flags — the CLI rejects that) (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt (it may already have published the comment). Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + 'Then map the final JSON to the output schema:\n' + '- completed: true if response_text contains a real, parseable, single well-formed trailing VERDICT line — regardless of the literal "status" field; false only if no such line exists after all attempts;\n' + '- verdict: the trailing "VERDICT: " line of response_text — SHIP only for a single well-formed "VERDICT: SHIP"; anything absent, duplicated, or malformed is REVISE (fail-closed);\n' + diff --git a/skills/ship/references/review-loops.md b/skills/ship/references/review-loops.md index 9b029a02..4970b544 100644 --- a/skills/ship/references/review-loops.md +++ b/skills/ship/references/review-loops.md @@ -13,9 +13,12 @@ bodies inline, and change them here so there is one copy: - `chatgpt-plan-author-loop.workflow.mjs` — `--planner chatgpt`: ChatGPT authors and Fable/high approves. - `code-review-pass.workflow.mjs` — exactly one PR review pass per call, scoped to one unit's own PR (SKILL.md step 2.6). `pass`/`session` reset to `1`/`null` for every - unit — never carried over from a previous unit's loop, even on the same spine. The - coordinator pushes, waits for CI, and re-invokes (the 3-pass cap is enforced by the - `chatgpt-review` script itself). + unit — never carried over from a previous unit's loop, even on the same spine. + Pass 1's `session: null` should still carry `seedFromSession` set to this unit's own + plan-loop session handle (see "one ChatGPT conversation" below) so it reopens that + conversation instead of starting a disconnected one. The coordinator pushes, waits + for CI, and re-invokes with the returned `session` handle (the 3-pass cap is + enforced by the `chatgpt-review` script itself). These Workflow calls are part of this skill's contract — invoking `/ship` is the explicit multi-agent opt-in. Workflows run in the background: launch one, then wait for @@ -42,30 +45,51 @@ its task notification; do not poll and do not start other review work meanwhile. - **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. -- **One unit, one ChatGPT session, start to finish — never `session: null` a second - time.** A unit's plan authoring, every plan-review round, every PR code-review pass, - and any ad hoc advisory question asked outside the formal pass-counted loop (e.g. - "what's your honest assessment of this fix" after a real fix) all belong in the SAME - conversation. This holds even when a formal loop is exhausted without certification - (5 plan-review passes, or 3 code-review passes with no certified head): do not call - `chatgpt-review`/invoke a review workflow with `session: null` to route around an - exhausted cap or a `needs_human` outcome — that starts a second, disconnected - conversation and throws away everything ChatGPT already reviewed and found. Instead, - after a genuine fix lands (per the human's ruling at a FULL STOP, or per standing - direction already given for this run), continue the EXISTING session. - **The `chatgpt-review.mjs pr` CLI itself hard-rejects a 4th call tied to one session - (`"PR review sessions permit at most three total passes"`, `status: invalid_request`, - exit before any prompt reaches ChatGPT) — this is enforced by the tool, not just the - workflow script's loop bound, so re-invoking the workflow with that session's handle - past pass 3 will fail outright, not silently succeed.** Once a session's 3 formal - `pr`-mode passes are spent, continue it by driving the existing conversation tab - directly instead: read the DOM, submit a revision/advisory/final-verdict message via +- **One unit, one ChatGPT *conversation*, start to finish.** `chatgpt-review.mjs` + scopes each *session record* (handle, pass counter) to one CLI mode + target — a + `plan-author`-mode session and a `pr`-mode session are always separate records, and + `--session ` hard-rejects a handle whose recorded mode doesn't match + (`"Session does not match this mode and target"`). That is a real, permanent + constraint, not a bug to route around: within ONE mode (repeatedly resuming the same + `pr`-mode PR review, or the same `plan`/`plan-author` authoring/revision loop), + always reuse that mode's own session handle — never `session: null` a second time + for work already in progress under that handle. This holds even when a formal loop + is exhausted without certification (5 plan-review passes, or 3 code-review passes + with no certified head): do not call `chatgpt-review`/invoke a review workflow with + `session: null` to route around an exhausted cap or a `needs_human` outcome — that + abandons everything ChatGPT already reviewed and found in that mode's conversation. + Continue the EXISTING session instead (see the pass-cap paragraph below for what to + do once a mode's own cap is truly spent). + + **Across modes** (plan authoring → plan review → PR code review → any ad hoc + advisory question outside a formal pass-counted loop), the CLI cannot literally + share one session RECORD, but it can and should share one underlying ChatGPT + *conversation*: pass `--seed-from-session ` (instead of `--session`) the + FIRST time a new mode starts for this unit, naming the most recent session handle + from whichever mode/loop just finished. This creates a brand-new session record for + the new mode (its own pass counter, so a `pr`-mode cap counts only `pr`-mode passes) + but reopens the SAME browser tab/conversation rather than starting a fresh chat — + `SKILL.md` step 2.6 wires this from the plan loop's returned session into + `code-review-pass.workflow.mjs`'s `seedFromSession` argument on that PR's first pass. + Once that new mode's own session exists, resume IT with `--session` for every + further pass in that mode, per the paragraph above — `--seed-from-session` is only + for the one-time handoff into a new mode, never for continuing within one. + + **The `chatgpt-review.mjs pr` CLI itself hard-rejects a 4th call tied to one + `pr`-mode session (`"PR review sessions permit at most three total passes"`, + `status: invalid_request`, exit before any prompt reaches ChatGPT) — this is + enforced by the tool, not just the workflow script's loop bound, so re-invoking the + workflow with that session's handle past pass 3 will fail outright, not silently + succeed.** Once a session's 3 formal `pr`-mode passes are spent, continue the SAME + conversation by driving the existing tab directly instead of the capped CLI: read + the DOM, submit a revision/advisory/final-verdict message via `document.execCommand('insertText', ...)` + a real click on the send button (same technique as the Chrome-crash recovery path in `SKILL.md` step 2.2), ask explicitly for the standard `VERDICT: SHIP`/`VERDICT: REVISE` protocol if you need a formal certification out of it, and post the result as a PR comment yourself (`gh pr - comment`) since no CLI publish step ran. A fresh session is correct only when - starting a genuinely new unit that has never had one. + comment`) since no CLI publish step ran. A fresh conversation (no `--session` and no + `--seed-from-session`) is correct only when starting a genuinely new unit that has + never had one. ## Default plan loop — `plan-review-loop.workflow.mjs` From 372873eb6be0bde154886b8d95b7b60b331b59e1 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 8 Aug 2026 11:28:22 +0200 Subject: [PATCH 2/2] fix(ship): give each ChatGPT review pass its own output filename code-review-pass.workflow.mjs and plan-review-loop.workflow.mjs told the review-runner agent to redirect chatgpt-review's JSON output to a fixed literal example filename (chatgpt-review-pr.json / chatgpt-review-plan.json) reused across every PR/unit and every pass for an entire /ship run that can span many hours. Observed live on #630 phase 6, pass 2: the review-runner agent's own structured-output step reported an old, already-resolved review (a different PR's, from hours earlier) as this pass's result, even though the real command for this PR/pass had already run and posted a correct comment moments before. The coordinator only caught this by cross-checking the actual posted GitHub comment against the workflow's returned session/SHA/ findings. Derive a unique filename per invocation (PR number + pass, or plan file slug + pass) and require the agent use it exactly, instead of a generic example it's been observed copying verbatim across unrelated passes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- skills/ship/references/code-review-pass.workflow.mjs | 12 ++++++++++-- skills/ship/references/plan-review-loop.workflow.mjs | 11 +++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/skills/ship/references/code-review-pass.workflow.mjs b/skills/ship/references/code-review-pass.workflow.mjs index 4478d69a..b344c8b6 100644 --- a/skills/ship/references/code-review-pass.workflow.mjs +++ b/skills/ship/references/code-review-pass.workflow.mjs @@ -78,10 +78,18 @@ log(`Code review pass ${runArgs.pass}/3 — ${runArgs.prUrl}`) const sessionFlag = runArgs.session ? ` --session ${runArgs.session}` : runArgs.seedFromSession ? ` --seed-from-session ${runArgs.seedFromSession}` : '' +// A generic output filename (e.g. the old literal example chatgpt-review-pr.json) is reused +// across every PR and every pass for an ENTIRE /ship run that can span many hours — observed +// live on #630 phase 6: a review-runner agent's structured-output step reported a stale file's +// content (a different PR's old, already-resolved review) as if it were this pass's real +// result, even though the real command for THIS pr/pass had already run and posted correctly. +// Naming the file after this exact PR+pass makes that class of stale-read impossible to miss. +const prNumberMatch = /\/pull\/(\d+)/.exec(runArgs.prUrl) +const outputFile = `$TMPDIR/chatgpt-review-pr-${prNumberMatch ? prNumberMatch[1] : 'unknown'}-pass${runArgs.pass}.json` const review = await agent( 'A repo-grounded ChatGPT review pass commonly takes 10-25 minutes. Two things you MUST NOT do: (1) run_in_background — a background wait inside this kind of agent call has been observed getting force-terminated (structured-output-enforce) under two minutes in, before any response can exist, regardless of effort; (2) omit --timeout — the script defaults to a 1800s internal wait, but the Bash tool itself hard-kills any FOREGROUND command at 10 minutes with no output flushed, so an uncapped call dies with nothing to read.\n\n' + - 'Instead, run this command with Bash IN THE FOREGROUND, with the Bash call\'s own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to a file under $TMPDIR (e.g. `> $TMPDIR/chatgpt-review-pr.json`) — it publishes a PR comment:\n\n' + - `node skills/chatgpt-review/scripts/chatgpt-review.mjs pr ${runArgs.prUrl} --question-file ${runArgs.questionFile} --timeout 540${sessionFlag}\n\n` + + `Instead, run this command with Bash IN THE FOREGROUND, with the Bash call's own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to EXACTLY this file — do not substitute a generic name. This filename is unique to this PR and pass on purpose: a long /ship run reuses the same $TMPDIR across many PRs and passes over many hours, and a generic filename risks a LATER pass silently reading a stale file left over from an EARLIER one instead of its own real result. It publishes a PR comment:\n\n` + + `node skills/chatgpt-review/scripts/chatgpt-review.mjs pr ${runArgs.prUrl} --question-file ${runArgs.questionFile} --timeout 540${sessionFlag} > ${outputFile}\n\n` + '--timeout 540 caps the script\'s OWN internal wait at 9 minutes — safely inside the Bash tool\'s 10-minute ceiling — so the process exits cleanly with valid JSON instead of being killed. A "status" of "timed_out" is EXPECTED and NORMAL here, not a failure: the script persists its session handle and conversation URL even on a timeout.\n' + 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: SHIP" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status, retry immediately. Either way, retry the SAME chatgpt-review command, but REPLACE whatever `--session`/`--seed-from-session` flag it had with `--session ` using the "session" field from the JSON (never keep --seed-from-session, and never pass both flags — the CLI rejects that) (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt (it may already have published the comment). Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + 'Then map the final JSON to the output schema:\n' + diff --git a/skills/ship/references/plan-review-loop.workflow.mjs b/skills/ship/references/plan-review-loop.workflow.mjs index 89f65a3c..07635fc2 100644 --- a/skills/ship/references/plan-review-loop.workflow.mjs +++ b/skills/ship/references/plan-review-loop.workflow.mjs @@ -67,10 +67,17 @@ const startPass = Number.isInteger(runArgs.startPass) && runArgs.startPass >= 1 for (let pass = startPass; pass <= 5; pass++) { log(`Plan review pass ${pass}/5 — ${label}`) const sessionFlag = session ? ` --session ${session}` : '' + // A generic output filename is reused across every unit's plan review for an ENTIRE /ship + // run that can span many hours — observed live on #630 phase 6's code-review loop (the + // analogous PR-review case): a review-runner agent's structured-output step reported a + // stale file's content (a different unit's old, already-resolved review) as this pass's + // real result. Naming the file after this exact plan file + pass avoids that collision. + const planSlug = runArgs.planFile.split('/').pop().replace(/\.[^.]+$/, '') + const outputFile = `$TMPDIR/chatgpt-review-plan-${planSlug}-pass${pass}.json` const review = await agent( 'A repo-grounded ChatGPT review pass commonly takes 10-25 minutes. Two things you MUST NOT do: (1) run_in_background — a background wait inside this kind of agent call has been observed getting force-terminated (structured-output-enforce) under two minutes in, before any response can exist, regardless of effort; (2) omit --timeout — the script defaults to a 1800s internal wait, but the Bash tool itself hard-kills any FOREGROUND command at 10 minutes with no output flushed, so an uncapped call dies with nothing to read.\n\n' + - 'Instead, run this command with Bash IN THE FOREGROUND, with the Bash call\'s own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to a file under $TMPDIR (e.g. `> $TMPDIR/chatgpt-review-plan.json`):\n\n' + - `node skills/chatgpt-review/scripts/chatgpt-review.mjs plan ${runArgs.planFile} --question-file ${runArgs.contextFile} --timeout 540${sessionFlag}\n\n` + + `Instead, run this command with Bash IN THE FOREGROUND, with the Bash call's own timeout set to 580000 (its practical ceiling is 600000ms), redirecting stdout to EXACTLY this file — do not substitute a generic name. This filename is unique to this plan file and pass on purpose: a long /ship run reuses the same $TMPDIR across many units and passes over many hours, and a generic filename risks a LATER pass silently reading a stale file left over from an EARLIER one instead of its own real result:\n\n` + + `node skills/chatgpt-review/scripts/chatgpt-review.mjs plan ${runArgs.planFile} --question-file ${runArgs.contextFile} --timeout 540${sessionFlag} > ${outputFile}\n\n` + '--timeout 540 caps the script\'s OWN internal wait at 9 minutes — safely inside the Bash tool\'s 10-minute ceiling — so the process exits cleanly with valid JSON instead of being killed. A "status" of "timed_out" is EXPECTED and NORMAL here, not a failure: the script persists its session handle and conversation URL even on a timeout.\n' + 'Read the output file (it is JSON). FIRST check response_text regardless of "status": if it already ends with exactly one well-formed "VERDICT: APPROVED" or "VERDICT: REVISE" line, ChatGPT had already finished generating — treat this as a complete result and stop retrying, even if "status" says "rate_limited"/"timed_out"/etc (a UI-level banner can appear over an already-finished answer; the literal status field is NOT authoritative about whether real content exists). Only if response_text has NO parseable verdict line do you need to retry: if "status" is "rate_limited", ChatGPT is throttling conversation access — hammering it immediately makes this WORSE, so wait first using a small-increment loop in ONE Bash call (a bare `sleep 90` prefix gets blocked as chaining), e.g. `end=$(( $(date +%s) + 90 )); while [ $(date +%s) -lt $end ]; do sleep 5; done; node ...`. For any other non-completed, no-verdict status, retry immediately. Either way, retry the SAME chatgpt-review command, adding/updating `--session ` from the JSON (again foreground, again --timeout 540, again Bash timeout 580000) — this resumes the same conversation instead of resubmitting the prompt. Repeat for up to 4 total attempts. After 4 attempts with still no parseable verdict line, stop and treat it as incomplete.\n' + 'Then map the final JSON to the output schema:\n' +