diff --git a/e2e/path-security.test.ts b/e2e/path-security.test.ts index 2d1b7444..a8f3ea1a 100644 --- a/e2e/path-security.test.ts +++ b/e2e/path-security.test.ts @@ -27,11 +27,12 @@ import { setSessionMode, setSessionDangerLevel, answerPathConfirmation, + stopSessionChat, type TestClient, type TestProject, type TestServerHandle, } from './utils/index.js' -import type { ServerMessage } from '@openfox/shared/protocol' +import type { ServerMessage, SessionStatePayload } from '@openfox/shared/protocol' // Type for path confirmation payload interface PathConfirmationPayload { callId: string @@ -499,4 +500,68 @@ describe('Path Security', () => { expect(confirmations.length).toBe(0) }) }) + + describe('Stopping While Waiting for a Path Confirmation', () => { + it('closes out the pending confirmation and clears the waiting state (no resurrection on reload)', async () => { + client.clearEvents() + + // /home/test is outside the workdir (and outside /tmp) → confirmation. + // The write never completes: the query is stopped while it waits. + await client.send('chat.send', { + content: 'Write to /home/test/denied.txt with content "denied"', + }) + + const confirmation = await client.waitFor('chat.path_confirmation', undefined, 10000) + const callId = (confirmation.payload as PathConfirmationPayload).callId + const session = client.getSession()! + + client.clearEvents() + await stopSessionChat(server.url, session.id) + + // The final session.state broadcast must carry the clean state: not + // running and no pending confirmations. The client derives + // "Waiting for input" and the Allow/Deny buttons from exactly these + // fields, so both must clear. + const finalState = await client.waitFor( + 'session.state', + (p) => { + const payload = p as SessionStatePayload + return payload.session.isRunning === false && payload.pendingConfirmations.length === 0 + }, + 15000, + ) + const state = finalState.payload as SessionStatePayload + expect(state.session.isRunning).toBe(false) + expect(state.pendingConfirmations).toEqual([]) + expect(state.pendingQuestions ?? []).toEqual([]) + + // The resolution is also announced so other same-project clients clear + // their home-page waiting dot for this session. + const resolved = await client.waitFor( + 'session.confirmation_resolved', + (p) => (p as { callId: string }).callId === callId, + 10000, + ) + expect((resolved.payload as { callId: string }).callId).toBe(callId) + + // The REST status projection behind the "Waiting for input" tooltip + // must no longer report waiting for user input. + const status = (await fetch(`${server.url}/api/sessions/${session.id}/status`).then((r) => r.json())) as { + state: string + waitingForUser: boolean + } + expect(status.waitingForUser).toBe(false) + expect(status.state).not.toBe('waiting') + + // Reload parity: a full session refetch must not resurrect the + // cancelled confirmation either. + const reloaded = (await fetch(`${server.url}/api/sessions/${session.id}`).then((r) => r.json())) as { + session: { isRunning: boolean } + pendingConfirmations?: Array<{ callId: string }> + } + expect(reloaded.session.isRunning).toBe(false) + expect(reloaded.pendingConfirmations ?? []).toEqual([]) + expect(callId).not.toBe('') + }) + }) }) diff --git a/src/server/index.ts b/src/server/index.ts index 72bec196..c31418ea 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -1046,14 +1046,12 @@ export async function createServerHandle(config: Config): Promise } // Cancel any active execution before deleting — mirrors /stop endpoint - const { stopSessionExecution } = await import('./session/chat-handler.js') - const { cancelQuestionsForSession, cancelPathConfirmationsForSession } = await import('./tools/index.js') + const { stopSessionExecution, cancelSessionInteractions } = await import('./session/chat-handler.js') sessionManager.clearMessageQueue(sessionId) stopSessionExecution(sessionId, sessionManager) abortSession(sessionId) - cancelQuestionsForSession(sessionId, 'Session deleted') - cancelPathConfirmationsForSession(sessionId, 'Session deleted') + await cancelSessionInteractions(sessionId, sessionManager, 'Session deleted') sessionManager.deleteSession(sessionId) wssExports.broadcastAll({ @@ -1648,8 +1646,7 @@ export async function createServerHandle(config: Config): Promise return res.status(404).json({ error: 'Session not found' }) } - const { stopSessionExecution } = await import('./session/chat-handler.js') - const { cancelQuestionsForSession, cancelPathConfirmationsForSession } = await import('./tools/index.js') + const { stopSessionExecution, cancelSessionInteractions } = await import('./session/chat-handler.js') // Drain queued messages BEFORE stopping execution, so the QueueProcessor // doesn't pick them up when running_changed fires from setRunning(false) @@ -1660,11 +1657,18 @@ export async function createServerHandle(config: Config): Promise stopSessionExecution(sessionId, sessionManager) abortSession(sessionId) - cancelQuestionsForSession(sessionId, 'Session stopped by user') - cancelPathConfirmationsForSession(sessionId, 'Session stopped by user') + const cancelledCallIds = await cancelSessionInteractions(sessionId, sessionManager, 'Session stopped by user') - const eventStore = (await import('./events/index.js')).getEventStore() - eventStore.append(sessionId, { type: 'running.changed', data: { isRunning: false } }) + // Mirror /confirm-path: tell every same-project client the confirmation + // is resolved so its home-page "waiting for input" dot clears (the + // session-scoped session.state above only reaches the session's own clients). + for (const callId of cancelledCallIds) { + wssExports.broadcastForSession(sessionId, { + type: 'session.confirmation_resolved', + sessionId, + payload: { sessionId, callId }, + }) + } res.json({ success: true, queuedMessages }) }) @@ -3764,14 +3768,18 @@ export async function createServerHandle(config: Config): Promise launchWorkflow: (sessionId, launch) => deferTasksLaunchWorkflow(sessionId, launch), stopSession: (sessionId) => { void (async () => { - const { stopSessionExecution } = await import('./session/chat-handler.js') - const { cancelQuestionsForSession, cancelPathConfirmationsForSession } = await import('./tools/index.js') + const { stopSessionExecution, cancelSessionInteractions } = await import('./session/chat-handler.js') sessionManager.clearMessageQueue(sessionId) stopSessionExecution(sessionId, sessionManager) abortSession(sessionId) - cancelQuestionsForSession(sessionId, 'Session stopped by user') - cancelPathConfirmationsForSession(sessionId, 'Session stopped by user') - getEventStore().append(sessionId, { type: 'running.changed', data: { isRunning: false } }) + const cancelledCallIds = await cancelSessionInteractions(sessionId, sessionManager, 'Session stopped by user') + for (const callId of cancelledCallIds) { + wssExports.broadcastForSession(sessionId, { + type: 'session.confirmation_resolved', + sessionId, + payload: { sessionId, callId }, + }) + } })().catch((error) => { logger.error(`MCP stopSession failed for ${sessionId}`, { error: error instanceof Error ? error.message : String(error), diff --git a/src/server/session/chat-handler.test.ts b/src/server/session/chat-handler.test.ts new file mode 100644 index 00000000..a65abd46 --- /dev/null +++ b/src/server/session/chat-handler.test.ts @@ -0,0 +1,179 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { mkdir, rm } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { initDatabase, closeDatabase, getDatabase } from '../db/index.js' +import { initEventStore, getEventStore } from '../events/index.js' +import { foldPendingConfirmations, foldIsRunning } from '../events/folding.js' +import { SessionManager, type SessionEvent } from './manager.js' +import { createProject } from '../db/projects.js' +import { projectSessionStatus } from '../routes/session-status.js' +import { + AskUserInterrupt, + askUserTool, + cancelQuestionsForSession, + getPendingQuestionsForSession, +} from '../tools/ask.js' +import { hasPendingPathConfirmation, requestPathAccess } from '../tools/path-security.js' +import type { ToolContext } from '../tools/types.js' +import type { Config } from '../../shared/types.js' +import { cancelSessionInteractions } from './chat-handler.js' + +// Default to a Unix shell so the path-extraction assumptions hold on any host. +vi.mock('../utils/platform.js', () => ({ + getPlatformShell: vi.fn(() => ({ command: '/bin/sh', args: ['-c'] })), +})) + +const REAL_PLATFORM = process.platform + +const mockProviderManager = { + getCurrentModelContext: () => 200000, +} + +function createTestConfig(): Config { + return { + llm: { baseUrl: 'http://localhost:8000/v1', model: 'test', timeout: 1000, idleTimeout: 30000, backend: 'vllm' }, + context: { maxTokens: 100000, compactionThreshold: 0.85, compactionTarget: 0.6 }, + agent: { maxIterations: 10, maxConsecutiveFailures: 3, toolTimeout: 1000 }, + server: { port: 3000, host: 'localhost' }, + database: { path: ':memory:' }, + workdir: process.cwd(), + } +} + +async function waitForPending(callId: string): Promise { + for (let attempt = 0; attempt < 200; attempt++) { + if (hasPendingPathConfirmation(callId)) return + await new Promise((resolve) => setTimeout(resolve, 5)) + } + throw new Error(`Confirmation ${callId} never went pending`) +} + +/** + * cancelSessionInteractions — the shared stop-tail used by the /stop endpoint, + * the MCP stopSession tool, and session deletion. + * + * Stopping a query while it waits on user interaction (Allow/Deny path + * confirmation or ask_user) must leave the session in a clean, converging + * state: no pending questions, no pending confirmations (closed out in the + * event log so they cannot resurrect on the next session.state broadcast or + * reload), a terminal running.changed, and a final session_updated emission so + * live clients re-derive the clean state. + */ +describe('cancelSessionInteractions', () => { + let testDir: string + let workdir: string + let sessionManager: SessionManager + let sessionId: string + let emitted: SessionEvent[] + let unsubscribe: () => void + + beforeEach(async () => { + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }) + closeDatabase() + initDatabase(createTestConfig()) + initEventStore(getDatabase()) + + testDir = join(tmpdir(), `openfox-stop-tail-${Date.now()}-${Math.random().toString(36).slice(2)}`) + await mkdir(testDir, { recursive: true }) + workdir = testDir + + sessionManager = new SessionManager(mockProviderManager as any) + const project = createProject('stop-tail-test', testDir) + sessionId = sessionManager.createSession(project.id).id + + emitted = [] + unsubscribe = sessionManager.subscribe((event) => emitted.push(event)) + }) + + afterEach(async () => { + unsubscribe() + Object.defineProperty(process, 'platform', { value: REAL_PLATFORM, configurable: true }) + closeDatabase() + await rm(testDir, { recursive: true, force: true }).catch(() => {}) + }) + + it('cancels questions and path confirmations, records the terminal state, and forces a final session_updated', async () => { + // One pending path confirmation (with its persisted pending event). + const onEvent = vi.fn() + const confirmationPromise = requestPathAccess( + ['/etc/passwd'], + workdir, + sessionId, + 'call-stop-1', + 'run_command', + onEvent, + 'normal', + 'cat /etc/passwd', + ) + await waitForPending('call-stop-1') + const rejectedConfirmation = expect(confirmationPromise).rejects.toThrow('Session stopped by user') + + // One pending ask_user question. + const context: ToolContext = { + workdir, + sessionId, + sessionManager, + toolCallId: 'ask-stop-1', + } + let interrupt: unknown = null + try { + await askUserTool.execute({ question: 'What should I do?' }, context) + } catch (err) { + interrupt = err + } + expect(interrupt).toBeInstanceOf(AskUserInterrupt) + expect(getPendingQuestionsForSession(sessionId)).toHaveLength(1) + + const cancelledCallIds = await cancelSessionInteractions(sessionId, sessionManager, 'Session stopped by user') + + // The cancelled path confirmation is reported back so the caller can + // broadcast a session.confirmation_resolved for it. + expect(cancelledCallIds).toEqual(['call-stop-1']) + + // In-memory interaction gates are cleared. + expect(getPendingQuestionsForSession(sessionId)).toEqual([]) + expect(hasPendingPathConfirmation('call-stop-1')).toBe(false) + await rejectedConfirmation + + // Event log: the confirmation is closed out (no resurrection on fold), + // and the terminal running state is recorded. + const events = getEventStore().getEvents(sessionId) + expect(foldPendingConfirmations(events)).toEqual([]) + expect(foldIsRunning(events)).toBe(false) + const runningChanged = events.filter((e) => e.type === 'running.changed') + expect(runningChanged.length).toBeGreaterThan(0) + expect(runningChanged[runningChanged.length - 1]!.data).toEqual({ isRunning: false }) + + // The final emission is a session_updated so the WS layer re-broadcasts + // session.state with the clean fold (the last broadcast converges). + const last = emitted[emitted.length - 1] + expect(last?.type).toBe('session_updated') + if (last?.type === 'session_updated') { + expect(last.session.id).toBe(sessionId) + } + + // The status projection (drives the "Waiting for input" tooltip) no + // longer reports waiting for user input. + const status = projectSessionStatus({ + session: sessionManager.getSession(sessionId)!, + pendingQuestionsCount: getPendingQuestionsForSession(sessionId).length, + pendingConfirmationsCount: foldPendingConfirmations(events).length, + activeWorkflowStepName: null, + }) + expect(status.state).not.toBe('waiting') + expect(status.waitingForUser).toBe(false) + }) + + it('is a no-op for a session with nothing pending and never throws', async () => { + expect(await cancelSessionInteractions(sessionId, sessionManager, 'Session stopped by user')).toEqual([]) + + const events = getEventStore().getEvents(sessionId) + expect(foldIsRunning(events)).toBe(false) + const runningChanged = events.filter((e) => e.type === 'running.changed') + expect(runningChanged[runningChanged.length - 1]!.data).toEqual({ isRunning: false }) + expect(getPendingQuestionsForSession(sessionId)).toEqual([]) + expect(cancelQuestionsForSession(sessionId, 'noop')).toBe(0) + expect(emitted.some((e) => e.type === 'session_updated')).toBe(true) + }) +}) diff --git a/src/server/session/chat-handler.ts b/src/server/session/chat-handler.ts index de37f899..d38dae53 100644 --- a/src/server/session/chat-handler.ts +++ b/src/server/session/chat-handler.ts @@ -175,3 +175,34 @@ export function stopSessionExecution(sessionId: string, sessionManager: SessionM sessionManager.clearPauseState(sessionId) sessionManager.setRunning(sessionId, false) } + +/** + * Cancel all pending user-interaction gates for a session (ask_user + * questions, path confirmations), record the terminal running state in the + * event log, and force a final session.state re-broadcast. + * + * The session.state broadcast triggered by setRunning(false) fires BEFORE the + * gates are cancelled, and cancelled path confirmations only leave the + * event-sourced fold once a path.confirmation_responded event closes them + * out. Without the explicit re-broadcast here, clients would converge on the + * stale "waiting for input" state — the Allow/Deny buttons and tooltip would + * stick after Stop. + * + * @returns the callIds of the cancelled path confirmations, so the caller can + * broadcast a session.confirmation_resolved for each (cross-delivered to + * other clients of the same project, clearing their home-page waiting dots). + */ +export async function cancelSessionInteractions( + sessionId: string, + sessionManager: SessionManager, + reason: string, +): Promise { + const { cancelQuestionsForSession, cancelPathConfirmationsForSession, getPendingPathConfirmationCallIds } = + await import('../tools/index.js') + const cancelledCallIds = getPendingPathConfirmationCallIds(sessionId) + cancelQuestionsForSession(sessionId, reason) + cancelPathConfirmationsForSession(sessionId, reason) + getEventStore().append(sessionId, { type: 'running.changed', data: { isRunning: false } }) + sessionManager.emitBranchChange(sessionId) + return cancelledCallIds +} diff --git a/src/server/tools/index.ts b/src/server/tools/index.ts index b94caf52..f6093952 100644 --- a/src/server/tools/index.ts +++ b/src/server/tools/index.ts @@ -443,6 +443,7 @@ export { PathAccessDeniedError, requestPathAccess, cancelPathConfirmationsForSession, + getPendingPathConfirmationCallIds, autoApprovePendingConfirmationsForSession, providePathConfirmation, getConfirmationSessionId, diff --git a/src/server/tools/path-security-cancellation.test.ts b/src/server/tools/path-security-cancellation.test.ts new file mode 100644 index 00000000..a113c21b --- /dev/null +++ b/src/server/tools/path-security-cancellation.test.ts @@ -0,0 +1,228 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { mkdir, rm } from 'node:fs/promises' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { initDatabase, closeDatabase, getDatabase } from '../db/index.js' +import { initEventStore, getEventStore } from '../events/index.js' +import { foldPendingConfirmations } from '../events/folding.js' +import { SessionManager } from '../session/manager.js' +import { createProject } from '../db/projects.js' +import type { Config } from '../../shared/types.js' +import { + requestPathAccess, + registerPathConfirmation, + providePathConfirmation, + cancelPathConfirmation, + cancelPathConfirmationsForSession, + hasPendingPathConfirmation, +} from './path-security.js' + +// Default to a Unix shell so the path-extraction assumptions hold on any host. +vi.mock('../utils/platform.js', () => ({ + getPlatformShell: vi.fn(() => ({ command: '/bin/sh', args: ['-c'] })), +})) + +const REAL_PLATFORM = process.platform + +const mockProviderManager = { + getCurrentModelContext: () => 200000, +} + +function createTestConfig(): Config { + return { + llm: { baseUrl: 'http://localhost:8000/v1', model: 'test', timeout: 1000, idleTimeout: 30000, backend: 'vllm' }, + context: { maxTokens: 100000, compactionThreshold: 0.85, compactionTarget: 0.6 }, + agent: { maxIterations: 10, maxConsecutiveFailures: 3, toolTimeout: 1000 }, + server: { port: 3000, host: 'localhost' }, + database: { path: ':memory:' }, + workdir: process.cwd(), + } +} + +async function waitForPending(callId: string): Promise { + for (let attempt = 0; attempt < 200; attempt++) { + if (hasPendingPathConfirmation(callId)) return + await new Promise((resolve) => setTimeout(resolve, 5)) + } + throw new Error(`Confirmation ${callId} never went pending`) +} + +function respondedEvents(sessionId: string) { + return getEventStore() + .getEvents(sessionId) + .filter((e) => e.type === 'path.confirmation_responded') +} + +/** + * Cancellation lifecycle for path confirmations. + * + * Pending confirmations are event-sourced: every session.state broadcast and + * session load re-derives them from the event log (foldPendingConfirmations). + * Cancelling a confirmation (e.g. when the user stops the query) therefore + * must close the event out with path.confirmation_responded, otherwise the + * cancelled confirmation resurrects on every re-broadcast and on reload — + * keeping "Waiting for input" and the Allow/Deny buttons stuck in the UI. + */ +describe('path confirmation cancellation lifecycle (event store)', () => { + let testDir: string + let workdir: string + let sessionManager: SessionManager + let sessionId: string + let otherSessionId: string + + beforeEach(async () => { + Object.defineProperty(process, 'platform', { value: 'linux', configurable: true }) + closeDatabase() + initDatabase(createTestConfig()) + initEventStore(getDatabase()) + + testDir = join(tmpdir(), `openfox-cancellation-${Date.now()}-${Math.random().toString(36).slice(2)}`) + await mkdir(testDir, { recursive: true }) + workdir = testDir + + sessionManager = new SessionManager(mockProviderManager as any) + const project = createProject('cancellation-test', testDir) + sessionId = sessionManager.createSession(project.id).id + otherSessionId = sessionManager.createSession(project.id).id + }) + + afterEach(async () => { + Object.defineProperty(process, 'platform', { value: REAL_PLATFORM, configurable: true }) + closeDatabase() + await rm(testDir, { recursive: true, force: true }).catch(() => {}) + }) + + describe('cancelPathConfirmation', () => { + it('writes a path.confirmation_responded event (approved:false) and the fold forgets the confirmation', async () => { + const onEvent = vi.fn() + const promise = requestPathAccess(['/etc/passwd'], workdir, sessionId, 'call-cancel', 'read_file', onEvent) + await waitForPending('call-cancel') + + expect(cancelPathConfirmation('call-cancel', 'Session stopped by user')).toBe(true) + const rejected = expect(promise).rejects.toThrow('Session stopped by user') + + const responded = respondedEvents(sessionId) + expect(responded).toHaveLength(1) + expect(responded[0]!.data).toEqual({ callId: 'call-cancel', approved: false, alwaysAllow: false }) + + // The event-sourced fold must no longer resurrect the confirmation. + expect(foldPendingConfirmations(getEventStore().getEvents(sessionId))).toEqual([]) + + await rejected + }) + }) + + describe('cancelPathConfirmationsForSession', () => { + it('writes one responded event per cancelled confirmation and leaves other sessions untouched', async () => { + const pendingA = registerPathConfirmation( + 'call-a', + ['/etc/a'], + sessionId, + 'read_file', + workdir, + 'outside_workdir', + ) + const pendingB = registerPathConfirmation( + 'call-b', + ['/etc/b'], + sessionId, + 'write_file', + workdir, + 'sensitive_file', + ) + const pendingOther = registerPathConfirmation( + 'call-other', + ['/etc/c'], + otherSessionId, + 'read_file', + workdir, + 'outside_workdir', + ) + const rejectedA = expect(pendingA).rejects.toThrow('Session stopped by user') + const rejectedB = expect(pendingB).rejects.toThrow('Session stopped by user') + const rejectedOther = expect(pendingOther).rejects.toThrow('cleanup') + + // Mirror what requestPathAccess persists for each pending confirmation. + getEventStore().append(sessionId, { + type: 'path.confirmation_pending', + data: { callId: 'call-a', tool: 'read_file', paths: ['/etc/a'], workdir, reason: 'outside_workdir' }, + }) + getEventStore().append(sessionId, { + type: 'path.confirmation_pending', + data: { callId: 'call-b', tool: 'write_file', paths: ['/etc/b'], workdir, reason: 'sensitive_file' }, + }) + getEventStore().append(otherSessionId, { + type: 'path.confirmation_pending', + data: { callId: 'call-other', tool: 'read_file', paths: ['/etc/c'], workdir, reason: 'outside_workdir' }, + }) + + expect(cancelPathConfirmationsForSession(sessionId, 'Session stopped by user')).toBe(2) + expect(hasPendingPathConfirmation('call-a')).toBe(false) + expect(hasPendingPathConfirmation('call-b')).toBe(false) + expect(hasPendingPathConfirmation('call-other')).toBe(true) + + const responded = respondedEvents(sessionId) + expect(responded.map((e) => (e.data as { callId: string }).callId).sort()).toEqual(['call-a', 'call-b']) + for (const e of responded) { + expect(e.data).toMatchObject({ approved: false, alwaysAllow: false }) + } + + // This session's fold is clean; the other session's confirmation stays. + expect(foldPendingConfirmations(getEventStore().getEvents(sessionId))).toEqual([]) + expect(foldPendingConfirmations(getEventStore().getEvents(otherSessionId))).toHaveLength(1) + + cancelPathConfirmation('call-other', 'cleanup') + await Promise.all([rejectedA, rejectedB, rejectedOther]) + }) + }) + + describe('stop-while-waiting (the reported bug)', () => { + it('a confirmation pending during requestPathAccess is closed out when the session stops', async () => { + const onEvent = vi.fn() + const promise = requestPathAccess( + ['/etc/passwd'], + workdir, + sessionId, + 'call-stop', + 'run_command', + onEvent, + 'normal', + 'cat /etc/passwd', + ) + await waitForPending('call-stop') + + // Before the fix, the pending event stayed unmatched in the log, so the + // fold (and therefore session.state / GET /api/sessions/:id) kept + // resurrecting the confirmation forever. + expect(foldPendingConfirmations(getEventStore().getEvents(sessionId))).toHaveLength(1) + + const cancelled = cancelPathConfirmationsForSession(sessionId, 'Session stopped by user') + const rejected = expect(promise).rejects.toThrow('Session stopped by user') + expect(cancelled).toBe(1) + expect(foldPendingConfirmations(getEventStore().getEvents(sessionId))).toEqual([]) + + await rejected + }) + }) + + describe('providePathConfirmation (refactor regression)', () => { + it('still writes the responded event with the approval flags from the response', async () => { + const onEvent = vi.fn() + const promise = requestPathAccess(['/etc/passwd'], workdir, sessionId, 'call-provide', 'read_file', onEvent) + await waitForPending('call-provide') + + expect(providePathConfirmation('call-provide', true, true)).toEqual({ + found: true, + sessionId, + approved: true, + }) + + const responded = respondedEvents(sessionId) + expect(responded).toHaveLength(1) + expect(responded[0]!.data).toEqual({ callId: 'call-provide', approved: true, alwaysAllow: true }) + expect(foldPendingConfirmations(getEventStore().getEvents(sessionId))).toEqual([]) + + await expect(promise).resolves.toBeUndefined() + }) + }) +}) diff --git a/src/server/tools/path-security.ts b/src/server/tools/path-security.ts index f4ad5424..93dca27e 100644 --- a/src/server/tools/path-security.ts +++ b/src/server/tools/path-security.ts @@ -1498,6 +1498,27 @@ const pendingConfirmations = new Map< } >() +/** + * Persist a path.confirmation_responded event closing out the pending + * confirmation. Pending confirmations are event-sourced (session.state and + * session loads re-derive them from the log), so every resolution path for a + * live session — approval, denial, cancellation, or the stale rejection at + * boot — must leave a responded event behind, or the confirmation resurrects + * on the next broadcast or reload. (Deleting a session is not a resolution + * path: it removes the event log wholesale, so there is nothing to resurrect.) + */ +function emitConfirmationResponded(sessionId: string, callId: string, approved: boolean, alwaysAllow: boolean): void { + try { + const eventStore = getEventStore() + eventStore.append(sessionId, { + type: 'path.confirmation_responded', + data: { callId, approved, alwaysAllow }, + }) + } catch { + // Event store might not be initialized in tests, continue without event + } +} + /** * Register a pending path confirmation. * Stores the paths and sessionId so they can be added to allowlist on approval. @@ -1541,15 +1562,7 @@ export function providePathConfirmation( } // Emit path.confirmation_responded event for persistence - try { - const eventStore = getEventStore() - eventStore.append(pending.sessionId, { - type: 'path.confirmation_responded', - data: { callId, approved, alwaysAllow: alwaysAllow ?? false }, - }) - } catch { - // Event store might not be initialized in tests, continue without event - } + emitConfirmationResponded(pending.sessionId, callId, approved, alwaysAllow ?? false) if (approved && alwaysAllow) { // Add real filesystem paths to the allowlist only when alwaysAllow is true. @@ -1579,11 +1592,28 @@ export function cancelPathConfirmation(callId: string, reason: string): boolean return false } + // Close the confirmation out in the event log before unwinding, so the + // event-sourced fold (session.state / session load) stops resurrecting it. + emitConfirmationResponded(pending.sessionId, callId, false, false) + pending.reject(new Error(reason)) pendingConfirmations.delete(callId) return true } +/** + * CallIds of the path confirmations currently pending for a session. + */ +export function getPendingPathConfirmationCallIds(sessionId: string): string[] { + const callIds: string[] = [] + for (const [callId, pending] of pendingConfirmations.entries()) { + if (pending.sessionId === sessionId) { + callIds.push(callId) + } + } + return callIds +} + export function cancelPathConfirmationsForSession(sessionId: string, reason: string): number { let cancelledCount = 0 @@ -1592,6 +1622,10 @@ export function cancelPathConfirmationsForSession(sessionId: string, reason: str continue } + // Close each confirmation out in the event log before unwinding, so the + // event-sourced fold (session.state / session load) stops resurrecting it. + emitConfirmationResponded(pending.sessionId, callId, false, false) + pending.reject(new Error(reason)) pendingConfirmations.delete(callId) cancelledCount += 1 diff --git a/web/src/components/shared/ThinkingSummary.test.tsx b/web/src/components/shared/ThinkingSummary.test.tsx index cf15f7d0..dd9f8b97 100644 --- a/web/src/components/shared/ThinkingSummary.test.tsx +++ b/web/src/components/shared/ThinkingSummary.test.tsx @@ -1,6 +1,7 @@ // @vitest-environment happy-dom import { afterEach, describe, expect, it, vi } from 'vitest' import { act, cleanup, render } from '@testing-library/react' +import { latchThinkingStart } from '../../lib/thinking-timing' import { ThinkingSummary } from './ThinkingSummary' afterEach(() => { @@ -21,6 +22,18 @@ describe('ThinkingSummary', () => { expect(container.textContent).toContain('(12s)') }) + it('starts the elapsed time from the thinking start, not the collapse time', () => { + vi.useFakeTimers() + vi.setSystemTime(100_000) + latchThinkingStart('m-late') + + // The block is collapsed 30s after thinking began + vi.setSystemTime(130_000) + const { container } = render() + + expect(container.textContent).toContain('Thinking… (30s)') + }) + it('ticks every 100ms while under ten seconds', () => { vi.useFakeTimers() vi.setSystemTime(100_000) diff --git a/web/src/components/shared/ThinkingSummary.tsx b/web/src/components/shared/ThinkingSummary.tsx index 43b01946..1b9e5bec 100644 --- a/web/src/components/shared/ThinkingSummary.tsx +++ b/web/src/components/shared/ThinkingSummary.tsx @@ -1,42 +1,7 @@ import { memo, useEffect, useState } from 'react' import { useT } from '../../hooks/useT' import { formatTime } from '../../lib/format-stats' - -interface ThinkingTimingEntry { - start: number - end?: number -} - -// Client-side thinking timing, keyed by message id. The start is latched when -// the indicator first appears during a live stream; the end is latched when the -// first non-thinking output arrives. This bridges the gap between the live -// "Thinking…" state and the authoritative server-measured duration that lands -// with the message stats at turn end (and survives page reloads). Keeping it at -// module scope lets the timing survive scroll-driven remounts within a session. -const thinkingTiming = new Map() -// Bound the map: entries for messages that never receive a server-measured -// duration (aborted turns, brief thinking, no stats) are never evicted on their -// own, so cap the total to keep memory bounded over long sessions. -const MAX_THINKING_TIMING_ENTRIES = 500 - -function ensureThinkingStart(messageId: string): number { - const existing = thinkingTiming.get(messageId) - if (existing) return existing.start - if (thinkingTiming.size >= MAX_THINKING_TIMING_ENTRIES) { - const oldest = thinkingTiming.keys().next().value - if (oldest !== undefined) thinkingTiming.delete(oldest) - } - const entry: ThinkingTimingEntry = { start: Date.now() } - thinkingTiming.set(messageId, entry) - return entry.start -} - -function latchThinkingEnd(messageId: string): number | undefined { - const entry = thinkingTiming.get(messageId) - if (!entry) return undefined - if (entry.end === undefined) entry.end = Date.now() - return (entry.end - entry.start) / 1000 -} +import { clearThinkingTiming, getThinkingStart, latchThinkingEnd, latchThinkingStart } from '../../lib/thinking-timing' interface ThinkingSummaryProps { messageId: string @@ -55,19 +20,19 @@ export const ThinkingSummary = memo(function ThinkingSummary({ const t = useT() const [now, setNow] = useState(() => Date.now()) const [clientDuration, setClientDuration] = useState() - const startedAt = thinkingTiming.get(messageId)?.start + const startedAt = getThinkingStart(messageId) // Sub-10s the timer shows tenths (e.g. "7.8s"), so it needs to tick every // 100ms; past 10s whole seconds are enough. const fastTicking = now - (startedAt ?? now) < 10_000 useEffect(() => { if (thinkingDuration !== undefined) { - thinkingTiming.delete(messageId) + clearThinkingTiming(messageId) setClientDuration(undefined) return } if (isStreaming && !thinkingFinished) { - ensureThinkingStart(messageId) + latchThinkingStart(messageId) const timer = setInterval(() => setNow(Date.now()), fastTicking ? 100 : 1000) return () => clearInterval(timer) } diff --git a/web/src/lib/session-status.test.ts b/web/src/lib/session-status.test.ts index d3330dfc..d22c13ec 100644 --- a/web/src/lib/session-status.test.ts +++ b/web/src/lib/session-status.test.ts @@ -92,6 +92,30 @@ describe('projectClientSessionStatus pause states', () => { expect(statusLabel('paused')).toBe('Paused') }) + it('a pending path confirmation while running derives the waiting state (precondition of the stop bug)', () => { + const view = projectClientSessionStatus({ + phase: 'plan', + isRunning: true, + pendingQuestionsCount: 0, + pendingConfirmationsCount: 1, + activeWorkflow: null, + }) + expect(view.state).toBe('waiting') + expect(view.waitingForUser).toBe(true) + }) + + it('a stopped session with no pending input never derives the waiting state', () => { + const view = projectClientSessionStatus({ + phase: 'plan', + isRunning: false, + pendingQuestionsCount: 0, + pendingConfirmationsCount: 0, + activeWorkflow: null, + }) + expect(view.state).toBeNull() + expect(view.waitingForUser).toBe(false) + }) + function makeSession(overrides: Partial = {}): Session { return { id: 's1', diff --git a/web/src/lib/thinking-timing.ts b/web/src/lib/thinking-timing.ts new file mode 100644 index 00000000..ba5d36af --- /dev/null +++ b/web/src/lib/thinking-timing.ts @@ -0,0 +1,48 @@ +interface ThinkingTimingEntry { + start: number + end?: number +} + +// Client-side thinking timing, keyed by message id. The start is latched by +// the session store when the first `chat.thinking` payload arrives for a +// message during a live stream; the end is latched when the first +// non-thinking output (text delta or tool preparing) arrives — and, if that +// was never observed live, when the summary first renders after the thinking +// finished. This bridges the gap between the live "Thinking…" state and the +// authoritative server-measured duration that lands with the message stats at +// turn end (and survives page reloads). Keeping it at module scope lets the +// timing survive scroll-driven remounts within a session. +const thinkingTiming = new Map() +// Bound the map: entries for messages that never receive a server-measured +// duration (aborted turns, brief thinking, no stats) are never evicted on +// their own, so cap the total to keep memory bounded over long sessions. +const MAX_THINKING_TIMING_ENTRIES = 500 + +/** Latch the thinking start for a message; idempotent — the first latch wins. */ +export function latchThinkingStart(messageId: string): number { + const existing = thinkingTiming.get(messageId) + if (existing) return existing.start + if (thinkingTiming.size >= MAX_THINKING_TIMING_ENTRIES) { + const oldest = thinkingTiming.keys().next().value + if (oldest !== undefined) thinkingTiming.delete(oldest) + } + const entry: ThinkingTimingEntry = { start: Date.now() } + thinkingTiming.set(messageId, entry) + return entry.start +} + +export function getThinkingStart(messageId: string): number | undefined { + return thinkingTiming.get(messageId)?.start +} + +/** Latch the thinking end; returns the elapsed time in seconds, or undefined if no start was latched. */ +export function latchThinkingEnd(messageId: string): number | undefined { + const entry = thinkingTiming.get(messageId) + if (!entry) return undefined + if (entry.end === undefined) entry.end = Date.now() + return (entry.end - entry.start) / 1000 +} + +export function clearThinkingTiming(messageId: string): void { + thinkingTiming.delete(messageId) +} diff --git a/web/src/stores/session/messageHandler.test.ts b/web/src/stores/session/messageHandler.test.ts index a8c433f2..96e3ca6f 100644 --- a/web/src/stores/session/messageHandler.test.ts +++ b/web/src/stores/session/messageHandler.test.ts @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' vi.stubGlobal('requestAnimationFrame', (cb: () => void) => setTimeout(cb, 0)) vi.stubGlobal('cancelAnimationFrame', (id: number) => clearTimeout(id)) @@ -762,6 +762,68 @@ describe('chat.stats handler', () => { }) }) +describe('chat.thinking timing latch', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('latches the thinking start on the first payload and keeps it for later payloads', async () => { + const useSessionStore = await loadSessionStore() + // Import after the store's resetModules so this is the same module instance + // the handler writes through. + const timing = await import('../../lib/thinking-timing') + + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + + useSessionStore.getState().handleServerMessage({ + type: 'chat.thinking', + sessionId: 'session-1', + payload: { messageId: 'msg-t1', content: 'one' }, + }) + expect(timing.getThinkingStart('msg-t1')).toBe(1_000_000) + + vi.setSystemTime(1_060_000) + useSessionStore.getState().handleServerMessage({ + type: 'chat.thinking', + sessionId: 'session-1', + payload: { messageId: 'msg-t1', content: 'two' }, + }) + expect(timing.getThinkingStart('msg-t1')).toBe(1_000_000) + + useSessionStore.getState().handleServerMessage({ + type: 'chat.thinking', + sessionId: 'session-1', + payload: { messageId: 'msg-t2', content: 'three' }, + }) + expect(timing.getThinkingStart('msg-t2')).toBe(1_060_000) + }) + + it('latches the thinking end on the first non-thinking output, not on a later remount', async () => { + const useSessionStore = await loadSessionStore() + const timing = await import('../../lib/thinking-timing') + + vi.useFakeTimers() + vi.setSystemTime(1_000_000) + useSessionStore.getState().handleServerMessage({ + type: 'chat.thinking', + sessionId: 'session-1', + payload: { messageId: 'msg-te', content: 'think' }, + }) + + vi.setSystemTime(1_050_000) + useSessionStore.getState().handleServerMessage({ + type: 'chat.delta', + sessionId: 'session-1', + payload: { messageId: 'msg-te', content: 'hello' }, + }) + + // Simulates a scroll-driven remount 60s after thinking ended + vi.setSystemTime(1_110_000) + expect(timing.latchThinkingEnd('msg-te')).toBe(50) + }) +}) + describe('session.deleted handler', () => { beforeEach(() => { wsSendMock.mockClear() diff --git a/web/src/stores/session/messageHandler.ts b/web/src/stores/session/messageHandler.ts index d05da18f..a3788043 100644 --- a/web/src/stores/session/messageHandler.ts +++ b/web/src/stores/session/messageHandler.ts @@ -40,6 +40,7 @@ import type { AgentType } from '../notifications' import type { SessionState, PendingQuestion, SessionPane } from './types' import { handleGlobalSoundEffects, resolveAgentType } from './sounds' import { getBuffer, scheduleStreamingFlush, cancelStreamingFlush } from './streamingBuffer' +import { latchThinkingEnd, latchThinkingStart } from '../../lib/thinking-timing' import { snapshot } from '../../lib/resourceCache' import { mcpServersResource, settingResource, SETTINGS_KEYS, type McpServerInfo } from '../../lib/resources' import { @@ -562,6 +563,7 @@ export function handleServerMessage( case 'chat.delta': { const sessionId = message.sessionId const payload = message.payload as ChatDeltaPayload + latchThinkingEnd(payload.messageId) if ( !applyChat(set, get, sessionId, (pane) => { if (sessionId === activeSessionId) { @@ -589,6 +591,7 @@ export function handleServerMessage( case 'chat.thinking': { const sessionId = message.sessionId const payload = message.payload as ChatThinkingPayload + latchThinkingStart(payload.messageId) if ( !applyChat(set, get, sessionId, (pane) => { const buf = getBuffer(sessionId ?? '') @@ -607,6 +610,7 @@ export function handleServerMessage( case 'chat.tool_preparing': { const sessionId = message.sessionId const payload = message.payload as ChatToolPreparingPayload + latchThinkingEnd(payload.messageId) if ( !applyChat(set, get, sessionId, (pane) => { const msg = pane.messages.find((m) => m.id === payload.messageId)