diff --git a/docs/architecture/decisions/022-call-protocol-client-and-adapter-contract.md b/docs/architecture/decisions/022-call-protocol-client-and-adapter-contract.md index 778f977..cc5c643 100644 --- a/docs/architecture/decisions/022-call-protocol-client-and-adapter-contract.md +++ b/docs/architecture/decisions/022-call-protocol-client-and-adapter-contract.md @@ -2,7 +2,7 @@ ## Status -Accepted (amended 2026-06-26, 2026-07-13, and 2026-07-16 — see "Amendments" below; the 2026-07-16 amendment per ADR-045 §5 removes `CallClient::connect`; amendment 2026-09-03 — the bootstrap-op set and the connect-side serving loop, see "Amendment (2026-09-03)" below) +Accepted (amended 2026-06-26, 2026-07-13, and 2026-07-16 — see "Amendments" below; the 2026-07-16 amendment per ADR-045 §5 removes `CallClient::connect`; amendment 2026-09-03 — the bootstrap-op set and the connect-side serving loop, see "Amendment (2026-09-03)" below; amendment 2026-09-04 — the `op/register` collision policy, in that amendment's "Collision policy" paragraph) ## Context @@ -443,6 +443,21 @@ unless `replace: true` (the reconnect path re-announces). The overlay dies with the connection (Layer 2), so reconnect re-announce is naturally scoped. +Collision policy (amended 2026-09-04, review 005 G-03): a +peer-announced op may collide with other *peer-announced* ops on the +same connection (`replace` governs) but **never** with the serving +side's own registrations — a name present on the serving registry +rejects with `ALREADY_EXISTS` regardless of `replace`. The connection +overlay shadows the base registry in `PeerCompositeEnv` (connections +resolve before base, ADR-024 §1), so an unscreened same-name announce +would silently rewrite what a wire-dispatched handler's +`ctx.env.invoke` resolves for any name the deployment registered: +composition authority (ADR-018) belongs to the composing handler's +deployer, not the connected peer. Visibility is irrelevant to this +gate (`Internal` ops are as shadowable as `External` — +`OverlayOperationEnv` gates on `AccessControl`, not visibility; the +composed child is `internal: true` by design). + The envelope kind set stays closed at six — bootstrap ops over channel 0 are the door (AGENTS.md §7 allows adding kinds; none is needed). diff --git a/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md b/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md index 59113ed..24809b1 100644 --- a/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md +++ b/docs/reviews/005-serving-loop-concurrency-and-op-register-review.md @@ -2,8 +2,8 @@ ## Status -Unit 1 (G-01, G-02) remediated and verified; Units 2–3 open for -remediation. See Remediation log. +Units 1 (G-01, G-02) and 2 (G-03) remediated and verified; Unit 3 +(G-04, G-05) open for remediation. See Remediation log. ## Scope @@ -223,6 +223,9 @@ with an interleaved-directions phase. ## G-03 [major] — Announced ops can shadow the serving side's own ops in nested composition; the collision gate checks the overlay only +**Status: REMEDIATED (Unit 2)** — see Remediation log; the ADR-022 +amendment records the collision policy. + **ADR drift:** ADR-018 (composition authority) and ADR-019 (the overlay is the *landing zone* for imported ops — a layer beneath the deployment's own registry, not a rival to it); ADR-022 amendment @@ -375,8 +378,8 @@ either way. Sequenced by dependency. All units are alkcall work; Unit 4 (alkhttp wiring) stays downstream and should **not** start before Unit 1 — alkhttp's serving consumers would compose over the same connection -and hit G-01 immediately. (Unit 1 landed — see Remediation log; -Units 2–3 remain.) +and hit G-01 immediately. (Units 1–2 landed — see Remediation log; +Unit 3 remains.) ## Unit 1 — Concurrent serving loop + a stub-exercising gate (G-01, G-02) @@ -417,6 +420,73 @@ Units 2–3 remain.) # Remediation log +## Unit 2 — `op/register` collision policy (G-03) — LANDED + +**Fix shape.** `op_register_handler` now takes the serving registry +alongside the connection (`op_register_handler(connection, +serving_registry)` — the review's "name-set closure" recommendation, +materialized as the `Arc` itself, mirroring how +`install_bootstrap_discovery` closes over its registry). The handler +checks `serving_registry.registration(name)` **before** the overlay +gate and rejects base collisions with `ALREADY_EXISTS` regardless of +`replace`; overlay collisions keep the existing `replace` semantics. +Announced ops may collide with announced ops, never with the serving +side's own registrations. + +**ADR-022 amendment:** the 2026-09-03 amendment's `op/register` +section gained a "Collision policy (amended 2026-09-04, review 005 +G-03)" paragraph recording the decided policy and the rationale (the +`PeerCompositeEnv` connections-before-base resolution would let an +unscreened announce rewrite composition resolution; visibility is +irrelevant to the gate). The ADR's status line notes the sub-amendment. + +**Call sites updated:** both e2e gates pass the fork the session +dispatches over (`Arc::clone(&accept_registry)` after `fork()`), which +is the production shape — the collision set is exactly the registry +the serving loop dispatches against. + +**Gates:** + +- `handler_rejects_base_registry_collision_even_with_replace` — + base-registered `fs/readFile` (External) + `replace: true` → + `ALREADY_EXISTS`; the overlay stays clean and the serving + registration is untouched. +- `handler_rejects_collision_with_internal_serving_op` — an + `Internal` base op is equally protected (the review's point that + forced `Visibility::Internal` on the *announced* spec never helped: + `OverlayOperationEnv` gates on `AccessControl`, and the composed + child is `internal: true` by design). +- `handler_overlay_collision_still_governed_by_replace` — announce/ + announce collisions still follow `replace` (the base gate is + scoped to the serving registry, not widened into the overlay). +- `nested_composition_of_base_op_unaffected_by_unrelated_announce` — + after a successful distinct-name announce, composing the base op + through the real `compose_root_env` shape (`PeerCompositeEnv` + + `attach_peer(conn.overlay_env())`) resolves the serving side's own + op, not a peer stub. (Unit-level, using the actual env types rather + than a new full-wire fixture — the wire shape is already covered by + the Unit-1 gates.) + +**Deliberate non-change:** `PeerCompositeEnv` resolution order is +untouched (connections before base) — the review's recommendation; +reordering would change long-decided layering semantics +(ADR-019/ADR-024) for no gain the registration-side gate doesn't +deliver. + +**Verification (post-fix):** + +``` +cargo test → 587 passed, 0 failed +cargo test --all-features → 604 passed, 0 failed +cargo clippy --all-targets -- -D warnings → clean +cargo clippy --all-features --all-targets -- -D warnings → clean +cargo fmt --check → clean +cargo clippy --target wasm32-unknown-unknown -- -D warnings → clean +cargo doc --no-deps → clean +``` + +--- + ## Unit 1 — Concurrent serving loop + stub-exercising gates (G-01, G-02) — LANDED **Fix shape.** `dispatch()` was split into a synchronous start half and diff --git a/src/channels/client.rs b/src/channels/client.rs index f6aced9..52422ca 100644 --- a/src/channels/client.rs +++ b/src/channels/client.rs @@ -1458,6 +1458,7 @@ mod tests { crate::registry::op_register::op_register_spec(AccessControl::default()), HandlerKind::Once(crate::registry::op_register::op_register_handler( Arc::clone(&call_connection), + Arc::clone(&accept_registry), )), OperationProvenance::Local, None, @@ -1848,6 +1849,7 @@ mod tests { crate::registry::op_register::op_register_spec(AccessControl::default()), HandlerKind::Once(crate::registry::op_register::op_register_handler( Arc::clone(&call_connection), + Arc::clone(&accept_registry), )), OperationProvenance::Local, None, diff --git a/src/registry/op_register.rs b/src/registry/op_register.rs index 1dfedc4..8d2660a 100644 --- a/src/registry/op_register.rs +++ b/src/registry/op_register.rs @@ -36,7 +36,7 @@ use crate::core::types::Capabilities; use crate::protocol::connection::CallConnection; use crate::protocol::wire::{CallError, ResponseEnvelope}; use crate::registry::registration::{ - make_handler, Handler, HandlerKind, HandlerRegistration, OperationProvenance, + make_handler, Handler, HandlerKind, HandlerRegistration, OperationProvenance, OperationRegistry, }; use crate::registry::spec::{AccessControl, OperationSpec, OperationType, Visibility}; @@ -127,18 +127,45 @@ pub fn op_register_spec(access_control: AccessControl) -> OperationSpec { /// serving it there. `services/list-peers` still discovers it (the /// overlay is peer-keyed, provenance `FromCall`). /// +/// Collision policy (review 005 G-03): announced ops may collide with +/// other *announced* ops on the same connection (`replace` governs, +/// the reconnect path) but **never** with the serving side's own +/// registrations — a name on `serving_registry` rejects with +/// `ALREADY_EXISTS` regardless of `replace`. Without this gate the +/// connection overlay shadows the base registry in `PeerCompositeEnv` +/// (connections resolve before base), so a peer could silently +/// rewrite what a wire-dispatched handler's `ctx.env.invoke` resolves +/// for any name the deployment registered — composition authority +/// (ADR-018) belongs to the composing handler's deployer, not the +/// connected peer. +/// /// Replace semantics: a registration for the same name already on the /// overlay is rejected with `ALREADY_EXISTS` unless `replace: true` /// (the reconnect path re-announces). -pub fn op_register_handler(connection: Arc) -> Handler { +pub fn op_register_handler( + connection: Arc, + serving_registry: Arc, +) -> Handler { make_handler(move |input, context| { let connection = Arc::clone(&connection); + let serving_registry = Arc::clone(&serving_registry); async move { let request = match OpRegisterRequest::from_json(&input) { Ok(r) => r, Err(e) => return ResponseEnvelope::error(context.request_id, e), }; + if serving_registry.registration(&request.spec.name).is_some() { + return ResponseEnvelope::error( + context.request_id, + CallError::already_exists(format!( + "op/register: `{}` is registered by this side's own serving \ + registry; peer-announced ops may not shadow it", + request.spec.name + )), + ); + } + if connection.overlay_contains(&request.spec.name) && !request.replace { return ResponseEnvelope::error( context.request_id, @@ -300,7 +327,7 @@ mod tests { #[tokio::test] async fn handler_registers_announced_op_in_overlay() { let conn = Arc::new(CallConnection::new(stub_connection())); - let handler = op_register_handler(Arc::clone(&conn)); + let handler = op_register_handler(Arc::clone(&conn), Arc::new(OperationRegistry::new())); let input = OpRegisterRequest { spec: announced_spec("worker/exec"), @@ -322,7 +349,7 @@ mod tests { #[tokio::test] async fn handler_rejects_collision_without_replace() { let conn = Arc::new(CallConnection::new(stub_connection())); - let handler = op_register_handler(Arc::clone(&conn)); + let handler = op_register_handler(Arc::clone(&conn), Arc::new(OperationRegistry::new())); let input = OpRegisterRequest { spec: announced_spec("worker/exec"), @@ -340,7 +367,7 @@ mod tests { #[tokio::test] async fn handler_replaces_with_replace_flag() { let conn = Arc::new(CallConnection::new(stub_connection())); - let handler = op_register_handler(Arc::clone(&conn)); + let handler = op_register_handler(Arc::clone(&conn), Arc::new(OperationRegistry::new())); let original = OpRegisterRequest { spec: announced_spec("worker/exec"), @@ -366,7 +393,7 @@ mod tests { #[tokio::test] async fn registered_spec_forced_internal_with_fromcall_provenance() { let conn = Arc::new(CallConnection::new(stub_connection())); - let handler = op_register_handler(Arc::clone(&conn)); + let handler = op_register_handler(Arc::clone(&conn), Arc::new(OperationRegistry::new())); let input = OpRegisterRequest { spec: announced_spec("worker/exec"), @@ -385,4 +412,209 @@ mod tests { crate::registry::registration::OperationProvenance::FromCall ); } + + // --- review 005 Unit 2 acceptance gates (G-03 collision policy) ------- + + /// G-03 gate: an announce colliding with a **serving-registry** + /// name rejects with `ALREADY_EXISTS` even with `replace: true` — + /// peer-announced ops never shadow the serving side's own + /// registrations (composition authority stays with the deployer). + #[tokio::test] + async fn handler_rejects_base_registry_collision_even_with_replace() { + let serving = Arc::new(OperationRegistry::new()); + serving + .register(HandlerRegistration::new( + crate::registry::spec::OperationSpec::new( + "fs/readFile", + OperationType::Query, + Visibility::External, + json!({}), + json!({}), + vec![], + AccessControl::default(), + None, + ), + HandlerKind::Once(make_handler(|input, ctx| async move { + ResponseEnvelope::ok(ctx.request_id, input) + })), + crate::registry::registration::OperationProvenance::Local, + None, + None, + Capabilities::new(), + )) + .unwrap(); + let conn = Arc::new(CallConnection::new(stub_connection())); + let handler = op_register_handler(Arc::clone(&conn), Arc::clone(&serving)); + + let input = OpRegisterRequest { + spec: announced_spec("fs/readFile"), + replace: true, + } + .to_json(); + let response = handler(input, test_context("req-or-5")).await; + let err = response.result.expect_err("base collision rejected"); + assert_eq!(err.code, "ALREADY_EXISTS"); + assert!( + !conn.overlay_contains("fs/readFile"), + "the rejected announce never lands in the overlay" + ); + // The serving side's registration is untouched. + assert!(serving.registration("fs/readFile").is_some()); + } + + /// G-03 gate: an announce colliding with an **Internal** serving + /// op is also rejected — the visibility of the shadowed op is + /// irrelevant to composition shadowing (`OverlayOperationEnv` + /// gates on `AccessControl`, not visibility; the composed child is + /// `internal: true` by design). + #[tokio::test] + async fn handler_rejects_collision_with_internal_serving_op() { + let serving = Arc::new(OperationRegistry::new()); + serving + .register(HandlerRegistration::new( + crate::registry::spec::OperationSpec::new( + "internal/vault", + OperationType::Query, + Visibility::Internal, + json!({}), + json!({}), + vec![], + AccessControl::default(), + None, + ), + HandlerKind::Once(make_handler(|input, ctx| async move { + ResponseEnvelope::ok(ctx.request_id, input) + })), + crate::registry::registration::OperationProvenance::Local, + None, + None, + Capabilities::new(), + )) + .unwrap(); + let conn = Arc::new(CallConnection::new(stub_connection())); + let handler = op_register_handler(Arc::clone(&conn), Arc::clone(&serving)); + + let input = OpRegisterRequest { + spec: announced_spec("internal/vault"), + replace: false, + } + .to_json(); + let response = handler(input, test_context("req-or-6")).await; + let err = response + .result + .expect_err("internal base collision rejected"); + assert_eq!(err.code, "ALREADY_EXISTS"); + } + + /// G-03 gate: an announced op colliding with another *announced* + /// op still follows `replace` semantics — the base-registry gate + /// must not widen into the overlay. + #[tokio::test] + async fn handler_overlay_collision_still_governed_by_replace() { + let serving = Arc::new(OperationRegistry::new()); + let conn = Arc::new(CallConnection::new(stub_connection())); + let handler = op_register_handler(Arc::clone(&conn), Arc::clone(&serving)); + + let first = OpRegisterRequest { + spec: announced_spec("worker/exec"), + replace: false, + } + .to_json(); + assert!(handler(first, test_context("req-or-7a")) + .await + .result + .is_ok()); + + let collision = OpRegisterRequest { + spec: announced_spec("worker/exec"), + replace: false, + } + .to_json(); + let err = handler(collision, test_context("req-or-7b")) + .await + .result + .expect_err("overlay collision without replace rejected"); + assert_eq!(err.code, "ALREADY_EXISTS"); + + let replacement = OpRegisterRequest { + spec: announced_spec("worker/exec"), + replace: true, + } + .to_json(); + assert!( + handler(replacement, test_context("req-or-7c")) + .await + .result + .is_ok(), + "overlay replace still permitted; base gate is scoped to the serving registry" + ); + } + + /// G-03 gate: after a successful announce of a distinct name, + /// nested composition of a **base-registered** op still resolves + /// the serving side's own op — the exact `compose_root_env` shape + /// (`PeerCompositeEnv` with the connection overlay attached). The + /// G-03 defect was that the collision gate was overlay-only; this + /// pins the invariant that survived the fix: the base layer is + /// reachable and correct whenever the name is not announced. + #[tokio::test] + async fn nested_composition_of_base_op_unaffected_by_unrelated_announce() { + let serving = Arc::new(OperationRegistry::new()); + serving + .register(HandlerRegistration::new( + crate::registry::spec::OperationSpec::new( + "fs/readFile", + OperationType::Query, + Visibility::External, + json!({}), + json!({}), + vec![], + AccessControl::default(), + None, + ), + HandlerKind::Once(make_handler(|_input, ctx| async move { + ResponseEnvelope::ok(ctx.request_id, json!({ "from": "base" })) + })), + crate::registry::registration::OperationProvenance::Local, + None, + None, + Capabilities::new(), + )) + .unwrap(); + + let conn = Arc::new(CallConnection::new(stub_connection())); + let handler = op_register_handler(Arc::clone(&conn), Arc::clone(&serving)); + + // Announce a *distinct* name; it lands in the connection overlay. + let input = OpRegisterRequest { + spec: announced_spec("worker/exec"), + replace: false, + } + .to_json(); + assert!(handler(input, test_context("req-or-8a")) + .await + .result + .is_ok()); + assert!(conn.overlay_contains("worker/exec")); + + // Compose the base op through the same env shape + // `compose_root_env` produces for a wire-dispatched handler. + let base = Arc::new(crate::registry::env::LocalOperationEnv::new(Arc::clone( + &serving, + ))); + let mut composite = crate::registry::env::PeerCompositeEnv::new(base); + composite.attach_peer("consumer-peer".to_string(), conn.overlay_env()); + let env: Arc = Arc::new(composite); + + let mut ctx = test_context("req-or-8b"); + ctx.env = env; + ctx.scoped_env = crate::registry::context::ScopedPeerEnv::new(["fs/readFile"]); + let response = ctx.env.invoke("fs", "readFile", json!({}), &ctx).await; + let out = response.result.expect("base op composes"); + assert_eq!( + out, + json!({ "from": "base" }), + "the serving side's own op resolves through composition, not a peer stub" + ); + } }