feat(cp): openab-cp control plane — registry, router, policy (PR 1/4) - #1469
Merged
Conversation
👥 Top 3 recent committers of changed files
Updated for e7c29c8 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
force-pushed
the
feat/openab-cp
branch
2 times, most recently
from
August 11, 2026 03:40
2f15fe8 to
81cd5f7
Compare
… PR 1/4) Implements the first slice of docs/adr/agent-control-plane.md: the standalone openab-cp binary with wire protocol, identity-bound registry, CP-authoritative policy engine, and delegation router. Addresses round-1 review findings: CP-generated registration handles for all ownership (F1), atomic delegation admission with insert-before-send (F2), parent delegation bound to its serving instance (F3), JSON-RPC 2.0 envelope validation (F4), transport/prompt/queue resource bounds (F5), lease-only heartbeats + least-loaded label scheduling (F6), and unrelated artifacts removed from the diff (F7).
chaodu-agent
force-pushed
the
feat/openab-cp
branch
from
August 11, 2026 03:41
81cd5f7 to
e2853b4
Compare
Collaborator
Author
|
All round-1 findings addressed in e2853b4 (branch rewritten; the unrelated artifacts are gone from the diff entirely).
ADR's "v1 contract amendments" section updated to record the handle-based ownership, envelope validation, and resource-bound semantics. Verified at e2853b4 on the build host: |
chaodu-agent
force-pushed
the
feat/openab-cp
branch
from
August 11, 2026 04:11
637e4cd to
29c3b02
Compare
…ult cap Round-2 review F4/F5: bearer keys never cross cleartext non-loopback TCP without an explicit allow_insecure_bind acknowledgment (TLS proxy or private network required), and result truncation counts the marker against max_result_bytes.
chaodu-agent
force-pushed
the
feat/openab-cp
branch
from
August 11, 2026 04:13
29c3b02 to
33ed835
Compare
Collaborator
Author
|
Round-2 response — note the round-2 review examined stale head
The remainder of F5 (bounded outbound queues, Per the review contract's stopping rule, round 3 should verify these fixes against head Verified at |
This comment has been minimized.
This comment has been minimized.
8 tasks
…mespace-scoped delegation ids, admission bounds
F1 — lease expiry left a zombie connection. The sweeper deregistered the
instance and failed its in-flight work but never told the connection task to
stop, and the task holds its own clone of the outbound sender, so `rx.recv()`
never ended. The socket stayed open bound to a registration that no longer
existed: every later frame (heartbeats included) hit `registry.get(handle) ->
None` and got no reply, and the client could not recover because registration
is first-frame-only. Each connection now creates a `watch` shutdown signal
(`registry::shutdown_signal`), subscribes to it before registering, and hands
a clone to the registry, which keeps it in a CP-internal `Entry` alongside the
public `Instance` (nothing routed or serialized carries it). `sweep_leases`
calls `signal_shutdown` before `deregister`, and the main loop `select!`s on
the signal, sends a Close frame, and tears down; the client then reconnects,
re-authenticates, and registers again. Proven end-to-end over a real loopback
socket by tests/ws_lifecycle.rs
`lease_expiry_closes_the_connection_and_permits_reregistration`, and at the
sweeper level by `sweep_leases_signals_the_connection_before_dropping_it` /
registry `signal_shutdown_reaches_the_owning_connection`.
F2 — remove-then-check race in `complete` and `cancel`. Both removed the
in-flight entry, released the lock, validated the caller's handle, and
reinserted on mismatch. A genuine result landing in that window saw an unknown
id and was silently dropped, the entry was briefly invisible to the deadline
sweep, and the wrong-handle frame's reinsert left the delegation to stall to
its deadline. Ownership is now validated under the SAME lock acquisition that
removes: `get()`, check the handle, `remove()` only on success, never reinsert.
Proven by `wrong_handle_result_never_hides_the_genuine_one` (both frame orders),
`genuine_result_survives_concurrent_non_owner_frames` (200 barrier-synchronised
races), and `refused_cancel_leaves_the_delegation_cancellable` for the cancel
path. The pre-existing test `result_from_wrong_handle_dropped_and_restored` was
renamed to `result_from_wrong_handle_dropped_and_entry_untouched`: its name
asserted the remove-and-reinsert behaviour this commit removes. Its body and
assertions are unchanged.
F3 — global delegation_id keyspace crossed namespaces. The in-flight table was
keyed on the client-supplied `delegation_id` alone, so one namespace's ids were
observable from another: a colliding id was denied with `DUPLICATE_DELEGATION`,
and `cp/cancel` distinguished "no such id" (`INVALID_PARAMS`) from "someone
else's live id" (`POLICY_DENIED`) — a cross-tenant existence oracle. The table
is now keyed by a composite `DelegationKey { namespace, delegation_id }`, and
parent-chain resolution is scoped to the caller's namespace too, so a foreign
parent id cannot be borrowed for its trusted chain and deadline budget. The
namespace always comes from the authenticated registration behind the handle,
never from the frame. `cp/cancel` returns one indistinguishable error — same
code and same message — for an unknown id and for not-the-initiator; only the
CP's own logs keep the distinction. Proven by
`same_delegation_id_in_two_namespaces_is_independent` (one `d-1` live in prod
and dev at once, each completing to its own initiator),
`parent_lookup_is_namespace_scoped`, and `cancel_refusals_are_byte_identical`
(the two error objects compared as serialized JSON).
F4 — unbounded authenticated pre-registration sockets. A valid key could open
unlimited sockets and park them in the registration loop forever, kept alive by
pings. Two bounds, both new config fields with serde defaults so absent fields
keep working (documented in cp.toml.example, rejected when zero by
`CpConfig::validate`): (a) `register_timeout_secs` (default 10) is a deadline on
the mandatory `cp/register` first frame, measured from the completed upgrade —
pings are skipped inside the timeout rather than extending it; (b)
`max_connections_per_identity` (default 8) is acquired in the upgrade handler,
before `on_upgrade`, so pre-registration sockets count too and an over-quota
peer is refused at the HTTP layer with 503. The slot is held by an RAII
`ConnPermit` whose `Drop` releases it, so no early return — including a failed
ack write, a rejected registration, or an upgrade that never completes — can
leak it. Proven by ws_lifecycle
`ping_only_pre_registration_socket_is_closed_at_the_deadline`,
`registration_after_the_deadline_is_not_accepted`,
`connection_quota_rejects_over_limit_and_recycles_on_disconnect`,
`pre_registration_sockets_count_against_the_quota`, and the unit tests
`conn_quota_bounds_and_recycles_slots` / `conn_quota_is_per_identity` /
`admission_bounds_default_and_are_validated`.
The end-to-end tests need a WebSocket client, added as a dev-dependency on
tokio-tungstenite 0.29 — the version axum 0.8.9 already resolves to, so
Cargo.lock gains only openab-cp's dependency edge and no new package version.
Audit follow-up: added genuine_cancel_survives_concurrent_non_owner_frames —
the cancel-side racing regression mirroring the complete-side one (200
barrier-synchronised rounds, non-initiator cancel racing the genuine
initiator cancel; genuine must always win, capacity released exactly once).
17 tasks
This comment has been minimized.
This comment has been minimized.
…helper, ADR contract sync
F1 InFlight.namespace documented, not removed: the field is read by the
observer event layer one PR up this stack (per-namespace cp/event fan-out
in fail_instance/sweep_deadlines), and it is part of the entry's identity
because delegation ids are only unique within their namespace. Removing
it here would only force the stack to re-add it.
F2 Dropped the unused `uuid` dependency from crates/openab-cp/Cargo.toml —
registration handles come from an AtomicU64, nothing generated a UUID.
Cargo.lock regenerated (openab-cp's dep list loses `uuid`; the package
itself stays, other crates still use it).
F3 Removed `codes::DEADLINE_EXCEEDED` instead of reserving it. Verified that
admission already rejects an already-expired deadline: policy::check
returns PolicyDenial::DeadlinePast, which router::delegate maps to
POLICY_DENIED — so the reserved-for-admission rationale does not hold. A
deadline that elapses in flight is not an error response either; the
sweeper synthesizes a cp/delegate_result with status `timeout`. -32006 is
left unassigned with a comment so the published numbering stays stable.
F4 Extracted the duplicated ownership-check-and-remove sequence from
complete() and cancel() into one private helper, Router::claim(), returning
Claim { Owned(InFlight), WrongOwner{..}, NotFound{..}, Unregistered }.
The single-lock-acquisition property is preserved inside the helper: the
owner comparison and the removal happen under one `inflight` lock, so a
non-owner frame never makes the entry momentarily invisible. Callers keep
their distinct logging and return shapes (None vs. byte-identical
POLICY_DENIED). All pre-existing tests pass with zero non-comment changes
inside the tests module — that is the behavior-preservation proof.
F5 Rewrote every `(review round-N FX)` comment tag across the crate into
self-standing rationale, keeping the WHY prose and dropping the round
index (src/**, plus tests/ws_lifecycle.rs, which a reviewer greps too).
`grep -rniE 'review|round-[0-9]' crates/openab-cp/` is now empty.
F6 docs/adr/agent-control-plane.md §4 amended to the enforced contract:
registration deadline (pings do not extend it; 1008 close), lease expiry
actively closing the connection with reconnect + re-register as the only
recovery, per-identity connection quota counted from the upgrade and
RAII-released, and (namespace, delegation_id) scoping including the
indistinguishable cancel refusals. The "CP restart semantics" section was
corrected against the code rather than to the reviewer's phrasing: a
restart is all leases expiring at once, but the CP cannot synthesize any
failure because the table and the sockets die with the process —
initiators reconcile against the deadline they already propagated.
Synthesized failures are the live-CP path only, and they are
**side-specific**: Router::fail_instance is a mutually exclusive branch —
a dead serving instance sends `target_disconnected` to the initiator, a
dead initiator sends a best-effort `cp/cancel` downstream (and releases
the worker's reserved capacity), never both for one delegation. Only
sweep_deadlines emits both frames, because there both peers are still
connected. Two sentences that claimed lease expiry/disconnect of either
side produces both outcomes were rewritten to match.
F7 CP-initiated closes now carry meaning: registration timeout and lease
expiry both send WS close 1008 (policy violation) with reason
"registration timeout" / "lease expired"; the over-quota upgrade's HTTP
503 gets a body naming max_connections_per_identity. ws_lifecycle
assertions extended (not weakened) to check code + reason and the 503
body; verified they fail when the code/reason/body is mutated.
F13 handle_frame() now answers NOT_REGISTERED when registry.get(handle) is
None — frames landing between a lease sweep and the connection close were
dropped silently, indistinguishable from a hung CP. New unit test:
heartbeat answered while registered, then the same frame on the swept
handle returns error code NOT_REGISTERED correlated with the request id;
a cp/delegate on a swept handle likewise never reaches the router.
F15 Added a module-level lock-hierarchy doc to router.rs: admission →
inflight, never the reverse, with the registry lock called out as an
independent lock never held together with `inflight`.
Verification (macmini, isolated worktree matching this branch so openab-core
resolves): 63 lib + 5 ws_lifecycle tests pass (baseline 62 + 5; +1 is the new
F13 test), cargo clippy -p openab-cp --all-targets -- -D warnings clean,
cargo fmt --check 0 diffs — all equal to or better than the b9850da baseline.
This was referenced Aug 13, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ty bounds, panic-safe teardown
F1 (blocking) — admission identity is now protocol-visible. The round-6
generation stopped at the CP boundary: the wire carried only the reusable
delegation_id, so after cancel-then-retry re-admitted the same id to the same
worker, a late cp/delegate_result for the old admission A peeked the live B,
was delivered to the initiator as B's terminal frame, and committed B (peek
and commit both saw B, so B's own generation matched) — freeing capacity B
still occupied and leaving B's genuine result to be dropped as unknown.
- proto.rs: `admission` (AdmissionToken) added to DelegateAck (initiator
learns it), DelegateForward (worker learns it), and DelegateResultParams as
a REQUIRED field (worker MUST echo it; absent = INVALID_PARAMS, never a
wildcard). CP-synthesized terminal frames build DelegateResultParams from
the in-flight entry, so sweep `timeout` and `target_disconnected` carry the
token of the admission they end.
- router.rs: peek_for_completion matches the echoed token against the live
entry BEFORE the initiator-bound frame is built (delivery is the
irreversible half). Mismatch → new Peek::StaleAdmission → Dropped, logged
distinctly with both tokens, but answered with the same generic ack as any
other drop: a distinguishable reply would tell the serving side whether an
id is currently re-admitted (the oracle class round-3 F3 removed).
- Token design: the generation counter was ONE GLOBAL monotonic counter;
putting that raw value on the wire would leak cross-namespace delegation
volume. Commit matching only needs never-reuse per (namespace,
delegation_id), so counters are now per namespace, held as the value guarded
by the admission lock (BTreeMap<String, u64>) — minting already happened
there, and this makes minting outside the admission sequence structurally
impossible. Fail-closed exhaustion is preserved (a counter at the ceiling is
never bumped, so it can never wrap and re-issue tokens) and is now also
proven to be per-namespace: prod exhaustion does not refuse dev.
- Contract: ADR §4 amendment rewritten — the token's full round trip, the echo
requirement, the stale-token drop, the namespace-scoping rationale — and
"first terminal frame wins" is re-keyed per admission token (correlating on
the reusable id lets a late frame for a superseded admission permanently
mask the live one). docs/control-plane.md carries the same for runtime
authors. cp/cancel deliberately does NOT carry the token yet; it lands with
the runtime-client slice.
F2 — per-connection outbound BYTE budget. 256 entries is not a memory bound
when frame sizes are configured independently (~2 GiB at 8 MiB results).
registry.rs now owns OutboundBudget: bytes are reserved before enqueue,
released on dequeue and on teardown (FrameRx::drop drains and releases, so a
surviving sender clone never sees budget consumed by a dead connection).
FrameTx/FrameRx replace the bare mpsc alias; refusal semantics are unchanged
(SendRefused is still just "this peer cannot take the frame → treat as
disconnected"). Config `max_outbound_queue_bytes` (default 16 MiB, validated >
max_frame_bytes so one maximum-size frame always fits), documented in
cp.toml.example.
F3 — global in-flight bound. Config `max_inflight_delegations` (default 4096,
validated > 0), enforced at admission before any capacity is reserved,
refused with SATURATED naming the bound. DEVIATION with rationale: the bound
is the in-flight table's own length rather than a parallel counter, so no
removal path has to remember to decrement — commit, cancel, sweep,
fail_instance and the send-failure rollback all remove the entry and the count
follows by construction. A parallel counter is one refactor away from
drifting, and a drifted global bound wedges the whole CP. The regression walks
all five removal paths and re-admits after each.
F4 — default clamp for advertised capacity. Config
`default_max_delegated_sessions_cap` (default 16, validated > 0) applied via
CpConfig::effective_max_sessions to every identity with no
max_delegated_sessions_cap; per-identity value still overrides in either
direction. There is no longer an uncapped path, so a worker cannot advertise
its way out of saturation-based backpressure.
F5 — panic-safe teardown. Teardown ran only on handle_connection's normal
return path; a panic (reachable from expect("serializable") on production
paths) left the registry entry, the in-flight rows, and the capacity they
reserve on OTHER instances for the lease sweeper to reclaim up to
lease_expiry_secs later. A RegistrationGuard scoped to the registered lifetime
now owns teardown and is its ONLY caller here, so the normal and unwind paths
cannot diverge. Idempotency (relied on because the sweeper runs the same
deregister + fail_instance pair) is verified, not assumed: deregister is
handle-keyed and returns None for an absent entry, and fail_instance releases
capacity only for entries it actually removes.
Tests: 71 lib + 6 e2e → 82 lib + 9 e2e; clippy -D warnings clean, fmt 0.
Touched existing assertions, each an extension rather than a weakening:
- every DelegateResultParams literal and the result_of helper gained the now
required `admission` field, sourced from the live entry via a new `token()`
test helper or from the ack — mechanical consequence of the required field.
- happy_path_roundtrip additionally asserts the ack, the forwarded frame and
the terminal frame all carry the same token (added coverage).
- peek_for_completion's test helper passes the live token (new parameter).
- generation_exhaustion_fails_closed_instead_of_wrapping →
admission_token_exhaustion_fails_closed_instead_of_wrapping: same
fail-closed property, now set on a per-namespace counter, plus a new
assertion that another namespace is unaffected and that the refused
admission never advances the counter.
- the stalled-initiator server test now exhausts the initiator's BYTE budget
instead of a 1-entry channel — same contract, exercising the new bound.
- the e2e stalled-writer test echoes the token from each forward (required
field) and asserts the forward carries one.
Audit follow-up: the stalled-writer e2e regression sets its byte budget above
the ~48 MiB it enqueues, so byte-refusal backpressure cannot preempt the
write-timeout path the test exists to prove (budget refusal has dedicated
regressions of its own).
Collaborator
Author
|
Round-8 response -- the critical finding and all four important findings addressed at Fixed
Stack propagationThe echo requirement lands with the stack: #1471's executor now carries Verification
On the accepted residual risks: agreed on all three; the key-length timing normalization is noted for the F1-adjacent follow-up. The docs cross-reference follow-ups are noted for a docs pass after the stack merges. |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
pending-maintainer
pending-contributor
and removed
pending-contributor
pending-maintainer
labels
…le fix, wire-break callout F1 (blocking) — the admission token now covers cancellation, symmetrically with the result path. Round 8 stopped at results, leaving `cp/cancel` keyed on the reusable `delegation_id` alone, and that gap was reachable from two directions: - CP-synthesized, no client error required. `sweep_deadlines` removes an expired entry under the in-flight lock and builds its best-effort `cp/cancel` AFTER releasing it. In that gap the initiator can legitimately re-admit the same id; with a single replica admission B routes to the SAME worker and its forward is enqueued first. The worker then sees `forward(B)` followed by an id-only cancel for A — different producers into one queue, so per-connection ordering guarantees say nothing — and B's work is aborted at the source while the CP's table still shows B live, so B can only resolve by deadline. Result-token validation cannot repair this: the work is already gone. - Initiator-facing. An application-level retry of `cancel(A)` arriving after the re-admission matched B (`claim` compared key + `from_handle` only) and removed it: B's capacity was released while its work continued, and B's genuine result was later dropped as unknown with no synthesized terminal frame. An ordinary retry became a silent kill of the retry it was cleaning up after. Changes: - proto.rs: `admission: AdmissionToken` added to `CancelParams` as a REQUIRED field. Optional would be a "cancel whatever holds this id now" wildcard — exactly the misdelivery the token exists to close. `AdmissionToken`'s doc loses the "cp/cancel does not carry a token yet" deferral and documents the both-directions round trip instead. - router.rs: `claim` takes the token and matches (from_handle, namespace + delegation_id, admission) — all three under the ONE lock acquisition that removes the entry. New `Claim::StaleAdmission` leaves the live entry and its capacity strictly untouched and is refused BYTE-IDENTICALLY with the unknown-id and wrong-owner refusals: a distinguishable reply would tell a caller whether the id it reused is currently re-admitted, the oracle class round-3 F3 removed. - Every CP-synthesized cancel stamps the ended admission's generation. Grepped all 15 `CancelParams` construction sites: the two production ones are `fail_instance` (initiator-disconnect downstream cancels) and `sweep_deadlines`, and both build from the entry they removed. The stalled-initiator/backpressure teardown needs no separate change because it synthesizes nothing of its own — `server::teardown`, which the `RegistrationGuard` drives, and `sweep_leases` both route through `fail_instance`, and `grep methods::CANCEL src/` confirms those two plus the initiator forward are the only frame producers. The initiator forward passes the caller's params verbatim, which is correct: the token was just matched against the live entry. - `Claim`'s doc comment no longer lets its single-phase framing imply the cancel path needs no admission identity. The atomicity claim is kept and scoped: it is about this operation's consistency, not about the frame having been composed against state that may since have moved on. Both properties are required; the two-phase completion path needs the token for the additional reason that it has a peek-to-commit window of its own. Deterministic regressions (no barrier timing), both negative-controlled: - `swept_admissions_cancel_frame_cannot_target_a_reused_id` — expire A via the sweep, admit same-id/same-worker B in the gap, then assert A's outbound cancel AND timeout frames carry A's token and not B's, and that B is still in flight with its capacity intact. Mis-stamping the sweep's token fails it. - `retried_cancel_for_a_superseded_admission_never_removes_the_live_one` — the initiator retries cancel with A's token after B's re-admission: refused, byte-identical to the unknown-id refusal, B untouched (in flight, capacity held, live token unchanged, nothing forwarded downstream), and B then completes normally with its own token. Disabling the generation check fails it. - `cancel_refusals_are_byte_identical` extended (not weakened) with the stale-token case and with post-refusal assertions that the live token, the in-flight count, and the capacity reservation are all unchanged. Disabling the generation check fails it too. F2 — ADR §4's `cp/delegate` example showed a client-sent `chain`, a field `DelegateParams` has never had; serde ignored it silently, which is why the drift survived. The example now shows what an initiator actually sends (`parent_delegation_id`, no chain, no admission) and the CP-forwarded frame is documented separately with `chain` + `admission` + the authenticated `from`. A `cp/cancel` example is added, since that frame is now wire-relevant. BONUS: `adr_wire_examples_round_trip_through_the_serde_structs` makes the ADR examples part of the compiled contract. `include_str!` pulls the markdown in at compile time (no runtime fs, so the crate's test discipline holds), every ` ```json ` block must parse into the struct its `method` names, and the round trip must reproduce the exact key set — so a field the example has and the struct does not fails here. The initiator-sent arm asserts the absence of `chain` and `admission` explicitly, with the message a future drift would need. An unrecognized example shape panics rather than being skipped. Verified by reintroducing the drift: the test fails with "the initiator-sent cp/delegate example must not show a client-supplied `chain`". The `../../../docs` path does couple the test to the repo layout; that is the trade, and the doc comment says so — a fixture copied into the file would drift from the ADR the same way the ADR drifted from the structs. F3 — the required-field additions are wire-breaking. ADR §4's amendment block and docs/control-plane.md now carry an explicit⚠️ callout: `admission` is required on `cp/delegate_result` (round 8) and on `cp/cancel` (here), there is no backward-compatible optional spelling, and a runtime built against the pre-token contract has EVERY result and EVERY cancel refused with INVALID_PARAMS after the CP is upgraded. The runtime-author guidance covers both roles and states that refusals are indistinguishable by design. The release-note paragraph for the PR description is at /tmp/1469-wire-break-note.md. Verification (isolated worktree on the build host): 85 lib + 9 e2e pass (baseline 82 + 9; +3 = two F1 regressions and the F2 drift test), clippy --all-targets -D warnings clean, fmt 0 diffs. Assertion changes are extensions only; the 13 test `CancelParams` literals gained the now-required field, sourced from the ack or the live entry via the existing `token()` helper.
Collaborator
Author
|
Round-9 response -- the critical finding and both important findings addressed at Fixed
Stack propagation#1471's runtime is now token-complete on both directions: the executor echoes Verification
|
This comment has been minimized.
This comment has been minimized.
F1 🔴 Parent linkage was the third surface binding CP-authoritative state to the reusable `delegation_id` without an admission token, after results (round 8) and cancels (round 9). `delegate()` resolved a parent by (namespace, parent_delegation_id) plus serving handle only, so once parent admission A ended (cancel/completion/sweep) and the id was re-admitted as B to the same worker, a residual child request composed against A satisfied every check against B: the child inherited B's CP-constructed chain and B's remaining deadline budget, and depth, cycle and parent-budget were evaluated for the wrong admission. Unlike the earlier two this is a policy-envelope hijack feeding authorization decisions, and a best-effort `cp/cancel` cannot make the worker police it. - proto.rs: `DelegateParams` gains `parent_admission: Option<AdmissionToken>`. The parent reference is a coupled pair — a parented request must carry both halves; an id without a token is INVALID_PARAMS, never a wildcard. A root delegation carries neither and is byte-unchanged on the wire. A token without an id is refused rather than ignored (documented choice), so a client that drops the id sees its bug instead of silently getting an unparented delegation. - router.rs: the parent branch matches (namespace + id) + serving handle + generation under the same single in-flight lock acquisition it used before. Chain and deadline are still read from the `p` binding that acquisition validated — no re-lookup after validation. Stale admissions join the existing single refusal shape for unknown/unauthorized parents, so the reply is not an oracle for whether an id is currently re-admitted. - Docs: ADR §4 redefines "currently serving that parent" as the specific forwarded admission (serving handle + admission), not handle + reusable id; the parented `cp/delegate` example gains `parent_admission`; the wire-breaking callout now covers parented delegations while stating roots are unaffected. docs/control-plane.md quickstart mirrors it. The ADR round-trip test additionally asserts the example shows both parent halves or neither, since key-set equality alone cannot catch a missing `None` half. Tests (85 → 91 lib, 9 e2e unchanged): - child_naming_a_cancelled_parent_admission_cannot_hijack_the_re_admission - child_naming_a_swept_parent_admission_cannot_hijack_the_re_admission Both assert the byte-identical refusal and that B is untouched — chain, deadline, admission stamp, endpoints, capacity, and no child admitted. - child_naming_the_live_parent_admission_extends_it_normally — positive control: extends B's chain and is clamped to B's budget. A 90s child is denied because it fits A's old 120s but not B's 60s, so the clamp is provably B's. - parent_reference_requires_both_the_id_and_the_admission — both coupling directions, plus the root shape still admitted. - parent_refusals_are_byte_identical — one parent id, three CP states (unknown, foreign handle, stale admission), identical error bytes. - proto: parent_reference_is_a_pair_and_root_delegations_are_unchanged_on_the_wire Existing parent tests were threaded with the token mechanically: each names its parent's real admission so the refusal under test stays attributable to its original cause (foreign serving handle, cross-namespace lookup, policy cycle) rather than to a missing token. Negative control: reverting only the generation comparison fails both ABA regressions and the oracle test while the positive control still passes; stripping `parent_admission` from the ADR example fails the round-trip test. Scope: crates/openab-cp + the two CP docs. openab-core untouched (PR 1471 constructs root delegations only). No dependency changes. Round-10 loop follow-up: the proto.rs module-level contract summary was still pre-round-10 — its AdmissionToken usage list omitted parent_admission and the chain bullet said callers supply only parent_delegation_id, contradicting DelegateParams, the router enforcement, the ADR and the quickstart. The summary now states that parent linkage is the coupled parent_delegation_id + parent_admission pair (both required on a parented request, INVALID_PARAMS otherwise, neither on a root), that AdmissionToken is carried there, and that no surface accepts a bare reusable id as a reference to an existing delegation.
Collaborator
Author
|
Round-10 response -- the critical finding addressed at Fixed
The module-level contract summary now states the general invariant the three rounds converged on: every surface that references an existing delegation names it by the (id, admission) pair; a bare reusable id is never accepted as a reference. A future fourth surface has a stated rule to violate rather than an enumeration to fall outside of. Regressions (all negative-controlled)
DocsADR section 4: "currently serving that parent" redefined as serving handle + specific forwarded admission; wire-breaking callout extended to parented delegations (roots unchanged). Quickstart mirrors both. Verification
|
…tract The v1 facade tool table and CLI examples still referenced existing delegations by bare reusable delegation_id, contradicting the invariant established for the wire protocol (result r8, cancel r9, parent r10): every surface that references an existing delegation names it by the (id, admission) pair. Async spawn_agent now returns an opaque delegation handle encapsulating (delegation_id, admission); check_delegation, cancel_delegation, and 'openab agent status <handle>' take that handle, so a delayed local API operation can never observe or abort a same-id re-admission. This is a contract-only amendment directing PR 3; no shipped code changes.
Contributor
|
Note LGTM ✅ -- The round-10 critical fix is verified: parent linkage is admission-exact, completing the (id, admission) invariant across all three wire surfaces; the one gap found this round (the planned facade/CLI contract still referenced work by bare reusable id) was fixed at this head by extending the same invariant to the local API. Full CI is green (40 checks: 39 success, 1 skipped). What This PR DoesThis is PR 1/4 of the Agent Control Plane ADR (#1465): a standalone How It WorksA runtime authenticates with a bearer key at the WebSocket upgrade, registers within a deadline, and delegates through CP-owned state. As of this head, every surface that references an existing delegation names it by the Round-10 Fix Verification
FindingsThis round consolidates a full multi-angle team review (architecture x2, performance, correctness, testing, readability, safety, docs/UX, security/CI, operability, infrastructure) at this exact head.
Finding Details🟢 F1-F2: The invariant is complete and self-defendingRounds 8-10 each moved the admission identity one layer further (result wire, cancel wire, parent linkage). This round audited for a fourth surface and found none on the wire: heartbeats reference CP-minted handles (never reused), registration references no delegation, and CP-internal paths read the in-flight table directly. The one remaining bare-id reference was in the ADR's planned facade/CLI contract (section 6) -- a delayed local check or cancel could have targeted a same-id re-admission, recreating the ABA one layer above the wire. Fixed at this head by a contract-only amendment: the facade returns an opaque admission-aware handle, which is restart-safe (no facade-side mapping state) and keeps the invariant statement uniform across layers. The module-level rule gives any future surface a stated invariant to violate rather than an enumeration to fall outside of. 🟢 F3: Verification pathwayReviewer environments have no Rust toolchain, so the suite was not executed locally. The claims are accepted on two independent grounds: a source-level test-attribute count reproducing 91 lib + 9 e2e exactly, and green CI on this exact head (the 🟢 F4-F6: Cross-lane verification highlights
Baseline Check
Non-blocking follow-ups (tracked)
5. Three Reasons We Might Not Need This PR
Addressing External Reviewer Feedback
Round History
|
thepagent
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this solve?
First implementation slice (PR 1 of 4) of the Agent Control Plane ADR (#1465): the standalone
openab-cpbinary — wire protocol, identity-bound registry, CP-authoritative policy engine, and delegation router. Agents delegate to each other over ACP/WS instead of round-tripping through Discord/Telegram.Planned follow-up PRs: (2) OAB-side
[control_plane]config + registration client + worker pool wiring, (3) MCP facade over UDS +openab agent <verb>CLI, (4) streaming + observer endpoint.What's in this PR
crates/openab-cp(new workspace member, standalone binary per theopenab-gatewayprecedent):proto.rs— versioned JSON-RPC 2.0 envelopes;cp/register(+ack),cp/heartbeat,cp/delegate,cp/delegate_result,cp/cancel; distinct machine-actionable error codes (IDENTITY_MISMATCH,POLICY_DENIED,NO_TARGET,SATURATED,DEADLINE_EXCEEDED,TARGET_DISCONNECTED,DUPLICATE_DELEGATION,UNSUPPORTED_VERSION)config.rs— CP-owned identity table (auth key → immutablenamespace/name/typeclaims,${ENV_VAR}expansion, constant-time key lookup, duplicate-key rejection), per-namespace policy overridesregistry.rs— instance registry with replica semantics (rolling deploys route new work to the newest healthy instance), lease/heartbeat expiry, saturation-aware selection distinguishingNO_TARGETfromSATURATEDpolicy.rs— CP-authoritative checks: initiator role, depth, cycle (incl. self), namespace boundary, deadline sanity/cap/parent-budgetrouter.rs— in-flight table; CP-constructed chain (callers pass onlyparent_delegation_id; ancestry derives from authenticated identities — unforgeable); deadline sweep (synthesizedtimeout+ best-effortcp/cancel); disconnect handling both directions; result size truncation; idempotencyserver.rs— axum WS server; auth viaAuthorization: Bearerat upgrade (keys never in URLs); mandatorycp/registerfirst frame verified against the key's bound identitydocs/adr/agent-control-plane.md): new "v1 contract amendments" section freezing the docs(adr): agent control plane for direct inter-agent delegation #1465 review findings (identity binding, CP-authoritative policy, restart/saturation/timeout semantics, result cap, idempotency); §11 Q1 streaming resolved as committed fast-follow with the observer endpoint sketch.Review Contract
Goal
Ship a correct, tested control-plane core (registry + router + policy + wire contract) that later PRs (runtime client, facade, streaming) build on without protocol changes, implementing the frozen ADR baseline including the #1465 review checklist items.
Non-goals
No OAB-runtime-side code: no
[control_plane]config section, no registration client, no MCP facade/UDS/CLI, no streaming/observer endpoint (PRs 2–4). No CP HA (single instance; restart semantics are defined and implemented). No durable state, queueing, or delegation history. No Docker/Helm packaging.Accepted Residual Risks
protocol_versionfield +UNSUPPORTED_VERSIONrejection gives a compatibility gate.delegation_id, not rpc id). Acceptable for v1; revisit if per-frame acks become necessary.cp/cancelto a serving runtime is best-effort; a runtime that ignores it burns tokens until its own deadline handling stops it. The propagated deadline is the hard upper bound.Acceptance Criteria
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test -p openab-cp(42/42),cargo check --workspaceall pass#[cfg(test)]Follow-ups
[control_plane]OAB config + outbound registration client (gateway.rs backoff shape) + worker pool wiringspawn_agent,check_delegation,list_agents,cancel_delegation) over UDS + CLI/observeendpoint +openab-cp tailVerified
On macmini at the squashed head (content-identical to ac9c023):
cargo fmt -p openab-cp -- --check— cleancargo clippy -p openab-cp --all-targets -- -D warnings— cleancargo test -p openab-cp— 42 passed, 0 failedcargo check --workspace— passes with the new memberadmissionis required oncp/delegate_resultandcp/cancelThe CP wire protocol gains a required
admissiontoken on two frames:cp/delegate_result(round-8 revision) andcp/cancel(this revision, bothdirections). Both are required fields, so this is a wire-breaking protocol
change: a runtime built against the pre-token contract has every result and
every cancel refused with
INVALID_PARAMSonce the CP is upgraded.There is no backward-compatible spelling. An optional token would mean "apply
this frame to whatever holds the
delegation_idright now", which is preciselythe misdelivery the token exists to close:
delegation_idis client-suppliedand cancel-then-retry legitimately reuses it, so with a single replica the retry
re-admits the same id to the same worker. Without a token on the wire, a late
result for the cancelled admission is delivered to the initiator as the retry's
terminal frame and commits the retry — releasing capacity it still occupies —
and a stale or CP-synthesized cancel for the ended admission aborts the retry's
work at the worker.
Migration for runtime authors
admissionfrom the forwardedcp/delegateintothe matching
cp/delegate_result. Match incomingcp/cancelon the token,not on
delegation_id: a CP-synthesized cancel names the admission it endsand can arrive after a forward that reused the same id.
admissionfrom thecp/delegateack. Send it oncp/cancelto name the admission you mean to abort — a cancel naming asuperseded admission is refused, which is what prevents a retried cancel from
killing the re-admission that replaced its target. Correlate terminal frames
on
admission, not ondelegation_id("first terminal frame per admissionwins").
instance's live id, and your own id under a superseded admission all return
the same byte-identical
POLICY_DENIED. Treat a refusal as "not mine / notlive" and reconcile against your own state; the CP deliberately reports
nothing about whether an id is currently re-admitted.
No shipped clients are affected at this point in the stack — every serving
runtime in PRs 2–4 learns the token from the forwarded
cp/delegate— so themigration is mechanical. It is not, however, silent or optional.
Contract details:
docs/adr/agent-control-plane.md§4 (v1 contractamendments, "Admissions carry a protocol-visible token"); runtime-author
summary in
docs/control-plane.md("Client behavior to expect").