fix(chatgpt-review): plan-author early-exit once a complete protocol appears

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

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; }
+}

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<<<CHATGPT_PLAN_BEGIN>>>\n# Heading\nbody\n<<<CHATGPT_PLAN_END>>>';
+  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<<<CHATGPT_PLAN_BEGIN>>>\n# Heading\nbody\n<<<CHATGPT_PLAN_END>>>';
+  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<<<CHATGPT_PLAN_BEGIN>>>\n# Heading\nbody\n<<<CHATGPT_PLAN_END>>>';
+  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');