fix(desktop): stop finished turns from showing stale approval prompts
Two clear paths missed, both leaving a per-session approval entry parked after the turn ended — the floating "↓ needs approval" bar then reappeared on a session the sidebar already showed as finished, whenever scrolling unmounted the inline anchor (#86577): - session.reclaimed now clears the prompts keyed to the reclaimed runtime id. The runtime id rotates on every resume, so the NEW runtime's turn-end edges can never remove an entry keyed to the old one; a reopened conversation remounted the stale bar. - a running=false session.info for a session we knew was live (busy or awaitingResponse) now clears its prompts. The agent loop's finally block emits running=false even when a reconnect gap or crash swallowed message.complete — the only existing turn-end clear — so the terminal edge doubles as an authoritative prompt clear. Bystander sessions are untouched: both clears are scoped per session id. Tests: the reclaimed-runtime approval retires while a bystander keeps its prompt; a missed-complete turn retires its approval; an idle session's running=false heartbeat does not. Re-implemented on the split gateway-event modules from PR #86616 (author credited); the needsInput sidebar-dot half stays with the closed sibling #86565. Fixes #86577 Salvaged from a fix by fangliquanflq (GitHub account since removed).
This commit is contained in:
committed by
brooklyn!
parent
01f468a297
commit
bda04a699a
@@ -12,6 +12,7 @@ import {
|
||||
type PetChangeMeta,
|
||||
setChangeEventsAvailable
|
||||
} from '@/store/live-sync'
|
||||
import { clearAllPrompts, clearClarifyRequest } from '@/store/prompts'
|
||||
import { markRuntimeGone } from '@/store/runtime-gone'
|
||||
import { dropSessionState, unbindTileRuntime } from '@/store/session-states'
|
||||
// Leaf import (not the `@/themes` barrel) to avoid pulling the ThemeProvider
|
||||
@@ -105,6 +106,13 @@ export function handleLifecycleEvent(ctx: GatewayEventContext): boolean {
|
||||
// Heal while the cached stored-id mapping is still intact, then drop.
|
||||
markRuntimeGone(reclaimedRuntimeId)
|
||||
dropSessionState(reclaimedRuntimeId)
|
||||
// A prompt keyed to the dead runtime must not outlive it. The runtime id
|
||||
// rotates on every resume (cold/lazy/eager all mint a fresh sid), so the
|
||||
// new runtime's turn-end clears can never remove an entry keyed to THIS
|
||||
// one — a stale approval would re-mount the floating "needs approval"
|
||||
// bar whenever the reclaimed conversation is reopened (#86577).
|
||||
clearAllPrompts(reclaimedRuntimeId)
|
||||
clearClarifyRequest(undefined, reclaimedRuntimeId)
|
||||
// A tile bound to the reclaimed runtime would otherwise render an
|
||||
// empty transcript forever: its view reads $sessionStates[runtime]
|
||||
// (just dropped) and its resume effect is gated on !runtimeId, so a
|
||||
|
||||
@@ -4,6 +4,7 @@ import { reconcileApprovalModeForProfile } from '@/store/approval-mode'
|
||||
import { reconcileSessionCompacting } from '@/store/compaction'
|
||||
import { requestDesktopOnboardingForCredentialWarning } from '@/store/onboarding'
|
||||
import { followActiveSessionCwd } from '@/store/projects'
|
||||
import { clearAllPrompts, clearClarifyRequest } from '@/store/prompts'
|
||||
import {
|
||||
$activeSessionId,
|
||||
$currentCwd,
|
||||
@@ -284,6 +285,18 @@ export function handleSessionInfoEvent(ctx: GatewayEventContext): boolean {
|
||||
// mutates the per-runtime cache entry, and syncSessionStateToView
|
||||
// guards the view publish to the active session, so this is safe.
|
||||
if (runningChanged && sessionId) {
|
||||
// The agent loop's finally block emits running=false even when a
|
||||
// reconnect gap or provider crash swallowed message.complete — and
|
||||
// message.complete is where the turn-end prompt clear lives. An
|
||||
// approval left parked by that miss re-mounts the floating "needs
|
||||
// approval" bar on a session whose turn is already finished, so treat
|
||||
// the end of a turn we knew was live as an authoritative clear edge
|
||||
// too (#86577). Bystander sessions keep their prompts: the clear is
|
||||
// scoped to this sessionId.
|
||||
if (!payload!.running && (knownState?.busy || knownState?.awaitingResponse)) {
|
||||
clearAllPrompts(sessionId)
|
||||
clearClarifyRequest(undefined, sessionId)
|
||||
}
|
||||
// Set when THIS event releases a confirmed live turn whose terminal
|
||||
// message never arrived. The updater is invoked exactly once,
|
||||
// synchronously, by updateSessionState.
|
||||
|
||||
@@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { ClientSessionState } from '@/app/types'
|
||||
import { createClientSessionState } from '@/lib/chat-runtime'
|
||||
import { clearAllPrompts, sessionApprovalRequest, setApprovalRequest } from '@/store/prompts'
|
||||
import { resetRuntimeGoneHealing } from '@/store/runtime-gone'
|
||||
import { $activeSessionId, $sessionResumeRequest } from '@/store/session'
|
||||
import { $sessionStates, $sessionTiles, publishSessionState } from '@/store/session-states'
|
||||
@@ -43,6 +44,7 @@ beforeEach(() => {
|
||||
$sessionTiles.set([])
|
||||
$activeSessionId.set(null)
|
||||
$sessionResumeRequest.set(null)
|
||||
clearAllPrompts()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
@@ -52,6 +54,7 @@ afterEach(() => {
|
||||
$sessionTiles.set([])
|
||||
$activeSessionId.set(null)
|
||||
$sessionResumeRequest.set(null)
|
||||
clearAllPrompts()
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
@@ -69,6 +72,21 @@ describe('session.reclaimed', () => {
|
||||
expect($sessionStates.get()['live-kept']).toBeDefined()
|
||||
})
|
||||
|
||||
// The runtime id rotates on every resume, so a prompt keyed to the reclaimed
|
||||
// runtime can never be cleared by the NEW runtime's turn-end edges. Left
|
||||
// behind, it re-mounts the floating "needs approval" bar on a finished
|
||||
// conversation whenever it is reopened (#86577).
|
||||
it('retires only the reclaimed runtime approval', () => {
|
||||
mountStream()
|
||||
setApprovalRequest({ command: 'rm stale', description: 'stale request', sessionId: 'live-gone' })
|
||||
setApprovalRequest({ command: 'rm kept', description: 'kept request', sessionId: 'live-kept' })
|
||||
|
||||
reclaim('live-gone')
|
||||
|
||||
expect(sessionApprovalRequest('live-gone').get()).toBeNull()
|
||||
expect(sessionApprovalRequest('live-kept').get()?.command).toBe('rm kept')
|
||||
})
|
||||
|
||||
it('ignores a payload with no runtime id instead of clearing everything', () => {
|
||||
mountStream()
|
||||
publishSessionState('live-a', createClientSessionState())
|
||||
|
||||
@@ -6,12 +6,15 @@ import type { GatewayEvent } from '@hermes/shared'
|
||||
// running=false is the turn's finally-block signal and the only settle edge
|
||||
// those paths still emit, so it must finalize the bubble.
|
||||
import { act, cleanup } from '@testing-library/react'
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { clearAllPrompts, sessionApprovalRequest, setApprovalRequest } from '@/store/prompts'
|
||||
|
||||
import { type MessageStreamHarness, renderMessageStream } from './test-harness'
|
||||
import { STREAM_DELTA_FLUSH_MS } from './utils'
|
||||
|
||||
const SID = 'stale-pending-session'
|
||||
const OTHER_SID = 'other-session'
|
||||
|
||||
let stream: MessageStreamHarness
|
||||
|
||||
@@ -32,8 +35,13 @@ const flushDeltas = async () => {
|
||||
const emit = (event: GatewayEvent) => act(() => stream.handleEvent(event))
|
||||
|
||||
describe('turn end without message.complete (session.info running=false)', () => {
|
||||
beforeEach(() => {
|
||||
clearAllPrompts()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
cleanup()
|
||||
clearAllPrompts()
|
||||
vi.useRealTimers()
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
@@ -84,4 +92,49 @@ describe('turn end without message.complete (session.info running=false)', () =>
|
||||
expect(state?.messages.every(message => !message.pending)).toBe(true)
|
||||
expect(state?.streamId).toBeNull()
|
||||
})
|
||||
|
||||
// A turn whose message.complete was swallowed (reconnect gap, provider
|
||||
// crash) used to leave its approval entry parked: the floating "needs
|
||||
// approval" bar kept reappearing on a session the sidebar already showed
|
||||
// as finished. running=false is the agent loop's finally-block edge — it
|
||||
// must clear the turn's prompts just like message.complete does (#86577).
|
||||
it('retires the finished session approval when message.complete was missed', async () => {
|
||||
await mountHarness()
|
||||
|
||||
emit({ session_id: SID, type: 'message.start', payload: {} })
|
||||
await act(async () => {
|
||||
stream.handleRequest('approval', {
|
||||
command: 'rm -rf stale',
|
||||
description: 'stale request',
|
||||
session_id: SID
|
||||
})
|
||||
})
|
||||
setApprovalRequest({ command: 'rm other', description: 'other request', sessionId: OTHER_SID })
|
||||
|
||||
expect(sessionApprovalRequest(SID).get()?.command).toBe('rm -rf stale')
|
||||
|
||||
emit({ payload: { running: false }, session_id: SID, type: 'session.info' })
|
||||
|
||||
expect(sessionApprovalRequest(SID).get()).toBeNull()
|
||||
// Bystander sessions keep their prompts: only the finished turn clears.
|
||||
expect(sessionApprovalRequest(OTHER_SID).get()?.command).toBe('rm other')
|
||||
})
|
||||
|
||||
// An idle session's running=false heartbeat carries no turn edge, so it
|
||||
// must not retire a prompt another session's turn just raised.
|
||||
it('does not clear prompts on a running=false heartbeat for a session that was never busy', async () => {
|
||||
await mountHarness()
|
||||
|
||||
await act(async () => {
|
||||
stream.handleRequest('approval', {
|
||||
command: 'tail -f log',
|
||||
description: 'live request',
|
||||
session_id: SID
|
||||
})
|
||||
})
|
||||
|
||||
emit({ payload: { running: false }, session_id: SID, type: 'session.info' })
|
||||
|
||||
expect(sessionApprovalRequest(SID).get()?.command).toBe('tail -f log')
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user