Skip to content

feat(cp): runtime client for Phase 0 (openab run worker, openab agent CLI) - #1593

Open
chaodu-agent wants to merge 15 commits into
mainfrom
feat/cp-runtime-phase0
Open

chaodu-agent wants to merge 15 commits into
mainfrom
feat/cp-runtime-phase0

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

DRAFT. Not for merge. No host has been deployed.

Summary

This brings the control-plane runtime client that was on the closed PR #1528 onto current main, in 8 commits, scoped to Phase 0 of openabdev/openab-pty#77.

What is NOT included

  • Facade and MCP path: acp_mcp.rs, pool.rs facade changes, cp_tools.rs. Commit 4 skipped main.rs, acp/pool.rs, acp_mcp.rs and docs because the F2 fix needs the facade.
  • openab-cp crate changes. These are already on main via refactor(cp): make openab-cp proto usable without the server #1588. The one exception is a 4-line port of MIN_RUNTIME_FRAME_BYTES in openab-cp/src/proto.rs, made in commit 1.
  • openab-pty changes.

Commits

9585c377 docs(cp): [control_plane] config reference
90a0100f test(cp): Phase 0 two-worker acceptance tests
f2378adf test(cp): client integration tests
52228926 fix(cp): ext4-safe socket shutdown cleanup
042bd8d6 fix(cp): close review findings F1-F6 on the runtime client
612ef7f2 feat(cp): openab agent spawn/list/status/cancel over the local socket
e68e545e feat(cp): headless openab run with [control_plane]
78f3ed73 feat(cp): add [control_plane] config and runtime client modules

Test results

The full workspace suite ran on macmini at b1f3dc15: 895 passed, 1 failed (secrets::tests::resolve_exec_nonzero_exit, pre-existing). CI run 41216928821 at 18955be3 ran cargo test --workspace and concluded success. cargo fmt --all -- --check fails across most of the repo at the base commit (pre-existing, per b1f3dc15). .github/workflows/ci.yml has no fmt step, so that failure does not gate this PR. Other commits ran only the targeted commands below.

Per-commit results (macmini = macOS arm64, p1 = Linux x86_64):

Commit Subject macmini (macOS) p1 (Linux)
78f3ed7 add [control_plane] config and runtime client modules accepted; no output recorded not run
e68e545 headless openab run with [control_plane] check and clippy -D warnings pass; openab 26/26; control_plane 50/50 not run
612ef7f openab agent spawn/list/status/cancel over the local socket check pass; control_plane 106/106; openab 32/32 not run
042bd8d close review findings F1-F6 on the runtime client check pass; control_plane 119/119 not run
5222892 ext4-safe socket shutdown cleanup check pass; control_plane 120/120 control_plane 120/120
f2378ad client integration tests check pass; cp_client 11/11; control_plane 120/120 not run
90a0100 Phase 0 two-worker acceptance tests check pass; cp_client 13/13; control_plane 120/120; both phase0_ tests pass not run
9585c37 [control_plane] config reference check pass (cached); no tests named fresh check pass; root control_plane 4/4; openab-core control_plane 120/120; cp_client 13/13; phase0_ 2/2; cap test pass

Test selection note: plain cargo test phase0_ runs zero Phase 0 tests because they live in the openab-core cp_client target. The correct form is cargo test -p openab-core --test cp_client phase0_. That form was not run standalone. Its selection ran as part of commit 7 and on p1.

Not verified:

  • Linux runs for commits 1-4 and 6-8. p1 was run only at 9585c37, not at HEAD.
  • Commit 1 has no recorded test output.
  • Runtime behavior: nothing was started, no openab agent was run against a live socket, and no Kiro login was done. Acceptance criteria 1, 2 and 4 are manual.
  • Commit 8 rendered docs were not viewed, and openab agent --help was not run.

Known failing test on main, not caused by this PR

secrets::tests::resolve_exec_nonzero_exit fails on origin/main 6214700a with the same assertion. This PR does not change it.

Deviations from the plan

  • Tracking: the branch was created with switch -c, so it tracks origin/main. It was pushed with git push -u origin feat/cp-runtime-phase0.
  • Bundle: setup used a direct fetch. /tmp/phase0.bundle was created on macmini and copied to p1 to sync the checkout.
  • R1 fix: none. The cap-test CI failure did not reproduce on ext4. R9 fix: none. The parallel test passed unchanged.
  • R3 in the plan was wrong: MIN_RUNTIME_FRAME_BYTES and its test are absent from origin/main. Commit 1 adds a 4-line port in openab-cp/src/proto.rs.
  • Commit 2: the bail text also names [line], [lineworks], [feishu] and [teams]. Author is Tautui <tautui.dev@gmail.com>, not amended.
  • Commit 3: the reference headless_run_mode change was not taken. Helpers were copied from cp_tools.rs into main.rs, serve_local is wired in, and six unit tests are included.
  • Commit 4: applied as a patch with no cherry-pick trailer. It skipped main.rs, acp/pool.rs, acp_mcp.rs and docs, because F2 needs the facade.
  • Commit 5: message amended to the plan subject. The first stress run was invalid (partial test name with --exact) and was rerun with the full name.
  • Commit 6: added an axum = "0.8" dev-dependency on openab-core. The plan named only openab-cp.
  • Commit 7: inline TOML config and a connect_raw helper. Two spawns run sequentially on one socket.
  • Commit 8: headless-primary wording follows HEAD's startup error, not the reference's "Full runtime". MCP paragraphs and [secrets.refs] text were dropped.

Open decision, unresolved: a headless primary with no adapter and no [mcp] still refuses to boot on HEAD. The options are (a) take the reference headless_run_mode change, which alters main's boot behavior, or (b) add [mcp] to the macmini primary config. This blocks plan Host setup step 6.

Pending human input: tailnet HTTPS, p1 and mini host changes (kiro-cli install, login, linger), and the Part 2 re-land check (R13).

Review Contract

Goal

Bring the control-plane runtime client from closed PR #1528 onto current main, scoped to Phase 0 of openabdev/openab-pty#77. The result: an openab run with [control_plane] can run headless as a worker, and the openab agent spawn/list/status/cancel CLI works over the local socket. Both behaviors are covered by tests on macmini (macOS) and p1 (Linux, partially).

Non-goals

  • The facade and MCP path (acp_mcp.rs, pool.rs facade changes, cp_tools.rs). Needed for F2, deferred.
  • Any change to the openab-cp crate beyond the 4-line MIN_RUNTIME_FRAME_BYTES port. The crate already landed on main via refactor(cp): make openab-cp proto usable without the server #1588.
  • Any change to openab-pty.
  • Re-running the full openab test suite at HEAD by hand. b1f3dc15 ran it on macmini, and CI runs cargo test --workspace (see Test results).
  • Deploying or starting any host. This PR does not change host state.
  • The worker-side socket command surface: openab agent spawn and cancel against a worker's local socket. The runtime refuses both on a non-primary (crates/openab-core/src/control_plane/client.rs:611 for Spawn, which sends cp/delegate, and :658 for Cancel). Test a_worker_runtime_refuses_spawn_locally (crates/openab-core/tests/cp_client.rs:1064) pins the spawn refusal. No test covers the cancel refusal. This is by design for Phase 0, which validates primary-side spawn/list/status/cancel and worker-side delegation only. The worker-side socket command surface is out of scope for this PR and must be a follow-up.

Accepted Residual Risks

  • Linux coverage is partial. Commits 1-4 and 6-8 have no Linux run. p1 was run only at 9585c37, not at HEAD. Mitigation: CI and the Linux run at HEAD. Recovery: fix forward on a new commit.
  • Known failure on main: secrets::tests::resolve_exec_nonzero_exit fails on 6214700a too. Accepted as pre-existing and out of scope. Mitigation: none in this PR. Its cause is not diagnosed. Recovery: none needed from this PR.
  • Runtime behavior is unverified. Nothing was started, no openab agent ran against a live socket, and no Kiro login was done. Acceptance criteria 1, 2 and 4 are manual. Mitigation: manual check before any host deployment. Recovery: revert the affected commit.
  • Headless boot behavior: a headless primary with no adapter and no [mcp] refuses to boot on HEAD. The fix choice is still open, and this PR does not decide it. Mitigation: the operator config must include [mcp], or the reference change is taken after a decision. Recovery: revert the decision commit.
  • Commit 1 has no recorded test output. Mitigation: later commits pass their test runs on top of it, but that does not prove commit 1 alone builds or passes.
  • The CI run 34434639420 and the root cause of resolve_exec_nonzero_exit are not diagnosed.
  • Delegated tool execution is not limited by code. A delegated prompt runs in the worker's ordinary agent turn with the configured [agent]'s tool permissions, as the OS user that runs openab run, including shell and file access on the worker host. The code does not narrow tools per delegation. RouterPromptRunner::run (crates/openab-core/src/control_plane/executor.rs:659) calls stream_prompt_blocks on a fresh pool session with the configured default working_dir (line 629). The executor sets no tool restriction. The ACP connection answers every agent session/request_permission without a human prompt, choosing an allow option with allow_always first (auto-respond at crates/openab-core/src/acp/connection.rs:276-286, build_permission_response from line 49, pick_best_option lines 17-46). Delegated prompts use the same connection. The shipped Dockerfiles (Dockerfile:76, Dockerfile.unified:129), the chart (charts/openab/values.yaml:274), the setup wizard (crates/openab-core/src/setup/config.rs:88) and the docs (docs/config-reference.md:360) pass --trust-all-tools. The code default is empty: default_agent_args() (crates/openab-core/src/config.rs:2042-2050) returns no args unless OPENAB_AGENT_COMMAND is set. Channel and user allowlists run in the chat message path before handle_message (AGENTS.md message flow), not on this delegation path. The only mitigation is operator-side: docs/config-reference.md:834-838 says to give a worker's key only to primaries you trust. Mitigation in code: none. Recovery: none verified in this PR. Removing --trust-all-tools from the worker's [agent].args may change which tool requests kiro-cli makes. It does not narrow what OpenAB grants, because the connection auto-approves each permission request, and no config option in acp/ or config.rs narrows those grants. A real narrowing lever needs a code change not in this PR. Not verified: this PR did not test what kiro-cli does with tool requests once that flag is removed. Maintainer acceptance required.

Acceptance Criteria

  1. cargo clippy -- -D warnings passes on macmini at HEAD 18955be3. Status: cargo clippy --workspace -- -D warnings passed at 18955be3 (recorded in its commit message). cargo check is not recorded at 18955be3; it passed (cached) at 9585c377. CI run 41216928821 at 18955be3 concluded success.
  2. cargo test -p openab-core --test cp_client phase0_ passes. Status: both phase0_ tests passed at commit 90a0100 and on p1 at 9585c37 (p1 was not run at HEAD 18955be3).
  3. cargo test -p openab-core control_plane passes at HEAD. Status: 120/120 on macmini at 18955be3 (run as cargo test -p openab-core --lib control_plane, recorded in its commit message). Earlier: 120/120 on p1 at 9585c377, and on macmini at 5222892, f2378ad and 90a0100.
  4. Manual: openab run headless as a worker starts. openab agent spawn/list/status/cancel works from a primary's live socket. Status: NOT verified.
  5. The only failing test on main, secrets::tests::resolve_exec_nonzero_exit, still fails identically on 6214700a and is not introduced by this PR. Status: confirmed by the report.
  6. Phase 0 criterion 4: Stopping one host fails its in-flight delegation as target_disconnected within the lease window (45 s). Automated: a_stopped_worker_fails_its_inflight_delegation_as_target_disconnected (crates/openab-core/tests/cp_client.rs:639) stops a hanging worker and requires the initiator to receive target_disconnected within 10 s, inside the 45 s lease default (crates/openab-cp/src/config.rs:164). Commit 8354dc8a records it passing on macmini. Manual half (stop a real host): UNVERIFIED. Steps: start a primary and a worker on separate hosts; delegate to a worker whose agent hangs; stop the worker host (shut the machine down or cut its network); confirm the primary reports target_disconnected within 45 s. The lease-expiry path (crashed or partitioned host) is not covered end-to-end.

Follow-ups

  • Decide the headless-primary boot behavior: take the reference headless_run_mode change, or require [mcp] in the primary config.
  • Port the facade and MCP path (F2) in a later PR.
  • Run the full openab test suite and Linux tests for every commit, not only HEAD.
  • Diagnose CI run 34434639420 and resolve_exec_nonzero_exit.
  • Complete manual acceptance criteria 1, 2 and 4 before any host deployment.
  • Add the worker-side socket command surface (openab agent spawn and cancel against a worker's local socket) as a follow-up PR.

Tautui and others added 8 commits October 31, 2026 10:11
Port of the openab-core runtime control-plane client from
feat/cp-runtime-client-main 9455b50 (PR 1471) onto origin/main 6214700.
Plan: .agents/tasks/phase0-cp-runtime/plan.md, Commit 1.

Taken as-is:
- control_plane/mod.rs, client.rs, executor.rs (new files)
- config.rs: ControlPlaneConfig, CpAgentType, the [control_plane] validation,
  and the control_plane_* config tests. The AgentCoreConfig whitespace hunk in
  the same commit is not taken.
- lib.rs: pub mod control_plane
- crates/openab-core/Cargo.toml: runtime openab-cp dependency with
  default-features = false. The dev-dependencies (openab-cp server, axum) are
  left for commit 6, because only the cp_client test needs them.
- cron.rs, directives.rs: the two format! to to_string clippy hunks

Applied 3-way onto main, keeping main code:
- acp/pool.rs: see deviation below
- adapter.rs: PromptExecution and the stream_prompt_blocks return value
- dispatch.rs: the .map(|_| ()) adapter

Deviations from the plan:
- acp/pool.rs: main already has discard_session returning Result<()>, which
  the dispatch rollback uses. The branch version returns bool and does not
  send session/cancel, so it is added as discard_single_use_session. Main's
  discard_session is unchanged. The executor is the only caller and was
  updated.
- crates/openab-cp/src/proto.rs: MIN_RUNTIME_FRAME_BYTES (4 lines) ported
  from the branch. The client uses it, and main does not define it. The plan
  said #1588 added it, but main proto.rs has no change since the merge base.
- Cargo.lock: cargo added one line, "openab-cp", under openab-core.
- executor.rs: rustfmt applied. It is the only new fmt diff this commit adds.

Verification (macmini, cargo and rustc 1.99.1, clone ~/repo/openab-phase0):
- cargo check -p openab-core -p openab: pass
- cargo clippy -p openab-core -p openab -- -D warnings: pass
- cargo test -p openab-core --lib control_plane: 50 passed, 0 failed
- cargo test -p openab-core --lib: 754 passed, 1 failed:
  secrets::tests::resolve_exec_nonzero_exit. The same test fails on a clean
  origin/main export (6214700), so it predates this commit. Not fixed here.
- cargo fmt --check: no new diffs in files this commit touches, apart from
  executor.rs, which rustfmt was applied to. Main also has fmt diffs.
- Linux run: not needed for this commit.
Port of the src/main.rs hunks from feat/cp-runtime-client-main 9455b50
(PR 1471) onto origin/main 6214700. Plan: .agents/tasks/phase0-cp-runtime/
plan.md, Commit 2.

What it adds:
- HeadlessMode and headless_run_mode: an adapter-less config picks the
  full runtime for [control_plane] type = worker, or for [mcp] plus
  [control_plane]. [mcp] alone stays facade-only. A primary alone stays an
  error, because its prompts come from a chat platform.
- supervise_control_plane and wait_for_control_plane_exit: a client task
  that exits or fails stops the process after the ordered cleanup.
- The control-plane client is spawned after the router and stopped before
  pool.shutdown, so the CP sees a clean disconnect.
- Discord and no-Discord runtime waits race the client task with select!.

Main code kept: the main() structure, the pool/router construction, and
the existing shutdown order. The only removed lines are the facade block
moved into the match, the old no-adapter bail text, and the bare
shutdown_signal awaits that now sit inside select!. The bail text now also
names [line], [lineworks], [feishu] and [teams], as the branch wrote it.

Verification (macmini, cargo and rustc 1.99.1, clone ~/repo/openab-phase0):
- cargo check -p openab-core -p openab: pass
- cargo clippy -p openab-core -p openab -- -D warnings: pass
- cargo test -p openab: 26 passed, 0 failed. Includes
  tests::nothing_configured_is_still_a_startup_error and the 8 headless
  mode tests.
- cargo test -p openab-core --lib control_plane: 50 passed, 0 failed
- cargo test -p openab-core --test cp_client: no such target yet. It is
  added in commit 6.
- cargo fmt --check: 14 diffs in src/main.rs, the same count as main.
  No new diffs.
- Linux run: not needed.

Known failure on main, not caused by this series:
  secrets::tests::resolve_exec_nonzero_exit
  Fails on origin/main 6214700 with the same assertion. Not fixed or skipped.
Plan commit 3, from reference 3742e03 (#1529), restricted to:
- new control_plane/local.rs and primary.rs, taken whole
- control_plane/client.rs and mod.rs hunks, applied as a patch. The
  parent blobs of both files matched HEAD, so the patch applied cleanly.
- src/main.rs: Agent subcommand, AgentAction, run_agent_command,
  parse_agent_labels, validate_deadline_secs, local API supervision and
  shutdown, and six unit tests from the same commit.

Not taken: openab-mcp facade.rs, src/cp_tools.rs, src/acp_tunnel_source.rs,
the headless_run_mode change, and the MCP facade wiring.
spawn_within_bound and new_delegation_id are copied from cp_tools.rs into
main.rs because run_agent_command calls them. Five rustfmt fixes apply to
added lines only.

Checks, run on macmini (rustc 1.99.1) in ~/repo/openab-phase0:
- cargo check -p openab-core -p openab: pass, no warnings
- cargo test -p openab-core --lib control_plane: 106 passed, 0 failed
- cargo test -p openab: 32 passed, 0 failed
- cargo run -p openab -- agent --help: lists spawn, list, status, cancel
- cargo fmt -- --check: no hunks in added code; remaining hunks predate this commit
- Cargo.lock unchanged
Port of reference c061158, control_plane/* hunks only (cherry-pick -x not used; hunks applied from its diff).
Not taken: src/main.rs (both hunks wire the F2 FacadeMode status, which needs acp_mcp.rs; skipped with acp_mcp.rs and pool.rs), acp/pool.rs, acp_mcp.rs, docs.
Checks on macmini (cargo 1.99.1):
- cargo check -p openab-core -p openab: pass, no warnings
- cargo test -p openab-core --lib control_plane: 119 passed, 0 failed
- Cargo.lock unchanged (no new dependencies)
- git diff origin/main -- crates/openab-core/src/control_plane: 0 removed lines
Linux run for this commit: not run (the plan assigns the Linux fix to commit 5).
Take 91c0a89 (local.rs only) from the closed reference branch. Shutdown
now drops our listener first, then unlinks the socket only if its (dev, ino)
still matches the bound node AND a connect probe is refused. A successor
that answers the probe is left alone, which covers ext4 reusing the freed
inode number for the next create in the same directory.

Reference CI (run 34434639420, head 91c0a89) reported the cap test
`connections_past_the_cap_are_refused_with_a_diagnostic` failing on Linux.
Root cause: NOT reproduced. The cap test passes on p1 (ext4) at this commit,
so no change beyond the reference hunk was made. The CI cause remains
UNVERIFIED.

Commands and results:
- macmini: cargo check -p openab-core -p openab: pass
- macmini: cargo test -p openab-core --lib control_plane: 120 passed, 0 failed
- p1 (ext4, Linux): cargo test -p openab-core --lib control_plane: 120 passed, 0 failed
- p1: cargo test -p openab-core --lib control_plane::local, 25 repeated runs: 25/25 pass
- p1: cap test alone under 8-way parallel load, 40 runs (--exact, full name): 40/40 "1 passed"
- cp_client tests: not run, tests/ directory is added in a later commit

Cherry-picked from 91c0a89 (see trailer).
Adds crates/openab-core/tests/cp_client.rs, taken from the reference branch
feat/cp-runtime-client-main (9455b50 and 3742e03, the only commits that
touch the file). The test boots openab-cp in-process on an ephemeral
loopback port and drives the real ControlPlaneClient against it.

Dev-dependencies added to crates/openab-core/Cargo.toml: openab-cp with the
server feature, and axum 0.8 for axum::serve. Cargo.lock gains only the
openab-core -> axum edge.

Checks run on macmini (rustc 1.99.1):
- cargo check -p openab-core -p openab: pass
- cargo test -p openab-core --test cp_client: 11 passed, 0 failed
- cargo test -p openab-core --lib control_plane: 120 passed, 0 failed
Not run: Linux (not required for this commit).
Add two in-process tests to crates/openab-core/tests/cp_client.rs, using the
real CP server and the real ControlPlaneClient with a scripted runner:

- phase0_parallel_spawns_land_on_different_workers: two workers with
  max_delegated_sessions = 1 and the same label. Two cp/delegate frames are
  written before either ack is read. Asserts the two acks name worker-1 and
  worker-2, and that each worker runner started exactly one delegation.
- phase0_worker_identity_cannot_initiate: a raw worker-type connection sends
  cp/delegate. Asserts error code -32003 (POLICY_DENIED) and no in-flight
  entry.

R9 (least-loaded tie-break): no change needed. The first test passed without
modification. Router::delegate runs select and capacity reservation under the
admission guard, so the first spawn is counted before the second selection.

Commands run on macmini (cargo 1.99.1):
- cargo test -p openab-core --test cp_client phase0_parallel_spawns_land_on_different_workers: ok
- cargo test -p openab-core --test cp_client phase0_worker_identity_cannot_initiate: ok
- cargo test -p openab-core --test cp_client: 13 passed, 0 failed
- cargo test -p openab-core control_plane: 120 passed, 0 failed
- cargo check -p openab-core -p openab: ok
- rustfmt --check on cp_client.rs: ok (after rustfmt applied)
Adds the [control_plane] section to docs/config-reference.md, taken from the
reference branch feat/cp-runtime-client-main (91c0a89) and placed before the
[cron] section. Only the [control_plane] section is included; no MCP-tool docs.

Edits to the reference text, so the docs match HEAD:
- Headless table: a primary with no adapter and no [mcp] is a startup error on
  HEAD (headless_run_mode returns HeadlessMode::None), not "Full runtime" as the
  reference docs say. The reference code at 91c0a89 differs; reported separately.
- Dropped the MCP-tool paragraphs and rows (facade tools, spawn_agent).
- Delegation deadlines bullet now covers only `openab agent spawn`.
- auth_key: removed the [secrets.refs] mention; no code path for it was found.
- Kept the "One runtime per socket path" bullet from c061158 (F6), which
  documents behavior shipped in commit 4 of this branch.

Verification:
- cargo check -p openab-core -p openab (macmini, rustc 1.99.1): Finished, exit 0.
  Cached result: this commit changes no Rust source.
- CLI flags named in the docs (spawn --deadline-secs, --label, 1..=1800
  validation) checked against src/main.rs: match.
- Code claims (0700/0600 socket perms, 10 s connect timeout, allow_insecure_transport
  gate, deadline + 5 bound) checked against HEAD: match.
- git diff origin/main -- docs/config-reference.md: additions only, no removed lines.
- No automated docs test exists.
@chaodu-agent

This comment has been minimized.

…s M1, L1-L3, L5, L9

M1: wait_for_control_plane_exit and wait_for_local_api_exit awaited a oneshot
receiver inside tokio::select!. Once it yielded, the shutdown path awaited the
same receiver again, which tokio panics on ("called after complete"), so
pool.shutdown() and pre_shutdown were skipped. Each wait now drops its
supervision after the receiver yields, and shutdown skips it.
Test: tests::a_supervised_exit_is_not_awaited_again_at_shutdown.

L1: openab agent --socket no longer evaluates default_socket_path() when an
explicit socket is given, so HOME unset no longer fails an explicit path.
L2: the headless-primary comment and the no-adapter error now state that a
control-plane primary with no adapter also needs [mcp]. Boot behaviour is
unchanged.
L3: MIN_RUNTIME_FRAME_BYTES doc says the server does not enforce the floor.
L5: client.rs comment matches behaviour: spawn and cancel are refused locally,
list_agents is forwarded for either role.
L9: spawn and delegation-id doc comments no longer claim the MCP facade calls
them. No facade caller exists at this commit.

Checks on the tree at this commit (macmini, cargo via ~/.cargo/bin):
- cargo clippy -p openab-core -p openab -- -D warnings: pass (exit 0)
- cargo test -p openab-core --lib control_plane: 120 passed, 0 failed
- cargo test -p openab-core --test cp_client: 14 passed, 0 failed
- cargo test -p openab: 33 passed, 0 failed (includes the M1 test)
- cargo test --workspace: 895 passed, 1 failed:
  secrets::tests::resolve_exec_nonzero_exit (pre-existing; secrets.rs is not
  in this PR; the test expects the "exited with" text on this macOS host)
- cargo fmt --all -- --check: fails across most of the repo at the base commit
  (pre-existing). The lines this commit adds are wrapped to rustfmt style.
Commits in this pass were not built one by one. The checks above cover the
final tree of the pass.
Adds a_worker_runtime_refuses_spawn_locally. The worker's own client answers a
Spawn with the primary-only error and sends nothing to the CP. Until now only
the CP-side refusal (-32003) was tested.

Checks (macmini): cargo test -p openab-core --test cp_client: 14 passed,
0 failed, including a_worker_runtime_refuses_spawn_locally and the two
primary_local_api_* tests that cover primary-side spawn, list, await and cancel
over the real CP (the AC4 evidence on a primary). cargo clippy -D warnings:
pass on the same tree.
A cp/delegate from a primary enters the worker's normal agent turn with the
agent's configured tool permissions. Nothing narrows tools per delegation. The
config reference now says so, so the maintainer can accept it knowingly.

Docs only. Code checks are the same as the preceding fix commit (clippy
-D warnings pass, cp_client 14 passed, control_plane 120 passed, openab 33
passed on macmini).
@chaodu-agent

This comment has been minimized.

Phase 0 criterion 4: stopping one worker fails its in-flight delegation as
target_disconnected at the initiator, within the lease. No test covered it.
The new test delegates to a hanging worker, stops that worker, and asserts
the initiator's cp/delegate_result carries target_disconnected within 10s.
The lease defaults to 30s and the delegation deadline is 300s, so a 10s
bound rules out both as the source of the failure.

Checks run on macmini at this commit:
- cargo test -p openab-core --test cp_client a_stopped_worker: 1 passed, 0 failed
- cargo test -p openab-core --test cp_client: 15 passed, 0 failed (after rustfmt layout fix)
- rustfmt --check crates/openab-core/tests/cp_client.rs: clean (exit 0)
…(L2, L14)

L2: the comment on a_control_plane_primary_alone_does_not_unlock_a_headless_boot
still gave the chat-only rationale. It now matches HeadlessMode: the primary's
initiating surface is the local socket and `openab agent`, which the headless
boot serves only with [mcp] or a chat adapter. The production wording already
said this.

L14: reflowed the new_delegation_id doc paragraph and removed the orphaned
line break after "while".

Comments only; no behaviour change.

Checks run on macmini at this commit:
- cargo clippy -- -D warnings: pass
- cargo clippy --workspace -- -D warnings: pass
- cargo test -p openab: 33 passed, 0 failed
- cargo test -p openab-core --lib control_plane: 120 passed, 0 failed
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@thepagent
thepagent marked this pull request as ready for review October 31, 2026 18:18
@thepagent
thepagent self-requested a review as a code owner October 31, 2026 18:18
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Important

CHANGES REQUESTED ⚠️ - The runtime client is carefully built and CI is green, but a reconnect contract inconsistency, thin register-ack validation, a slow shutdown close, missing failure-path tests, and several owner decisions remain open.

What This PR Does

Ports the control-plane runtime client from closed PR #1528 onto current main, scoped to Phase 0 of openabdev/openab-pty#77. An openab run with [control_plane] can run headless as a worker, and a primary exposes openab agent spawn/list/status/cancel over a local Unix socket.

How It Works

  • [control_plane] config is the switch (no cargo feature). It is deny_unknown_fields, rejects an empty key, refuses non-loopback ws:// unless allow_insecure_transport = true, and redacts auth_key in Debug.
  • control_plane/client.rs owns one WebSocket session with jittered backoff and re-registration. It is the single owner of the sink, and commands arrive as data over an mpsc channel.
  • executor.rs runs inbound cp/delegate on the worker through a PromptRunner seam with admission, cancel, deadline, and drain handling.
  • primary.rs originates cp/delegate, cp/cancel, and cp/list_agents and correlates replies and terminals. Terminal history is process-lifetime.
  • local.rs serves the openab agent CLI over a 0700/0600 Unix socket. src/main.rs adds the headless run mode and the CLI.
  • Wire types come from openab_cp::proto (default-features = false). The only openab-cp change is the 4-line MIN_RUNTIME_FRAME_BYTES port.

Findings

# Severity Finding Location
1 🟡 Await/Check during reconnect backoff returns NotConnected although terminal history is preserved; the comment contradicts the design crates/openab-core/src/control_plane/client.rs:1050-1058
2 🟡 Register ack is validated for protocol_version only; effective_max_delegated_sessions = 0 silently disables all delegated work crates/openab-core/src/control_plane/client.rs:1072, :288
3 🟡 Shutdown close can block about 60s on a dead peer (two sequential 30s WRITE_TIMEOUT bounds) crates/openab-core/src/control_plane/client.rs:586-587
4 🟡 Failure-path integration tests missing: register rejection backoff, unknown method / malformed frame, await across reconnect crates/openab-core/tests/cp_client.rs
5 🟡 Three independent timeout ladders are not documented as one hierarchy src/main.rs:330, local.rs:716, client.rs:80
6 🟡 Ownership of the initiating surface (local UDS vs future MCP facade) is not decided before the facade lands src/main.rs:249, src/main.rs:314
7 🟡 Config parsing imports tokio_tungstenite only to validate the URL (layering) crates/openab-core/src/config.rs:6
8 🟡 PR body says "DRAFT. Not for merge" but the PR is marked ready for review PR state
9 🟡 Carried-over items from earlier review passes still open or not tracked in Follow-ups PR body
10 🟡 Small nits: backoff reset on the Err arm, unlogged cp/delegate_result parse error, unpinned worker ListAgents, stale residual-risk wording see details
11 🟢 Clean wire/server separation, fail-closed auth and transport, bounded queues, and fixes landed with regression tests -
Finding Details

🟡 F1: Await/Check during backoff contradicts preserved history

client.rs:186-193 keeps PrimaryState process-lifetime so that "a caller that recorded a completed delegation before a blip can still read it after reconnect". During the backoff sleep, reject_command (client.rs:1050-1058) answers Await and Check with NotConnected, and its comment says "a disconnected client holds none: there is nothing to wait on". No result is lost, since NotConnected is documented as retryable (primary.rs:232) and the state survives. The contract is still inconsistent.

Requested change: answer Check/Await from primary during backoff (or park awaiters until reconnect), or correct the comment and document the retry contract. Cover it with an await-across-reconnect test (see F4).

🟡 F2: Register ack sanity

validate_register_ack (client.rs:1072) checks only protocol_version. effective_max_delegated_sessions goes straight to set_effective_max (client.rs:288), so an ack of 0 makes the worker refuse every delegation with no diagnostic. The CP is authenticated, and 0 may be an intended drain, so a hard reject is not required. heartbeat_interval_secs is already clamped with .max(1) (client.rs:414), and lease_expiry_secs is only logged.

Requested change: log a warn! (or reject) when the effective budget is 0, and add a unit test.

🟡 F3: Bounded but slow shutdown close

After drain, write_bounded(&mut sink, Message::Close(None)) and tokio::time::timeout(WRITE_TIMEOUT, sink.close()) run in sequence (client.rs:586-587). Each is bounded by the 30s WRITE_TIMEOUT, so a peer that stopped reading can delay reconnect or process exit by about 60s.

Requested change: use a separate short close timeout (2-5s) for the close handshake.

🟡 F4: Failure-path test gaps

cp_client.rs has 16 tests. It already covers worker spawn refusal (:1030, :1064), oversized spawn (:849), CP-initiated close and re-register (:514), and worker stop as target_disconnected (:639). Still missing against the in-process CP harness:

  • register rejection: the client backs off and does not spin;
  • unknown method or malformed frame: METHOD_NOT_FOUND or ignore, and the session survives;
  • await across reconnect (pins F1).

🟡 F5: Timeout hierarchy

The CLI wraps a spawn in deadline_secs + 5 (src/main.rs:330). The local server awaits with MAX_DEADLINE_SECS + 120 (local.rs:716). The client write bound is 30s (client.rs:80), and the CP lease defaults to 45s. Each value is reasonable on its own, but nothing documents how they relate. Future edits can easily break the ordering.

Requested change: document the hierarchy in one place (the module doc or docs/config-reference.md), ideally with a test that asserts the ordering.

🟡 F6: Decide the initiating-surface owner before the facade lands

spawn_within_bound and new_delegation_id say the MCP facade "does not call it at this commit" (src/main.rs:249, :314). Today the CLI over local.rs is the only initiating surface, so nothing is duplicated yet. Once the facade is ported, two local APIs (UDS and MCP) would each carry their own auth model, frame limits, and deadline validation.

Requested change: record in the PR body or the follow-up which surface owns spawn/list/cancel validation, so the facade PR reuses one path and does not fork it.

🟡 F7: tokio_tungstenite in config parsing

config.rs:6 imports tokio_tungstenite::tungstenite::client::IntoClientRequest to validate the URL. This adds no dependency (openab-core already depends on tokio-tungstenite), but it couples config parsing to the transport crate. Validate with url/http instead, or move the check to connect time.

🟡 F8: Draft wording vs. ready state

The first line of the PR body is "DRAFT. Not for merge. No host has been deployed.", but GitHub marks the PR as ready for review. Mark it as a draft until the open items close, or update the body.

🟡 F9: Carried-over items from earlier passes

Status at 18955be3 after the body edits:

  • M4 (delegated tool execution): the risk is recorded in Accepted Residual Risks, which ends with "Maintainer acceptance required". It still needs explicit maintainer acceptance.
  • L2 (headless-primary boot decision): still "Open decision, unresolved" in the body, although the code (HeadlessMode) and docs already require [mcp]. Record the decision.
  • L4, L6, L7, L8, L17: accepted as follow-ups in earlier passes but missing from the Follow-ups section. Add them so they are tracked.
  • L10: the lint-only edits in cron.rs:220 and directives.rs:298 are still not listed in the PR body.

🟡 F10: Nits

  • client.rs:230: the Err(e) arm never applies backoff_after_session, so a long session that ends in Err keeps a previously escalated backoff.
  • client.rs:744-747: an invalid cp/delegate_result gets INVALID_PARAMS back, but the serde error is never logged locally.
  • client.rs:671: worker ListAgents is forwarded without a role check, and nothing pins whether the CP accepts or denies it. Add an assertion either way.
  • PR body, Accepted Residual Risks: "Acceptance criteria 1, 2 and 4 are manual". Only AC4 and the manual half of AC6 are manual now.

🟢 F11: What is done well

  • Wire types are reused from openab_cp::proto with default-features = false, so the server stack stays out of the runtime build. axum is a dev-dependency only.
  • Auth and transport fail closed: Bearer header marked set_sensitive, empty key rejected, redacted Debug, non-loopback ws:// refused at config load, and a startup warn! when the escape hatch is used (client.rs:141).
  • CpAgentType excludes Observer, which makes an invalid runtime role unrepresentable.
  • Queues and tables are bounded, and the code uses watch/mpsc/oneshot with no busy-polling.
  • Earlier findings landed with regression tests (M1 in b1f3dc15, M6 in 8354dc8a, L11 in 228f714a).
Baseline Check
  • PR opened: 2026-10-31
  • Base: main (not stacked). Head: 18955be3599269c28676b1644955e35561bc1e70. Merge-base: 6214700a1e52c899b23127025abf3a8ff461a579.
  • Diff (origin/main...pr-1593): 18 files, +9114/-31.
  • Main already has: the openab-cp crate (refactor(cp): make openab-cp proto usable without the server #1588) with the server-side registry, proto, and CP tools. It has no runtime client, no headless worker mode, and no openab agent CLI.
  • Net-new value: the runtime-side client, worker executor, primary tracker, local socket API, CLI, and the [control_plane] config.
  • CI at head: 39 checks pass, 3 skipped (operator, smoke-test, smoke-test-unified path filters), 0 failed.

Addressing External Reviewer Feedback

Earlier automated review passes (comments on this PR, 2026-10-31)

M1 - control-plane failure path panics before ordered cleanup

✅ Addressed in b1f3dc15, with regression test a_supervised_exit_is_not_awaited_again_at_shutdown.

M3 - AC4 expects spawn/cancel on a worker, which the runtime refuses

✅ Addressed in the PR body: the worker-side socket surface is a Non-goal and a Follow-up. This is a scope decision and remains open to challenge.

M4 - delegated prompts run with the worker's tool permissions

ℹ️ Recorded in Accepted Residual Risks. Maintainer acceptance is still pending (F9).

M5 - stale validation record

✅ Addressed in the PR body: AC1 and AC3 cite 18955be3 and CI run 41216928821. One stale sentence remains (F10).

M6 - Phase 0 criterion 4 (target_disconnected) untested

✅ Addressed in 8354dc8a and AC6. The lease-expiry path for a crashed host is still not covered end to end (L17).

L2, L4, L6, L7, L8, L10, L17

ℹ️ Accepted as follow-ups or owner decisions in earlier passes. Tracking gaps are listed in F9.

L1, L3, L5, L9, L11, L12, L13, L14, N1

✅ Addressed in b1f3dc15, 228f714a, and 18955be3, or verified without a code change in earlier passes.

This round's review team

Reviewer Verdict Key findings
Reviewer A CHANGES REQUESTED F1-F8, F10
Coordinator baseline CHANGES REQUESTED F9, verification of all claims at 18955be3

Three claims raised during this round were checked and dropped as incorrect: missing startup warning or docs for allow_insecure_transport (both exist), a CI workflow change (none in this diff), and a lease-death race from the 30s write timeout (lease default is 45s, and the lease outcome is server-side).

5️⃣ Three Reasons We Might Not Need This PR

  1. Size versus Phase 0 goal - about 6.7k lines across four modules prove a hypothesis that a smaller outbound-only worker could test. The primary tracker and local socket mainly pay off in later phases.
  2. No live validation yet - AC4 and the manual half of AC6 are unverified, and nothing has run against a real host. Merging does not prove the Phase 0 claim.
  3. Surface may churn - the facade port (F6) and the open headless-primary decision (L2) can reshape the CLI and socket API soon after this lands.

…und close; warn on zero budget

- F1: during reconnect backoff, Await and Check read the process-lifetime
  PrimaryState instead of answering NotConnected, so a delegation that
  completed before a blip stays readable. fail_all_live() now also runs on
  backoff entry, so an early Err out of connect_and_serve cannot leave a
  parked awaiter behind.
- F2: warn when the register ack grants effective_max_delegated_sessions = 0.
- F3: the end-of-session close handshake runs under one 3s CLOSE_TIMEOUT
  instead of two sequential 30s WRITE_TIMEOUT bounds.
- F10: the Err arm applies the same stability-based backoff reset; an
  invalid cp/delegate_result logs its serde error; the ListAgents comment
  states that the CP serves it to every role.
… across reconnect

- a_rejected_registration_backs_off_instead_of_spinning
- unknown_methods_and_malformed_frames_do_not_end_the_session
- a_completed_delegation_is_readable_during_reconnect_backoff
- a_worker_runtime_refuses_spawn_locally now also pins that the CP serves
  cp/list_agents to a worker

This branch has not been deployed

No deployments
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