From d5840b322f3c25692be0596e7667b3441d8df24b Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Sun, 9 Aug 2026 15:04:00 +0000 Subject: [PATCH 1/2] feat(hostrunner): blocked panes raise (and retract) attention (P3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wedge P3 of docs/plans/pane-state-manifests.md. The classifier wired in by P2 could see a codex approval dialog but could not tell anyone; now a `blocked` classification carrying `visible_blocker` opens one attention row per blocked streak, and withdraws it when the classification leaves blocked — the human answered the dialog in the terminal, where nothing would otherwise tell the hub. It is the first host-runner row that retracts itself. Evidence is a rule id, a manifest and its version, and the agent + pane; never screen text. An attention row fans out further than a transcript does, and the pane may be showing a secret. P4's explain verb is where a human asks for the region preview. Also in this wedge: - Capture-cost gating (B5). tmux's `#{window_activity}` rides the same single `list-panes` round-trip P2 already made for pane titles, and an idle pane whose window produced no output since the last read is not captured at all. Confined to idle panes exactly as upstream confines it, so a stale stamp can never freeze a blocked one. - The stall detector's guard becomes `hasAnyStateAuthority(agent)`. Four findings, each from checking rather than assuming: 1. The retirement this wedge was meant to perform HAD ALREADY HAPPENED, and doing it literally would have partly undone it. Every mapped family is a registered family, and the old guard skipped every registered family — so the stall detector already never touched a mapped pane. Swapping in "has any state authority" alone would have handed it the registered-but-unmapped families, `kimi-code-ts` above all, re-opening the W11 TUI-prompt false positive. The registered family clause stays, so the set can only contract, and a sweep test now asserts the disjointness instead of leaving it a coincidence. 2. D-2's structured-authority exception is NOT a port and is deferred. Upstream short-circuits on `lifecycle_authority_active` before the screen is read (pane.rs:809) and never consults `visible_blocker` for a pane it skipped. Whether the exception would complement claude's hook-raised `permission_prompt` rows or duplicate them turns on whether the TUI draws a dialog the hook already parked — one capture of a real claude pane settles it, and static reading cannot. 3. `#{window_activity}` is sound but one-second granular, so a stamp read during the second it names cannot be compared for equality later — output landing later in that same second is invisible, and for an idle pane that skip would repeat forever. The gate arms only on a stamp whose second had already elapsed. (tmux calls window_update_activity() from input_parse_buffer(), independent of monitor-activity; verified against the 3.4 source. There is no per-pane equivalent.) 4. The attention kind stays `idle`, which reads wrong next to this lane's own `blocked`, and is still right: on that surface the kind picks the affordance, and it is the only value both clients already route to acknowledge-only. A new kind inherits the unknown-kind default, which on mobile draws Approve / Reject for a state report nothing can approve. Fixes, found by testing against the real handler instead of a stub: `POST /attention` honoured a body-supplied `actor_handle` only when the caller had no handle of its own, which can never hold — principalFromScope falls back to "@principal" for absent handle, absent role and unparseable JSON alike. Every codex-bridge row since ADR-012 D3 has been attributed to the host token's principal rather than the agent. Now keyed on the token kind, so only a `host` token may name someone else. 19 new/changed tests; 8 mutations introduced, 7 caught, and the 8th is documented in place as a clause that changes no answer today rather than pretending a test covers it. Full `go test ./...` green. Co-Authored-By: Claude Opus 5 (1M context) --- desktop/src/i18n/index.ts | 8 + docs/changelog.md | 55 +++ docs/plans/pane-state-manifests.md | 112 ++++- docs/reference/attention-kinds.md | 24 +- hub/internal/hostrunner/client.go | 21 + hub/internal/hostrunner/driver_appserver.go | 11 + hub/internal/hostrunner/panestate_watch.go | 331 +++++++++++-- .../hostrunner/panestate_watch_test.go | 458 +++++++++++++++++- hub/internal/hostrunner/reconcile.go | 54 ++- hub/internal/hostrunner/runner.go | 96 ++-- hub/internal/panestate/region.go | 13 +- hub/internal/server/handlers_attention.go | 30 +- .../handlers_attention_panestate_test.go | 118 +++++ 13 files changed, 1226 insertions(+), 105 deletions(-) create mode 100644 hub/internal/server/handlers_attention_panestate_test.go diff --git a/desktop/src/i18n/index.ts b/desktop/src/i18n/index.ts index 1a606d058..932a0dec3 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 0cd4ff572..aa9b51748 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,35 @@ 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=operator` plus the host token's principal instead of + `agent` plus the agent's handle. 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 ef20ee0e8..b60d5c8c5 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 9a7a86483..d869912ee 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 5f59ea7d5..c9b6b516a 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 3144815fd..e07e4f181 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 d89b66278..eb1d64541 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 01c75d880..d0b015391 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 bdad0d391..d66737d4c 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 a148caba1..856398ac2 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 00348cb3b..d4c648334 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 f872bcd7d..13c574319 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 000000000..4735c6682 --- /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) + } +} From 6e6ee9b27e9968708ff8a713727102d331dc1c72 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 09:50:50 +0000 Subject: [PATCH 2/2] docs(changelog): the mis-attributed attention rows recorded actor_kind=host, not operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Fixed entry for the unreachable actor_handle branch said the affected rows recorded `actor_kind=operator`. The stored value is the caller's token kind verbatim (`actorFromContext` returns `tok.Kind`, inserted unmapped), and host-runners run under `host`-kind tokens (handlers_admin_tokens.go mints them; the install guide issues one) — so the rows say `host`. An auditor following the changelog would have queried `actor_kind='operator'` and found nothing. Review pass 2026-08-10. Co-Authored-By: Claude Fable 5 --- docs/changelog.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index aa9b51748..366d09261 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -176,8 +176,10 @@ binding). Seed entries prior to that are in 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=operator` plus the host token's principal instead of - `agent` plus the agent's handle. The condition is now the token + `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