From 6fd14c240882cd5e225b5f4148afef0811ff7bd3 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sun, 16 Aug 2026 18:31:18 -0400 Subject: [PATCH 1/2] fix(chat): closing the tab tears the conversation down, not just seals it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both review points on #82. They landed on that PR's stale diff — I branched the wordmark off feat/chat-editor-only instead of develop, so #82 carried #81's commits until #81 merged. Both are real, and both are about code that is now on develop, so they are fixed here rather than in the wordmark PR (whose diff is now the two files it should always have been). 1. SEALING WAS ONLY HALF THE TEARDOWN sealLiveSession ends the SESSION — liveId() goes null, so the next chat opens visually empty — while `conversation` and `agentMessages` still held every previous turn. The next message therefore shipped the old history to the model. An empty-looking chat that secretly remembers is worse than either honest option. The out-of-process half was worse: background commands and MCP servers are DETACHED children, so they outlived the surface that was reporting on them, and an in-flight agent run kept editing files with nothing left to show for it. newChat's teardown moves into resetConversationState() and the close path calls it. One implementation, or the close path drifts — and it is the path nobody watches. It deliberately does NOT post: New Chat re-renders afterwards because it has a surface to re-render; the close path is tearing one down. 2. A LEGACY `secondarySidebar` SETTING PRODUCED A LYING LOG The value was valid until the chat became editor-only, so it is still sitting in real settings.json files. chatStartLocation still accepted it, so revealChatAtStartup logged `where: secondarySidebar` and then opened the editor tab. Right behaviour, wrong story — and the accepted set no longer matched the enum the package ships. Now mapped explicitly. Guards, each bypass-verified by reverting the fix: - the close sealing but leaving history loaded (the reported bug) - conversation, checkpoints, abort, reapCommands and reapMcp each removed individually - newChat growing its own copy of the teardown - the shared teardown starting to post, which is wrong on the close path - the legacy value no longer mapped; the removed surface accepted again Two bypasses initially looked like misses: commenting out `reapMcp()` and neutering the abort with `if (false)` both left the strings present, and these are presence checks. Redone as deletions — which is how they would actually regress — they fail correctly. Worth stating plainly: these guards catch removal, not disabling. 21 tests in chatSurface, 34 suites green. --- extensions/levelcode-ai/extension.js | 40 +++++++++++--- .../levelcode-ai/test/chatSurface.test.js | 55 ++++++++++++++++++- 2 files changed, 87 insertions(+), 8 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 9ebd1e8..d9e4ab5 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -1106,19 +1106,39 @@ function sealLiveSession(why) { } } -function newChat() { - // Seal the outgoing session (its terminal state + a final index row) BEFORE the transcript is cleared, - // so it lands in History as a finished session and the next turn opens a fresh one. - sealLiveSession('newChat'); +/** + * Drop everything the finished conversation was holding, in memory and out of process. + * + * Extracted so the CLOSING TAB does the same teardown New Chat does. Sealing alone was not enough and + * left the two halves disagreeing: `sealLiveSession` ends the session, so `liveId()` goes null and the + * next chat opens visually empty — while `conversation` and `agentMessages` still hold every previous + * turn, so the next message silently ships the old history to the model. An empty-looking chat that + * secretly remembers is worse than either honest option. + * + * The out-of-process half matters just as much. Background commands and MCP servers are DETACHED + * children: without reaping them they outlive the surface that was reporting on them, and an + * in-flight agent run keeps editing files with nothing left to show for it. + * + * Deliberately does NOT post to the webview. New Chat re-renders afterwards because it has a surface + * to re-render; the close path is tearing one down. + */ +function resetConversationState() { conversation = []; agentMessages = []; checkpoints.length = 0; currentCheckpoint = null; // drop the per-turn restore stack pendingContext = null; contextFiles = []; - if (abort) { abort.abort(); } + if (abort) { abort.abort(); } // stop an in-flight run — its surface is going away reapCommands(); // kill any background servers/watchers from the old session reapMcp(); // …and any MCP servers: they are detached children too - if (review) { review.finalizeAll(); } // drop review UI without reverting the user's files + if (review) { review.finalizeAll(); } // drop review UI without reverting the user's files +} + +function newChat() { + // Seal the outgoing session (its terminal state + a final index row) BEFORE the transcript is cleared, + // so it lands in History as a finished session and the next turn opens a fresh one. + sealLiveSession('newChat'); + resetConversationState(); post({ type: 'reset' }); postContextFiles(); postMemoryDigest(); // the fresh empty state shows the welcome-back strip @@ -2341,6 +2361,7 @@ async function openChatInEditor(opts) { chatEditorPanel = undefined; activeWebview = undefined; // nothing may post into a disposed webview sealLiveSession('chatClosed'); + resetConversationState(); // …and nothing may survive into the next one dbg('chat.closedEditor', {}); }); } @@ -2358,7 +2379,12 @@ async function openChatInEditor(opts) { */ function chatStartLocation() { const raw = String(aiConfig().get('chat.startLocation', 'editor') || 'editor'); - return ['editor', 'secondarySidebar', 'none'].includes(raw) ? raw : 'editor'; + // `secondarySidebar` was a valid value until the chat became editor-only, so it is still sitting in + // real settings.json files. Mapped explicitly rather than left to fall through the unknown-value + // path: the result is the same, but this way the debug log names the location we actually opened + // instead of reporting a surface that no longer exists. + if (raw === 'secondarySidebar') { return 'editor'; } + return ['editor', 'none'].includes(raw) ? raw : 'editor'; } /** Open the chat where `chat.startLocation` says, once, as the window finishes starting. */ diff --git a/extensions/levelcode-ai/test/chatSurface.test.js b/extensions/levelcode-ai/test/chatSurface.test.js index a5a16a2..931a81f 100644 --- a/extensions/levelcode-ai/test/chatSurface.test.js +++ b/extensions/levelcode-ai/test/chatSurface.test.js @@ -218,7 +218,7 @@ test('START: the chat opens centred by default, and the setting is the only plac // One reader, so a second caller cannot quietly disagree about what an unknown value means. const body = fnBody(ext, 'chatStartLocation'); - assert.match(body, /'editor', 'secondarySidebar', 'none'/, 'the reader no longer validates against the enum'); + assert.match(body, /'editor', 'none'/, 'the reader must validate against exactly the enum the package ships'); assert.match(body, /: 'editor'/, 'an unknown value must fall back to the default, not leave the window with no chat'); }); @@ -348,4 +348,57 @@ test('CLOSE: nothing resurrects the chat on the right', () => { 'Sessions must KEEP its view — it is the right-hand container\'s reason to exist'); }); +test('CLOSE: the conversation is torn down, not just sealed', () => { + // Review caught the two halves disagreeing. sealLiveSession ends the SESSION — liveId() goes null, + // so the next chat opens visually empty — while `conversation` and `agentMessages` still held every + // previous turn, so the next message shipped the old history to the model anyway. An empty-looking + // chat that secretly remembers is worse than either honest option. + const dispose = ext.slice(ext.indexOf('panel.onDidDispose'), ext.indexOf('panel.onDidDispose') + 1400); + assert.match(dispose, /sealLiveSession\('chatClosed'\)/, 'closing no longer seals the session'); + assert.match(dispose, /resetConversationState\(\)/, + 'closing seals but leaves conversation/agentMessages loaded — the next send replays the old history'); + + // ONE teardown, shared with New Chat, or the close path drifts — and it is the path nobody watches. + assert.match(fnBody(ext, 'newChat'), /resetConversationState\(\)/, + 'New Chat has its own copy of the teardown again'); + const reset = fnBody(ext, 'resetConversationState'); + for (const [frag, why] of [ + ['conversation = []', 'the model history survives the close'], + ['agentMessages = []', 'the agent history survives the close'], + ['checkpoints.length = 0', 'the restore stack still points at a finished turn'], + ['contextFiles = []', 'stale attachments carry into the next chat'], + ['abort.abort()', 'an in-flight run keeps going with no surface to report to'], + ['reapCommands()', 'background commands outlive the chat — they are detached children'], + ['reapMcp()', 'MCP servers outlive the chat — they are detached children too'] + ]) { + assert.ok(reset.includes(frag), why + ' (missing: ' + frag + ')'); + } + + // It must NOT post: the close path is tearing the surface down, and New Chat re-renders itself. + assert.ok(!/\bpost\(/.test(reset), + 'resetConversationState posts to the webview — on the close path that webview is being disposed'); +}); + +test('START: a legacy secondarySidebar setting maps to the editor, and says so', () => { + // The value was valid until the chat became editor-only, so it is still sitting in real + // settings.json files. Left to fall through the unknown-value path it produced the right BEHAVIOUR + // with a lying debug log — `where: secondarySidebar` while opening the editor tab. + const body = fnBody(ext, 'chatStartLocation'); + assert.match(body, /raw === 'secondarySidebar'/, 'the legacy value is not mapped explicitly'); + assert.ok(!/\['editor', 'secondarySidebar', 'none'\]/.test(body), + 'secondarySidebar is still an accepted value — it names a surface that no longer exists'); + assert.match(body, /\['editor', 'none'\]/, 'the accepted set should be exactly what the enum ships'); + + // Behaviour, evaluated from the SHIPPED source rather than a copy of it: fnBody hands back the + // braces, so wrapping it in a declaration gives the real function with aiConfig injected. + // aiConfig is CALLED and returns the config object, so the stub has to be a function that returns + // one — passing the object itself is the obvious thing and it is wrong. + const run = (v) => new Function('aiConfig', + 'function chatStartLocation() ' + body + '\nreturn chatStartLocation();')(() => ({ get: () => v })); + assert.strictEqual(run('secondarySidebar'), 'editor', 'a legacy setting must resolve to the editor'); + assert.strictEqual(run('none'), 'none', 'the opt-out must survive'); + assert.strictEqual(run('editor'), 'editor'); + assert.strictEqual(run('nonsense'), 'editor', 'an unknown value must fall back to the default'); +}); + console.log('\nchatSurface: ' + n + ' tests passed.'); From 1ad4f14b393502e006c725c3efd5356ae6251d66 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sun, 16 Aug 2026 19:38:55 -0400 Subject: [PATCH 2/2] fix(chat): a teardown invalidates in-flight work instead of racing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #83, and the first point is sharp: the teardown was losing a race it did not know it was in. Closing the tab mid-stream calls abort(), and the abort lands in handleSend's catch AFTER resetConversationState has already cleared `conversation` — where the abort branch pushed the partial reply straight back in. The result was a dangling assistant turn with no user turn in front of it, shipped to the model on the next send: exactly the leak the teardown exists to prevent, reintroduced by the teardown's own abort(). agentFlow's finally was worse than reported. runAgent holds `agentMessages` BY REFERENCE, so a teardown that rebinds the global leaves the run pushing into an orphaned array — and then `agentMessages.slice(sessTurnStart)` slices the NEW empty one with an index into the old, recording an empty turn against the wrong session. `abort = null` and `currentCheckpoint = null` are worse still: those clobber whatever turn came next, leaving a fresh run unstoppable and its checkpoint unclosed. Fixed with an epoch. resetConversationState bumps `conversationEpoch` FIRST — before clearing anything, so an abort landing mid-teardown already reads as stale — and each turn captures the epoch before installing its AbortController, then checks it before writing anything back. It also now clears pending approvals/questions, which the guarded finally no longer does on a torn-down turn. Second point: the chatStartLocation docstring still called `secondarySidebar` a supported surface ("kept because the sidebar is the right answer when…") while the code below mapped it away as legacy. In-code documentation sitting directly on top of the change is the worst place to leave a contradiction. Guards, each bypass-verified by reverting the fix: - the teardown no longer invalidating in-flight work - the epoch bumped AFTER clearing, which leaves the race window open - handleSend pushing back without checking; the finally nulling a new turn's controller - agentFlow's finally clobbering the next turn - the epoch captured after the controller is installed - the docstring dropping the legacy marker Caught while wiring it: handleSend referenced `epoch` without capturing it — a ReferenceError at runtime that `node --check` cannot see, because it only checks syntax. 23 tests in chatSurface, 34 suites green. --- extensions/levelcode-ai/extension.js | 37 +++++++++++-- .../levelcode-ai/test/chatSurface.test.js | 53 +++++++++++++++++++ 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index d9e4ab5..9929371 100644 --- a/extensions/levelcode-ai/extension.js +++ b/extensions/levelcode-ai/extension.js @@ -63,6 +63,10 @@ let pendingTranscriptReplay = ''; let sessionsWebview; // the Sessions sidebar webview (for pushing list refreshes after a History action) /** @type {{role:string,content:string}[]} */ let conversation = []; +// Bumped by every teardown. A turn captures it when it starts and checks it before writing anything +// back, so work still unwinding after the conversation was torn down cannot repopulate the state the +// teardown just cleared — nor clobber the turn that replaced it. +let conversationEpoch = 0; /** @type {string | null} */ let pendingContext = null; /** Files pinned as chat context (whole codebase-wide context). @type {{id:string,uri:vscode.Uri,name:string,rel:string}[]} */ @@ -1123,6 +1127,12 @@ function sealLiveSession(why) { * to re-render; the close path is tearing one down. */ function resetConversationState() { + // FIRST: anything already in flight is now stale, and must not write back. Without this the + // teardown loses a race it does not know it is in — see handleSend/agentFlow, both of which mutate + // this state from a catch/finally that runs long after abort() returns. + conversationEpoch++; + clearApprovals(); + clearQuestions(); conversation = []; agentMessages = []; checkpoints.length = 0; currentCheckpoint = null; // drop the per-turn restore stack @@ -1594,6 +1604,7 @@ async function agentFlow(text) { if (!req.ok) { post({ type: 'agentError', message: providerErrorMessage(req) }); post({ type: 'agentDone', reason: 'error' }); return; } post({ type: 'agentStart' }); + const epoch = conversationEpoch; // this turn belongs to the conversation as it is RIGHT NOW abort = new AbortController(); repairAgentMemory(); // Open a workspace checkpoint for this turn (before the goal is pushed) so the user can roll back here. @@ -1689,6 +1700,12 @@ async function agentFlow(text) { signal: abort.signal }); } finally { + // Everything below writes to state a teardown may already have replaced. runAgent holds + // `agentMessages` BY REFERENCE, so a teardown that rebinds the global leaves this run pushing + // into an orphaned array — and then `agentMessages.slice(sessTurnStart)` would slice the NEW, + // empty one with an index into the old, recording an empty turn against the wrong session. + // `abort` and `currentCheckpoint` are worse: they would clobber whatever turn came next. + if (epoch !== conversationEpoch) { return; } clearApprovals(); clearQuestions(); abort = null; @@ -1736,6 +1753,7 @@ async function handleSend(text) { post({ type: 'clearContext' }); post({ type: 'assistantStart' }); + const epoch = conversationEpoch; // this turn belongs to the conversation as it is RIGHT NOW abort = new AbortController(); let assistant = ''; const onDelta = (d) => { assistant += d; post({ type: 'assistantDelta', text: d }); }; @@ -1764,6 +1782,11 @@ async function handleSend(text) { conversation.push({ role: 'assistant', content: assistant }); post({ type: 'assistantDone' }); } catch (e) { + // Closing the tab mid-stream aborts the request, which lands HERE — after resetConversationState + // has already cleared `conversation`. Pushing the partial reply back in would leave a dangling + // assistant turn with no user turn in front of it, and the next send would ship it to the model: + // exactly the leak the teardown exists to prevent, reintroduced by the teardown's own abort. + if (epoch !== conversationEpoch) { return; } if (abort && abort.signal.aborted) { if (assistant) { conversation.push({ role: 'assistant', content: assistant }); } post({ type: 'assistantDone' }); @@ -1772,7 +1795,9 @@ async function handleSend(text) { post({ type: 'assistantError', message: String((e && e.message) || e), code: e && e.code }); } } finally { - abort = null; + // Only if this turn still owns it. A new turn may already have installed its own controller, and + // nulling that one would leave it unstoppable. + if (epoch === conversationEpoch) { abort = null; } } } @@ -2369,10 +2394,12 @@ async function openChatInEditor(opts) { /** * Where the chat opens when the window does. * - * The default is the EDITOR: the chat is the thing most sessions are actually about, and a centred - * column is where the reference puts it. `secondarySidebar` is the old behaviour, kept because the - * sidebar is the right answer when you want the chat beside code rather than instead of it, and - * `none` is the honest opt-out for anyone who would rather open it themselves. + * Two supported values. `editor` (the default) opens the chat as a centred editor tab — the only + * surface it has; `none` is the honest opt-out for anyone who would rather open it themselves. + * + * `secondarySidebar` is LEGACY. It was valid until the chat became editor-only and is still sitting in + * real settings.json files, so it is mapped to `editor` rather than left to the unknown-value path — + * same result, but the debug log then names the surface we actually opened. * * Unknown values fall back to the default rather than throwing: this is read at startup, and a typo * in settings.json should not be able to leave a window with no chat and no explanation. diff --git a/extensions/levelcode-ai/test/chatSurface.test.js b/extensions/levelcode-ai/test/chatSurface.test.js index 931a81f..741e6bb 100644 --- a/extensions/levelcode-ai/test/chatSurface.test.js +++ b/extensions/levelcode-ai/test/chatSurface.test.js @@ -401,4 +401,57 @@ test('START: a legacy secondarySidebar setting maps to the editor, and says so', assert.strictEqual(run('nonsense'), 'editor', 'an unknown value must fall back to the default'); }); +test('CLOSE: work still unwinding cannot repopulate the state the teardown just cleared', () => { + // Review found the teardown losing a race it did not know it was in. Closing mid-stream aborts the + // request, and the abort lands in handleSend's catch AFTER resetConversationState has cleared + // `conversation` — where it pushed the partial reply straight back in. The result was a dangling + // assistant turn with no user turn in front of it, shipped to the model on the next send: the exact + // leak the teardown exists to prevent, reintroduced by the teardown's own abort(). + // + // agentFlow's finally was worse. runAgent holds `agentMessages` BY REFERENCE, so a teardown that + // rebinds the global leaves the run pushing into an orphaned array — and then + // `agentMessages.slice(sessTurnStart)` slices the NEW empty one with an index into the old. + // `abort = null` and `currentCheckpoint = null` would clobber whatever turn came next. + const reset = fnBody(ext, 'resetConversationState'); + assert.match(reset, /conversationEpoch\+\+/, 'the teardown does not invalidate in-flight work'); + + // FIRST, before anything is cleared: an abort landing mid-teardown must already read as stale. + assert.ok(reset.indexOf('conversationEpoch++') < reset.indexOf('conversation = []'), + 'the epoch must be bumped before the state is cleared, or the race window survives the fix'); + + for (const fn of ['handleSend', 'agentFlow']) { + const body = fnBody(ext, fn); + const captured = body.indexOf('const epoch = conversationEpoch'); + assert.ok(captured >= 0, fn + ' never captures the epoch — it cannot tell if its turn is still current'); + assert.ok(captured < body.indexOf('abort = new AbortController()'), + fn + ' captures the epoch after installing its controller; capture it before the turn can be torn down'); + assert.match(body, /epoch !== conversationEpoch/, fn + ' writes back without checking it is still current'); + } + + // The guard has to come before the first write in the block it protects, or it guards nothing. + const send = fnBody(ext, 'handleSend'); + const tail = send.slice(send.lastIndexOf('} catch (e) {')); + assert.ok(tail.indexOf('epoch !== conversationEpoch') < tail.indexOf("conversation.push"), + 'handleSend pushes the partial reply before checking the turn is still current'); + assert.match(tail, /if \(epoch === conversationEpoch\) \{ abort = null; \}/, + 'the finally nulls `abort` unconditionally — that clobbers the controller of the turn that replaced this one'); + + const agent = fnBody(ext, 'agentFlow'); + const afin = agent.slice(agent.lastIndexOf('} finally {')); + assert.ok(afin.indexOf('epoch !== conversationEpoch') < afin.indexOf('abort = null'), + 'agentFlow clobbers abort/currentCheckpoint/recordTurn before checking the turn is still current'); +}); + +test('START: the docstring describes the values that actually exist', () => { + // It still called secondarySidebar a supported surface ("kept because the sidebar is the right + // answer when…") while the code below mapped it away as legacy. In-code documentation sitting + // directly on top of the change is the worst place to leave a contradiction. + const at = ext.indexOf('Where the chat opens when the window does'); + assert.ok(at > 0, 'the chatStartLocation docstring is gone'); + const doc = ext.slice(at, ext.indexOf('function chatStartLocation', at)); + assert.match(doc, /LEGACY/, 'the docstring does not mark secondarySidebar as legacy'); + assert.ok(!/kept because/.test(doc), 'the docstring still describes secondarySidebar as a supported surface'); + assert.match(doc, /`editor`[\s\S]*`none`/, 'the docstring should name the two values that are actually supported'); +}); + console.log('\nchatSurface: ' + n + ' tests passed.');