Skip to content

feat(cp): observer/lobby — 3-phase design + Phase 1 protocol scaffold (PR 2/4) - #1470

Merged
thepagent merged 10 commits into
mainfrom
feat/cp-observer
Sep 8, 2026
Merged

thepagent merged 10 commits into
mainfrom
feat/cp-observer

Conversation

@chaodu-obk

@chaodu-obk chaodu-obk Bot commented Aug 12, 2026 •

Copy link
Copy Markdown
Contributor

CP Observer / Lobby — phased design + Phase 1 scaffold

Stacked on #1469 (feat/openab-cp). This PR carries the full three-phase design for the observer/lobby capability and lands the minimal Phase 1 protocol scaffold. Server-side wiring follows in this PR; Phases 2–3 are separate PRs.

Review Contract

Goal

Land the complete Phase 1 of the observer/lobby capability on the CP: the observer agent type (read-only, unconditionally non-delegating), the cp/event notification 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

  • Phase 2 (client relay / cp/attach) and Phase 3 (lobby app, Cloudflare DO deployment) — separate PRs.
  • Durable event history / replay: v1 is live-only + cp/list_agents snapshot resync.
  • Guaranteed delivery to observers: fan-out is best-effort by design; a slow observer loses frames and detects it via seq.

Accepted Residual Risks

  • Best-effort fan-out means a saturated observer silently loses frames; the seq gap + snapshot resync is the recovery contract, verified by tests but not yet exercised by a real lobby client.
  • seq is not durable across CP restarts; observers treat any regression/reset as a full resync.
  • metadata_only suppresses 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

  • Observer can never initiate, serve, cancel, or complete delegations — enforced at identity binding, registry selection, policy (unconditional first check), and an explicit server-side method guard; no config can relax it
  • seq is per-namespace monotonic and dense per observer stream (a process-global counter would manufacture false gaps from other namespaces' activity)
  • Concurrent emits in one namespace enqueue in seq order — regression test concurrent_emits_enqueue_in_seq_order (8 threads x 16 events) fails 5/5 under the racy pattern, passes under the shipped one
  • Observer fan-out uses non-blocking try_send on the existing bounded per-connection queues; the delegation path never waits on an observer (saturated-queue test)
  • Excerpts are UTF-8-boundary-safe and reuse the router's truncation-marker helper; metadata_only = true namespaces omit prompt/result excerpts entirely
  • Events emitted at all hooks: registration (incl. observers), disconnect, lease expiry, delegation_requested, delegation_completed (incl. timeout / target_disconnected), delegation_cancelled (with from/to)
  • cp/list_agents returns the caller's namespace roster to any registered client
  • cargo test -p openab-cp: 81 passed (54 -> 81, 0 removed); clippy --all-targets -D warnings clean; rustfmt clean for the crate

Follow-ups

  • Phase 2: cp/attach relay + human identity class (clients allowlist) — next PR in the stack.
  • Phase 3: lobby app + Cloudflare Durable Objects frontend split (cp-core extraction).
  • Durable event history for lobby scrollback (same future layer as durable inboxes).
  • Decide whether observers should receive 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" .- ROUTE
Loading

Phases

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.

Piece Contract
type = "observer" Registers via the same cp/register first-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 design
cp/event JSON-RPC notification (no id) pushed to all observers in the event's namespace. Carries a process-monotonic seq (gap detection) + ts + flattened event payload
Event kinds agent_registered, agent_deregistered (disconnect / lease-expired), delegation_requested, delegation_completed (all terminal statuses incl. timeout / target_disconnected), delegation_cancelled
Payload bounds Prompt/result bodies are mirrored as excerpts capped by max_event_excerpt_bytes (default 4 KiB) — the lobby is an audit surface, not a second delivery path
cp/list_agents Namespace-scoped registry snapshot (name, type, labels, load) for any registered client — the lobby's roster view and the observer's resync path after a seq gap
Isolation Observers are namespace-scoped like everything else; an observer key for prod sees nothing from dev
sequenceDiagram
    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}
Loading

Delivery semantics (v1, deliberate): best-effort fan-out over each observer's bounded outbound queue. A slow observer loses frames, detects the seq gap, and resyncs via cp/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.

  • New method pair: cp/attach {target} → relayed session/prompt / session/update frames (reusing ACP session semantics end-to-end)
  • New identity class for humans: per-user keys with a clients allowlist (which namespaces/agents they may attach to)
  • The OAB runtime bridges relayed sessions into its local session pool exactly like a platform adapter — the CP link becomes, in effect, another adapter
  • Streaming: relay traffic gives observers delegation-equivalent visibility once Phase 1's event vocabulary is extended with session_* 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:

  1. 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.

  2. Cloudflare Durable Objects deployment: split openab-cp into a pure logic core (registry/router/policy/events — no tokio/axum dependency; events in, frames out) with two transport frontends:

    • native binary (axum + tokio) — self-hosted daemon, current path
    • workers-rs + one DO per namespace — WebSocket Hibernation for idle connections, DO Alarms for lease/deadline sweeps, zero-ingress global reach

    The 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 --> C
Loading

What is in this PR (Phase 1, complete)

  • proto.rs: AgentType::Observer; JsonRpcNotification; EventParams (seq/ts/namespace + flattened CpEvent); the five CpEvent kinds; cp/list_agents types (AgentSummary, ListAgentsResult); method names cp/event, cp/list_agents
  • registry.rs: select() excludes observers (never a delegation target, even by exact name); observers(namespace) fan-out set
  • policy.rs: PolicyDenial::ObserverInitiation — checked before everything else, unconditional
  • config.rs: max_event_excerpt_bytes (default 4096)
  • cp.toml.example: observer identity example
  • Unit tests for all of the above (proto roundtrips, notification shape, registry exclusion, policy denial under maximally-relaxed namespace config)

Server wiring (second commit on this branch):

  • events.rs (new): EventHub — per-namespace seq streams; one serialization per emission; ordered, non-blocking try_send fan-out to registry.observers(ns); excerpt/metadata_only policy. 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_agents handler, explicit observer guard on the delegation methods.
  • router.rs: delegation_requested / delegation_completed (incl. timeout and target-disconnect terminal paths) / delegation_cancelled emission; shared truncate_with_marker reused for excerpts.
  • Wire-type changes vs the scaffold commit (nothing had shipped): AgentDeregistered.reason is now an enum (disconnect / lease_expired); DelegationCancelled carries from/to; prompt_excerpt is Option<String> (omitted under metadata_only); seq documented per-namespace.
  • New config: [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 fmt clean for the crate.
  • Ordering regression verified by mutation: restoring the seq-outside-lock pattern makes concurrent_emits_enqueue_in_seq_order fail 5/5 runs; the shipped pattern passes.

Open questions

  1. Should observers receive session_* relay events in Phase 2, or is delegation visibility enough for the lobby's v1?
  2. agent_registered events for observer connections themselves — currently planned to be emitted (the lobby sees other lobbies); trivial to filter if it's noise.
  3. Event replay: v1 is live-only + snapshot resync. Durable event history (lobby scrollback after reconnect) belongs to the same future layer as durable inboxes.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-obk

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.
@chaodu-obk

chaodu-obk Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Important

CHANGES REQUESTED ⚠️ - Round 13 (group review) at head 6485afcd: the one fix commit since round 12 resolves every round-12 finding (F63 aggregated drop logging, F64 SATURATED-domain documentation, F65 precomputed-excerpt regression test, F66 guard-doc accuracy, plus the handle-allocation NIT and the deliver_result expect rationale), all independently verified at exact lines by two review lanes with zero new findings. No criticals. Only the known carried hardening/architecture backlog keeps the verdict at changes-requested.

What This PR Does

Adds Phase 1 of the CP observer/lobby capability: a read-only observer agent type, a cp/event notification stream with per-namespace sequence numbers, cp/list_agents roster snapshots, and server-side event emission at every delegation lifecycle hook. Based on main (#1469 merged).

How It Works

  • Observer read-only is enforced in non-redundant layers; delegation lifecycle events carry the admission token and correlate on (namespace, delegation_id, admission).
  • delegate()'s announce/forward critical section makes emission and forwarding atomic with entry removal; complete() is commit-first, so only the entry-removing path publishes a terminal on either wire.
  • Observer population and per-frame work are configured bounds (max_observers_per_namespace, validated max_event_excerpt_bytes); teardown-reachable serialization is fail-soft.
  • New this round: EventHub::emit logs ONE aggregated warn per emission (drop count out of observer count) instead of one warn per refused observer, keeping saturation visible at default log levels without multiplying log I/O by observer count inside the in-flight critical section; per-observer detail moves to debug.

Round-12 Disposition

The delta since round 12 (47016a90) is exactly one coordinator-authored fix commit (6485afcd); every claim was re-verified from scratch by two independent lanes (conflict of interest disclosed and discharged):

R12 item Status Independent evidence
F63 per-observer warn spam Fixed events.rs:143-174: loop still try_sends to every observer, borrowed not moved; dropped counted; single aggregated warn with dropped/observers.len() fires once per emission; per-observer detail at debug; log I/O inside the critical section now O(1) per emission
F64 overloaded SATURATED code Fixed (documented decision) proto.rs:159-173 names the three capacity domains; all three emission sites verified to name the exhausted bound (router.rs:492, :560; server.rs:361); docs/control-plane.md SATURATED bullet includes the observer-cap case
F65 precomputed-excerpt test gap Fixed router.rs:2862-2911: dedicated regression pins truncation (64 KiB prompt, marker present, <= 4096 bytes) and metadata_only (excerpt key absent, secret nowhere in the frame); all test helpers verified to behave as assumed
F66 guard-doc misread Fixed server.rs:556-568 correctly scopes the Drop chain to fail_instance + EventHub::emit and calls sweep_deadlines teardown-adjacent (sweeper task only, confirmed at server.rs:1000); fail-soft claim re-verified: zero expect/unwrap reachable from Drop
NIT handle burn on refusal Fixed registry.rs:288-306: handle allocated only after the cap check, all under one write-lock acquisition; no TOCTOU; no caller depends on the old ordering
deliver_result expect asymmetry Fixed (documented) router.rs:1511-1522: the intentional request-path expect is now explained; verified deliver_result has exactly one call site (complete()) and is unreachable from Drop or the sweeper

Findings

No 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)
ID One-line summary
F51-LT Long-term: fan-out still runs inside the in-flight critical section, bounded by config; structural fix (split seq-allocate from fan-out) deferred
R9-F53 (half) No public-API-driven test for the teardown-vs-announce race (needs lock-timing hooks)
F60 Lock hierarchy and cap invariant enforced by call-site discipline and docs, not structurally
F61 AdmissionToken is a type alias, not a newtype
R4-F2 Observer membership snapshot outside the per-namespace seq lock
R4-F6 cp/list_agents exposes load metrics and labels regardless of metadata_only
R4-F8 No length bounds on instance_id/labels
R4-F10 No seq watermark ties the roster snapshot to the event stream
R4-F11 EventHub threaded through every Router method signature
R4-F15 proto.rs module-level wire summary omits the observer surface (partially addressed in a prior round; residual scope)
R4-F16 No lifecycle state-machine doc (narrative only)
R4-F18 No restart/epoch signal; seq regression is the only restart indicator
R4-F19 Rejected delegations emit no event
R4-F21 cp/list_agents has no rate limit
R4-F22 Phase 2/3 specified to binding detail before Phase 1 has a consumer
R4-F25 EventHub two-level lock vs the crate's coarse-lock philosophy
R4-F26 Request-path expect("serializable") sites remain (now documented as an accepted, bounded exception at deliver_result)
R4-F33 Wall-clock sweeps; a host time step shifts expiry
R4-F35 delegate() length; extraction candidates remain
R5-F5 metadata_only is a construction-time snapshot
R5-F7 Observer guard error message lists cp/event as if callable (re-verified open at server.rs:773)
R5-F8 DelegationCompleted.error lacks the _excerpt suffix
R5-F10 Observer registration silently accepts max_delegated_sessions
R5-F11 DeregisterReason Display has no production call site (re-verified at proto.rs:517)
R6-F18 No integration-tier observer coverage over a real socket
R6-F19 (residual) No metrics/counters for fan-out, hold time, or drops - aggregated warn logging landed; counters still absent
R3-F32 ListAgentsParams {} dead code (re-verified at proto.rs:617)
R8-F42 Admission critical section still covers select/policy/serde of large prompts
Baseline Check
  • PR opened: 2026-08-12; reviewed head 6485afcdee974bfa9c83a743d5ae33c6b0ea1774; 9 commits, 11 files, +3740/-532
  • Base main; merge-base equals the base tip 94354a75 - clean, up-to-date; reviewed diff is main...head
  • Delta since round 12 (47016a90): exactly one commit, 6485afcd "fix(review): aggregate drop logging, document SATURATED domains (round-12 findings)" (+109/-18 across events.rs, proto.rs, registry.rs, router.rs, server.rs, docs/control-plane.md); range-diff shows the eight prior commits byte-identical
  • CI at review time: the check job (build + crate tests) completed green at this exact head, consistent with the commit's 149-passed claim; 15 of 16 smoke-test jobs green, one unified batch and build-builder still in progress, none failing; validate-packaged-pins green
  • Review method: local clone diff; two independent verification lanes (correctness/concurrency; docs/security + backlog re-verification), each re-verifying the coordinator-authored fix commit from scratch at exact lines; no Rust toolchain in the review environment, so CI is the build/test evidence

Addressing External Reviewer Feedback

No 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 (🟢)
  • The aggregation fix is the right shape: the default-level signal survives (systemic saturation stays loud) while the per-emission log cost inside the critical section drops from O(observers) to O(1) - and the new warn is strictly less revealing than the per-observer warn it replaces (observer identities moved to debug)
  • F64 is resolved the honest way: not by splitting the code, but by recording the one-code-three-domains decision with its rationale (same client reaction; every message names the exhausted bound) and a named revisit condition
  • The F65 regression test pins exactly the property the round-12 refactor could have silently broken: the hoisted excerpt is still truncated and still metadata_only-gated, asserted down to key absence and secret non-presence in the serialized frame
  • The handle-allocation reorder shows attention to review NITs without scope creep: cap check and allocation now share one atomic write-lock transaction
  • The deliver_result comment converts an invisible design decision into a guarded one - a future contributor now cannot "fix" the intentional request-path expect without confronting the stated rationale
  • Nine commits in, the change remains dependency-free with every commit mapped 1:1 to named review findings

Three Reasons We Might Not Need This PR

  1. In-process synchronous fan-out is bounded, not removed - fan-out serialization still runs inside the in-flight critical section and large-prompt serde still runs under the admission guard (R8-F42); if Phase 2/3 needs multi-instance lobbies or replay, an async dispatcher with an event log likely replaces this design and its carefully-won ordering guarantees.
  2. metadata_only remains a partial privacy control by documented choice - delegation metadata, timing, labels, and load metrics still reach observers (R4-F6); an operator who wants a content-free lobby still cannot configure one.
  3. Fourteen rounds in, the marginal round now yields zero new findings - every remaining item is previously-adjudicated hardening or long-term architecture. That is the strongest signal yet that the review has converged and the residual tail belongs in tracked follow-ups rather than further rounds. Counterpoint: the same convergence is what makes the tail credible - nothing in it blocks correctness, and the wire contract has been stable across the last three heads.

@github-actions github-actions Bot added pending-contributor closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. and removed pending-maintainer labels Aug 15, 2026
@github-actions

Copy link
Copy Markdown

Caution

This PR has been waiting on the author for more than 2 days (labeled pending-contributor since 2026-08-15).
It will be automatically closed in 24 hours if there is no update.

@chaodu-obk[bot] — You must add a new comment on this PR to remove the closing-soon label and keep it open. Pushing commits alone is not sufficient. Feel free to reopen a new PR later if it gets closed and you want to pick it back up.

@github-actions

Copy link
Copy Markdown

🔒 Auto-closing: this PR has had the closing-soon label for more than 1 days without activity from the author.

If you'd like to continue working on this, please submit a new PR and link to this one if necessary.

@github-actions github-actions Bot closed this Aug 18, 2026
@thepagent thepagent reopened this Sep 3, 2026
@thepagent

Copy link
Copy Markdown
Collaborator

reopened

@chaodu-agent chaodu-agent added pending-maintainer and removed closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. pending-contributor labels Sep 3, 2026
@github-actions github-actions Bot added pending-contributor closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. and removed pending-maintainer labels Sep 4, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

🔒 Auto-closing: this PR has had the closing-soon label for more than 1 days without activity from the author.

If you'd like to continue working on this, please submit a new PR and link to this one if necessary.

@github-actions github-actions Bot closed this Sep 7, 2026
@thepagent thepagent reopened this Sep 8, 2026
@openab-app openab-app Bot removed the closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. label Sep 8, 2026
@chaodu-agent

Copy link
Copy Markdown
Collaborator

Fixed the observer membership/sequence race in 9ba4e18.

Root cause

EventHub::emit cloned the observer set before acquiring the per-namespace stream lock. An older emit could therefore retain a stale membership snapshot while a newer emit won the stream lock first. A newly joined observer could receive seq N, miss seq N+1 without queue saturation, then receive seq N+2 and falsely diagnose a dropped frame.

What changed

  • Acquire the per-namespace stream lock before cloning the observer snapshot.
  • Keep membership snapshot, seq allocation, serialization, and non-blocking try_send fan-out in one serialized boundary.
  • Update the documented lock order to admission -> inflight -> event stream -> registry(read).
  • Add deterministic regression observer_join_after_stream_lock_is_included_in_that_sequence. It pauses an emit under the stream lock, registers a joining observer, and requires that observer to receive dense seq 1 and 2.

Validation

Validated on macmini against exact commit 9ba4e187ed1ad1f384f950b44ae5f08eb591dad4:

  • cargo fmt -p openab-cp -- --check: clean
  • New targeted regression: passed
  • Negative control restoring only the old snapshot-before-lock order: failed as expected with joining observer missed the locked emit (1 frame received vs 2 expected, exit 101)
  • cargo clippy -p openab-cp --all-targets -- -D warnings: clean
  • cargo test -p openab-cp: 141 unit + 9 integration tests passed (150 total)

I also audited every EventHub::emit call site; no reverse registry -> event stream lock nesting exists.

@chaodu-agent

Copy link
Copy Markdown
Collaborator

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 metadata_only cancel-reason bypass) are resolved.

What This PR Does

Adds Phase 1 of the CP observer/lobby capability on top of the already-merged CP core (#1469): a read-only observer agent type, a cp/event JSON-RPC notification stream with per-namespace sequence numbers, cp/list_agents roster snapshots, and the server/router wiring that emits lifecycle events at every hook (register, disconnect, lease expiry, delegation requested/completed/cancelled, timeout, target-disconnect). The lobby is an audit surface: best-effort fan-out, bounded excerpts, no durable replay.

How It Works

  • Read-only observers, four layers. AgentType::Observer is refused as an initiator unconditionally in policy::check (first check, no relaxation knob), excluded from registry.select() (never a delegation target even by exact name), rejected up front for cp/delegate|delegate_result|cancel by a server method guard, and bound at identity registration. No config path relaxes any of these.
  • Per-namespace dense seq. EventHub keeps one monotonic counter per namespace; membership snapshot, seq allocation, serialization, and non-blocking try_send fan-out all happen under a single per-namespace stream-lock critical section. That single linearization point is what this head commit (9ba4e18) fixed - snapshotting membership before the stream lock let a joining observer receive N, miss N+1, then receive N+2 and false-detect a gap.
  • Commit-first delivery. complete now runs peek -> cap -> commit -> emit -> deliver. Only the path that authoritatively removes the in-flight entry emits the observer terminal and the initiator-bound frame, so the two audiences can never record divergent outcomes for one admission. A result whose commit loses the race is dropped undelivered.
  • announced gate. An admission removed by teardown/sweep before delegate's announce/forward critical section ran emits no observer terminal and synthesizes no wire frames (no dangling requested, no result to a caller whose cp/delegate is already erroring); capacity release still applies.
  • metadata_only redaction. Agent-supplied content (prompt/result excerpts, worker failed error text, initiator cancel reasons) routes through excerpt/excerpt_opt and is suppressed key-absent; CP-synthesized diagnostics (timeout/disconnect) route through cp_diagnostic and survive by design.
  • Bounds are configuration, not hope. max_event_excerpt_bytes validated to 1..=65536, max_observers_per_namespace (>=1, atomic count-and-insert), inbound cancel-reason capped one layer before the excerpt path. Lock order documented and acyclic: admission -> inflight -> event stream -> registry(read).

Findings

# Severity Finding Location
1 🟢 Read-only observer invariant enforced at four independent layers with matching tests policy.rs, registry.rs, server.rs, proto.rs
2 🟢 Commit-first ordering removes CP-produced terminal divergence; documented, tested (result_racing_a_concurrent_cancel_is_dropped_undelivered) router.rs
3 🟢 Seq/membership atomicity fix is correct and regression-tested (observer_join_after_stream_lock_is_included_in_that_sequence) events.rs
4 🟢 metadata_only correctly distinguishes agent content from CP diagnostics; round-13 cancel-reason bypass resolved router.rs, events.rs
5 🟢 Fail-soft serialization on all teardown/Drop/sweeper paths avoids double-panic aborts router.rs, events.rs, server.rs
6 🟢 Excellent docs: wire contract, client contract, and accepted residual limits all documented docs/control-plane.md, docs/adr/agent-control-plane.md
Finding Details

🟢 F1: Read-only enforced in depth

Observer initiation is denied unconditionally as the first policy check, observers are filtered out of registry.select() so they cannot be routed to even by exact name, the server rejects the three delegation methods before dispatch, and cp/list_agents scopes to the caller's authenticated namespace. Tests: observer_initiation_always_denied, observers_never_selectable_but_listed, observers_are_rejected_from_the_delegation_methods.

🟢 F2: Commit-first delivery

The 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 CompleteOutcome::Completed { delivered, stalled_initiator } shape correctly keeps the serving-side ack byte-identical regardless of initiator liveness, so the reply is not an oracle. Accepted residual (documented): an observer may see completed for an admission whose disconnected initiator never received the frame.

🟢 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. concurrent_emits_enqueue_in_seq_order (8x16) and the join-under-lock test both guard it.

🟢 F4: metadata_only contract

cp_diagnostic vs excerpt/excerpt_opt is applied consistently across complete, cancel, fail_instance, and sweep_deadlines. The initiator cancel reason now goes through excerpt (round-13's critical) and is suppressed under metadata_only while still forwarded verbatim to the counterparty. Tests: metadata_only_omits_excerpts_but_keeps_metadata, oversized_error_truncated_like_result, precomputed_prompt_excerpt_is_truncated_and_metadata_only_aware.

🟢 F5: Fail-soft teardown

synthesized_frame and EventHub::emit drop a frame with an error log instead of panicking, which matters because they are reachable from RegistrationGuard::drop during an unwind. deliver_result deliberately keeps expect("serializable") since it runs on the request path, not a Drop - the distinction is documented.

🟢 F6: Documentation

The 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 metadata_only.

Addressing External Reviewer Feedback

@thepagent (Rounds 1-13)

The peer reviewer drove this PR through 13 rounds. The head commit 9ba4e18 and its predecessors resolve the tracked findings:

Round-13: agent-supplied cancel reason bypasses the metadata_only redaction contract (critical)

✅ Addressed: Router::cancel now emits reason: events.excerpt(&entry.namespace, &params.reason), so an initiator-supplied cancel reason is suppressed key-absent in a metadata_only namespace (the counterparty still receives it verbatim in the forwarded cp/cancel). CP-synthesized cancel reasons continue through cp_diagnostic.

Round-10/12: observer fan-out latency inside the delegation critical section; aggregate drop logging; observer population bound

✅ Addressed: max_observers_per_namespace (atomic count-and-insert, refused with SATURATED) bounds the population; one aggregated warn per emission (not per observer) bounds log I/O on the delegation path; the lock-hierarchy note documents the bounded work under the hold.

Round-6: error free text unbounded while result is capped

✅ Addressed: cap_payload truncates error with the same truncate_with_marker helper and max_result_bytes budget as result.

Baseline Check
Verification
  • Diff reviewed in full against main (events.rs, proto.rs, policy.rs, registry.rs, config.rs, router.rs, server.rs, docs, example config).
  • cargo test -p openab-cp could NOT be reproduced locally: the review sandbox has no C linker (cc not found), so the Rust toolchain cannot link. The PR body claims 81 passed; the CI check job on this exact head was still in_progress at review time. Merge should wait on green CI (check, operator) - branch protection already gates on it.
  • All other criteria verified by source reading and the in-tree tests enumerated above.

5. Three Reasons We Might Not Need This PR

  1. No client consumes it yet. Nothing dials the CP in a stock deployment (runtime client + relay are later PRs), so Phase 1 ships a protocol surface with zero production readers - it could sit unexercised until Phase 2/3. Counter: landing the wire contract now lets the lobby app and relay build against a stable surface, and the invariants are cheaper to get right before there are clients depending on them.
  2. The audit stream is lossy by design. Best-effort fan-out + non-durable seq + no replay means the lobby can silently miss frames; an audit surface that can drop events may give false confidence. Counter: the seq-gap + cp/list_agents resync is the explicit recovery contract, and the non-goal (this is a live view, not a durable audit log) is documented.
  3. Fan-out cost sits on the delegation hot path. Serializing and fanning out events runs inside the router's in-flight critical section, adding bounded-but-real latency per observer to every delegation. Counter: the cost is bounded by max_observers_per_namespace and max_event_excerpt_bytes, both validated, and sends are non-blocking.

@thepagent
thepagent merged commit 843bb72 into main Sep 8, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants