diff --git a/skills/chatgpt-review/scripts/chatgpt-review.mjs b/skills/chatgpt-review/scripts/chatgpt-review.mjs index a03edd9a..e7c13c1f 100755 --- a/skills/chatgpt-review/scripts/chatgpt-review.mjs +++ b/skills/chatgpt-review/scripts/chatgpt-review.mjs @@ -98,6 +98,7 @@ export async function run(argv, dependencies = {}) { publish: options.requestedPublication, diagnosticsDir: options.diagnosticsDir ? path.resolve(options.diagnosticsDir) : null, mode: options.mode, + onHeartbeat: (state) => store.writeHeartbeat(session.handle, state), }); 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 7323111b..92e65128 100644 --- a/skills/chatgpt-review/scripts/lib/browser.mjs +++ b/skills/chatgpt-review/scripts/lib/browser.mjs @@ -52,14 +52,35 @@ export async function connectToChrome(cdpUrl, importer = () => import('playwrigh } } +// Sent at most once per waitForCompletion() call when generation has shown zero text +// growth for noProgressStallMs while still "generating" — a live tool call stuck mid-turn +// (observed repeatedly across issue #630, previously only recoverable by a human manually +// clicking Stop and nudging the same conversation). Kept short and explicit so it reads +// unambiguously as an automated recovery nudge, not a new question. +export const RECOVERY_NUDGE = 'Please continue without further tool calls.'; + export class ChatGptBrowser { - constructor({ browser, stderr = process.stderr, now = Date.now, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), stableMs = 7000, pollMs = 1000 }) { + constructor({ + browser, stderr = process.stderr, now = Date.now, + sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), + stableMs = 7000, pollMs = 1000, + // How long generation may show zero text growth while still "generating" before one + // automatic stop+nudge recovery attempt fires. 4 minutes is long enough that no normal + // slow-but-progressing tool call trips it, short enough to matter inside a 9-30 minute + // overall --timeout. + noProgressStallMs = 240_000, + // Throttle for onHeartbeat callbacks during waitForCompletion's poll loop — writing on + // every 1s poll would be excessive I/O for a call that can run 30 minutes. + heartbeatIntervalMs = 10_000, + }) { this.browser = browser; this.stderr = stderr; this.now = now; this.sleep = sleep; this.stableMs = stableMs; this.pollMs = pollMs; + this.noProgressStallMs = noProgressStallMs; + this.heartbeatIntervalMs = heartbeatIntervalMs; } async pageFor(session) { @@ -84,17 +105,29 @@ 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, mode }) { + async review({ session, prompt, uploadPath, timeoutMs, target, publish, diagnosticsDir, mode, onHeartbeat }) { + // ONE deadline for the whole call, not a separate independent worst-case allowance per + // phase. Before this, assertReady (~15s) + upload (~30s) + fillAndSend (up to 240s via + // its own retry) + waitForPermanentConversationUrl (~15s) were ALL uncounted + // against the caller's own --timeout, so the real worst-case wall time was `timeoutMs + + // ~300s` — not `timeoutMs` — and a caller budgeting its own outer process-kill ceiling + // from `timeoutMs` alone (as this skill's own ship-integration does) could get SIGKILLed + // mid-poll with zero output ever flushed. Confirmed live: 6 real `chatgpt-review` + // invocations across issue #630 left nothing but their initial progress line in the + // caller's captured output file. Each setup phase below still gets its own short, + // reasonable per-phase ceiling (so a stuck composer fails fast rather than silently + // eating the whole budget) — Math.min caps it at whichever is sooner. + const deadline = this.now() + timeoutMs; const { page, reopened } = await this.pageFor(session); try { - await this.assertReady(page); + await this.assertReady(page, Math.min(this.now() + 15_000, deadline)); const generationActive = await anyVisible(page, SELECTORS.stop); const currentTail = await this.latestAssistantText(page); const recordedFingerprint = session?.lastResponseFingerprint ?? null; 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, mode }); + const responseText = await this.waitForCompletion(page, { before: null, deadline, target, publish, mode, onHeartbeat }); // 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 @@ -102,12 +135,12 @@ export class ChatGptBrowser { // message would make every later resume spuriously look "uncollected" forever. return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: true, responseFingerprint: fingerprintText(await this.latestAssistantText(page)) }; } - if (uploadPath) await this.upload(page, uploadPath); + if (uploadPath) await this.upload(page, uploadPath, Math.min(this.now() + 30_000, deadline)); const before = currentTail; - await this.fillAndSend(page, prompt); - await this.waitForPermanentConversationUrl(page); + await this.fillAndSend(page, prompt, Math.min(this.now() + 242_000, deadline)); + await this.waitForPermanentConversationUrl(page, Math.min(this.now() + 15_000, deadline)); this.stderr.write('Waiting for ChatGPT response...\n'); - const responseText = await this.waitForCompletion(page, { before, timeoutMs, target, publish, mode }); + const responseText = await this.waitForCompletion(page, { before, deadline, target, publish, mode, onHeartbeat }); return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: false, responseFingerprint: fingerprintText(await this.latestAssistantText(page)) }; } catch (error) { error.conversationUrl = page.url(); @@ -116,9 +149,8 @@ export class ChatGptBrowser { } } - async assertReady(page) { + async assertReady(page, deadline = this.now() + 15_000) { await page.waitForLoadState?.('domcontentloaded').catch(() => {}); - const deadline = this.now() + 15_000; while (this.now() < deadline) { if (await anyVisible(page, SELECTORS.composer)) return; if (await anyVisible(page, SELECTORS.login)) throw new ReviewError('login_required', 'ChatGPT is not logged in in the connected Chrome profile'); @@ -127,39 +159,62 @@ export class ChatGptBrowser { throw new ReviewError('ui_incompatible', 'Could not find the ChatGPT composer; the UI may have changed'); } - async upload(page, uploadPath) { + async upload(page, uploadPath, deadline = this.now() + 30_000) { const input = await firstExisting(page, SELECTORS.fileInput); if (!input) throw new ReviewError('ui_incompatible', 'ChatGPT file upload input was not found'); await input.setInputFiles(uploadPath); if (page.getByText) { const attachment = page.getByText(path.basename(uploadPath), { exact: false }).last(); - try { await attachment.waitFor({ state: 'visible', timeout: 30_000 }); } + try { await attachment.waitFor({ state: 'visible', timeout: Math.max(1000, deadline - this.now()) }); } catch { throw new ReviewError('ui_incompatible', 'ChatGPT did not confirm the requested file upload'); } } } - async fillAndSend(page, prompt) { + // Inserts `value` into `element` (a composer or similar contenteditable) via the real DOM, + // in-page, bypassing Playwright's own actionability-checked .fill(). Measured live against + // the production ChatGPT composer: an 80KB insertText call lands in ~50ms regardless of + // size — .fill()'s slowness (below) is not about text volume. Returns false (never throws) + // on anything that goes wrong, so callers can fall back rather than fail outright: no real + // DOM (e.g. this skill's own mocked test harness), the element vanishing, or any other + // in-page error. + async insertNatively(element, value) { + try { + await element.click({ timeout: 5000 }); + await element.evaluate((el, text) => { + el.focus(); + document.execCommand('selectAll', false, null); + document.execCommand('delete', false, null); + document.execCommand('insertText', false, text); + el.dispatchEvent(new InputEvent('input', { bubbles: true })); + }, value); + return true; + } catch { return false; } + } + + async fillAndSend(page, prompt, deadline = this.now() + 242_000) { const composer = await firstVisible(page, SELECTORS.composer); if (!composer) throw new ReviewError('ui_incompatible', 'ChatGPT composer disappeared before submission'); - // A large prompt (a full delivery contract plus accumulated review context can run - // tens of KB) reproducibly takes longer than Playwright's default 30s actionability - // wait for .fill() to insert into ChatGPT's rich-text composer — not a transient - // "busy" blip: three quick retries at the default timeout hit the identical timeout - // three times in a row (observed live, twice, on issue #585 phase 0's revision - // passes). Give the single attempt real headroom instead of retrying too fast to help. - let lastError; - for (let attempt = 0; attempt < 2; attempt += 1) { - try { await composer.fill(prompt, { timeout: 120_000 }); lastError = null; break; } - catch (error) { lastError = error; await this.sleep(2000); } + if (!(await this.insertNatively(composer, prompt))) { + // Fallback only: Playwright's own .fill() has been observed to still genuinely time + // out under real load even at 120s x 2 retries (issue #630, 246.9s elapsed, both + // attempts exhausted) — not a text-size effect (native insertion above is ~50ms + // regardless of size), most likely CPU contention from concurrent agents during a + // live /ship run. Bounded by whatever remains of the overall call's deadline, never an + // unconditional extra 240s tacked on top of it. + let lastError; + for (let attempt = 0; attempt < 2; attempt += 1) { + const remaining = Math.max(1000, deadline - this.now()); + try { await composer.fill(prompt, { timeout: Math.min(remaining, 120_000) }); lastError = null; break; } + catch (error) { lastError = error; await this.sleep(Math.min(2000, Math.max(0, deadline - this.now()))); } + } + if (lastError) throw lastError; } - if (lastError) throw lastError; const send = await firstVisible(page, SELECTORS.send); if (send) await send.click(); else await composer.press('Enter'); } - async waitForPermanentConversationUrl(page) { - const deadline = this.now() + 15_000; + async waitForPermanentConversationUrl(page, deadline = this.now() + 15_000) { while (this.now() < deadline) { if (/^https:\/\/chatgpt\.com\/c\/(?!WEB:)[^/?#]+/i.test(page.url())) return true; await this.sleep(100); @@ -203,11 +258,36 @@ export class ChatGptBrowser { } catch { return null; } } - async waitForCompletion(page, { before, timeoutMs, target, publish, mode }) { + // A single clipboard read can fail transiently (a permission race, one slow tick) — for + // every OTHER mode that's a soft quality loss (extraction regexes may miss a Markdown- + // formatted SHA/URL), but for plan-author it is FATAL: innerText can never contain the + // literal '#' parsePlanAuthorResponse's heading check requires, so a bare fallback is + // structurally guaranteed to fail that validation downstream in chatgpt-review.mjs's own + // run() — confirmed live: "The delimited plan is not complete Markdown with a heading" + // recurred 6 times across real issue #630/#585 invocations. Retrying a couple more times + // (each internally bounded to ~4s by copyLatestAssistantMarkdown's own race) is cheap + // insurance against surrendering to a fallback known to be doomed for this one mode. + async copyPreferredResponseText(page, mode) { + let markdown = await this.copyLatestAssistantMarkdown(page); + if (!markdown && mode === 'plan-author') { + for (let attempt = 0; attempt < 2 && !markdown; attempt += 1) { + await this.sleep(1000); + markdown = await this.copyLatestAssistantMarkdown(page); + } + } + return markdown; + } + + async waitForCompletion(page, { before, timeoutMs, deadline, target, publish, mode, onHeartbeat }) { + // deadline (absolute, from review()'s unified budget) takes precedence; timeoutMs + // (relative, from callers/tests that predate the unified-budget fix) still works. const started = this.now(); + const effectiveDeadline = deadline ?? (started + timeoutMs); let lastText = ''; let stableSince = null; let streamRetries = 0; + let recoveryAttempted = false; + let lastHeartbeatAt = 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 @@ -220,7 +300,7 @@ export class ChatGptBrowser { // 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) { + while (this.now() < effectiveDeadline) { const streamError = await firstVisible(page, SELECTORS.streamError); const retry = streamError ? await firstVisible(page, SELECTORS.streamRetry) : null; if (retry) { @@ -264,7 +344,7 @@ export class ChatGptBrowser { // 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); + const markdown = await this.copyPreferredResponseText(page, mode); 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 @@ -279,10 +359,41 @@ export class ChatGptBrowser { } if (text && text === lastText) stableSince ??= this.now(); else { lastText = text; stableSince = text ? this.now() : null; } + // One automatic stop+nudge recovery attempt if generation has shown ZERO text growth + // for noProgressStallMs while still "generating" — a live tool call stuck mid-turn. + // waitForCompletion's own DOM polling has no way to distinguish this from a slow-but- + // progressing tool call except elapsed time; previously this required a human to + // notice and manually intervene (documented in skills/ship/references/review-loops.md + // as a recurring, purely manual procedure across issue #630). Capped at one attempt + // per call so a conversation that is stuck for some other reason still times out + // normally instead of nudging forever. + if (!recoveryAttempted && generating && text && stableSince !== null && this.now() - stableSince >= this.noProgressStallMs) { + recoveryAttempted = true; + this.stderr.write(`No response growth for ${Math.round(this.noProgressStallMs / 1000)}s while ChatGPT is still "generating" — attempting one automatic stop+nudge recovery...\n`); + const stop = await firstVisible(page, SELECTORS.stop); + if (stop) await stop.click().catch(() => {}); + await this.sleep(1000); + const composer = await firstVisible(page, SELECTORS.composer); + if (composer && (await this.insertNatively(composer, RECOVERY_NUDGE))) { + const send = await firstVisible(page, SELECTORS.send); + if (send) await send.click(); else await composer.press('Enter'); + } + lastText = ''; + stableSince = null; + planAuthorPendingConfirm = false; + await this.sleep(this.pollMs); + continue; + } if (text && !generating && stableSince !== null && this.now() - stableSince >= this.stableMs) { - const markdown = await this.copyLatestAssistantMarkdown(page); + const markdown = await this.copyPreferredResponseText(page, mode); return markdown || text; } + if (onHeartbeat && this.now() - lastHeartbeatAt >= this.heartbeatIntervalMs) { + lastHeartbeatAt = this.now(); + // Promise.resolve(...) tolerates a synchronous (non-Promise-returning) callback — + // calling .catch() directly on its return value would throw on undefined. + await Promise.resolve(onHeartbeat({ elapsedMs: this.now() - started, generating, textLength: text.length || lastText.length, recoveryAttempted })).catch(() => {}); + } await this.sleep(this.pollMs); } throw new ReviewError('timed_out', 'Timed out before ChatGPT produced a stable completed response', lastText); diff --git a/skills/chatgpt-review/scripts/lib/state.mjs b/skills/chatgpt-review/scripts/lib/state.mjs index 080c5d4e..40516e9e 100644 --- a/skills/chatgpt-review/scripts/lib/state.mjs +++ b/skills/chatgpt-review/scripts/lib/state.mjs @@ -54,6 +54,23 @@ export class SessionStore { } } sessionPath(handle) { return path.join(this.root, 'sessions', `${handle}.json`); } + // Best-effort progress snapshot, written periodically during a single run's poll loop + // (browser.mjs's onHeartbeat), so a process killed externally with zero stdout ever + // flushed (confirmed live 6 times across issue #630 — see browser.mjs's review() comment) + // still leaves something recoverable: was it alive, still generating, how much text had + // it seen, how long had it been running. A separate file/directory from the session + // record itself since this is transient run state, not the session's own durable identity. + async writeHeartbeat(handle, data) { + if (!/^[0-9a-f-]{36}$/i.test(handle)) throw new CliError('Invalid session handle'); + await this.initialize(); + const directory = path.join(this.root, 'heartbeat'); + await fs.mkdir(directory, { recursive: true, mode: 0o700 }); + const destination = path.join(directory, `${handle}.json`); + const temporary = `${destination}.${crypto.randomUUID()}.tmp`; + await fs.writeFile(temporary, `${JSON.stringify({ handle, ...data, updatedAt: new Date().toISOString() }, null, 2)}\n`, { mode: 0o600, flag: 'wx' }); + await fs.rename(temporary, destination); + await fs.chmod(destination, 0o600); + } async writeIndex(identity, handle) { const filename = path.join(this.root, 'targets.json'); let current = {}; diff --git a/skills/chatgpt-review/tests/browser.test.mjs b/skills/chatgpt-review/tests/browser.test.mjs index cdd72ded..a7ae3566 100644 --- a/skills/chatgpt-review/tests/browser.test.mjs +++ b/skills/chatgpt-review/tests/browser.test.mjs @@ -1,9 +1,9 @@ import assert from 'node:assert/strict'; import test from 'node:test'; -import { ChatGptBrowser, ReviewError, SELECTORS, classifyAlertText, classifyPermission, connectToChrome, fingerprintText } from '../scripts/lib/browser.mjs'; +import { ChatGptBrowser, ReviewError, RECOVERY_NUDGE, SELECTORS, classifyAlertText, classifyPermission, connectToChrome, fingerprintText } from '../scripts/lib/browser.mjs'; class Element { - constructor({ text = '', visible = true, onClick, nested = {} } = {}) { this.text = text; this.visible = visible; this.onClick = onClick; this.nested = nested; } + constructor({ text = '', visible = true, onClick, nested = {}, evaluate } = {}) { this.text = text; this.visible = visible; this.onClick = onClick; this.nested = nested; this._evaluate = evaluate; } async isVisible() { return this.visible; } async count() { return 1; } async innerText() { return this.text; } @@ -11,6 +11,14 @@ class Element { async fill(value) { this.value = value; } async press(value) { this.pressed = value; } async setInputFiles(value) { this.files = value; } + // Real Playwright's locator.evaluate(fn, arg) runs fn IN THE BROWSER PAGE, with a real + // DOM element bound as fn's first argument. This mock has no DOM at all by default — + // matching today's actual test environment (Node, no browser) — so it throws unless a + // test explicitly supplies an `evaluate` override to simulate a real page. + async evaluate(fn, arg) { + if (this._evaluate) return this._evaluate(fn, arg); + throw new TypeError('no DOM available in the test harness'); + } locator(selector) { return new Locator(this.nested[selector] ?? []); } } class Locator { @@ -72,7 +80,10 @@ test('streaming response must be new, non-empty, stopped, and stable', async () let sent = false; const assistant = new Element({ text: 'complete answer' }); const composer = new Element(); - const send = new Element({ onClick: () => { sent = true; } }); + // ChatGPT swaps its temporary root URL for a permanent /c/ one within ~1-2s of a + // real submission — simulating that here keeps waitForPermanentConversationUrl from + // burning its own poll budget against this test's unrelated (tiny, fake-clock) timeout. + const send = new Element({ onClick: () => { sent = true; page.currentUrl = 'https://chatgpt.com/c/test'; } }); const page = new Page('https://chatgpt.com/', { [SELECTORS.composer[0]]: [composer], [SELECTORS.send[0]]: [send], [SELECTORS.assistant[0]]: () => sent ? [assistant] : [], @@ -114,7 +125,10 @@ test('session retry recovers an uncollected response without sending a duplicate test('fresh submission detects a new response even when DOM pruning keeps the assistant-message count flat', async () => { let sent = false; const composer = new Element(); - const send = new Element({ onClick: () => { sent = true; } }); + // ChatGPT swaps its temporary root URL for a permanent /c/ one within ~1-2s of a + // real submission — simulating that here keeps waitForPermanentConversationUrl from + // burning its own poll budget against this test's unrelated (tiny, fake-clock) timeout. + const send = new Element({ onClick: () => { sent = true; page.currentUrl = 'https://chatgpt.com/c/test'; } }); // Simulates ChatGPT virtualizing old turns out of the DOM: the assistant locator always // returns exactly one element (a fixed-size window), but its content is the STALE prior // answer until submit, then the NEW one — never two elements at once, so a count-based @@ -198,7 +212,10 @@ test('a completed response is fingerprinted by its plain rendered tail even when const responseGroup = new Element({ nested: { [SELECTORS.responseCopyButton[0]]: [copyButton] } }); let stage = 0; // 0: nothing sent yet, 1: first reply present, 2: second reply present const composer = new Element(); - const send = new Element({ onClick: () => { stage += 1; } }); + // ChatGPT swaps its temporary root URL for a permanent /c/ one within ~1-2s of a + // real submission — simulating that here keeps waitForPermanentConversationUrl from + // burning its own poll budget against this test's unrelated (tiny, fake-clock) timeout. + const send = new Element({ onClick: () => { stage += 1; page.currentUrl = 'https://chatgpt.com/c/test'; } }); const firstText = 'PLAN_STATUS: READY rendered without markdown syntax'; const secondText = 'a later, different reply'; const page = new Page('https://chatgpt.com/', { @@ -325,6 +342,160 @@ test('composer fill gives up and throws after exhausting its retries', async () await assert.rejects(() => driverWith(page).fillAndSend(page, 'prompt text'), /Timeout 30000ms exceeded/); }); +test('native insertion is preferred when the composer supports it; Playwright .fill() is never called', async () => { + // Measured live against the production ChatGPT composer: an 80KB execCommand insertText + // call lands in ~50ms regardless of size, sidestepping Playwright's own actionability- + // checked .fill(), which has been observed to still genuinely time out under real load + // even at 120s x 2 retries (issue #630, 246.9s elapsed). This proves the fast path is + // actually tried first, not merely available. + let fillCalled = false; + let insertedValue = null; + const composer = new Element({ evaluate: (fn, value) => { insertedValue = value; } }); + composer.fill = async (value) => { fillCalled = true; composer.value = value; }; + const send = new Element(); + const page = readyPage({ [SELECTORS.composer[0]]: [composer], [SELECTORS.send[0]]: [send] }); + await driverWith(page).fillAndSend(page, 'a large prompt body'); + assert.equal(insertedValue, 'a large prompt body'); + assert.equal(fillCalled, false); + assert.equal(composer.value, undefined); +}); + +test('the whole review() call is bounded by its own timeoutMs even when a setup phase needs its own retry', async () => { + // Before the unified-deadline fix, assertReady/upload/fillAndSend/waitForPermanentConversationUrl + // each had their own independent, ADDITIONAL worst-case allowance on top of timeoutMs, so + // the real total wall time could run ~300s past what a caller's own --timeout 540 assumed — + // confirmed live: 6 real invocations across issue #630 were killed with zero output ever + // flushed. A composer that never accepts native insertion AND whose Playwright .fill() + // always fails, combined with a conversation URL that never becomes permanent, forces every + // setup phase to burn its own ceiling — the whole call must still fail by (approximately) + // the caller's timeoutMs, not additively stack every phase's ceiling on top of it. + const composer = new Element(); + composer.fill = async () => { throw new Error('locator.fill: Timeout 30000ms exceeded'); }; + const page = readyPage({ [SELECTORS.composer[0]]: [composer] }); // no [SELECTORS.send] -> composer.press('Enter') fallback; URL never becomes /c/ + const clock = { value: 0 }; + const driver = new ChatGptBrowser({ + browser: { contexts: () => [new Context([], page)] }, now: () => clock.value, + sleep: async (ms) => { clock.value += ms; }, stableMs: 2, pollMs: 1, stderr: { write() {} }, + }); + // The composer.fill() timeout surfaces as a plain Error here (fillAndSend's fallback + // re-throws it unwrapped, exactly as chatgpt-review.mjs's own run() does in production, + // where it becomes an "internal_error" status) — what matters for THIS test is only that + // the call fails promptly, not which error type carries it. + await assert.rejects(() => driver.review({ prompt: 'review', timeoutMs: 500, target: null, publish: false })); + // Old behavior would have let assertReady/fillAndSend/waitForPermanentConversationUrl each + // additively consume their own full ceiling BEFORE waitForCompletion's clock even started, + // pushing total elapsed well past 500 (into the multiple-thousands). The fix bounds the + // whole call close to the caller's own timeoutMs. + assert.ok(clock.value < 2000, `expected total elapsed to stay close to timeoutMs (500), got ${clock.value}`); +}); + +test('plan-author mode retries a failed clipboard copy before falling back to plain text', async () => { + const copyButton = new Element(); + const responseGroup = new Element({ nested: { [SELECTORS.responseCopyButton[0]]: [copyButton] } }); + let readAttempts = 0; + const realMarkdown = 'PLAN_STATUS: READY\n<<>>\n# Heading\nbody\n<<>>'; + const page = readyPage({ + [SELECTORS.assistant[0]]: [new Element({ text: 'plain rendered tail, no literal heading syntax' })], + [SELECTORS.responseActions[0]]: [responseGroup], + }, { evaluate: () => { readAttempts += 1; return readAttempts === 1 ? null : realMarkdown; } }); + const driver = driverWith(page); + const text = await driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false, mode: 'plan-author' }); + assert.equal(text, realMarkdown); + assert.ok(readAttempts >= 2, `expected at least one retry, got ${readAttempts} attempt(s)`); +}); + +test('non-plan-author modes do not retry a failed clipboard copy — a single miss falls back to plain text immediately', async () => { + const copyButton = new Element(); + const responseGroup = new Element({ nested: { [SELECTORS.responseCopyButton[0]]: [copyButton] } }); + let readAttempts = 0; + const page = readyPage({ + [SELECTORS.assistant[0]]: [new Element({ text: 'plain rendered tail' })], + [SELECTORS.responseActions[0]]: [responseGroup], + }, { evaluate: () => { readAttempts += 1; return readAttempts === 1 ? null : '# would only appear on a retry'; } }); + const driver = driverWith(page); + const text = await driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }); + assert.equal(text, 'plain rendered tail'); + assert.equal(readAttempts, 1); +}); + +test('heartbeat is emitted periodically during polling, throttled, with useful progress fields', async () => { + const heartbeats = []; + const page = readyPage({ [SELECTORS.assistant[0]]: [new Element({ text: 'final answer' })] }); + const clock = { value: 0 }; + const driver = new ChatGptBrowser({ + browser: { contexts: () => [new Context([], page)] }, now: () => clock.value, + sleep: async (ms) => { clock.value += ms; }, stableMs: 2, pollMs: 1, heartbeatIntervalMs: 1, + stderr: { write() {} }, + }); + const text = await driver.waitForCompletion(page, { + before: '', timeoutMs: 50, publish: false, + onHeartbeat: (state) => { heartbeats.push(state); }, + }); + assert.equal(text, 'final answer'); + assert.ok(heartbeats.length >= 1, 'expected at least one heartbeat during polling'); + assert.ok(heartbeats.every((h) => typeof h.elapsedMs === 'number' && typeof h.generating === 'boolean' && typeof h.textLength === 'number')); +}); + +test('a heartbeat write failure never aborts the review', async () => { + const page = readyPage({ [SELECTORS.assistant[0]]: [new Element({ text: 'final answer' })] }); + const clock = { value: 0 }; + const driver = new ChatGptBrowser({ + browser: { contexts: () => [new Context([], page)] }, now: () => clock.value, + sleep: async (ms) => { clock.value += ms; }, stableMs: 2, pollMs: 1, heartbeatIntervalMs: 1, + stderr: { write() {} }, + }); + const text = await driver.waitForCompletion(page, { + before: '', timeoutMs: 50, publish: false, + onHeartbeat: async () => { throw new Error('disk full'); }, + }); + assert.equal(text, 'final answer'); +}); + +test('a stalled generation with zero growth triggers one automatic stop+nudge recovery, then completes normally', async () => { + let stopped = false; + let nudged = false; + let recoveredText = null; + const stop = new Element({ onClick: () => { stopped = true; } }); + const composer = new Element({ evaluate: (fn, value) => { nudged = value === RECOVERY_NUDGE; } }); + const send = new Element({ onClick: () => { recoveredText = 'now producing real content'; } }); + const page = readyPage({ + [SELECTORS.composer[0]]: [composer], + [SELECTORS.send[0]]: [send], + [SELECTORS.stop[0]]: () => (stopped ? [] : [stop]), + [SELECTORS.assistant[0]]: () => [new Element({ text: recoveredText ?? 'stalled text, never growing' })], + }); + const clock = { value: 0 }; + const driver = new ChatGptBrowser({ + browser: { contexts: () => [new Context([], page)] }, now: () => clock.value, + sleep: async (ms) => { clock.value += ms; }, stableMs: 2, pollMs: 1, noProgressStallMs: 5, + stderr: { write() {} }, + }); + const text = await driver.waitForCompletion(page, { before: '', timeoutMs: 5000, publish: false }); + assert.equal(stopped, true, 'expected the stop control to be clicked'); + assert.equal(nudged, true, 'expected the recovery nudge to be inserted into the composer'); + assert.equal(text, 'now producing real content'); +}); + +test('a conversation that stalls for a reason recovery cannot fix still times out normally, without nudging forever', async () => { + let stopClicks = 0; + const stop = new Element({ onClick: () => { stopClicks += 1; } }); + const page = readyPage({ + [SELECTORS.stop[0]]: [stop], // never clears, even after the one recovery attempt + [SELECTORS.assistant[0]]: [new Element({ text: 'stalled text, never growing' })], + }); + const clock = { value: 0 }; + const driver = new ChatGptBrowser({ + browser: { contexts: () => [new Context([], page)] }, now: () => clock.value, + sleep: async (ms) => { clock.value += ms; }, stableMs: 2, pollMs: 1, noProgressStallMs: 5, + stderr: { write() {} }, + }); + await assert.rejects( + () => driver.waitForCompletion(page, { before: '', timeoutMs: 50, publish: false }), + (error) => error.status === 'timed_out', + ); + assert.equal(stopClicks, 1, 'expected exactly one recovery attempt, not repeated nudging'); +}); + test('message stream failures use Retry without completing or creating a new prompt', async () => { let failed = true; let retries = 0; diff --git a/skills/chatgpt-review/tests/core.test.mjs b/skills/chatgpt-review/tests/core.test.mjs index ed52ad9b..d42f7f8d 100644 --- a/skills/chatgpt-review/tests/core.test.mjs +++ b/skills/chatgpt-review/tests/core.test.mjs @@ -164,6 +164,30 @@ test('state records are permission restricted, atomic, indexed, and sanitized', assert.equal(defaultStateDir({ XDG_STATE_HOME: '/state' }, 'linux', '/u'), '/state/chatgpt-review'); }); +test('heartbeat snapshots are permission-restricted, atomic, and separate from the durable session record', async (t) => { + // A process killed externally with zero stdout ever flushed (confirmed live 6 times + // across issue #630 — see browser.mjs's review() comment) still leaves this behind: was + // it alive, still generating, how much text had it seen. Written to its own directory, + // not the session record itself, since this is transient run state, not durable identity. + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'chatgpt-review-heartbeat-')); + t.after(() => fs.rm(dir, { recursive: true, force: true })); + const store = new SessionStore(dir); + const record = await store.create({ mode: 'pr', targetIdentity: 'pr:o/r#1' }); + await store.writeHeartbeat(record.handle, { elapsedMs: 12_345, generating: true, textLength: 40, recoveryAttempted: false }); + const heartbeatPath = path.join(dir, 'heartbeat', `${record.handle}.json`); + const saved = JSON.parse(await fs.readFile(heartbeatPath, 'utf8')); + assert.equal(saved.handle, record.handle); + assert.equal(saved.elapsedMs, 12_345); + assert.equal(saved.generating, true); + assert.ok(saved.updatedAt); + assert.equal((await fs.stat(heartbeatPath)).mode & 0o777, 0o600); + assert.deepEqual((await fs.readdir(path.join(dir, 'heartbeat'))).filter((name) => name.endsWith('.tmp')), []); + // A second write overwrites in place — only the latest snapshot matters, not a growing log. + await store.writeHeartbeat(record.handle, { elapsedMs: 99_999, generating: false, textLength: 400, recoveryAttempted: true }); + assert.equal(JSON.parse(await fs.readFile(heartbeatPath, 'utf8')).elapsedMs, 99_999); + await assert.rejects(() => store.writeHeartbeat('../bad', {}), CliError); +}); + test('output schema is stable and statuses map to distinct exit codes', () => { const doc = resultDocument({ status: 'completed', response_text: 'ok' }); assert.deepEqual(Object.keys(doc), ['status', 'response_text', 'session', 'conversation_url', 'elapsed_seconds', 'pass_number', 'requested_publication', 'reported_reviewed_sha', 'reported_github_comment_url', 'plan_status', 'plan_file', 'blocker', 'error']);