From 0ea09bcd9472d1a66f31ae2ccc753dd80766282e Mon Sep 17 00:00:00 2001 From: Ronen Mars Date: Wed, 12 Aug 2026 11:16:50 +0300 Subject: [PATCH 1/2] feat(session): derive agent phase server-side and emit it as an additive field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `subStatus`, the agent's phase within a running turn, scraped from the rendered PTY screen alongside the detectors that already share that read. Codex only for now — its status bar is binary and needs no footer archaeology — while the Claude branch returns null until its marker grammar is re-verified against a capture, because a guessed phase is worse than none. The field is always serialised and null when there is no phase. Absence must never carry meaning: clients merge session frames, a merge cannot express a removed key, and an omitted field would keep its previous value and latch the indicator on a finished turn. For the same reason the phase is cleared server-side in markReady rather than left for consumers to age out, and the mapper emits it unconditionally rather than through the `!= null` guard the neighbouring optional fields use. The frame is a scoped `session_phase` message rather than a session copy, because managedToResponse recomputes elapsedMs on every call and a copy would differ on every tick regardless of the phase. Bumps the pty-host protocol to 3 — the detector runs in the host, so without the event the feature silently no-ops when that flag is on. Refs #538 --- __tests__/agent-phase-contract.test.ts | 133 ++++++++++++++++++++++ __tests__/parse-agent-phase.test.ts | 64 +++++++++++ docs/compatibility/tb-mobile.md | 11 ++ src/pty-host/host.ts | 2 + src/pty-host/protocol.ts | 14 ++- src/pty-host/remote-session-runner.ts | 3 + src/pty-manager.ts | 38 +++++++ src/server.ts | 12 ++ src/services/questions/parseAgentPhase.ts | 65 +++++++++++ src/session-store.ts | 11 ++ src/types.ts | 82 +++++++++++++ 11 files changed, 433 insertions(+), 2 deletions(-) create mode 100644 __tests__/agent-phase-contract.test.ts create mode 100644 __tests__/parse-agent-phase.test.ts create mode 100644 src/services/questions/parseAgentPhase.ts diff --git a/__tests__/agent-phase-contract.test.ts b/__tests__/agent-phase-contract.test.ts new file mode 100644 index 00000000..dfc08b43 --- /dev/null +++ b/__tests__/agent-phase-contract.test.ts @@ -0,0 +1,133 @@ +// Contract tests for the agent-phase field: the wire shape and the pty-host relay. +// +// These exist because every failure mode this feature has is SILENT. A dropped +// null latches the indicator on a finished turn; a missing pty-host relay makes +// the feature no-op when the host flag is on. Neither shows up in a typecheck, +// and neither throws. + +import { describe, expect, it, vi } from "vitest"; +import { + encodeMessage, + type HostEvent, + type HostRequest, + PTY_HOST_PROTOCOL_VERSION, +} from "../src/pty-host/protocol"; +import { RemoteSessionRunner } from "../src/pty-host/remote-session-runner"; +import { SessionStore } from "../src/session-store"; +import type { ManagedSession, PTYManagerOptions } from "../src/types"; + +const STARTED = new Date("2026-08-12T10:00:00Z"); + +function mkSession(over: Partial = {}): ManagedSession { + return { + id: "sess-1", + provider: "claude-code", + projectPath: "/repo", + projectName: "repo", + branch: "main", + status: "running", + startedAt: STARTED, + completedAt: null, + lastOutput: "", + promptCount: 0, + inputHistory: [], + ...over, + } as ManagedSession; +} + +/** Minimal transport, mirroring the harness in pty-host-protocol.test.ts. */ +function mkHost() { + let onLine: (line: string) => void = () => {}; + const transport = { + send(line: string) { + const req = JSON.parse(line) as HostRequest; + queueMicrotask(() => { + const result = + req.type === "status" ? { protocolVersion: PTY_HOST_PROTOCOL_VERSION, sessions: [] } : {}; + onLine(encodeMessage({ id: req.id, ok: true, result })); + }); + }, + onLine(handler: (line: string) => void) { + onLine = handler; + }, + onClose(_handler: () => void) {}, + close: vi.fn(), + }; + return { transport, emit: (e: HostEvent) => onLine(encodeMessage(e)) }; +} + +describe("agent phase — wire contract", () => { + // The load-bearing one. managedToResponse guards ~19 optional fields with + // `...(x != null && { x })`, and `!=` catches null as well as undefined. If + // subStatus is ever moved into that block, an explicit "no phase" silently + // becomes an absent key — and because clients merge session frames, an absent + // key keeps its previous value. That is how tb-mobile PR #647's indicator + // latched onto finished turns. + it("serialises an explicit null rather than omitting the key", () => { + const store = new SessionStore(); + store.addManaged(mkSession({ subStatus: null })); + + const resp = store.get("sess-1", new Set()); + + expect(resp).not.toBeNull(); + expect("subStatus" in resp!).toBe(true); + expect(resp!.subStatus).toBeNull(); + // The distinction a merge cannot express: absent !== null. + expect(JSON.stringify(resp)).toContain('"subStatus":null'); + }); + + it("serialises a set phase", () => { + const store = new SessionStore(); + store.addManaged(mkSession({ subStatus: "working" })); + expect(store.get("sess-1", new Set())!.subStatus).toBe("working"); + }); + + it("emits null for a session that predates the field", () => { + const store = new SessionStore(); + const s = mkSession(); + delete (s as Partial).subStatus; + store.addManaged(s); + + const resp = store.get("sess-1", new Set()); + expect("subStatus" in resp!).toBe(true); + expect(resp!.subStatus).toBeNull(); + }); +}); + +describe("agent phase — pty-host relay", () => { + // protocol.ts's header: "the detectors that fire them run in the host, so + // every one of those callbacks needs an event here or the feature silently + // stops working when the flag is on." Declaring the event in the union type + // is NOT the same as wiring it — the type alone typechecks and does nothing. + it("delivers a phase-change event to onPhaseChange", async () => { + const onPhaseChange = vi.fn(); + const host = mkHost(); + await RemoteSessionRunner.connect(host.transport, { onPhaseChange } as PTYManagerOptions); + + host.emit({ type: "event", event: "phase-change", sessionId: "sess-1", phase: "working" }); + + expect(onPhaseChange).toHaveBeenCalledWith("sess-1", "working"); + }); + + // JSON.stringify drops undefined but preserves null — the property the whole + // clearing contract rests on. Asserted through the real encoder rather than + // assumed, because it is why `null` was chosen over omission. + it("relays an explicit null across the boundary without dropping it", async () => { + const onPhaseChange = vi.fn(); + const host = mkHost(); + await RemoteSessionRunner.connect(host.transport, { onPhaseChange } as PTYManagerOptions); + + host.emit({ type: "event", event: "phase-change", sessionId: "sess-1", phase: null }); + + expect(onPhaseChange).toHaveBeenCalledWith("sess-1", null); + }); + + it("does not throw when the callback is omitted (additive)", async () => { + const host = mkHost(); + await RemoteSessionRunner.connect(host.transport, {} as PTYManagerOptions); + + expect(() => + host.emit({ type: "event", event: "phase-change", sessionId: "sess-1", phase: "working" }), + ).not.toThrow(); + }); +}); diff --git a/__tests__/parse-agent-phase.test.ts b/__tests__/parse-agent-phase.test.ts new file mode 100644 index 00000000..6a6e6e8d --- /dev/null +++ b/__tests__/parse-agent-phase.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it } from "vitest"; +import { CLAUDE_CODE_PROVIDER, CODEX_CLI_PROVIDER } from "../src/providers"; +import { parseAgentPhase } from "../src/services/questions/parseAgentPhase"; + +// Captured Codex status bars (live PTY probe, Phase 0 findings). +const CODEX_WORKING = "gpt-5.5 medium · /Users/x/dev/tb · gpt-5.5 · medium · Working · Wo…"; +const CODEX_READY = "gpt-5.5 medium · /Users/x/dev/tb · gpt-5.5 · medium · Ready · Wo…"; +const CODEX_STARTING = "gpt-5.5 medium · /Users/x/dev/tb · gpt-5.5 · medium · Starting · Wo…"; + +describe("parseAgentPhase — codex", () => { + it("reports working while the status bar says Working", () => { + expect(parseAgentPhase([CODEX_WORKING], CODEX_CLI_PROVIDER)).toBe("working"); + }); + + it("reports working while MCP servers are Starting", () => { + expect(parseAgentPhase([CODEX_STARTING], CODEX_CLI_PROVIDER)).toBe("working"); + }); + + it("reports no phase when the composer is Ready", () => { + expect(parseAgentPhase([CODEX_READY], CODEX_CLI_PROVIDER)).toBeNull(); + }); + + it("reads the last non-blank line, not the last line", () => { + const screen = ["some output", CODEX_WORKING, "", " "]; + expect(parseAgentPhase(screen, CODEX_CLI_PROVIDER)).toBe("working"); + }); + + // PRE-EXISTING LIMITATION, documented rather than asserted-away. The status + // bar carries the cwd, and CODEX_BUSY_STATUS_RE is /\b(?:Starting|Working)\b/ + // — whose comment claims whole-word matching stops a project path from + // false-hitting. It does not: `/` is a non-word character, so `\b` matches at + // a path boundary and `/Users/x/Working/repo` tests true even on a Ready bar. + // + // This affects codexScreenBlocksComposer() and codexScreenShowsReady() today, + // independently of this feature — a session under a path segment named + // "Working" or "Starting" never reads as ready. Filed separately; this test + // pins current behaviour so the fix has something to flip. + it("inherits the status-bar path false-positive (known, filed separately)", () => { + const screen = ["gpt-5.5 · /Users/x/Working/repo · gpt-5.5 · medium · Ready · Wo…"]; + expect(parseAgentPhase(screen, CODEX_CLI_PROVIDER)).toBe("working"); + }); + + it("reports no phase for an empty screen", () => { + expect(parseAgentPhase([], CODEX_CLI_PROVIDER)).toBeNull(); + expect(parseAgentPhase(["", " "], CODEX_CLI_PROVIDER)).toBeNull(); + }); +}); + +describe("parseAgentPhase — claude", () => { + // The Claude grammar is deliberately unimplemented until its markers are + // re-verified against a fresh PTY capture. Until then it must report no + // phase rather than guess — a wrong phase is worse than none. + it("reports no phase pending marker re-verification", () => { + const footer = "✽ Roosting… (5s · ↓ 82 tokens)"; + expect(parseAgentPhase([footer], CLAUDE_CODE_PROVIDER)).toBeNull(); + }); +}); + +describe("parseAgentPhase — unknown provider", () => { + it("makes no claim rather than falling back to another provider's grammar", () => { + // Codex's own bar, read as some future provider: must not be interpreted. + expect(parseAgentPhase([CODEX_WORKING], "some-future-cli" as never)).toBeNull(); + }); +}); diff --git a/docs/compatibility/tb-mobile.md b/docs/compatibility/tb-mobile.md index 72a41749..37fa0dca 100644 --- a/docs/compatibility/tb-mobile.md +++ b/docs/compatibility/tb-mobile.md @@ -54,6 +54,17 @@ A recovered session's `status` has to report `idle` (it holds no PTY, and a nove This field carries that one bit separately, so a client can say "interrupted mid-response" instead of "idle". Purely additive and safe to ignore: nothing keys off it server-side, and a client that never reads it behaves exactly as it does today. Adopting it is tracked as tb-mobile PR M2. +- Session — new field `subStatus`: the agent's phase *within* a running turn (`"thinking" | "streaming" | "hooks" | "acting" | "working"`), scraped from the rendered PTY screen. +**Unlike every other addition on this list it is NOT optional — it is always serialised, and is `null` when there is no phase.** +That is deliberate and must not be "tidied" into the `...(x != null && { x })` block that the neighbouring optional fields use: `!=` catches `null` as well as `undefined`, so an explicit clear would become an absent key. +Mobile merges session frames (`{...prev, ...next}`), and a merge cannot express a removed key, so an absent field keeps its previous value and the indicator latches on a finished turn — the failure tb-mobile PR #647 shipped. +A client that ignores the field behaves exactly as today; a client that renders it must treat an unrecognised value as "no phase" rather than coercing it, because the union will grow. +It deliberately does **not** add a `SessionStatus` value: `VALID_STATUSES` rejects unknown values and `?status=` filtering would make those sessions vanish from already-shipped apps. +- WebSocket — new event `session_phase`: `{ type, sessionId, phase: AgentPhase | null, updatedAt }`, scoped to that session's subscribers rather than broadcast globally. +Purely additive; a client that never handles it behaves as today, and `subStatus` on the session object stays the source of truth for the GET path and for reconnect. +It is a minimal frame rather than a `SessionResponse` copy on purpose: `managedToResponse` recomputes `elapsedMs` from `new Date()` on every call for a live session, so a session-copy frame would differ on every tick whether or not the phase changed, and a merging client would get a fresh object identity several times a second for the whole turn. +`phase` follows the same always-present/nullable contract as the field. +- **pty-host protocol version 3** (private streamer-to-host protocol; no mobile-visible change): adds the `phase-change` event. The detectors run in the host, so without it the indicator silently no-ops when `THREADBASE_FEATURE_PTY_HOST=1`. - Session — new value on the existing optional `lifecycle` field: `"starting"`, for a managed session this run holds no PTY for and has observed no exit for (no `completedAt`, no `failureReason`). It replaces the `"completed"` those sessions used to report, which made "has not attached yet" and "has ended" the same value on the wire — the ambiguity tb-mobile #508 needs resolved. `status`, `ptyAttached` and every other field are unchanged, and `lifecycle` was already optional, so a client that does not know the value behaves exactly as it does today; adopting it is tracked in tb-mobile #508. diff --git a/src/pty-host/host.ts b/src/pty-host/host.ts index 8cfba57f..df7e86f7 100644 --- a/src/pty-host/host.ts +++ b/src/pty-host/host.ts @@ -87,6 +87,8 @@ export class SessionHost { onReady: (session) => this.emit({ type: "event", event: "ready", session }), onPermissionChange: (sessionId, gate) => this.emit({ type: "event", event: "permission-change", sessionId, gate }), + onPhaseChange: (sessionId, phase) => + this.emit({ type: "event", event: "phase-change", sessionId, phase }), onLiveQuestion: (sessionId, questions) => this.emit({ type: "event", event: "live-question", sessionId, questions }), onLiveQuestionGone: (sessionId) => diff --git a/src/pty-host/protocol.ts b/src/pty-host/protocol.ts index c19c1bba..2a57d356 100644 --- a/src/pty-host/protocol.ts +++ b/src/pty-host/protocol.ts @@ -1,5 +1,6 @@ import type { ProviderName } from "../providers"; import type { + AgentPhase, AskQuestion, ManagedSession, PermissionOption, @@ -43,9 +44,12 @@ import type { /** * Bumped on any incompatible change to the shapes below. Version 2 adds the - * heartbeat and shutdown controls required for host supervision. + * heartbeat and shutdown controls required for host supervision. Version 3 + * adds the `phase-change` event — the detector runs in the host, so without a + * verb here the agent-phase indicator silently stops working when the flag is + * on, which is exactly the failure this file's header warns about. */ -export const PTY_HOST_PROTOCOL_VERSION = 2; +export const PTY_HOST_PROTOCOL_VERSION = 3; export interface HostHeartbeatState { registryState: "known" | "unknown"; @@ -119,6 +123,12 @@ export type HostEvent = cursor?: number; } | null; } + /** + * Agent phase changed within a running turn, including to `null` at turn + * end. Carries the same always-present/nullable contract as the wire field: + * absence must never mean "cleared", because the consumer merges state. + */ + | { type: "event"; event: "phase-change"; sessionId: string; phase: AgentPhase | null } | { type: "event"; event: "live-question"; sessionId: string; questions: AskQuestion[] } | { type: "event"; event: "live-question-gone"; sessionId: string } | { type: "event"; event: "user-message"; sessionId: string; text: string; ts: number } diff --git a/src/pty-host/remote-session-runner.ts b/src/pty-host/remote-session-runner.ts index eed40626..b16884fe 100644 --- a/src/pty-host/remote-session-runner.ts +++ b/src/pty-host/remote-session-runner.ts @@ -288,6 +288,9 @@ export class RemoteSessionRunner implements SessionRunner { case "permission-change": this.options.onPermissionChange?.(event.sessionId, event.gate); break; + case "phase-change": + this.options.onPhaseChange?.(event.sessionId, event.phase); + break; case "live-question": this.options.onLiveQuestion?.(event.sessionId, event.questions); break; diff --git a/src/pty-manager.ts b/src/pty-manager.ts index 627f5414..36bded95 100644 --- a/src/pty-manager.ts +++ b/src/pty-manager.ts @@ -18,7 +18,9 @@ import { questionContentKey, } from "./services/questions/detectQuestionFromScreen"; import { detectShellPrompt } from "./services/questions/detectShellPrompt"; +import { parseAgentPhase } from "./services/questions/parseAgentPhase"; import type { + AgentPhase, ManagedSession, PTYManagerOptions, SessionRunner, @@ -211,6 +213,7 @@ export class PTYManager implements SessionRunner { private onStatusChange: PTYManagerOptions["onStatusChange"]; private onReady: PTYManagerOptions["onReady"]; private onPermissionChange: PTYManagerOptions["onPermissionChange"]; + private onPhaseChange: PTYManagerOptions["onPhaseChange"]; private onLiveQuestion: PTYManagerOptions["onLiveQuestion"]; private onLiveQuestionGone: PTYManagerOptions["onLiveQuestionGone"]; private onUserMessage: PTYManagerOptions["onUserMessage"]; @@ -273,6 +276,7 @@ export class PTYManager implements SessionRunner { this.onStatusChange = options.onStatusChange; this.onReady = options.onReady; this.onPermissionChange = options.onPermissionChange; + this.onPhaseChange = options.onPhaseChange; this.onLiveQuestion = options.onLiveQuestion; this.onLiveQuestionGone = options.onLiveQuestionGone; this.onUserMessage = options.onUserMessage; @@ -941,6 +945,16 @@ export class PTYManager implements SessionRunner { const lines = await this.getOutputLines(sessionId, 60); + // Phase refinement, off the screen read the detectors below already share. + // Only while the turn is running: the phase describes what the agent is + // doing *within* `running`, and markReady clears it at turn end. Reading it + // in any other state would let a stale screen re-assert a phase after the + // clear, which is the latch this design exists to prevent. + const phaseSession = this.sessions.get(sessionId); + if (phaseSession?.status === "running") { + this.setPhase(sessionId, phaseSession, parseAgentPhase(lines, CLAUDE_CODE_PROVIDER)); + } + // Authoritative footer test on the FULL rendered screen (not just the // trigger chunk). The "Enter to select · Tab/Arrow keys to navigate" footer // is unique to an AskUserQuestion menu; a permission gate uses "Tab to amend @@ -1165,6 +1179,21 @@ export class PTYManager implements SessionRunner { // Transition a session from "running" to "waiting_input", clear pendingReady, // and flush any queued input. Idempotent: callers can invoke at any chunk. + /** + * Record the agent's phase and notify only on a real change. + * + * The change guard is load-bearing, not an optimisation: the scrape pass runs + * on every chunk, so an unguarded setter would fire the callback — and the + * WS frame behind it — several times a second for the entire duration of a + * turn while reporting the same value. + */ + private setPhase(sessionId: string, session: InternalSession, phase: AgentPhase | null): void { + const next = phase ?? null; + if ((session.subStatus ?? null) === next) return; + session.subStatus = next; + this.onPhaseChange?.(sessionId, next); + } + private markReady( sessionId: string, session: InternalSession, @@ -1174,6 +1203,15 @@ export class PTYManager implements SessionRunner { this.clearReadyFallback(sessionId); session.lastActivityAt = new Date(); session.status = "waiting_input"; + // Turn end clears the phase. This is the only place it can happen + // correctly: the exit edge is not an output event, so it cannot be read off + // the screen — Claude's TUI does differential repaints and a return to idle + // can go undetected forever if the last chunk didn't carry the marker. + // Without this the phase latches on any session that stops emitting, which + // is the bug tb-mobile PR #647 shipped. markReady is the single idempotent + // running -> waiting_input transition, and it is driven by the waiting-for- + // input OSC, which arrives even when no further chunk will. + this.setPhase(sessionId, session, null); // C3: record HOW we concluded this, not just that we did. A // `timeout-fallback` here means no marker ever appeared and we assumed — // previously indistinguishable on the wire from an observed marker. diff --git a/src/server.ts b/src/server.ts index 461064e0..a0bbdd38 100644 --- a/src/server.ts +++ b/src/server.ts @@ -926,6 +926,18 @@ export class StreamerServer { seq, }); }, + onPhaseChange: (sessionId, phase) => { + // Scoped to this session's subscribers and sent as a minimal frame — + // NOT routed through onStatusChange's handler, which writes a DB row, + // refreshes the scanner index, broadcasts globally and pokes the APNs + // and push notifiers on every call. This can fire every scrape tick. + this.wsHub.broadcastToClients(this.sessionSubscribers.get(sessionId) ?? [], { + type: "session_phase", + sessionId, + phase, + updatedAt: new Date().toISOString(), + }); + }, onUserMessage: (sessionId, text, ts) => { this.wsHub.broadcastToClients(this.sessionSubscribers.get(sessionId) ?? [], { type: "user_message", diff --git a/src/services/questions/parseAgentPhase.ts b/src/services/questions/parseAgentPhase.ts new file mode 100644 index 00000000..499978cf --- /dev/null +++ b/src/services/questions/parseAgentPhase.ts @@ -0,0 +1,65 @@ +// Agent-phase detection. Answers "what is the agent doing right now" for a +// session whose status is already `running` — a refinement of that status, not +// a state with its own lifecycle. +// +// Pure — no I/O. Operates on rendered screen lines (getOutputLines), because +// the TUI paints its footer with absolute-cursor moves and it does not exist as +// a contiguous run of bytes in the raw PTY stream. This is the same constraint +// parseStatusLine documents, and the reason mobile's own attempt at this +// (tb-mobile PR #647) failed: it searched a client-side emulator that resolves +// absolute cursor moves against the wrong rows. +// +// Returns `null` when no phase is recognised. Callers must treat that as "no +// phase", never as "unchanged" — a derive that holds its previous value is how +// an indicator latches on a finished turn. +// +// Why phase is readable from a repainted footer at all, when parseStatusLine +// deliberately refuses to forward the elapsed counter from that same line: the +// counter is a continuous function of wall-clock time sampled at output events, +// so between samples it is simply wrong and the error grows without bound. A +// phase is a step function whose transitions ARE output events — the agent +// starts streaming *because* tokens began painting — so sampling at output +// events is exact for it, not approximate. The one exception is the exit edge, +// which is not an output event; that is why the phase is cleared out-of-band in +// markReady() rather than inferred from the screen going quiet. + +import { CODEX_BUSY_STATUS_RE, codexStatusBarLine } from "../../codex-pty-runner"; +import type { ProviderName } from "../../providers"; +import { CLAUDE_CODE_PROVIDER, CODEX_CLI_PROVIDER } from "../../providers"; +import type { AgentPhase } from "../../types"; + +/** + * Codex's status bar is binary: a turn walks Ready → Working → Ready with no + * intermediate state observed on a live PTY probe, so reporting anything finer + * would be invention. `working` is the only phase this provider can support. + */ +function codexPhase(lines: string[]): AgentPhase | null { + return CODEX_BUSY_STATUS_RE.test(codexStatusBarLine(lines)) ? "working" : null; +} + +/** + * Claude's footer carries a richer phase, but the marker set has not yet been + * re-verified against a fresh PTY capture — the candidate grammar was derived + * from two captured turns, and the distinction it rests on (output-token `↓` + * markers, which parseStatusLine does not read, versus the input-token `↑` + * counter, which it deliberately rejects) is too load-bearing to build on + * unconfirmed. Reporting no phase is correct until that lands: the indicator + * simply does not render, which is the pre-feature behaviour. + */ +function claudePhase(_lines: string[]): AgentPhase | null { + return null; +} + +/** + * Derive the agent's phase from a rendered screen. + * + * An unrecognised provider yields `null` rather than falling back to a + * provider-specific grammar. `getTerminalChromeFilter` already establishes that + * instinct — "prefer passthrough over wrong Claude filters" — and the opposite + * default is what let a wrong grammar run against the wrong provider in #647. + */ +export function parseAgentPhase(lines: string[], provider: ProviderName): AgentPhase | null { + if (provider === CODEX_CLI_PROVIDER) return codexPhase(lines); + if (provider === CLAUDE_CODE_PROVIDER) return claudePhase(lines); + return null; +} diff --git a/src/session-store.ts b/src/session-store.ts index 374f651d..71c4103c 100644 --- a/src/session-store.ts +++ b/src/session-store.ts @@ -278,6 +278,13 @@ function managedToResponse(s: ManagedSession, ptyAttached: boolean): SessionResp promptCount: s.promptCount, startedAt: s.startedAt.toISOString(), completedAt: s.completedAt?.toISOString() ?? null, + // Unconditional, like completedAt above — do NOT move this into the + // `...(x != null && { x })` block below. That guard uses loose `!=`, which + // catches null as well as undefined, and would turn an explicit "no phase" + // back into an absent key. The client merges session frames, so an absent + // key keeps the previous value and the indicator latches on a finished + // turn — the tb-mobile PR #647 bug, arriving through the serialiser. + subStatus: s.subStatus ?? null, ptyAttached, ...(s.projectId != null && { projectId: s.projectId }), ...(s.sessionName != null && { sessionName: s.sessionName }), @@ -329,6 +336,10 @@ function discoveredToResponse(d: DiscoveredProcess, conversationId: string): Ses // cannot see the process's prompt state. lifecycle: "detached", lifecycleSource: "probe", + // No PTY here, so nothing to scrape and no phase to report. Emitted + // explicitly rather than omitted, for the same reason as in + // managedToResponse: absence must never be a third state on the wire. + subStatus: null, projectPath: d.projectPath, projectName: d.projectName, branch: d.branch, diff --git a/src/types.ts b/src/types.ts index 4d9e1a93..7379ed5c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -8,6 +8,25 @@ import type { ProviderName } from "./providers"; export type SessionStatus = "running" | "waiting_input" | "idle"; +/** + * Phase axis *inside* `status === "running"` — what the agent is doing during a + * turn. Deliberately a separate field rather than new SessionStatus members: + * VALID_STATUSES rejects unknown values and the store drops sessions outside + * the requested set, so a new status string would make those sessions vanish + * from already-shipped apps. Additive fields are safe; additive values in a + * union a shipped client filters on are not. + * + * The full set is defined here even though Codex only ever emits `working` + * (its status bar is binary — Ready/Working, and claiming otherwise would be + * invention). Defining it up front keeps a two-valued provider from fixing the + * field's shape before Claude's richer footer lands. Consumers must ignore an + * unrecognised value rather than coerce it. + * + * This union lives in exactly one place. Two independently-maintained copies of + * a TUI-derived grammar have already drifted once (tb-mobile PR #647). + */ +export type AgentPhase = "thinking" | "streaming" | "hooks" | "acting" | "working"; + /** * Process-lifetime axis for a managed session (C1 durable session runtime). * Orthogonal to SessionStatus — see SessionResponse.lifecycle for why the two @@ -93,6 +112,15 @@ export interface ManagedSession { */ statusSource?: StatusSource; statusUpdatedAt?: Date; + /** + * Agent phase within a running turn, scraped from the rendered screen. + * Optional internally (every existing construction site predates it), but + * managedToResponse emits it unconditionally as `?? null` — on the wire + * absence must never be a third state, because the client merges session + * frames and a merge cannot express a removed key. + * Cleared in markReady(), the one turn-end transition. + */ + subStatus?: AgentPhase | null; filePath?: string; resumedFromConversationId?: string; @@ -231,6 +259,28 @@ export type WSMessage = stalledSinceMs?: number; reworkAttempt?: number; } + /** + * Agent phase changed within a running turn. Scoped to that session's + * subscribers, like terminal_output and user_message. + * + * A minimal frame rather than a SessionResponse copy, deliberately: + * managedToResponse recomputes `elapsedMs` from `new Date()` on every call + * for a live session, so a session copy would differ on every tick whether + * or not the phase changed — and a client that merges frames would get a + * fresh object identity several times a second, re-rendering every consumer + * for the whole turn. + * + * `phase` is always present and is `null` when there is no phase. Absence + * must never carry meaning: clients merge session state, and a merge cannot + * express a removed key, so an omitted field would keep its previous value + * and the indicator would latch on a finished turn. + */ + | { + type: "session_phase"; + sessionId: string; + phase: AgentPhase | null; + updatedAt: string; // ISO 8601 + } | { type: "session_list"; sessions: readonly SessionResponse[] } | { type: "conversation_event"; sessionId: string; line: string } // Additive batched variant: one message carries all lines from a single @@ -392,6 +442,20 @@ export interface SessionResponse { * Live sessions only. */ permissionMode?: string; + /** + * Agent phase within a running turn, scraped from the rendered PTY screen. + * + * NOT optional, and always serialised — `null` when there is no phase. A + * client that merges session frames (`{...prev, ...next}`) cannot express a + * removed key, so an omitted field would keep its previous value and the + * indicator would latch on a finished turn. That is the bug tb-mobile PR #647 + * shipped; absence must never carry meaning here. + * + * Consequently this must NOT be moved into the `...(x != null && { x })` + * guard block in managedToResponse: `!= null` catches null and undefined + * alike and would convert an explicit clear back into absence. + */ + subStatus: AgentPhase | null; account?: string; messageCount?: number; preview?: string; @@ -575,6 +639,24 @@ export interface PTYManagerOptions { cursor?: number; } | null, ) => void; + /** + * Fired when the agent's phase within a running turn changes, including to + * `null` at turn end. Additive; absent in tests that omit it. + * + * Deliberately NOT routed through onStatusChange, even though that callback + * already exists and is already relayed across the pty-host boundary. Its + * handler writes a DB row per invocation with no same-status guard, refreshes + * the scanner index, broadcasts globally, and pokes the APNs and push + * notifiers — machinery built for a handful of transitions per session, not + * for a signal that can fire every SCRAPE_THROTTLE_MS. + * + * The server must broadcast this to that session's subscribers only + * (wsHub.broadcastToClients), as a minimal frame rather than a SessionResponse + * copy: managedToResponse recomputes elapsedMs on every call, so a session + * copy would differ every tick and re-render every client consumer of that + * session for the whole turn. + */ + onPhaseChange?: (sessionId: string, phase: AgentPhase | null) => void; // Fired when an AskUserQuestion menu is detected on the rendered screen (before // the JSONL tool_use block flushes). The server de-dupes against the JSONL path. onLiveQuestion?: (sessionId: string, questions: AskQuestion[]) => void; From b36c9f3232b65dd89193ca4c9ab4c1a7101b599a Mon Sep 17 00:00:00 2001 From: Ronen Mars Date: Wed, 12 Aug 2026 21:19:10 +0300 Subject: [PATCH 2/2] test(session): drop non-null assertions from the agent-phase contract tests Replaces the `!` assertions with a `responseFor` helper that throws on a missing session, so a broken store fails with a named error rather than a TypeError on a null property. Verified the tests still discriminate afterwards: moving `subStatus` into managedToResponse's `!= null` guard block fails two, and deleting the `phase-change` relay case fails two. Refs #538 --- __tests__/agent-phase-contract.test.ts | 36 ++++++++++++++------------ 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/__tests__/agent-phase-contract.test.ts b/__tests__/agent-phase-contract.test.ts index dfc08b43..473207ec 100644 --- a/__tests__/agent-phase-contract.test.ts +++ b/__tests__/agent-phase-contract.test.ts @@ -14,7 +14,7 @@ import { } from "../src/pty-host/protocol"; import { RemoteSessionRunner } from "../src/pty-host/remote-session-runner"; import { SessionStore } from "../src/session-store"; -import type { ManagedSession, PTYManagerOptions } from "../src/types"; +import type { ManagedSession, PTYManagerOptions, SessionResponse } from "../src/types"; const STARTED = new Date("2026-08-12T10:00:00Z"); @@ -56,6 +56,15 @@ function mkHost() { return { transport, emit: (e: HostEvent) => onLine(encodeMessage(e)) }; } +/** Serialise one session and hand back a definitely-present response. */ +function responseFor(over: Partial): Readonly { + const store = new SessionStore(); + store.addManaged(mkSession(over)); + const resp = store.get("sess-1", new Set()); + if (resp === null) throw new Error("session was not stored"); + return resp; +} + describe("agent phase — wire contract", () => { // The load-bearing one. managedToResponse guards ~19 optional fields with // `...(x != null && { x })`, and `!=` catches null as well as undefined. If @@ -64,33 +73,28 @@ describe("agent phase — wire contract", () => { // key keeps its previous value. That is how tb-mobile PR #647's indicator // latched onto finished turns. it("serialises an explicit null rather than omitting the key", () => { - const store = new SessionStore(); - store.addManaged(mkSession({ subStatus: null })); + const resp = responseFor({ subStatus: null }); - const resp = store.get("sess-1", new Set()); - - expect(resp).not.toBeNull(); - expect("subStatus" in resp!).toBe(true); - expect(resp!.subStatus).toBeNull(); + expect("subStatus" in resp).toBe(true); + expect(resp.subStatus).toBeNull(); // The distinction a merge cannot express: absent !== null. expect(JSON.stringify(resp)).toContain('"subStatus":null'); }); it("serialises a set phase", () => { - const store = new SessionStore(); - store.addManaged(mkSession({ subStatus: "working" })); - expect(store.get("sess-1", new Set())!.subStatus).toBe("working"); + expect(responseFor({ subStatus: "working" }).subStatus).toBe("working"); }); it("emits null for a session that predates the field", () => { const store = new SessionStore(); - const s = mkSession(); - delete (s as Partial).subStatus; - store.addManaged(s); + const stored = mkSession(); + delete (stored as Partial).subStatus; + store.addManaged(stored); const resp = store.get("sess-1", new Set()); - expect("subStatus" in resp!).toBe(true); - expect(resp!.subStatus).toBeNull(); + if (resp === null) throw new Error("session was not stored"); + expect("subStatus" in resp).toBe(true); + expect(resp.subStatus).toBeNull(); }); });