From eeff0a3ef1941eaea378809c56cc45418880f9dc Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 8 Aug 2026 13:04:04 +0200 Subject: [PATCH] fix(chatgpt-review): plan-author early-exit once a complete protocol appears MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waitForCompletion() required the WHOLE turn to stop generating (no visible stop button) and hold stable for 7s before returning, for every mode alike. That's the right contract for pr/plan/issue's trailing VERDICT line, but plan-author's protocol is a fixed-content island: parsePlanAuthorResponse already requires exactly one delimiter pair, so once a valid PLAN_STATUS: READY/BLOCKED block appears, its content can never change no matter what ChatGPT keeps writing afterward (more reasoning, another tool call, a citation footnote — already tolerated by the same parser). Observed live on #630 phase 7: an Extra-High-effort plan-author turn kept the stop button visible for 20+ minutes of further tool calls after already emitting a complete, valid plan. waitForCompletion's !generating+stableMs requirement can never fire in that case, so the whole call times out despite a perfectly good answer already sitting in the DOM — the coordinator had to recover it by hand from the live page. For plan-author mode only: once the same complete-protocol match holds for two consecutive polls (guarding against a transient mid-stream coincidence), confirm it against the authoritative clipboard-copied Markdown (innerText can strip the literal '#' heading syntax the parser's heading check needs) and return immediately if it agrees, without waiting for generation to stop. Falls back to the ordinary wait if the clipboard copy disagrees. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz --- .../chatgpt-review/scripts/chatgpt-review.mjs | 1 + skills/chatgpt-review/scripts/lib/browser.mjs | 48 ++++++++++++++-- skills/chatgpt-review/tests/browser.test.mjs | 55 +++++++++++++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/skills/chatgpt-review/scripts/chatgpt-review.mjs b/skills/chatgpt-review/scripts/chatgpt-review.mjs index 576b48f2..a03edd9a 100755 --- a/skills/chatgpt-review/scripts/chatgpt-review.mjs +++ b/skills/chatgpt-review/scripts/chatgpt-review.mjs @@ -97,6 +97,7 @@ export async function run(argv, dependencies = {}) { target: prepared.target, publish: options.requestedPublication, diagnosticsDir: options.diagnosticsDir ? path.resolve(options.diagnosticsDir) : null, + mode: options.mode, }); const metadata = extractReportedMetadata(review.responseText); session = await store.write({ ...session, conversationUrl: review.conversationUrl, passCount: passNumber, lastResponseFingerprint: review.responseFingerprint ?? null, ...metadata }); diff --git a/skills/chatgpt-review/scripts/lib/browser.mjs b/skills/chatgpt-review/scripts/lib/browser.mjs index 1f97dafe..7323111b 100644 --- a/skills/chatgpt-review/scripts/lib/browser.mjs +++ b/skills/chatgpt-review/scripts/lib/browser.mjs @@ -1,6 +1,7 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import crypto from 'node:crypto'; +import { parsePlanAuthorResponse } from './plan-author.mjs'; // ChatGPT virtualizes/prunes older turns out of the DOM in long conversations, so a raw // element COUNT of assistant messages does not grow monotonically — it can plateau or even @@ -83,7 +84,7 @@ export class ChatGptBrowser { return { page, checks: { cdp: true, login: true, composer: true, fileUpload: upload, predefinedModelAndEffort: true } }; } - async review({ session, prompt, uploadPath, timeoutMs, target, publish, diagnosticsDir }) { + async review({ session, prompt, uploadPath, timeoutMs, target, publish, diagnosticsDir, mode }) { const { page, reopened } = await this.pageFor(session); try { await this.assertReady(page); @@ -93,7 +94,7 @@ export class ChatGptBrowser { const hasUncollected = generationActive || (Boolean(currentTail) && fingerprintText(currentTail) !== recordedFingerprint); if (session && hasUncollected) { this.stderr.write('Recovering an uncollected ChatGPT response...\n'); - const responseText = await this.waitForCompletion(page, { before: null, timeoutMs, target, publish }); + const responseText = await this.waitForCompletion(page, { before: null, timeoutMs, target, publish, mode }); // Fingerprint the plain rendered tail, not responseText (which may now be the // upgraded Markdown from copyLatestAssistantMarkdown) — a future call's staleness // check compares against THIS stored value using latestAssistantText's same plain @@ -106,7 +107,7 @@ export class ChatGptBrowser { await this.fillAndSend(page, prompt); await this.waitForPermanentConversationUrl(page); this.stderr.write('Waiting for ChatGPT response...\n'); - const responseText = await this.waitForCompletion(page, { before, timeoutMs, target, publish }); + const responseText = await this.waitForCompletion(page, { before, timeoutMs, target, publish, mode }); return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: false, responseFingerprint: fingerprintText(await this.latestAssistantText(page)) }; } catch (error) { error.conversationUrl = page.url(); @@ -202,11 +203,23 @@ export class ChatGptBrowser { } catch { return null; } } - async waitForCompletion(page, { before, timeoutMs, target, publish }) { + async waitForCompletion(page, { before, timeoutMs, target, publish, mode }) { const started = this.now(); let lastText = ''; let stableSince = null; let streamRetries = 0; + // plan-author's protocol is a fixed-content island: once exactly one well-formed + // PLAN_STATUS: READY/BLOCKED block appears, its content is final by construction + // (parsePlanAuthorResponse requires exactly one delimiter pair) — anything ChatGPT + // keeps writing afterward (more reasoning, another tool call, a citation footnote) + // is explicitly tolerated by that same parser and never changes the extracted plan. + // Observed live on #630 phase 7: an Extra-High-effort turn can keep the "generating" + // indicator (stop button) visible for 20+ minutes after already emitting a complete, + // valid plan — the ordinary !generating + 7s-stability requirement below can never + // fire in that case, so the whole call times out despite a perfectly good answer + // already sitting in the DOM. For plan-author only, treat a validated match as done + // immediately, without waiting for `generating` to clear. + let planAuthorPendingConfirm = false; while (this.now() - started < timeoutMs) { const streamError = await firstVisible(page, SELECTORS.streamError); const retry = streamError ? await firstVisible(page, SELECTORS.streamRetry) : null; @@ -245,6 +258,25 @@ export class ChatGptBrowser { // so DOM pruning of older turns in a long conversation cannot spuriously suppress it. const text = (before === null || currentText !== before) ? currentText : ''; const generating = await anyVisible(page, SELECTORS.stop); + if (mode === 'plan-author' && text && hasCompletePlanAuthorProtocol(text)) { + if (planAuthorPendingConfirm) { + // Confirmed on two consecutive polls against the cheap innerText check — now + // validate against the authoritative clipboard-copied Markdown (innerText can + // strip literal '#' heading syntax the parser's heading check requires) before + // trusting it enough to return early while still generating. + const markdown = await this.copyLatestAssistantMarkdown(page); + if (markdown && hasCompletePlanAuthorProtocol(markdown)) return markdown; + if (!generating) return markdown || text; // clipboard read failed, but the turn is genuinely done anyway + // Clipboard copy disagreed while still generating (e.g. mid-stream race on a + // still-forming second attempt) — fall through to the ordinary stability wait + // rather than trusting an unconfirmed early exit. + planAuthorPendingConfirm = false; + } else { + planAuthorPendingConfirm = true; + } + } else { + planAuthorPendingConfirm = false; + } if (text && text === lastText) stableSince ??= this.now(); else { lastText = text; stableSince = text ? this.now() : null; } if (text && !generating && stableSince !== null && this.now() - stableSince >= this.stableMs) { @@ -328,3 +360,11 @@ export function classifyAlertText(text) { } function canonicalConversation(url) { return url?.replace(/[?#].*$/, '').replace(/\/$/, ''); } function summarize(text) { return text.replace(/\s+/g, ' ').trim().slice(0, 240); } +// Non-throwing peek at whether `text` already satisfies plan-author's complete +// protocol (exactly one READY delimiter pair with a real Markdown heading, or a +// well-formed BLOCKED response) — reuses the real parser so this can never drift +// from what actually gets accepted downstream. +function hasCompletePlanAuthorProtocol(text) { + try { parsePlanAuthorResponse(text); return true; } + catch { return false; } +} diff --git a/skills/chatgpt-review/tests/browser.test.mjs b/skills/chatgpt-review/tests/browser.test.mjs index 3e8aee87..cdd72ded 100644 --- a/skills/chatgpt-review/tests/browser.test.mjs +++ b/skills/chatgpt-review/tests/browser.test.mjs @@ -220,6 +220,61 @@ test('a completed response is fingerprinted by its plain rendered tail even when assert.equal(stage, 2); }); +test('plan-author mode returns as soon as a complete protocol appears, without waiting for generation to stop', async () => { + // Reproduced live on #630 phase 7: an Extra-High-effort plan-author turn kept the + // "generating" indicator (stop button) visible for 20+ minutes of further reasoning + // and tool calls AFTER already emitting a complete, valid PLAN_STATUS: READY block — + // the ordinary !generating + stableMs requirement can never fire in that case, so the + // whole call times out despite a perfectly good answer already sitting in the DOM. + const planText = 'PLAN_STATUS: READY\n<<>>\n# Heading\nbody\n<<>>'; + const copyButton = new Element(); + const responseGroup = new Element({ nested: { [SELECTORS.responseCopyButton[0]]: [copyButton] } }); + const page = readyPage({ + [SELECTORS.assistant[0]]: [new Element({ text: planText })], + [SELECTORS.responseActions[0]]: [responseGroup], + [SELECTORS.stop[0]]: [new Element()], // never clears — ChatGPT is still "generating" + }, { evaluate: () => planText }); + const driver = driverWith(page); + const text = await driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false, mode: 'plan-author' }); + assert.equal(text, planText); +}); + +test('non-plan-author modes still require generation to stop before returning', async () => { + // The early-exit is scoped to plan-author's fixed-content delimiter protocol only — + // other modes end with a VERDICT: line that could still change if more text follows, + // so they must keep waiting for the ordinary !generating + stability requirement. + const planText = 'PLAN_STATUS: READY\n<<>>\n# Heading\nbody\n<<>>'; + const page = readyPage({ + [SELECTORS.assistant[0]]: [new Element({ text: planText })], + [SELECTORS.stop[0]]: [new Element()], // never clears + }); + const driver = driverWith(page); + await assert.rejects( + () => driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }), + (error) => error.status === 'timed_out', + ); +}); + +test('plan-author early-exit requires the clipboard-copied Markdown to confirm the match before trusting it while still generating', async () => { + // innerText can pass the cheap protocol check (e.g. on plain text with no real Markdown + // heading, or a transient mid-stream coincidence) while the authoritative copied Markdown + // disagrees — must never return early on an unconfirmed match, however many times the + // cheap check alone keeps passing. + const copyButton = new Element(); + const responseGroup = new Element({ nested: { [SELECTORS.responseCopyButton[0]]: [copyButton] } }); + const planText = 'PLAN_STATUS: READY\n<<>>\n# Heading\nbody\n<<>>'; + const page = readyPage({ + [SELECTORS.assistant[0]]: [new Element({ text: planText })], + [SELECTORS.responseActions[0]]: [responseGroup], + [SELECTORS.stop[0]]: [new Element()], // never clears + }, { evaluate: () => 'not a valid protocol response' }); + const driver = driverWith(page); + await assert.rejects( + () => driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false, mode: 'plan-author' }), + (error) => error.status === 'timed_out', + ); +}); + test('missing copy control, denied permission, or a hung clipboard read fall back to the rendered text without hanging', async () => { const noGroupPage = readyPage({ [SELECTORS.assistant[0]]: [new Element({ text: 'plain answer' })] }); assert.equal(await driverWith(noGroupPage).waitForCompletion(noGroupPage, { before: '', timeoutMs: 20 }), 'plain answer');