@@ -218,7 +218,7 @@ test('START: the chat opens centred by default, and the setting is the only plac
218218
219219 // One reader, so a second caller cannot quietly disagree about what an unknown value means.
220220 const body = fnBody ( ext , 'chatStartLocation' ) ;
221- assert . match ( body , / ' e d i t o r ' , ' s e c o n d a r y S i d e b a r ' , ' n o n e ' / , 'the reader no longer validates against the enum' ) ;
221+ assert . match ( body , / ' e d i t o r ' , ' n o n e ' / , 'the reader must validate against exactly the enum the package ships ' ) ;
222222 assert . match ( body , / : ' e d i t o r ' / , 'an unknown value must fall back to the default, not leave the window with no chat' ) ;
223223} ) ;
224224
@@ -348,4 +348,110 @@ test('CLOSE: nothing resurrects the chat on the right', () => {
348348 'Sessions must KEEP its view — it is the right-hand container\'s reason to exist' ) ;
349349} ) ;
350350
351+ test ( 'CLOSE: the conversation is torn down, not just sealed' , ( ) => {
352+ // Review caught the two halves disagreeing. sealLiveSession ends the SESSION — liveId() goes null,
353+ // so the next chat opens visually empty — while `conversation` and `agentMessages` still held every
354+ // previous turn, so the next message shipped the old history to the model anyway. An empty-looking
355+ // chat that secretly remembers is worse than either honest option.
356+ const dispose = ext . slice ( ext . indexOf ( 'panel.onDidDispose' ) , ext . indexOf ( 'panel.onDidDispose' ) + 1400 ) ;
357+ assert . match ( dispose , / s e a l L i v e S e s s i o n \( ' c h a t C l o s e d ' \) / , 'closing no longer seals the session' ) ;
358+ assert . match ( dispose , / r e s e t C o n v e r s a t i o n S t a t e \( \) / ,
359+ 'closing seals but leaves conversation/agentMessages loaded — the next send replays the old history' ) ;
360+
361+ // ONE teardown, shared with New Chat, or the close path drifts — and it is the path nobody watches.
362+ assert . match ( fnBody ( ext , 'newChat' ) , / r e s e t C o n v e r s a t i o n S t a t e \( \) / ,
363+ 'New Chat has its own copy of the teardown again' ) ;
364+ const reset = fnBody ( ext , 'resetConversationState' ) ;
365+ for ( const [ frag , why ] of [
366+ [ 'conversation = []' , 'the model history survives the close' ] ,
367+ [ 'agentMessages = []' , 'the agent history survives the close' ] ,
368+ [ 'checkpoints.length = 0' , 'the restore stack still points at a finished turn' ] ,
369+ [ 'contextFiles = []' , 'stale attachments carry into the next chat' ] ,
370+ [ 'abort.abort()' , 'an in-flight run keeps going with no surface to report to' ] ,
371+ [ 'reapCommands()' , 'background commands outlive the chat — they are detached children' ] ,
372+ [ 'reapMcp()' , 'MCP servers outlive the chat — they are detached children too' ]
373+ ] ) {
374+ assert . ok ( reset . includes ( frag ) , why + ' (missing: ' + frag + ')' ) ;
375+ }
376+
377+ // It must NOT post: the close path is tearing the surface down, and New Chat re-renders itself.
378+ assert . ok ( ! / \b p o s t \( / . test ( reset ) ,
379+ 'resetConversationState posts to the webview — on the close path that webview is being disposed' ) ;
380+ } ) ;
381+
382+ test ( 'START: a legacy secondarySidebar setting maps to the editor, and says so' , ( ) => {
383+ // The value was valid until the chat became editor-only, so it is still sitting in real
384+ // settings.json files. Left to fall through the unknown-value path it produced the right BEHAVIOUR
385+ // with a lying debug log — `where: secondarySidebar` while opening the editor tab.
386+ const body = fnBody ( ext , 'chatStartLocation' ) ;
387+ assert . match ( body , / r a w = = = ' s e c o n d a r y S i d e b a r ' / , 'the legacy value is not mapped explicitly' ) ;
388+ assert . ok ( ! / \[ ' e d i t o r ' , ' s e c o n d a r y S i d e b a r ' , ' n o n e ' \] / . test ( body ) ,
389+ 'secondarySidebar is still an accepted value — it names a surface that no longer exists' ) ;
390+ assert . match ( body , / \[ ' e d i t o r ' , ' n o n e ' \] / , 'the accepted set should be exactly what the enum ships' ) ;
391+
392+ // Behaviour, evaluated from the SHIPPED source rather than a copy of it: fnBody hands back the
393+ // braces, so wrapping it in a declaration gives the real function with aiConfig injected.
394+ // aiConfig is CALLED and returns the config object, so the stub has to be a function that returns
395+ // one — passing the object itself is the obvious thing and it is wrong.
396+ const run = ( v ) => new Function ( 'aiConfig' ,
397+ 'function chatStartLocation() ' + body + '\nreturn chatStartLocation();' ) ( ( ) => ( { get : ( ) => v } ) ) ;
398+ assert . strictEqual ( run ( 'secondarySidebar' ) , 'editor' , 'a legacy setting must resolve to the editor' ) ;
399+ assert . strictEqual ( run ( 'none' ) , 'none' , 'the opt-out must survive' ) ;
400+ assert . strictEqual ( run ( 'editor' ) , 'editor' ) ;
401+ assert . strictEqual ( run ( 'nonsense' ) , 'editor' , 'an unknown value must fall back to the default' ) ;
402+ } ) ;
403+
404+ test ( 'CLOSE: work still unwinding cannot repopulate the state the teardown just cleared' , ( ) => {
405+ // Review found the teardown losing a race it did not know it was in. Closing mid-stream aborts the
406+ // request, and the abort lands in handleSend's catch AFTER resetConversationState has cleared
407+ // `conversation` — where it pushed the partial reply straight back in. The result was a dangling
408+ // assistant turn with no user turn in front of it, shipped to the model on the next send: the exact
409+ // leak the teardown exists to prevent, reintroduced by the teardown's own abort().
410+ //
411+ // agentFlow's finally was worse. runAgent holds `agentMessages` BY REFERENCE, so a teardown that
412+ // rebinds the global leaves the run pushing into an orphaned array — and then
413+ // `agentMessages.slice(sessTurnStart)` slices the NEW empty one with an index into the old.
414+ // `abort = null` and `currentCheckpoint = null` would clobber whatever turn came next.
415+ const reset = fnBody ( ext , 'resetConversationState' ) ;
416+ assert . match ( reset , / c o n v e r s a t i o n E p o c h \+ \+ / , 'the teardown does not invalidate in-flight work' ) ;
417+
418+ // FIRST, before anything is cleared: an abort landing mid-teardown must already read as stale.
419+ assert . ok ( reset . indexOf ( 'conversationEpoch++' ) < reset . indexOf ( 'conversation = []' ) ,
420+ 'the epoch must be bumped before the state is cleared, or the race window survives the fix' ) ;
421+
422+ for ( const fn of [ 'handleSend' , 'agentFlow' ] ) {
423+ const body = fnBody ( ext , fn ) ;
424+ const captured = body . indexOf ( 'const epoch = conversationEpoch' ) ;
425+ assert . ok ( captured >= 0 , fn + ' never captures the epoch — it cannot tell if its turn is still current' ) ;
426+ assert . ok ( captured < body . indexOf ( 'abort = new AbortController()' ) ,
427+ fn + ' captures the epoch after installing its controller; capture it before the turn can be torn down' ) ;
428+ assert . match ( body , / e p o c h ! = = c o n v e r s a t i o n E p o c h / , fn + ' writes back without checking it is still current' ) ;
429+ }
430+
431+ // The guard has to come before the first write in the block it protects, or it guards nothing.
432+ const send = fnBody ( ext , 'handleSend' ) ;
433+ const tail = send . slice ( send . lastIndexOf ( '} catch (e) {' ) ) ;
434+ assert . ok ( tail . indexOf ( 'epoch !== conversationEpoch' ) < tail . indexOf ( "conversation.push" ) ,
435+ 'handleSend pushes the partial reply before checking the turn is still current' ) ;
436+ assert . match ( tail , / i f \( e p o c h = = = c o n v e r s a t i o n E p o c h \) \{ a b o r t = n u l l ; \} / ,
437+ 'the finally nulls `abort` unconditionally — that clobbers the controller of the turn that replaced this one' ) ;
438+
439+ const agent = fnBody ( ext , 'agentFlow' ) ;
440+ const afin = agent . slice ( agent . lastIndexOf ( '} finally {' ) ) ;
441+ assert . ok ( afin . indexOf ( 'epoch !== conversationEpoch' ) < afin . indexOf ( 'abort = null' ) ,
442+ 'agentFlow clobbers abort/currentCheckpoint/recordTurn before checking the turn is still current' ) ;
443+ } ) ;
444+
445+ test ( 'START: the docstring describes the values that actually exist' , ( ) => {
446+ // It still called secondarySidebar a supported surface ("kept because the sidebar is the right
447+ // answer when…") while the code below mapped it away as legacy. In-code documentation sitting
448+ // directly on top of the change is the worst place to leave a contradiction.
449+ const at = ext . indexOf ( 'Where the chat opens when the window does' ) ;
450+ assert . ok ( at > 0 , 'the chatStartLocation docstring is gone' ) ;
451+ const doc = ext . slice ( at , ext . indexOf ( 'function chatStartLocation' , at ) ) ;
452+ assert . match ( doc , / L E G A C Y / , 'the docstring does not mark secondarySidebar as legacy' ) ;
453+ assert . ok ( ! / k e p t b e c a u s e / . test ( doc ) , 'the docstring still describes secondarySidebar as a supported surface' ) ;
454+ assert . match ( doc , / ` e d i t o r ` [ \s \S ] * ` n o n e ` / , 'the docstring should name the two values that are actually supported' ) ;
455+ } ) ;
456+
351457console . log ( '\nchatSurface: ' + n + ' tests passed.' ) ;
0 commit comments