fix(chat): closing the tab tears the conversation down, not just seals it

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,24 +2386,32 @@ 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', {});
 	});
 }
 
 /**
  * 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. */

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