From d0c5558960603b6bb1486e37829d7758186a8840 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 25 Jul 2026 19:34:51 -0700 Subject: [PATCH] Gate the agent_done marker release to carbonize; hedge the failure toast Cursor Bugbot caught a real hole: accept unlocks at the first variant, so a late generation agent_done for the same session id could arrive after Accept and close the awaited failure window early, reopening the exact #384 gap. The SSE broadcast carries no sourceEventType, so only a carbonize agent_done is provably accept-side; the release is now gated on it. Copilot's wording point led somewhere real too: a carbonize-phase failure raises the same error after the source WAS promoted, so the toast now says "may not have been saved" and normalizes the server message's terminal punctuation. Regression guard extended to pin both. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code --- skill/scripts/live-browser.js | 17 ++++++++++++----- tests/live-browser-regression.test.mjs | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index dcac7f4d1..eb3e533bf 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -6490,10 +6490,12 @@ // The deterministic accept has already committed the reviewed DOM // and fenced generation. Carbonize may continue in the background; // it must not hold the foreground picker hostage. - // Any agent_done naming the awaited id post-accept is accept-side - // work reporting back (accept ack or carbonize), so the awaited - // failure window closes here too. - if (awaitingAcceptResult?.id && msg.id === awaitingAcceptResult.id) awaitingAcceptResult = null; + // Only a carbonize agent_done is provably accept-side: accept + // unlocks at the first variant, so a late generation agent_done + // for the same session id can still arrive after Accept and must + // not close the awaited failure window early (the SSE broadcast + // carries no sourceEventType to tell the two apart). + if (msg.data?.carbonize === true && awaitingAcceptResult?.id && msg.id === awaitingAcceptResult.id) awaitingAcceptResult = null; if (msg.data?.carbonize === true && maybeCompleteAcceptedSession(msg)) break; break; case 'discarded': @@ -6518,7 +6520,12 @@ if (awaitingAcceptResult?.id && msg.id === awaitingAcceptResult.id) { awaitingAcceptResult = null; console.error('[impeccable] Accept failed after teardown:', msg.message); - showToast('Accept failed: ' + msg.message + ' The variant was not saved; pick the element and generate again.', 8000); + // Hedged on purpose: a carbonize-phase failure raises this same + // error after the source WAS promoted, so "was not saved" would + // overclaim. Normalize the server message's terminal punctuation + // so the two sentences don't run together. + const acceptFailDetail = String(msg.message || 'unknown error').trim().replace(/[.!?]?$/, '.'); + showToast('Accept failed: ' + acceptFailDetail + ' The variant may not have been saved. If the change is missing, pick the element and generate again.', 8000); break; } if (maybeCompleteSteer(msg)) break; diff --git a/tests/live-browser-regression.test.mjs b/tests/live-browser-regression.test.mjs index 4fb9e6c47..2106bc1a6 100644 --- a/tests/live-browser-regression.test.mjs +++ b/tests/live-browser-regression.test.mjs @@ -318,8 +318,18 @@ describe('live-browser.js regression guards', () => { assert.ok(errorCase, 'expected an SSE case \'error\' handler in live-browser.js'); assert.match( errorCase[0], - /if \(awaitingAcceptResult\?\.id && msg\.id === awaitingAcceptResult\.id\) \{[\s\S]{0,400}?awaitingAcceptResult = null;[\s\S]{0,400}?not saved[\s\S]{0,200}?break;/, - 'an error naming the awaited accept must clear the marker and tell the user the variant was not saved', + /if \(awaitingAcceptResult\?\.id && msg\.id === awaitingAcceptResult\.id\) \{[\s\S]{0,700}?awaitingAcceptResult = null;[\s\S]{0,700}?may not have been saved[\s\S]{0,300}?break;/, + 'an error naming the awaited accept must clear the marker and warn that the variant may not have been saved (hedged: a carbonize-phase failure fires this after the source WAS promoted)', + ); + // Accept unlocks at the first variant, so a late generation agent_done + // for the same session id can arrive after Accept; only a carbonize + // agent_done is provably accept-side and may close the window. + const agentDoneCase = SOURCE.match(/case 'agent_done':[\s\S]{0,1200}?break;/); + assert.ok(agentDoneCase, 'expected an SSE case \'agent_done\' handler in live-browser.js'); + assert.match( + agentDoneCase[0], + /msg\.data\?\.carbonize === true && awaitingAcceptResult\?\.id && msg\.id === awaitingAcceptResult\.id/, + 'agent_done must only release the awaited accept marker for carbonize completions, or a late generation agent_done reopens the #384 hole', ); const cleanupFn = SOURCE.match(/function cleanupAcceptedSession\(\) \{[\s\S]{0,1200}?\n \}/); assert.ok(cleanupFn, 'expected cleanupAcceptedSession in live-browser.js');