Repository navigation
feat(cp): observer/lobby — 3-phase design + Phase 1 protocol scaffold (PR 2/4) - #1470
Conversation
e68b8e7 to
2330bba
Compare
This comment has been minimized.
This comment has been minimized.
2330bba to
3f3d181
Compare
This comment has been minimized.
This comment has been minimized.
3f3d181 to
4a290d3
Compare
This comment has been minimized.
This comment has been minimized.
4a290d3 to
b23b309
Compare
This comment has been minimized.
This comment has been minimized.
b23b309 to
718a9f8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…d-12 findings) - F63: EventHub::emit now logs ONE aggregated warn per emission (drop count out of observer count) instead of one warn per refused observer, keeping systemic lobby saturation visible at default log levels without multiplying log I/O by observer count on the delegation path; per-observer detail moves to debug. - F64: recorded as an explicit decision instead of an accident - the SATURATED code doc in proto.rs now states the three capacity domains it deliberately covers and why one code suffices (same client reaction; every message names the exhausted bound), and the client doc's SATURATED bullet includes the observer-cap case. Revisit if machine-readable attribution is needed. - F65: dedicated regression pins the precomputed prompt-excerpt path: truncation preserved in a normal namespace, excerpt key absent under metadata_only, secret content nowhere in the frame. - F66: RegistrationGuard::drop doc rephrased - sweep_deadlines is teardown-adjacent (sweeper task), not on the Drop chain; the fail-soft discipline claim now cannot be misread. - Review NIT: register_conn_capped allocates the handle only after the cap check, so a refused registration no longer consumes a handle id. - Reviewer-suggested guard comment on deliver_result: its intentional expect (request path, absorbed by fail-soft teardown) is now documented so the asymmetry with synthesized_frame is not "fixed" blindly. cargo test -p openab-cp: 149 passed, 0 failed. clippy --all-targets -D warnings clean; rustfmt clean.
|
Important CHANGES REQUESTED What This PR DoesAdds Phase 1 of the CP observer/lobby capability: a read-only How It Works
Round-12 DispositionThe delta since round 12 (
FindingsNo new findings this round. Both lanes returned lane-level LGTMs on the delta: no logic, race, lock-ordering, log-leak, or input-validation regressions (the aggregated warn is strictly less revealing than the per-observer warn it replaces). The verdict remains changes-requested solely on the carried backlog below - all 🟡 important, none critical, previously adjudicated as hardening/docs/architecture follow-up material. Carried open backlog (spot-re-verified at this head)
Baseline Check
Addressing External Reviewer FeedbackNo external reviewer has commented on this PR at review time (the requested reviewer has not yet responded). All prior visible comments are earlier rounds of this review, disposed in Round-12 Disposition above. What's Good (🟢)
Three Reasons We Might Not Need This PR
|
|
Caution This PR has been waiting on the author for more than 2 days (labeled @chaodu-obk[bot] — You must add a new comment on this PR to remove the |
|
🔒 Auto-closing: this PR has had the If you'd like to continue working on this, please submit a new PR and link to this one if necessary. |
|
reopened |
|
🔒 Auto-closing: this PR has had the If you'd like to continue working on this, please submit a new PR and link to this one if necessary. |
|
Fixed the observer membership/sequence race in Root cause
What changed
ValidationValidated on macmini against exact commit
I also audited every |
|
Note LGTM ✅ - Phase 1 observer/lobby lands clean: the read-only invariant is enforced at four independent layers, commit-first delivery makes the observer stream and the initiator wire agree by construction, and this head's seq/membership race fix is correct and regression-tested. Prior rounds' blocking findings (incl. the round-13 What This PR DoesAdds Phase 1 of the CP observer/lobby capability on top of the already-merged CP core (#1469): a read-only How It Works
Findings
Finding Details🟢 F1: Read-only enforced in depthObserver initiation is denied unconditionally as the first policy check, observers are filtered out of 🟢 F2: Commit-first deliveryThe reorder to commit-before-deliver is the load-bearing correctness change. The documented rationale is sound: delivery is irreversible, so committing first guarantees exactly one path owns both terminals for an admission. The 🟢 F3: Seq/membership race fix (this head)Holding the per-namespace stream lock across the membership snapshot, seq allocation, and enqueue gives one linearization point. 🟢 F4: metadata_only contract
🟢 F5: Fail-soft teardown
🟢 F6: DocumentationThe Observer surface section documents the event table, seq/correlation/ordering contract, best-effort delivery, and content redaction; the ADR amendment records the commit-gated delivery rule and accepted operational limits. The example config demonstrates the observer identity and Addressing External Reviewer Feedback@thepagent (Rounds 1-13)The peer reviewer drove this PR through 13 rounds. The head commit
✅ Addressed:
✅ Addressed:
✅ Addressed: Baseline Check
Verification
5. Three Reasons We Might Not Need This PR
|
CP Observer / Lobby — phased design + Phase 1 scaffold
Review Contract
Goal
Land the complete Phase 1 of the observer/lobby capability on the CP: the
observeragent type (read-only, unconditionally non-delegating), thecp/eventnotification stream with per-namespace sequence numbers,cp/list_agents, and the server-side wiring that emits events at every lifecycle hook (register, disconnect, lease expiry, delegation requested / completed / cancelled, timeout, target disconnect).Non-goals
cp/attach) and Phase 3 (lobby app, Cloudflare DO deployment) — separate PRs.cp/list_agentssnapshot resync.seq.Accepted Residual Risks
seqgap + snapshot resync is the recovery contract, verified by tests but not yet exercised by a real lobby client.seqis not durable across CP restarts; observers treat any regression/reset as a full resync.metadata_onlysuppresses agent-supplied bodies only; CP-synthesized diagnostics (timeout, disconnect reasons) remain visible, which is intentional but means event streams are not fully content-free.Acceptance Criteria
seqis per-namespace monotonic and dense per observer stream (a process-global counter would manufacture false gaps from other namespaces' activity)concurrent_emits_enqueue_in_seq_order(8 threads x 16 events) fails 5/5 under the racy pattern, passes under the shipped onetry_sendon the existing bounded per-connection queues; the delegation path never waits on an observer (saturated-queue test)metadata_only = truenamespaces omit prompt/result excerpts entirelydelegation_requested,delegation_completed(incl. timeout / target_disconnected),delegation_cancelled(withfrom/to)cp/list_agentsreturns the caller's namespace roster to any registered clientcargo test -p openab-cp: 81 passed (54 -> 81, 0 removed); clippy--all-targets -D warningsclean; rustfmt clean for the crateFollow-ups
cp/attachrelay + human identity class (clientsallowlist) — next PR in the stack.cp-coreextraction).session_*relay events once Phase 2 lands (open question below).Motivation
The control plane (ADR:
docs/adr/agent-control-plane.md, implementation: #1469) makes every agent-to-agent delegation flow through one hub — which means the CP is structurally the single point where the whole fleet's activity is visible. Today that visibility exists only as tracing logs.This roadmap turns it into a first-class product surface — a lobby: any authorized client (a macOS/iOS app, a web dashboard,
wscat) connects to the CP and watches, live, who is registered, who delegates what to whom, and how each delegation ends. Phase 2 extends the same connection into a relay, so a human client can talk to any registered agent directly through the CP — no Discord/Slack in the loop, no per-agent URLs.This also answers ADR Open Question #3 (human visibility of delegation traffic): instead of mirroring into platform threads, visibility becomes a protocol-level subscription.
Target architecture (end state, Phase 3)
flowchart TB subgraph clients["Lobby clients (Phase 3)"] APP["macOS / iOS app"] WEB["web dashboard / wscat"] end subgraph cp["openab-cp — native binary or Cloudflare Durable Object"] REG["registry<br/>(who is alive)"] ROUTE["router<br/>(delegate RPC)"] POL["policy<br/>(who → whom)"] EVT["event stream<br/>(Phase 1)"] RELAY["client relay<br/>(Phase 2)"] end subgraph fleet["OAB runtimes (outbound dial, no ingress)"] A["OAB 'koudu'<br/>type=primary<br/>ACP stdio → Agent A"] B["OAB 'worker-1'<br/>type=worker (headless)<br/>ACP stdio → Agent B"] end APP -- "WS: observer<br/>cp/event subscription" --> EVT APP -- "WS: relay session<br/>(Phase 2)" --> RELAY WEB -- "WS: observer" --> EVT A -- "register / WS" --> REG B -- "register / WS" --> REG A <-. "cp/delegate ⇄ cp/delegate_result" .-> ROUTE ROUTE <-. "forward ⇄ result" .-> B RELAY <-. "session/prompt ⇄ session/update" .-> A EVT -. "mirrors registry + delegation lifecycle" .- ROUTEPhases
Phase 1 — Observer protocol (this PR)
A third client type joins
primary/worker:observer— a read-only lobby client, authenticated by the same per-key identity binding as agents.type = "observer"cp/registerfirst-frame rule. Never selectable as a delegation target; can never initiate, serve, cancel, or complete delegations — enforced unconditionally in CP policy, with no relaxation knob by designcp/eventseq(gap detection) +ts+ flattened event payloadagent_registered,agent_deregistered(disconnect / lease-expired),delegation_requested,delegation_completed(all terminal statuses incl. timeout / target_disconnected),delegation_cancelledmax_event_excerpt_bytes(default 4 KiB) — the lobby is an audit surface, not a second delivery pathcp/list_agentsseqgapprodsees nothing fromdevsequenceDiagram participant O as Lobby app (observer) participant CP as openab-cp participant P as OAB primary participant W as OAB worker O->>CP: cp/register (type=observer) CP-->>O: ack {heartbeat, lease} O->>CP: cp/list_agents CP-->>O: roster snapshot P->>CP: cp/delegate {target, prompt, deadline} CP-->>O: cp/event {seq:n, delegation_requested, prompt_excerpt, chain} CP->>W: cp/delegate (forwarded, CP-stamped chain) CP-->>P: ack {assigned_to} W->>CP: cp/delegate_result {status, result} CP->>P: cp/delegate_result CP-->>O: cp/event {seq:n+1, delegation_completed, result_excerpt}Delivery semantics (v1, deliberate): best-effort fan-out over each observer's bounded outbound queue. A slow observer loses frames, detects the
seqgap, and resyncs viacp/list_agents— the CP never buffers unboundedly for a spectator, and observers can never backpressure the delegation path.Phase 2 — Client relay (human ↔ agent through the CP)
Upgrade the CP from agent-to-agent hub to a general session hub: an authorized client opens a relay session to any registered agent by name, through its existing CP connection.
cp/attach {target}→ relayedsession/prompt/session/updateframes (reusing ACP session semantics end-to-end)clientsallowlist (which namespaces/agents they may attach to)delegation-equivalent visibility once Phase 1's event vocabulary is extended withsession_*events (open question below)Rationale for CP-relay over app→OAB direct dial: the app only ever needs one URL + one credential; workers stay ingress-free; policy/audit stay centralized.
Phase 3 — Lobby surfaces + Cloudflare deployment
Two independently shippable tracks:
Lobby app (macOS/iOS, SwiftUI): roster sidebar with live state (registered / working / saturated — borrowing herdr's four-state sidebar density), delegation timeline fed by
cp/event, tap-an-agent-to-chat via Phase 2 relay.Cloudflare Durable Objects deployment: split
openab-cpinto a pure logic core (registry/router/policy/events — no tokio/axum dependency; events in, frames out) with two transport frontends:workers-rs+ one DO per namespace — WebSocket Hibernation for idle connections, DO Alarms for lease/deadline sweeps, zero-ingress global reachThe namespace→DO mapping matches the CP's isolation model 1:1. Agents are agnostic: it's the same
wss://URL either way.flowchart LR subgraph core["cp-core (pure logic crate)"] C["registry · router · policy · events"] end subgraph native["native frontend"] N["axum + tokio<br/>systemd / ECS / k8s"] end subgraph cf["Cloudflare frontend"] D["workers-rs + Durable Object<br/>1 DO per namespace<br/>WS Hibernation + Alarms"] end N --> C D --> CWhat is in this PR (Phase 1, complete)
proto.rs:AgentType::Observer;JsonRpcNotification;EventParams(seq/ts/namespace+ flattenedCpEvent); the fiveCpEventkinds;cp/list_agentstypes (AgentSummary,ListAgentsResult); method namescp/event,cp/list_agentsregistry.rs:select()excludes observers (never a delegation target, even by exact name);observers(namespace)fan-out setpolicy.rs:PolicyDenial::ObserverInitiation— checked before everything else, unconditionalconfig.rs:max_event_excerpt_bytes(default 4096)cp.toml.example: observer identity exampleServer wiring (second commit on this branch):
events.rs(new):EventHub— per-namespace seq streams; one serialization per emission; ordered, non-blockingtry_sendfan-out toregistry.observers(ns); excerpt/metadata_onlypolicy. Seq allocation and enqueue are atomic per namespace (concurrency regression test included).server.rs: registration announce (after ack), disconnect/lease-expiry deregister announce,cp/list_agentshandler, explicit observer guard on the delegation methods.router.rs:delegation_requested/delegation_completed(incl. timeout and target-disconnect terminal paths) /delegation_cancelledemission; sharedtruncate_with_markerreused for excerpts.AgentDeregistered.reasonis now an enum (disconnect/lease_expired);DelegationCancelledcarriesfrom/to;prompt_excerptisOption<String>(omitted undermetadata_only);seqdocumented per-namespace.[namespaces.X] metadata_only = true— events carry lifecycle metadata but no prompt/result content (CP-synthesized diagnostics like timeout reasons remain).Verification
cargo test -p openab-cp: 81 passed, 0 failed (54 at the scaffold commit -> 81; 0 tests removed or renamed).cargo clippy -p openab-cp --all-targets -- -D warnings: clean.cargo fmtclean for the crate.concurrent_emits_enqueue_in_seq_orderfail 5/5 runs; the shipped pattern passes.Open questions
session_*relay events in Phase 2, or is delegation visibility enough for the lobby's v1?agent_registeredevents for observer connections themselves — currently planned to be emitted (the lobby sees other lobbies); trivial to filter if it's noise.