Merge salvage/4750-composer-clear

static/messages.js

@@ -1552,9 +1552,41 @@ async function send(){
   const activeSid=S.session.session_id;
   _sendInProgressSid=activeSid;
 
-  setComposerStatus(S.pendingFiles&&S.pendingFiles.length?'Uploading…':'');
+  // Salvage of #4750 (@harryazj): capture the composer text and clear the
+  // textarea NOW — immediately after capture and BEFORE the uploadPendingFiles()
+  // / forced-skill-directive awaits below. send() re-reads the LIVE composer when
+  // it is re-entered while a send is in flight (the _sendInProgress guard at the
+  // top of this function reads _composerTextWithPendingSelections()). If we
+  // cleared only after the async work — as the pre-fix code did, down at the
+  // _clearComposerDraft site — a re-entrant/interrupt-mode send during the upload
+  // window would read the still-populated DOM and double-submit the same message.
+  // _submittedDraftTextForClear is the sole authority for the send-time draft
+  // signature from here down; no code path below re-reads $('msg').value on the
+  // happy path.
+  const _submittedDraftTextForClear=$('msg').value||'';
+  $('msg').value='';autoResize();
+
+  // #5912 gate CORE fix: snapshot the pending files that belong to THIS send
+  // BEFORE the await, and upload exactly that snapshot. Otherwise a re-entrant /
+  // interrupt-mode send during the upload window inherits the still-live
+  // S.pendingFiles and later re-uploads the first send's attachment. Detach the
+  // snapshot from S.pendingFiles now so files staged AFTER this point belong to
+  // the next send only.
+  const _submittedFiles=[...(S.pendingFiles||[])];
+  const _submittedDraftFilesForClear=[..._submittedFiles];
+  S.pendingFiles=[];
+  if(typeof renderTray==='function')renderTray();
+
+  // #5912 gate SILENT fix: clear the PERSISTED draft here — alongside the
+  // textarea clear, BEFORE any await — so a new draft typed during the upload
+  // window is not clobbered by a delayed text:'' post. Keep the promise so the
+  // #5472 failed-send restore can chain its re-persist after this clear resolves.
+  let _composerDraftClearPromise=null;
+  if (activeSid && typeof _clearComposerDraft === 'function') _composerDraftClearPromise=_clearComposerDraft(activeSid,_submittedDraftTextForClear,_submittedDraftFilesForClear);
+
+  setComposerStatus(_submittedFiles.length?'Uploading…':'');
   let uploaded=[];
-  try{uploaded=await uploadPendingFiles();}
+  try{uploaded=await uploadPendingFiles({files:_submittedFiles, sessionId:activeSid, clearPending:false});}
   catch(e){if(!text){setComposerStatus(`Upload error: ${e.message}`);return;}}
   // Clear the uploading status now that upload is done — if we don't clear here
   // it stays visible for the entire duration of the agent stream, since
@@ -1589,16 +1621,11 @@ async function send(){
     }
   }
   if(!msgText){setComposerStatus('Nothing to send');return;}
-  const _submittedDraftTextForClear=$('msg').value||'';
-  const _submittedDraftFilesForClear=Array.isArray(_failedSendFilesSnapshot)?[..._failedSendFilesSnapshot]:[];
-  $('msg').value='';autoResize();
-  // Clear persisted composer draft since message was sent. Capture the promise
-  // so the #5472 failed-send restore can chain its re-persist AFTER this clear
-  // resolves — otherwise the two same-origin POSTs (clear text:'' then restore
-  // text:<draft>) can be reordered under HTTP/2 multiplexing and leave the
-  // server draft empty after a reload. (Opus #5484 NIT.)
-  let _composerDraftClearPromise=null;
-  if (activeSid && typeof _clearComposerDraft === 'function') _composerDraftClearPromise=_clearComposerDraft(activeSid,_submittedDraftTextForClear,_submittedDraftFilesForClear);
+  // Composer textarea + persisted draft were already captured and cleared
+  // immediately after capture (above, salvage of #4750 + #5912 gate fix) to close
+  // the re-entrant double-send race AND avoid clobbering a draft typed during the
+  // upload window. _composerDraftClearPromise / _submittedDraftFilesForClear are
+  // set there; nothing to re-declare here.
   const displayText=_slashDisplayTextOverride||text||(uploaded.length?`Uploaded: ${uploadedNames.join(', ')}`:'(file upload)');
   const userMsg={role:'user',content:displayText,attachments:uploaded.length?uploadedNames:undefined,_ts:Date.now()/1000};
   S.toolCalls=[];  // clear tool calls from previous turn

tests/test_composer_capture_clear_race.py

@@ -0,0 +1,188 @@
+"""Regression coverage for the composer capture-and-clear race (salvage of #4750).
+
+Bug (salvaged from #4750, credit @harryazj): ``send()`` in ``static/messages.js``
+captured the composer text early but only wiped the textarea
+(``$('msg').value=''``) LATE — after ``await uploadPendingFiles()`` and the
+forced-skill-directive await. ``send()`` is re-entrant: while a send is in
+flight, a second invocation (e.g. an interrupt-mode / ``busy_input_mode`` drain,
+or a fast second Enter) hits the ``if (_sendInProgress)`` guard at the top and
+re-reads the LIVE composer via ``_composerTextWithPendingSelections()``. Because
+the textarea still held the original text during the async upload window, the
+re-entrant guard read the stale DOM and QUEUED the same message again ->
+double-submit.
+
+Fix: capture the composer value and clear the textarea IMMEDIATELY after capture,
+BEFORE any await. Once cleared, a re-entrant read sees an empty composer and the
+guard's ``if(_text && _targetSid)`` short-circuits -> no duplicate queue.
+
+This module verifies BOTH:
+  1. (static) the capture+wipe is ordered before the upload await in send(), and
+  2. (behavioral, via node's ``vm``) the REAL re-entrancy guard block extracted
+     from send() does NOT re-queue when the composer was already cleared, and
+     WOULD have re-queued had the composer still held the stale text.
+"""
+import json
+import shutil
+import subprocess
+import textwrap
+from pathlib import Path
+
+import pytest
+
+ROOT = Path(__file__).resolve().parents[1]
+MESSAGES_JS = ROOT.joinpath("static", "messages.js").read_text(encoding="utf-8")
+
+
+# ---------------------------------------------------------------------------
+# Static ordering assertions
+# ---------------------------------------------------------------------------
+
+def _function_body(src: str, name: str) -> str:
+    marker = f"function {name}"
+    start = src.index(marker)
+    brace = src.index("{", start)
+    depth = 1
+    i = brace + 1
+    while depth and i < len(src):
+        if src[i] == "{":
+            depth += 1
+        elif src[i] == "}":
+            depth -= 1
+        i += 1
+    return src[brace + 1 : i - 1]
+
+
+def test_composer_captured_and_cleared_before_upload_await():
+    """The fix: capture + textarea wipe must run before uploadPendingFiles()."""
+    body = _function_body(MESSAGES_JS, "send")
+
+    capture_idx = body.index("const _submittedDraftTextForClear=$('msg').value||'';")
+    # The wipe sits immediately after the capture.
+    wipe_rel = body.index("$('msg').value='';autoResize();", capture_idx)
+    assert wipe_rel - capture_idx < 120, "the textarea wipe must sit immediately after the capture"
+
+    upload_idx = body.index("uploaded=await uploadPendingFiles(")
+    directive_await_idx = body.index("const _directivePayload = await _pending.promise;")
+
+    assert capture_idx < upload_idx, (
+        "composer capture+clear must happen BEFORE the uploadPendingFiles() await "
+        "(salvage of #4750 — closes the re-entrant double-send race)"
+    )
+    assert capture_idx < directive_await_idx, (
+        "composer capture+clear must happen BEFORE the forced-skill-directive await too"
+    )
+
+
+def test_reentrancy_guard_reads_live_composer():
+    """Sanity: the in-flight guard reads the LIVE composer (why the clear must be early)."""
+    body = _function_body(MESSAGES_JS, "send")
+    guard_idx = body.index("if (_sendInProgress) {")
+    guard_block = body[guard_idx : body.index("_sendInProgress = true;", guard_idx)]
+    assert "_composerTextWithPendingSelections().trim()" in guard_block, (
+        "the re-entrant guard reads the live composer, so the composer must be "
+        "cleared before the first await to avoid a stale double-send"
+    )
+    assert "queueSessionMessage(" in guard_block, (
+        "the re-entrant guard queues the message it reads from the live composer"
+    )
+
+
+# ---------------------------------------------------------------------------
+# Behavioral test — run the REAL re-entrancy guard against a cleared composer
+# ---------------------------------------------------------------------------
+
+def _extract_reentrancy_guard() -> str:
+    """Slice the real `if (_sendInProgress) { ... }` guard block out of send()."""
+    body = _function_body(MESSAGES_JS, "send")
+    start = body.index("if (_sendInProgress) {")
+    # Balance braces from the guard's opening brace to its close.
+    brace = body.index("{", start)
+    depth = 1
+    i = brace + 1
+    while depth and i < len(body):
+        if body[i] == "{":
+            depth += 1
+        elif body[i] == "}":
+            depth -= 1
+        i += 1
+    return body[start:i]
+
+
+def _run_reentrant_guard_in_node(composer_value: str):
+    """Execute the REAL re-entrancy guard with a given live-composer value.
+
+    Returns the list of queueSessionMessage calls the guard made. A non-empty
+    list means the guard re-queued (double-submit); an empty list means it
+    short-circuited on an empty composer (the fix's post-clear state).
+    """
+    node = shutil.which("node")
+    if not node:  # pragma: no cover
+        pytest.skip("node not available")
+
+    helper = _function_body(MESSAGES_JS, "_composerTextWithPendingSelections")
+    guard = _extract_reentrancy_guard()
+
+    harness = textwrap.dedent(
+        """
+        const queued = [];
+        const state = { input: { value: %(composer_value)s } };
+        const $ = (id) => (id === 'msg' ? state.input : null);
+        global.document = { getElementById: (id) => $(id) };
+        // No pending inline selections in this scenario.
+        const _pendingSelections = [];
+        function _formatSelectedTextReplyQuote(t){ return t; }
+        // Real helper the guard uses to read the live composer.
+        function _composerTextWithPendingSelections(){%(helper)s}
+
+        // Minimal in-flight state: a send is already running for sid-1.
+        let _sendInProgress = true;
+        let _sendInProgressSid = 'sid-1';
+        const S = { session: { session_id: 'sid-1' }, pendingFiles: [], activeProfile: 'default' };
+
+        // Stubs the guard branch touches.
+        function _chatPayloadModelState(){ return { model: 'm', model_provider: 'p' }; }
+        function queueSessionMessage(sid, payload){ queued.push({ sid, payload }); }
+        function _clearComposerAfterQueuedSelectionSend(){ state.input.value = ''; }
+        function _clearComposerDraft(){}
+        function updateQueueBadge(){}
+        function renderTray(){}
+        function showToast(){}
+
+        // Run the REAL guard block verbatim.
+        (function () {
+          %(guard)s
+        })();
+
+        console.log(JSON.stringify({ queued, composerAfter: state.input.value }));
+        """
+    ) % {
+        "composer_value": json.dumps(composer_value),
+        "helper": helper,
+        "guard": guard,
+    }
+
+    proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30)
+    assert proc.returncode == 0, f"node harness failed: {proc.stderr}"
+    return json.loads(proc.stdout.strip())
+
+
+def test_reentrant_send_does_not_requeue_after_composer_cleared():
+    """With the fix, the composer is EMPTY when the re-entrant guard runs, so it
+    must NOT queue a duplicate of the stale text."""
+    out = _run_reentrant_guard_in_node("")
+    assert out["queued"] == [], (
+        "a re-entrant send must not re-queue anything once the composer has been "
+        "cleared by the in-flight send (salvage of #4750)"
+    )
+
+
+def test_reentrant_send_would_double_submit_if_composer_not_cleared():
+    """Sanity / bug demonstration: had the composer NOT been cleared before the
+    async window (the pre-fix behaviour), the re-entrant guard reads the stale
+    DOM text and queues a DUPLICATE — exactly the double-send this fix prevents."""
+    out = _run_reentrant_guard_in_node("hello world")
+    assert len(out["queued"]) == 1, (
+        "the stale-composer scenario must reproduce the double-submit the fix removes"
+    )
+    assert out["queued"][0]["payload"]["text"] == "hello world"
+    assert out["queued"][0]["sid"] == "sid-1"

tests/test_issue5472_preserve_draft_on_failed_send.py

@@ -74,7 +74,7 @@ def test_send_captures_immutable_snapshot_before_rewrites_and_upload():
         "const _failedSendFilesSnapshot=Array.isArray(S.pendingFiles)?[...S.pendingFiles]:[];"
     )
     moa_idx = MESSAGES_JS.find("text=_moaArgs;")
-    upload_idx = MESSAGES_JS.find("uploaded=await uploadPendingFiles();")
+    upload_idx = MESSAGES_JS.find("uploaded=await uploadPendingFiles(")
     assert snap_idx != -1 and files_idx != -1, "send() must snapshot text + files for #5472"
     assert moa_idx != -1 and upload_idx != -1
     # Snapshot happens before both the /moa rewrite and the upload drain.
@@ -93,22 +93,50 @@ def test_error_branch_restores_original_snapshot_not_mutated_payload():
 
 
 def test_send_still_clears_composer_on_the_happy_path():
-    # Anchor to the MAIN-path clear specifically (the send-time composer wipe +
-    # persisted-draft clear), not a bare `$('msg').value=''` string that also
-    # appears at ~13 other sites (slash returns, bundle-error paths). This
-    # assertion must actually guard the main send path's clear. (Opus #5484 NIT.)
+    # Anchor to the MAIN-path persisted-draft clear specifically (not a bare
+    # `$('msg').value=''` string that also appears at ~13 other sites — slash
+    # returns, bundle-error paths). This assertion must actually guard the main
+    # send path's clear.
     main_clear = (
         "if (activeSid && typeof _clearComposerDraft === 'function') "
         "_composerDraftClearPromise=_clearComposerDraft(activeSid,_submittedDraftTextForClear,_submittedDraftFilesForClear);"
     )
     assert main_clear in MESSAGES_JS, "main send path must clear the persisted draft at send time"
-    # The composer textarea wipe must sit immediately above that clear.
-    clear_idx = MESSAGES_JS.find(main_clear)
-    window_before = MESSAGES_JS[clear_idx - 700:clear_idx]
-    assert "const _submittedDraftTextForClear=$('msg').value||'';" in window_before
-    assert "const _submittedDraftFilesForClear=Array.isArray(_failedSendFilesSnapshot)?[..._failedSendFilesSnapshot]:[];" in window_before
-    assert "$('msg').value='';autoResize();" in window_before, (
-        "the main-path composer wipe must precede the persisted-draft clear"
+
+    # Salvage of #4750: the composer textarea capture + wipe was moved UP to run
+    # immediately after capture (right after `_sendInProgressSid=activeSid;`) and
+    # BEFORE `uploadPendingFiles()` / the forced-skill-directive await — so a
+    # re-entrant/interrupt-mode send during the async window can't re-read the
+    # still-populated DOM and double-submit. Verify that ordering here.
+    capture = "const _submittedDraftTextForClear=$('msg').value||'';"
+    assert capture in MESSAGES_JS, "send() must still capture the send-time draft text"
+    capture_idx = MESSAGES_JS.index(capture)
+    # The textarea wipe sits immediately after the capture (same 120-char window).
+    window_after_capture = MESSAGES_JS[capture_idx : capture_idx + 120]
+    assert "$('msg').value='';autoResize();" in window_after_capture, (
+        "the composer textarea wipe must sit immediately after the capture"
+    )
+    upload_idx = MESSAGES_JS.index("uploaded=await uploadPendingFiles(")
+    clear_idx = MESSAGES_JS.index(main_clear)
+    # THE FIX: capture+wipe happen before the upload await (closes the race)...
+    assert capture_idx < upload_idx, (
+        "composer must be captured+cleared BEFORE the uploadPendingFiles() await "
+        "so a re-entrant send can't re-read stale DOM text (salvage of #4750)"
+    )
+    # ...and the captured text is still what feeds the persisted-draft clear.
+    assert capture_idx < clear_idx, (
+        "the captured send-time draft text must precede the persisted-draft clear"
+    )
+    # The files snapshot (#5912 gate fix) is taken from S.pendingFiles BEFORE the
+    # upload await and feeds the persisted-draft clear which now runs before the await.
+    assert (
+        "const _submittedDraftFilesForClear=[..._submittedFiles];"
+        in MESSAGES_JS
+    ), "the files snapshot for the persisted-draft clear must be captured pre-await"
+    # The persisted-draft clear must run BEFORE the upload await (not after it),
+    # so a draft typed during the upload window is not clobbered.
+    assert clear_idx < upload_idx, (
+        "the persisted-draft clear must precede the uploadPendingFiles() await (#5912)"
     )