diff --git a/extensions/levelcode-ai/extension.js b/extensions/levelcode-ai/extension.js index 9ebd1e8..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}[]} */ @@ -1106,19 +1110,45 @@ 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() { + // 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 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 @@ -1574,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. @@ -1669,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; @@ -1716,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 }); }; @@ -1744,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' }); @@ -1752,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; } } } @@ -2341,6 +2386,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', {}); }); } @@ -2348,17 +2394,24 @@ 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. */ 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..741e6bb 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,110 @@ 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'); +}); + +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.');