Merge pull request #418 from pbakaus/accept-failure-recovery

Live mode: recognize a late accept failure after the optimistic teardown

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,20 @@
           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.
+          // 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':
@@ -6491,11 +6507,27 @@
         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);
+            // 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;
           console.error('[impeccable] Error:', msg.message);
           showToast('Error: ' + msg.message, 5000);
@@ -6958,6 +6990,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 +7072,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 +7906,7 @@ void main() {
         markSessionHandled();
         setLiveState('CONFIRMED');
         document.documentElement.dataset.impeccableAcceptToPickingMs = String(Date.now() - acceptPayload.clientSentAt);
+        awaitingAcceptResult = { id: acceptedSessionId };
         scheduleAcceptCleanup(pending);
       })
       .catch(() => {

tests/live-browser-regression.test.mjs

@@ -298,6 +298,48 @@ 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,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');
+    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,

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