Skip to content

feat(cp): add primary delegation MCP tools and CLI - #1529

Merged
thepagent merged 3 commits into
feat/cp-runtime-client-mainfrom
feat/cp-control-plane-pr4
Sep 9, 2026
Merged

thepagent merged 3 commits into
feat/cp-runtime-client-mainfrom
feat/cp-control-plane-pr4

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Complete Agent Control Plane v1 with the primary-side initiating surface defined in ADR section 6:

  • primary-side cp/delegate, result tracking, cancel, and cp/list_agents
  • opaque local delegation handles that encapsulate one admission
  • owner-only Unix socket shared by MCP and CLI
  • four direct MCP tools: spawn_agent, check_delegation, list_agents, cancel_delegation
  • openab agent spawn|list|status|cancel
  • automatic MCP facade/session injection for primary runtimes, including adapter-less headless primaries

Stacked on recovery PR #1528 because the original #1471 merge landed into an obsolete stacked base rather than main.

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1532377620241449040

Review Contract

Goal

A primary OpenAB agent can discover peers and delegate work directly through four typed MCP tools, while operators/hooks can perform the same operations with openab agent. Both frontends use one owner-only local API; neither the agent nor CLI receives CP credentials or addresses.

Non-goals

Accepted Residual Risks

  • Opaque local-handle storage is bounded at 4096 tokens with deterministic oldest-first eviction. Terminal delegation history is independently bounded by both 4096 entries and 16 MiB, evicting the oldest terminal records only; live work is never evicted. A very old evicted handle returns unknown handle; live CP work is not cancelled by eviction.
  • Blocking spawn_agent waits until the requested deadline plus a five-second local response margin. Async mode is the escape hatch for long work.
  • The automatic primary facade uses loopback port 8848 when [mcp] is absent. A bind conflict is fatal and visible rather than silently removing the agent's delegation tools.

Acceptance Criteria

  • Primary requests share the single WebSocket writer/id space and correlate delegate acks/results by (delegation_id, admission).
  • Stale/duplicate terminal frames cannot complete the wrong admission; first terminal wins.
  • check_delegation is nonblocking and reports pending/running/terminal state.
  • Local handles are random 256-bit tokens and do not encode delegation ids or admission tokens.
  • UDS parent/socket permissions are 0700/0600; existing parents are validated without chmod, fresh default state directories are created safely, and stale non-socket/symlink/foreign-owner entries are refused.
  • JSON-lines framing preserves pipelined requests and strictly bounds each line at 1 MiB.
  • Primary state survives reconnects; terminal history is bounded by both count and retained bytes without evicting live work.
  • Delegate acknowledgements must return the requested delegation id; oversized outbound spawn frames fail locally without disconnecting the CP session.
  • The four direct MCP tools are session-token gated and invisible/unreachable to anonymous loopback clients.
  • MCP tools and CLI both traverse the same JSON-lines UDS API.
  • Primary without a chat adapter boots full runtime and receives automatic MCP tool injection.
  • Worker serving and all prior facade built-ins remain behavior-compatible.
  • Real integration tests drive primary UDS -> primary runtime -> real CP -> worker runtime -> scripted agent for list/spawn/await/cancel.
  • No CP key, prompt, or raw tool arguments enter child env or logs.
  • Full affected check/clippy/test matrix passes on macmini.

Follow-ups

Review Remediation

All actionable round-1, Sol/Fable, and final-regression findings are resolved in commits 027a7689b57bc1806bc0049b7e0060ad2ba2cf73 and a2bd011909989d1c491437a1e4fd6c05a5dafc54:

  • Existing UDS parents are validated and never chmod'd; the built-in default safely creates a fresh $HOME/.openab/run, stale entries are removed only when they are current-user Unix sockets, and all local lines are strictly bounded while preserving pipelined requests.
  • Primary delegation state now has process lifetime across reconnects, validates ack delegation ids, and bounds terminal history by both count and retained bytes without evicting live work. Terminal entries are ordered by completion, so out-of-order completion evicts the oldest completed result rather than the oldest admission.
  • Oversized serialized spawn frames return InvalidRequest locally without dropping the CP connection; a real-CP integration test immediately lists agents over the still-live connection.
  • MCP and CLI share one delegation-id generator and one admission-inclusive deadline + 5s operation bound; deadlines outside 1..=1800 are rejected consistently.
  • MCP label selectors require at least one property, redundant feature wiring is removed, the ADR describes headless primaries and the explicit Unix-only transport non-goal, and a real HTTP MCP test proves session-gated tools/list/tools/call behavior.

Architecture

agent MCP call                         operator CLI
      |                                     |
      v                                     v
session-token-gated facade        openab agent <verb>
      |                                     |
      +----------- JSON over UDS -----------+
                    0600 socket
                         |
                         v
             primary command/state machine
                         |
                  registered CP WebSocket
                         |
                         v
                    openab-cp
                         |
                         v
                 worker ACP session

The handle returned to local callers is random and opaque. The runtime alone maps it to the protocol (delegation_id, admission) pair, so delayed checks/cancels cannot target a same-id re-admission.

Verification

macmini exact-content matrix:

  • workspace --all-features: passed
  • openab-core and root --no-default-features: passed
  • production clippy (openab-core, openab-mcp, root): passed with -D warnings
  • openab-cp: 142 unit + 9 integration passed
  • openab-mcp: 245 passed
  • openab-core: 834 unit + 11 real-CP integration passed (one known unrelated macOS secret test intentionally skipped)
  • root binary: 40 passed
  • final independent Sol and Fable fix-verification audits: PASS / PASS

@chaodu-agent

This comment has been minimized.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Important

CHANGES REQUESTED ⚠️ — The fix commit resolves every prior finding with tests and adds no new defects; one remaining item is a documentation-accuracy/hardening nit on terminal-history eviction.

What This PR Does

Completes Agent Control Plane v1 by adding the primary-side initiating surface: a primary OpenAB runtime can discover peers and delegate work through four typed MCP tools (spawn_agent, check_delegation, list_agents, cancel_delegation) or the equivalent openab agent CLI. Both frontends share one owner-only local Unix-domain socket, and neither the agent nor the CLI ever receives control-plane credentials or addresses. This round reviews a follow-up fix commit that addresses the eight items raised on the previous head.

How It Works

  • A newline-delimited-JSON UDS API (local.rs) bridges out-of-process callers to the single in-process CP serve loop, handing back opaque 256-bit random handle tokens that encode nothing about the underlying (delegation_id, admission) pair.
  • A primary-side correlation state machine (primary.rs) tracks each initiated delegation, keyed on the composite (delegation_id, admission), with first-terminal-wins semantics and stale-admission dropping.
  • The MCP facade and the CLI now share one spawn_within_bound wrapper so a blocking spawn bounds the admission round-trip AND the terminal await under a single deadline_secs + 5 ceiling; both surfaces mint delegation ids from one shared generator.
  • The socket parent directory is now verified-not-mutated when it already exists (owner/type/mode checks) and only a runtime-owned run/ subdirectory is created; the default path moved to ~/.openab/run/agent.sock.

Resolution of Previous-Round Findings

All eight items raised on the prior head are addressed in fix(control-plane): address PR4 review findings, each with a regression test:

Prev Item Status
F1 Socket parent chmod'd unconditionally Fixed — existing parent is verified (dir type, owner uid, no group/world bits), never mutated; only the owned run/ subdir is created 0700; default path is now ~/.openab/run/agent.sock
F2 Spawn ack not validated against request id Fixed — ack.delegation_id != delegation_id is rejected as a protocol error and tracks nothing (primary.rs), with a mismatch regression test
F3 Blocking bound did not cover admission Fixed — shared spawn_within_bound wraps admission + await in one deadline_secs + 5 timeout, used by both CLI and MCP
F4 Schema allowed empty labels: {} Fixed — minProperties: 1 added to the labels sub-schema, with a schema test
F5 Duplicate delegation-id generators Fixed — cli_delegation_id removed; both surfaces call one new_delegation_id (no d-cli- fork), asserted by test
F6 Redundant openab-core/acp-mcp feature wiring Fixed — the acp feature no longer re-lists it; enabled once on the dependency with a comment
F7 ADR body stale vs headless-primary behavior Fixed — ADR gained a headless-primary section and the Unix-only note
F8 Test gaps (MCP HTTP, CLI arg validation, sync timeout) Fixed — added CLI deadline/XOR/label tests, a spawn_within_bound hung-admission timeout test (paused time), and a real-HTTP facade test

Findings (This Round)

# Severity Finding Location
1 🟡 Terminal-history eviction comment overstates ordering; out-of-order completion can evict the just-completed entry, affecting only post-terminal status queries crates/openab-core/src/control_plane/primary.rs (on_delegate_result / make_history_room)
2 🟢 All eight prior findings resolved with targeted regression tests; no new defects introduced —
3 🟢 Shared spawn_within_bound and single id generator cleanly unify the CLI and MCP enforcement paths —
Finding Details

🟡 F1: Terminal-history eviction comment and out-of-order edge

make_history_room evicts the first terminal entry found in tracked_order, which records admission (insertion) order, not completion order. The comment in on_delegate_result states "The just-terminal entry is the newest in insertion order, so older terminals are evicted before it." That holds only when delegations complete in admission order. If the oldest-admitted delegation completes last and its terminal payload trips the byte bound, the scan can select that same just-completed entry for eviction.

Impact is bounded: parked awaiters are notified before make_history_room runs, so a blocking Await always receives its result; only a later check/lifecycle query would miss an evicted entry, and reaching the byte bound requires a very large terminal payload. This is consistent with the PR's documented accepted residual risk that history is bounded and an aged-out handle returns unknown handle.

Suggested change: correct the comment to describe admission-order eviction, or make eviction skip the just-terminated key / evict by completion order, and add an out-of-order-completion regression test (the current byte-bound test inserts terminals in admission order only).

🟢 F2: Prior findings fully resolved

Each of the eight previous-round items is fixed at this head with a matching regression test, verified against the diff of the fix commit. The socket-directory hardening (asymmetric verify-vs-create with symlink/uid/mode guards) and the ack-id validation are particularly solid.

🟢 F3: Unified enforcement path

The closure-based spawn_within_bound and the single new_delegation_id remove the previous CLI/MCP divergence so both surfaces share one deadline wrapper and one id space; CLI- and MCP-initiated delegations are now indistinguishable on the wire.

Baseline Check
  • Reviewed head SHA: 027a7689b57bc1806bc0049b7e0060ad2ba2cf73
  • Declared base branch: feat/cp-runtime-client-main (stacked PR, not the default branch)
  • Merge-base: 9455b50405d19ab3858a9e7b005c914a7dad2f5a (equals base head; cleanly rebased)
  • Diff reviewed against the stacked base only: 15 files, +5347 / -106
  • Delta since previous round: one fix commit on top of 40b789e8, touching 10 files
  • Net-new value: the entire primary-side initiating surface. No duplication of base content.
Verification Notes
  • CI on this head: check, validate, conformance, and the first smoke-test lanes report success; the remaining build/smoke matrix was still in progress at review time with no failures. mergeable_state was unstable due to in-progress checks only.
  • Author-reported macmini matrix (per PR body): workspace --all-features, --no-default-features, production clippy -D warnings, and all crate test suites passed.
  • Independent local build/test verification could not be run in the review environment because it lacks a C toolchain (cc not found; no sudo to install build-essential). Compilation and test execution on this head should be confirmed by a runner with a working toolchain before merge, in addition to the green CI matrix.

5. Three Reasons We Might Not Need This PR

  1. Ship MCP tools first, defer the CLI — the two frontends are one API; one could argue for a smaller initial surface. Counter: operators/hooks need a non-agent entry point, and both already share one validation and deadline path.
  2. Fixed default loopback port for headless primaries — auto-binding 127.0.0.1:8848 when [mcp] is absent is implicit behavior; a required explicit [mcp] block would be more predictable, at the cost of ergonomics. Documented as an accepted residual risk (a bind conflict is fatal and visible).
  3. The opaque-handle registry adds complexity — returning the raw (delegation_id, admission) pair would be simpler. Counter: opacity is a deliberate security boundary preventing a caller from targeting a same-id re-admission.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Important

CHANGES REQUESTED ⚠️ — A well-architected, heavily-tested primary delegation surface; only non-blocking hardening/clarity nits remain, no correctness or auth defects.

What This PR Does

Adds the primary-side (initiating) half of the Agent Control Plane: four typed MCP tools (spawn_agent, check_delegation, list_agents, cancel_delegation) and an openab agent CLI, both driving one owner-only Unix-domain-socket local API. Neither the agent nor the CLI ever receives CP credentials or addresses. Reviewed as a stacked PR against its declared base feat/cp-runtime-client-main (PR #1528), not main.

How It Works

A JSON-lines UDS (0600 inside a 0700 dir) bridges out-of-process callers to the single in-process CP serve loop. Callers hold opaque random 256-bit handle tokens that encode nothing about the underlying (delegation_id, admission) pair; the runtime alone maps token to protocol identity, so a delayed check/cancel cannot target a re-admission. The primary state machine correlates acks/results by the full (delegation_id, admission) key with first-terminal-wins semantics, survives reconnects with process-lifetime state, and bounds terminal history by both entry count (4096) and retained bytes (16 MiB) while never evicting live work. The MCP facade gains a DirectToolProvider extension point whose session-bound providers are both invisible in tools/list and unreachable in tools/call without a broker-minted session token.

Findings

# Severity Finding Location
1 🟡 make_history_room count-bound eviction is O(n^2) near cap (linear position() scan per eviction on the hot ack/result path) crates/openab-core/src/control_plane/primary.rs:652
2 🟡 A just-completed terminal whose payload alone exceeds 16 MiB is self-evicted in the same call, so a later check/await returns UnknownDelegation, indistinguishable from "never existed" crates/openab-core/src/control_plane/primary.rs:557
3 🟡 Direct-tool published names are a function of session ctx; a session-filtered provider can shift which bare name a later provider claims. Harmless for the single-provider wiring but the doc calls the policy "deterministic" without the ctx qualifier crates/openab-mcp/src/mcp/facade.rs:175
4 🟡 args_sha256 uses to_vec(&args_map).unwrap_or_default(), so a serialization failure would hash an empty string and collapse distinct audit lines. Not currently reachable (a just-parsed map re-serializes) but the audit line is security-relevant crates/openab-mcp/src/mcp/facade.rs:414
5 🟢 UDS security model is airtight: symlink_metadata + uid + mode &0o077 checks, never chmods a dir it did not create, refuses symlink/non-socket/foreign-owner before unlink; bounded 1 MiB framing preserves pipelined requests crates/openab-core/src/control_plane/local.rs
6 🟢 Correlation, byte-accounting symmetry, and fail_all_live across reconnect cycles are correct and each backed by a dedicated regression test crates/openab-core/src/control_plane/primary.rs
7 🟢 Single session-gating chokepoint: list_tools and call_direct_tool both derive from one direct_tools(ctx) filter, so "invisible" and "unreachable" cannot diverge crates/openab-mcp/src/mcp/facade.rs:497
8 🟢 Whole-operation deadline_secs + 5 bound wraps admission AND await under one timeout, shared by CLI and MCP; proven by a paused-clock hung-admission test src/cp_tools.rs
Finding Details

🟡 F1: O(n^2) terminal-history eviction near capacity

make_history_room gates on tracked.len() (which counts live entries too) and, on each eviction, re-scans tracked_order with position() to find the oldest terminal. Near the 4096 cap, evicting k entries costs O(k*n) on the hot ack/result path. Correctness is fine and progress is guaranteed; this is a latency-hardening concern. Suggestion: maintain a terminal-only ordered index for O(1) amortized eviction.

🟡 F2: Self-evicted huge terminal is silently unqueryable

In on_delegate_result, pre-completion awaiters are answered first, then the entry is pushed to the back and make_history_room runs. If that single outcome alone exceeds MAX_TRACKED_BYTES, it is evicted immediately, so a subsequent check/begin_await returns UnknownDelegation for a delegation that genuinely completed. The "it alone is huge" case is documented, but the caller-visible ambiguity (completed-but-evicted vs never-existed) is worth surfacing distinctly, or exempting the newest terminal from same-call byte eviction.

🟡 F3: Direct-tool name resolution is ctx-relative

direct_tools() filters requires_session() providers when ctx.is_none(), then resolves collisions first-registrant-wins. Because the filtered set differs by ctx, the published (bare vs qualified) name of a later provider can differ anonymous-vs-authenticated. It is harmless with the current single control-plane provider, but the doc comment describes the collision policy as "deterministic" without noting it is deterministic relative to ctx. Add a clarifying comment to prevent a future multi-provider regression.

🟡 F4: Audit hash can collapse on serialization failure

Sha256::digest(serde_json::to_vec(&args_map).unwrap_or_default()) hashes an empty byte string if serialization ever fails, mapping distinct failed calls to the same audit hash. The input is a freshly-parsed Map so this is not reachable today, but since the audit line is security-relevant a debug_assert! or explicit error branch would harden it rather than rely on the implicit invariant. Same pattern exists on the source path.

🟢 F5-F8: What's done well

Strong UDS threat model (TOCTOU-safe unlink, no chmod of foreign dirs, bounded framing), airtight (delegation_id, admission) correlation with first-terminal-wins, symmetric byte accounting with no drift, reconnect-safe fail_all_live, a single session-gating chokepoint that cannot diverge between list and call, and one shared whole-operation timeout closing the prior CLI-vs-MCP divergence. Test coverage is exceptional and includes real HTTP-facade and real-CP integration paths.

Baseline Check
  • Base branch: feat/cp-runtime-client-main (stacked on PR feat(cp): land Agent Control Plane v1 on main #1528), not main. Reviewed origin/feat/cp-runtime-client-main...HEAD.
  • Merge-base: 9455b50405d19ab3858a9e7b005c914a7dad2f5a.
  • Net-new diff: ~5389 insertions / 106 deletions across 15 files.
  • Net-new value over the base: the primary-side initiating surface (local UDS API, four MCP tools, openab agent CLI, headless-primary boot, automatic facade/session injection). The runtime-client half lands in the base PR and is out of scope here.
Accepted Residual Risk (not counted as a finding)

The handle registry bounds at 4096 cumulative minted tokens with oldest-first eviction (not live-count). After more than 4096 sequential spawns a still-live delegation's token can age out, after which await/check/cancel return unknown handle while the CP work keeps running. This is explicitly listed as an Accepted Residual Risk in the PR contract, so it is acknowledged rather than blocking.

5. Three Reasons We Might Not Need This PR

  1. The runtime already works headless as a worker — if the near-term deployment only needs workers to serve delegations, the entire initiating surface (tools + CLI + UDS) could be deferred until a concrete primary use-case exists.
  2. openab agent CLI and four MCP tools overlap — two frontends over one API doubles the surface to document and test; a single frontend first would ship sooner and prove demand before the second is added.
  3. Unix-only by design — the local API, CLI, and MCP transport are Unix-only with fail-loud non-Unix stubs; if any target deployment is non-Unix, this surface delivers nothing there and the abstraction cost is pure overhead.

Each is answerable (primaries are the point of the ADR; the CLI serves operators/hooks while tools serve agents; the deployment target is Unix containers), which sharpens rather than weakens the case for the PR.

@thepagent thepagent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final regression review — approved

Reviewed final head a2bd011909989d1c491437a1e4fd6c05a5dafc54 under the frozen review contract and three-stage stopping rule.

  • The round-2 terminal-order finding is resolved with completion-ordered eviction and an out-of-order regression test.
  • macmini validation passed: 97 focused control-plane tests, strict openab-core clippy, and the complete 834-test core suite plus 11 real-CP integrations.
  • Independent Sol and Fable final-regression audits both returned PASS.

The round-3 automation explicitly reports no correctness or auth defects and labels its remaining items non-blocking hardening/clarity nits. They are classified as post-freeze SCOPE EXPANSION, not regressions or Late Blockers:

  1. O(n²) worst-case eviction scan: bounded to the 4096-entry history and correctness-preserving; performance hardening only.
  2. A >16 MiB single terminal self-eviction: unreachable through the current sub-1-MiB transport/result caps and already covered by bounded-history residual-risk semantics.
  3. Context-relative collision-policy wording: harmless with the single shipped session-gated provider; documentation clarification only.
  4. Audit-hash serialization fallback: the reviewer confirms the failure path is not reachable for a freshly parsed JSON map; no present security defect.

No additional review round is authorized. The review contract is satisfied and this head is approved.

@thepagent
thepagent merged commit 3742e03 into feat/cp-runtime-client-main Sep 9, 2026
45 checks passed
chaodu-agent added a commit that referenced this pull request Sep 10, 2026
- 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.
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