alkgit/docs/reviews/005-implementation-post-wave-4-general-review.md

status, last_updated, reviewed_artifacts, tool, reviewer, scope_note

status	last_updated	reviewed_artifacts	tool	reviewer	scope_note
open	2026-10-23 (opened + reviewed in one session: the post-wave-4 general code-quality review, the review-004 precedent. The inline findings were fixed and stamped in that session — D-1 (the wire reader's malformed-frame coverage gap) and D-2 (the lib.rs crate-root doc rewrite). Recorded-and-open: R-1 (the session.rs split — the recommended head-of-wave-5 refactor task), R-2..R-4 (duplication judged keep/don't-share with rationale), R-5 (the residual thin arms — input to wave 5's fuzz inventory). The findings volume was within the inline bar; no remediation wave. The wave-5 decomposition reads this review.	src/session.rs, src/ops.rs, src/authorize.rs (the wave-4 surface, read in full) src/duplex.rs, src/stateless.rs, src/preamble.rs, src/lib.rs (the god-file candidates + the doc-drift instance, read in full) Cargo.toml (the root manifest — the wave-4 dependency additions, the feature graph) the seam/split measurements + re-export/allow/TODO scans across all of src/ and tests/ (review-004 read every module in full; the wave-4 delta plus targeted re-reads are what this review adds)	manual source read + cargo test / clippy (both feature sets) / fmt / doc + cargo-llvm-cov per-module + measured impl/test split + re-export consumption scan	general implementation review (code quality only — god-files, duplication, coverage, dead code, doc drift) — the wave-4 gate's ADR-compliance verdict stands and is not re-litigated here	the protocol core as landed through wave 4, the review-004 precedent's dimensions (this review does not repeat review-004's security/correctness/budget passes; the wave-4 modules were built on the fixed surface it stamped). The substrate crate is still the manifest-only stub — out of scope. Findings the wave-5 charter already owns (OQ-19 wire validations, fuzzing, the size-bomb bound, release readiness) are named, not re-derived.

Review 005 — The Protocol Core as of Wave 4 (Post-Landing General Code-Quality Review)

Purpose

General review of the core after wave 4 (consumer half + ops) landed — the charter's five dimensions, the ones the wave gates do not read (the gates were task-compliance passes against pinned ADR text; this is the other kind):

1. God-files — measured impl/test split, seam-quality assessment per candidate, split/no-split recommendations.
2. Duplication — the known instances plus a fresh sweep (including the session push client's deliberate ADR-023 mirror of the server grammar: which parse helpers could share without coupling the client to the server's internals).
3. Coverage — per-ingress-surface numbers as evidence for wave 5's fuzz-target inventory; a disposition for coverage itself (verification-list member vs review-time evidence).
4. Dead code / unused surface — unused items, re-exports nobody consumes, feature-gated paths never exercised (sha256 especially).
5. Doc drift — the known lib.rs instance and the rest of src/.

Method note: the wave-4 delta (session.rs, ops.rs, authorize.rs — everything else was read in full at review 004) read in full; the god-file candidates assessed file-wide; the scans (allow/TODO/unwrap/ re-export consumption/doc phrases) run across src/ and tests/.

Verification Baseline

On the tree as reviewed, before the inline fixes:

• cargo test --workspace: green — 358 tests (210 unit + 10 suites). cargo test -p alkgit: same surface, no substrate dependency. cargo test --all-features: green — 361 tests (the sha256 rider paths).
• cargo clippy --all-targets -- -D warnings and cargo clippy --all-features --all-targets -- -D warnings: clean. cargo fmt --check: clean. cargo doc --no-deps: zero warnings.
• git status clean at fec9649 (the task-insertion commit), the wave-4 gate's tree verbatim.
• Zero TODO/FIXME/HACK markers in src/ and tests/ (the only "unimplemented" hits are doc prose about declined capabilities); zero unwrap()/expect()/panic! outside #[cfg(test)]; #![forbid(unsafe_code)] holds.

After the inline fixes (stamped in Part A): the full command set re-ran green — cargo test 360 tests (two preamble wire-reader tests added), --all-features green, both clippies clean, fmt clean, cargo doc --no-deps clean.

Summary Statistics

Severity	Count	IDs
Hygiene/coverage, fixed inline	2	D-1, D-2
Booked — head-of-wave-5 refactor task	1	R-1
Duplication, judged keep/don't-share	3	R-2, R-3, R-4
Booked — wave-5 fuzz-inventory input	1	R-5

No blocking issues; no remediation wave (the findings volume sat inside the inline bar — the review-004 rule). The wave-5 decomposition reads Part B: R-1 is the recommended first task of the wave (refactor → fuzz → docs → publish is the charter's natural order — refactoring after fuzzing would double the fix work and invalidate corpus mapping), and R-5's table is the fuzz-target inventory's starting data. Nothing here blocks wave 5.

---

Part A — Findings fixed inline

D-1 [hygiene, coverage] — The wire reader's malformed-frame arms were the thinnest ingress surface: read_request_line's fail-closed branches had zero byte-level tests

Fixed inline (2026-10-23, this session): two tokio unit tests landed in src/preamble.rs — the full frame reads and parses through the reader (the fill loop exercised with dribbled writes, the header split across reads) and a byte-level malformed-frame table (truncated header, non-hex length header, below-minimum length, the 4096-byte cap, truncated payload, non-UTF-8 payload — each asserted to fail closed with the typed Malformed error naming the shape). The clean byte-less close reads as Ok(None) (the quiet session end) in the same table's positive test.

Files/measurement: src/preamble.rs (read_request_line, lines 172–226 — the pre-fix arms at 181–186, 198, 207–215, 223–224 had no direct test; the parse-shape tests covered parse_request_line, not the byte reader around it).

Problem: the request-line grammar is ADR-016 §2's pinned wire format on a published ALPN and the direct path's first frame — and it was the crate's thinnest ingress surface by coverage (85.06% lines, the only module below 90% besides the client-side session). The gap was exactly where an attacker lives: the byte-level malformed-frame arms (header truncation, non-hex, the cap breach) were reachable only through the parse tests one layer down. Wave 5 fuzzes this surface; a fuzz target started from an untested reader can mask failures that ought to assert. Not a correctness bug — every arm fails closed correctly — but the ingress the charter names ("the git-daemon request-line grammar") should not be the coverage floor at the pre-release review.

Fix (landed): the two tests as above; per-module coverage after: preamble.rs 85.06% → 96.06% lines (the one remaining uncovered fn is the io read-error arm at 190–193 — not drivable over an in-memory duplex without a custom failing reader; recorded in R-5). Wave 5's fuzz inventory starts from a tested reader.

D-2 [docs] — The crate-root doc was a ~90-line "joined by…" changelog narrative: eleven waves' worth of landing-order sentences in a row

Fixed inline (2026-10-23, this session): src/lib.rs's crate-root doc (pre-fix lines 1–86) rewritten into module-oriented docs — six short sections (the crate's what/why + the substrate-free posture; the backend seam; the session substrates and serving entries; the protocol machines; the transport + ops; the consumer half), each naming its pub types with working intra-doc links and the pinning ADRs. The per-item contract stamps (which the old narrative's sentence-chains duplicated from the module docs) stay on the items where they belong.

Files: src/lib.rs (the doc rewrite; no code change — the re-export list and module tree untouched, zero public-API change).

Problem: each wave appended a sentence to the crate doc with a "—joined by—" connector, per the review's charter the known instance ("it needs a rewrite into module-oriented docs regardless"). The result read as a changelog, not a map: the crate's actual structure (five-trait seam / two substrates / serving machines / transport / consumer half) was invisible below the landing order, and the stamp detail repeated what the module docs already carry.

The rest of the drift scan: a phrase sweep (is landing / has begun / lands next / joined by / will land) across src/ hit only lib.rs; every other module doc describes the landed surface accurately (the wave-4 gate already fixed the one stale sentence session.rs carried). No other doc action.

---

Part B — Recorded findings (not fixed: judged or booked)

R-1 [booked — head-of-wave-5 refactor task] — session.rs is a three-workstream god file with clean seams: split it (and fold in the in-file helper dedups)

Measurement (impl lines exclude #[cfg(test)]; measured 2026-10-23, pre-fix):

File	Impl	Tests	Verdict (this review)
src/session.rs	1976	515	split — three workstreams, clean seams
src/duplex.rs	1473	585	no split (mechanically safe; marginal benefit)
src/stateless.rs	1146	673	no split (same posture)
src/push.rs	931	342	no split (one cohesive state machine)
src/ops.rs	643	1070	no split (62% tests; spec/handler pairs)
all others	≤530	—	fine

The session.rs split (the recommendation). session.rs carries three wave-4 workstreams that landed as three separate tasks: the construction+handshake (the GitTransport alias, SessionError, HandshakeResponse, both constructors, the terminal-caching accessors — session.rs:123–594 plus the shared AdvertisementBounds/ drain_advertisement helpers at 1210–1276), the fetch client (ls_refs/fetch + FetchRound/FetchRoundReport + prepare_ls_refs_args/parse_ref_line/pump_pack/command_features — 680–960 and 1278–1499), and the push client (push_advertisement/ push + the whole push grammar helper family PushAdvertisedRef/ PushSendOptions/PushReport/PushObjectFormat/ push_object_format/push_capabilities_text/parse_v0_advertisement /stream_caller_pack/read_push_report/framed_report_lines/ parse_push_report — 982–1207 and 1501–1975). The evidence that the seams are clean:

• every fetch-only helper is called only from ls_refs/fetch/ fetch_once; every push-only helper only from the push ops; nothing shared beyond the construction+handshake surface and the two shared drain helpers;
• the push helpers are pure functions (no session state, no transport types in their signatures beyond the error enum);
• an inherent-impl split needs no trait surgery: impl GitSession blocks compose freely across sibling modules, and the #[async_trait(?Send)]-shaped one-operation-at-a-time bound is per-method, unaffected.

Shape: flat siblings per convention 14 (one module per file, literally — no session/ directory, no convention amendment): session.rs keeps construction+handshake+the shared helpers, session_fetch.rs and session_push.rs take the two client workstreams with their helpers and their unit rows. lib.rs re-exports the same names from the (unchanged) public paths — zero public-API change by construction, so the OQ-03 freeze inventory cannot be disturbed. Risk: low. The task should fold in the in-file helper dedups found by this review:

• pump_pack and read_push_report share the identical fill-buf/consume/max_pack_size-budget drain loop and the verbatim band-error progress-handler closure (session.rs:1444–1494 vs 1825–1873) — one parameterized band-drain helper serves both;
• drain_advertisement and ls_refs_once's readline loop share the verbatim err-line downcast block and the same byte/ref budget-check shape (session.rs:737–772 vs 1232–1276) — the same bounded-drain helper class.

The no-split verdicts (recorded with rationale per the charter). duplex.rs and stateless.rs each carry two concerns (substrate machinery vs serving orchestration) and the seam is clean — the split is mechanically safe on both (serving consumes only pub machinery APIs; the serving halves are ~420/~275 impl lines). The recommendation is still no split now: any split leaves the machinery half as the largest file in the crate (duplex ~1050, stateless ~870 — bigger than any session.rs survivor), so the size win is marginal; the substrate narrative is genuinely cohesive (the module docs document the stop-delimiter/error-break machinery rules and the serving entries' behavior as one story per ADR-005 section); and the cost is rewriting that dual narrative twice. Revisit trigger recorded: if wave 5's OQ-19 wire-validation work or the fuzz harnesses grow the serving entries, re-read this verdict at the wave-5 gate. push.rs (one state machine + its grammar, cohesive — prior read confirmed) and ops.rs (62% tests; four parallel spec/handler pairs — prior read confirmed) do not split.

R-2 [judged keep] — wall_clock_breach exists twice by design

Files: src/duplex.rs:1053–1058, src/stateless.rs:1055–1060.

Judgment: keep. The two functions are four lines each and their messages are intentionally different wire-adjacent text — duplex's is the session breach ("the session exceeded … the session closes") because the shape is error band + session close; stateless's is the per-request breach ("the request exceeded … 503-mapped at the door"). A shared helper taking a label parameter would save two lines per site and cost one indirection in the exact place the tests pin the budget-naming text. Two call sites, distinct phrasings, no drift surface — sharing buys less than it obscures.

R-3 [judged don't-share, with one cheap exception folded into R-1/R-5] — the four request readers repeat the same classification skeleton; the two serving entries repeat thin glue

Files/measurement: src/duplex.rs read_request (360–518) and read_push_request (639–793); src/stateless.rs read_post_receive_pack (451–649) and read_post_upload_pack (679–871); each pair shares the packet-classification enum + terminal/budget enforcement skeleton, and across the two substrates the clean-close/EOF classification recurs (the STOP_AT_FLUSH constant is defined twice, duplex.rs:118 / stateless.rs:89). The serving-entry glue likewise repeats: drive_ls_refs (duplex 1080–1096) vs drive_one_post's ls-refs arm (stateless 1001–1008) run the same parse→list_refs→map→serve sequence, and the unknown-command decline message is byte-identical (duplex 1146–1148 / stateless 1025–1027).

Judgment: don't-share the reader skeletons. Each loop is ~100 lines, locally coherent, and its grammar is capture-pinned per service per substrate — a shared abstraction would parameterize across the very dimensions that differ (V2 command/delim shapes vs V0 caps-NUL shapes; duplex stop-delimiter discipline vs stateless section-skipping; clean-close classifications that are deliberately not identical). The parameter soup would make the pinned grammars harder to diff against the captures, which is the actual review workflow here. Two cheap exceptions, recorded for the refactor task: the duplicated STOP_AT_FLUSH const can move to one home (stateless already imports the duplex escape types — crate::duplex::{CountingRead, RawTail, Request} — so one pub(crate) const costs nothing), and the verbatim unknown-command decline string can share a const. No functional change implied by either; carry them in R-1's task or wave-5's housekeeping, not as separate work.

R-4 [judged keep] — the session push client's grammar stays a deliberate mirror of the server's (nothing worth sharing)

Files: src/session.rs (parse_v0_advertisement, push_object_format, push_capabilities_text, parse_push_report, framed_report_lines) mirroring src/push.rs/src/v0.rs's server-side shapes.

Judgment: keep the mirror whole — the charter's question ("which parse helpers could share without coupling the client to the server's internals") answers to none of them. The two sides are direction-opposed (write vs parse) over the same wire shapes; a shared module would be constants-only, and the one sharing that already pays for itself exists (UNPACKER_ERROR_REASON / ATOMIC_PUSH_FAILURE_REASON imported from push.rs — the peer's reason text must match ours verbatim by semantic necessity). Beyond that, the honest-duplication posture ADR-023 records is the discipline itself: the client parse is pinned by push-captures.md independently of the server's writer, so any server-side grammar change forces a conscious capture re-check on both sides rather than free desync-by-refactor. Pre-publish (OQ-03 freeze), the captures are still the normative pins; re-read this verdict only if the client grammar needs a change the captures don't explain.

R-5 [booked — wave-5 fuzz-target inventory input] — the per-ingress coverage table and the residual thin arms

Files/measurement: cargo llvm-cov --workspace per-module (lines; post-D-1-fix; the fuzz-relevant surface per wave-5's charter in brackets):

Ingress surface	Module	Lines	Residual missed (the fuzz targets' starting data)
the git-daemon request-line grammar [preamble.rs]	src/preamble.rs	96.06% (pre-fix 85.06)	the io read-error arm (:190–193, one fn — undrivable over in-memory io; fuzz reaches it)
pkt-line framing parsers [duplex + stateless machinery]	src/duplex.rs, src/stateless.rs	90.94% / 92.00%	the defensive arms (machinery-misuse refusals, terminal re-reports; same shape review-004 named)
negotiation ingress [fetch.rs parse]	src/fetch.rs	92.18%	parse-error arms + the include-tag-shaped declines
push request grammar [push.rs]	src/push.rs	93.04%	malformed-command + zero-outcome defensive arms
repo-id grammar [registry.rs]	src/registry.rs	100%	—
open-op params [channels.rs establisher]	src/channels.rs	94.41%	the defense-in-depth plan: None fallback (:335–354) + the accept_bi failure arm (:313) — documented postures, fuzz-relevant
(client direction, not a server ingress)	src/session.rs	85.22%	the *_once timeout/error arms — the client's budget machinery; candidate for a wave-5 hostile-server test, not a fuzz target
(context) ops/authorize/refname/v2/v0	—	97–100%	—

Judgment (the disposition the charter asks for): coverage stays review-time evidence, not a standing verification-list member. The gates' test rows already pin behavior; both general reviews (004, 005) ran llvm-cov and acted on it immediately (S-1/S-2/C-1 there, D-1 here) — the per-gate cadence is what makes the number useful, and the ~1–2 minute cycle on every commit buys trend data the per-commit loop never acts on. Concretely: run it at every wave gate and at the wave-5 release gate; wave 5's fuzzing is the structural answer to thin ingresses, and the fuzz corpus gate (charter) subsumes the per-commit interest. The table above rides into wave 5's decomposition record as the fuzz-target inventory's first column.

N-1 [observation] — the #[allow] inventory is small, justified, and contains no dead code

Seven #[allow(clippy::too_many_arguments)] sites (serve_duplex, serve_duplex_resolved, drive_command, serve_stateless_post, drive_one_post, serve_receive_pack, register_openable's establisher closure) — each is a pinned input tuple the docs explicitly refuse to flatten ("the seven inputs are the dispatch surface both machines share, not a shape to flatten" and kin). No #[allow(dead_code)], no #[allow(unused…)] anywhere; the -D warnings clippy over both feature sets proves no unused internal item exists. Recorded so the next reviewer skips the inventory.

---

Part C — Verified sound (the pass, stated so the next reviewer skips it)

• The god-file table (Dimension 1) is measured and dispositioned (R-1); no candidate beyond the five in the charter's table comes close (the runner-up is src/fetch.rs at 530 impl lines).
• The duplication sweep (Dimension 2): beyond R-2/R-3/R-4 and the R-1 fold-ins, the one remaining repeat is test-side (pkt() / read_pkts helpers duplicated in the duplex and stateless test modules) — self-contained per-module test seams, fine as-is. The wave-4 modules are themselves duplication-free (ops.rs's four handlers share exactly the collapse/error-shape helpers, and authorize.rs adds a pure function rather than copying tiers).
• Dead surface (Dimension 4): the re-export consumption scan found every re-exported type consumed in-tree or by the integration suites; the only zero-direct-consumer re-exports are the config constants (MAX_*/PACKLESS_PEEK/SESSION_WALL_CLOCK, MAX_REQUEST_LINE_BYTES, PACK_SOURCE_*, REPO_ID_MAX_BYTES, the refname constants, PUSH_SERVED_CAPABILITIES, REPO_RESOURCE_TYPE, the OP_GIT_REPO_* names) — the budget-model's and wire-schema's documented embedder knobs, each consumed inside its defining module by the machinery that enforces it. Intentional surface, not dead.
• The sha256 feature matrix rides: eight cfg(feature = "sha256") sites (three impl-side — the advertisement/parse sizes in v0/v2/ fetch — five test-side), all compiled and run by cargo test --all-features (361 tests green); no never-exercised feature path exists in the core.
• Doc honesty (Dimension 5): post-D-2, cargo doc --no-deps is warning-free and the crate doc is a structural map; the manifest comments match the landed wave-4 posture (the R-1 pin note from review 004's disposition reads current).

Part D — Recommended disposition order

For the wave-5 decomposition (nothing here blocks it; R-1 is the recommended first task of the wave per the charter's refactor → fuzz → docs → publish order):

1. R-1 — the session.rs split task (+ the two in-file helper dedups, + the R-3 cheap constants): run the full verification set after it; the split's unit rows move mechanically.
2. R-5 — the inventory feeds wave 5's fuzz-target list verbatim (the D-1 tests make the request-line reader fuzz-ready from a tested base); the residual arms are the targets' assertions to reach.
3. R-2/R-3/R-4 — no action; the verdicts are recorded for the freeze inventory and any future re-read.
4. Coverage disposition (from R-5): review-time evidence, per-gate; wave 5's release gate re-runs it as part of the publish checklist.
5. D-1/D-2 — landed and stamped; wave 5 composes against the fixed surface.