Repository navigation
feat(cp): land Agent Control Plane v1 on main - #1528
chaodu-agent wants to merge 5 commits into
Conversation
…ing, headless mode (PR 3/4) (#1471) * feat(cp): runtime control-plane client — [control_plane] config, worker delegation serving, headless mode (PR 3/4) PR 3/4 of the Agent Control Plane stack: the OAB runtime side. The runtime dials out to the CP, registers, serves `cp/delegate` through the same ACP turn driver every chat platform uses, and reports each turn as `cp/delegate_result`. Config surface (`[control_plane]`, `crates/openab-core/src/config.rs`) - `url`, `auth_key`, `namespace`, `name`, `type` (`primary` | `worker`), optional `labels`, `max_delegated_sessions` (default 1). - Presence is the opt-in — no cargo feature — following the `[mcp]` precedent. `deny_unknown_fields`, so a mistyped key fails startup instead of defaulting silently. `${ENV}` expansion comes from the shared loader. - `CpAgentType` has only Primary and Worker: `observer` is a read-only lobby role, so a runtime claiming it is unrepresentable rather than merely validated against. - Validation: non-empty url/auth_key/namespace/name, and `max_delegated_sessions > 0`. Each of these would otherwise fail at registration and reconnect forever. State machine (`control_plane/client.rs`) - One process-lifetime instance id (uuid), reused across reconnects: it identifies the replica, not the connection. - Connect ws/wss with `Authorization: Bearer` (never in the URL), send `cp/register` as the first frame, adopt `effective_max_delegated_sessions` from the ack, then one loop over inbound frames / heartbeat tick / delegation completions / shutdown watch. - On close or error: cancel every local in-flight delegation (each stops its agent and drops its session; the results are deliberately dropped, since the CP synthesizes `target_disconnected`), then 1/2/4/8/16/30s shutdown-aware backoff, reconnect, re-register. - CP-issued requests get JSON-RPC result acks; a delegation's outcome never rides in that ack. Delegation serving (`control_plane/executor.rs`) - Admission never executes: over-cap or duplicate `delegation_id` returns `status = failed` without touching the session pool. - Bounded by the nearer of the CP deadline and `[pool].prompt_hard_timeout_secs`. Outcomes map to completed / failed (agent error, broker error, silent failure) / timeout / cancelled. - One fresh session per delegation, keyed `control-plane:<sha256(instance_id + delegation_id)>`, discarded on every terminal outcome. Adds `SessionPool::discard_session` for the non-cancel teardown (`reset_session` would emit a spurious `session/cancel` and log an error for the benign already-gone case). Prompt seam refactor safety - `AdapterRouter::stream_prompt_blocks` now returns `Result<PromptExecution { final_text, terminal_error, silent_failure }>`. Nothing else changed inside it: two values are captured before the existing composition consumes them, and the delivery paths are byte-for-byte the same. `Err` still means what it meant (incomplete user-visible delivery). - All platform callers map back with `.map(|_| ())`, and `DispatchTarget` stays at `Result<()>`, so Discord/Slack/gateway/ambient — and the existing dispatch mock — are untouched. Every pre-existing adapter test passes unmodified. - The executor reuses that same driver with a private sink adapter (send-once, no placeholder, no reactions) and reads the reply from the return value. Feature split (Dockerfile-neutral) - `openab-cp` gains a default `server` feature gating config/registry/policy/ router/events/server and the binary (`required-features`); `proto` — the wire contract — stays unconditional, as do serde/serde_json/chrono. - `openab-core` depends on `openab-cp` with `default-features = false` for wire types only. Verified: `cargo tree -p openab-core -e normal` shows only chrono/serde/serde_json under openab-cp, an inverted axum query finds nothing, and `cargo build -p openab-cp` still produces the binary. - No new crate, no new runtime cargo feature. Headless mode (`src/main.rs`) - `[agent]` + `[control_plane] type = "worker"` with no adapter is a valid boot; a `primary` alone is not (its prompts come from a chat platform). `[mcp]`-only is unchanged, and mcp + worker runs both. - Extracted as `headless_run_mode` so the matrix is unit-tested. - Client spawned after router construction, wired to the existing shutdown watch, and stopped before `pool.shutdown()` so the CP sees a clean disconnect rather than a lease timeout. Security - `auth_key` is used only for the upgrade header (marked sensitive), is never logged, and never enters the agent child env — `[agent].env` plumbing is untouched. Tests - Config: absent section, full section incl. `type` rename and `${ENV}`, defaults, observer refusal, unknown-key refusal, every validation failure. - Run modes: the full headless matrix. - Executor: cap enforcement, duplicate id, all outcome mappings, and cancel/deadline cleanup asserting the session is gone. - `crates/openab-core/tests/cp_client.rs`: seven tests against the REAL in-process CP (ephemeral loopback port + `run_sweeper`), with a raw-WS primary as initiator and a scripted `PromptRunner` at the trait seam — register/ack, heartbeats holding the lease, delegate→result round trip, local deadline → `timeout`, cancel mid-flight, zero-lease sweep → reconnect + re-register (same instance id, and the new registration serves), and clean deregistration on shutdown. Deviations from the design 1. `openab-cp`'s `config.rs` is behind `server` too. The design said "proto.rs (+ config types needed by proto) stays unconditional"; proto needs nothing from config (the dependency runs the other way), so gating it keeps anyhow and toml out of the runtime build. 2. Completed delegations report through an mpsc channel rather than a `JoinSet` polled in the `select!`. The inbound branch must spawn while the completion branch is armed, and one `select!` cannot lend the same `JoinSet` to both — the borrow checker rejects it. Same for the socket, which is split into sink/stream halves. Behaviour is unchanged; handles are still tracked for the drain-then-abort teardown. 3. `axum` added as a dev-dependency of `openab-core`: `openab_cp::server::app` returns an `axum::Router`, and the integration test needs `axum::serve` to bind it. Test-only — the runtime links no HTTP server. Audit round-1 fixes (all four blocking findings): 1. [mcp] + [control_plane type=primary] now takes the full boot path so the facade and the CP client both run; facade-only had foreclosed the client. Run-mode test updated to pin both-run. 2. Reconnect backoff no longer resets on every clean close: a session must live STABLE_SESSION_SECS (60s) before a close resets backoff to 1s, so a CP that accepts-then-closes (lease misconfig, crash loop) faces 1/2/4/8/16/30 escalation instead of a 1s reconnect storm. 3. The executor's best-effort session/cancel before discard is now bounded at 5s (the pool's own cleanup bound): a wedged agent stdin can no longer block discard and leak the delegation slot. Both sites (cancel, deadline). 4. Shutdown is storm-proof: the dial/register phase races the shutdown signal via select (a hung connect cannot stall shutdown), and main aborts the client task if it misses the 10s deadline instead of detaching it over pool teardown. * fix(cp): cap delegate_result bodies below the CP transport frame limit Self-review finding. The worker sent the agent's full final text in cp/delegate_result (SinkAdapter deliberately does no chunking), but the CP enforces max_frame_bytes (default 1 MiB) at the WS transport BEFORE parsing — its router-level max_result_bytes truncation can never save an oversized frame. A single >1 MiB result therefore dropped the whole connection, and every in-flight delegation on that worker died as target_disconnected. The executor now caps Completed results at 512 KiB (half the default frame budget, leaving JSON-envelope headroom) with a UTF-8-boundary-safe truncation marker; the CP still applies its own, typically smaller, max_result_bytes on arrival. Regression test covers the oversized, multibyte-straddling, and under-cap cases. * fix(cp): free the delegation slot via RAII so task abort cannot leak it Self-review round-2 finding. serve() released its inflight slot with a plain call after the execute().await — but the client ABORTS serving tasks that outlive the 5s drain window on disconnect, and an aborted task never runs code after its await point. The leaked entry then survives reconnection (executor state is process-lifetime), permanently consuming capacity: with the default max_delegated_sessions = 1, one abort turns the worker into a zombie that refuses every delegation while heartbeating happily. The window is realistic, not theoretical: the bounded (5s) session/cancel added in the previous fix means a wedged agent stdin makes the task overrun the equally-sized drain window by construction. release() now lives in a Drop guard taken at admission, so the abort's unwind frees the slot. Regression test aborts a serving task mid-run and asserts active() == 0; mutation-verified (plain-release pattern fails it 5/5, the guard passes). * fix(cp): echo the CP admission token in every delegate_result and match it on cancel Round-8 made the admission token protocol-visible on the result path; round-9 extended it to cancellation. The runtime side of both: - The executor carries forward.admission into all five terminal frame kinds (completed, failed, refused-at-admission, timeout, cancelled). - The worker-side cancel handler matches (delegation_id, admission): a stale cancel naming a superseded admission of a reused id is ignored instead of aborting the live delegation — the worker-side half of the misdelivery the wire token closes. Regression pins both directions. - The initiator-side integration test echoes the ack's admission in its cp/cancel, exercising the CP's required-field validation end-to-end. The real-server integration suite is the first live client of the token-enforcing contract on both the result and cancel paths. * fix(review): cap error payloads, admission-scope session keys, bound teardown Round-7 review fixes: - F1: failed() now caps every error string (64 KiB, UTF-8-safe) inside the constructor so no call site can bypass it; oversized-error regression test mirrors the success-path one - F34: delegation_session_key mixes in the CP admission token, so a re-admission of a reusable id can never resume an earlier admission's session (e.g. one orphaned by a drain-timeout abort); false reconnect claim in the doc corrected; key-property test pins admission scoping - F22/F31: duplicated cancel+discard blocks extracted into bounded helpers; discard now shares the 5s bound instead of being unbounded on every terminal path - F15: client WS now sets max_message_size/max_frame_size (1 MiB), mirroring the CP's accept-side max_frame_bytes instead of tungstenite's 64 MiB default - F5: the cp/register ack wait is bounded (10s, mirroring the CP's register_timeout_secs) so a stalled CP enters backoff instead of hanging - F27: config/docs no longer claim a local min() the code does not implement — the runtime enforces the ack value - F37: ADR section 7 facade label corrected from PR 3/4 to PR 4/4 * fix(review): bound escaped result payloads, share teardown bound, defer success discard Round-8 review fixes: - F46: cap_text and cap_result now evaluate JSON-escaped byte length instead of raw UTF-8 length so control-character and quote/slash escape inflation cannot inflate a 512 KiB result past the CP's pre-parse 1 MiB max_frame_bytes limit; regression test pins escape-heavy and quote-heavy inflation safety; client::send() adds an outbound transport size guard - F52: cancel_and_discard() now shares a single 5s deadline across both the cancel and discard phases (discard uses whatever budget remains, or is skipped if cancel consumed the bound), keeping total teardown strictly <= DRAIN_TIMEOUT (5s); regression test pins stalled cancel + stalled discard bound - F53: success path now spawns session discard in the background off the critical path, preventing a slow or wedged discard from delaying result delivery to the initiator; safe because session keys are admission-scoped - F54: executor module-level invariant doc updated to state the three-parameter session key (instance_id, delegation_id, admission) - F55: truncation marker uses ASCII "..." instead of U+2026 ellipsis - F51: documented MAX_INBOUND_FRAME_BYTES relation to CP-side configurable max_frame_bytes and future RegisterAck negotiation direction * fix(clippy): remove useless format! invocations * fix(review): route register through send() guard, clarify primary headless doc (review nits) * fix(cp): resolve runtime client review findings * fix(cp): address runtime client regression findings * docs(cp): document delegation context schema --------- Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
pending-maintainer
pending-contributor
and removed
pending-maintainer
labels
* feat(cp): add primary delegation tools and local API * fix(control-plane): address PR4 review findings * fix(control-plane): order terminal eviction by completion --------- Co-authored-by: chaodu-agent <chaodu-agent@users.noreply.github.com> Co-authored-by: chaodu-agent <274062505+chaodu-agent@users.noreply.github.com>
chaodu-agent
changed the title
This comment has been minimized.
This comment has been minimized.
- Cargo.lock: #1529 added the `reqwest013` dev-dependency to the root crate but the lockfile's `openab` block was never regenerated, so `cargo check --workspace --all-features --locked` failed and any plain `cargo check` rewrote the lock. Regenerated; `--locked` now passes. - main.rs: `report_facade_status` was keyed on `cfg.mcp.is_some()`, so a headless control-plane primary without `[mcp]` logged "unconfigured — nothing was started" while the automatic loopback facade on 127.0.0.1:8848 was in fact serving. Key it on `facade_serving`, the same bool that decides whether the listener is spawned. Self-review round 3 findings F1/F2 on #1528.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![chaodu-obk[bot]](https://avatars.githubusercontent.com/u/262003297?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1528 (comment)
| match self.classify_inbound(&text, primary) { | ||
| FrameAction::Serve { ack, forward } => { | ||
| let executor = Arc::clone(&self.executor); | ||
| let tx = result_tx.clone(); | ||
| serving.push(tokio::spawn(async move { | ||
| let result = executor.serve(forward).await; | ||
| // A closed channel means the connection | ||
| // that would carry this result is gone; | ||
| // the CP fails it as target_disconnected. | ||
| let _ = tx.send(result).await; | ||
| })); | ||
| // Prune finished tasks so a long-lived | ||
| // connection does not accumulate handles. | ||
| serving.retain(|h| !h.is_finished()); | ||
| if sink.send(Message::Text(ack)).await.is_err() { |
There was a problem hiding this comment.
🔴 F1 - Preserve cancellation before worker admission
This task is spawned before executor.serve is polled and inserts the admission. If the next ordered frame is cp/cancel, executor.cancel() sees no inflight entry, the cancel is still acknowledged, and this task can later admit and run cancelled work.
Requested change: reserve admission synchronously before spawn/ack, or retain admission-scoped pre-cancel tombstones. Add a deterministic test that handles cancel before the serving future's first poll and asserts the runner never starts.
| let facade_sessions = openab_mcp::mcp::sources::SessionTokens::new(); | ||
| // Only read under the acp feature (pool facade wiring below). | ||
| #[cfg(feature = "acp")] | ||
| let facade_serving = cfg.mcp.is_some(); | ||
| let facade_serving = facade_listen.is_some(); | ||
| // Startup, not per-session: report whether the facade is serving, so an operator learns it | ||
| // here rather than by inferring it from tools that never appear. This is NOT the | ||
| // `OPENAB_BROWSER_MODE` migration notice — that was removed (see `acp_mcp`), and nothing | ||
| // reports the variable now. | ||
| // Gated on `acp` (the root feature that pulls in core's `acp-mcp`), not on `acp-mcp` itself — | ||
| // that is a core feature and naming it here is an unknown-cfg error. | ||
| // | ||
| // Keyed on `facade_serving`, NOT `cfg.mcp.is_some()`: a headless control-plane primary | ||
| // without `[mcp]` still gets the automatic loopback facade (see `facade_listen` above), and | ||
| // reporting "nothing was started — add [mcp]" while that listener is up would contradict | ||
| // the running process. | ||
| #[cfg(feature = "acp")] | ||
| openab_core::acp_mcp::report_facade_status(cfg.mcp.is_some(), &cfg.agent.working_dir); | ||
| if let Some(mcp_cfg) = cfg.mcp.clone() { | ||
| let listen = mcp_cfg.listen.clone(); | ||
| openab_core::acp_mcp::report_facade_status(facade_serving, &cfg.agent.working_dir); |
There was a problem hiding this comment.
🔴 F2 - Make automatic headless-primary tools reachable
This automatic path starts with an empty SessionTokens registry, while ControlPlaneTools requires a resolved session. The only production mint path is creation of an acp: session, but an adapter-less primary creates no chat session and does not serve worker delegations. The four advertised tools are therefore hidden/unreachable. This status call also runs before bind and selects text claiming [mcp] is configured and an entry was written, which is false here.
Requested change: implement a real bootstrap session/token plus agent configuration or environment injection for this mode, test an authenticated tool call end to end, and report readiness only after bind/injection succeeds.
| pub struct PrimaryState { | ||
| /// JSON-RPC id → the request the loop originated and awaits a reply for. | ||
| pending: HashMap<u64, PendingRequest>, | ||
| /// (delegation_id, admission) → its lifecycle. | ||
| tracked: HashMap<DelegationKey, TrackedDelegation>, | ||
| /// Insertion order for deterministic oldest-terminal history eviction. | ||
| tracked_order: VecDeque<DelegationKey>, | ||
| /// Callers awaiting a terminal result, keyed by the same composite key. | ||
| /// A caller can await before OR after the terminal arrives. | ||
| awaiters: HashMap<DelegationKey, Vec<AwaitReply>>, |
There was a problem hiding this comment.
🟡 F3 - Bound and expire parked primary requests
The mpsc ingress is capped, but entries moved into pending and awaiters have no count cap or expiry. A connected CP can accept writes while withholding replies or terminal frames, allowing these maps and the Unix-socket tasks waiting on their oneshots to grow for process life. Caller-side spawn timeout does not remove the parked server-side entry.
Requested change: add per-request expiry, caller-drop cleanup, fixed pending/handler caps, and bounded local I/O/response waits. Test a connected-but-withholding peer and assert state returns to a fixed bound.
| Ok(frame) | ||
| } | ||
|
|
||
| async fn send(sink: &mut WsSink, frame: &JsonRpcRequest) -> anyhow::Result<()> { | ||
| let text = serde_json::to_string(frame)?; | ||
| if text.len() > MAX_FRAME_BYTES { | ||
| anyhow::bail!( | ||
| "outbound frame ({} bytes) exceeds transport limit ({} bytes)", | ||
| text.len(), | ||
| MAX_FRAME_BYTES | ||
| ); | ||
| } | ||
| sink.send(Message::Text(text)).await?; |
There was a problem hiding this comment.
🟡 F4 - Put a deadline on every runtime WebSocket write
This helper awaits the sink without a timeout, and several ack/reply/pong writes bypass even this helper. Because writes run inside the sole serve loop, one backpressured peer stops inbound processing, heartbeats, completions, command traffic, and graceful shutdown.
Requested change: route all writes through one bounded helper raced against shutdown; on timeout, disconnect, fail/cancel live state, and reconnect. Add a stalled-sink regression test.
| pub fn delegation_session_key( | ||
| instance_id: &str, | ||
| delegation_id: &str, | ||
| admission: AdmissionToken, | ||
| ) -> String { | ||
| let mut hasher = Sha256::new(); | ||
| hasher.update(instance_id.as_bytes()); | ||
| hasher.update(delegation_id.as_bytes()); | ||
| hasher.update(admission.to_be_bytes()); | ||
| format!("control-plane:{:x}", hasher.finalize()) |
There was a problem hiding this comment.
🟡 F5 - Reclaim creation gates for one-shot admission keys
A unique session key per admission is correct for isolation, but SessionPool::get_or_create retains a strong creating gate per key and discard_session deliberately does not remove it. Every completed delegation therefore leaves a permanent string, Arc, and mutex; max_sessions does not bound this map.
Requested change: preserve same-key serialization with reclaimable gates (for example Weak entries plus identity-checked final-holder cleanup) and test many unique admissions while asserting bounded gate cardinality.
| mut shutdown: watch::Receiver<bool>, | ||
| ) -> anyhow::Result<()> { | ||
| prepare_socket_dir(&path)?; | ||
| remove_stale_socket(&path)?; |
There was a problem hiding this comment.
🟡 F6 - Do not unlink a live same-UID runtime socket
remove_stale_socket checks type and UID but not liveness or path generation. A second runtime can unlink the first live pathname and bind a successor; the older runtime's unconditional shutdown cleanup can then unlink that successor.
Requested change: hold an exclusive lifetime lock, refuse a live listener, and make cleanup generation/inode-aware. Add two-live-runtime and predecessor-cleanup tests.
F1 cancel-before-admission: the connection loop spawned `executor.serve`, so admission happened on the task's first poll and a `cp/cancel` that was the very next frame found nothing in flight, was acked as unknown, and the task then admitted and ran cancelled work. `serve` is split into a synchronous `admit_delegation` (called on the loop's own task before it yields) and `Admitted::run`; the reservation is an owned RAII value so an ack-write failure or task abort still frees the slot. Refusals are answered inline after the ack. F4 unbounded writes: every WebSocket write in the single-owner loop goes through one bounded helper (30s, mirroring the CP's write_timeout) raced against shutdown. A stall classifies the connection as disconnected and reconnects; shutdown never waits on a stalled peer. F3 unbounded pending state: PrimaryState caps outstanding CP requests at 256 (`Overloaded` beyond), ages unanswered requests out after 60s (`Timeout`), drops awaiters whose callers left, and drops tracking for a delegation still non-terminal 60s past its deadline, answering its waiters `Timeout`. `prune` runs on every heartbeat tick. The local socket server bounds concurrent handlers at 64 (one error line, then close), idles connections out after 5 minutes, bounds response writes and every CP round trip, and aborts remaining handlers on shutdown. F5 creating-gate leak: `purge_session_entries` now drops a key's creating gate when the map holds the last reference (checked under the state write lock), so single-use control-plane session keys no longer leave a permanent String + Arc<Mutex> behind. F6 live-socket unlink: `remove_stale_socket` connect-probes an owned socket node and refuses to replace a listener that answers, pointing at OPENAB_AGENT_SOCKET; only ECONNREFUSED is unlinked. The bound node's (dev, ino) is recorded and shutdown removes the path only if it still names that node, so an older runtime cannot delete a successor's socket. F2 unreachable MCP surface: the only production mint of a facade session token is an `acp:` session, so chat-adapter and adapter-less primaries never see the four tools. v1 is narrowed honestly rather than widened: docs/control-plane.md, the ADR, and config-reference.md now state that reach and name the CLI as the initiating surface for those deployments; per-adapter injection is recorded as explicitly deferred. The startup report takes a three-way FacadeMode so the automatic facade is neither reported as "[mcp] configured" nor as "nothing was started". Also from #1530: `admit()` reads the ceiling under the inflight lock, and a `cp/delegate` ack that repeats an already-tracked admission is rejected instead of overwriting the entry. Tests: cancel handled before first poll never starts the runner; a dropped Admitted releases its slot; stalled write -> Disconnected at the bound; shutdown wins over a stalled write; pending cap/TTL/orphan/overdue pruning; terminal history untouched by prune; duplicate ack rejected; gate map bounded across 1000 single-use keys; live same-uid socket refused (helper and full boot path); predecessor cleanup leaves a successor's node; connections past the cap get a diagnostic and a freed slot is served.
|
Note Round 4 findings addressed in Addressing Round-4 Review Feedback🔴 F1 — Cancel-before-admission is lost
✅ Addressed in Regression tests ( 🔴 F2 — Automatic headless-primary tools cannot authenticate
✅ Addressed in
The ask to "define and implement the real bootstrap session/token … for an adapter-less primary" is recorded as Follow-up 5: it needs a decision on rotation/revocation for long-lived chat sessions and is out of this PR's frozen Goal. What ships is honest about it. 🟡 F3 — Pending request state and local waits are unbounded
✅ Addressed in Tests: 🟡 F4 — A stalled runtime WebSocket write blocks all progress
✅ Addressed in Tests ( 🟡 F5 — Admission-scoped creation gates accumulate forever
✅ Addressed in 🟡 F6 — Live same-UID Unix sockets are treated as stale
✅ Addressed in Tests ( Follow-through Also closed from #1530
VerifiedClean
Diff LineageF1 REGRESSION-class correctness within the frozen scope (cancel semantics) — fixed. F2 NEW EVIDENCE against the Goal's MCP clause — resolved by contract revision (owner-authorized) and documentation, deferred implementation tracked as Follow-up 5. F3–F6 correctness/liveness within scope — fixed. #1530 keeps the remaining non-blocking items (managed-parent verification, deadline-range constant, bail string, heartbeat test margin). |
CI (Linux) failed `cleanup_leaves_a_successors_node_alone`: ext4 hands the very next create in a directory the inode number just freed by unlink, so a successor's socket node can carry the predecessor's exact (dev, ino). Identity alone therefore cannot protect the successor there. Shutdown now closes our own listener first and unlinks only if the node still matches our identity AND a connect probe is refused — anyone who answers after we stopped listening is a successor. The identity check still catches replaced nodes on filesystems with fresh inode numbers; the probe catches ext4's reuse. Reproduced on an ext4 host, fixed test passes there (850/850) and on macOS; a second test covers a foreign dead node being left alone.
|
> [!IMPORTANT] What This PR DoesLands the complete Agent Control Plane v1 stack on How It Works
Findings
Finding Details🔴 F1: Required CI is red on the reviewed headThe main Requested change: identify the failing test from the CI logs, fix or justify it, and get the required check green. Note the PR's own final acceptance criterion is still unchecked and 🟡 F2: Narrowed contract not applied to all claimsThe core reach documentation is consistent and honest (control-plane.md "Who can reach the MCP tools in v1", config-reference.md, ADR reach callout, startup log). But the outer claims were not synced: README.md:61 and README.zh-TW.md:61 still advertise four MCP tools for primaries unconditionally; ADR section 6 still titles the MCP facade "primary interface" while most primary deployments cannot reach it in v1; the ADR alternatives table still rejects a CLI-only interface even though CLI-only is exactly what v1 delivers for chat-adapter and adapter-less primaries; and the PR body Summary bullet still says "automatic MCP facade/session injection", contradicting the body's own Non-goals. Requested change: qualify the README bullets with the v1 reach, align the ADR heading/alternatives wording with the narrowed scope, and fix PR body Summary bullet 6. 🟡 F3: Prune cadence is CP-controlled
Requested change: clamp the prune cadence (e.g. 🟡 F4: Refusal tasks break the local connection boundAt the cap, Requested change: do not count refusals against the live-handler cap (drop the stream, or bound refusals separately); add a flood test asserting the set stays bounded. 🟡 F5: Shutdown unlink TOCTOU (F6 residual)
Requested change: preferred fix is to not unlink on shutdown at all — startup's probe-plus- 🟡 F6: Blocking connect probes are unboundedBoth probes use std blocking Requested change: bound the probe (nonblocking connect with a short poll deadline, or 🟡 F7:
|
| Reviewer | Scope | Outcome |
|---|---|---|
| Reviewer A | Architecture, API contract | CHANGES REQUESTED: findings 2, 7 |
| Reviewer B | Performance, resource lifecycle | CHANGES REQUESTED: finding 3 |
| Reviewer C | Correctness, cancellation semantics | finding 10; round-4 fixes confirmed |
| Reviewer D | Readability plus general | findings 11; round-4 fixes confirmed |
| Reviewer E | Safety, concurrency | CHANGES REQUESTED: findings 4, 5 |
| Reviewer F | Docs/UX, config accuracy | LGTM in lane; praise findings 12–13 |
| Reviewer G | Security, CI | CHANGES REQUESTED: finding 1; security posture clean |
| Reviewer H | Decision boundary (F2 narrowing) | Endorsed narrowing; findings 1 (confirm), 2, 3 (confirm), 8, 14 |
| Reviewer I | Operability, shutdown/drain | CHANGES REQUESTED: findings 6, 8, 9 |
| Reviewer J | Simplicity, scope | LGTM in lane; review-lineage note folded into Baseline |
Three Reasons We Might Not Need This PR
- v1 utility is narrower than the headline — after the F2 narrowing, the most common deployment shape (chat-driven primaries) cannot use the MCP delegation surface at all; v1 delivers CP transport, worker execution, and operator/ACP-session initiation. If per-session injection (Follow-up 5) stalls, the flagship agent-to-agent story stays unshipped. Rebuttal: the CLI surface is fully functional and shares the same enforcement path, and landing the reviewed transport unblocks incremental adoption.
- A 10k-line roll-up before any production canary — sweeper supervision (fix(cp): sweeper task has no panic supervision or liveness signal #1474), drain (feat(cp): container-grade shutdown — SIGTERM handling and connection drain #1475), and packaging are all deferred, so maintainers carry transitional failure modes. Rebuttal: the code is opt-in behind
[control_plane], and carrying a 10k-line branch out-of-tree rots faster than landing it under CI. - The auto-facade serves deployments that cannot use it — a fixed-port listener with fatal bind for clients that can never authenticate is surface without demand (finding 8). Rebuttal: loopback-only and session-gated, so the risk is operational rather than security; a follow-up can gate auto-start.
github-actions
Bot
added
the
closing-soon
|
Caution This PR has been waiting on the author for more than 2 days (labeled @chaodu-agent — 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. |
1 participant
Summary
Land the complete Agent Control Plane v1 stack on current
main:spawn_agent,check_delegation,list_agents,cancel_delegationopenab agent spawn|list|status|cancelThis PR began as the recovery replay for #1471 after that PR was merged into an obsolete stacked base rather than
main. Stacked PR #1529 has now merged into this branch, so #1528 is the final roll-up that makes the complete reviewed stack reachable frommain.Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1532377620241449040
Review Contract
Goal
Make Agent Control Plane v1 reachable from
main: Unix-hosted OpenAB runtimes can register as primaries/workers, serve and initiate delegations over one CP WebSocket, and expose the initiating surface through session-gated MCP tools and the equivalent owner-only local CLI API without exposing CP credentials to agents or operators.Non-goals
cp/attach), session streaming, or lobby UI.openab-cpimage/release packaging and production canary deployment.acp:sessions. In v1 the facade session token (and the agent's facade entry) is minted only for sessions opened through the ACP-over-WebSocket gateway; chat-adapter sessions and adapter-less primaries initiate through theopenab agentCLI. The docs, ADR, and startup log state this reach explicitly (round-4 F2).Accepted Residual Risks
unknown handle; eviction does not cancel live CP work.[mcp]is absent. A bind conflict fails visibly. Its four tools are reachable only fromacp:sessions (see Non-goals); the startup report names the mode and that reach.Overloadedbeyond), unanswered requests failedTimeoutafter 60s, and a delegation still non-terminal 60s past its own deadline dropped from tracking with waiters answeredTimeout. A terminal frame arriving after that is acked and ignored.OPENAB_AGENT_SOCKET.write_timeout_secs); a stall is treated as a disconnect and recovered by reconnect.Acceptance Criteria
(delegation_id, admission)with first-terminal-wins semantics.Follow-ups
openab-cpimage/release workflow and single-replica canary.cp/attach, session streaming, lobby UI, and durable event history.acp:sessions (chat adapters, adapter-less primaries), including rotation/revocation for long-lived chat sessions.max_frame_bytesbail string, heartbeat test margin).Architecture
Review History
a2bd011909989d1c491437a1e4fd6c05a5dafc54; Sol and Fable final audits returnedPASS, and code-owner review approved it before merge.3742e03) found a staleCargo.lockand a misreported auto-facade status; both fixed in644024f. Remaining hardening items were filed as refactor(cp): post-v1 hardening follow-ups from #1528 self-review #1530.644024f) raised six findings: cancel-before-admission race (F1), no production credential path for the automatic facade's tools (F2), unbounded pending/awaiter/handler state (F3), unbounded WebSocket writes (F4), creating-gate leak per delegation (F5), and live same-uid socket unlink (F6). All six are addressed inc061158(+91c0a89, which makes the F6 shutdown cleanup ext4-safe after Linux CI caught inode-number reuse): F1/F3/F4/F5/F6 by code plus regression tests; F2 by narrowing the v1 contract (this body, docs, ADR, startup log) rather than adding a bootstrap identity model. Two refactor(cp): post-v1 hardening follow-ups from #1528 self-review #1530 items (admit()lock ordering, duplicate-ack guard) rode along.Verification
Validated on macmini before the stacked merge:
--all-features: passed--no-default-features: passedopenab-core,openab-mcp, root): passed with-D warningsopenab-cp: 142 unit + 9 integration passedopenab-mcp: 245 passedopenab-core: 848 unit + 11 real-CP integration passed (one known unrelated macOS secret test,secrets::tests::resolve_exec_nonzero_exit, fails identically onmain)cargo check --workspace --all-features --locked,cargo check -p openab-core --no-default-features --locked,cargo check --no-default-features --locked: passed atc061158;91c0a89re-verified on macOS and ext4 Linux (850/850 core lib on Linux)rustfmt --checkoncrates/openab-core/src/control_plane/*.rs: cleanPASS/PASSCurrent-main GitHub merge-ref CI is running on
91c0a89; the final checkbox above will be updated only after all required checks are terminal and green.