Post-remediation review of f84d214 (review 004 Units 1-3). Five
findings, verified in source and (for G-01) empirically via a probe
test that was added, run, and removed:
- G-01 [major]: serve_single_stream awaits dispatch inline; a
wire-dispatched handler composing a peer-announced op (or a
from_call import) over the same connection deadlocks — the nested
call resolves only via the 30s sweeper (probe: TIMEOUT at 30.0007s).
- G-02 [major]: the F-05 e2e gate calls the announced op directly,
bypassing the forwarding stub — the one path G-01 breaks.
- G-03 [major]: op/register's collision gate is overlay-only;
PeerCompositeEnv resolves connections before base, so an announced
op can shadow the serving side's own ops in nested composition.
- G-04 [minor]: resource_id_path does not survive the spec wire
round-trip (pre-existing shape, load-bearing for op/register).
- G-05 [minor]: install_bootstrap_discovery registers
services/list-peers; ADR-022's bootstrap set doesn't name it.
Non-findings bound the re-review: fork surface lock discipline,
bootstrap discovery closure, frame-arm equivalence of the composed
loop, unchanged pure-consumer default, alkhttp cross-repo claims,
CJK sweep (none), all gates reproduce (581/598, clippy, fmt, wasm,
doc).
Remediation plan: Unit 1 (concurrent serving loop + stub-exercising
gate) gates Unit 4 downstream; Unit 2 (collision policy); Unit 3
(round-trip completeness + doc alignment).
Verification: cargo doc --no-deps clean; tree unchanged apart from
this review doc.
Focused design-mismatch review found via alkhttp's WS data-channel
drill-down (alkhttp review 003 WS-24/WS-25). The channel machinery is
done and proven; the gaps are the dispatch-resolution mechanism and
the connect-side serving half — both upstream of any transport.
Findings:
- F-01 [major]: top-level dispatch consults only the dispatcher's
base registry — ops registered per the ADR-047 §4 amendment's
overlay mechanism resolve NOT_FOUND on the wire; the only proven
shape (per-session registry as the dispatcher's base) differs from
the ADR's wording
- F-02 [major]: no Clone/fork surface on OperationRegistry or
HandlerRegistration — every inner payload type IS Clone-able
(verified type-by-type, incl. jsonschema::Validator and
Capabilities), so the fork is a small addition
- F-03 [minor]: fork must carry handlers + validators, not just specs
- F-04 [major]: connect-side channel-0 read pump resolves responses
only; inbound call.requested frames are silently dropped — no
serving half on the single-stream shape (ADR-022/AGENTS §8
bidirectionality unreachable from the connect side)
- F-05 [major]: no wire mechanism announces client-side ops; the
six call.* kinds are closed. Resolution candidate: bootstrap op
(op/register) served per-session, handler writes into the
connection-local overlay; discovery rides services/list-peers
- F-06 [minor]: per-session fork must carry bootstrap discovery ops
for per-session openables to be discoverable
Includes a non-findings section (e2e reference shape,
register_openable completeness, channel-id split, envelope-kind
closure, wasm-cleanliness) and a 4-unit plan: ADR decisions (Unit 1)
-> fork surface (Unit 2) -> client serving + bootstrap op (Unit 3)
-> alkhttp wiring downstream (Unit 4, tracked in alkhttp review 003).
Verification: cargo test (565), clippy --all-targets -D warnings,
fmt, doc --no-deps — all clean at c16b069. No source changes.
- CF-004: services_schema_handler now applies the same visibility +
AccessControl gates as invoke() (identity resolution mirrors invoke:
handler_identity under internal). Restricted ops return spec-404 NOT_FOUND
— matches "restricted ops don't exist" and leaks nothing about the
restricted surface. Closes the unauthenticated /call-path disclosure.
- CF-003: publish_schema compiled at registration time (both
OperationRegistry::register and OperationRegistryBuilder::store);
un-compilable schemas are a registration error — an unvalidated ingest
path can no longer be constructed. Compiled validator cached per-op
(publish_validator) and consumed by dispatch; per-request compile gone.
BEHAVIOR CHANGE: register/builder reject un-compilable publish_schema.
- CF-002: demux TooLarge skip streams through a fixed 64 KiB buffer
instead of allocating the peer-declared length (u32, up to ~4 GiB);
cumulative 256 MiB skipped-bytes budget tears down dribbling peers.
Existing resync test passes unchanged.
- CF-001: new retryable CallError::connection_closed (CONNECTION_CLOSED)
applied only where the call is provably undelivered — request-frame
write failures on all consumer paths (call/subscribe/publish, both
stream modes; publish pump tags write stages). Mid-publish failures and
producer-side fail_all stay non-retryable INTERNAL (delivery ambiguous).
New code string is additive; retryable flag is the machine-readable
signal.
Verification: cargo test (558 pass, 15 new), clippy --all-targets -D
warnings, fmt --check, wasm32-unknown-unknown check.
Found via alkhttp Review 002 (PRJ-16): the services/schema handler
does a bare registry.registration(name) with no Visibility and no
AccessControl check, so POST /call (and the MCP call tool) can fetch
any Internal op's complete spec unauthenticated. The GET /schema route
and MCP schema tool enforce the pre-checks; the /call path is the
hole.
Found while fixing alkhttp's HTTP-side instance
(review-001-publish-schema-validation-robust): the identical
warn-and-skip pattern exists at src/protocol/dispatch.rs:352-366 — a
compile failure of a Pub op's publish_schema proceeds with
validator: None, so arbitrary unvalidated JSON reaches the sink
handler over the wire. Suggested direction: fail-closed + per-
registration validator cache (worked shape in alkhttp 1572a9d,
src/gateway/schema_cache.rs) + registration-time schema compilation.
Single compile site verified (both pump arms consume the one
InFlightSink.publish_validator).
Cross-crate finding from alkhttp Review 001 (WS-12), filed here per the
ledger's purpose (consumer-surfaced alkcall findings).
src/channels/adapter.rs:144: the ChunkError::TooLarge arm allocates
vec![0u8; length] from the peer's untrusted 8-byte header before
reading; length is u32, so ~4 GiB can be pinned per connection and held
indefinitely by a dribbling peer. Reachable via the alkhttp WS path by
any authenticated browser. Skip/resync logic is correct; the memory
shape is wrong — stream-skip with a bounded buffer instead.
The normal payload arm (:159) is safe (parse_header bounds it at
MAX_CHUNK_LEN); only the TooLarge arm is unbounded.
- Remove alktype from Cargo.toml (its only usage was a thin wrapper
over jsonschema::options().build())
- Replace alktype::validation::build_validator with direct jsonschema
in dispatch.rs
- Add docs/architecture/chunk-header.bast.json — the chunk header's
BAST (Binary Abstract Syntax Tree) machine-readable wire spec
- Embed as channels::wire::CHUNK_HEADER_BAST via include_str! so
downstream Rust crates can consume it without a file lookup
- Add test asserting the embedded BAST doc is valid JSON and matches
the wire format
- Update AGENTS.md §10 and implementation-specialist.md: BAST docs
are the contract; trivial/hot-path formats stay hand-rolled,
complex formats use the alktype engine or codegen
Verification:
- cargo test: 543 passed, 0 failed
- cargo clippy --all-targets -- -D warnings: clean
- cargo fmt --check: clean
- cargo doc --no-deps: clean
- BAST doc compiles + round-trips against alktype v0.2.0 engine
The R-02 acceptance gate was a line-level grep
(`grep -v 'mod tests'`) that cannot return zero while any test uses
`.expect()` — `grep -v` filters per-line, not per-scope, so every
`.expect(` inside a `#[cfg(test)] mod tests {}` block whose
individual line lacks the literal "mod tests" still matches. The suite
has ~269 such test-only `expect` calls, making the literal gate
unsatisfiable (an impossible-goal spec bug, surfaced during Unit 1
remediation).
Replace both copies of the gate (the R-02 finding gate and the Unit 1
acceptance gate) with a scope-aware verification: either (a) a targeted
grep for the specific removed strings, or (b) file-by-file confirmation
that every remaining `.expect(` is within a `#[cfg(test)] mod tests`
boundary. The intent — no `expect` in library (non-test) code — is
unchanged; only the verification method is corrected.
The publish path's decided-but-unimplemented behaviors land: per-chunk
schema validation, initiator call.error terminates the stream, and an
early handler return short-circuits the feed. alktype::validation::
build_validator is now used on both dispatch paths (the dependency was
declared and unused); jsonschema was added as a direct dependency
because its Validator type is part of alktype's public API.
P-03 — OperationSpec::publish_schema was declared, had a builder,
defaulted to None, and was never read. It now round-trips through
discovery: spec_to_json emits "publish_schema" when Some,
operation_spec_schema() advertises the property, and rebuild_spec_for
parses it back (null/absent treated as None, so an imported Pub op no
longer loses the schema). Both dispatch paths (pump_sink and
run_loop_single_stream) validate each call.published chunk's input
against the schema before yielding it to the SinkHandler; on
validation failure an Err(CallError::invalid_input(...)) with the
offending chunk in `details` is injected and the feed terminates,
matching the SinkHandler contract that an Err terminates the stream.
The validator is built once at dispatch time and carried in the
SinkDispatch (and, for the single-stream path, in a new InFlightSink
struct alongside chunk_tx); a schema that fails to compile logs a
warn and falls back to no validation. When publish_schema is None,
chunks are yielded as-is (unchanged behavior).
P-04 — pump_sink matched only call.published/call.completed/call.
aborted and dropped everything else into a debug-log ignore branch; a
call.error from the initiator was silently ignored. Both dispatch
paths now match call.error, parse the CallError from the payload,
inject it as Err(call_error) into the sink's chunk_tx, and terminate
the feed — the handler sees the initiator's error, not a synthetic
"aborted" message. A malformed payload falls back to
CallError::internal("publish error from initiator (malformed)").
P-07 — pump_sink used tokio::join!(feed_fut, handler) and only wrote
the response after both completed; an early-returning handler (e.g.
rejects after chunk 1) had its response deferred until the initiator
finished publishing or the next chunk_tx.send failed. Replaced with a
tokio::select! loop over handler.fuse() and reader.read_frame(): when
the handler completes first, the response is captured and the loop
breaks immediately, the feed is short-circuited (chunk_tx dropped,
the handler's PublishStream sees EOF), and the response is written
without waiting for the feed. The feed-wins branch (natural end /
abort / error / read failure) awaits the handler after the loop as
before. This removes the unbounded stall on a path ADR-046 intends
for long-lived streams.
Tests (15 new, 480 total):
- discovery: spec_to_json emits/omits publish_schema; operation_spec_
schema documents the property (3).
- from_call: rebuild_spec_for parses publish_schema (present/omitted/
null) and round-trips with spec_to_json (4).
- dispatch (stream-per-request): publish_schema rejects invalid chunk
+ passes valid chunks + no-schema yields as-is (3); initiator
call.error terminates with the initiator's error + malformed
fallback (2); early handler return short-circuits a slow feed and
the response is not deferred (1).
- channels/client (single-stream): publish_schema rejects invalid
chunk over channel 0; initiator call.error terminates the publish
over channel 0 (drives run_loop_single_stream directly with crafted
frames since the public publish() API takes Stream<Item = Value> and
cannot emit an initiator call.error) (2).
Verification:
- cargo test → 480 passed, 0 failed (was 465; +15 new tests)
- cargo clippy --all-targets -- -D warnings → clean
- cargo fmt --check → clean
- cargo doc --no-deps → 0 warnings
The original Unit 6 was the largest unit by item count and mixed
trivial doc fixes with real protocol-semantics work (P-03/P-04/P-06/
P-07) in one commit. Decomposed into five focused units, each
independently shippable, ordered by risk and dependency:
- Unit 6 — convention + doc cleanup (C-09, C-20-rem, C-22-rem, C-24,
P-10, P-11): mechanical; `cargo doc` clean, producer/consumer
naming, ADR-046 §3 text amendment, EVENT_* constants in pump_sink.
- Unit 7 — small correctness fixes (C-19-misc, C-21, C-23, P-13):
write_header returns Result not panic, honest error envelopes,
mux pump map leak, derive_alpn_from_op_name dead branch, Box::leak
Arc<str> refactor, REQ-CH-04 counter, opaque-wrap chunk_tx.
- Unit 8 — Pub protocol semantics (P-03, P-04, P-07): publish_schema
per-chunk validation via alktype, initiator call.error terminates
the stream, early handler return short-circuits the feed.
- Unit 9 — abort-cancels-Pub (P-06): drop the handler future on
abort (not just join! to completion); needs a cancellation token
per in-flight sink; depends on Unit 8's pump_sink changes.
- Unit 10 — stubs + substrate + doc renumber (C-10, C-11, C-14,
C-15, C-26): honestly stub the deferred behaviors with OQs, remove
stale doc claims, renumber alknet ADR refs to alkcall ADR-001..047.
Also records which original Unit 6 items were already absorbed by
Units 1-5 (P-09, C-20's reassembly deliberation, C-22's env tautology,
C-19's next_id/demux_sender/mux-error-mapping) so the units below
cover only what remains.
Verified 2026-08-13 against tree f25d0a6 (post-Units 1-5): every
remaining item re-checked in source; the `cargo doc` warning count
(2, down from 4 — the register_openable links were fixed in Unit 3)
and file:line references updated to the current tree.
Consolidates three code-review passes (one main summary + sub-review A
on ADR-046 + sub-review B on channels ADRs 034–043/047) into a single
verified document under docs/reviews/. Every finding was re-verified
directly against the source at f305f8c with exact file:line refs.
Findings (all verified): 10 critical, 19 major, 7 minor.
- Pub end-to-end cannot work (chunks written to a different bi-stream
than the request); publish() takes Vec<Value> not a Stream; 30s
client timeout; abort-doesn't-cancel-Pub; dispatch re-implements
invoke_sink inline; from_call sink forwarding swallows errors.
- Channel 0 dead in both directions; register_openable absent;
resolve_channel_manager stub; backpressure drops chunks; buffer cap
in messages not bytes; ledger decrement on 1/4 teardown paths; demux
desyncs on oversized chunk; no channel-adoption/collision scheme.
- channel/control + resources/subscribe stubs; busy-wait spin; lost
EOF sentinel; TOCTOU on max_channels; ~50 lines of abandoned
deliberation in poll_write; cargo doc 4 warnings; spec docs
inconsistent post-047.
Includes a 6-unit remediation plan sequenced by dependency, with
acceptance gates. Units 1/2/4 need no spec decisions; Units 3 and 5
each need one written ADR decision first (ADR-047 §4 env-resolution;
§5 channel-id adoption). The overarching acceptance gate is the
end-to-end ChannelClient ↔ ChannelsAdapter test whose absence let
both commits land green.
Two corrections to the original reviews noted in the verification log:
- C-17 (lost EOF sentinel): Sub Review B overstated "the pump does not
write an EOF chunk" — the pump does write EOF when it receives the
sentinel; the real bug is narrower (sentinel lost on full buffer →
pump exits on recv→None without writing EOF).
- Warning count: main review said 5 cargo doc warnings; this pass sees
4 (2× register_openable, 1× default_policy, 1× env module/macro).
Verification:
- cargo test → 432 passed (no source changed; docs-only commit)
- cargo clippy --all-targets -- -D warnings → clean
- cargo fmt --check → clean
- cargo doc --no-deps → 4 warnings (documented as C-09; not addressed
here — fixing them requires the register_openable method to exist,
which is Unit 3 of the remediation plan)
glm-5.3-flash
glm-5.2
deepseek-v4-pro