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');