From f9ea2f0de0b25622d0767330b7adc5d8b91f107c Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 25 Jul 2026 19:24:53 -0700 Subject: [PATCH] Recognize a late accept failure after the optimistic teardown Accept is optimistic: POST /events acknowledging the intent schedules cleanupAcceptedSession(), which nulls pendingAcceptedSession before live-accept.mjs has run. When the accept later failed (missing markers, preview error, receipt conflict, source_locked), the SSE 'error' guard keyed on pendingAcceptedSession could no longer match its id, so the tailored recovery never fired: the user got a generic error toast, the session was gone, and nothing said the variant was never written (issue #384, analysis by Cursor Bugbot on #381). Following the issue's fix sketch, an awaitingAcceptResult id is set on the optimistic success path and deliberately survives the teardown. The 'error' case matches it and tells the user plainly that the variant was not saved and to pick + generate again (post-teardown the wrapper may already be gone, so restoring CYCLING is not honestly possible). The marker is released when the real accept result arrives (complete / accept / post-accept agent_done) or when a new session supersedes it. Regression guard covers the set-before-teardown ordering, the error match, and cleanupAcceptedSession leaving the marker alone; the existing source contract now also asserts handleGo clears it. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code --- skill/scripts/live-browser.js | 32 ++++++++++++++++++++++++++ tests/live-browser-regression.test.mjs | 32 ++++++++++++++++++++++++++ tests/live-browser-source.test.mjs | 6 ++--- 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index ecfc212c6..dcac7f4d1 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -175,6 +175,14 @@ let pickedAnchorViewportTop = null; let pendingVariantAnchorRetryObserver = null; let pendingAcceptedSession = null; + // Survives cleanupAcceptedSession on purpose: the id of an accept whose + // POST was acknowledged (intent durable, epoch fenced) but whose actual + // source promotion hasn't reported back yet. Accept is optimistic, so the + // teardown nulls pendingAcceptedSession long before live-accept.mjs runs; + // this marker is what lets the SSE 'error' branch still recognize a late + // accept failure and say the variant was not saved (issue #384). Released + // when the real accept result arrives or a new session starts. + let awaitingAcceptResult = null; let variantObserver = null; let variantSelectionInFlight = false; let variantSelectionPromise = null; @@ -6474,12 +6482,18 @@ break; case 'complete': case 'accept': + // The real accept result arrived: the awaited failure window closed. + if (awaitingAcceptResult?.id && msg.id === awaitingAcceptResult.id) awaitingAcceptResult = null; if (maybeCompleteAcceptedSession(msg)) break; break; case 'agent_done': // 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; if (msg.data?.carbonize === true && maybeCompleteAcceptedSession(msg)) break; break; case 'discarded': @@ -6491,11 +6505,22 @@ case 'error': if (pendingAcceptedSession?.id && msg.id === pendingAcceptedSession.id) { pendingAcceptedSession = null; + awaitingAcceptResult = null; setLiveState('CYCLING'); updateBarContent('cycling'); showToast('Could not complete accept cleanup. Try Accept again.', 5000); break; } + // The optimistic teardown already released the session, so the + // CYCLING recovery above can no longer match; without this branch + // the failure fell through to the generic toast and the user had + // no hint their variant was never written (issue #384). + 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); + break; + } if (maybeCompleteSteer(msg)) break; console.error('[impeccable] Error:', msg.message); showToast('Error: ' + msg.message, 5000); @@ -6958,6 +6983,9 @@ stripManualEditRuntimeState(selectedElement); pendingAcceptedSession = null; + // A new session supersedes any accept still awaiting its result; a late + // failure toast for the previous session would only mislead here. + awaitingAcceptResult = null; currentSessionId = id8(); expectedVariants = selectedCount; arrivedVariants = 0; @@ -7037,6 +7065,9 @@ stopVoice({ suppressSubmit: true }); pendingAcceptedSession = null; + // A new session supersedes any accept still awaiting its result; a late + // failure toast for the previous session would only mislead here. + awaitingAcceptResult = null; currentSessionId = id8(); expectedVariants = selectedCount; arrivedVariants = 0; @@ -7868,6 +7899,7 @@ void main() { markSessionHandled(); setLiveState('CONFIRMED'); document.documentElement.dataset.impeccableAcceptToPickingMs = String(Date.now() - acceptPayload.clientSentAt); + awaitingAcceptResult = { id: acceptedSessionId }; scheduleAcceptCleanup(pending); }) .catch(() => { diff --git a/tests/live-browser-regression.test.mjs b/tests/live-browser-regression.test.mjs index c092cc7db..4fb9e6c47 100644 --- a/tests/live-browser-regression.test.mjs +++ b/tests/live-browser-regression.test.mjs @@ -298,6 +298,38 @@ describe('live-browser.js regression guards', () => { ); }); + it('a late accept failure is recognized after the optimistic teardown (#384)', () => { + // Accept is optimistic: POST /events acknowledging the intent schedules + // cleanupAcceptedSession(), which nulls pendingAcceptedSession before + // live-accept.mjs has actually run. When the accept later fails (missing + // markers, preview error, receipt conflict, source_locked), the SSE + // 'error' guard keyed on pendingAcceptedSession could no longer match, + // so the user got only the generic error toast with no hint that their + // variant was never written. An awaitingAcceptResult id must be set on + // the optimistic success path, survive cleanupAcceptedSession, be + // matched in the 'error' case with an explicit not-saved message, and + // be released when the real accept result arrives. + assert.match( + SOURCE, + /awaitingAcceptResult = \{ id: acceptedSessionId \};[\s\S]{0,400}?scheduleAcceptCleanup\(pending\);/, + 'the optimistic POST-success path must record awaitingAcceptResult before scheduling the teardown', + ); + const errorCase = SOURCE.match(/case 'error':[\s\S]{0,2600}?setLiveState\('PICKING'\);\s*break;/); + 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', + ); + const cleanupFn = SOURCE.match(/function cleanupAcceptedSession\(\) \{[\s\S]{0,1200}?\n \}/); + assert.ok(cleanupFn, 'expected cleanupAcceptedSession in live-browser.js'); + assert.doesNotMatch( + cleanupFn[0], + /awaitingAcceptResult\s*=/, + 'cleanupAcceptedSession must not clear awaitingAcceptResult - surviving the teardown is the point', + ); + }); + it('handleServerLost preserves the current recoverable phase', () => { assert.doesNotMatch( SOURCE, diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index f30141c9b..d2539df04 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -320,7 +320,7 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /case 'complete':\s*case 'accept':\s*if \(maybeCompleteAcceptedSession\(msg\)\) break;/, + /case 'complete':\s*case 'accept':[\s\S]{0,400}?if \(maybeCompleteAcceptedSession\(msg\)\) break;/, 'final accepted DOM cleanup should be driven by explicit complete or harness accept replies', ); assert.match( @@ -340,8 +340,8 @@ describe('live-browser source contracts', () => { assert.match(agentDoneSource, /maybeCompleteAcceptedSession\(msg\)/); assert.match( SOURCE, - /function handleGo\(\)[\s\S]{0,900}?pendingAcceptedSession = null;[\s\S]{0,80}?currentSessionId = id8\(\);/, - 'starting a new generation should clear any stale accepted-session sentinel first', + /function handleGo\(\)[\s\S]{0,900}?pendingAcceptedSession = null;[\s\S]{0,400}?awaitingAcceptResult = null;[\s\S]{0,120}?currentSessionId = id8\(\);/, + 'starting a new generation should clear any stale accepted-session sentinel (and the awaited accept-result marker, #384) first', ); const handleAcceptStart = SOURCE.indexOf('function handleAccept()'); const maybeCompleteStart = SOURCE.indexOf('function maybeCompleteAcceptedSession', handleAcceptStart);