refactor: unify the three skip filters and make one-shot narrowing structural - #738
Merged
Merged
Conversation
…ructural Closes the remaining one-shot/decoder follow-ups in plans/claude-md-knowledge-graph.md. TickDecoder::MESSAGE_TYPE becomes RESPONSE_MESSAGE_IDS and the tick driver filters with the same is_undeclared helper as the two subscription drivers — one skip decision, not three. A tick type declares one entry; the arity was the only difference. New request_helpers::expect_proto(expected, decode_proto) pairs the message type a one-shot asked for with its payload decoder. Narrowing was opt-in: each domain hand-wrote a decode_*_message wrapper for it and 24 sites had none, decoding whatever arrived on the shared channel. 44 call sites now narrow by construction; 30 decoder functions deleted (8 dispatchers, 22 require_proto wrappers). The two wsh wrappers stay — dual-use with a StreamDecoder. Converts the last seven hand-rolled folds: head_timestamp, histogram_data, market_depth_exchanges, historical_schedule (all sync+async) plus the four option-computation sites via a new fold_one_shot_mut that fold_one_shot delegates to. Only historical_data's fetch stays hand-rolled — it reads two frames. Behaviour: those four APIs retry a connection reset 3 times instead of unboundedly (async head_timestamp recursed, the others looped), and sync/async now agree on what a closed stream means. Two test fixtures were carrying wrong message-type prefixes (MarketRule 87 vs 93, news providers a non-numeric literal) — invisible until narrowing read the field no assertion depended on.
…issed /simplify findings. The sweep had missed wsh (4 sites) and next_valid_order_id (2), so the "every one-shot narrows" claim was false while they existed. next_valid_order_id is the instructive one: it reads the shared RequestIds channel through fold_one_shot with a bare decoder — adopting the fold helper reads as adopting the convention, and isn't. Nothing gated the (IncomingMessages, decoder) pair. A review mutated three sites to mismatched pairs and only incidental round-trip tests failed. Adds test_expect_proto_sites_match_the_roster: every site scraped from src/, checked against a PAIRS roster both directions, each pair required exactly twice so sync and async cannot drift. The same mutation now fails by name. Deletes 22 tests that were retargeted onto expect_proto in the previous commit. In each the decoder argument is never invoked — expect_type or require_proto fails first — so they asserted only the combinator's own behaviour and would pass with |_| Ok(()). Three canonical tests plus the roster replace them. Reverts one thing from the previous commit: removing expect_type from the two single-type StreamDecoder::decode impls. A decoder with no match has no `_ =>` arm, so expect_type IS its backstop — test_response_message_ids_match_decode_arms went red immediately. The misleading doc comment that invited the removal was the actual defect. Adds docs/rules/wire/one-shot-narrowing.md — the convention had shipped as a doc comment on a pub(crate) fn, which rules-check cannot see. Records on proto-only-decoding that TickDecoder got the const's name but not its gate, with the follow-up to close it. expect_type's error now names the expected type; it was the only runtime signal for a mispairing and withheld the identifying fact.
wboayue
added a commit
that referenced
this pull request
Aug 8, 2026
… consts (#739) #738 extended RESPONSE_MESSAGE_IDS to TickDecoder so all three drivers share one skip filter, but the gate did not follow: the roster collector matched `impl StreamDecoder<` only, leaving the three tick consts unchecked in both directions. Reproduced first — TickDecoder<TickBidAsk> declaring [HistoricalTickBidAsk, UserInfo] passed all 1364 sync tests. The precondition was the job. With no backstop, all three decode impls dive straight into require_proto() and answer UnexpectedWireFormat to every probe, so the check would have read them as handling all 88 scanned discriminants and asserted nothing. Each now narrows with expect_type first, the single-type form scanner and both wsh impls already use. #738 learned the same fact from the opposite end when it deleted two of those narrows as redundant. The two traits share check_decoder, with the differing decode signatures erased by a closure at the two call sites. /simplify then found the roster length was a lie the test could not catch: it compared the tree count against a hand-declared constant and never inspected check_all, so bumping the constant while forgetting the check line passed green with that decoder never probed. The check_* calls tally themselves now, and both constants are gone. Also: one tree walk instead of one per trait, the walk itself deduplicated against one_shot_pairing_tests.rs into test_utils::source_scan::visit_production_sources, and check_decoder brought to the three-parameter budget. The rule node records why the const stays a slice and why the narrow stays a hardcoded literal — deriving it from Self::RESPONSE_MESSAGE_IDS would make the cross-check circular. No user-visible change: the driver filters on the const before calling decode, so the new narrow is unreachable in production.
wboayue
added a commit
that referenced
this pull request
Aug 8, 2026
23 decoder tests spelled the same assertion four ways — expect_err plus matches!, unwrap_err plus matches!, a match with panic!, and three different panic messages. #731 is the evidence: renaming one variant touched every one of them and produced most of its test-side diff. assert_rejects_text_framing also closes a blind spot a plain extraction would have preserved. require_proto rejects the framing without reading the message type, so a fixture claiming the wrong id passes anyway — the class #738 caught four times by accident. The helper checks the frame's leading discriminant against the type the caller names, and a mutation (OrderStatus fixture framed as 4) fails by name. connection's handshake reader still takes &mut, so it passes a cloning closure; the divergence is one visible line instead of a fourth spelling.
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
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.
Closes the remaining one-shot/decoder follow-ups in
plans/claude-md-knowledge-graph.md.Two of the five items were already shipped in #736 (
expect_typeand the seven dispatchers) — the follow-up list itself had gone stale, which the plan now records as its own instance of the rot the migration was about.Two commits: the conversion, then
/simplify, which found real holes in the first one. Both are described below.Item 1 — unify the third driver
TickDecoder::MESSAGE_TYPEbecomesRESPONSE_MESSAGE_IDS: &'static [IncomingMessages], andtick.rs::classifyfilters with the sameis_undeclaredhelper the sync and async subscription drivers use. One skip decision instead of three.Caveat, recorded rather than papered over:
collect_stream_decoder_implsmatchesimpl StreamDecoder<only, so the threeTickDecoderconsts got the name but not the gate. Setting one to&[HistoricalTickBidAsk, UserInfo]passes all 1384 sync tests. The node says so out loud and the fix is a follow-up —TickDecoder::decodeneeds a backstop before the probe can distinguish "arm exists".Items 2/3 residue + item 5 —
expect_protorequest_helpers::expect_proto(expected, decode_proto)pairs the message type a one-shot asked for with the decoder for its payload, returning theimpl Fn(&ResponseMessage)the helpers already take. A combinator rather than a fourth parameter: those helpers already carry six arguments against a budget of three.Narrowing was opt-in — each domain hand-wrote a
decode_*_messagewrapper, and the sites without one decoded whatever arrived on the shared channel. The original follow-up put that at four sites; it is 26, over 13 decoders.20 replaced a wrapper, 26 replaced nothing, 4 replaced an inline
if message.message_type() == ..guard. 30 decoder functions deleted.The four option-computation sites the plan called "genuinely blocked" use a new
fold_one_shot_mutthatfold_one_shotdelegates to, so theSome(Err)/Nonedisposition stays one decision.Item 4 — the last hand-rolled folds
head_timestamp,histogram_data,market_depth_exchanges,historical_schedule, sync and async.historical_data's fetch stays hand-rolled and should: it reads two frames.Behaviour change. All four retry a connection reset at most 3 times instead of unboundedly — async
head_timestamprecursed on a closed stream, the other three looped. Sync and async now also agree on what a closed stream means; they disagreed before. Changelog entry under## [Unreleased].What
/simplifychangedFour review agents (reuse, simplification, efficiency, altitude). Efficiency found no regression — the combinator is a 4-byte stack value built once per call, not per retry — and confirmed two improvements (a
Box::pinremoved per retry, six unbounded spins capped). The other three found this:The sweep had holes. WSH (4 sites) and
next_valid_order_id(2) were never converted, so the "every one-shot narrows" claim was false while they existed.next_valid_order_idis the instructive one: it reads the sharedRequestIdschannel throughfold_one_shotwith a bare decoder. Adopting the fold helper reads as adopting the convention and isn't the same thing.Nothing gated the pair.
expectedanddecodeare unrelated to the compiler, so a mispairing feeds one message's bytes to another's prost type. The altitude agent mutated three sites and only incidental round-trip tests failed. Addedtest_expect_proto_sites_match_the_roster(src/common/one_shot_pairing_tests.rs): every site scraped fromsrc/, checked against aPAIRSroster in both directions, each pair required exactly twice so sync and async cannot drift. The same mutation now fails by name.22 tests deleted. In the tests the first commit retargeted onto
expect_proto, the decoder argument is never invoked —expect_typeorrequire_protofails first — so they asserted only the combinator's own behaviour and would pass with|_| Ok(()). Three canonical tests plus the roster replace them.One revert, and it is the most useful thing in the PR. Three of the four agents independently said the
expect_typeinside the two single-typeStreamDecoder::decodeimpls was a redundant third copy ofRESPONSE_MESSAGE_IDS, quoting the newexpect_protodoc comment. Removing them turnedtest_response_message_ids_match_decode_armsred within one run: a decoder with no match has no_ =>arm, soexpect_typeis its backstop, and without it the probe reads the decoder as claiming an arm for every message type includingShutdown. The doc comment that invited the removal was the actual defect. Recorded on the node as a counter-example — convergent agreement across independent reviewers is not evidence; the gate is.Docs. New node
docs/rules/wire/one-shot-narrowing.mdplus its index line — the convention had shipped as a doc comment on apub(crate)fn, whichjust rules-checkcannot see because it validates structure, not coverage.expect_type's error now names the expected type; it was the only runtime signal for a mispairing and withheld the identifying fact.And a count I got wrong. The plan bullet cited
grep -v _tests, which returns 56, not the 44 it claimed — four test files are namedtests.rswith no underscore. Written inside the bullet arguing that a follow-up list is an ungated completeness claim. Corrected, with the correct filter.Four fixtures that had been lying
Narrowing reads the message-type prefix, which no assertion previously depended on:
MarketRulebuilt with id 87 (it is 93), news providers led with the literal"newsProviders"(parses as no discriminant), market-depth-exchanges 71 (it is 80). All passed for as long as they existed.Gate
cargo fmt; clippy ×3; rustdoc ×3;just test(all three legs);cargo build --examples×2; both integration crates;just rules-check(32 nodes).Deferred as restructuring
Widening the one-shot helpers to
&mutto deletefold_one_shot_mutand give the option-computation sites retry; aProtoPayloadtrait that would retire thePAIRSroster entirely; gating the threeTickDecoderconsts; deciding which one-shots should retry at all. All four are written up inplans/claude-md-knowledge-graph.md.