feat(cp): OAB runtime CP client — [control_plane] config, worker serving, headless mode (PR 3/4)

[chaodu-agent]

OAB runtime CP client — [control_plane] config, worker serving, headless mode (PR 3/4)

Stacked on #1470 (feat/cp-observer). PR 1/4 (#1469) is the CP hub; PR 2/4 (#1470) the observer/lobby protocol; this slice connects the OAB runtime itself to the CP. PR 4/4 (MCP facade tools: spawn_agent / check_delegation / list_agents / cancel_delegation + primary-side initiation) follows.

Implements ADR §3 (Registration, OAB side), §3-Headless, and the worker half of §4 — docs/adr/agent-control-plane.md.

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

Review Contract

Goal

An OAB runtime can join the control plane: a new optional [control_plane] config section makes the runtime dial the CP over WebSocket, register with its key-bound identity, heartbeat, and — when type = "worker" — serve incoming cp/delegate requests by running the prompt through its local ACP session pool and replying cp/delegate_result within the deadline. type = "worker" also unlocks headless mode: [agent] + [control_plane] with no platform adapters is a valid boot.

Non-goals

• Primary-side initiation and the agent-facing MCP facade tools (spawn_agent, …) — PR 4/4 (ADR §6).
• The Unix domain socket / openab agent CLI — PR 4/4.
• Durable delegation state across runtime restarts: in-flight work dies with the process; the CP synthesizes target_disconnected (ADR §4 v1 contract).
• Kiro-style session primitives (inbox, interrupt, broadcast) — explicitly deferred from v1 in the ADR.

Accepted Residual Risks

• A worker that loses its CP connection abandons in-flight delegations without notifying anyone locally; the CP's target_disconnected synthesis is the single source of truth. Deliberate: per-connection ownership is the ADR's replica-safety rule.
• The prompt seam refactor (stream_prompt_blocks → PromptExecution) touches the shared platform delivery path; safety is argued by mechanical mapping + every pre-existing adapter/dispatch test passing unmodified, not by new platform-level tests.
• Windows cross-check is UNVERIFIED (build host lacks the x86_64-pc-windows-gnu target); no platform-specific code was added and the crate compiles with --no-default-features.

Acceptance Criteria

• Absent [control_plane] → zero behavior change (config parse test: None; no task spawned)
• [agent] + [control_plane type="worker"], no adapters → boots headless; type="primary" alone without adapters still bails; [mcp]-only unchanged; [mcp] + [control_plane] runs facade AND client (run-mode test matrix)
• Client registers against the REAL openab-cp server in-process (integration test, not a mock): register/ack, heartbeat keeps lease, delegate→result roundtrip, cancel mid-flight, local deadline → Timeout, lease-expiry close → reconnect + re-register succeeds
• Reconnect: exponential backoff 1/2/4/8/16/30s, shutdown-aware; backoff resets only after a ≥60s session (no reconnect storm against an accept-then-close CP); one instance_id per process across reconnects
• Executor: local concurrency cap enforced (over-cap / duplicate delegation_id → immediate Failed, never executed, CP capacity released); outcome mapping Completed/Failed/Timeout/Cancelled pinned by unit tests; cancel/deadline cleanup bounded (5s) and the pool session is discarded — no slot leaks
• Completed result bodies are capped (512 KiB, UTF-8-safe marker) below the CP's max_frame_bytes transport limit — an oversized agent result can no longer drop the connection and kill co-inflight delegations
• openab-cp gains a default server feature; openab-core depends on it default-features = false — cargo tree shows no axum edge into the runtime; the openab-cp binary still builds with default features; zero Dockerfile changes needed
• CP auth_key never enters the agent child env (untouched env_clear discipline) and never appears in logs
• Full matrix on the build host: openab-cp 94 passed; openab-core 712 passed (+35 new; 1 pre-existing macOS-only failure unrelated); root bin 25 passed; integration 7 passed; clippy and rustfmt at exact pre-change baselines

Follow-ups

• PR 4/4: MCP facade tools + Unix socket + primary-side initiation (ADR §6).
• ADR §3 example URL said wss://…/acp; the server mounts /cp — example corrected in this PR; revisit if an alias is preferred instead.
• Consider surfacing delegation-serving activity in the runtime's own logs/metrics once the fleet runs this (observability today is CP-side via feat(cp): observer/lobby — 3-phase design + Phase 1 protocol scaffold (PR 2/4) #1470's lobby events).

At a Glance

[control_plane]                     openab (runtime)                    openab-cp (hub)
url / auth_key / namespace /   ┌──────────────────────────┐       ┌──────────────────────┐
name / type / labels /         │ control_plane::client    │──wss─►│ register → ack       │
max_delegated_sessions    ───► │  dial → register → serve │◄──────│ heartbeat lease      │
                               │  backoff 1..30s, 1 uuid  │       │                      │
absent section = no client,    │ control_plane::executor  │◄──────│ cp/delegate forward  │
existing deployments untouched │  cap check → session per │──────►│ cp/delegate_result   │
                               │  delegation → ACP pool   │       │ (→ lobby cp/event)   │
[agent]+[control_plane worker] │  cancel/deadline bounded │       └──────────────────────┘
= valid headless boot          └──────────────────────────┘

Prior Art & Industry Research

• The reconnect/backoff loop mirrors the existing standalone gateway WS client (crates/openab-core/src/gateway.rs).
• The headless run-mode gate extends the facade-only precedent introduced with [mcp] (feat(mcp): facade-only run mode — adapter-less [mcp] config is valid #1453).
• The delegation executor reuses the prompt path cron jobs already exercise (AdapterRouter + SessionPool), rather than the ACP-over-WS acp_client synthesis path — one seam, no event fabrication.

Proposed Solution

1. Feature split (crates/openab-cp): default server feature gates axum/registry/router/policy/events/server + the binary; proto (wire types) stays unconditional. The runtime consumes openab-cp with default-features = false — wire types only, no server deps, no new Dockerfile stubs.
2. Config (crates/openab-core/src/config.rs): ControlPlaneConfig with deny_unknown_fields, CpAgentType (primary|worker — observer unrepresentable by construction), ${ENV} expansion free via the existing pass, validation for empty fields and max_delegated_sessions > 0.
3. Client (crates/openab-core/src/control_plane/client.rs): process-lifetime instance_id; Bearer auth at upgrade; cp/register first frame; single select! loop over inbound / heartbeat / completion channel / shutdown; on disconnect cancels local work, drains (5s) then aborts, backs off shutdown-aware.
4. Executor (crates/openab-core/src/control_plane/executor.rs): admission (cap, duplicate) decided before execution; fresh ACP session per delegation keyed control-plane:<sha256(instance_id + delegation_id)>; budget min(deadline − now, prompt_hard_timeout); PromptRunner trait seam so integration tests inject a fake runner while production uses the pool.
5. Prompt seam (crates/openab-core/src/adapter.rs): stream_prompt_blocks returns PromptExecution { final_text, terminal_error, silent_failure }; platform callers map back mechanically — no observable change, pre-existing tests untouched.
6. Lifecycle (src/main.rs): client spawned after router construction when the section is present; stopped (or aborted after 10s) before pool.shutdown(); headless run-mode matrix extended.

Alternatives Considered

• New openab-cp-client crate: rejected — every one of the 19 Dockerfiles stubs workspace crates and would need edits; a module inside openab-core is Dockerfile-neutral and the client is small.
• Reusing the ACP-over-WS acp_client event-synthesis path for delegated prompts: rejected — it fabricates gateway events to re-enter the dispatcher; the cron precedent calls the router/pool seam directly with less indirection and no fake sender identity.
• A cargo feature for the client: rejected — the optional config section is already the opt-in; a feature would double the build matrix for no isolation gain (the client adds no heavy deps).
• CP-side-only concurrency accounting: rejected — the runtime also enforces its cap locally so a CP bug cannot flood a worker; over-cap arrivals get a terminal Failed so CP capacity releases.

Validation

• cargo test -p openab-cp — 94 passed (server unaffected by the feature split; binary still builds)
• cargo test -p openab-core — 713 passed, +36 new (config/executor/client incl. the transport-cap regression); 1 failure is the known pre-existing macOS-only secrets::tests::resolve_exec_nonzero_exit, present at baseline
• cargo test -p openab-core --test cp_client — 7 integration tests against the real in-process CP server: registration, heartbeat/lease, roundtrip, cancel, timeout, lease-expiry reconnect, shutdown deregistration
• cargo test (root bin) — 25 passed incl. the headless run-mode matrix
• cargo clippy --workspace --all-targets — warning count identical to baseline (12); cargo fmt --check diff count identical to baseline (217, pre-existing drift untouched)
• cargo check --workspace --no-default-features — passes
• cargo check --target x86_64-pc-windows-gnu — UNVERIFIED (target not installed on the build host); no platform-specific code added
• Independent audit round: four blocking findings (facade-only foreclosing the client, backoff reset storm, unbounded cancel before discard, non-abortable shutdown) — all fixed and re-verified

[chaodu-agent]

Self-Review & CI Verification Summary (Head 13271cb3)

Follow-up update after clippy fix commit 13271cb3:

• CI Status: All jobs passed green on Run 33816869090:
  • cargo check, cargo clippy, cargo clippy (unified): passed with -D warnings.
  • cargo test, all ACP test suites (gateway, mcp core, mcp pool, root): passed.
  • cargo build (unified): completed successfully.
• Contract & Boundary Audit:
  • Transport safety: json_escaped_len + escape-aware cap_text verified safe against worst-case 6x control character and 2x quote/slash expansions under the 1 MiB frame ceiling; outbound transport-layer guard active.
  • Teardown bound: cancel_and_discard() strictly constrained within the shared 5s TEARDOWN_BOUND, eliminating overrun risks during connection drain.
  • Discard latency: Clean turn session discard deferred to background (spawn_discard), keeping initiator response latency minimal.
  • Lifecycle & Headless modes: Clean shutdown ordering verified (CP client unwinds delegations before pool teardown); headless worker boots smoothly without chat platform requirements.

PR is clean, fully converged with base feat/cp-observer, and ready for review.

[chaodu-agent]

Independent Deep Systems Review (Model: Gemini Pro Extended Thinking)

An adversarial systems-level code review of branch feat/cp-runtime-client (head 13271cb3) was performed focusing on concurrency, bounds enforcement, protocol fidelity, and teardown invariants.

---

1. High-Confidence Invariant Verifications (Passed)

• JSON Escape & Frame Bound (F46):
  • json_escaped_len() and cap_text() correctly account for control character (up to 6x) and quote/slash (2x) expansions.
  • Multi-byte UTF-8 sequences are protected: c if (c as u32) < 0x20 strictly filters ASCII control characters; multi-byte continuation bytes ($\ge 0x80$) pass through intact.
  • Slicing via char_indices() guarantees valid UTF-8 boundaries.
  • Outbound transport guard in client.rs:send() prevents oversized frames (> 1 MiB) from ever hitting the wire.
• SlotGuard RAII:
  • The drop guard ensures release() executes even if the serving task is cancelled or aborted mid-await during drain-window disconnects, preventing capacity leaks.
• Race-Free Background Discard (F53):
  • Deferring session discard to spawn_discard() on clean turn completion is proven safe: session keys mix in (instance_id, delegation_id, admission). Immediate re-admission of the same delegation_id receives a new AdmissionToken, guaranteeing an isolated session key.
• Graceful Shutdown Ordering:
  • main.rs shuts down the CP client (with a 10s deadline covering the 5s DRAIN_TIMEOUT) strictly before the session pool teardown, preventing dangling writes to dead sessions.

---

2. Architectural Observations & Follow-Up Recommendations

1. Synchronous Disk I/O during Session Discard (SessionPool):

   • discard_session() in crates/openab-core/src/acp/pool.rs acquires the pool write lock and calls self.save_mapping() and self.save_meta(), which perform synchronous file writes (std::fs::write / rename).
   • Analysis: This mirrors the existing legacy pattern in reset_session(). While practically instantaneous on local SSDs, in Tokio async tasks synchronous disk I/O cannot be preempted by tokio::time::timeout.
   • Recommendation: Recommend addressing this in a future PR as part of a general SessionPool persistence refactoring (e.g. offloading to tokio::task::spawn_blocking or tokio::fs), keeping PR feat(cp): OAB runtime CP client — [control_plane] config, worker serving, headless mode (PR 3/4) #1471 focused and free of scope creep.

2. Headless Mode Semantics ([mcp] + primary):

   • In src/main.rs:headless_run_mode(), type = "primary" alone fails fast (correct), but when combined with [mcp], it returns HeadlessMode::ControlPlaneWorker to run both the MCP facade and CP client registration in preparation for initiator capabilities.
   • Recommendation: Consider renaming the enum variant in a future cleanup to FullBoot or HeadlessClient to distinguish it from pure worker mode.

3. Uniform Transport Guard (client.rs:register):

   • client.rs:register() calls sink.send(Message::Text(...)) directly rather than using the send() helper.
   • Analysis: Safe currently because RegisterParams is small (~hundreds of bytes), but standardizing on send(&mut sink, &frame) in the future maintains defense-in-depth across all outbound frame types.

---

Verdict

LGTM for PR #1471 scope. The Round 8 fixes (F46, F51-F55) are verified correct, CI is 100% green at 13271cb3, and identified observations are tracked for future subsystem improvements.

[chaodu-obk]

Important

CHANGES REQUESTED ⚠️ -- Security, protocol-validation, failure-containment, and headless-contract changes are still required.

Consolidated review: #1471 (comment)

GitHub event: COMMENT -- self-review delivery only; this is not an approval.

[chaodu-obk]

🟡 F1 - Require an explicit secure transport policy

auth_key is the complete bearer credential, but this endpoint accepts non-loopback ws:// without rejection or an explicit insecure opt-in. That sends the credential in cleartext over the network.

Requested change: reject non-loopback ws:// by default, or require an explicit allow_insecure_transport opt-in with a startup warning and documented local-development exception.

[chaodu-obk]

🟡 F2 - Bound the complete dial

connect_async_with_config covers DNS, TCP, TLS, and the WebSocket handshake but has no explicit deadline. A blackholed endpoint can therefore stall this reconnect iteration until the OS timeout, far beyond the documented retry schedule.

Requested change: wrap the dial in a shutdown-aware timeout and add a paused-time regression test.

[chaodu-obk]

🟡 F3 - Jitter fleet reconnects

Every replica follows the same deterministic 1/2/4/8/16/30-second sequence. After a CP restart, the fleet can reconnect in lockstep and create a thundering herd.

Requested change: apply full or equal jitter while preserving the ceiling, and test the allowed delay range.

[chaodu-obk]

🟡 F4 - Recover this bookkeeping lock from poisoning

All inflight operations use expect("inflight mutex"). If any panic poisons the lock, SlotGuard::drop can panic again while unwinding through release, causing a process abort. The map stores only tokens and Arc<Notify>, so recovery is safe.

Requested change: use an unpoisoned mutex or recover with PoisonError::into_inner() consistently, including the drop path, and add a poison-recovery test.

[chaodu-obk]

🟡 F5 - Fail closed on the registered role

This branch accepts cp/delegate for both Worker and Primary. That makes hub routing the only role boundary even though the runtime already has the configured role.

Requested change: reject delegation requests unless self.cfg.agent_type == CpAgentType::Worker, and test the primary path.

[chaodu-obk]

🟡 F7 - Redact the credential at its owning type

Deriving Debug includes auth_key verbatim. Header sensitivity protects request formatting only; one future config diagnostic can expose the bearer credential.

Requested change: provide a manual redacted Debug implementation or use a secret wrapper whose Debug is always masked.

[chaodu-obk]

🟡 F8 - Name and document the actual full-boot mode

This ControlPlaneWorker result also represents type = "primary" plus [mcp]. The config table still omits that combination and leaves the primary/no-adapter error row unqualified.

Requested change: rename the variant to behavior-oriented FullBoot/ControlPlaneClient, add the primary + [mcp] row, and qualify the error row as primary without [mcp].

[chaodu-obk]

🟡 F9 - Name the bidirectional frame contract accurately

The complete outbound frame is now checked against MAX_INBOUND_FRAME_BYTES, while the constant comment says outbound is already capped elsewhere and this closes only the inbound direction. Payload-field caps and complete-frame caps are distinct.

Requested change: rename this to MAX_FRAME_BYTES (or split the limits) and document its use for both WebSocket receive configuration and serialized sends.

[chaodu-obk]

🟡 F10 - Supervise the control-plane task during normal operation

This handle is only awaited during shutdown. If run returns early or panics, the process can remain healthy-looking while no longer registering or serving; a worker-only process has no other duty that exposes the failure.

Requested change: supervise early task exit and fail readiness or terminate worker-only mode so the deployment supervisor can recover it.

[chaodu-obk]

🟡 F11 - Keep startup remediation complete

has_unified_platform recognizes LINE, LINE WORKS, and Teams, but this error omits [line], [lineworks], and [teams] from the valid adapter list.

Requested change: include those adapters or replace the brittle enumeration with a reference to the platform configuration documentation.

[github-actions]

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

[chaodu-agent]

Resolved the round-9 findings at ffca86904b8d77add12e2df20c416ba138296669.

The branch now also includes the latest stacked base (feat/cp-observer at 9ba4e187) through merge commit 54779263; this was a fast-forward push, not a force push.

Round-9 disposition

Finding	Resolution
F1 remote cleartext bearer transport	Non-loopback ws:// now fails closed unless allow_insecure_transport = true; loopback IPv4/IPv6 remains available for local development, and explicit cleartext opt-in logs a warning
F2 unbounded connect	DNS/TCP/TLS/WebSocket establishment has a 10-second deadline, still raced against shutdown
F3 reconnect herd	Reconnect uses equal jitter over the existing 1/2/4/8/16/30-second base schedule; reset still requires a 60-second stable session
F4 mutex poison / panic-in-Drop	Every inflight-map access recovers poisoned state, including SlotGuard::drop; regression poisons the lock and verifies active/admit/drop recovery
F5 primary serving worker requests	A primary runtime rejects cp/delegate with POLICY_DENIED before parsing/execution
F6 ack version unchecked	Registration now requires RegisterAck.protocol_version == PROTOCOL_VERSION
F7 credential Debug exposure	ControlPlaneConfig has a manual Debug implementation that always renders auth_key as [REDACTED]
F8 headless contract naming/docs	HeadlessMode::FullRuntime replaces the role-specific name; docs and tests cover worker, primary, MCP-only, MCP+worker, and MCP+primary combinations
F9 frame-limit naming	The shared complete-frame ceiling is now MAX_FRAME_BYTES, documented as bidirectional
F10 unsupervised CP task	The client JoinHandle is supervised during normal runtime; clean exit, panic, or supervisor loss becomes a fatal runtime error, while shutdown remains bounded and abortable
F11 incomplete startup remediation	The error now names every supported first-class adapter, including LINE, LINE WORKS, Feishu, and Teams

Also removed initiator-supplied cancel-reason text from info logs (only its byte count is recorded).

Validation

Validated on macmini against the merged stack and exact resulting content:

• Changed library/test files: rustfmt-clean
• Root main.rs: only the 14 pre-existing rustfmt baseline hunks remain; no unrelated formatting churn committed
• cargo check --workspace --all-features: passed
• cargo check -p openab-core --no-default-features: passed
• cargo clippy -p openab-core --all-features --lib -- -D warnings: passed
• cargo clippy -p openab -- -D warnings: passed
• cargo test -p openab-cp: 141 unit + 9 integration passed
• cargo test -p openab-core --all-features: 772 unit + 7 integration passed (known macOS-only secret test skipped)
• cargo test -p openab-core --test cp_client: 7/7 real-server integration tests passed
• cargo test -p openab: 26/26 passed
• Independent final audit: APPROVE; all F1-F11 fixes verified, no reverse regressions or unrelated churn

New-head review follow-up (fbe8c7e)

Resolved the automated new-head findings from #1471 (comment):

• F1: silent-failure classification now uses the raw answer body before tool summaries/reset notices/presentation text; tool-only zero-output turns report Failed, not Completed.
• F2: cp/delegate_result now fits the complete serialized JSON-RPC envelope, not only the result field. CP and runtime share MIN_RUNTIME_FRAME_BYTES (1 MiB), and CP validation rejects a smaller server ceiling.
• F3: executor teardown is 4 seconds, strictly below the client drain/abort window of 5 seconds.
• F4: an unexpected CP-task exit records a fatal error, runs the shared ordered cleanup (pool.shutdown() and hooks included), then returns the error.
• F5: only connect/register race the outer shutdown signal. Once registered, the serve loop exclusively owns cancel, drain, abort, and clean close.

Additional regressions cover raw silent failure with tool-rendered text, a 600 KiB delegation id plus 512 KiB result fitting the full frame, the CP shared-minimum validation, and shutdown while a delegated turn is in flight.

Updated validation at exact commit fbe8c7ee11c67f68fa3850ebb158c3e40f6817b3:

• Workspace all-features and core no-default-features checks: passed
• Production core and root clippy with -D warnings: passed
• openab-cp: 142 unit + 9 integration passed
• openab-core: 774 unit + 8 integration passed (known macOS-only secret test skipped)
• Explicit real-server cp_client: 8/8 passed
• Root binary: 26/26 passed
• Independent second-round audit: APPROVE

Final documentation follow-up (119c6b3)

Resolved the remaining review items from #1471 (comment):

• Documented the agent-facing openab.delegation.v1 schema in docs/control-plane.md, including all fields, CP-stamped trust semantics, and a complete <sender_context> example.
• Explicitly documented that <sender_context> is a multi-schema structural envelope: chat arrivals use openab.sender.v1, delegated prompts use openab.delegation.v1, and consumers must dispatch on schema.
• Updated the control-plane status block to PR 3/4.
• Corrected the teardown regression assertion message to interpolate the actual 4-second TEARDOWN_BOUND rather than saying 5 seconds.

The focused teardown test and rustfmt check pass at exact commit 119c6b34258a8bf23ea67fef80d9a424de3e3c46.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ — One correctness bug in delegation failure classification (a silent agent failure is reported as Completed), plus four hardening items around transport-frame sizing, teardown timing, and shutdown ordering.

What This PR Does

Connects an OAB runtime to the control plane (ADR §3/§4, worker half): a new optional [control_plane] config section makes the runtime dial the CP over WebSocket, register with a key-bound identity, heartbeat, and — when type = "worker" — serve incoming cp/delegate requests by running the prompt through the local ACP session pool and replying cp/delegate_result within the deadline. type = "worker" also unlocks a headless boot ([agent] + [control_plane], no platform adapters).

How It Works

• Feature split: openab-cp gains a default server feature; openab-core depends on it default-features = false (wire types only, no axum edge into the runtime, no Dockerfile churn).
• Client (control_plane/client.rs): process-lifetime instance_id, Bearer auth in the Authorization header, cp/register first frame, a single select! serve loop over inbound / heartbeat / completion channel / shutdown, and a shutdown-aware reconnect loop with jittered 1/2/4/8/16/30s backoff that only resets after a >=60s session.
• Executor (control_plane/executor.rs): atomic admission (cap + duplicate) decided before execution, a fresh ACP session per delegation keyed by sha256(instance_id + delegation_id + admission), a min(deadline - now, prompt_hard_timeout) budget, and a result-body cap below the CP transport limit.
• Prompt seam (adapter.rs): stream_prompt_blocks now returns PromptExecution { final_text, terminal_error, silent_failure }; chat callers map back with .map(|_| ()).

Findings

#	Severity	Finding	Location
1	🔴	Silent agent failure is classified as Completed when any tool ran	adapter.rs:1154, executor.rs:328
2	🟡	Result cap is a fixed 512 KiB, not related to the negotiated frame limit and ignores envelope bytes	executor.rs:412, client.rs:61
3	🟡	Per-task teardown bound equals the drain window (both 5s), not under it	executor.rs (TEARDOWN_BOUND), client.rs:51
4	🟡	CP-task-failure path returns early, bypassing ordered cleanup and pool.shutdown()	src/main.rs:1857-1894
5	🟡	Outer run select arms a redundant shutdown branch that can win over the inner drain/abort	client.rs:139
6	🟢	Strong admission control, ASCII/JSON-escape-aware caps, honest feature split, real in-process CP integration tests	—

Finding Details

🔴 F1: Silent failure misclassified as Completed when tools ran

In adapter.rs, the silent-failure signal handed to the executor is derived from the rendered body, not the raw turn:

let silent_failure = final_content.is_empty() && turn_result.is_silent_failure();

final_content is produced by display_for(platform_is_acp, ...). The control-plane platform label is "control-plane", so platform_is_acp is false and display_for falls through to compose_display, which prepends a tool summary (for example "✅ 3 tool(s)\n") whenever tools ran and tool_display != None. ToolDisplay::default() is Full (config.rs:1925).

Consequence: a delegated turn that runs one or more tools and then silently fails (stopReason = end_turn, outputTokens = 0, no answer text) has turn_result.is_silent_failure() == true but a non-empty final_content, so silent_failure becomes false. executor.rs:328 then reports Completed with the tool-summary string as the delegation result, instead of Failed. The initiator gets a bogus success carrying no answer. This defeats the acceptance-criterion the PR explicitly claims ("empty turn -> Failed").

Requested change: derive the silent-failure classification from the raw agent output / TurnResult (answer text + is_silent_failure()), independent of the platform-rendered final_content. Add a seam-level test where a delegated turn runs a tool and reports end_turn/0 output tokens, and assert the executor maps it to Failed.

🟡 F2: Result cap is fixed 512 KiB and decoupled from the negotiated frame limit

MAX_RESULT_BYTES = 512 * 1024 caps the JSON-escaped result string, and the doc-comment reasons about the CP default max_frame_bytes of 1 MiB. Two gaps: (a) the cap covers only the result field, not the full serialized JSON-RPC envelope (delegation_id, method, id, wrapper), so a large delegation_id plus a maxed result can still exceed the transport frame; (b) CP max_frame_bytes is operator-configurable and could be set below 512 KiB, in which case a capped result still drops the connection and kills co-inflight delegations — the exact failure this cap exists to prevent.

Requested change: cap against the full serialized frame with headroom for the envelope, and either negotiate the limit from the register ack or validate/enforce a documented shared minimum between CP max_frame_bytes and this cap.

🟡 F3: Teardown bound is not under the drain window

TEARDOWN_BOUND = 5s (executor.rs) equals DRAIN_TIMEOUT = 5s (client.rs:51), yet the executor comment states teardown "stays under the client's disconnect drain window." Because they are equal, a task performing a full-length cancel+discard on disconnect can be abort()ed at the same instant its teardown would finish, leaving discard_session incomplete (the pool idle/hung sweep is the only backstop).

Requested change: make TEARDOWN_BOUND strictly less than DRAIN_TIMEOUT (or run session discard on a path that survives the drain-abort), and fix the comment to match reality.

🟡 F4: CP-task-failure return bypasses ordered shutdown and pool.shutdown()

main.rs races the platform run against wait_for_control_plane_exit(&mut cp_task); on CP-task termination it does return Err(error) directly (1857, 1883, 1894). That early return skips the entire cleanup block below it — shutdown_tx.send(true), per-platform teardown, the deliberate "stop CP before the pool" step, and pool.shutdown(). So the one failure mode where the CP has already died is also the one that skips the ordered ACP session teardown the rest of the block guarantees.

Requested change: route the CP-exit error through the shared cleanup path (record the exit result, run cleanup, then return it), and add a regression test proving pool.shutdown() is reached on the CP-failure path.

🟡 F5: Redundant outer shutdown arm can win over the inner drain

run wraps connect_and_serve in a select! whose second arm is shutdown_signal.changed() => return. That arm exists to unblock a hung dial, but it stays armed while serve is running. When shutdown flips, the inner serve select! and this outer arm are ready simultaneously; if select! picks the outer arm, run returns and drops the serve future — dropping its serving Vec<JoinHandle> without cancel_all() + drain + abort, detaching in-flight delegation tasks (and their ACP children) and skipping the clean Close frame.

Requested change: only arm the outer shutdown branch around the dial/register phase, or make the inner serve the sole owner of shutdown so the drain/abort path always runs.

🟢 F6: What is done well

• Admission (cap + duplicate) is decided atomically under one lock before any runner/session use, and SlotGuard releases capacity on every exit including abort-at-await; over-cap arrivals return a terminal Failed so CP capacity is released.
• The result/error caps are UTF-8-safe and JSON-escape-aware (json_escaped_len), and centralized so a new call site cannot forget the bound.
• Honest feature split: openab-core takes openab-cp with default-features = false; no axum edge leaks into the runtime and the CP binary still builds.
• Integration tests exercise the real in-process CP server (register/ack, heartbeat lease, roundtrip, cancel, timeout, lease-expiry reconnect, shutdown deregistration), not a mock.
• The auth_key rides a set_sensitive(true) Authorization header, never the URL, and no value logging was found.

Baseline Check

• This is a stacked PR; base branch is feat/cp-observer (PR feat(cp): observer/lobby — 3-phase design + Phase 1 protocol scaffold (PR 2/4) #1470), not main.
• Merge-base: 9ba4e187. Diff reviewed: feat/cp-observer...ffca869 — 19 files, ~3594 insertions.
• Net-new value over the base: the runtime-side CP client, delegation executor, [control_plane] config + headless boot, the prompt-execution seam, and the openab-cp server feature split. Findings about code already present on feat/cp-observer are out of scope.

Three Reasons We Might Not Need This PR

1. Failure signal already lives CP-side — target_disconnected and lobby events (feat(cp): observer/lobby — 3-phase design + Phase 1 protocol scaffold (PR 2/4) #1470) already surface delegation health; if F1 is not fixed, the runtime's own Completed/Failed distinction is actively misleading rather than additive.
2. Local cap duplicates CP accounting — the CP already tracks capacity; the local cap is defense against a CP bug, which is speculative until the fleet actually runs multi-worker delegation.
3. Headless mode has no consumer yet — primary-side initiation and the MCP facade land in PR 4/4, so a worker can register and serve but nothing in-tree drives delegations end to end until then.

None of these blocks the slice — they are decision context. The strongest case for merging as-is is the real in-process integration coverage; the strongest case against is F1, which should be fixed before this ships.

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ — Every round-9-followup finding (the one silent-failure correctness bug plus four hardening items) is verified fixed at this head with regression tests, and the long carried backlog is now closed. Two important items remain: the openab.delegation.v1 agent-facing context schema is still undocumented in docs/, and one teardown test carries a misleading assertion message.

What This PR Does

Connects the OAB runtime to the Agent Control Plane (ADR sect.3/sect.4, worker half): an optional [control_plane] section makes the runtime dial the CP over WebSocket, register with a key-bound identity, heartbeat, and -- when type = "worker" -- serve cp/delegate requests through the local ACP session pool, replying cp/delegate_result within the deadline. type = "worker" also unlocks a headless boot ([agent] + [control_plane], no chat adapters).

How It Works

• Feature split: openab-cp gains a default server feature; openab-core depends on it default-features = false (wire types only, no axum edge into the runtime, no Dockerfile churn).
• Client (control_plane/client.rs): process-lifetime instance_id, Bearer auth in the Authorization header, cp/register first frame, a single select! serve loop, jittered 1/2/4/8/16/30s reconnect backoff that resets only after a >=60s session, a 10s connect timeout, and a bidirectional MAX_FRAME_BYTES ceiling.
• Executor (control_plane/executor.rs): atomic admission (cap + duplicate) before execution, a fresh ACP session per delegation keyed by sha256(instance_id + delegation_id + admission), a min(deadline - now, prompt_hard_timeout) budget, RAII slot accounting, and a shared-deadline bounded teardown.
• Prompt seam (adapter.rs): stream_prompt_blocks returns PromptExecution { final_text, terminal_error, silent_failure }; chat callers map back mechanically while the executor consumes the classification to decide Completed vs Failed.

Round Scope Note

Reviewed head fbe8c7ee, stacked on feat/cp-observer (9ba4e187), merge-base equals the base head (clean stack). This round is a multi-lane group review (correctness/safety, security/operability, docs/tests) verifying the follow-up fixes the author landed after the previous new-head review (round-1 self-review at this SHA). Every claimed fix was re-checked line-by-line at this head, not assumed.

Verified fixed this round:

• NF1 🔴 (silent failure misclassified as Completed): the classification now runs on the raw answer body before any tool-summary/reset/presentation composition (adapter.rs:1142 via is_silent_execution, adapter.rs:1537-1539), is carried in PromptExecution.silent_failure (adapter.rs:1395-1399), and is consumed by the executor to emit Failed (executor.rs:327-335). Regression tests pin both the seam (adapter.rs:2542, raw body vs rendered tool text) and the executor mapping (a fake runner returns rendered tool text with silent_failure = true and the result is Failed, executor.rs:899).
• NF2 (result cap ignored the envelope / was decoupled from the frame limit): cp/delegate_result now fits the complete serialized JSON-RPC frame (client.rs:550-596); the client ceiling MAX_FRAME_BYTES and the CP share MIN_RUNTIME_FRAME_BYTES = 1 MiB (proto.rs:43), and CP config validation rejects a server ceiling below the shared minimum (crates/openab-cp/src/config.rs:310-315, test :625). A regression builds a 600 KiB delegation_id + 512 KiB result and asserts the full serialized frame stays under the ceiling (client.rs:804-815).
• NF3 (teardown bound not strictly under the drain window): TEARDOWN_BOUND = 4s (executor.rs:414) is now strictly below DRAIN_TIMEOUT = 5s (client.rs:51), and cancel_and_discard shares one absolute deadline across both phases (executor.rs:359-378).
• NF4 (CP-task-failure early return bypassed ordered cleanup): an unexpected CP-task exit is recorded rather than returned immediately (main.rs:1739-1742), then routed through the shared cleanup that signals shutdown, stops the CP before the pool, runs pool.shutdown() and the pre-shutdown hook, and only afterward returns the fatal error (main.rs:1860-1964).
• NF5 (redundant outer shutdown arm could win over the inner drain): outer run now awaits connect_and_serve with no competing shutdown branch (client.rs:123-145); only connect/register race the outer shutdown signal, and once registered the serve loop exclusively owns cancel/drain/abort/clean-close (client.rs:411-439). Integration test starts a hanging delegation, sends shutdown, and asserts exactly one cancel + one discard before exit (tests/cp_client.rs:573-606).

Findings

#	Severity	Finding	Location
F25	🟡	Carried: the openab.delegation.v1 context schema (delegation_id, from, chain, deadline) is an agent-facing contract but is documented nowhere under docs/ (zero grep hits). The code comment now explains the multi-schema envelope, but a delegated-agent author has no doc reference	executor.rs:620-637
F26	🟡	Carried: openab.delegation.v1 is emitted inside the shared <sender_context> envelope, which the turn-boundary ADR says stays openab.sender.v1. The in-code doc comment now states the envelope is multi-schema and agents must dispatch on schema, but the ADR/config docs still do not record this	executor.rs:576-603,620
F56	🟡	New (nit): the teardown regression test's assertion failure message says "teardown must complete at the shared 5s bound", but it asserts against TEARDOWN_BOUND (4s). The assertion is correct; only the message is stale and would mislead a future debugger	executor.rs:1224
F13	🟢	Praise: silent-failure classification fixed at the raw-answer seam with a constructor-level Failed path and paired seam+executor tests; full-serialized-frame sizing with a shared MIN_RUNTIME_FRAME_BYTES floor and CP-side validation; shared-deadline bounded teardown; ordered CP-before-pool shutdown with supervised client task; admission-scoped session keys; lock-poison recovery including SlotGuard::drop; primary POLICY_DENIED worker guard; ack protocol-version check; ws:// fail-closed with explicit opt-in; redacted auth_key Debug; reconnect jitter + connect timeout; real in-process CP integration tests	--

Finding Details

🟡 F25 + F26: Document the delegation context schema

delegation_context_json (executor.rs:620-637) stamps schema: "openab.delegation.v1" with delegation_id, from, chain, and deadline, delivered inside the same <sender_context> envelope every platform arrival uses (adapter.rs:576-603). This is a stable, agent-facing contract: a delegated agent parses these fields to learn its provenance and deadline. It is documented in no file under docs/ at this head, and it reuses an envelope the turn-boundary ADR promised would stay openab.sender.v1. The in-code comment now correctly explains the multi-schema intent, which is a real improvement, but the external contract still needs a home.

Requested change: document openab.delegation.v1 (fields, meaning, trust model) in the ADR or config reference, and state explicitly that the <sender_context> envelope is multi-schema and consumers MUST dispatch on schema. Alternatively, record an explicit deferral to PR 4/4 where the consumer surface lands.

🟡 F56: Fix the teardown test's assertion message

stalled_cancel_and_stalled_discard_share_one_teardown_bound asserts elapsed == TEARDOWN_BOUND (now 4s) but its message reads "teardown must complete at the shared 5s bound". The test is correct; the message is a stale copy from when the bound equalled the 5s drain window and would misdirect anyone diagnosing a future failure.

Requested change: update the message to reference the 4s shared teardown bound (or interpolate TEARDOWN_BOUND).

Baseline Check

• PR opened: 2026-08-12; reviewed head fbe8c7ee11c67f68fa3850ebb158c3e40f6817b3
• Stacked PR: base is feat/cp-observer (not the default branch); merge-base 9ba4e187 equals GitHub's base.sha (clean stack)
• Diff stat: 22 files, +3752/-45, reviewed locally against base...head only
• Delta vs the previous new-head review: the follow-up fix work for the round-9-followup findings (silent-failure classification, full-frame sizing + shared minimum, teardown bound, ordered shutdown cleanup, shutdown-select ownership), plus the merged latest base
• Net-new value: the runtime-side CP client, delegation executor, [control_plane] config, headless boot, the prompt-execution seam, and the openab-cp server/proto feature split
• CI at review time: check, conformance, validate, validate-packaged-pins, and the native-sandbox smoke test green on this SHA; the remaining docker smoke tests and build-builder were still in progress with no failures; no .github/workflows/ changes in the diff

What's Good (🟢)

• The critical was fixed at the right layer: silent-failure classification is computed from the raw answer body before composition and threaded through PromptExecution to a constructor-level Failed path, with paired seam and executor regression tests.
• Transport safety is now contract-linked: the result cap measures the complete serialized frame, the client and CP share MIN_RUNTIME_FRAME_BYTES, and CP config validation rejects a smaller ceiling -- closing the escape-inflation and envelope-bytes gaps from earlier rounds.
• Teardown and shutdown ordering are correct: a single shared 4s teardown deadline stays under the 5s drain window, and a CP-task failure now runs the full ordered cleanup (CP before pool, pool.shutdown(), hooks) instead of returning early.
• Defense-in-depth landed across the board: admission-scoped session keys, lock-poison recovery including the drop path, a primary POLICY_DENIED worker guard, ack protocol-version validation, ws:// fail-closed with an explicit opt-in, and a redacted auth_key Debug.
• Credential hygiene re-verified: header-only, set_sensitive(true), absent from the serialized register frame, never logged, agent child from env_clear().
• Real-server integration coverage drives the actual in-process CP server, including the new shutdown-while-delegating drain test.

Addressing External Feedback

• Author fix summaries (round-9 and new-head follow-up): independently re-read at this exact head. All five new-head findings (silent-failure classification, full-frame sizing + shared minimum, teardown bound, ordered shutdown cleanup, shutdown-select ownership) are verified fixed with the tests the author described. The round-9 F1-F11 fixes (transport policy, connect timeout, jitter, lock poisoning, worker-role guard, ack version, credential redaction, headless naming/table, frame-limit naming, task supervision, startup remediation) are also verified in place.
• Independent audit note (APPROVE): its positive conclusions on escape-aware truncation, RAII slot release, admission-scoped discard safety, and shutdown ordering match this review. The two remaining items above are documentation and a test message, not code-correctness regressions.
• Inline review threads: none exist on the current head, so there is nothing to reply to or resolve.

5. Three Reasons We Might Not Need This PR

1. The operational half still lands before the initiator half. The fleet gains long-lived sockets, reconnect behavior, and headless workers before PR 4/4 provides the primary-side user surface; F25/F26 are the last symptom -- the agent-facing delegation context contract ships before its documented consumer.
2. The null chat adapter still expresses an abstraction mismatch. Delegation is request/response RPC delivered through a chat-rendering seam; the silent-failure bug fixed this round was a direct symptom, and future chat-contract changes can still ripple into the delegation path.
3. A sidecar could isolate protocol churn. Keeping the CP client out of the main runtime would allow independent upgrades and failure containment, at the cost of another process and a local interface.

The ADR already chose an in-process client, and the correctness and hardening work this round is verifiably sound; these remain timing and factoring arguments, not verdicts against the feature.