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