fix(review 005 Unit 3): resource_id_path wire round-trip + bootstrap-list doc alignment (G-04, G-05)
- resource_id_path rides both halves of the spec wire round-trip: spec_to_json_pub serializes it (optional string key), rebuild_spec_for parses it. Additive optional field - absent stays absent. Previously an announced (or from_call-imported) op declaring ownership-scoped resource extraction silently rebuilt with resource_id: None, so ACL checks ran without the resource ID. - Gates: spec_round_trips_resource_id_path (serialize -> parse -> field intact) + spec_without_resource_id_path_stays_absent (additive field breaks no consumer). - ADR-022 amendment: bootstrap-op set gains services/list-peers with a dated G-05 note (the installer has registered it since the amendment landed; the doc lagged the code). Set remains closed at four. Verification: cargo test 589 / --all-features 606, clippy (all-targets, all-features, wasm32) clean, fmt clean, doc clean. Refs docs/reviews/005-...md (G-04, G-05; all findings closed).
This commit is contained in:
@@ -385,6 +385,11 @@ channel 0. The set is closed:
|
||||
- `services/list` — discovery (the op `from_call` dials on every
|
||||
import; each side is expected to serve it)
|
||||
- `services/schema` — per-op schema disclosure
|
||||
- `services/list-peers` — peer-keyed discovery (the op that makes
|
||||
peer-announced ops discoverable; `from_call`-import discovery
|
||||
relies on it). Added to this list 2026-09-04 (review 005 G-05) —
|
||||
`install_bootstrap_discovery` has installed it since the amendment
|
||||
landed; the set was declared closed and the doc lagged the code.
|
||||
- `op/register` — peer op announcement (below)
|
||||
|
||||
These are the ops a peer may assume are reachable (subject to each
|
||||
|
||||
@@ -2,8 +2,8 @@
|
||||
|
||||
## Status
|
||||
|
||||
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.
|
||||
Units 1–3 remediated and verified (G-01..G-05). All findings closed;
|
||||
the review is resolved. See Remediation log.
|
||||
|
||||
## Scope
|
||||
|
||||
@@ -278,6 +278,8 @@ registration-side gate is the cheaper, narrower door.
|
||||
|
||||
## G-04 [minor] — `resource_id_path` does not survive the spec wire round-trip
|
||||
|
||||
**Status: REMEDIATED (Unit 3)** — see Remediation log.
|
||||
|
||||
**Verified:** YES. `spec_to_json_pub` serializes
|
||||
name/namespace/op_type/visibility/schemas/error_schemas/access_control
|
||||
(+ `channel_open`/`publish_schema` markers;
|
||||
@@ -303,6 +305,8 @@ round-trip plus a round-trip test. Additive optional field in the
|
||||
|
||||
## G-05 [minor] — `install_bootstrap_discovery` registers `services/list-peers`; the ADR-022 amendment's bootstrap set doesn't name it
|
||||
|
||||
**Status: REMEDIATED (Unit 3)** — see Remediation log.
|
||||
|
||||
**Verified:** YES. `install_bootstrap_discovery` registers
|
||||
`services/list`, `services/list-peers`, and `services/schema`
|
||||
(`src/registry/discovery.rs:290-313`; `list-peers` at `:300`). The
|
||||
@@ -378,8 +382,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. (Units 1–2 landed — see Remediation log;
|
||||
Unit 3 remains.)
|
||||
and hit G-01 immediately. (All three units landed — see Remediation
|
||||
log.)
|
||||
|
||||
## Unit 1 — Concurrent serving loop + a stub-exercising gate (G-01, G-02)
|
||||
|
||||
@@ -420,6 +424,46 @@ Unit 3 remains.)
|
||||
|
||||
# Remediation log
|
||||
|
||||
## Unit 3 — Round-trip completeness + doc alignment (G-04, G-05) — LANDED
|
||||
|
||||
**G-04 fix.** `resource_id_path` now rides both halves of the spec
|
||||
wire round-trip: `spec_to_json_pub` serializes it as an optional
|
||||
`resource_id_path` string (`src/registry/discovery.rs`), and
|
||||
`rebuild_spec_for` parses it into the rebuilt spec's fourth
|
||||
constructor argument (`src/client/from_call.rs`). Additive optional
|
||||
field — absent stays absent, no existing consumer breaks (verified by
|
||||
the companion gate). `rebuild_spec_for`'s doc note from the review
|
||||
("namespace *is* serialized but ignored in favor of
|
||||
`namespace_prefix`") was addressed by leaving the behavior as-is: for
|
||||
`op/register` the parameter is `None` (consistent) and for `from_call`
|
||||
the prefix is authoritative — the round-trip tests pin the `name`
|
||||
field handling.
|
||||
|
||||
**Gates:** `spec_round_trips_resource_id_path` (serialize → parse →
|
||||
field intact) and
|
||||
`spec_without_resource_id_path_stays_absent_through_round_trip`
|
||||
(absent key serializes nothing; rebuilt `None`) in
|
||||
`src/client/from_call.rs`.
|
||||
|
||||
**G-05 fix.** The ADR-022 amendment's bootstrap-op set gained
|
||||
`services/list-peers` with a dated note (review 005 G-05) explaining
|
||||
that the installer has registered it since the amendment landed and
|
||||
the doc lagged the code. The set remains closed at four.
|
||||
|
||||
**Verification (post-fix):**
|
||||
|
||||
```
|
||||
cargo test → 589 passed, 0 failed
|
||||
cargo test --all-features → 606 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 2 — `op/register` collision policy (G-03) — LANDED
|
||||
|
||||
**Fix shape.** `op_register_handler` now takes the serving registry
|
||||
|
||||
+65
-1
@@ -256,7 +256,10 @@ pub(crate) fn rebuild_spec_for(
|
||||
output_schema,
|
||||
error_schemas,
|
||||
access_control,
|
||||
None,
|
||||
schema_json
|
||||
.get("resource_id_path")
|
||||
.and_then(|v| v.as_str())
|
||||
.map(String::from),
|
||||
);
|
||||
|
||||
// ADR-047 §2: the `channel_open` marker survives discovery
|
||||
@@ -602,6 +605,67 @@ mod tests {
|
||||
assert_eq!(spec.access_control.resource_type.as_deref(), Some("fs"));
|
||||
}
|
||||
|
||||
// --- review 005 Unit 3 gate (G-04 spec wire round-trip) ----------------
|
||||
|
||||
/// G-04 gate: `resource_id_path` survives the spec wire round-trip —
|
||||
/// `spec_to_json_pub` serializes it, `rebuild_spec_for` parses it.
|
||||
/// The review found the field silently dropped: an announced (or
|
||||
/// `from_call`-imported) op with ownership-scoped resource
|
||||
/// extraction rebuilt with `resource_id: None`, so the rebuilt
|
||||
/// spec's ACL checks ran without the resource ID.
|
||||
#[test]
|
||||
fn spec_round_trips_resource_id_path() {
|
||||
use crate::registry::discovery::spec_to_json_pub;
|
||||
|
||||
let spec = OperationSpec::new(
|
||||
"fs/readFile",
|
||||
OperationType::Query,
|
||||
Visibility::External,
|
||||
json!({}),
|
||||
json!({}),
|
||||
vec![],
|
||||
crate::registry::spec::AccessControl::default(),
|
||||
Some("/path".to_string()),
|
||||
);
|
||||
let wire = spec_to_json_pub(&spec);
|
||||
assert_eq!(
|
||||
wire.get("resource_id_path").and_then(|v| v.as_str()),
|
||||
Some("/path"),
|
||||
"resource_id_path serialized"
|
||||
);
|
||||
|
||||
let rebuilt = rebuild_spec_for(&wire, "fs/readFile", &None).expect("rebuild");
|
||||
assert_eq!(
|
||||
rebuilt.resource_id_path.as_deref(),
|
||||
Some("/path"),
|
||||
"resource_id_path survives the round-trip"
|
||||
);
|
||||
}
|
||||
|
||||
/// G-04 companion: a spec without `resource_id_path` serializes no
|
||||
/// `resource_id_path` key and rebuilds with `None` (additive
|
||||
/// optional field — absent stays absent).
|
||||
#[test]
|
||||
fn spec_without_resource_id_path_stays_absent_through_round_trip() {
|
||||
use crate::registry::discovery::spec_to_json_pub;
|
||||
|
||||
let spec = OperationSpec::new(
|
||||
"fs/readFile",
|
||||
OperationType::Query,
|
||||
Visibility::External,
|
||||
json!({}),
|
||||
json!({}),
|
||||
vec![],
|
||||
crate::registry::spec::AccessControl::default(),
|
||||
None,
|
||||
);
|
||||
let wire = spec_to_json_pub(&spec);
|
||||
assert!(wire.get("resource_id_path").is_none());
|
||||
|
||||
let rebuilt = rebuild_spec_for(&wire, "fs/readFile", &None).expect("rebuild");
|
||||
assert_eq!(rebuilt.resource_id_path, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rebuild_spec_channel_open_marker_set_for_channels_alpn_op() {
|
||||
let mut schema = sample_schema_json("channels/tty/sub", "sub");
|
||||
|
||||
@@ -224,6 +224,9 @@ pub fn spec_to_json_pub(spec: &OperationSpec) -> Value {
|
||||
"error_schemas": error_schemas,
|
||||
"access_control": access_control_to_json(&spec.access_control),
|
||||
});
|
||||
if let Some(resource_id_path) = &spec.resource_id_path {
|
||||
json["resource_id_path"] = json!(resource_id_path);
|
||||
}
|
||||
if spec.channel_open.is_some() {
|
||||
json["channel_open"] = json!(true);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user