From 7229ce021daa371171ca2a91b3b70e9bbaf67264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 18 Aug 2026 07:52:06 +0000 Subject: [PATCH] watcher loop: the prompt/dialog half, and stopped no longer reads as failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit owner's call, confirmed 2026-08-18: implement watcher-loop-proposal.md exactly as written, before the first real tests — this governs agent behavior generally, not just command hq. items 1-2 of the proposal's own suggested order, done; items 3-4 (harness-owned re-arm, arming state on the commander row) are the harder half, next. FireBehavior replaces the stopOnFire boolean: 'stop' | 'handoff' | 'keep-going'. handoff carries the proposal's one-sentence contract verbatim ("the harness re-arms you — do not re-arm yourself, and do not build your own polling loop on top of this one") and rewrites the closing IF IT EXITS block so it stops contradicting rule 7 (the old universal "do not restart it, i will start it again" directly lied about what handoff jobs actually do). isCommander gates the option to where the promise can be kept. the proposal frames this entirely in terms of commanders — arming state's only home is the commander row — so outside one there's nowhere to persist "keep re-arming this," and offering the option there would make a promise nothing fulfills. not wired to true anywhere yet: command hq's own composer, which would set it, doesn't exist. every existing /chat caller is unaffected — two options, stop default, unchanged from before tonight. stopped-vs-died (§5): a job officer had just SIGTERM'd came back status:'failed' (exit code 144) from the harness's own classification, which this codebase does not control. fixed client-side — the tray already knows the instant stop is clicked, so it now also marks that, and useChat's task:notification handler overrides the displayed status regardless of what the harness reported. known gap, stated in both the code and the doc: in-memory, so it doesn't survive a refresh in the narrow window between clicking stop and the notification landing. docs/command-hq.md gains "the watcher loop" section: what's built, what isn't, the proposal's own suggested order followed as given. bunx tsgo clean. suite: 1038 pass (+9 new tests), same 7 pre-existing baseline failures. Co-Authored-By: Claude Opus 5 --- docs/command-hq.md | 62 +++++++++++-- .../apps/Chat/background-job-prompt.test.ts | 55 ++++++++++++ .../src/apps/Chat/background-job-prompt.ts | 86 +++++++++++++++---- .../Chat/components/BackgroundJobDialog.tsx | 46 ++++++++-- .../Chat/components/BackgroundTaskTray.tsx | 6 +- .../src/background-job-origin.test.ts | 34 ++++++++ .../officerdev/src/background-job-origin.ts | 29 +++++++ .../officerdev/src/hooks/useChat.ts | 14 ++- 8 files changed, 299 insertions(+), 33 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Chat/background-job-prompt.test.ts diff --git a/docs/command-hq.md b/docs/command-hq.md index 2f247d03..7f5c08fe 100644 --- a/docs/command-hq.md +++ b/docs/command-hq.md @@ -287,13 +287,12 @@ an independent record of what sessions exist. Consequences the owner confirmed e is a commander for life, and (given commanders live in their own cwd, never a `/chat` cwd) the two spaces never actually overlap. -## Built on, unchanged +## Built on, mostly unchanged -Everything about _how a background job runs_ is out of scope — this only adds a place to browse, name and -categorise the sessions the existing system produces. In particular, unmodified: +Everything about _how a background job runs_ was out of scope for the original build — a place to browse, +name and categorise the sessions the existing system produces. That held until real testing found the +mechanism itself wanting; see "The watcher loop" below for what changed and why. Otherwise, unmodified: -- The Background Job dialog and `buildBackgroundJobPrompt` — Command vs Prompt, the watch/stop-on-fire - rules, "run until stopped" vs "run for N minutes" (both prose in the prompt, no server-side timer). - The whole lifecycle: `run_in_background`/`Monitor`, `task:started`/`task:notification`, `chat_session_events` as the durable log, `collectRunningTasks` as the fold, the tray. - Stop/pause/restart routes under `/chat/tasks/:id`. @@ -301,6 +300,59 @@ categorise the sessions the existing system produces. In particular, unmodified: - `officer-notify` — assume it works and is wired up separately; do not build a parallel notification path for this feature. +## The watcher loop — exit, act, drain, re-arm + +`COMMS/command-hq/watchers-what-broke.md` and `watcher-loop-proposal.md`, both 2026-08-18: the mobile side +actually ran a watcher (poll `git ls-remote`, react to each push) through the dialog, and the mechanism +broke in ways no amount of reading the code would have found — two watchers alive at once because a new +one was armed before the old was reaped, a watcher firing on the agent's own push to the branch it was +watching, "keep going" silently meaning the agent would never see a firing again because a background +process reaches the model only when it EXITS. The owner's call, confirmed 2026-08-18: **implement the +proposal as written, before the first real tests** — this governs the agent's behaviour generally, not +only Command HQ, so it is not optional groundwork the way most of this document is. + +**Built:** + +- **A third dialog option**, `FireBehavior = 'stop' | 'handoff' | 'keep-going'` (was a `stopOnFire` + boolean). `handoff` — "Exit and hand back to the agent, then re-arm" — carries the proposal's one-sentence + contract (`handoffContract` in `background-job-prompt.ts`) verbatim: _"When the condition fires you will + be handed back, with the change. Do the work. The harness re-arms you — do not re-arm yourself, and do + not build your own polling loop on top of this one."_ The closing "IF IT EXITS" section is rewritten for + `handoff` too, so it does not contradict rule 7 by telling the model an exit means stay-stopped. +- **`isCommander`, gating the promise to where it can be kept.** The proposal frames this entirely in terms + of commanders — "a commander already has a row," arming state's only home — and outside one there is + nowhere to persist "this should keep re-arming itself," so nothing would fulfil the promise. Rather than + make it anyway, `BackgroundJobDialog` only offers `handoff` when `isCommander` is true, defaulting to it + there and to the pre-existing `stop` everywhere else. **Not yet wired to `true` anywhere** — Command HQ's + own composer, which would set it, does not exist yet (web) or does not use this dialog (mobile has its + own). Every existing `/chat` caller is unaffected: two options, `stop` default, exactly as before tonight. +- **Stopped no longer reads as failed.** `watchers-what-broke.md` §5: a job Officer had just SIGTERM'd came + back `status: 'failed'` (exit code 144) from the harness's own classification, which this codebase does + not control. Fixed client-side, at the one place it can be fixed cheaply: `stopTask` in + `BackgroundTaskTray.tsx` now also calls `markStopRequested(taskId)` + (`background-job-origin.ts`), and `useChat.ts`'s `task:notification` handler overrides the displayed + status to `stopped` when a stop was just requested for that id, regardless of what the harness reported. + Known gap: this is in-memory client state, so it does not survive a page refresh between clicking Stop + and the notification arriving — an acceptable, narrow window, not attempted to be closed tonight. + +**Not yet built — the harder half:** + +- **Harness-owned re-arm.** The proposal's actual infrastructure ask: once the agent's turn responding to a + firing ends, the PLATFORM (not the model) automatically respawns the original watch command as a fresh + background task. The natural implementation reuses `task-restart.ts`'s existing `commandForTask` (already + reconstructs a shell command from a transcript's tool_use/tool_result pairing) and its detached-spawn + machinery — re-arming becomes "run the restart path again, automatically, keyed off a `result` event for + a commander whose watcher just fired," rather than new command-recovery logic. +- **Arming state on the commander row.** `armed`, the current watch task's id, `lastFiredAt` — the + proposal's own suggested order puts this LAST, "once 3 exists to write it." Settles "one armed watcher + per commander" as a natural side effect (the row can refuse a second) and "stopped vs died" for commanders + specifically (the thing that re-arms already knows which it was). + +Suggested order from the proposal, followed here: (1) prompt + dialog — done, (2) stopped-vs-died — done, +(3) harness-owned re-arm, (4) arming state. "Nothing here blocks the first live test of the apps; all of it +blocks trusting a watcher that is meant to run for months" — the proposal's own words, and the reason (3) +and (4) are still open rather than rushed. + ## Later — explicitly deferred, not forgotten Raised during spec and set aside for a second version, in the owner's words: _"that's not... that's out of diff --git a/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.test.ts b/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.test.ts new file mode 100644 index 00000000..cb29e817 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, test } from 'bun:test'; +import { buildBackgroundJobPrompt, handoffContract, type BackgroundJobSpec } from './background-job-prompt'; + +const base: BackgroundJobSpec = { + mode: 'prompt', + body: '', + condition: 'watch for a new push to master; pull it and run the tests', + onFire: 'handoff', + duration: null, + cwd: '/home/owner/project', +}; + +describe('buildBackgroundJobPrompt — fire behavior', () => { + test('handoff exits AND carries the harness-owns-re-arm contract verbatim', () => { + const prompt = buildBackgroundJobPrompt(base); + expect(prompt).toContain('7. EXIT when it fires'); + expect(prompt).toContain(handoffContract); + // The whole point: the model must not be told to re-arm itself. + expect(prompt).not.toContain('Re-arm only if I ask'); + }); + + test("handoff's closing contract does not contradict rule 7 by telling the model to stay stopped", () => { + const prompt = buildBackgroundJobPrompt(base); + // The generic closing ("Do not restart it... I will start it again") would directly contradict a + // mechanism whose entire point is coming back on its own — asserting both would tell the model to + // distrust whichever one it read second. + expect(prompt).not.toContain('If I want it running again I will start it again'); + expect(prompt).toContain('the harness re-arms a'); + }); + + test('stop exits once and says so — no handoff language, no auto-restart language', () => { + const prompt = buildBackgroundJobPrompt({ ...base, onFire: 'stop' }); + expect(prompt).toContain('7. EXIT when it fires'); + expect(prompt).not.toContain(handoffContract); + expect(prompt).toContain('does not come back on its own'); + // The GENERIC closing applies here — stop really does mean stop. + expect(prompt).toContain('If I want it running again I will start it again'); + }); + + test('keep-going never exits on fire, and gets neither exit contract', () => { + const prompt = buildBackgroundJobPrompt({ ...base, onFire: 'keep-going' }); + expect(prompt).toContain('Do NOT exit when it fires'); + expect(prompt).not.toContain(handoffContract); + // The keep-going job's own exit is still covered by the trailing "IF IT EXITS" — an unexpected exit + // is still a crash for a job whose product is its live output. + expect(prompt).toContain('If I want it running again I will start it again'); + }); + + test('onFire is irrelevant without a condition — no watcher, no rule 7 at all', () => { + const prompt = buildBackgroundJobPrompt({ ...base, condition: '', onFire: 'handoff' }); + expect(prompt).not.toContain(handoffContract); + expect(prompt).not.toContain('7. EXIT when it fires'); + expect(prompt).not.toContain('Do NOT exit when it fires'); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.ts b/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.ts index e511f058..3714a9b3 100644 --- a/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.ts +++ b/src/workspaces/officerdev/src/apps/Chat/background-job-prompt.ts @@ -9,6 +9,21 @@ * refactor. */ +/** + * What happens when a watcher's condition fires. Ignored without a condition. + * + * Three, not two, since `COMMS/command-hq/watcher-loop-proposal.md` (2026-08-18, from a real watcher + * actually run and actually breaking): the original two conflated "the job is one-shot" with "the agent + * needs to see what fired", and a job armed as `keep-going` for that second reason could not react at all + * — a background PROCESS reaches the model only when it EXITS, so a loop that never exits can do shell + * work and nothing else. It cannot read a diff, judge whether a push matters, or write code. + * + * `handoff` is the fix: exit is still the notification (the one part of `keep-going` that always worked), + * but re-arming is no longer the model's job. See `handoffContract` below — the harness owns the loop now, + * which is the part `watchers-what-broke.md` found the model could not reliably hold across turns anyway. + */ +export type FireBehavior = 'stop' | 'handoff' | 'keep-going'; + export type BackgroundJobSpec = { /** `command` runs verbatim; `prompt` is an intent the agent works out. A watcher is always `prompt`. */ mode: 'command' | 'prompt'; @@ -16,8 +31,8 @@ export type BackgroundJobSpec = { body: string; /** Prose: what to wait for and what to do when it happens. Empty means no watcher. */ condition: string; - /** Whether firing ends the job. Ignored without a condition. */ - stopOnFire: boolean; + /** What happens on fire — see `FireBehavior`. Ignored without a condition. */ + onFire: FireBehavior; /** * Minutes to run for, or `null` to run until stopped. * @@ -35,6 +50,37 @@ const durationLine = (d: BackgroundJobSpec['duration']): string => ? 'Run until I stop it. Do not bound the loop by time.' : `Run for about ${d} minute${d === 1 ? '' : 's'}, then exit 1 to say it timed out.`; +/** + * Rule 7 — what happens on fire. `stop` and `handoff` both exit; only the sentence after that differs, + * and that sentence is the entire fix `watcher-loop-proposal.md` asked for. Kept as one exported constant + * (`handoffContract`) rather than inlined, because it is meant to change meaning at exactly one place — + * the day the harness actually owns re-arming — and a prompt asserting that has to say the true thing. + */ +export const handoffContract = + 'When the condition fires you will be handed back, with the change. Do the work. The harness re-arms ' + + 'you — do not re-arm yourself, and do not build your own polling loop on top of this one.'; + +function fireRule(onFire: FireBehavior): string { + if (onFire === 'keep-going') { + return [ + `7. Do NOT exit when it fires. Do what was asked, print ONE line saying what happened, and`, + ` keep going. Here the stream is the notification, not the exit — the output is visible`, + ` live, so a line per event is the point. Still print nothing on a tick where nothing`, + ` happened, and never let one failure end the job: log it and continue.`, + ].join('\n'); + } + + const exitLines = [ + `7. EXIT when it fires — the exit IS the notification. A loop that prints and continues has`, + ` told no one. Stay silent until then: everything printed lands in context at once on exit.`, + ` Report what fired.`, + ]; + + return onFire === 'handoff' + ? [...exitLines, ``, ` ${handoffContract}`].join('\n') + : [...exitLines, ` This does not come back on its own — if I want it again I will start it again.`].join('\n'); +} + export function buildBackgroundJobPrompt(spec: BackgroundJobSpec): string { const watching = spec.condition.trim().length > 0; @@ -78,18 +124,7 @@ export function buildBackgroundJobPrompt(spec: BackgroundJobSpec): string { `6. Probe read-only — \`git ls-remote\`, never \`git fetch\`. Mutate only AFTER it fires, and only`, ` as asked above. \`timeout\` every network call, or a hung one leaves it alive and blind.`, ``, - spec.stopOnFire - ? [ - `7. EXIT when it fires — the exit IS the notification. A loop that prints and continues has`, - ` told no one. Stay silent until then: everything printed lands in context at once on exit.`, - ` Report what fired. Re-arm only if I ask.`, - ].join('\n') - : [ - `7. Do NOT exit when it fires. Do what was asked, print ONE line saying what happened, and`, - ` keep going. Here the stream is the notification, not the exit — the output is visible`, - ` live, so a line per event is the point. Still print nothing on a tick where nothing`, - ` happened, and never let one failure end the job: log it and continue.`, - ].join('\n'), + fireRule(spec.onFire), ); } @@ -126,6 +161,27 @@ export function buildBackgroundJobPrompt(spec: BackgroundJobSpec): string { `that is the entire point of using the harness's mechanism, and checking by hand just adds noise`, `to the one case where nothing is wrong.`, ``, + ...exitContract(watching && spec.onFire === 'handoff'), + ].join('\n'); +} + +/** + * The closing contract differs for `handoff`, and has to: the generic version tells the model an exit + * means STOP and never comes back on its own, which is exactly backwards for a job whose whole design is + * that it comes back on its own. Two different promises, so two different closings — asserting the wrong + * one would tell the model to distrust the very mechanism rule 7 just described. + */ +function exitContract(isHandoff: boolean): string[] { + if (isHandoff) { + return [ + `IF IT EXITS:`, + ``, + `That is either the condition firing — expected, and rule 7 already told you what to do — or me`, + `stopping it by hand from its chip. Either way, do not restart it yourself: the harness re-arms a`, + `firing on its own, and a stop I asked for staying stopped is the whole point of the button.`, + ]; + } + return [ `IF IT EXITS:`, ``, `Say so in one line and STOP. Do not restart it. Do not diagnose why. Do not theorise about what`, @@ -134,5 +190,5 @@ export function buildBackgroundJobPrompt(spec: BackgroundJobSpec): string { `I can stop a job myself, from a button on its chip — so an exit is at least as likely to be me`, `as it is to be a failure, and a job I stopped that comes back on its own is the opposite of a`, `stop button. If I want it running again I will start it again.`, - ].join('\n'); + ]; } diff --git a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundJobDialog.tsx b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundJobDialog.tsx index 1b145c15..ecc81805 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundJobDialog.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundJobDialog.tsx @@ -13,7 +13,7 @@ import { import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; import { Textarea } from '@/components/ui/textarea'; -import { buildBackgroundJobPrompt, type BackgroundJobSpec } from '../background-job-prompt'; +import { buildBackgroundJobPrompt, type BackgroundJobSpec, type FireBehavior } from '../background-job-prompt'; // Spawning a background job. // @@ -33,14 +33,36 @@ type BackgroundJobDialogProps = { onOpenChange: (open: boolean) => void; cwd: string; onSubmit: (intent: string, preamble: string) => void; + /** + * Is this composer running inside a Command HQ commander? + * + * `handoff` is a promise the PLATFORM has to keep, not just the prompt: `watcher-loop-proposal.md`'s + * whole point is that re-arming is the harness's job now, and the only place that job's state can live + * is the commander row it fires against — see `docs/command-hq.md`. Outside a commander there is no row, + * so nothing would ever actually re-arm it; offering the option there would tell the model "the harness + * re-arms you" and then not do it, which is worse than the two options that already existed. Defaults to + * `false` deliberately — every EXISTING `/chat` caller of this dialog gets it, and none of them are + * commanders yet, so none of them should change behaviour by omission. + */ + isCommander?: boolean; }; -export const BackgroundJobDialog = ({ open, onOpenChange, cwd, onSubmit }: BackgroundJobDialogProps) => { +export const BackgroundJobDialog = ({ + open, + onOpenChange, + cwd, + onSubmit, + isCommander = false, +}: BackgroundJobDialogProps) => { const [mode, setMode] = useState<'command' | 'prompt'>('command'); const [body, setBody] = useState(''); const [watching, setWatching] = useState(false); const [condition, setCondition] = useState(''); - const [stopOnFire, setStopOnFire] = useState(true); + // `handoff` is the default WHERE IT IS OFFERED: `watcher-loop-proposal.md` — a watcher exists so the + // AGENT can react, and a background process reaches the model only when it exits. Defaulting to `stop` + // or `keep-going` defaults most watchers into a shape that cannot do the one thing they were armed for. + // Outside a commander, `stop` remains the default — unchanged from before this option existed. + const [onFire, setOnFire] = useState(isCommander ? 'handoff' : 'stop'); // Unchecked means no time bound — the job runs until stopped. There is no third option: the backend // keeps the agent alive while a job runs, so "this session" would have meant the same thing. const [timed, setTimed] = useState(false); @@ -52,10 +74,10 @@ export const BackgroundJobDialog = ({ open, onOpenChange, cwd, onSubmit }: Backg setBody(''); setWatching(false); setCondition(''); - setStopOnFire(true); + setOnFire(isCommander ? 'handoff' : 'stop'); setTimed(false); setMinutes(60); - }, [open]); + }, [open, isCommander]); // A watcher is always prompted. Flipping the checkbox flips the mode rather than leaving an // impossible combination selected and rejecting it on submit. @@ -74,7 +96,7 @@ export const BackgroundJobDialog = ({ open, onOpenChange, cwd, onSubmit }: Backg mode, body, condition: watching ? condition : '', - stopOnFire, + onFire, duration: timed ? minutes : null, cwd, }; @@ -138,12 +160,18 @@ export const BackgroundJobDialog = ({ open, onOpenChange, cwd, onSubmit }: Backg In prose. The agent will ask before arming if anything is unclear.

+ {isCommander && ( + + )}
diff --git a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx index 73351914..1f1c471e 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/BackgroundTaskTray.tsx @@ -9,7 +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'; +import { markUserBackgroundJob, markStopRequested } from '../../../background-job-origin'; type BackgroundTaskTrayProps = { messages: ChatMessage[]; @@ -60,6 +60,10 @@ export const BackgroundTaskTray = ({ messages, sessionId }: BackgroundTaskTrayPr // 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); + // Separately: the harness does not reliably say `status: 'stopped'` for a job Officer just SIGTERM'd + // (observed live at exit code 144, read as "failed") — so the client relabels it itself. See + // `background-job-origin.ts`. + markStopRequested(taskId); try { await client.delete(`/chat/tasks/${taskId}`); } catch (err) { diff --git a/src/workspaces/officerdev/src/background-job-origin.test.ts b/src/workspaces/officerdev/src/background-job-origin.test.ts index d991f132..caf1ab69 100644 --- a/src/workspaces/officerdev/src/background-job-origin.test.ts +++ b/src/workspaces/officerdev/src/background-job-origin.test.ts @@ -78,4 +78,38 @@ describe('createBackgroundJobOrigin', () => { expect(b.claim('x')).toBe(false); expect(a.claim('x')).toBe(true); }); + + // Observed live: a job the client SIGTERM'd via Stop still came back `status: 'failed'` from the + // harness (exit code 144). The client's own record of having pressed Stop is what the notification + // handler trusts instead — these are the bare mechanics of that record. + describe('stop requests', () => { + it('is not requested for a job nobody stopped', () => { + const origin = createBackgroundJobOrigin(); + expect(origin.consumeStopRequested('dev-server')).toBe(false); + }); + + it('remembers a stop request until the one notification that answers it', () => { + const origin = createBackgroundJobOrigin(); + origin.markStopRequested('dev-server'); + expect(origin.consumeStopRequested('dev-server')).toBe(true); + // A task's life is exactly two events — the mark does not outlive the one notification it was for. + expect(origin.consumeStopRequested('dev-server')).toBe(false); + }); + + it('is a separate record from "the user started or clicked this job"', () => { + // `mark`/`has` answers "should a completion of THIS reload the preview" and a user-started job can + // still genuinely fail — the two must not share state, or a real failure would misreport as stopped. + const origin = createBackgroundJobOrigin(); + origin.mark('dev-server'); + expect(origin.has('dev-server')).toBe(true); + expect(origin.consumeStopRequested('dev-server')).toBe(false); + }); + + it('does not confuse two different jobs', () => { + const origin = createBackgroundJobOrigin(); + origin.markStopRequested('dev-server'); + expect(origin.consumeStopRequested('a-build')).toBe(false); + expect(origin.consumeStopRequested('dev-server')).toBe(true); + }); + }); }); diff --git a/src/workspaces/officerdev/src/background-job-origin.ts b/src/workspaces/officerdev/src/background-job-origin.ts index f5f50c43..1108282a 100644 --- a/src/workspaces/officerdev/src/background-job-origin.ts +++ b/src/workspaces/officerdev/src/background-job-origin.ts @@ -46,6 +46,21 @@ const MAX_TRACKED = 100; export function createBackgroundJobOrigin() { let armedAt: number | null = null; let ids: string[] = []; + /** + * Task ids the user just pressed Stop on, waiting for the `task:notification` that follows. + * + * Exists because the harness does not reliably say `status: 'stopped'` for a job Officer just SIGTERM'd — + * observed live (`COMMS/command-hq/watchers-what-broke.md` §5): a deliberate stop came back as "failed + * with exit code 144". The classification happens inside the SDK's own Monitor tool, which this codebase + * does not control, so the fix is on the reading side — the client already knows, at the moment it clicks + * Stop, that whatever status arrives next for THIS id should read as "stopped", not whatever the harness + * decided. A SEPARATE set from `ids` above: that one means "the user started or interacted with this job" + * and a job the user started can still genuinely fail, so it cannot double as "I stopped this one". + * + * Consumed once, by the first `task:notification` for that id — a task's life is exactly two events, so + * there is exactly one notification to relabel. + */ + let stopRequested: string[] = []; const track = (taskId: string) => { if (ids.includes(taskId)) return; @@ -84,6 +99,18 @@ export function createBackgroundJobOrigin() { * and it bends towards the user actually having clicked something. */ mark: track, + + /** The user just pressed Stop on this job. */ + markStopRequested: (taskId: string) => { + if (!stopRequested.includes(taskId)) stopRequested = [...stopRequested, taskId].slice(-MAX_TRACKED); + }, + + /** Was Stop just pressed on this job? Consumes the mark — the next notification is the last word. */ + consumeStopRequested: (taskId: string): boolean => { + if (!stopRequested.includes(taskId)) return false; + stopRequested = stopRequested.filter((id) => id !== taskId); + return true; + }, }; } @@ -93,3 +120,5 @@ export const armBackgroundJob = shared.arm; export const claimBackgroundJob = shared.claim; export const isUserBackgroundJob = shared.has; export const markUserBackgroundJob = shared.mark; +export const markStopRequested = shared.markStopRequested; +export const consumeStopRequested = shared.consumeStopRequested; diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index 9f842651..55a26dce 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -5,7 +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 { claimBackgroundJob, isUserBackgroundJob, consumeStopRequested } from '../background-job-origin'; import { randomId } from 'helpers/random-id'; const SAVE_DEBOUNCE_MS = 1000; @@ -583,19 +583,27 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, // persistent worker. Resolved onto the row that announced it rather than appended as a second // bubble: the two used to be minutes and a screenful of scrollback apart, with the taskId that // relates them thrown away on both sides. + // + // The harness's own `status` is not trusted blindly here. Observed live: a job Officer had just + // SIGTERM'd via the Stop button came back `failed` (exit code 144) rather than `stopped` — the + // classification happens inside the SDK's own Monitor tool, which this codebase does not control. + // `consumeStopRequested` is the client's own record of having pressed Stop, and it is authoritative + // over whatever the harness reports: a card that shows "failed" next to a job the user deliberately + // turned off reads as something broken, which is worse than the harness simply being wrong here. + const status = consumeStopRequested(msg.taskId) ? 'stopped' : msg.status; setMessages((prev) => { const at = prev.findIndex((m) => m.role === 'task' && m.taskId === msg.taskId); // No announcing row — a replay that starts after `task:started` was pruned. Stand one up. if (at < 0) { return [ ...prev, - { role: 'task', taskId: msg.taskId, description: msg.summary, status: msg.status, summary: msg.summary }, + { role: 'task', taskId: msg.taskId, description: msg.summary, status, summary: msg.summary }, ]; } const next = [...prev]; next[at] = { ...(next[at] as Extract), - status: msg.status, + status, summary: msg.summary, }; return next;