alkgit/docs/reviews/004-implementation-post-wave-3-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 (R-2's disposition amended: the bound decision + the doubles' guard re-booked to wave 5, pre-publish — the original wave-8 booking would have decided the shape after the OQ-03 freeze of `Limits`. Opened 2026-10-22: post-wave-3 general code review. The minor findings were fixed inline in that session and stamped below (S-1, S-2, C-1, D-1). Recorded-and-open: R-1 (dead-until-wave-4 deps, judged keep), R-2 (the ingestion size-bomb class — now split: wave 5 decides + guards, wave 8 implements), N-1/N-2 (observations). The findings volume was within the inline bar; the review is the record, and the wave-4 decomposition reads it.	src/ (all 19 modules — adapter, authorize, backend, channels, duplex, error, fetch, limits, pack_source, preamble, push, refname, registry, seam, stateless, substrate, v0, v2, lib) tests/ (the eight machine/serving suites + common's doubles and fixture builders — spot-read in full) Cargo.toml (root + substrate manifests, feature graph)	manual source read (every src/ module in full) + cargo test / clippy / fmt / doc + cargo-llvm-cov	general implementation review (correctness + security + bounded-resources + code smells) — the wave 1–3 review gates were each targeted at that wave's ADR/capture criteria; this one is the post-wave-3 whole-surface pass, following alkblobs review 001's template	the protocol core as landed through wave 3. The substrate crate is the wave-1 scaffold stub (manifest only — out of scope; its dead deps are R-2's sibling note). Findings that belong to wave 4 (GitSession/ops), wave 5 (wire validations/fuzzing/release), or wave 8 (the real ingestion impl) are named, not re-litigated — the waves 2/3 gate verdicts and their dispositions stand.

Review 004 — The Protocol Core as of Wave 3 (Post-Landing General Review)

Purpose

General review of the core after wave 3 (receive-pack) landed, asked broadly rather than at a wave gate:

1. Security — the wire-facing surface (ACL fidelity, the no-existence-oracle collapse, honest capability advertisement, authenticated-push posture) and the untrusted-input paths (parsed requests, option sections, pack streams).
2. Correctness — state-machine hazards across the two substrates (the raw-tail handoff, the peek's disciplines, the CAS/report composition, the relay lifecycles) that a per-ADR gate's targeted checklist would not surface.
3. Bounded resources — the ADR-009 budget model's actual enforcement coverage: every place a session, request, stream, or loop could grow without a budget.
4. Code smells — panics/unwrap, god files, dead surface, doc drift, dependency hygiene.

Method note: every src/ module read in full; the tests read for the doubles (the machines' contract evidence) and the serving shapes; the inline fixes were landed and verified in the same session (the frontmatter's last_updated records it).

Verification Baseline

On the reviewed tree (post-inline-fix; the fixes are stamped in Part A so the pre-fix shape is recoverable from their text):

• cargo test --workspace: green — 287 tests across 11 suites (283 pre-review + the 4 regression tests the fixes added).
• cargo test --all-features: green (the sha256 rider compile paths).
• cargo clippy --all-targets -- -D warnings: clean. cargo fmt --check: clean. cargo doc --no-deps: zero warnings.
• cargo llvm-cov --workspace: 93.88% lines / 96.68% functions / 93.98% regions. Per-module: authorize/limits/refname/registry/seam at 100% lines; the substrates (duplex 90.9%, stateless 92.0%) and push (93.0%) carry the residue — concentrated in the defensive arms (machinery-misuse refusals, the stateless PacketLineRef::Flush inline-flush defense, preamble error paths at 85%), i.e. the same shape alkblobs review 001 named, but here the load-bearing paths are all covered and the machines' e2e suites are the tree's core.

Zero TODO/FIXME/HACK markers anywhere in src/ and tests/; zero unwrap()/expect()/panic! in library code (every hit is inside #[cfg(test)]); #![forbid(unsafe_code)] holds on both crates. The wave discipline kept the codebase honest in the classic ways.

Summary Statistics

Severity	Count	IDs
Security/DoS, fixed inline	2	S-1, S-2
Correctness, fixed inline	1	C-1
Hygiene, fixed inline	1	D-1
Recorded — judged keep/booked	2	R-1, R-2
Observations	2	N-1, N-2

No blocking issues. The recorded items below are the ones the wave-4 decomposition should read (R-1's disposition; R-2's re-booking — amended 2026-10-23 to wave 5, pre-publish) and wave-8's decomposition must inherit (R-2's implementation half); the observations are hardening notes for wave 5/future gates.

---

Part A — Findings fixed inline

S-1 [security/DoS] — The duplex request readers had no request-byte budget: the accumulated request grew for the whole 30-minute session wall clock

Fixed inline (2026-10-22, this session): read_request and read_push_request now carry the stateless enforcement's twin — a per-request consumed-delta check against Limits::max_request_body (fail-closed, terminal, Protocol). Two regression tests landed.

Files: src/duplex.rs (the readers' loops had no budget check; contrast StatelessRequest::read_post's max_request_body enforcement — src/stateless.rs, which bounds the same accumulated args on the http side); Limits::max_request_body and MAX_REQUEST_BODY_BYTES documented as http-POST-only (src/limits.rs, src/fetch.rs's parse_fetch_args consuming the request length).

Problem: on the stateless substrate the request-body budget is real (64 MB default), but the duplex readers accumulated pre_lines/args with no byte budget at all — only session_wall_clock (default 30 min) bounded them. An authenticated client (fetch needs only read; push needs write) streaming tiny pkt-lines for the session's lifetime drove the accumulated Vec<String> to tens of GB on a fast link before the wall clock answered — a memory DoS on an always-on session surface. The machinery's own docs said "the request-body budget is the machinery's (read_post)" — the duplex twin simply did not exist.

Fix (landed): per-request delta check against Limits::max_request_body in both readers (budget captured before the request, saturating_sub over the consumed counter, terminal on breach) — semantically "each protocol request is bounded by the request-body budget on both substrates"; the http-only wording on the Limits field corrected. Tests: a fetch request and a push request over a 200-byte budget fail closed with the budget-naming terminal error; a 21-line request under a 4000-byte budget parses in full.

S-2 [security/DoS] — parse_fetch_args deduplicated wants/haves with a linear Vec::contains scan: O(n²) over the unbounded request length, checked against the haves cap only after the parse

Fixed inline (2026-10-22, this session): set-backed dedup (HashSet, first-occurrence order preserved), with the recorded rationale in place (convention 1's non-obvious-correctness carve-out).

Files: src/fetch.rs:165-215 (slot.contains(&oid) per want/have line; the max_haves_per_round cap applied in serve_fetch_round after the whole parse).

Problem: the request length bounds the parse input (S-1 closed that gate), but the parse itself was quadratic before the cap applied: a 64 MB body of have <oid> lines is ~2 M distinct haves → ~2×10¹² comparisons inside one round before max_haves_per_round was ever consulted — a CPU DoS by one POST, plus the duplicate-preservation semantics unchanged. The contract rows order-pin the dedup (first-occurrence), and the fix preserves that exactly while making the parse linear.

Fix (landed): HashSet-backed seen-sets for wants and haves, pushing in first-occurrence order; the existing order/duplicate rows (fetch_machines.rs / the unit rows) stay green unmodified.

C-1 [minor, correctness] — The stateless advertisement GET's one backend call was the session surface without any time budget

Fixed inline (2026-10-22, this session): the listing is now under Limits::session_wall_clock, mapped to the same typed breach error the POST entry emits; a regression test drives a hung listing. The UploadPack arm needs nothing (its emission is the static dump — no backend call).

Files: src/stateless.rs (serve_stateless_advertisement's ReceivePack arm called list_refs unbounded; contrast serve_duplex's timeout_at-bound phases and serve_stateless_post's wall-clocked drive_one_post).

Problem: both serving entries bounds their phases (the duplex entry wraps every phase in the session deadline; the POST entry wraps the invocation in the wall clock) — but the advertisement GET was the one entry whose single backend call ran with no budget anywhere. A hung backend call wedged the door's info/refs route for the door's whole route-lifetime (which, per the entry's own contract, the entry — not the door — should bound; the door maps statuses, it does not babysit the machine).

Fix (landed): tokio::time::timeout(session_wall_clock, ...) with the wall-clock breach error (the same wall_clock_breach shape the POST entry carries); hang test asserts the bound's promptness and the empty-sink fail-closed posture.

D-1 [minor, hygiene] — The unused direct tracing dependency

Fixed inline (2026-10-22, this session): removed from [dependencies].

Files: Cargo.toml (tracing = "0.1" — zero uses anywhere in src/ or tests/; cargo tree -i tracing confirmed only the direct edge).

Problem: the family precedent is recorded (alkblobs review 001, S-4's disposition): this crate carries no log/tracing dependency and none was added. A stale direct dependency both inflates the build and invites the next session to start interpolating raw errors into log lines — the exact shape S-4 there spent a wave removing.

Fix (landed): the dependency is gone; a future telemetry pass would be a deliberate feature, not a habit.

---

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

R-1 [booked, judged keep] — gix-protocol + gix-transport are dead manifest surface until wave 4

Files: Cargo.toml (gix-protocol 0.67, gix-transport 0.61 — no use anywhere in src/ or tests/ until wave 4's GitSession lands).

Judgment: keep — the two declarations are wave-4 pre-commitments with their pins and rationale recorded in the manifest's own comment (the ADR-017 note; the pins are the point, so wave 4 cannot silently drift the versions), and they are the only dead weight the lean-posture tolerates. The wave-4 decomposition should nevertheless confirm the pins against docs/research/gitoxide.md at its first commit — that is the disposition this finding asks for, not a removal. (Contrast D-1: tracing was dead and unclaimed by any wave.)

R-2 [booked on wave 8] — The ingestion's expansion path trusts the pack-declared decompressed size as an allocation: the size-bomb class, unguarded at the one place the pack data is inflated

Files: tests/common/mod.rs (inflate_entry: vec![0u8; entry.decompressed_size as usize] — the size is attacker-controlled pack-header data; the BudgetedReader/relay envelope caps the compressed stream at max_pack_size but nothing caps the expansion ratio); the wave-8 substrate impl (ADR-020 §2's decomposed shape — the task is written against this composition, so the hazard class transfers verbatim).

Problem: a hostile pack within the 64 MB wire budget can declare an entry whose decompressed size is arbitrarily large; a Vec allocation of the declared size is an allocation-failure abort (Rust's default), not an error path — a process-wide crash reachable from one authenticated push. No test drives it (the fixtures are trusted); the doubles' only unbounded-input surface is exactly this one.

The booking: wave 8's GitPackIngest impl decomposition must own the decompressed-size bound — the shape to decide is either a named Limits field (a per-entry/per-object expansion cap) or a fixed multiple of the budget (fsck-style object-size bound), with the inflate path reading incrementally instead of pre-allocating. The doubles' inflate_entry should take the same treatment when wave 8's task re-reads the composition (the doubles are test surface — the finding is not urgent as test code, which is why it is recorded, not fixed inline; the wire-facing version of this exact code is what wave 8 would otherwise have landed).

Recommended rider: name the bound choice in the wave-8 decomposition record (or as an OQ if the value needs a spec-level decision) so the size-bomb class cannot slip through the same window in the real impl.

Disposition amended (2026-10-23, at the wave-4 decomposition — operator review): the wave-8 booking above is split. The decision (the bound shape) and the doubles' guard re-book to wave 5, pre-publish: the original booking would have placed the shape decision after the OQ-03 freeze of Limits (a post-freeze Limits field is semver surface on a published crate — the freeze is exactly why the decision cannot wait for the wave that first needs the field), and the doubles are the reference ingest composition the wave-8 task is written against (this finding's own "transfers verbatim" note) — leaving them unguarded through the core's publish teaches the vulnerable shape. Wave 8 retains the implementation half: the real impl's incremental inflate under the wave-5-pinned cap, named in that wave's decomposition record. The recommended rider is superseded: the choice is named in the plan's wave-5 section, not deferred to a later decomposition record.

N-1 [observation] — The pipeline permit is held across the whole pack relay: a stalled reader pins one of the 8 permits until the wall clock

Files: src/fetch.rs (generate_packfile_section: the permit is acquired before the spawn_blocking generate; the generate task ends only when the sink's sends complete — which a stalled client's backpressured writer delays until the wall clock); src/push.rs (serve_receive_pack's header path mirrors it — the permit wraps prepare, whose reads pace against the client's uploads, though there the client is the staller's counterpart).

Problem: fetch requires only a read grant, the semaphore is server-wide and small (8), and done rounds are the permit's takers: N slow-reading fetch clients park N permits for up to 30 minutes each, starving push ingestion and every other generate/prepare. The resource is bounded (wall clock, the accounting stays exact — no leak), so this is not a bug; it is the availability posture worth a named decision: either accept it (the ADR-009 model says the permit protects blocking-pool concurrency, which is what it does here) or wave 5's hardening pass re-scopes the permit to the generation-only phase (release before the relay drain). Record in the affected wave's gate; no inline action.

N-2 [observation] — The substrate crate's scaffold manifest carries its wave-6..10 dependency set while stub-dead

Files: substrate/Cargo.toml (gix-pack/gix-fsck/gix-object/ gix-ref/tokio/parking_lot/thiserror/tracing — none usable until the composition's waves land).

Judgment: intentional and recorded (the stub's own docs say the manifest is the wave-1 scaffold's ship). Flagged only so wave 6's decomposition re-spects the pins against gitoxide.md the same way R-1 asks of wave 4 — and so the tracing line there gets the same decision as D-1 when the real code lands (the family's no-log-dep precedent applies to this crate too).

---

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

• The ACL/gate posture: the AuthorizedRepo marker is unskippable by type on every entry; resolve→authorize precedes any protocol byte (serve_duplex_resolved, the channels establisher, the advertisement GET's marker input); every denial collapses to the one generic wire shape with only the client-sent repo id in the message; authorize's monotonicity and the write-always- authenticated rule are pinned by the policy tests.
• Honest advertisement: the V2 capability list and the V0 served set are pinned constants with anti-drift test rows; push-options rides the gate; unknown/duplicate caps, shallow, and unadvertised object-formats fail closed at parse on both services.
• The budget model's fail-closed behavior (post-fix): request bytes (both substrates), the pack stream (wire-side relay + impl-side counting reader), negotiation rounds, haves per round, the advertisement ref cap (nothing emitted on breach), the wall clock (every serving surface), the pipeline permit, and the bounded channels (sideband sink, pack bridge — capacity 4, backpressure both ways).
• The raw-tail discipline: the no-byte-loss argument holds on both substrates (dribbled-chunk proofs), the escape's misuse refusals are machine-use errors, never wire shapes, and the packless/malformed classification matches ADR-020 §1's rules (i)–(iv) with tests per rule.
• The CAS semantics: stale = outcome not error; duplicate ref commands fail closed; name validation excludes from the CAS; the atomic abort maps verbatim; the zero-outcome slot fails closed (merge_cas_outcomes's unwrap_or_else — a CAS hole reports an error, never a silent ok).
• The error-break/stop-delimiter machinery classes: terminally cached and re-reported without transport re-entry (read-counter freeze tests); the reset discipline refuses rather than swallowing.
• The stateless framing facts: one-shot flush, no 0002, probe-200, capability-dump skip with the delim-aware split — all carried by the response type, not by handler discipline.

Part D — Recommended disposition order

For the wave-4 decomposition (nothing here blocks it):

1. R-1 disposition — confirm the gix-protocol/transport pins in the wave-4 first commit (hours; a checklist item, not a task).
2. R-2 booking — (amended 2026-10-23: the bound decision + the doubles' guard land in wave 5, pre-freeze; wave 8's decomposition record names the inherited cap — see the R-2 disposition note above.)
3. N-1 — read at the wave-5 gate with the OQ-19 wire validations; decide accept-vs-re-scope there.
4. N-2 — the wave-6 manifest read (rides R-1's posture).
5. S-1/S-2/C-1/D-1 — landed already; the wave-4 decomposition inherits the fixed surface, and its machines compose against the request-budget semantics as landed.