diff --git a/desktop/src/i18n/index.ts b/desktop/src/i18n/index.ts index 1a606d05..932a0dec 100644 --- a/desktop/src/i18n/index.ts +++ b/desktop/src/i18n/index.ts @@ -465,6 +465,13 @@ const en: Dict = { 'approval.attn.permission_prompt': 'Permission needed', 'approval.attn.select': 'Choose', 'approval.attn.help_request': 'Help needed', + // Host-runner's agent-state rows: the legacy stall detector, and lane P's + // manifest classifier when a pane shows a blocked dialog. Labelled for what + // the reader must do (nothing but look), not for the classification — the + // summary carries "blocked". Without this line the card's kind label falls + // back to the raw wire value and the dock shows a blocked agent under the + // word "idle". + 'approval.attn.idle': 'Waiting at a prompt', // D3: the desktop screenshot card. No "Allow session" button exists for it — // this line says why, so the missing option reads as a decision. 'att.perCallOnly': 'Screenshots are approved one call at a time — there is no standing grant.', @@ -2616,6 +2623,7 @@ const zh: Dict = { 'approval.attn.permission_prompt': '需要授权', 'approval.attn.select': '请选择', 'approval.attn.help_request': '需要协助', + 'approval.attn.idle': '等待输入', 'att.perCallOnly': '截屏需逐次批准，不提供长期授权。', 'kanban.todo': '待办', diff --git a/docs/changelog.md b/docs/changelog.md index 0cd4ff57..366d0926 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -43,6 +43,32 @@ binding). Seed entries prior to that are in ### Added +- **A blocked pane now raises attention, and withdraws it again.** When + the pane-state classifier matches a rule that says *blocked* AND + reports a dialog visible on screen, host-runner opens one + attention row for that blocked streak — carrying the matched rule id, + the manifest and its version, and the agent + pane, but never any pane + text (evidence is a rule id; an attention row fans out further than a + transcript, and the pane may be showing a secret). When the + classification leaves blocked — because the human answered the dialog + in the terminal, where nothing would otherwise tell the hub — the row + is resolved, as it is when the agent stops running. It is the first + host-runner row that retracts itself. This closes the concrete gap the + lane opened on: codex sitting at "Allow command?" raised nothing at + all before. The row's kind stays `idle`, deliberately: on that surface + the kind picks the affordance, and it is the only value both clients + already route to acknowledge-only — a new kind would have inherited + the unknown-kind default, which on mobile draws Approve / Reject for a + state report nothing can approve. +- **Idle panes are no longer captured when nothing has happened in + them.** Host-runner reads tmux's `#{window_activity}` stamp in the + same single `list-panes` round-trip it already makes for pane titles, + and skips the `capture-pane` subprocess for a pane whose published + state is idle and whose window has produced no output since the last + read. Blocked and working panes are always re-read, so a stale stamp + can never freeze the state this detector exists to report. + Plan: pane-state-manifests P3. + - **Pane-state detection is live on the host-runner poll tick.** The evaluator below is now fed from the runner's pane pass: mapped families whose pane has no live state-authoring driver are captured, classified @@ -128,6 +154,37 @@ binding). Seed entries prior to that are in because the argv the driver uses is threaded per turn, not spliced at spawn. Pane-state-manifests plan, N1. +### Changed + +- **The legacy idle detector's guard now names its own reason.** + `hasStructuredDriver(kind)` becomes `hasAnyStateAuthority(agent)` — a + live structured driver, or manifest coverage, or a registered engine + family. The set of agents it scrapes is unchanged (every mapped family + is a registered one, so mapped panes were already excluded) and a + sweep test now asserts that instead of leaving it a coincidence. + Deliberately conservative: the guard still exempts + registered-but-unmapped families such as `kimi-code-ts`, so + retirement can only shrink the legacy detector's reach, never grow it. + Plan: pane-state-manifests P3. + +### Fixed + +- **A host-runner-raised attention row was attributed to the host, never + to the agent that asked.** `POST /attention` honoured a body-supplied + `actor_handle` only when the authenticated caller had no handle of its + own — a condition that can never hold, because `principalFromScope` + falls back to `"@principal"` for an absent handle, an absent role and + unparseable scope JSON alike. So the branch was unreachable, and every + row the codex approval bridge has raised since ADR-012 D3 recorded + `actor_kind=host` plus the host token's principal instead of + `agent` plus the agent's handle (the stored kind is the caller's + token kind verbatim, and host-runners run under `host` tokens — an + audit of the affected rows should filter on `actor_kind='host'`). The condition is now the token + *kind*: only a `host` token may name someone else, so an agent's own + token still loses to its context identity. Found by testing the new + pane-state row against the real handler instead of a stub; neither + client renders these two columns yet, which is why it went unnoticed. + ### Security - **A session id from the engine could inject shell into the next diff --git a/docs/plans/pane-state-manifests.md b/docs/plans/pane-state-manifests.md index ef20ee0e..b60d5c8c 100644 --- a/docs/plans/pane-state-manifests.md +++ b/docs/plans/pane-state-manifests.md @@ -16,9 +16,11 @@ > push channel). P/N/S/Q are unused elsewhere: **P** reads the pane, > **Q** writes to it, **N** is native resume, **S** is settle semantics. > **Audience:** principal · contributors -> **Last verified vs code:** main `e498416d` (2026-08-08; -> `hub/internal/hostrunner/idle.go`, `driver_pane.go`, -> `hub/internal/agentfamilies/`, `docs/reference/attention-kinds.md`) +> **Last verified vs code:** main `9e06d8fa` (2026-08-09; P1/Q1/P2 +> shipped and P3 built against it — `hub/internal/panestate/`, +> `hub/internal/hostrunner/panestate_watch.go`, `idle.go`, `runner.go`, +> `hub/internal/server/handlers_attention.go`, +> `docs/reference/attention-kinds.md`; herdr re-read at `6f311498`) **TL;DR.** Host-runner can only say "this agent needs you" for the three engines with structured M4 adapters; everything else gets @@ -88,6 +90,33 @@ pane-input hardening (`paste-buffer -p`, generic multi-line path). structured-authority pane MAY raise attention (screen shows a live permission dialog the hooks never reported). It adjusts attention only, never the session's driver-authored state. + - **Corrected at P3 (2026-08-09): the exception is not a port, and + it is deferred.** Upstream has no such case — `pane.rs:809` is + `if lifecycle_authority_active && !process_exited { pending_idle + .clear(); continue; }`, an unconditional short-circuit *before* + the screen is read, and nothing downstream consults + `visible_blocker` for a pane it skipped. This is a termipod + invention wearing a port's clothes, so it has to earn its place on + its own evidence. It needs two things first, neither available + today: + 1. **Proof it complements rather than duplicates.** The target + case is claude's trust dialog, which is hook-blind. But + `claude.toml` also ships `bash_permission_prompt` and + `generic_permission_prompt` (both `visible_blocker`), and our + claude agents already raise `permission_prompt` rows from the + `canUseTool` hook for those events — with Approve/Deny that + work. Whether the TUI *draws* a dialog the hook has already + parked decides whether this exception adds a signal or a + second, un-actionable row beside the right one. Nobody has + watched a real claude pane (this lane's standing device-verify + debt), and static reading cannot settle it. + 2. **A suppression rule if it does duplicate** — "no row while one + is open for this agent" needs a hub query host-runner does not + have (`handleListAttention` filters on status and scope_kind + only, never actor). + One capture of a claude pane mid-permission-prompt settles both. + Until then P3 ships the safe half: panes with no state authority, + where there is no other row to collide with. - **D-3 — engine-kind mapping lives in the overlay, not in vendored files.** herdr ids (`claude`, `kimi`, `gemini`) differ from our family names (`claude-code`, `kimi-code-ts`, `gemini-cli`). A @@ -170,6 +199,24 @@ pane-input hardening (`paste-buffer -p`, generic multi-line path). `lifecycle`, since it is an agent state transition rather than turn telemetry, and on a raw pane it is often the only structured signal a reader has. + - **Answered at P3 (2026-08-09): the attention kind stays `idle`, + and for a reason that is not "no new kind was needed".** D-6 left + the door open to minting one if `idle` read wrong for "blocked on + approval". It does read wrong — this lane spent P1 making `idle` + and `blocked` contrasting states — but the kind on this surface + selects an *affordance*, and `idle` is the only value both clients + already route correctly for a row a human can acknowledge but not + answer: mobile buckets it under Agents with a single Dismiss + (`me_screen.dart` `_filterForAttention`, `inline_actions.dart` + `_isInformational`), and the hub keeps it out of + `attentionAwaitsAgentReply`, which is what makes `/resolve` — the + retract leg — legal at all. A newly minted kind inherits the + unknown-kind default instead, and on mobile that default is + **Approve / Reject** for any row carrying a `pending_payload`: + two buttons on a state report nothing can approve. Same hazard P2 + found in the event feed, second registry, opposite direction. The + collision is contained to the wire name; summary, payload, and the + `pane_state` event all say blocked. - **D-7 — distribution starts embedded, hub later.** P1 embeds vendor + overlay via `go:embed`; binary upgrades ship rule fixes. P5 adds hub-distributed updates with herdr's exact hardening: @@ -335,6 +382,65 @@ takes a codex approval screen → attention item with rule id; the idle-shell false-positive class (bare `$` prompt) provably cannot raise; sweep test: no remaining `IdleDetector` path for mapped kinds. +**As built (2026-08-09).** Attention raise + retract, the capture +gate, and the guard rewrite landed; the D-2 exception did not (see +D-2's own correction above — it is not a port, and settling it needs +one real claude pane). All three acceptance clauses are met, by +`TestBlockedScreenRaisesAttentionWithRuleID`, +`TestBareShellPromptCannotRaiseAttention` and +`TestIdleDetectorSkipsEveryMappedFamily`. Four things worth carrying +forward: + +- **The retirement was already done, and the guard this line + proposes would have UNDONE part of it.** Every mapped family is a + registered agent family, and the old guard skipped every registered + family — so `IdleDetector` already never touched a mapped pane. + Replacing it with "has any state authority" *literally* would have + handed the legacy regex the registered-but-unmapped families, + `kimi-code-ts` above all: deliberately unmapped, and an instance + whose M4 launch fell back to a raw pane has no authority of either + sort. That is the W11 TUI-prompt false positive, re-opened by a + wedge whose job was to close things. `hasAnyStateAuthority` keeps + the registered-family clause as its third limb, so the legacy set + only ever contracts. The real change here is precision, not + coverage — plus the sweep test that turns "already disjoint" from a + coincidence into an assertion. +- **The `covers()` clause of that guard is dead today, and says so.** + A mutation deleting it survives the entire suite, because clause 3 + subsumes it. It is kept as the clause that names the actual reason, + with the subsumption written down and pinned to the test that would + fail if the overlay ever mapped an unregistered family. Recording a + shadowed guard is better than pretending a test covers it. +- **`#{window_activity}` is sound, with one sharp edge.** tmux calls + `window_update_activity()` from `input_parse_buffer()` + (tmux 3.4 `input.c:975`) on every non-empty chunk of pane output, + independent of `monitor-activity` — that option only gates the + alert. But it is per-WINDOW (tmux 3.4 has no `pane_activity` + format) and one-second resolution, so output landing later in the + same second as the stamp we read is invisible to an equality test — + and for an idle pane that skip would repeat forever. The gate + therefore arms only on a stamp whose second had already elapsed when + we captured (`now.Unix() > activity`), which makes equality sound + rather than probabilistic. Skipping is also confined to panes whose + published state is idle, exactly as upstream confines it + (`should_skip_idle_screen_scan`, agent_detection.rs:91) — a stale + stamp can never freeze a blocked pane. +- **Attention is decided on every classified tick, not on the + transition.** Deciding it inside the publish branch makes a failed + raise permanent: the streak's transition has already happened, so + the retry tick has nothing to publish and never looks again. + +Also landed: `listTmuxPaneTitles` became `listTmuxPaneMeta` (title + +activity in the one round-trip P2's note reserved for it), and +`internal/panestate/region.go`'s `Input.Screen` comment lost the last +copy of the retracted 24-row claim. + +19 new/changed tests; 8 mutations introduced, 7 caught, 1 documented +above as shadowed. + +**Still owed by this wedge:** the D-2 exception, and the device-verify +line it is blocked on. + ### P4 — explain verb + Inspect surface `host.pane_explain` host verb returns the herdr-style evaluation diff --git a/docs/reference/attention-kinds.md b/docs/reference/attention-kinds.md index 9a7a8648..d869912e 100644 --- a/docs/reference/attention-kinds.md +++ b/docs/reference/attention-kinds.md @@ -3,7 +3,7 @@ > **Type:** reference > **Status:** Current (2026-06-05) > **Audience:** contributors (humans + AI agent maintainers) -> **Last verified vs code:** 2026.730.1231 (D3 introduced `desktop_action`; D5 adds the hub-raised leg) +> **Last verified vs code:** 2026.805.1022 (P3 gives `idle` a second raiser — the pane-state classifier — and its first self-retracting leg) **TL;DR.** When an agent needs the principal to weigh in, it picks one of three interaction shapes — `approval_request` (binary), `select` @@ -49,7 +49,27 @@ Two more attention kinds exist but are not agent-callable (plus reviews a structured diff, not a free-text reply. Use that tool when the right artifact is the template body itself. - `idle` — emitted by host-runner when an agent is paused awaiting - input. State signal, not a request. + input. State signal, not a request. **Two detectors raise it**, and a + reader tells them apart by `pending_payload`, not by the kind: + - the legacy stall heuristic (`idle.go`) — one prompt regex plus a + 90 s content-hash stall, for agents with no state authority at + all. No payload. + - the pane-state manifest classifier (`panestate_watch.go`, plan + P3) — a vendored per-engine rule matched a **drawn blocking + dialog**. `pending_payload.detector` is `"panestate"` and carries + `{state: "blocked", rule_id, manifest_id, manifest_version, + agent_id, pane}`. Never screen text: the evidence is a rule id, + because an attention row fans out further than a transcript does + and the pane may be showing a secret. Raised once per blocked + streak and **withdrawn via `/resolve`** when the classification + leaves blocked — the only host-runner row that retracts itself. + + A blocked agent therefore arrives under the kind named `idle`. That + is deliberate: on this surface the kind selects the affordance (both + clients route `idle` to acknowledge-only), and no kind that means + "blocked" would have inherited it — an unrecognized kind carrying a + `pending_payload` draws Approve / Reject on mobile, for a row nothing + can approve. The summary and payload carry the classification. ### The gated sibling — `browser_action` diff --git a/hub/internal/hostrunner/client.go b/hub/internal/hostrunner/client.go index 5f59ea7d..c9b6b516 100644 --- a/hub/internal/hostrunner/client.go +++ b/hub/internal/hostrunner/client.go @@ -264,6 +264,27 @@ func (c *Client) PostAttention(ctx context.Context, in AttentionIn) (AttentionOu return out, err } +// ResolveAttention closes an open row WITHOUT fanning a reply to any agent — +// the hub's dismiss path (`/resolve`), not the decision path (`/decide`). +// +// host-runner uses it to retract a row whose cause is gone: a pane-state +// `blocked` streak that ended because the human answered the dialog in the +// terminal (plan P3). Nothing is owed to the agent — the row was a state +// report, so there is no parked turn to wake. +// +// The hub REFUSES /resolve for kinds that owe a waiting agent a reply +// (`attentionAwaitsAgentReply`), which is one more reason a detector-raised +// row must not borrow one of those kinds. +// +// A 409 means the row was already resolved — the director dismissed it before +// we noticed the pane moved on. That is the normal race, not a fault: callers +// log at debug and drop the id either way. +func (c *Client) ResolveAttention(ctx context.Context, id string) error { + return c.do(ctx, http.MethodPost, + fmt.Sprintf("/v1/teams/%s/attention/%s/resolve", c.Team, id), + map[string]any{}, nil) +} + type AgentPatch struct { Status *string `json:"status,omitempty"` PauseState *string `json:"pause_state,omitempty"` diff --git a/hub/internal/hostrunner/driver_appserver.go b/hub/internal/hostrunner/driver_appserver.go index 3144815f..e07e4f18 100644 --- a/hub/internal/hostrunner/driver_appserver.go +++ b/hub/internal/hostrunner/driver_appserver.go @@ -50,6 +50,17 @@ type AttentionPoster interface { PostAttention(ctx context.Context, in AttentionIn) (AttentionOut, error) } +// AttentionResolver is the retract half: it closes a row this host-runner +// raised, once the condition that justified it is gone. Only a raiser that +// keeps watching its own condition can implement this honestly — the codex +// bridge above cannot (its rows are answered, not withdrawn), the pane-state +// watcher can (plan P3: a blocked streak ends when the screen stops saying +// blocked). Split from AttentionPoster so the two capabilities are asked for +// separately rather than one interface implying the other. +type AttentionResolver interface { + ResolveAttention(ctx context.Context, id string) error +} + // pendingApproval tracks one server-initiated approval request that // has been bridged to an attention_items row but not yet resolved. // jsonRPCID is the parked codex request id we'll respond on; method diff --git a/hub/internal/hostrunner/panestate_watch.go b/hub/internal/hostrunner/panestate_watch.go index d89b6627..eb1d6454 100644 --- a/hub/internal/hostrunner/panestate_watch.go +++ b/hub/internal/hostrunner/panestate_watch.go @@ -1,32 +1,38 @@ // Declarative pane-state classification, wired into the host-runner's poll -// tick (docs/plans/pane-state-manifests.md lane P, wedge P2). +// tick (docs/plans/pane-state-manifests.md lane P, wedges P2 + P3). // // `internal/panestate` (P1) is a pure library: screen in, classification out. // This file is everything around it — which panes are eligible, where the // screen and the OSC title come from, the debounce/hysteresis/startup-grace -// state machine, and the agent event a transition becomes. +// state machine, the agent event a transition becomes, and (P3) the attention +// row a blocked streak opens and later withdraws. // // Where it runs, and why not in PaneDriver // ---------------------------------------- // The plan's P2 line says "feed the evaluator from `PaneDriver`'s tick". It -// lives in the runner's pane tick instead, because three of the plan's own +// lives in the runner's pane tick instead, because two of the plan's own // decisions cannot be satisfied from inside a driver: // // - D-3 needs the agent's family. PaneDriver only knows an agent id and a // pane id; the family lives on the hub's agent row. // - D-4 wants ONE `list-panes -F` round-trip covering all panes for the -// OSC title. A per-driver call is one round-trip per agent. -// - D-2's ported exception (a visible blocker on a pane whose adapter DOES -// author state may still raise attention, P3) is about panes PaneDriver -// does not own and cannot see. -// -// The runner tick already walks every running pane for the IdleDetector -// that P3 retires, so this adds no new enumeration — and the two are -// disjoint by construction (see paneStateWatch.tick). +// OSC title (and, since P3, the activity stamp). A per-driver call is one +// round-trip per agent. +// +// The runner tick already walks every running pane for the stall detector, so +// this adds no new enumeration — and the two are disjoint by construction +// (see paneStateWatch.tick and Runner.hasAnyStateAuthority). +// +// D-2's structured-authority exception is NOT here, and P3 records why: it is +// not a port (upstream short-circuits on `lifecycle_authority_active` before +// reading the screen), and whether it would complement or duplicate claude's +// hook-raised permission_prompt rows cannot be settled without watching a real +// pane. See the plan's D-2 correction. package hostrunner import ( "context" + "encoding/json" "log/slog" "time" @@ -51,6 +57,40 @@ const PaneStateEventKind = "pane_state" const paneStateEventProducer = "system" +// paneStateAttentionKind is the attention kind a blocked classification +// raises (P3). D-6 left the choice open — "no new attention kind unless P3 +// review finds `idle` semantically wrong for 'blocked on approval'" — and the +// review's answer is: reuse `idle`, because the KIND on this surface is a +// routing-and-affordance token, not the classification. +// +// `idle` is the only kind both clients already route correctly for a row a +// human can acknowledge but not answer: +// +// - mobile buckets it under Agents and renders a single Dismiss +// (`me_screen.dart` _filterForAttention, `inline_actions.dart` +// _isInformational). Its kind test runs BEFORE the pending_payload test, +// so attaching evidence below does not flip it into Requests. +// - the hub keeps it out of `attentionAwaitsAgentReply`, so /resolve accepts +// it — which is what makes the retract leg legal at all. +// +// A newly minted kind would have inherited the unknown-kind default instead: +// on mobile, a row carrying a pending_payload falls into Requests and draws +// **Approve / Reject** for a state report nothing can approve. That is the +// same unknown-kind hazard P2 found in the event feed, in a second registry — +// the affordance defaults are per-surface, and neither defaults to silence. +// +// The cost is the term collision this lane spent P1 avoiding: `idle` and +// `blocked` are contrasting states in `internal/panestate`, and the row we +// raise for `blocked` is kind `idle`. It is contained to the wire name — the +// summary, the payload, and the pane_state event all say blocked — and it is +// the smaller of the two prices. +const paneStateAttentionKind = "idle" + +// paneStateAttentionSeverity matches the stall detector's. A blocked pane is +// worth surfacing, not worth escalating: nothing is broken, someone just has +// to answer a question the agent asked its terminal instead of the hub. +const paneStateAttentionSeverity = "minor" + // D-5 constants, read from herdr `src/pane/agent_detection.rs` at the same // commit the manifests are vendored from (6f311498): // @@ -156,6 +196,93 @@ type paneStateEntry struct { published paneStatePublish graceUntil time.Time pending pendingIdleHold + + // attentionID is the open row for the CURRENT blocked streak, "" when + // none. One row per streak, not per tick: the hub's attention model owns + // re-delivery, which is why D-5's 800 ms visible-blocker re-publish was + // deliberately not ported. + attentionID string + + // scanActivity is the `#{window_activity}` stamp the last capture is + // known-good for, or 0 when this pane is not skippable. See noteScan. + scanActivity int64 +} + +// attentionAction is what a freshly published classification owes the +// attention surface. Separated from the doing so the decision is testable +// without a hub. +type attentionAction int + +const ( + attentionNone attentionAction = iota + attentionRaise + attentionRetract +) + +// attentionFor decides raise / retract / nothing for a classification that is +// about to be published. +// +// Raising needs BOTH `blocked` and `visible_blocker` (plan P3). The strictness +// is the point: `blocked` alone can come from a rule that inferred the state +// from an OSC title or a spinner's absence, and a guess is not worth waking +// someone for. `visible_blocker` means the manifest matched a dialog that is +// on the screen right now — evidence a human can go look at. +// +// The streak is keyed on the STATE only, so blocked→blocked with the dialog +// scrolling out of the matched region keeps the row (still blocked, still +// waiting); it retracts when the classification leaves blocked entirely. +func (e *paneStateEntry) attentionFor(next paneStatePublish) attentionAction { + if next.state == panestate.StateBlocked { + if e.attentionID == "" && next.visibleBlocker { + return attentionRaise + } + return attentionNone + } + if e.attentionID != "" { + return attentionRetract + } + return attentionNone +} + +// skipCapture is B5's capture-cost gate: don't even read the screen of a pane +// that is idle and has produced no output since we last read it. +// +// Ported from upstream's `should_skip_idle_screen_scan` (agent_detection.rs:91) +// with its conditions intact — skip ONLY when the published state is idle and +// no idle hold is in flight. Upstream's other two guards, `agent_changed` and +// `process_exited`, are the same structurally-false pair documented on +// pendingIdleHold.hold, and its `agent.is_none()` guard is unreachable here +// because eligibility already established the agent. +// +// The asymmetry is what makes a wrong skip survivable: a blocked or working +// pane is re-read every tick no matter what the stamp says, so the gate can +// never freeze the state this lane exists to report. The worst a stale stamp +// can do is delay noticing that an IDLE pane became something else. +func (e *paneStateEntry) skipCapture(activity int64) bool { + if e.scanActivity == 0 || activity != e.scanActivity { + return false + } + return e.published.state == panestate.StateIdle && !e.pending.active() +} + +// noteScan records the activity stamp the capture just taken is valid for. +// +// It stores the stamp only when the SECOND it names had already elapsed when +// we captured. `#{window_activity}` has one-second resolution, so output that +// lands later in the same second as the stamp we read produces an IDENTICAL +// stamp — equality would then read as "nothing happened" when something did, +// and for an idle pane that skip would repeat forever. Requiring +// `now > activity` means any later output must fall in a strictly greater +// second, which makes the equality test sound rather than probabilistic. +// +// A 0 stamp (unknown / unparseable / a tmux without the format) stores 0 and +// the pane is simply never skipped. +func (e *paneStateEntry) noteScan(activity int64, now time.Time) { + if activity > 0 && now.Unix() > activity { + e.scanActivity = activity + return + } + e.scanActivity = 0 } // identify runs on the first tick an agent becomes eligible. Upstream @@ -219,11 +346,18 @@ type paneStateWatch struct { reg *panestate.Registry log *slog.Logger capture PaneCaptureFunc - titles func(ctx context.Context) (map[string]string, error) + meta func(ctx context.Context) (map[string]paneMeta, error) now func() time.Time entries map[string]*paneStateEntry } +// paneStateAttention is the attention surface the watcher needs: raise a row, +// and withdraw the one it raised. *Client satisfies it; tests stub it. +type paneStateAttention interface { + AttentionPoster + AttentionResolver +} + // newPaneStateWatch builds the watcher, or returns nil when the embedded // manifests will not load. // @@ -242,12 +376,25 @@ func newPaneStateWatch(log *slog.Logger) *paneStateWatch { reg: reg, log: log, capture: tmuxCapturePane, - titles: listTmuxPaneTitles, + meta: listTmuxPaneMeta, now: time.Now, entries: map[string]*paneStateEntry{}, } } +// covers reports whether the declarative evaluator has rules for an agent +// family — i.e. whether this watcher is a state authority for it (plan D-3). +// +// Nil-safe on both receiver and registry so the caller's guard reads the same +// whether or not the embedded manifests loaded. +func (w *paneStateWatch) covers(kind string) bool { + if w == nil || w.reg == nil || kind == "" { + return false + } + _, ok := w.reg.ManifestForFamily(kind) + return ok +} + // tick classifies every eligible pane once and posts the transitions. // // `hasAuthority` is D-2: it reports whether a live in-process driver authors @@ -255,14 +402,17 @@ func newPaneStateWatch(log *slog.Logger) *paneStateWatch { // map. Upstream has the same gate — `lifecycle_authority_active` short- // circuits its detection loop before the screen is ever read (pane.rs:807). // -// Disjointness with the IdleDetector that P3 retires is structural, not -// coincidental: every family the overlay maps is a registered agent family, -// so hasStructuredDriver() already makes tickIdle skip it. That invariant is a -// test (TestPaneStateFamiliesAreRegisteredAgentFamilies) because it is the -// kind that rots silently — adding a mapping for an unregistered kind would -// have both detectors scraping the same pane and disagreeing. +// Disjointness with the stall detector (IdleDetector) is enforced from the other +// too: Runner.hasAnyStateAuthority asks w.covers() before running it, so no +// pane is ever scraped by both. TestPaneStateFamiliesAreRegisteredAgentFamilies +// and TestIdleDetectorSkipsEveryMappedFamily lock the two halves — this is the +// kind of invariant that rots silently. +// +// `attn` may be nil, which turns off the attention leg while leaving state +// events flowing. Nothing wires it that way in production; it keeps the +// classification tests free of an attention stub. func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, - agents []Agent2, hasAuthority func(agentID string) bool) { + attn paneStateAttention, agents []Agent2, hasAuthority func(agentID string) bool) { if w == nil || w.reg == nil || poster == nil { return } @@ -294,9 +444,13 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, e := w.entries[ag.ID] if e == nil || e.manifestID != manifestID { + // A manifest swap under a live agent re-identifies it, so the row + // raised under the old rules is withdrawn first — its rule id + // names evidence the new manifest may not even have. + w.retract(ctx, attn, e) e = &paneStateEntry{manifestID: manifestID} w.entries[ag.ID] = e - w.post(ctx, poster, ag, e, e.identify(now), paneStatePublish{}, nil) + w.post(ctx, poster, ag, e, e.identify(now), paneStatePublish{}, nil, "") continue } if e.inGrace(now) { @@ -311,9 +465,12 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, } // Prune before the (possibly expensive) evaluation pass so a long agent - // list does not keep dead entries alive for an extra tick. - for id := range w.entries { + // list does not keep dead entries alive for an extra tick. An agent that + // left the running set takes its attention row with it: the row asked + // someone to go answer a dialog on a pane that no longer exists. + for id, e := range w.entries { if _, ok := seen[id]; !ok { + w.retract(ctx, attn, e) delete(w.entries, id) } } @@ -321,16 +478,20 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, return } - // D-4: one round-trip for every pane's OSC title. A failure degrades to - // empty titles rather than skipping the tick — the screen regions still - // classify, and only the `osc_title` rules go quiet. - titles, err := w.titles(ctx) + // D-4: one round-trip for every pane's OSC title, and B5's activity stamp + // in the same call. A failure degrades rather than skipping the tick — the + // screen regions still classify, only the `osc_title` rules go quiet, and + // an unknown stamp simply disables the capture gate for this pass. + meta, err := w.meta(ctx) if err != nil { - w.log.Debug("pane title read failed; classifying on screen text alone", "err", err) - titles = nil + w.log.Debug("pane metadata read failed; classifying on screen text alone", "err", err) + meta = nil } for _, d := range due { + if d.entry.skipCapture(meta[d.agent.PaneID].activity) { + continue + } screen, cerr := w.capture(ctx, d.agent.PaneID) if cerr != nil { // Transient tmux failures (pane gone, server restarted) are @@ -348,7 +509,7 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, // rules were written to see on a taller pane. ex, eerr := w.reg.EvaluateManifest(d.manifestID, panestate.Input{ Screen: screen, - OSCTitle: titles[d.agent.PaneID], + OSCTitle: meta[d.agent.PaneID].title, // OSCProgress stays empty: tmux does not surface OSC 9;4 // progress to a client. Three vendored rules reference it and // are inert for us (D-4, documented rather than worked around). @@ -357,12 +518,115 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, w.log.Debug("pane state evaluation failed", "agent", d.agent.ID, "err", eerr) continue } + // The gate arms only after a capture actually succeeded and evaluated, + // so a failed read never counts as "we have seen this screen". + d.entry.noteScan(meta[d.agent.PaneID].activity, now) + prev := d.entry.published next, publish := d.entry.step(ex, now) + + // Attention is decided on every classified tick, NOT only on a + // transition, and before the event so the event can name the row. + // Deciding it inside the publish branch would make a failed raise + // permanent: the streak's transition already happened, so the retry + // tick has nothing to publish and would never look again. + raised := "" + switch d.entry.attentionFor(next) { + case attentionRaise: + raised = w.raise(ctx, attn, d.agent, d.entry, ex) + case attentionRetract: + w.retract(ctx, attn, d.entry) + case attentionNone: + } if !publish { continue } - w.post(ctx, poster, d.agent, d.entry, next, prev, &ex) + w.post(ctx, poster, d.agent, d.entry, next, prev, &ex, raised) + } +} + +// raise opens one attention row for a blocked streak and returns its id. +// +// The evidence is a rule id and a manifest version, never screen text — the +// same rule post() follows, for the same reason: a blocked pane is showing +// whatever the agent was doing, which may be a secret, and an attention row is +// the most widely-fanned surface the hub has. P4's explain verb is where a +// human asks for the region preview, deliberately. +func (w *paneStateWatch) raise(ctx context.Context, attn paneStateAttention, + ag Agent2, e *paneStateEntry, ex panestate.Explain) string { + if attn == nil { + return "" + } + who := ag.Handle + if who == "" { + who = ag.ID + } + rule := "" + if ex.MatchedRule != nil { + rule = ex.MatchedRule.ID + } + summary := "agent blocked at a prompt: " + who + if rule != "" { + summary += " (" + rule + ")" + } + payload := map[string]any{ + "detector": "panestate", + "state": string(panestate.StateBlocked), + "agent_id": ag.ID, + "family": ag.Kind, + "pane": ag.PaneID, + "manifest_id": e.manifestID, + } + if rule != "" { + payload["rule_id"] = rule + } + if m, ok := w.reg.Manifest(e.manifestID); ok { + payload["manifest_version"] = m.Version + payload["manifest_source"] = m.Source + } + pending, err := json.Marshal(payload) + if err != nil { + // A payload we cannot marshal must not cost the raise — the summary + // alone still tells a human which agent to go look at. + w.log.Debug("pane_state attention payload marshal failed", "agent", ag.ID, "err", err) + pending = nil + } + out, err := attn.PostAttention(ctx, AttentionIn{ + ScopeKind: "team", + Kind: paneStateAttentionKind, + Summary: summary, + Severity: paneStateAttentionSeverity, + ActorHandle: ag.Handle, + PendingPayload: pending, + }) + if err != nil { + // Leaving attentionID empty means the next tick that still classifies + // blocked retries — the streak owes a row, not this tick. That only + // works because tick() decides attention on every classified pass; see + // the note there. + w.log.Debug("pane_state attention raise failed", "agent", ag.ID, "err", err) + return "" + } + e.attentionID = out.ID + w.log.Info("pane blocked; attention raised", + "agent", ag.ID, "handle", ag.Handle, "rule", rule, "attention", out.ID) + return out.ID +} + +// retract closes the row this watcher raised, if any. Tolerant by design: a +// 409 means the director dismissed it first, which is the outcome we wanted +// anyway. Either way the id is dropped so the next blocked streak raises fresh. +func (w *paneStateWatch) retract(ctx context.Context, attn paneStateAttention, e *paneStateEntry) { + if e == nil || e.attentionID == "" { + return + } + id := e.attentionID + e.attentionID = "" + if attn == nil { + return + } + if err := attn.ResolveAttention(ctx, id); err != nil { + w.log.Debug("pane_state attention resolve failed", "attention", id, "err", err) } } @@ -372,7 +636,7 @@ func (w *paneStateWatch) tick(ctx context.Context, poster AgentEventPoster, // which a human asks for; putting it in every transition would push pane // contents into the transcript of an agent that may be showing a secret. func (w *paneStateWatch) post(ctx context.Context, poster AgentEventPoster, ag Agent2, - e *paneStateEntry, next, prev paneStatePublish, ex *panestate.Explain) { + e *paneStateEntry, next, prev paneStatePublish, ex *panestate.Explain, attentionID string) { payload := map[string]any{ "state": string(next.state), "previous_state": string(prev.state), @@ -403,6 +667,9 @@ func (w *paneStateWatch) post(ctx context.Context, poster AgentEventPoster, ag A if next.visibleWorking { payload["visible_working"] = true } + if attentionID != "" { + payload["attention_id"] = attentionID + } if err := poster.PostAgentEvent(ctx, ag.ID, PaneStateEventKind, paneStateEventProducer, payload); err != nil { w.log.Debug("post pane_state event failed", "agent", ag.ID, "err", err) } diff --git a/hub/internal/hostrunner/panestate_watch_test.go b/hub/internal/hostrunner/panestate_watch_test.go index 01c75d88..d0b01539 100644 --- a/hub/internal/hostrunner/panestate_watch_test.go +++ b/hub/internal/hostrunner/panestate_watch_test.go @@ -2,8 +2,12 @@ package hostrunner import ( "context" + "encoding/json" + "errors" + "fmt" "io" "log/slog" + "strings" "testing" "time" @@ -36,8 +40,10 @@ const ( type testWatch struct { *paneStateWatch poster *recordingPoster + attn *recordingAttention screen string titles map[string]string + activity map[string]int64 captured []string now time.Time capErr error @@ -50,9 +56,11 @@ func newTestWatch(t *testing.T) *testWatch { t.Fatalf("load embedded manifests: %v", err) } tw := &testWatch{ - poster: &recordingPoster{}, - titles: map[string]string{}, - now: time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC), + poster: &recordingPoster{}, + attn: &recordingAttention{}, + titles: map[string]string{}, + activity: map[string]int64{}, + now: time.Date(2026, 8, 9, 12, 0, 0, 0, time.UTC), } tw.paneStateWatch = &paneStateWatch{ reg: reg, @@ -66,13 +74,47 @@ func newTestWatch(t *testing.T) *testWatch { } return tw.screen, nil }, - titles: func(context.Context) (map[string]string, error) { return tw.titles, nil }, + meta: func(context.Context) (map[string]paneMeta, error) { + m := map[string]paneMeta{} + for id, title := range tw.titles { + m[id] = paneMeta{title: title, activity: tw.activity[id]} + } + for id, act := range tw.activity { + if _, ok := m[id]; !ok { + m[id] = paneMeta{activity: act} + } + } + return m, nil + }, } return tw } func (tw *testWatch) tickAgents(agents []Agent2, authority func(string) bool) { - tw.paneStateWatch.tick(context.Background(), tw.poster, agents, authority) + tw.paneStateWatch.tick(context.Background(), tw.poster, tw.attn, agents, authority) +} + +// recordingAttention stands in for the hub's /attention surface. It hands out +// increasing ids so a test can tell "the same row" from "a second row". +type recordingAttention struct { + raised []AttentionIn + resolved []string + postErr error + next int +} + +func (r *recordingAttention) PostAttention(_ context.Context, in AttentionIn) (AttentionOut, error) { + if r.postErr != nil { + return AttentionOut{}, r.postErr + } + r.raised = append(r.raised, in) + r.next++ + return AttentionOut{ID: fmt.Sprintf("att-%d", r.next)}, nil +} + +func (r *recordingAttention) ResolveAttention(_ context.Context, id string) error { + r.resolved = append(r.resolved, id) + return nil } func codexAgent() Agent2 { @@ -498,6 +540,317 @@ func TestPaneStatePrunesEntriesForVanishedAgents(t *testing.T) { } } +// --- attention (P3) ------------------------------------------------------- + +// blockedWatch settles a codex agent and leaves it showing the approval +// dialog, which is the starting point for most of the attention tests. +func blockedWatch(t *testing.T) (*testWatch, []Agent2, func(string) bool) { + t.Helper() + tw := newTestWatch(t) + agents := []Agent2{codexAgent()} + none := func(string) bool { return false } + tw.titles["%7"] = "project" + tw.screen = codexWorkingScreen + tw.settle(agents, none) + tw.tickAgents(agents, none) // publish working + + tw.screen = codexBlockedScreen + tw.now = tw.now.Add(3 * time.Second) + return tw, agents, none +} + +// The plan's headline acceptance clause: a real codex approval screen becomes +// an attention item naming the rule that matched. +func TestBlockedScreenRaisesAttentionWithRuleID(t *testing.T) { + tw, agents, none := blockedWatch(t) + tw.tickAgents(agents, none) + + if len(tw.attn.raised) != 1 { + t.Fatalf("raised %d attention items, want 1", len(tw.attn.raised)) + } + got := tw.attn.raised[0] + if got.Kind != paneStateAttentionKind { + t.Errorf("kind = %q, want %q", got.Kind, paneStateAttentionKind) + } + if got.ActorHandle != "cx" { + t.Errorf("actor_handle = %q, want the agent handle", got.ActorHandle) + } + if !strings.Contains(got.Summary, "blocked") || !strings.Contains(got.Summary, "cx") { + t.Errorf("summary = %q, want it to name the state and the agent", got.Summary) + } + + var payload map[string]any + if err := json.Unmarshal(got.PendingPayload, &payload); err != nil { + t.Fatalf("pending_payload is not json: %v", err) + } + if payload["rule_id"] != "live_strong_blocker" { + t.Errorf("rule_id = %v, want live_strong_blocker", payload["rule_id"]) + } + if payload["agent_id"] != "ag-1" || payload["pane"] != "%7" { + t.Errorf("payload lost the agent/pane pointer: %+v", payload) + } + // Same rule as the event: evidence is a rule id, never pane text. An + // attention row fans out further than the transcript does. + for k, v := range payload { + if s, ok := v.(string); ok && strings.Contains(s, "Yes, proceed") { + t.Fatalf("payload key %q leaked the screen", k) + } + } + if strings.Contains(got.Summary, "Yes, proceed") { + t.Fatalf("summary leaked the screen: %q", got.Summary) + } + // The transition event points at the row it raised. + last, ok := tw.poster.last() + if !ok || last.Payload["attention_id"] != "att-1" { + t.Errorf("pane_state event should name the attention row: %+v", last.Payload) + } +} + +// Once per streak, not once per tick. The hub's attention model owns +// re-delivery — that is why D-5's 800 ms visible-blocker re-publish was +// deliberately not ported. +func TestBlockedStreakRaisesExactlyOnce(t *testing.T) { + tw, agents, none := blockedWatch(t) + for i := 0; i < 5; i++ { + tw.tickAgents(agents, none) + tw.now = tw.now.Add(3 * time.Second) + } + if len(tw.attn.raised) != 1 { + t.Fatalf("raised %d rows across one blocked streak, want 1", len(tw.attn.raised)) + } + if len(tw.attn.resolved) != 0 { + t.Fatalf("resolved %v while still blocked", tw.attn.resolved) + } +} + +// The human answered the dialog in the terminal. Nothing tells the hub, so the +// row would sit open forever; the classification leaving blocked is the signal. +func TestAttentionRetractsWhenClassificationLeavesBlocked(t *testing.T) { + tw, agents, none := blockedWatch(t) + tw.tickAgents(agents, none) + + tw.screen = codexWorkingScreen + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) + + if len(tw.attn.resolved) != 1 || tw.attn.resolved[0] != "att-1" { + t.Fatalf("resolved = %v, want [att-1]", tw.attn.resolved) + } + // And a NEW streak gets its own row rather than reviving the closed one. + tw.screen = codexBlockedScreen + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) + if len(tw.attn.raised) != 2 { + t.Fatalf("raised %d rows across two streaks, want 2", len(tw.attn.raised)) + } +} + +// An agent that stopped running takes its row with it: the row asks someone to +// answer a dialog on a pane that no longer exists. +func TestAttentionRetractsWhenAgentLeavesRunningSet(t *testing.T) { + tw, agents, none := blockedWatch(t) + tw.tickAgents(agents, none) + + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(nil, none) + + if len(tw.attn.resolved) != 1 || tw.attn.resolved[0] != "att-1" { + t.Fatalf("resolved = %v, want [att-1]", tw.attn.resolved) + } +} + +// A hub that was down when the streak began must not cost the whole streak its +// row. The retry works only because tick() decides attention on every +// classified pass, not just on a transition — deleting that property here +// leaves this test as the one that fails. +func TestAttentionRaiseRetriesAfterAFailure(t *testing.T) { + tw, agents, none := blockedWatch(t) + tw.attn.postErr = errors.New("hub down") + tw.tickAgents(agents, none) + if len(tw.attn.raised) != 0 { + t.Fatalf("recorded a raise that failed: %+v", tw.attn.raised) + } + + tw.attn.postErr = nil + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) // same screen: no transition to publish + + if len(tw.attn.raised) != 1 { + t.Fatalf("raised %d rows after the hub recovered, want 1", len(tw.attn.raised)) + } +} + +// `blocked` without `visible_blocker` is an inference — an OSC title, a +// missing spinner. Worth recording as state, not worth waking someone for. +func TestAttentionNeedsAVisibleBlocker(t *testing.T) { + var e paneStateEntry + inferred := paneStatePublish{state: panestate.StateBlocked} + if got := e.attentionFor(inferred); got != attentionNone { + t.Errorf("inferred blocked: action = %v, want none", got) + } + seen := paneStatePublish{state: panestate.StateBlocked, visibleBlocker: true} + if got := e.attentionFor(seen); got != attentionRaise { + t.Errorf("visible blocker: action = %v, want raise", got) + } + // The dialog scrolling out of the matched region does not end the streak — + // only leaving `blocked` does. + e.attentionID = "att-1" + if got := e.attentionFor(inferred); got != attentionNone { + t.Errorf("still blocked: action = %v, want none", got) + } + if got := e.attentionFor(paneStatePublish{state: panestate.StateIdle}); got != attentionRetract { + t.Errorf("left blocked: action = %v, want retract", got) + } +} + +// The plan's second acceptance clause: the idle-shell false-positive class +// that the legacy regex detector exists to catch must be UNABLE to raise here. +// A bare `$` prompt is not a blocked agent, and no vendored rule says it is. +func TestBareShellPromptCannotRaiseAttention(t *testing.T) { + tw := newTestWatch(t) + agents := []Agent2{codexAgent()} + none := func(string) bool { return false } + tw.screen = codexWorkingScreen + tw.settle(agents, none) + tw.tickAgents(agents, none) // publish working, so idle is a transition + + // Two ticks: working → plain idle is the one transition D-5 holds, and at + // our 3 s cadence the cap releases it on the second observation. + tw.screen = "$ \n" + for i := 0; i < 2; i++ { + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) + } + + if len(tw.attn.raised) != 0 { + t.Fatalf("a bare shell prompt raised %+v", tw.attn.raised) + } + last, ok := tw.poster.last() + if !ok || last.Payload["state"] != string(panestate.StateIdle) { + t.Fatalf("bare prompt should classify idle, got %+v", last.Payload) + } + if last.Payload["fallback_reason"] != panestate.FallbackKnownAgentIdle { + t.Errorf("want the known-agent idle fallback, got %+v", last.Payload) + } +} + +// --- capture-cost gating (B5) --------------------------------------------- + +func TestSkipCaptureOnlyForAnIdleUnchangedPane(t *testing.T) { + base := func() *paneStateEntry { + e := &paneStateEntry{published: paneStatePublish{state: panestate.StateIdle}} + e.scanActivity = 1770000000 + return e + } + if !base().skipCapture(1770000000) { + t.Error("idle pane, unmoved activity: want skip") + } + if base().skipCapture(1770000001) { + t.Error("activity moved: must re-read") + } + if base().skipCapture(0) { + t.Error("unknown stamp: must re-read") + } + + // A blocked or working pane is re-read every tick no matter what the stamp + // says. That asymmetry is what keeps a stale stamp from ever freezing the + // state this lane exists to report. + for _, s := range []panestate.State{panestate.StateBlocked, panestate.StateWorking, panestate.StateUnknown} { + e := base() + e.published.state = s + if e.skipCapture(1770000000) { + t.Errorf("state %s: must never be skipped", s) + } + } + // Nor mid-hysteresis: the hold needs its next observation to resolve. + held := base() + held.pending.startedAt = time.Now() + if held.skipCapture(1770000000) { + t.Error("a pending idle hold must not be skipped") + } + // Never scanned: nothing to compare against. + fresh := &paneStateEntry{published: paneStatePublish{state: panestate.StateIdle}} + if fresh.skipCapture(0) || fresh.skipCapture(1770000000) { + t.Error("an unarmed gate must not skip") + } +} + +// `#{window_activity}` has one-second resolution, so a stamp read DURING the +// second it names cannot be compared for equality later — output arriving +// later in that same second would produce an identical stamp, and for an idle +// pane that skip would repeat forever. +func TestNoteScanRefusesAStampFromTheCurrentSecond(t *testing.T) { + at := time.Unix(1770000000, 0) + var e paneStateEntry + + e.noteScan(1770000000, at) // captured inside the second the stamp names + if e.scanActivity != 0 { + t.Errorf("armed the gate on a same-second stamp: %d", e.scanActivity) + } + e.noteScan(1770000000, at.Add(1500*time.Millisecond)) // that second has passed + if e.scanActivity != 1770000000 { + t.Errorf("scanActivity = %d, want the stamp", e.scanActivity) + } + e.noteScan(0, at.Add(time.Hour)) // unknown stamp disarms + if e.scanActivity != 0 { + t.Errorf("an unknown stamp must disarm the gate, got %d", e.scanActivity) + } +} + +// End to end: an idle pane whose window produced no output is not captured at +// all — no subprocess, no evaluation. +func TestCaptureGateSkipsAnUnchangedIdlePane(t *testing.T) { + tw := newTestWatch(t) + agents := []Agent2{codexAgent()} + none := func(string) bool { return false } + // A stamp from well before the settle clock, so noteScan arms. + tw.activity["%7"] = tw.now.Add(-time.Minute).Unix() + tw.screen = "$ \n" + tw.settle(agents, none) + + tw.tickAgents(agents, none) // first read arms the gate + if len(tw.captured) != 1 { + t.Fatalf("first pass captured %d times, want 1", len(tw.captured)) + } + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) + if len(tw.captured) != 1 { + t.Fatalf("unchanged idle pane was captured again: %v", tw.captured) + } + + // Output arrives: the stamp moves and the pane is read again, this time + // showing a dialog that must still reach attention. + tw.activity["%7"] = tw.now.Unix() + tw.screen = codexBlockedScreen + tw.titles["%7"] = "project" + tw.now = tw.now.Add(3 * time.Second) + tw.tickAgents(agents, none) + if len(tw.captured) != 2 { + t.Fatalf("moved activity should force a re-read: %v", tw.captured) + } + if len(tw.attn.raised) != 1 { + t.Fatalf("the dialog behind the gate raised %d rows, want 1", len(tw.attn.raised)) + } +} + +// A pane the gate has never armed on — because the metadata read failed, or +// tmux has no `#{window_activity}` — is captured every tick, as before. +func TestCaptureGateDefaultsToCapturing(t *testing.T) { + tw := newTestWatch(t) + agents := []Agent2{codexAgent()} + none := func(string) bool { return false } + tw.screen = "$ \n" + tw.settle(agents, none) + + for i := 0; i < 3; i++ { + tw.tickAgents(agents, none) + tw.now = tw.now.Add(3 * time.Second) + } + if len(tw.captured) != 3 { + t.Fatalf("captured %d times without an activity stamp, want 3", len(tw.captured)) + } +} + // --- invariants ----------------------------------------------------------- // The legacy IdleDetector and this watcher must never scrape the same pane. @@ -529,7 +882,11 @@ func TestPaneStateFamiliesAreRegisteredAgentFamilies(t *testing.T) { // classification is a degraded host-runner, panicking is a dead one. func TestPaneStateNilWatchIsInert(t *testing.T) { var w *paneStateWatch - w.tick(context.Background(), &recordingPoster{}, []Agent2{codexAgent()}, func(string) bool { return false }) + w.tick(context.Background(), &recordingPoster{}, &recordingAttention{}, + []Agent2{codexAgent()}, func(string) bool { return false }) + if w.covers("codex") { + t.Error("a disabled watcher covers nothing — else the legacy detector stays off too") + } } // --- runner-level authority ---------------------------------------------- @@ -558,24 +915,89 @@ func TestPaneStateAuthorityDistinguishesRawPaneDriver(t *testing.T) { } } -// --- tmux title parsing --------------------------------------------------- +// P3's IdleDetector retirement, asserted rather than assumed: for every family +// the evaluator covers, the legacy regex detector must stay quiet — otherwise +// two detectors scrape the same pane and disagree about it. +// +// This holds even for a raw PaneDriver, which is the whole point: that is the +// agent the evaluator is FOR, and it is also the only one the legacy detector +// would have taken. +func TestIdleDetectorSkipsEveryMappedFamily(t *testing.T) { + a := &Runner{ + drivers: map[string]Driver{"ag-raw": &PaneDriver{AgentID: "ag-raw"}}, + paneStates: newPaneStateWatch(slog.New(slog.NewTextHandler(io.Discard, nil))), + } + if a.paneStates == nil { + t.Fatal("embedded manifests failed to load") + } + families := a.paneStates.reg.Families() + if len(families) == 0 { + t.Fatal("no families mapped; the overlay lost its engines block") + } + for _, family := range families { + if !a.hasAnyStateAuthority(Agent2{ID: "ag-raw", Kind: family}) { + t.Errorf("family %q is evaluated by the manifests but the legacy "+ + "idle detector would also scrape its pane", family) + } + } +} + +// The retirement must not widen the legacy detector's reach. `kimi-code-ts` is +// a registered family the overlay deliberately does NOT map, and an instance +// that fell back to a raw pane has no structured driver either — the third +// clause of hasAnyStateAuthority is the only thing keeping the regex detector +// off it, and off the TUI-prompt false positive the W11 smoke found. +func TestUnmappedRegisteredFamilyStaysOutOfTheLegacyDetector(t *testing.T) { + a := &Runner{ + drivers: map[string]Driver{"ag-raw": &PaneDriver{AgentID: "ag-raw"}}, + paneStates: newPaneStateWatch(slog.New(slog.NewTextHandler(io.Discard, nil))), + } + const family = "kimi-code-ts" + if _, mapped := a.paneStates.reg.ManifestForFamily(family); mapped { + t.Skipf("%s is mapped now; this test's premise moved", family) + } + if _, ok := agentfamilies.ByName(family); !ok { + t.Fatalf("%s is no longer a registered agent family", family) + } + if !a.hasAnyStateAuthority(Agent2{ID: "ag-raw", Kind: family}) { + t.Error("an unmapped registered family must stay out of the legacy detector") + } + + // An unregistered, unmapped kind is exactly what the legacy detector is + // still for, and it must still get it. + if a.hasAnyStateAuthority(Agent2{ID: "ag-raw", Kind: "some-legacy-script"}) { + t.Error("the legacy detector lost the agents it exists for") + } +} + +// --- tmux pane metadata parsing ------------------------------------------- + +func TestParsePaneMetaKeepsSpacesAndEmpties(t *testing.T) { + out := "%1 1770000000 codex — my project\n" + // both fields + "%2\n" + // pane id alone + "%3 1770000001 \n" + // trailing space, empty title + "\n" + // blank line + "%4 llm-proxy\n" + // tmux too old for #{window_activity} + "%5 not-a-number title\n" // garbage where the stamp should be -func TestParsePaneTitlesKeepsSpacesAndEmpties(t *testing.T) { - out := "%1 codex — my project\n%2\n%3 \n\n%4 llm-proxy\n" - got := parsePaneTitles(out) + got := parsePaneMeta(out) - want := map[string]string{ - "%1": "codex — my project", - "%2": "", - "%3": "", - "%4": "llm-proxy", + want := map[string]paneMeta{ + "%1": {title: "codex — my project", activity: 1770000000}, + "%2": {}, + "%3": {activity: 1770000001}, + // An absent or unparseable stamp must read as UNKNOWN (0), never as a + // timestamp — 0 disables the capture gate, a wrong number would make it + // skip a pane forever. + "%4": {title: "llm-proxy"}, + "%5": {title: "title"}, } if len(got) != len(want) { t.Fatalf("parsed %d panes, want %d: %+v", len(got), len(want), got) } - for id, title := range want { - if got[id] != title { - t.Errorf("%s title = %q, want %q", id, got[id], title) + for id, w := range want { + if got[id] != w { + t.Errorf("%s = %+v, want %+v", id, got[id], w) } } } diff --git a/hub/internal/hostrunner/reconcile.go b/hub/internal/hostrunner/reconcile.go index bdad0d39..d66737d4 100644 --- a/hub/internal/hostrunner/reconcile.go +++ b/hub/internal/hostrunner/reconcile.go @@ -2,6 +2,7 @@ package hostrunner import ( "context" + "strconv" "strings" ) @@ -45,38 +46,63 @@ func listTmuxPanes(ctx context.Context) (map[string]paneInfo, error) { return m, nil } -// listTmuxPaneTitles returns pane_id → pane_title for every pane on the -// server in ONE round-trip (pane-state plan D-4: the manifests' `osc_title` -// region is fed from tmux's view of the title the app set via OSC 0/2). +// paneMeta is what the pane-state watcher needs to know about a pane besides +// its screen: the OSC title the app set, and when output last reached it. +// +// activity is `#{window_activity}` — epoch SECONDS, and per WINDOW, not per +// pane. tmux 3.4 has no per-pane activity stamp (the `pane_*` format list has +// none; verified against the 3.4 man page and source), so a window holding two +// agent panes reports one timestamp for both. That errs towards capturing when +// nothing changed, which is the harmless direction. 0 means "unknown" — an +// unparseable or absent value must never look like "nothing happened". +type paneMeta struct { + title string + activity int64 +} + +// listTmuxPaneMeta returns pane_id → paneMeta for every pane on the server in +// ONE round-trip (pane-state plan D-4 for the title, B5 for the activity +// stamp; P2 left a note that P3's gating folds into this same call). // // Kept separate from listTmuxPanes rather than folded into it: a title is // free-form text that can contain spaces, so it has to be LAST in the format // string and parsed positionally, and widening the reconcile format to carry // it would put a field that can contain anything ahead of nothing but still -// change a parser three transitions depend on. P3's capture-cost gating adds -// `#{window_activity}` to this same call and is the moment to merge the two. -func listTmuxPaneTitles(ctx context.Context) (map[string]string, error) { - out, err := runTmux(ctx, "list-panes", "-a", "-F", "#{pane_id} #{pane_title}") +// change a parser three transitions depend on. +func listTmuxPaneMeta(ctx context.Context) (map[string]paneMeta, error) { + out, err := runTmux(ctx, "list-panes", "-a", "-F", + "#{pane_id} #{window_activity} #{pane_title}") if err != nil { return nil, err } - return parsePaneTitles(out), nil + return parsePaneMeta(out), nil } -// parsePaneTitles splits ` ` lines. Split on the FIRST -// space only: a pane id never contains one (`%17`), a title routinely does. -func parsePaneTitles(out string) map[string]string { - m := map[string]string{} +// parsePaneMeta splits ` ` lines. Cut on +// the first two spaces only: neither a pane id (`%17`) nor an epoch stamp ever +// contains one, and a title routinely does. +// +// A tmux too old to know `#{window_activity}` expands it to the empty string, +// which lands here as an unparseable field and resolves to activity 0 — the +// gate then never skips. Same for a title-only line from any other shape we +// have not anticipated: the pane id is the only field this function insists on. +func parsePaneMeta(out string) map[string]paneMeta { + m := map[string]paneMeta{} for _, line := range strings.Split(strings.TrimRight(out, "\n"), "\n") { line = strings.TrimSuffix(line, "\r") if line == "" { continue } - id, title, _ := strings.Cut(line, " ") + id, rest, _ := strings.Cut(line, " ") if id == "" { continue } - m[id] = title + activity, title, _ := strings.Cut(rest, " ") + secs, err := strconv.ParseInt(activity, 10, 64) + if err != nil || secs < 0 { + secs = 0 + } + m[id] = paneMeta{title: title, activity: secs} } return m } diff --git a/hub/internal/hostrunner/runner.go b/hub/internal/hostrunner/runner.go index a148caba..856398ac 100644 --- a/hub/internal/hostrunner/runner.go +++ b/hub/internal/hostrunner/runner.go @@ -103,10 +103,12 @@ type Runner struct { EgressProxyAddr string egressProxy *egressProxy - // idle + idleProbes are the pane detector P3 RETIRES: one global prompt - // regex plus a 90 s stall, the only state signal for engines with no - // structured driver. paneStates is its declarative successor (lane P); - // the two cover disjoint pane sets until that retirement lands. + // idle + idleProbes are the stall detector: one global prompt regex plus a + // 90 s content hash stall. paneStates is its declarative successor (lane + // P) and covers a strictly disjoint pane set — see hasAnyStateAuthority, + // which is the single guard deciding which of the two owns a pane. The + // stall detector is NOT scheduled for removal: it remains the only signal + // for an engine that has neither a structured driver nor a manifest. idle *IdleDetector idleProbes map[string]idleProbeState // keyed by agent id // paneStates classifies mapped families' panes against the vendored @@ -478,17 +480,15 @@ func (a *Runner) Start(ctx context.Context) error { // Errors on a single pane are logged and skipped — one bad pane shouldn't // prevent us from watching the others. // -// Agents whose kind matches a registered engine family (claude-code, -// codex, gemini-cli, kimi-code-ts, antigravity) are skipped: their -// drivers emit explicit busy/idle signals via lifecycle / turn.result / -// completion events, so mobile already has authoritative state, and the -// regex-based -// pane scrape false-positives on these engines' always-visible chat -// prompt (the W11 smoke surfaced this — every 30 min an "agent idle at -// prompt" attention item landed on the Me page even though agy was -// behaving normally, just sitting at its TUI prompt waiting for input). -// The detector remains for legacy/unknown agents that PaneDriver runs -// without structured state. +// Agents that already have a state authority are skipped +// (hasAnyStateAuthority): a driver that emits explicit busy/idle signals +// via lifecycle / turn.result / completion events, or lane P's manifest +// evaluator. The regex-based pane scrape false-positives on a modern +// engine's always-visible chat prompt (the W11 smoke surfaced this — +// every 30 min an "agent idle at prompt" attention item landed on the Me +// page even though agy was behaving normally, just sitting at its TUI +// prompt waiting for input). The detector remains for unregistered/unknown +// agents that PaneDriver runs without structured state. func (a *Runner) tickIdle(ctx context.Context) { agents, err := a.Client.ListRunningAgents(ctx, a.HostID) if err != nil { @@ -496,14 +496,13 @@ func (a *Runner) tickIdle(ctx context.Context) { } now := time.Now() - // Lane P (P2): declarative classification for the families the vendored - // screen manifests cover. It shares this tick's agent list rather than - // re-fetching, and covers a set DISJOINT from the older detector below - // (the one P3 removes) — every mapped family is a registered agent - // family, so the hasStructuredDriver() skip already excludes it here. The two never - // scrape the same pane; TestPaneStateFamiliesAreRegisteredAgentFamilies - // fails if a mapping is added that breaks that. - a.paneStates.tick(ctx, a.agentPoster, agents, a.paneStateAuthority) + // Lane P: declarative classification for the families the vendored screen + // manifests cover. It shares this tick's agent list rather than + // re-fetching, and covers a set DISJOINT from the stall detector below — + // hasAnyStateAuthority asks the watcher first, so no pane is scraped by + // both. TestPaneStateFamiliesAreRegisteredAgentFamilies and + // TestIdleDetectorSkipsEveryMappedFamily lock the two halves. + a.paneStates.tick(ctx, a.agentPoster, a.Client, agents, a.paneStateAuthority) seen := map[string]struct{}{} for _, ag := range agents { @@ -511,11 +510,12 @@ func (a *Runner) tickIdle(ctx context.Context) { if ag.PaneID == "" || ag.PauseState == "paused" { continue } - if hasStructuredDriver(ag.Kind) { - // Engine reports its own state; the pane-tail scrape would - // false-positive on the always-on chat prompt. Drop any - // stored hash so a future regression that re-enables this - // path doesn't replay a stale "stuck for hours" baseline. + if a.hasAnyStateAuthority(ag) { + // Something else already authors this agent's state; the + // pane-tail scrape would false-positive on the always-on chat + // prompt. Drop any stored hash so a future regression that + // re-enables this path doesn't replay a stale "stuck for + // hours" baseline. delete(a.idleProbes, ag.ID) continue } @@ -1174,6 +1174,46 @@ func (a *Runner) paneStateAuthority(agentID string) bool { return !rawPane } +// hasAnyStateAuthority is the stall detector's guard (IdleDetector, plan P3): +// quiet whenever SOMETHING already authors this agent's state. +// +// Three clauses, narrowest first: +// +// 1. a live structured driver (the D-2 gate above); +// 2. lane P's manifest evaluator, which is a state authority for every +// family the overlay maps; +// 3. any registered agent family. +// +// Clause 3 is the one doing work. `kimi-code-ts` is a registered family the +// overlay deliberately does NOT map (nobody has confirmed upstream's `kimi` +// manifest is the compiled-TypeScript CLI), so an instance whose M4 launch +// fell back to a raw pane satisfies neither 1 nor 2. Dropping clause 3 would +// hand that pane to the regex detector — GROWING the stall detector's reach, +// in a wedge whose job is to shrink it, and re-opening the exact TUI-prompt +// false positive the W11 smoke found. Retirement means the set only contracts. +// +// **Clause 2 changes no answer today, and that is stated rather than +// implied.** Every mapped family is a registered one, so clause 3 already +// covers all of them; a mutation deleting clause 2 survives the whole suite. +// It stays because it names the actual reason a mapped pane is exempt, and +// because the subsumption is an invariant of the overlay rather than a law — +// TestPaneStateFamiliesAreRegisteredAgentFamilies is what would fail if it +// were relaxed, and on that day this clause becomes load-bearing without +// anyone having to remember it. +// +// So P3's change here is precision, not coverage: the mapped families were +// already skipped via clause 3, and TestIdleDetectorSkipsEveryMappedFamily now +// asserts it instead of leaving it a coincidence. +func (a *Runner) hasAnyStateAuthority(ag Agent2) bool { + if a.paneStateAuthority(ag.ID) { + return true + } + if a.paneStates.covers(ag.Kind) { + return true + } + return hasStructuredDriver(ag.Kind) +} + // putDriver registers a driver under agentID. Guarded by agentsMu (#77). func (a *Runner) putDriver(agentID string, d Driver) { a.agentsMu.Lock() diff --git a/hub/internal/panestate/region.go b/hub/internal/panestate/region.go index 00348cb3..d4c64833 100644 --- a/hub/internal/panestate/region.go +++ b/hub/internal/panestate/region.go @@ -36,9 +36,16 @@ const maxRegionLineCount = 65535 // Input is what the evaluator classifies: a screen snapshot plus the strings // the terminal reported out of band. type Input struct { - // Screen is the bottom-anchored capture. Plan D-4 makes the geometry a - // contract: the vendored rules were authored against upstream's last-24- - // rows snapshot, so P2 must trim to that before calling here. + // Screen is the visible viewport, untrimmed — what `capture-pane -p -J` + // returns. + // + // Plan D-4 called the geometry "the bottom-anchored last 24 rows + // (DEFAULT_DETECTION_ROWS)". P2 read the source and found that backwards: + // upstream's `ghostty_detection_text` reads `terminal.rows()` and falls + // back to 24 only when the row count is unavailable + // (herdr src/pane/terminal.rs:2468-2475). 24 is a fallback, not the + // contract — trimming to it would cut rows the `top_*` region rules were + // written to see on any pane taller than 24. Screen string // OSCTitle comes from tmux `#{pane_title}`. OSCTitle string diff --git a/hub/internal/server/handlers_attention.go b/hub/internal/server/handlers_attention.go index f872bcd7..13c57431 100644 --- a/hub/internal/server/handlers_attention.go +++ b/hub/internal/server/handlers_attention.go @@ -112,11 +112,31 @@ func (s *Server) handleCreateAttention(w http.ResponseWriter, r *http.Request) { now := NowUTC() _, actorKind, actorHandle := actorFromContext(r.Context()) // When the caller is a host-runner raising an attention on behalf - // of an agent (codex permission_prompt bridge — ADR-012 D3), they - // pass actor_handle in the body and we stamp actor_kind=agent so - // the mobile UI shows the right origin chip. The agent's own MCP - // calls leave the body field empty and the auth context wins. - if in.ActorHandle != "" && actorHandle == "" { + // of an agent (codex permission_prompt bridge — ADR-012 D3; the + // pane-state blocked row — pane-state-manifests P3), they pass + // actor_handle in the body and we stamp actor_kind=agent, so the + // row records WHICH AGENT the ask came from rather than which + // process delivered it. The agent's own MCP calls leave the body + // field empty and the auth context wins. + // + // (Both columns are returned by GET /attention and /attention/{id}; + // no client renders them as an origin chip yet, which is why the + // bug below survived. The stored attribution is the point — it is + // what an audit or an API consumer reads.) + // + // The condition is the TOKEN KIND, not an empty context handle. It + // used to be `actorHandle == ""`, which can never hold on an + // authenticated request: actorFromContext runs the scope through + // principalFromScope, which falls back to "@principal" for an + // absent handle, an absent role, and unparseable JSON alike. So the + // body field was unreachable and every host-runner-raised row — + // including every codex approval since ADR-012 — was attributed to + // the host token's principal rather than to the agent that asked. + // Only `host` tokens may name someone else: a host-runner is + // trusted to speak for the agents it supervises, and an agent's own + // token still loses to its context identity, so this is not an + // impersonation vector. + if in.ActorHandle != "" && actorKind == "host" { actorKind = "agent" actorHandle = in.ActorHandle } diff --git a/hub/internal/server/handlers_attention_panestate_test.go b/hub/internal/server/handlers_attention_panestate_test.go new file mode 100644 index 00000000..4735c668 --- /dev/null +++ b/hub/internal/server/handlers_attention_panestate_test.go @@ -0,0 +1,118 @@ +package server + +import ( + "encoding/json" + "net/http" + "strings" + "testing" +) + +// The pane-state classifier's attention row, exercised against the real +// handlers rather than a host-runner-side stub. +// +// host-runner's own tests fake the hub, so they prove the state machine and +// prove nothing about the contract. The retract leg in particular rests on +// three hub-side facts that live in a different package and could each be +// changed by someone who never opens the pane-state code: +// +// 1. `idle` is not in attentionAwaitsAgentReply, so /resolve accepts it — +// /decide is for rows that owe a parked agent a reply, and this row owes +// nobody anything. +// 2. a detector-supplied `pending_payload` survives the round-trip, since it +// is the only place the rule id and manifest version are carried. +// 3. resolving twice is a 409, not a 500 — the director dismissing the row +// before host-runner notices the pane moved on is the normal race. +// +// Plan: docs/plans/pane-state-manifests.md P3. +func TestPaneStateAttentionRaisesAndResolves(t *testing.T) { + s, _ := newA2ATestServer(t) + // A host token, because that is who raises this row — and because the + // origin-chip rule keys off the token KIND. A principal token here would + // have quietly passed a test of the wrong thing. + token := mintToken(t, s, "host", map[string]any{"team": defaultTeamID, "role": "host"}) + + // What paneStateWatch.raise() posts, field for field. + code, body := doReq(t, s, token, http.MethodPost, + "/v1/teams/"+defaultTeamID+"/attention", map[string]any{ + "scope_kind": "team", + "kind": "idle", + "summary": "agent blocked at a prompt: cx (live_strong_blocker)", + "severity": "minor", + "actor_handle": "cx", + "pending_payload": map[string]any{ + "detector": "panestate", + "state": "blocked", + "agent_id": "ag-1", + "family": "codex", + "pane": "%7", + "manifest_id": "codex", + "manifest_version": "1", + "rule_id": "live_strong_blocker", + }, + }) + if code != http.StatusCreated { + t.Fatalf("create = %d, want 201: %s", code, body) + } + var created struct { + ID string `json:"id"` + } + if err := json.Unmarshal(body, &created); err != nil || created.ID == "" { + t.Fatalf("create response has no id: %s", body) + } + + // The evidence survives the round-trip: a client that renders the card + // reads the rule id from here, and P4's explain verb keys off the pane. + code, body = doReq(t, s, token, http.MethodGet, + "/v1/teams/"+defaultTeamID+"/attention/"+created.ID, nil) + if code != http.StatusOK { + t.Fatalf("get = %d: %s", code, body) + } + var got map[string]any + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("get response: %v", err) + } + payload, ok := got["pending_payload"].(map[string]any) + if !ok { + t.Fatalf("pending_payload missing or not an object: %s", body) + } + if payload["rule_id"] != "live_strong_blocker" || payload["detector"] != "panestate" { + t.Errorf("payload lost its evidence: %+v", payload) + } + if got["actor_kind"] != "agent" || got["actor_handle"] != "cx" { + t.Errorf("origin chip = %v/%v, want agent/cx", got["actor_kind"], got["actor_handle"]) + } + + // The retract leg. /resolve, NOT /decide: nothing is parked on this row. + code, body = doReq(t, s, token, http.MethodPost, + "/v1/teams/"+defaultTeamID+"/attention/"+created.ID+"/resolve", + map[string]any{}) + if code != http.StatusNoContent { + t.Fatalf("resolve = %d, want 204 — is `idle` in attentionAwaitsAgentReply now? %s", + code, body) + } + + code, body = doReq(t, s, token, http.MethodGet, + "/v1/teams/"+defaultTeamID+"/attention/"+created.ID, nil) + if code != http.StatusOK { + t.Fatalf("get after resolve = %d: %s", code, body) + } + got = nil + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("get response: %v", err) + } + if got["status"] != "resolved" { + t.Errorf("status = %v, want resolved", got["status"]) + } + + // Losing the race to a director who dismissed it first is a 409 the + // caller logs and drops, not a failure it retries. + code, body = doReq(t, s, token, http.MethodPost, + "/v1/teams/"+defaultTeamID+"/attention/"+created.ID+"/resolve", + map[string]any{}) + if code != http.StatusConflict { + t.Fatalf("second resolve = %d, want 409: %s", code, body) + } + if !strings.Contains(string(body), "already resolved") { + t.Errorf("409 body should say why: %s", body) + } +}