[chaodu-agent]

What problem does this solve?

The runtime half of the Agent Control Plane (#1528) needs the CP wire types (openab_cp::proto) inside openab-core, but today openab-cp always pulls in axum, tokio and the whole WebSocket server. This PR is part 1 of 3 re-landing #1528 (auto-closed by the stale-PR bot on 2026-09-13, not rejected), split so each piece can be reviewed in one sitting. All three are needed by openabdev/openab-pty#77 (Phase 0: openab-pty hosts as CP workers, Connect as primary).

Refs #1528, openabdev/openab-pty#77

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

Review Contract

Goal

Make openab_cp::proto usable with default-features = false, without axum/tokio/the server, while keeping the default build of openab-cp (library + openab-cp binary) unchanged. Add the shared MIN_RUNTIME_FRAME_BYTES constant (not enforced yet; CP config validation of the floor lands with the runtime client in PR B).

Non-goals

• No runtime client, worker executor, headless mode or [control_plane] config: that is PR B.
• No primary state, local UDS API or openab agent CLI: that is PR C.
• No MCP direct tools or auto-facade: optional PR D.
• No change to the CP protocol, registry, router or policy behaviour. No other crate depends on openab-cp yet.

Accepted Residual Risks

• Until PR B lands, nothing in the workspace uses --no-default-features for openab-cp. The proto-only build is covered by the explicit checks listed under Validation, not yet by a dependent crate.

Acceptance Criteria

• cargo test -p openab-cp passes (unit tests + ws_lifecycle)
• cargo check -p openab-cp --no-default-features and cargo test -p openab-cp --no-default-features pass, and cargo tree -p openab-cp --no-default-features -e normal contains no axum/tokio/hyper
• cargo clippy -p openab-cp --all-targets -- -D warnings is clean, with and without default features
• Cargo.lock is unchanged (--locked builds pass)
• The required CI / check job is green

Follow-ups

• PR B feat(cp): runtime CP client and worker delegation serving (headless worker): [control_plane] config, client (register/heartbeat/reconnect, bounded writes), worker executor (PromptRunner), PromptExecution seam, drop_idle_gate rebased onto fix(pool): keep the [[ws:]] workspace across hung eviction #1557, headless worker mode. Fixes feat(cp): land Agent Control Plane v1 on main #1528 round-6 F3 (client tick clamp), F10, F11 (Serve comment) and drops the round-1 ride-along edits.
• PR C feat(cp): primary delegation state, owner-only local API and openab agent CLI: primary.rs, local.rs, the primary command arm, openab agent spawn|list|status|cancel. Fixes round-6 F1 (the red CI test), F3 (independent prune timer), F4, F5, F6, F7 (including CP-side empty-selector rejection), F9, F11 and F14.
• PR D (optional): session-gated MCP tools. Fixes round-6 F2 and F8.

At a Glance

openab-cp crate
├── proto            (always)          ← runtime clients: default-features = false (PR B)
└── feature "server" (default)
    ├── config / registry / policy / router / events / server
    └── [[bin]] openab-cp  (required-features = ["server"])

Prior Art & Industry Research

Not applicable: this is a Cargo feature split of an existing internal crate with no runtime, agent or scheduling behaviour change. The pattern (wire types always on, server behind a default feature) is the standard Cargo approach for a crate that is both a library contract and a binary.

OpenClaw: Not applicable (no equivalent crate boundary).

Hermes Agent: Not applicable (no equivalent crate boundary).

Proposed Solution

• crates/openab-cp/Cargo.toml: serde, serde_json and chrono stay unconditional. tokio, axum, futures-util, toml, tracing, tracing-subscriber, anyhow, parking_lot, clap and subtle become optional behind server (default). [[bin]] openab-cp and [[test]] ws_lifecycle require server.
• src/lib.rs: proto is always compiled. The other modules are #[cfg(feature = "server")]. A doc block explains the split.
• src/proto.rs: MIN_RUNTIME_FRAME_BYTES = 1 MiB, the frame ceiling the runtime client in PR B relies on. It is not enforced in this PR: the validate() floor and its test move to PR B with the client that needs them (review round 1 F1/F2).
• src/config.rs tests: the max_outbound_queue_bytes <= max_frame_bytes case uses queue 1048576, frame 2097152, and every rejection case asserts its error text instead of a bare is_err().

This is the code reviewed as part of #1528 (commit 9455b50, which round 6 F12 found clean), carried onto current main with no conflicts. The changes to it are the test tightening above, deferring the frame-floor validation to PR B, and doc wording that describes the runtime client as a follow-up PR rather than already bundled.

Why this approach?

It keeps one crate as the single source of the wire contract and avoids a separate openab-cp-proto crate before a second consumer exists. The default build is unchanged, so cargo run -p openab-cp and the docs keep working. Landing it first gives B and C a small, already-reviewed base.

Alternatives Considered

• Separate openab-cp-proto crate: a cleaner boundary, but it adds a crate and a path move with no consumer that needs it yet. It can be extracted later if openab-pty embeds the client.
• Reopen feat(cp): land Agent Control Plane v1 on main #1528 as one 10k-line PR: rejected by the owner in favour of the split; see the PR B/C follow-ups.

#1528 review findings addressed here

• Round 6: none of F1–F14 are in this PR's files. F12 verified this split as clean. Every other round-6 finding maps to B, C or D (see Follow-ups).
• Round 3 F7 (deferred to refactor(cp): post-v1 hardening follow-ups from #1528 self-review #1530, since closed): the max_frame_bytes bail! string and its substring-only test. Moves to PR B together with the floor validation itself; fix it there (ASCII message, exact-message test, no claim of frame-size negotiation).
• Also: tests/ws_lifecycle.rs is gated on server so cargo test --no-default-features runs the proto tests instead of failing to compile.

Validation

Head 98626dc (review fix) removes one unit test and the floor check; the local runs below were on 3fbe402, and CI is the gate for 98626dc.

Run on macOS (M4, rustc 1.99.1) in a clean copy of this head:

• cargo build -p openab-cp --locked
• cargo test -p openab-cp --locked: 142 unit + 9 integration passed
• cargo check -p openab-cp --no-default-features --locked; cargo test -p openab-cp --no-default-features --locked: 16 proto tests passed
• cargo tree -p openab-cp --no-default-features -e normal: no axum/tokio/hyper
• cargo clippy -p openab-cp --all-targets -- -D warnings (default and --no-default-features); cargo fmt -p openab-cp --check clean
• CI parity: cargo check --workspace, cargo clippy --workspace [--features unified] -- -D warnings, cargo test -p openab-gateway --features acp (417), cargo test -p openab-core --features acp-mcp {acp_mcp::,acp::pool::} (5 / 24), cargo test --features acp (42), cargo build --features unified
• cargo test --workspace: all pass on macOS except openab-core secrets::tests::resolve_exec_nonzero_exit, which spawns /bin/false (macOS only has /usr/bin/false). This PR does not touch openab-core; Linux CI covers it.
• Docker dummy-layer check: manifests plus empty lib.rs files and no crates/openab-cp/src/main.rs → cargo metadata --locked and root cargo check --locked succeed
• git diff origin/main -- Cargo.lock is empty

[chaodu-agent]

Important

CHANGES REQUESTED ⚠️ - The round-1 findings are fixed and the feature split is correct, but no CI job builds or tests the proto-only (--no-default-features) configuration this PR exists to provide.

What This PR Does

openab-cp always pulled in axum, tokio and the WebSocket server, so a runtime that only needs the wire types (openab_cp::proto) would inherit a whole HTTP server. This PR (part 1 of 3 re-landing #1528) makes proto usable with default-features = false and adds a shared, not-yet-enforced MIN_RUNTIME_FRAME_BYTES constant for the runtime-client PR.

How It Works

• Cargo.toml: serde, serde_json and chrono stay unconditional. The ten server dependencies are optional behind a default server feature (dep: syntax). The openab-cp binary and the ws_lifecycle integration test declare required-features = ["server"].
• lib.rs: proto is always compiled; config, events, policy, registry, router and server are #[cfg(feature = "server")].
• proto.rs: adds MIN_RUNTIME_FRAME_BYTES = 1024 * 1024, documented as not enforced yet and with no frame-size negotiation in the protocol.
• config.rs: the round-1 validate() floor was removed (98626dcd). The rejection tests now assert the specific error text instead of a bare is_err().
• 1a317128 / 9c116627: document Cargo feature unification in Cargo.toml and lib.rs.

Findings

#	Severity	Finding	Location
1	🟡	CI never builds, lints or tests the proto-only configuration	.github/workflows/ci.yml (job check)
2	🟢	Round-1 F1 and F2 resolved: the floor check, its test and the "negotiated ceiling" text are gone	crates/openab-cp/src/config.rs, crates/openab-cp/src/proto.rs:40
3	🟢	Feature split is structurally correct; the proto-only build has no server imports	crates/openab-cp/Cargo.toml, crates/openab-cp/src/lib.rs
4	🟢	Feature-unification caveat is now documented, and the dev-dependency scope is stated	crates/openab-cp/Cargo.toml:17, crates/openab-cp/src/lib.rs:22
5	🟢	Config rejection tests assert the specific error, and the queue-vs-frame case runs above the old floor	crates/openab-cp/src/config.rs:639

Finding Details

🟡 F1: The proto-only build is not covered by CI

Every cargo step in ci.yml uses --workspace or a specific feature set, and --workspace always enables openab-cp's default server feature. Nothing in CI compiles openab-cp with --no-default-features, so a server-only import added to proto.rs, a missing cfg gate, or a server dependency that loses optional = true would all merge without notice. That configuration is the reason this PR exists.

Requested change: add one step to the check job:

- name: openab-cp (proto only, no default features)
  run: |
    set -euo pipefail
    cargo clippy -p openab-cp --no-default-features --all-targets -- -D warnings
    cargo test -p openab-cp --no-default-features
    leaked=$(cargo tree -p openab-cp --no-default-features -e normal --prefix none \
      | grep -E '^(tokio|axum|hyper|tower|tracing|clap|toml) ' || true)
    if [ -n "$leaked" ]; then
      echo "::error::openab-cp proto-only build pulls in server dependencies:"
      echo "$leaked"
      exit 1
    fi

-e normal leaves out the test-only tokio-tungstenite dev-dependency, so the leak check covers only what a downstream crate would inherit. This change is prepared, but it has not been pushed yet because the push credential cannot modify workflow files. If it does not land here, it should land no later than the runtime-client PR.

🟢 F2: Round-1 findings resolved

The startup-breaking max_frame_bytes floor, its test, and the error text about a negotiated ceiling were removed from validate(). The constant's doc comment now says that enforcement arrives with the runtime client and that the protocol has no frame-size negotiation. A repository search finds no leftover references.

🟢 F3: Feature split is correct

All server modules are gated. The binary and ws_lifecycle require server. proto.rs, including its unit tests, imports only serde, serde_json, chrono and the ADR via include_str!. Cargo.lock is unchanged.

🟢 F4: Feature unification is documented

The manifest and crate docs now state that the no-server guarantee holds only when nothing else in the same build enables server. For example, cargo build --workspace enables it through the openab-cp binary. The docs also state that tokio-tungstenite is a test-only dev-dependency that never reaches downstream crates.

🟢 F5: Tests assert specific errors

Each rejection case now pairs its input with an expected error substring, which closes a false-pass gap where any error satisfied is_err(). The strictly-below case uses max_frame_bytes = 2097152, so the queue rule is what rejects it.

Baseline Check

• PR opened: 2026-10-30
• Reviewed head: 9c1166270f1563d681c6bf4a8147db64b705a26f; base main; merge-base 017ed21b3d4f2befffa2fb47ab522ee041f97ba7 (not stacked)
• Diff: 4 files under crates/openab-cp/
• Main already has: openab-cp as a single always-server crate
• Net-new value: a server-free proto build for runtime clients, and a shared frame-ceiling constant
• CI at review time: check and the smoke tests are still running on this head; the final merge gate is the required checks

What's Good (🟢)

• Small, reversible first step of the feat(cp): land Agent Control Plane v1 on main #1528 split, with no churn for existing openab-cp users (default features unchanged)
• The round-1 feedback was addressed by deferring the floor instead of rewording it, so this PR does not change runtime behavior
• Comments explain why each gate exists, not only what it does

Reviewer Verdicts

Reviewer	Verdict	Key finding
Reviewer A	CHANGES REQUESTED	F1 (proto-only CI gap)
Reviewer B	LGTM after F1 follow-up	F1, plus the feature-unification and dev-dependency caveats (now F4)
Coordinator	CHANGES REQUESTED	F1; verified F2-F5 against the local checkout

Addressing Previous Review Feedback

Round 1 (3fbe4022)

F1: The 1 MiB floor is a startup-breaking validation with no consumer in this PR

✅ Addressed in 98626dcd: the validate() check and its test moved to the runtime-client PR; the constant stays in proto.rs.

F2: Error message promises a "negotiated ceiling" that the protocol does not implement

✅ Addressed in 98626dcd: the error path was removed with the check; the constant's docs state that there is no negotiation.

No external reviewer comments or inline review threads exist on this PR.

5. Three Reasons We Might Not Need This PR

1. No in-tree consumer yet - nothing uses default-features = false until the runtime-client PR, so the split could land together with its first user.
2. A separate openab-cp-proto crate gives a stronger boundary - a feature split is opt-out: any consumer that forgets default-features = false, or any build that unifies server in, gets the server stack anyway. A dedicated crate would make that impossible.
3. More feature surface to maintain - one more feature combination needs CI coverage (F1) indefinitely, and that cost is only worth paying if more than one runtime crate needs the types.

These do not block the PR. The feature flag is the cheaper, reversible first step, and extracting a crate later stays straightforward if feature unification causes problems.