fix(chat): a teardown invalidates in-flight work instead of racing it

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.

Author: ndemianc

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.

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