fix(tui): drop stale stream events after ctrl-c interrupt (#16706)

* fix(tui): drop stale stream events after ctrl-c interrupt

Once interruptTurn() flips this.interrupted, only recordMessageDelta
short-circuited.  recordReasoningDelta/Available, recordToolStart/
Progress/Complete, and recordInlineDiffToolComplete kept populating
turnState until the python loop reached its next _interrupt_requested
check (~1s on busy turns), making it look like ctrl-c was ignored
while late "thinking" + tool calls kept landing in the UI.

Add the same interrupted guard to every stream-side recorder, and
clear the flag at startMessage() so the next turn isn't suppressed
if the previous turn never delivered message.complete.

* fix(tui): guard recordTodos against post-interrupt mutation; fake-timers in test

Copilot review on PR #16706:

1. `recordToolStart` is interruption-guarded, but `tool.start`
   handler also calls `recordTodos(payload.todos)` first — so a
   late tool.start carrying todos could still mutate `turnState.todos`
   after Ctrl-C, leaving ghost rows in the panel.  Adds the same
   `if (this.interrupted) return` early-exit to `recordTodos` so
   *all* tool.start side-effects are dropped post-interrupt.

2. The interrupt test was leaking a real `setTimeout` (interrupt
   cooldown) across test files, which could fire later and mutate
   uiStore from the wrong test context.  Wraps the test in
   `vi.useFakeTimers()` + `vi.runAllTimers()` and restores real
   timers in finally.

3. Extends the same test with a todos payload on the post-interrupt
   tool.start so we have explicit regression coverage for #1.

* fix(tui): guard pushTrail post-interrupt; harden interrupt-test cleanup

Round 2 Copilot review on PR #16706:

1. `tool.generating` events route through `pushTrail`, which was not
   interruption-guarded — late events could still write 'drafting …'
   into `turnTrail` after Ctrl-C, leaving a stale shimmer in the UI.
   Adds the same `if (this.interrupted) return` early-exit.

2. Test cleanup moved `vi.runAllTimers()` into `finally` (before
   `vi.useRealTimers()`) so a mid-test assertion failure can't leak
   the interrupt-cooldown setTimeout across other test files.

3. Replaced the misleading 'pre-interrupt todos … expected to be
   cleared by the interrupt cycle' comment with an accurate one
   reflecting current behaviour (interrupt does NOT clear todos).

4. Added an explicit assertion that a post-interrupt `tool.generating`
   event does not extend `turnTrail` — regression coverage for #1.
This commit is contained in:
brooklyn! 2026-04-28 14:51:07 -07:00 committed by GitHub
parent a830f25f71
commit e42065b1f7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 114 additions and 6 deletions

View file

@ -495,4 +495,87 @@ describe('createGatewayEventHandler', () => {
expect(getTurnState().activity).toMatchObject([{ text: 'boom', tone: 'error' }])
})
it('drops stale reasoning/tool/todos events after ctrl-c until the next message starts', () => {
// Repro for the discord report: ctrl-c interrupts, but late reasoning/tool
// events from the still-winding-down agent loop kept populating the UI for
// ~1s, making it look like the interrupt had been ignored.
//
// Fake timers because `interruptTurn` schedules a real setTimeout for
// its cooldown — without flushing it inside this test, the timeout
// can fire later and mutate uiStore/turnState during unrelated tests
// (cross-file flake).
vi.useFakeTimers()
try {
const appended: Msg[] = []
const ctx = buildCtx(appended)
ctx.gateway.gw.request = vi.fn(async () => ({ status: 'interrupted' }))
const onEvent = createGatewayEventHandler(ctx)
patchUiState({ sid: 'sess-1' })
onEvent({ payload: {}, type: 'message.start' } as any)
onEvent({
payload: {
context: 'pre',
name: 'search',
todos: [{ content: 'pre-interrupt', id: 'todo-1', status: 'pending' }],
tool_id: 't-1'
},
type: 'tool.start'
} as any)
// Pre-interrupt todos should land in turn state.
expect(getTurnState().todos).toEqual([
{ content: 'pre-interrupt', id: 'todo-1', status: 'pending' }
])
turnController.interruptTurn({
appendMessage: (msg: Msg) => appended.push(msg),
gw: ctx.gateway.gw,
sid: 'sess-1',
sys: ctx.system.sys
})
onEvent({ payload: { text: 'still thinking…' }, type: 'reasoning.delta' } as any)
// Post-interrupt tool.start with a todos payload — must NOT mutate todos.
onEvent({
payload: {
context: 'post',
name: 'browser',
todos: [{ content: 'late ghost', id: 'todo-ghost', status: 'pending' }],
tool_id: 't-2'
},
type: 'tool.start'
} as any)
// Late tool.generating must NOT push a 'drafting …' line into the trail.
const trailBefore = getTurnState().turnTrail.length
onEvent({ payload: { name: 'browser' }, type: 'tool.generating' } as any)
expect(getTurnState().turnTrail.length).toBe(trailBefore)
onEvent({ payload: { name: 'browser', preview: 'loading' }, type: 'tool.progress' } as any)
onEvent({ payload: { summary: 'done', tool_id: 't-2' }, type: 'tool.complete' } as any)
onEvent({ payload: { text: 'late chunk' }, type: 'message.delta' } as any)
expect(getTurnState().tools).toEqual([])
expect(turnController.reasoningText).toBe('')
expect(turnController.bufRef).toBe('')
expect(getTurnState().streamPendingTools).toEqual([])
expect(getTurnState().streamSegments).toEqual([])
// Stale post-interrupt todos must not have leaked through.
// (This test does not assert that pre-interrupt todos are cleared —
// current interrupt path leaves them visible until the next message.)
expect(getTurnState().todos.find(t => t.content === 'late ghost')).toBeUndefined()
onEvent({ payload: {}, type: 'message.start' } as any)
onEvent({ payload: { text: 'fresh' }, type: 'reasoning.delta' } as any)
expect(turnController.reasoningText).toBe('fresh')
} finally {
// Drain pending fake timers BEFORE restoring real timers so a mid-
// test assertion failure can't leak the interrupt-cooldown setTimeout
// across test files (the original Copilot concern).
vi.runAllTimers()
vi.useRealTimers()
}
})
})