diff --git a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx index 6b12b9d8..8b083b81 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx @@ -9,6 +9,7 @@ import type { ChatMessage } from '../types'; import { useBackgroundTasks, useTaskDetail, type BackgroundTask } from '../useBackgroundTasks'; import { SubagentTrace } from './ToolActivity'; import { CodeSurface } from './CodeSurface'; +import { markUserBackgroundJob } from '../../../background-job-origin'; type BackgroundTaskTrayProps = { messages: ChatMessage[]; @@ -45,6 +46,9 @@ export const BackgroundTaskTray = ({ messages }: BackgroundTaskTrayProps) => { * A stop that fails leaves the chip running, which is the truth. */ const stopTask = async (taskId: string) => { + // Pressing stop is deliberate and aimed at one job, so its death is worth reacting to even if an + // agent started it — the preview may well be pointed at exactly this. + markUserBackgroundJob(taskId); try { await client.delete(`/chat/tasks/${taskId}`); } catch (err) { diff --git a/src/workspaces/officerdev/src/apps/Chat/components/InputArea.tsx b/src/workspaces/officerdev/src/apps/Chat/components/InputArea.tsx index 3d0622ad..9c40e8af 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/InputArea.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/InputArea.tsx @@ -6,6 +6,7 @@ import type { QueuedPrompt, UseEmbeddableChatType } from '../EmbeddableChat/useE import { useAudioRecording } from '../useAudioRecording'; import { ModelSelector } from './ModelSelector'; import { BackgroundJobDialog } from './BackgroundJobDialog'; +import { armBackgroundJob } from '../../../background-job-origin'; import { useWorkspace } from '../../../components/Workspace'; import { AttachmentList } from './AttachmentList'; import { AttachButton } from './AttachButton'; @@ -168,7 +169,12 @@ export const InputArea = ({ manager }: InputAreaProps) => { open={jobDialogOpen} onOpenChange={setJobDialogOpen} cwd={cwd} - onSubmit={(intent, preamble) => void sendWithHiddenPrefix(intent, preamble)} + onSubmit={(intent, preamble) => { + // Claimed by the next job to start, which is how the preview tells this one apart from the + // background jobs an agent starts on its own. + armBackgroundJob(); + void sendWithHiddenPrefix(intent, preamble); + }} /> ); diff --git a/src/workspaces/officerdev/src/background-job-origin.test.ts b/src/workspaces/officerdev/src/background-job-origin.test.ts new file mode 100644 index 00000000..d991f132 --- /dev/null +++ b/src/workspaces/officerdev/src/background-job-origin.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from 'bun:test'; +import { createBackgroundJobOrigin } from './background-job-origin'; + +// The bug being guarded against is not a crash — it is a preview that reloads while you are typing in it, +// because an agent happened to start a test run. Every case here is "did this reload, and should it have". + +describe('createBackgroundJobOrigin', () => { + it('ignores a job nobody asked for', () => { + const origin = createBackgroundJobOrigin(); + expect(origin.claim('agent-ran-the-tests')).toBe(false); + expect(origin.has('agent-ran-the-tests')).toBe(false); + }); + + it('claims the job that follows an arm, and remembers it for its completion', () => { + const origin = createBackgroundJobOrigin(); + origin.arm(); + expect(origin.claim('dev-server')).toBe(true); + expect(origin.has('dev-server')).toBe(true); + }); + + it('claims only the first job after an arm', () => { + const origin = createBackgroundJobOrigin(); + origin.arm(); + expect(origin.claim('dev-server')).toBe(true); + // Same turn, agent also kicks off a build. One dialog submission asked for one job. + expect(origin.claim('a-build')).toBe(false); + expect(origin.has('a-build')).toBe(false); + }); + + it('survives a clarifying exchange — the arm is not consumed by time alone', () => { + const origin = createBackgroundJobOrigin(); + const t0 = 1_000_000; + origin.arm(t0); + // The prompt tells the agent to ask before arming anything ambiguous, so minutes can pass. + expect(origin.claim('dev-server', t0 + 8 * 60 * 1000)).toBe(true); + }); + + it('forgets an arm that was never used', () => { + const origin = createBackgroundJobOrigin(); + const t0 = 1_000_000; + origin.arm(t0); + // Conversation abandoned; an hour later an agent starts something of its own. + expect(origin.claim('unrelated', t0 + 60 * 60 * 1000)).toBe(false); + expect(origin.has('unrelated')).toBe(false); + }); + + it('adopts a job stopped by hand, whoever started it', () => { + const origin = createBackgroundJobOrigin(); + origin.mark('agent-started-dev-server'); + expect(origin.has('agent-started-dev-server')).toBe(true); + }); + + it('keeps tracking straight across several jobs', () => { + const origin = createBackgroundJobOrigin(); + origin.arm(); + origin.claim('first'); + origin.arm(); + origin.claim('second'); + expect(origin.has('first')).toBe(true); + expect(origin.has('second')).toBe(true); + expect(origin.has('third')).toBe(false); + }); + + it('does not grow without bound', () => { + const origin = createBackgroundJobOrigin(); + for (let i = 0; i < 250; i++) { + origin.mark(`job-${i}`); + } + // The oldest are dropped; a completion that old has long since arrived or never will. + expect(origin.has('job-0')).toBe(false); + expect(origin.has('job-249')).toBe(true); + }); + + it('is per-instance, so one panel cannot claim another’s arm in a test', () => { + const a = createBackgroundJobOrigin(); + const b = createBackgroundJobOrigin(); + a.arm(); + expect(b.claim('x')).toBe(false); + expect(a.claim('x')).toBe(true); + }); +}); diff --git a/src/workspaces/officerdev/src/background-job-origin.ts b/src/workspaces/officerdev/src/background-job-origin.ts new file mode 100644 index 00000000..f5f50c43 --- /dev/null +++ b/src/workspaces/officerdev/src/background-job-origin.ts @@ -0,0 +1,95 @@ +/** + * Which background jobs the USER started, as opposed to the ones an agent starts while working. + * + * ── Why this has to exist ── + * + * The preview reloads when a background job starts or ends, because a dev server restarting leaves a dead + * HMR socket inside the iframe — the page keeps rendering, keeps looking alive, and silently stops + * updating. Reloading is the only way out of that state. + * + * But `task:started` is not a rare event. An agent starts background jobs constantly while coding: a test + * run, a build, a long grep. Reloading the preview on those is exactly the churn the whole design was + * trying to avoid — it just moved the trigger from "every edit" to "every incidental job". + * + * The jobs that matter are the ones started from the background-job dialog. That is where somebody says + * "run the dev server" — so that is the only start worth reacting to. + * + * ── The claim, and why a flag rather than an id ── + * + * The dialog cannot name the task it is about to create: it sends a prompt, the agent works out the + * command, and the harness assigns the id. Nothing links the two but sequence. So the dialog ARMS, and the + * next `task:started` claims the arm and records its id. From then on the id is what is matched, so the + * job's completion is recognised however long it takes. + * + * One arm, one claim. If the agent starts two jobs, only the first is ours — which is the right guess when + * the dialog asked for one job. + * + * The arm expires. The dialog's prompt tells the agent to ASK FIRST when the request is ambiguous, so an + * arm can sit pending across a question, an answer, and only then a start — legitimately minutes. It can + * also sit pending forever, if that conversation is abandoned. The window bounds the damage of the second + * case to one spurious reload without breaking the first. + * + * ── Module state, deliberately ── + * + * Not a channel and not React state. Nothing re-renders on it: it is read inside the websocket message + * handler to decide whether to bump, and a value that no component displays has no business causing a + * render. `createBackgroundJobOrigin` exists so tests get a fresh one instead of a reset function that + * production code would also be able to call. + */ + +/** How long an arm stays claimable. Long enough for a clarifying exchange, short enough to forget. */ +const ARM_TTL_MS = 30 * 60 * 1000; + +/** Ids are kept only so a completion can be matched to its start; this caps a session's worth. */ +const MAX_TRACKED = 100; + +export function createBackgroundJobOrigin() { + let armedAt: number | null = null; + let ids: string[] = []; + + const track = (taskId: string) => { + if (ids.includes(taskId)) return; + ids = [...ids, taskId].slice(-MAX_TRACKED); + }; + + return { + /** The dialog was submitted. The next job to start is the one it asked for. */ + arm: (now: number = Date.now()) => { + armedAt = now; + }, + + /** + * A job started. Returns whether it is the one the dialog asked for — and if so, remembers it. + * + * Consuming the arm on the way past is what stops a second, unrelated job from also being claimed. + */ + claim: (taskId: string, now: number = Date.now()): boolean => { + if (armedAt === null || now - armedAt > ARM_TTL_MS) { + armedAt = null; + return false; + } + armedAt = null; + track(taskId); + return true; + }, + + /** Whether a job that just ended is one we care about. */ + has: (taskId: string): boolean => ids.includes(taskId), + + /** + * Adopt a job we did not start. + * + * Used by the stop button: pressing it is a deliberate act on a specific job, so its death is worth + * reacting to even for a job an agent started. This is the one place the "user-started" rule bends, + * and it bends towards the user actually having clicked something. + */ + mark: track, + }; +} + +const shared = createBackgroundJobOrigin(); + +export const armBackgroundJob = shared.arm; +export const claimBackgroundJob = shared.claim; +export const isUserBackgroundJob = shared.has; +export const markUserBackgroundJob = shared.mark; diff --git a/src/workspaces/officerdev/src/channels.ts b/src/workspaces/officerdev/src/channels.ts index b519f29b..a5fab7d9 100644 --- a/src/workspaces/officerdev/src/channels.ts +++ b/src/workspaces/officerdev/src/channels.ts @@ -66,7 +66,7 @@ export const useActiveChatSession = defineChannel('chat:active-se export const usePreviewRefresh = defineChannel('preview:refresh', 0); /** - * "A background job started or ended." + * "A background job the user started has started or ended." * * Bumped on `task:started` and on a task's completion — a few times a session, not once per edit. * @@ -77,9 +77,15 @@ export const usePreviewRefresh = defineChannel('preview:refresh', 0); * turn that touched a file — would throw away scroll, form state and route on every edit, which is the * thing HMR exists to avoid. * - * It over-triggers: any job starting reloads the preview, not only the one serving it. That costs one - * reload on an unrelated watcher and is preferable to the alternative, which is a preview that is stale - * and indistinguishable from a working one. + * Only jobs STARTED FROM THE DIALOG are bumped, plus any job stopped by hand. Agents start background + * jobs constantly while working, and firing on those would have reproduced the churn this was built to + * avoid with a different trigger. `background-job-origin` is what tells the two apart, and why it needs + * an arm-and-claim rather than an id. + * + * It still over-triggers within that set: any dialog-started job reloads every preview, not only one + * pointed at it. Nothing observable links a preview's URL to a job — the parent cannot see inside a + * cross-origin iframe — and a rare extra reload beats a preview that is stale and indistinguishable + * from a working one. */ const useBackgroundJobChangedChannel = defineChannel('background-job:changed', 0); diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index f2a97c21..2a88ccdd 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -5,6 +5,7 @@ import { useSettings } from 'state/useSettings'; import type { ChatMessage, ServerMessage, TaskInfo, Message } from '../apps/Chat/types'; import { spliceRunningTasks } from '../apps/Chat/running-tasks'; import { useBackgroundJobChanged } from '../channels'; +import { claimBackgroundJob, isUserBackgroundJob } from '../background-job-origin'; import { randomId } from 'helpers/random-id'; const SAVE_DEBOUNCE_MS = 1000; @@ -439,9 +440,10 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, ...prev, { role: 'task', taskId: msg.taskId, description: msg.description, taskType: msg.taskType }, ]); - // A job coming up may be a dev server coming back, which means any preview pointed at it is - // holding a dead HMR socket. See `background-job:changed`. - bumpJobChanged(); + // Only jobs the user started from the dialog. An agent starts background jobs constantly while + // working — tests, builds, greps — and reloading a preview on those is the same churn this was + // built to avoid, only with a different trigger. See `background-job-origin`. + if (claimBackgroundJob(msg.taskId)) bumpJobChanged(); break; case 'compact:start': @@ -493,9 +495,9 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, }; return next; }); - // And a job going down may be the dev server going down — a preview pointed at it should show - // that rather than a page that merely looks alive. - bumpJobChanged(); + // Matched by id rather than re-guessed: whatever the dialog's job turned out to be, its end is + // when the preview pointed at it went stale. + if (isUserBackgroundJob(msg.taskId)) bumpJobChanged(); break; } }