fix(review 005 Unit 2): op/register serving-registry collision gate (G-03)
- op_register_handler takes the serving registry alongside the connection and rejects announced names that collide with the serving side's own registrations (ALREADY_EXISTS regardless of replace). Peer-announced ops may collide with peer-announced ops (replace governs, the reconnect path) but never shadow the deployment's own ops: the connection overlay resolves before base in PeerCompositeEnv, so an unscreened same-name announce would silently rewrite what a wire-dispatched handler's ctx.env.invoke resolves. Composition authority (ADR-018) stays with the deployer. - ADR-022 amendment (2026-09-04): collision policy recorded in the 2026-09-03 amendment's op/register section (rationale + visibility irrelevance); status line notes the sub-amendment. - Gates: base-External collision rejected even with replace (overlay stays clean, serving registration untouched); Internal base op equally protected; overlay/overlay collisions still follow replace; nested composition of a base op resolves the serving side's own op after an unrelated announce (real compose_root_env env shape). - PeerCompositeEnv resolution order deliberately unchanged. Verification: cargo test 587 / --all-features 604, clippy (all-targets, all-features, wasm32) clean, fmt clean, doc clean. Refs docs/reviews/005-...md (G-03; Unit 3 open).
This commit is contained in:
glm-5.3-flash committed
1 parent
1cbb7c6536
commit
23c9b28c6b
4 files changed
+330
-11
No files matched your search
@@ -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).
|
||||
|
||||
|
||||
@@ -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<OperationRegistry>` 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
|
||||
|
||||
@@ -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,
|
||||
|
||||
+238
-6
@@ -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<CallConnection>) -> Handler {
|
||||
pub fn op_register_handler(
|
||||
connection: Arc<CallConnection>,
|
||||
serving_registry: Arc<OperationRegistry>,
|
||||
) -> 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<dyn crate::registry::env::OperationEnv + Send + Sync> = 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"
|
||||
);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user