Skip to content

feat(cp): openab-cp control plane — registry, router, policy (PR 1/4) - #1469

Open
chaodu-agent wants to merge 2 commits into
mainfrom
feat/openab-cp
Open

feat(cp): openab-cp control plane — registry, router, policy (PR 1/4)#1469
chaodu-agent wants to merge 2 commits into
mainfrom
feat/openab-cp

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

What problem does this solve?

First implementation slice (PR 1 of 4) of the Agent Control Plane ADR (#1465): the standalone openab-cp binary — wire protocol, identity-bound registry, CP-authoritative policy engine, and delegation router. Agents delegate to each other over ACP/WS instead of round-tripping through Discord/Telegram.

Planned follow-up PRs: (2) OAB-side [control_plane] config + registration client + worker pool wiring, (3) MCP facade over UDS + openab agent <verb> CLI, (4) streaming + observer endpoint.

What's in this PR

  • crates/openab-cp (new workspace member, standalone binary per the openab-gateway precedent):
    • proto.rs — versioned JSON-RPC 2.0 envelopes; cp/register(+ack), cp/heartbeat, cp/delegate, cp/delegate_result, cp/cancel; distinct machine-actionable error codes (IDENTITY_MISMATCH, POLICY_DENIED, NO_TARGET, SATURATED, DEADLINE_EXCEEDED, TARGET_DISCONNECTED, DUPLICATE_DELEGATION, UNSUPPORTED_VERSION)
    • config.rs — CP-owned identity table (auth key → immutable namespace/name/type claims, ${ENV_VAR} expansion, constant-time key lookup, duplicate-key rejection), per-namespace policy overrides
    • registry.rs — instance registry with replica semantics (rolling deploys route new work to the newest healthy instance), lease/heartbeat expiry, saturation-aware selection distinguishing NO_TARGET from SATURATED
    • policy.rs — CP-authoritative checks: initiator role, depth, cycle (incl. self), namespace boundary, deadline sanity/cap/parent-budget
    • router.rs — in-flight table; CP-constructed chain (callers pass only parent_delegation_id; ancestry derives from authenticated identities — unforgeable); deadline sweep (synthesized timeout + best-effort cp/cancel); disconnect handling both directions; result size truncation; idempotency
    • server.rs — axum WS server; auth via Authorization: Bearer at upgrade (keys never in URLs); mandatory cp/register first frame verified against the key's bound identity
  • ADR amendments (docs/adr/agent-control-plane.md): new "v1 contract amendments" section freezing the docs(adr): agent control plane for direct inter-agent delegation #1465 review findings (identity binding, CP-authoritative policy, restart/saturation/timeout semantics, result cap, idempotency); §11 Q1 streaming resolved as committed fast-follow with the observer endpoint sketch.

Review Contract

Goal

Ship a correct, tested control-plane core (registry + router + policy + wire contract) that later PRs (runtime client, facade, streaming) build on without protocol changes, implementing the frozen ADR baseline including the #1465 review checklist items.

Non-goals

No OAB-runtime-side code: no [control_plane] config section, no registration client, no MCP facade/UDS/CLI, no streaming/observer endpoint (PRs 2–4). No CP HA (single instance; restart semantics are defined and implemented). No durable state, queueing, or delegation history. No Docker/Helm packaging.

Accepted Residual Risks

  • Wire contract is exercised by unit tests, not yet by a real OAB runtime client; PR 2 integration may surface adjustments. Mitigation: protocol_version field + UNSUPPORTED_VERSION rejection gives a compatibility gate.
  • Registered-runtime JSON-RPC responses to CP-forwarded frames are logged and dropped (correlation is by delegation_id, not rpc id). Acceptable for v1; revisit if per-frame acks become necessary.
  • cp/cancel to a serving runtime is best-effort; a runtime that ignores it burns tokens until its own deadline handling stops it. The propagated deadline is the hard upper bound.

Acceptance Criteria

  • Registration rejects: wrong first frame, unsupported protocol version, identity claim mismatch (namespace/name/type each), empty instance_id — all tested
  • Policy denies: worker initiation (default), depth > namespace max, cycles incl. self-delegation, cross-namespace, past/over-cap/over-parent deadlines — all tested
  • Router: happy-path roundtrip with CP-stamped chain; duplicate id rejection; saturation fast-fail; deadline sweep synthesizing timeout+cancel; worker/initiator disconnect handling; spoofed-completion rejection; late-result drop (CP restart case); oversized-result truncation — all tested
  • cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test -p openab-cp (42/42), cargo check --workspace all pass
  • No mockall, no network/fs in unit tests, tests inline #[cfg(test)]

Follow-ups

  • PR 2: [control_plane] OAB config + outbound registration client (gateway.rs backoff shape) + worker pool wiring
  • PR 3: MCP facade (spawn_agent, check_delegation, list_agents, cancel_delegation) over UDS + CLI
  • PR 4: streaming frames + read-only /observe endpoint + openab-cp tail
  • Packaging (Dockerfile/Helm) once PR 2 makes the binary deployable end-to-end

Verified

On macmini at the squashed head (content-identical to ac9c023):

  • cargo fmt -p openab-cp -- --check — clean
  • cargo clippy -p openab-cp --all-targets -- -D warnings — clean
  • cargo test -p openab-cp — 42 passed, 0 failed
  • cargo check --workspace — passes with the new member

@github-actions

Copy link
Copy Markdown

👥 Top 3 recent committers of changed files

Login Last commit Commit File
chaodu-agent 2026-08-10 448b05fb docs/adr/agent-control-plane.md
brettchien 2026-08-04 3ace7de3 Cargo.toml
canyugs 2026-07-30 c5a75ac6 Cargo.toml

Updated for e7c29c8

@chaodu-obk

This comment has been minimized.

@chaodu-obk

This comment has been minimized.

@chaodu-agent
chaodu-agent force-pushed the feat/openab-cp branch 2 times, most recently from 2f15fe8 to 81cd5f7 Compare August 11, 2026 03:40
… PR 1/4)

Implements the first slice of docs/adr/agent-control-plane.md: the
standalone openab-cp binary with wire protocol, identity-bound registry,
CP-authoritative policy engine, and delegation router.

Addresses round-1 review findings: CP-generated registration handles for
all ownership (F1), atomic delegation admission with insert-before-send
(F2), parent delegation bound to its serving instance (F3), JSON-RPC 2.0
envelope validation (F4), transport/prompt/queue resource bounds (F5),
lease-only heartbeats + least-loaded label scheduling (F6), and unrelated
artifacts removed from the diff (F7).
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

All round-1 findings addressed in e2853b4 (branch rewritten; the unrelated artifacts are gone from the diff entirely).

# Finding Resolution
F1 🔴 Client-selected instance_id as registry/ownership key ✅ Registrations now keyed by a CP-generated handle (registry.rs); all ownership checks (completion, cancel, parent linkage, teardown) compare handles. A colliding instance_id cannot replace or tear down another connection's entry — regression tests colliding_instance_id_cannot_replace_other_registration, result_from_wrong_handle_dropped_and_restored
F2 🔴 Check-then-act admission; send-before-insert ✅ Admission (duplicate check → parent lookup → selection → capacity reservation → in-flight insert) is one atomic sequence under an admission lock; the in-flight entry exists before the forward frame is sent, with rollback on send failure — tests inflight_exists_before_target_receives_frame, send_failure_rolls_back_reservation
F3 🔴 Any live parent_delegation_id accepted ✅ The caller must be the instance serving the parent (p.to_handle == from_handle); unknown and unauthorized parents return the same non-enumerating error — test chain_extends_through_parent_and_foreign_parent_rejected (foreign-parent negative case included)
F4 🟡 JSON-RPC 2.0 envelope not validated jsonrpc field added to the incoming type; require_request_envelope() enforces "2.0" + request id for all cp/* methods, returning INVALID_REQUEST (-32600) — tests request_envelope_validation, register_invalid_envelope_rejected
F5 🟡 No pre-allocation resource bounds ✅ WS transport enforces max_frame_bytes (default 1 MiB) via max_message_size/max_frame_size before parsing; max_prompt_bytes rejects oversized prompts; per-connection outbound queues are bounded (256) with disconnect-on-overflow (try_send everywhere)
F6 🟡 Heartbeat count merge; recency-over-load label scheduling ✅ Heartbeats refresh the lease only — CP-owned counts are authoritative (test heartbeat_does_not_mutate_session_count); label selection is least-loaded-first with recency tie-break, exact-name keeps the newest-replica rule (inverse recency/load test label_selection_least_loaded_first)
F7 🟡 Unrelated artifacts in diff ✅ Branch rewritten from main; diff now contains only crates/openab-cp, workspace Cargo.toml/Cargo.lock, the ADR amendment, and the Dockerfile stub-layer updates required for the new workspace member

ADR's "v1 contract amendments" section updated to record the handle-based ownership, envelope validation, and resource-bound semantics.

Verified at e2853b4 on the build host: cargo fmt --check OK, cargo clippy --all-targets -- -D warnings clean, cargo test -p openab-cp 48/48 (up from 42 — six new regression tests for F1/F2/F3/F4/F6), cargo check --workspace passes.

…ult cap

Round-2 review F4/F5: bearer keys never cross cleartext non-loopback TCP
without an explicit allow_insecure_bind acknowledgment (TLS proxy or
private network required), and result truncation counts the marker
against max_result_bytes.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Round-2 response — note the round-2 review examined stale head 2842d264 (pre-rewrite); findings F1/F2/F3/F6/F7 were already resolved in the branch rewrite (e2853b4, see the previous response comment for the per-finding test evidence). The two findings that applied to current code are now fixed in 33ed835:

# Finding Resolution
F4 🔴 Bearer credentials over cleartext non-loopback TCP ✅ Default bind is now 127.0.0.1:9800; validate() rejects any non-loopback bind unless allow_insecure_bind = true is set explicitly, documented as requiring a TLS-terminating proxy (wss://) or a private network (tailnet) in front. cp.toml.example updated. Test: non_loopback_bind_requires_override (covers 0.0.0.0 rejection, override acceptance, and loopback variants incl. [::1])
F5 🟡 (marker overflow) Truncation marker pushed results past max_result_bytes ✅ The marker now counts against the cap (floor_char_boundary budget math); degenerate tiny caps also never exceed the limit. Test: oversized_result_truncated asserts result.len() <= cap for both normal and degenerate caps

The remainder of F5 (bounded outbound queues, max_frame_bytes WS transport limit, max_prompt_bytes rejection) was already in e2853b4.

Per the review contract's stopping rule, round 3 should verify these fixes against head 33ed835 and check for regressions only.

Verified at 33ed835 on the build host: fmt clean, clippy --all-targets -D warnings clean, 49/49 tests, cargo check --workspace passes.

@chaodu-obk

chaodu-obk Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

CHANGES REQUESTED ⚠️ - The current control-plane core still has lifecycle, ownership-race, namespace-isolation, and authenticated-admission defects that can drop work or leave runtimes unrecoverable.

What This PR Does

This adds the first openab-cp control-plane binary: a WebSocket JSON-RPC service that authenticates runtimes, registers instances, applies delegation policy, selects targets, and routes delegation results. It also adds the protocol, configuration, registry, router, ADR amendments, and Docker cache-manifest updates.

How It Works

A runtime authenticates during the WebSocket upgrade and must register first. The CP assigns a registration handle, derives delegation ancestry from its in-flight table, applies CP-owned policy, reserves target capacity, and forwards frames through bounded per-connection queues. A background sweeper expires leases and deadlines.

Findings

# Severity Finding Location
1 🟡 Important Lease expiry deregisters an instance but leaves its WebSocket alive and unable to recover or re-register. crates/openab-cp/src/server.rs:118-137, 325, 441-452
2 🟡 Important Completion and cancellation temporarily remove an in-flight entry before checking ownership, allowing a competing frame to make a legitimate result look unknown and be dropped. crates/openab-cp/src/router.rs:237-258, 311-329
3 🟡 Important A global client-controlled delegation_id keyspace crosses the stated namespace boundary and creates a cross-namespace denial/existence oracle. crates/openab-cp/src/router.rs:95, 311-329
4 🟡 Important Authenticated peers have no registration deadline or connection quota, so one valid identity can retain unlimited pre-registration sockets. crates/openab-cp/src/server.rs:68-102
5 🟢 Praise Registration ownership is correctly based on CP-generated handles, and identity claims are verified against per-key CP configuration. crates/openab-cp/src/config.rs:135-181, server.rs:124-145, 274-289
6 🟢 Praise Admission now records in-flight state before forwarding and rolls capacity back on a send failure; parent serving-handle checks prevent chain borrowing. crates/openab-cp/src/router.rs:91-123, 168-209
Finding Details

🟡 F1: Close or explicitly retire a lease-expired connection

run_sweeper removes the registry entry and fails its in-flight work, but it does not signal the corresponding handle_connection task to close. That task retains its own tx, so rx.recv() cannot close the loop. Every subsequent frame reaches registry.get(handle)? and returns no reply, including cp/heartbeat; the client cannot send cp/register again because registration is first-frame-only.

Requested change: Retain a per-connection shutdown mechanism and close the socket on lease expiry (or otherwise transition it through an explicit re-registration state). Add an async integration test that lets a lease expire, verifies a close/reconnect signal, and verifies the reconnect can register again.

🟡 F2: Check ownership before removing the in-flight entry

complete and cancel remove the entry under one lock acquisition, release the lock, then validate the caller and reinsert on a mismatch. A concurrent genuine completion in that interval sees an unknown ID and is silently dropped; the unauthorized frame subsequently restores the entry, so the delegation only resolves at its deadline.

Requested change: Hold the in-flight lock while verifying the serving/initiating handle and remove only after the check succeeds. Add a deterministic concurrent regression test for a wrong-handle result/cancel racing the genuine result.

🟡 F3: Scope delegation IDs and error behavior to a namespace

The in-flight map is keyed only by the caller-supplied delegation_id. An authenticated runtime in one namespace can reserve a predictable or observed ID and cause a different namespace to receive DUPLICATE_DELEGATION. For cancellation, unknown IDs return INVALID_PARAMS while another caller's live IDs return POLICY_DENIED, which also reveals that the ID exists.

Requested change: Key in-flight state by (namespace, delegation_id) (and scope parent lookup the same way) and use an indistinguishable response for unknown and unauthorized cancellation. Add cross-namespace collision and oracle regression tests.

🟡 F4: Bound authenticated connection admission before registration

Authentication completes before upgrade, but a valid key can then open any number of sockets and keep each one in the registration loop indefinitely with pings. No pre-registration timeout, global cap, or per-identity quota limits tasks and sockets. Bounded post-registration queues do not cover this path.

Requested change: Apply a short first-application-frame deadline, enforce global and per-identity connection limits, and release each reservation on every close/error path. Cover idle pre-registration, ping-only, over-quota, and recovery cases with WebSocket integration tests.

Addressing External Reviewer Feedback

The only third-party top-level comment is the github-actions[bot] recent-committers report; it is informational and has no code-review concern. The PR author reported that round-1/2 findings were fixed before this head. This review inspected 33ed835f67396007d994878b89935a39638a95e6 directly and confirms the earlier handle-based ownership, atomic admission, parent-handle validation, loopback-default bind, and truncation-cap fixes. No external submitted code reviews were present.

Baseline Check
  • PR opened: 2026-08-11
  • Declared base: main
  • Merge base: 448b05fbcc17d1ebe52fdc8f78344018bd50b080
  • Reviewed head: 33ed835f67396007d994878b89935a39638a95e6
  • Diff: 32 files, +3325/-65
  • Net-new value: a standalone control-plane registry, policy engine, WebSocket server, protocol, and delegation router not present in the base.
Validation
  • git diff --check origin/main...HEAD passed in an isolated worktree at the reviewed SHA.
  • GitHub reported 40 completed checks, all successful, including check, build-builder, validation, and Docker smoke tests.
  • Local Rust checks could not run because this reviewer environment has neither cargo nor rustc installed.
What's Good (🟢)
  • Key-to-identity claim validation, CP-generated registration handles, and the non-loopback opt-in guard materially improve the earlier design.
  • The router has targeted unit coverage for immediate result delivery, send rollback, parent ownership, result truncation, deadlines, and disconnect handling.
  • The Dockerfile stub-layer updates consistently include the new workspace member, and the CI matrix completed successfully on this SHA.

5. Three Reasons We Might Not Need This PR

  1. No runtime client exists in this slice - The standalone binary has no end-to-end OAB caller yet, so its public contract may still change before it produces user-visible value.
  2. It adds a separate authenticated service - Operating, monitoring, and securing a new WebSocket control plane may not be justified if simpler orchestration meets the actual workload.
  3. The lifecycle boundary is not ready to rely on - The remaining lease and admission defects make the service risky to deploy before the follow-up integration work exists.

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.

1 participant