From 69a369ec0ff842ecd6a6836dd9b3b033890a2ecf Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Wed, 5 Aug 2026 11:00:51 +0200 Subject: [PATCH] fix(chatgpt-review): detect completion by content, not DOM element count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ChatGPT virtualizes/prunes older turns out of the DOM in long conversations (confirmed live: only the last two rendered turns stay mounted, e.g. conversation-turn-6 and conversation-turn-8 — earlier ones are gone). browser.mjs's completion and recovery-vs-fresh-submit decisions were both built on assistantCount(page) > before, a raw element count that assumed the DOM only ever grows. Once pruning kicks in for a sufficiently long conversation, that count plateaus (or even drops), which silently broke two things at once: - waitForCompletion's `count > before` check never became true, so it waited out the full --timeout and threw 'timed_out' with an EMPTY response_text — discarding a real, complete answer that was sitting there the whole time. - review()'s recovery-vs-fresh-submission decision (`existingCount > recordedPasses`) could go the wrong way and submit a brand new duplicate message into the conversation instead of recovering the one already there. Confirmed live: at least one redundant resubmission went out and generated unwatched, and the plan file got uploaded 9 times under the same name, which is why ChatGPT's own upload UI started collision-renaming it (plan-590(9).md). Fix: track a SHA-256 fingerprint of the current LAST assistant message's text instead of a count. `review()` now decides "is there an uncollected response" by comparing the live tail's fingerprint against `session. lastResponseFingerprint` (persisted on every successful pass) — content identity, not position. `waitForCompletion`'s `before` is now either the pre-submission baseline text (fresh submission: accept only a tail that differs from it) or `null` (recovery: accept whatever is already there). Both are immune to how many turns are currently mounted. Also names each pass's upload with the real pass number (plan-590-pass4.md) instead of re-uploading the plan/diff file's own literal path every time — that path is the review-session identity and must never move, so a same-content, differently-named temp copy is uploaded instead. Fixes the ChatGPT-side collision-rename confusion above and, as a side effect, makes it easy to see in the ChatGPT UI which upload belongs to which pass. state.mjs's session record gains `lastResponseFingerprint` alongside `passCount`. Removed the now-fully-unused `assistantCount()`. Added two tests simulating a pruned DOM (assistant locator always returns exactly one element, with different content across submit/recovery) proving both paths detect the new response correctly regardless of how many turns are actually mounted. 30/30 tests pass. --- .../chatgpt-review/scripts/chatgpt-review.mjs | 26 +++++++-- skills/chatgpt-review/scripts/lib/browser.mjs | 42 +++++++++------ skills/chatgpt-review/scripts/lib/state.mjs | 2 +- skills/chatgpt-review/tests/browser.test.mjs | 54 ++++++++++++++++--- skills/chatgpt-review/tests/core.test.mjs | 19 +++++-- 5 files changed, 111 insertions(+), 32 deletions(-) diff --git a/skills/chatgpt-review/scripts/chatgpt-review.mjs b/skills/chatgpt-review/scripts/chatgpt-review.mjs index c990f6f6..e949b7e1 100755 --- a/skills/chatgpt-review/scripts/chatgpt-review.mjs +++ b/skills/chatgpt-review/scripts/chatgpt-review.mjs @@ -34,7 +34,8 @@ export async function run(argv, dependencies = {}) { } const prepared = await prepare(options); - cleanup = prepared.cleanup ?? cleanup; + const prepareCleanup = prepared.cleanup ?? (async () => {}); + cleanup = prepareCleanup; if (options.session) { 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'); @@ -43,6 +44,23 @@ export async function run(argv, dependencies = {}) { } passNumber = (session.passCount ?? 0) + 1; if (options.mode === 'pr' && passNumber > 3) throw new CliError('PR review sessions permit at most three total passes'); + + // Name each pass's upload distinctly (plan-590-pass4.md, not plan-590.md every time) — the + // plan/diff file's own path must never move (it is the plan-review-loop's session identity), + // so we upload a same-content, differently-named COPY. Reusing one literal filename across + // many passes made ChatGPT's own upload UI collision-rename it (plan-590(9).md) after enough + // retries, which is confusing and unrelated to the real pass count. + let uploadPath = prepared.uploadPath; + if (uploadPath) { + const ext = path.extname(uploadPath); + const base = path.basename(uploadPath, ext); + const content = await fs.readFile(uploadPath, 'utf8'); + const renamed = await writePrivateTempFile(`${base}-pass${passNumber}${ext}`, content); + uploadPath = renamed.filename; + const uploadCleanup = renamed.cleanup; + cleanup = async () => { await uploadCleanup(); await prepareCleanup(); }; + } + const context = options.questionFile ? await fs.readFile(path.resolve(options.questionFile), 'utf8') : ''; const prompt = buildPrompt({ mode: options.mode, @@ -51,19 +69,19 @@ export async function run(argv, dependencies = {}) { publish: options.requestedPublication, pass: passNumber, previousSha: session.reportedReviewedSha, - uploadName: prepared.uploadPath ? path.basename(prepared.uploadPath) : null, + uploadName: uploadPath ? path.basename(uploadPath) : null, }); const review = await driver.review({ session: options.session ? session : null, prompt, - uploadPath: prepared.uploadPath, + uploadPath, timeoutMs: options.timeoutMs, target: prepared.target, publish: options.requestedPublication, diagnosticsDir: options.diagnosticsDir ? path.resolve(options.diagnosticsDir) : null, }); const metadata = extractReportedMetadata(review.responseText); - session = await store.write({ ...session, conversationUrl: review.conversationUrl, passCount: passNumber, ...metadata }); + session = await store.write({ ...session, conversationUrl: review.conversationUrl, passCount: passNumber, lastResponseFingerprint: review.responseFingerprint ?? null, ...metadata }); return resultDocument({ status: 'completed', response_text: review.responseText, session: session.handle, conversation_url: review.conversationUrl, elapsed_seconds: elapsed(started), pass_number: passNumber, diff --git a/skills/chatgpt-review/scripts/lib/browser.mjs b/skills/chatgpt-review/scripts/lib/browser.mjs index 405ddc1a..fbbb42f1 100644 --- a/skills/chatgpt-review/scripts/lib/browser.mjs +++ b/skills/chatgpt-review/scripts/lib/browser.mjs @@ -1,5 +1,14 @@ import fs from 'node:fs/promises'; import path from 'node:path'; +import crypto from 'node:crypto'; + +// 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 +// drop as old turns are unmounted. Tracking "is there a new response" by content fingerprint +// of the current LAST assistant message (instead of by count) survives that pruning. +export function fingerprintText(text) { + return text ? crypto.createHash('sha256').update(text).digest('hex') : null; +} export const SELECTORS = Object.freeze({ composer: ['[data-testid="prompt-textarea"]', '#prompt-textarea', 'textarea[placeholder*="Message"]', '[contenteditable="true"][role="textbox"]'], @@ -73,21 +82,22 @@ export class ChatGptBrowser { const { page, reopened } = await this.pageFor(session); try { await this.assertReady(page); - const existingCount = await this.assistantCount(page); const generationActive = await anyVisible(page, SELECTORS.stop); - const recordedPasses = session?.passCount ?? 0; - if (session && (generationActive || existingCount > recordedPasses)) { - this.stderr.write(`Recovering an uncollected ChatGPT response (recorded passes: ${recordedPasses})...\n`); - const responseText = await this.waitForCompletion(page, { before: recordedPasses, timeoutMs, target, publish }); - return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: true }; + 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 }); + return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: true, responseFingerprint: fingerprintText(responseText) }; } if (uploadPath) await this.upload(page, uploadPath); - const before = await this.assistantCount(page); + const before = currentTail; await this.fillAndSend(page, prompt); await this.waitForPermanentConversationUrl(page); - this.stderr.write(`Waiting for ChatGPT response (assistant messages before submit: ${before})...\n`); + this.stderr.write('Waiting for ChatGPT response...\n'); const responseText = await this.waitForCompletion(page, { before, timeoutMs, target, publish }); - return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: false }; + return { responseText, conversationUrl: page.url(), reopened, predefinedModelAndEffort: true, recovered: false, responseFingerprint: fingerprintText(responseText) }; } catch (error) { error.conversationUrl = page.url(); if (diagnosticsDir) await this.captureDiagnostics(page, diagnosticsDir, error).catch(() => {}); @@ -126,10 +136,6 @@ export class ChatGptBrowser { else await composer.press('Enter'); } - async assistantCount(page) { - return page.locator(SELECTORS.assistant[0]).count(); - } - async waitForPermanentConversationUrl(page) { const deadline = this.now() + 15_000; while (this.now() < deadline) { @@ -181,12 +187,16 @@ export class ChatGptBrowser { await this.handlePermission(page, target, publish); const continuation = await firstVisible(page, SELECTORS.continue); if (continuation) { await continuation.click(); stableSince = null; } - const count = await this.assistantCount(page); - const text = count > before ? await this.latestAssistantText(page) : ''; + const currentText = await this.latestAssistantText(page); + // before === null means "recovering an uncollected response" — accept whatever is + // already there. Otherwise before is the pre-submission baseline text (possibly ''); + // only a DIFFERENT tail counts as the new response. Content-based, not count-based, + // 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 (text && text === lastText) stableSince ??= this.now(); else { lastText = text; stableSince = text ? this.now() : null; } - if (count > before && text && !generating && stableSince !== null && this.now() - stableSince >= this.stableMs) return text; + if (text && !generating && stableSince !== null && this.now() - stableSince >= this.stableMs) return text; 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 90684123..080c5d4e 100644 --- a/skills/chatgpt-review/scripts/lib/state.mjs +++ b/skills/chatgpt-review/scripts/lib/state.mjs @@ -67,6 +67,6 @@ export class SessionStore { } function sanitize(record) { - const allowed = ['handle', 'mode', 'targetIdentity', 'canonicalUrl', 'conversationUrl', 'passCount', 'createdAt', 'updatedAt', 'reportedReviewedSha', 'reportedGithubCommentUrl']; + const allowed = ['handle', 'mode', 'targetIdentity', 'canonicalUrl', 'conversationUrl', 'passCount', 'lastResponseFingerprint', 'createdAt', 'updatedAt', 'reportedReviewedSha', 'reportedGithubCommentUrl']; return Object.fromEntries(allowed.filter((key) => record[key] !== undefined).map((key) => [key, record[key]])); } diff --git a/skills/chatgpt-review/tests/browser.test.mjs b/skills/chatgpt-review/tests/browser.test.mjs index f9216bf8..c54f55f3 100644 --- a/skills/chatgpt-review/tests/browser.test.mjs +++ b/skills/chatgpt-review/tests/browser.test.mjs @@ -108,6 +108,44 @@ test('session retry recovers an uncollected response without sending a duplicate assert.equal(composer.value, undefined); }); +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; } }); + // 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 + // before/after check could never observe growth. + const page = new Page('https://chatgpt.com/', { + [SELECTORS.composer[0]]: [composer], [SELECTORS.send[0]]: [send], + [SELECTORS.assistant[0]]: () => [new Element({ text: sent ? 'brand new answer' : 'stale old answer' })], + }); + const driver = driverWith(page); + const result = await driver.review({ prompt: 'review', timeoutMs: 20, target: null, publish: false }); + assert.equal(result.responseText, 'brand new answer'); +}); + +test('recovery via stored fingerprint detects an uncollected response under DOM pruning, without a generation indicator', async () => { + let sent = false; + const composer = new Element(); + const page = new Page('https://chatgpt.com/c/recover-pruned', { + [SELECTORS.composer[0]]: [composer], + [SELECTORS.send[0]]: [new Element({ onClick: () => { sent = true; } })], + [SELECTORS.assistant[0]]: [new Element({ text: 'new uncollected answer' })], + }); + const driver = driverWith(page); + const result = await driver.review({ + // passCount/fingerprint reflect an earlier, DIFFERENT response never seen live on this + // page — simulating a prior invocation that crashed after ChatGPT answered but before it + // recorded anything. Absolute message count plays no part in this decision. + session: { conversationUrl: page.url(), passCount: 3, lastResponseFingerprint: 'stale-fingerprint-from-a-different-answer' }, + prompt: 'must not send', timeoutMs: 20, target: null, publish: false, + }); + assert.equal(result.responseText, 'new uncollected answer'); + assert.equal(result.recovered, true); + assert.equal(sent, false); +}); + test('submission waits for ChatGPT to replace its temporary conversation URL', async () => { const page = readyPage(); page.currentUrl = 'https://chatgpt.com/c/WEB:temporary'; @@ -125,7 +163,7 @@ test('continue generating is clicked harmlessly', async () => { const button = new Element({ onClick: (self) => { self.visible = false; } }); const page = readyPage({ [SELECTORS.continue[0]]: [button], [SELECTORS.assistant[0]]: [new Element({ text: 'done' })] }); const driver = driverWith(page); - assert.equal(await driver.waitForCompletion(page, { before: 0, timeoutMs: 20, publish: false }), 'done'); + assert.equal(await driver.waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }), 'done'); assert.equal(button.visible, false); }); @@ -138,7 +176,7 @@ test('message stream failures use Retry without completing or creating a new pro [SELECTORS.streamRetry[0]]: () => failed ? [retry] : [], [SELECTORS.assistant[0]]: () => [new Element({ text: failed ? 'Error in message stream\nRetry' : 'complete answer' })], }); - assert.equal(await driverWith(page).waitForCompletion(page, { before: 0, timeoutMs: 20, publish: false }), 'complete answer'); + assert.equal(await driverWith(page).waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }), 'complete answer'); assert.equal(retries, 1); }); @@ -151,7 +189,7 @@ test('persistent message stream failure is typed after two retries', async () => [SELECTORS.assistant[0]]: [new Element({ text: 'Error in message stream\nRetry' })], }); await assert.rejects( - () => driverWith(page).waitForCompletion(page, { before: 0, timeoutMs: 20, publish: false }), + () => driverWith(page).waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }), (error) => error.status === 'ui_incompatible' && /after two automatic retries/.test(error.message), ); assert.equal(retries, 2); @@ -163,11 +201,11 @@ test('login failure, UI drift, rate limit, and timeout are typed', async () => { const drift = new Page(); await assert.rejects(() => driverWith(drift).assertReady(drift), (error) => error.status === 'ui_incompatible'); const rate = readyPage({ [SELECTORS.rateLimit[0]]: [new Element()] }); - await assert.rejects(() => driverWith(rate).waitForCompletion(rate, { before: 0, timeoutMs: 2 }), (error) => error.status === 'rate_limited'); + await assert.rejects(() => driverWith(rate).waitForCompletion(rate, { before: '', timeoutMs: 2 }), (error) => error.status === 'rate_limited'); const uiError = readyPage({ [SELECTORS.error[0]]: [new Element({ text: 'Something went wrong' })] }); - await assert.rejects(() => driverWith(uiError).waitForCompletion(uiError, { before: 0, timeoutMs: 2 }), (error) => error.status === 'ui_incompatible'); + await assert.rejects(() => driverWith(uiError).waitForCompletion(uiError, { before: '', timeoutMs: 2 }), (error) => error.status === 'ui_incompatible'); const timeout = readyPage(); - await assert.rejects(() => driverWith(timeout).waitForCompletion(timeout, { before: 0, timeoutMs: 2 }), (error) => error.status === 'timed_out'); + await assert.rejects(() => driverWith(timeout).waitForCompletion(timeout, { before: '', timeoutMs: 2 }), (error) => error.status === 'timed_out'); }); test('empty and status live-region alerts do not abort an active review', async () => { @@ -175,7 +213,7 @@ test('empty and status live-region alerts do not abort an active review', async [SELECTORS.alert[0]]: [new Element({ text: '' }), new Element({ text: 'ChatGPT is working' })], [SELECTORS.assistant[0]]: [new Element({ text: 'complete answer' })], }); - assert.equal(await driverWith(page).waitForCompletion(page, { before: 0, timeoutMs: 20, publish: false }), 'complete answer'); + assert.equal(await driverWith(page).waitForCompletion(page, { before: '', timeoutMs: 20, publish: false }), 'complete answer'); }); test('live-region alerts are fatal only when their text identifies a real failure', async () => { @@ -184,7 +222,7 @@ test('live-region alerts are fatal only when their text identifies a real failur assert.deepEqual(classifyAlertText('Too many requests; try again later'), { status: 'rate_limited', message: 'Too many requests; try again later' }); assert.deepEqual(classifyAlertText('Something went wrong'), { status: 'ui_incompatible', message: 'Something went wrong' }); const page = readyPage({ [SELECTORS.alert[0]]: [new Element({ text: 'There was an error generating a response' })] }); - await assert.rejects(() => driverWith(page).waitForCompletion(page, { before: 0, timeoutMs: 2 }), (error) => error.status === 'ui_incompatible'); + await assert.rejects(() => driverWith(page).waitForCompletion(page, { before: '', timeoutMs: 2 }), (error) => error.status === 'ui_incompatible'); }); test('only a scoped comment permission is automatically approvable', () => { diff --git a/skills/chatgpt-review/tests/core.test.mjs b/skills/chatgpt-review/tests/core.test.mjs index 0831273a..2fba4b94 100644 --- a/skills/chatgpt-review/tests/core.test.mjs +++ b/skills/chatgpt-review/tests/core.test.mjs @@ -168,7 +168,7 @@ test('run retains a session and enforces three PR passes', async () => { assert.match(result.error, /at most three/); }); -test('plan mode uploads exactly the supplied file and never authorizes publication', async (t) => { +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 })); const planFile = path.join(dir, 'complete-plan.md'); @@ -178,10 +178,23 @@ test('plan mode uploads exactly the supplied file and never authorizes publicati async create(data) { return { handle: '00000000-0000-4000-8000-000000000002', passCount: 0, ...data }; }, async write(value) { return value; }, }; - const driver = { async review(input) { observed = input; return { responseText: 'review', conversationUrl: 'https://chatgpt.com/c/plan' }; } }; + let uploadContentAtCallTime; + const driver = { + async review(input) { + observed = input; + // Must read here: the temp copy is cleaned up in run()'s `finally` before it returns. + uploadContentAtCallTime = await fs.readFile(input.uploadPath, 'utf8'); + return { responseText: 'review', conversationUrl: 'https://chatgpt.com/c/plan' }; + }, + }; const result = await run(['plan', planFile], { store, driver }); assert.equal(result.status, 'completed'); - assert.equal(observed.uploadPath, planFile); + // The plan file's own path is the review-session identity and must never be the literal + // upload target — re-uploading one unchanging filename every pass is what caused ChatGPT's + // own UI to collision-rename it (plan-590(9).md) after enough retries. + assert.notEqual(observed.uploadPath, planFile); + assert.match(path.basename(observed.uploadPath), /^complete-plan-pass1\.md$/); + assert.equal(uploadContentAtCallTime, '# Complete plan\n'); assert.equal(observed.publish, false); assert.match(observed.prompt, /Do not write anything to GitHub/); });