Files
alkgit/docs/reviews/006-implementation-post-wave-9-general-review.md
T
glm-5.3-flash 7bef60ca33 docs(reviews): review-006 — the post-wave-9 general implementation review
The pre-freeze code-quality review of the substrate composition (the
review-004/005 precedent's five dimensions plus the two
substrate-specific ones: engine-twin parity and assembly-shape/per-piece
parity), inserted between the wave-9 gate and wave 10's decomposition.

Inline-fixed in the review session (Part A):
- S-1: the pg registry's put/remove transactions touched git_refs
  without the {schema}\0git_refs advisory lock the refs CAS twins hold
  for their whole call — the F-S2 sibling that didn't get the fix; an
  orphan-row window under multi-instance co-tenancy. Fixed per the
  gate's own F-S2 pattern (both legs + docs).
- S-2: the live-pg silence-probe gate was module-private while the
  compose entry's live tests announce ungated on the same container;
  the gate promoted to live_harness and taken by all three.
- D-1: the substrate crate-root doc rewritten (the review-005 D-2
  analog — the wave-status changelog paragraphs dropped, the
  lfs/lfs_adapter map entries added).
- D-2..D-5: the VersionedRefTable epoch comment corrected (every green
  call bumps), a duplicated liveness seam comment removed, stale
  wave-7/9 narration replaced with landed module pointers, and the one
  library-side expect restructured (convention 2 clean).

Recorded (Part B, the wave-10 decomposition's inputs): R-1 the
postgres.rs placement split (recommendation), R-2 the impl helper
dedups, R-3 the test-harness consolidation, R-4 the mem_registry
disposal, R-5 the per-surface coverage table (review-time evidence;
F-w10-B1's input), R-6 the per-piece parity class F-w10-B2 absorbs,
R-7 the engine-less combo joins F-w10-B6's feature hygiene; N-1..N-4
observations.

Verification: cargo test 376 / -p alkgit 376 / -p alkgit-substrate 184
/ --all-features 379 / live-pg 208 passed 0 skips / both clippy passes
clean / fmt clean / doc zero warnings / fuzz corpus gate 13 green /
taskgraph validate 81 valid.

Reviewed artifacts: all substrate/src modules in full, the wave-9
gate's F-B1/F-S1..S9 fixes at their sites, the twin families swept
family by family.
2026-10-24 10:56:36 +00:00

44 KiB
Raw Permalink Blame History

status, last_updated, reviewed_artifacts, tool, reviewer, scope_note
status last_updated reviewed_artifacts tool reviewer scope_note
open 2026-10-24 (opened + reviewed in one session: the post-wave-9 general code-quality review of the substrate composition — the review-004/005 precedent's dimensions plus the two substrate-specific ones. Inline-fixed in this session: S-1 (the pg git_refs advisory-lock coverage gap — the F-S2 sibling that didn't get the fix), S-2 (the live-pg test gate promoted shared + the compose entry tests take it), D-1..D-5 (the crate-root doc rewrite — the review-005 D-2 analog — and four doc/hygiene items). Recorded-and-open: R-1 (the postgres.rs placement split — recommended head-of-wave-10), R-2 (the impl-side helper dedups), R-3 (the test-harness consolidation), R-4 (the mem_registry disposal), R-5 (the coverage table — F-w10-B1's input), R-6 (the per-piece parity class F-w10-B2 should absorb), R-7 (the engine-less combo inventory), N-1..N-4 (observations). The findings volume was within the inline bar; no remediation wave. The wave-10 decomposition reads this review.
substrate/src/registry.rs, sqlite.rs, postgres.rs, refs.rs, lfs.rs (the twin families — read in full: the parity sweep's basis)
substrate/src/compose.rs, liveness.rs, lfs_adapter.rs, packgen.rs, ingest.rs (the god-file candidates, read in full)
substrate/src/lib.rs, error.rs, migrations.rs, odb.rs, maintenance.rs, reactor.rs, pins.rs (the residual scan + the doc sweep)
substrate/Cargo.toml (the feature graph), the core src/ doc-sweep overlap
the wave-9 gate's inline fixes (F-B1, F-S1..S9) read for quality and completeness at their sites (liveness.rs, lfs.rs, lfs_adapter.rs, refs.rs, compose.rs, substrate/tests/real_lfs.rs)
manual source read + the full verification set + cargo-llvm-cov per-module (live-pg harness included) + measured impl/test split + re-export consumption scan + phrase sweeps + engine-less clippy runs general implementation review (code quality only — the seven-dimension charter) — the wave-9 gate's ADR-compliance verdict stands and is not re-litigated here the alkgit-substrate composition as landed through wave 9 (the core was review-004/005's object and is out of scope except the doc-drift overlap). The known wave-10 bookings (F-w10-B1..B7, O-e/O-g/O-a) are inputs, not re-derivation; F-w10-B4 ( fetch_max_total_bytes) stays dispositioned to wave 10 untouched.

Review 006 — The Substrate Composition as of Wave 9 (Post-Landing General Code-Quality Review)

Purpose

The substrate crate is complete (waves 6–9 landed the registry family, refs rows, migrations, reactive halves, odb front, packgen, GC chain, pin registry, pack ingest, composition entry, LFS) and freezes at wave 10's publish. The wave gates were spec-compliance passes; this is the other kind — review-004/005's five dimensions plus the two substrate-specific ones:

  1. God-files — measured impl/test split, seam quality, split/no-split.
  2. Duplication — the engine-twin families plus the cross-module sweep.
  3. Coverage — per-surface numbers as F-w10-B1's test-evidence input.
  4. Dead code / unused surface.
  5. Doc drift.
  6. Engine-twin parity — the mem reference vs. the twins, both directions.
  7. Assembly-shape / per-piece parity — the entry vs. the per-piece paths.

Verification Baseline

On the tree as reviewed (the wave-9 gate's tree verbatim, ba24c5f, clean status before the inline fixes):

  • cargo test: green — 376 (the default members run the core's 13 suites; the substrate is covered by the -p/feature runs below — the two-member workspace keeps them selectable). cargo test -p alkgit: green (376). cargo test -p alkgit-substrate: green (184). cargo test --all-features: green (379 — the core's sha256 riders; the substrate's full matrix rides the postgres-feature run). cargo test -p alkgit-substrate --features postgres with the live-pg harness: 208 passed, 0 failed, 0 skips (sqlite+lfs+ postgres compiled and run together).
  • Both clippy passes (--all-targets -- -D warnings; also --all-features): clean. cargo fmt --check: clean. cargo doc --no-deps: zero warnings. The fuzz corpus gate: 13 targets green.
  • Zero TODO/FIXME/HACK in the substrate; zero unsafe (both crates #![forbid(unsafe_code)]); one expect outside tests (D-5 below) and zero library-side #[allow] (the one substrate allow is a test helper).
  • The engine-less clippy runs (outside the documented feature matrix) found dead-code warnings (R-7), distinct from the booked F-w10-B6.

After the inline fixes (stamped in Part A): the full verification set re-ran green — cargo test (376), -p alkgit (376), -p alkgit-substrate (184), --all-features (379), the live-pg run (208, 0 skips — the S-2 gate addition included), both clippies, fmt, cargo doc --no-deps (zero warnings), the fuzz corpus gate.

Summary Statistics

Severity Count IDs
Correctness-adjacent (serialization/test-gate), fixed inline 2 S-1, S-2
Doc/hygiene, fixed inline 5 D-1..D-5
Booked — head-of-wave-10 placement task 1 R-1
Duplication, booked (share calls) / judged keep 4 R-2, R-3, R-4, R-6
Booked — wave-10 test-evidence input 1 R-5
Booked — feature-matrix hygiene ride 1 R-7
Observations 4 N-1..N-4

No blocking issues; no remediation wave (the findings volume sat inside the inline bar — the review-004 rule). Per the disposition rules: R-1/R-2/R-3/R-4 are the named head-of-wave-10 recommendations (refactor → OQ-21 POC → test evidence → docs → publish is the natural order — the test-evidence and docs tasks pin against settled module paths); R-5's table rides into F-w10-B1's inventory; the verdict is the input to the wave-10 decomposition discussion. Nothing here blocks wave 10.


The dimension readings

Dimension 1 — God-files (measured, per candidate)

Impl lines exclude #[cfg(test)]; measured 2026-10-24 from the gate tree (the task's numbers reproduce to ±1 trailing-newline).

File Impl Tests Verdict (this review)
lfs_adapter.rs 1346 814 no split — one surface, load-bearing doc
postgres.rs 1294 840 R-1: re-home the plumbing (below)
refs.rs 1049 1038 no split — the twin-placement pattern working as designed
ingest.rs 852 1056 no split — one machine
lfs.rs 726 644 no split — same as refs.rs
compose.rs 710 1617 no split — the composition entry; the ratio is an integration surface's
liveness.rs 697 1421 no split — marginal
packgen.rs 678 414 no split — one pipeline + its serving shapes
sqlite.rs 623 863 no split — cohesive twin; the asymmetry explained by R-1
all others ≤548 — fine
  • lfs_adapter.rs carries five workstreams (DTOs/error, the grant provider, the batch/ops logic, the put path PutPipe, the download stream verified_windows) whose seams are mostly disjoint — but the helpers are genuinely shared across them (hex_of, require_identity), the 150-line module doc is ONE pinned narrative carrying the F-S3/F-S4 statements and the adapter/door split that the gate documents against, and the only sibling candidate (the transfer machinery, ~350 lines) is a marginal size win. Cohesive narrative beats it — the review-005 duplex/stateless precedent.
  • refs.rs / lfs.rs are the deliberately repeated shape: one shared semantics kernel (cas_decide/run_cas in refs; validation + the epoch wrapper in lfs) with the reference impl and both engine twins inline as sibling mod sqlite_twin/mod pg_twin feature-gated modules, the twin batteries running identical scripts per engine. Placement is symmetric across engines and the kernel orphans if split by engine. No split.
  • ingest.rs / packgen.rs are single machines (one call graph; the through-store pipe is the one genuine entanglement, and it is the machine's heart). No split.
  • compose.rs is the composition entry: wire() is called by all three constructors and the accessors bound to the one struct — nothing about the shape permits a mechanical cut, and 710 impl / 1617 test is the entry's business.
  • liveness.rs could mechanically drop a liveness_lfs.rs sibling (~230 impl lines — the LFS source is disjoint code from the refs closure), but the win is modest and the module doc carries both sources' invalidation rationale as one argument. No split now; the duplicated seam-comment block (D-3) deduped regardless. Revisit trigger: none pending — wave 10 adds test evidence, not machinery.
  • The sqlite.rs/postgres.rs asymmetry is placement, not hidden twins — the row twins live symmetrically in refs.rs/lfs.rs per engine; postgres.rs also bundles the CoTenantPg handle, the entire pg migration runner, and the crate's only cross-file private plumbing (qualify/advisory_lock_key/validated_schema, imported by refs.rs and lfs.rs), while the sqlite counterparts of the first two live in migrations.rs. R-1 remedies the placement.

Layout convention (pinned for any eventual split): convention 14's flat siblings (no directory, no amendment needed) — the re-export list is the surface to preserve verbatim through the not-yet-frozen OQ-03 inventory.

Dimension 2 — Duplication (share/don't-share per instance)

Share (beyond the cas_decide precedent):

  • row_to_record + visibility_str — verbatim duplicated between sqlite.rs and postgres.rs; engine-neutral serde-shape parsing feeding a correctness-critical path (corrupt-row reads → RegistryError::Invalid on both twins) with zero engine dependency. This is the cas_decide-quality second candidate: move to registry.rs beside validate_record. Into R-2.
  • The advisory-lock scope strings — the {schema}\0{scope} composition is spelled four times (postgres.rs's STAMP_TABLE lock, refs.rs's git_refs, lfs.rs's lfs_objects, and a hand-inlined copy at postgres.rs:838 — the F-S2 leg — where a typo would silently split the lock scope and nothing would error). Hoist the scope-string construction next to advisory_lock_key and delete the inline spelling. Into R-2. (The one-line acquire statement per transaction itself stays per-tx — that is the pattern, not duplication to remove.)
  • The pg recount helpers' table qualification — recount_sql's qualification rides a replacen string-hack (correct only because the raw SQL contains exactly the two occurrences it does); take the qualified table as a parameter. Into R-2. (The sqlite/pg helpers themselves stay separate — engine dialects.)

Test-harness dedup: wait_until ×4, spawn_wake_runner/stop_and_join ×3 sets, unique_db_path ×4, scratch_dir ×4, and the record/repo fixtures ×~10 across the test modules. One #[cfg(test)] testutil home deletes ~25 copies and is the natural home for the (now shared, per S-2) live test gate. R-3.

Judged don't-share (recorded so the next reviewer skips):

  • The five io_err-family one-liners — each welded to one engine error type × one target error family; a generic helper costs two generics and a Display bound to save one line per site.
  • The migration runners (sqlite BEGIN IMMEDIATE + stamp row vs. pg advisory-lock + re-key verify) — deliberate per ADR-024 §2's no-framework rule; parity is enforced by the step-parity test where it belongs. Don't couple the engines through shared migration code.
  • The engine twins' in-SQL removal-leg copies (remove_repo spelled three times: both twins' record-removal transactions + the rows' own legs) — documented deliberate in lfs.rs's module doc; the drift risk is real but the cost of abstraction is a SQL templating layer. Keep (N-2 records the parity-test recommendation).
  • The sqlite run_transaction/run_blocking helpers — one bundle's value is its own error family and its retry-bearing handle pin; sharing would be nominal.

Dimension 3 — Coverage (F-w10-B1's input table)

cargo llvm-cov --workspace --all-features with the live-pg harness env (the pg legs otherwise collapse — the pg-only surfaces run only live; the harness env is part of the honest procedure, the env-less run reads postgres.rs as 9% and refs.rs's pg twin as dead). Lines, post-inline-fix tree:

Substrate surface Lines Missed Cover
liveness.rs 1162 22 98.11%
pins.rs 545 6 98.90%
registry.rs 521 16 96.93%
compose.rs 1510 75 95.03%
refs.rs 1193 69 94.22%
reactor.rs 901 64 92.90%
odb.rs 833 65 92.20%
lfs.rs 669 54 91.93%
maintenance.rs 789 67 91.51%
packgen.rs 679 56 91.75%
sqlite.rs 1034 96 90.72%
lfs_adapter.rs 1295 121 90.66%
ingest.rs 1290 141 89.07%
migrations.rs 422 49 88.39%
postgres.rs 1381 157 88.63%
(workspace TOTAL, incl. the core) 23053 1567 93.20%

The thin arms by class (the residual-missed inventory, exactly what F-w10-B1's big-media/ceiling/corruption work and the fleet battery reach): lfs_adapter.rs — the download windows' io error arms and the upload PutPipe error arms (the S-3 mismatch arm has no mem-store corruption seam — F-w10-B1's booked row), the fleet custody arm (zero battery coverage — F-w10-B3's booked row: the battery's adapters are all mem_only(), ops.fleet_owner() resolves None and only the custodial arm ever runs); ingest.rs — the delta-apply overproduction aborts, the store-read corruption classification, the task-join failure legs; postgres.rs / migrations.rs — the io error-mapping arms and the migration re-key/verify paths; sqlite.rs / packgen / odb / maintenance — the error-mapping and defensive tails that fuzz/reach work can service later. The composition entry's shapes are strong (95%).

Disposition (the charter asks): coverage stays review-time evidence per the review-005 rule, with the live-pg env attached as part of the procedure. The gates' test rows already pin behavior; both review kinds acted on the numbers immediately (F-w10-B1/B3 are already booked off thin-arm reads). The per-gate cadence is what makes it useful; table above rides into the wave-10 test-evidence inventory.

Dimension 4 — Dead code / unused surface

  • Re-export consumption scan (65 names in substrate/src/lib.rs): every re-export consumed in-tree or by the suites except two classes: (a) the door-facing LFS DTOs (LfsBatchResponse, LfsDownload, LfsObjectAnswer, LfsObjectError, LfsActions) — returned by/elementary to the adapter's own surface and pinned by the wave-9 OQ-03 inventory; intentional; (b) the bare-twin nameability re-exports (PgRegistry, SqliteRegistry, AnnounceReport) — consumed by their assemblies/returns; intentional probe surface; and (c) one genuine candidate: mem_registry — production role superseded by the entry's mem shape (RegistryLegs::mem, which adds the reactor, the seed/removal legs and the mutation-legs surface mem_registry lacks); its consumers are test-only. It is on the wave-6 inventory seed (review-wave-6), so removal is a surface change the not-yet-frozen inventory settles — R-4.
  • #[allow] inventory: the shipped substrate library carries zero (one in a test helper, compose.rs's run_push; the tests/ harnesses carry their own benign ergonomics allows). The core's seven too_many_arguments sites are doc-justified dispatch surfaces (the review-005 N-1 disposition unchanged).
  • Markers/panics: zero TODO/FIXME/HACK; zero unwrap/expect outside tests after D-5 (the one expect lived in verified_windows).
  • Feature-gated paths never exercised: the documented matrix (lfs alone, +sqlite, +postgres, pg-pure, lfs-less) compiles green except the booked residuals. R-7 records the inventory around F-w10-B6: the engine-less combo (--no-default-features, no features at all) also fails -D warnings — row_state_from_columns (refs.rs, the engine-consumed-only parser, itself un-gated) + the Path import (compose.rs) + the engine-cfg'd test fixtures (scratch_dir, unique_db_path) + two test-target imports — and it is the cleaner combo to fix alongside F-w10-B6's lfs-alone combo (same sites include the lfs-alone warnings).
  • Config knobs: every knob consumed except the already-dispositioned fetch_max_total_bytes dead seed (F-w10-B4 — confirmed dead at lfs_adapter.rs, built into an OpsContext that only ever invokes blobs/stat; not re-decided here). LfsAdapterConfig, AssemblyOptions, MaintenanceOpts, PgRegistryOptions, RegistryLegs all fully consumed.

Dimension 5 — Doc drift

  • The substrate crate-root doc carried the exact defect the core fixed at review-005 (D-2 there): four "Wave-N status … have landed" changelog paragraphs (lib.rs:133–176) sitting behind a structural module map, one of them the stale "Later substrate-track waves land in their own modules" sentence directly contradicted by the wave-9 status paragraph that follows it, plus a dangling fragment (lib.rs:151). Fixed inline (D-1): the wave-status paragraphs dropped, the map retitled structural, and the lfs/lfs_adapter modules — previously described only inside the changelog — are now map entries in their own right. Contrast with the core's src/lib.rs, which is already the fixed shape.
  • The phrase sweep (stale future-tense narration): reactor.rs's module doc carried "the enqueue's consumer side is wave 7's … will land" and "driven by the scheduler tick once the maintenance chain's consumer side lands" about machinery that landed in wave 7 — fixed inline (D-4) with module pointers replacing the wave narrative. lfs.rs's "later wave-9 tasks, both" — fixed inline (D-4). The mild landed-task references written as location ("the w6-registry-postgres landing", "the wave-9 rows task's landing") are factual attribution with task-id pointers — kept deliberately.
  • The epoch-discipline honesty reads (the F-B1 rewrite): the load-before-read/bump-after-commit pairing is documented coherently on all four sides (liveness.rs's both consults, lfs.rs's wrapper doc with the bump-before review-out rationale, refs.rs's wrapper) — one residual contradiction was the refs.rs wrapper comment claiming denied calls bump nothing, which the code and the pinned denial-bump test contradict; fixed inline (D-2). The F-S3 verification statement (whole-span oid check before the last window yields; partial spans weaker) matches the impl in all three places it is pinned; no contradiction found.
  • Core-side overlap (out of charter to fix, recorded): N-3.

Dimension 6 — Engine-twin parity (per-family verdicts)

The mem implements are the reference; drift in either direction is a finding. Sweep result: one runtime defect found and fixed inline (S-1); the families are otherwise sound.

  • Registry family (mem / SqliteRegistry / PgRegistry): PARITY. Same validation (validate_record called identically; ids un-validated on the remove path in all three), same typed business denials, same corrupt-row → RegistryError::Invalid mapping, same observable watermark semantics (both twins pin "failed duplicate put consumed no seq"; pg's bump-first-vs-check-inside ordering is a declared, lock-discipline-driven divergence with none observable), same record-removal completeness at the GitRegistryStore seam (twins in-SQL; mem via MemSeededBackend), same HEAD seeding (HEAD_SEED_TARGET shared). Locking is per-engine but equivalent-serializing: mem one parking_lot lock, sqlite co-tenant mutex + BEGIN IMMEDIATE
    • WAL + busy timeout (whole-file write lock covers every leg — including the removal's full-table recount, which needs no advisory lock the way pg does), pg co-tenant mutex + the registry_seq row lock (which serializes every registry transaction cross-instance) + advisory locks where engine twins write the same tables cross-connection.
  • Refs family: PARITY structurally — the shared cas_decide/run_cas kernel makes CAS-semantics drift impossible, both twins run the same battery, the same atomic-abort matrix, the same request-order rules, and the epoch wrapper (VersionedRefTable) wires the bump-after-commit half identically through both assemblies. The pg CAS holds the whole-fleet {schema}\0git_refs advisory lock; the registry transactions touching git_refs did not — S-1 fixed that (the F-S2 sibling: the wave-9 gate added the lock to the lfs_objects leg of the removal and missed the git_refs legs). Mem drift noted, both benign: the mem removal bumps the refs counter (over-invalidation, the safe direction per the wrapper docs); the mem removal's legs are not one atomic span the way the twins' transactions are (N-1).
  • LFS rows family: PARITY, clean. The F-S1 fix verified in place (one RwLock<InMemoryLfsState> over rows+refcounts; mutation and recount in one locked section); the F-S2 fix verified on both pg sources (every PgLfsRows write transaction takes the {schema}\0lfs_objects advisory lock first, and the registry removal's leg takes the same scope key; sqlite gap-free by construct — the write lock covers both legs); identical per-oid trailing recount and full-table recount on repo removal across all three; validation shared by all three; the F-B1 pairing (bump-after-commit, Err bumps nothing, removals never bump) wired identically at the entry for both wrapper sources (mem wrapped at construction; both twins wrapped by the entry). The one F-B1 residual — the stale refs.rs comment (D-2) — fixed.
  • The S-1 defect, precisely: the pg registry's durable_put (the HEAD-seed insert into git_refs, postgres.rs) and durable_remove (the repo's git_refs delete leg) touched the refs table without the {schema}\0git_refs advisory lock the refs CAS twins hold for their whole call. Two co-tenant instances (ADR-024 names multi-instance co-tenancy native; the shipped single-client assembly masks it) could interleave a CAS's decided writes with the removal's delete: CAS reads → remove commits (rows deleted) → CAS commits rows for a repo whose record is gone — orphan rows surviving a removal whose contract is "the whole repo's co-tenant state in one removal", feeding the liveness fingerprints conservatively (no data loss — GC keeps — but unbounded row leak on repeated recreate/destroy races) and defeating the removal's completeness. Sqlite is immune by write-lock topology; mem is immune by single-process locking (N-1 records its smaller span). Fixed inline (S-1): both transactions take the git_refs advisory lock first statement (put: before the seq bump — the rare construction leg queues behind the CAS storm, the exact tradeoff the lfs_objects leg already accepts; remove: alongside the lfs lock, documented); the whole txs remain deadlock-free (no transaction takes another advisory lock after the seq row lock, and the put/remove orders agree). The marker legs need no lock — both marker writers serialize on the registry_seq row lock cross-instance, and the not-found rollback closes the interleave for free.

Dimension 7 — Assembly-shape / per-piece parity

The composition entry's three shapes vs. every per-piece constructor path, surface by surface:

Surface Entry hands out Per-piece path Parity
Registry pair RegistryBackend::with_cached_provider over the announced write side (+ mem's record legs for mem) the twin assemblies build the identical binding ✅ identical construction
Record-transaction legs fused (MemSeededBackend) / in-SQL (twins) same behavior, home per shape (documented) ✅
Mutation-marker legs RegistryMutations impls the twin assemblies ARE the impls ✅
Refs (GitRefs) VersionedRefTable-wrapped the twins carry the wrap themselves (refs() returns the wrapped surface) ✅ — the wave-7 per-piece obligation (twins carry the counter home) is honored
Liveness (refs closure) RefClosure::new+register in wire public ctors, per-piece reachable ✅
Ingest/front entry-only (PackIngest::new is pub(crate); pins private) not reachable per-piece at all ✅ no gap possible
Pack serving both shapes + the chain-shared PackCache PackGen::through_store(front) builds the same shape with a fresh private cache no maintenance chain can reach (PackHousekeeping needs the shared handle) ❌ the through-store sibling of F-w10-B2
LFS rows VersionedLfsRows-wrapped the twins' lfs_rows() returns the raw rows — the booked F-w10-B2 (the entry owns the counter, so the twins cannot match the mem/refs posture today) ❌ booked
LFS media registration LfsMedia::new+register in wire public cts, equivalent given the caller wraps the rows ✅ conditional on F-w10-B2's fix

So the answer to the charter's question is yes — per-piece paths can reach three surfaces the entry's shapes cannot produce in that unwrapped/unregistered form: the raw lfs_rows() (booked F-w10-B2), the unshared-cache through_store new instance, and — beyond the rows class — mem_registry(), a complete GitRegistryStore without the record-transaction legs, the reactor, or any announce wiring (none of the entry's shapes resembles it). R-6 records the recommendation: F-w10-B2's fix should absorb the whole class — the twins carry the VersionedLfsRows wrap (the refs-twins' posture is the model), the through-store constructor either takes the wrapped-cache home or documents the private-cache shape as deliberate per-piece probe surface alongside durable(), and mem_registry's eventual home settles with R-4. The per-piece parity hazard is otherwise honest probe surface (durable() documented as such).

AssemblyOptions plumbing verified: all options consumed in all three shapes (large_root deliberately unused on the mem path, documented).


Part A — Findings fixed inline

S-1 [correctness-adjacent, pg serialization] — the pg registry's put/remove transactions touched git_refs without the refs-table advisory lock the CAS twins hold for their whole call

Files: substrate/src/postgres.rs (durable_put, durable_remove, the docs on both).

Problem: the wave-9 gate's F-S2 fix established "every transaction touching lfs_objects takes the {schema}\0lfs_objects advisory lock, including the registry's removal leg" — but the same invariant for git_refs did not hold: durable_put inserted the HEAD seed row and durable_remove deleted the repo's ref rows under no lock the PgRefTable CAS respects. Two co-tenant instances (ADR-024 §3 names multi-instance co-tenancy native; the shipped single-client assembly masks it) could interleave a CAS's decided writes with a removal's delete — orphan ref rows surviving a completed removal (conservative GC cost, no loss; unbounded leak on repeated recreate/destroy races) and defeating the removal's completeness contract. Sqlite immune by BEGIN IMMEDIATE topology; mem immune by single-process locking; the window is pg-specific and real.

Fix (landed): durable_put takes pg_advisory_xact_lock(advisory_lock_key("{schema}\0git_refs")) as its first statement (before the seq bump — a rare construction leg queueing behind the CAS storm, the identical whole-fleet tradeoff the removal's lfs_objects leg accepts); durable_remove takes the same lock alongside its lfs_objects lock, before the delete legs; the module docs on both transactions record the invariant. Deadlock-free: no transaction takes an advisory lock after the seq row lock, and put/remove's orders agree. The marker legs serialize on the registry_seq row lock cross-instance (both marker writers bump it), so no further lock is owed there. The parity batteries (sqlite/pg/mem) re-ran green; the live-pg suite runs 208/0-skip with the fix. Test evidence note: the F-S2 fix's precedent stands — the interleave window itself is cross-connection and not drivable over the in-process batteries; the shape is documented and now uniform ("every transaction touching a co-tenant table holds that table's advisory lock").

S-2 [test-gate race] — the live-pg silence-assertion probe was serialized only against its own module while the compose entry's live tests announce on the same container

Files: substrate/src/postgres.rs (the live_harness module + the tests module), substrate/src/compose.rs (the three live-pg entry tests).

Problem: the module docs state the hazard (the LISTEN/NOTIFY wake channel is database-wide; a probe asserting silence must not read a sibling test's announce), and the mitigation was a module-private LIVE_GATE static — while the composition entry's three live tests (compose.rs, harness_dsn() directly) perform announced writes (put_repo, the marker+announce push legs) holding no gate at all. cargo runs one binary's test modules concurrently in threads; the asymmetry probe's wake.is_none() assertion races any concurrent compose announce. A latent fleet of live-pg flakes of exactly the O-e class the wave-8 gate recorded as the unreproduced candidate.

Fix (landed): the gate promoted to live_harness::LIVE_GATE (the shared, already cross-module-consumed test home, doc noting the database-wide rationale); postgres.rs's tests import it; all three compose entry live tests take it before their writes (the module doc on the first test says why). The live-pg suite re-verified green (208, 0 skips). Remaining live-pg consumers (the refs/lfs pg-twin tests) carry no announce and no silence assertion — they need no gate.

D-1 [docs] — the substrate crate-root doc was the review-005 D-2 defect verbatim: a structural map buried behind four "Wave-N status … have landed" changelog paragraphs, one self-contradicting

Fixed inline (2026-10-24, this session): the wave-status paragraphs (legacy lib.rs:133–176) dropped; the map retitled # The module map (with the wave-foundation framing removed from its heading and the per-module wave attributions reduced to structure); the lfs and lfs_adapter modules — described previously only inside the wave-9 changelog paragraph — added as proper map entries with their pinned facts (the epoch wrapper, the whole-span oid verification, the wired-provider posture); the trailing "pinned decisions" section retitled away from "wave-8". No re-export or code change.

D-2 [docs, the F-B1 residual] — the VersionedRefTable comment claimed denied calls bump nothing; the code (and the pinned denial-bump test) say every green call bumps

Fixed inline: the comment at the refs epoch wrapper rewritten to the pinned mechanism — every green call bumps (denials ride the outcome list; an all-denied call landing nothing still moves the counter past any racing consult — over-invalidation, the safe direction), only Err bumps nothing. Matches the behavior, the liveness battery's pinned row, and the F-B1 verdict ("a denied-but-green CAS call still bumps").

D-3 [hygiene] — a six-line comment block duplicated verbatim in liveness.rs (the refs-closure seam comment, copy-pasted at the LFS arm's landing)

Fixed inline: the duplicate (the copy ahead of the media test module) removed; the original (beside the seam's RefRows impls pointer) stays.

D-4 [docs] — reactor.rs's module doc narrated wave 7's consumer side in the future tense, and lfs.rs's module doc pointed its consumers at "later wave-9 tasks"

Fixed inline: reactor.rs's enqueue leg now names the landed homes (crate::maintenance, crate::liveness) instead of "the consumer side is wave 7's … land with the substrate Find (wave 7)", and the reconcile paragraph drops "once the … consumer side lands"; lfs.rs's "later wave-9 tasks, both" → the consumers named plainly. No content change — the facts were already the code's.

D-5 [hygiene, convention 2] — verified_windows carried the one library-side expect in the workspace

Fixed inline: the whole-span digest comparison restructured as an if let (Some(h), Some(expected_bytes)) over the hasher/expected pair (whole_span is exactly what makes both Some), eliminating the expect ("No unwrap() or expect() outside tests"). Behavior identical; the honest-case/byte-exact battery rows re-ran green.


Part B — Recorded findings (not fixed: booked as recommendations)

R-1 [booked — head-of-wave-10 placement task] — postgres.rs bundles the machinery the sqlite twin ships in migrations.rs: re-home the co-tenant handle + the pg migration runner + the pg plumbing to the ADR-024 machinery home

Measurement/cause: the sqlite.rs ↔ postgres.rs size asymmetry (623 vs. 1294 impl lines) is placement, not hidden twins — both engines' row twins live symmetrically (inline mod *_twin in refs.rs/lfs.rs), but postgres.rs additionally bundles the CoTenantPg handle (sqlite's counterpart CoTenantSqlite lives in migrations.rs's mod sqlite_twin), the entire pg migration runner (sqlite's steps live in migrations.rs), and the pg SQL plumbing (validated_schema, qualify, advisory_lock_key — imported by refs.rs and lfs.rs; the crate's only cross-file pub(crate) dependency). migrations.rs's own doc records the origin ("the postgres twin defines its own twin of this handle in its task") — coherent in placement history, inconsistent in the settled layout.

Shape (recommendation, not executed here): move CoTenantPg, the pg runner, and the three plumbing helpers into migrations.rs (the ADR-024 machinery home both engines share). Zero public-API change by construction — lib.rs re-exports CoTenantPg from wherever it lives; the OQ-03 inventory names are preserved verbatim. postgres.rs drops to ~900 lines (the twin + its options/assembly) and the sqlite/pg machinery placement becomes symmetric; the plumbing's consumers import from one home. Risk: low; mechanical (the runner's call graph touches only CoTenantPg::open). Per the disposition rules this is a named task at the head of wave 10, ahead of the OQ-21 POC and the test- evidence/docs tasks that pin against settled module paths.

R-2 [booked — head-of-wave-10, foldable into R-1's session] — the impl-side helper dedups: the twins' row_to_record/visibility_str, the advisory-lock scope strings (including the hand-inlined F-S2 copy), the pg recount qualification

Detail per Dimension 2: (1) row_to_record + visibility_str verbatim across sqlite.rs/postgres.rs, engine-neutral, feeding the corrupt-row → RegistryError::Invalid correctness path — move beside validate_record in registry.rs (the cas_decide-quality share); (2) the {schema}\0{scope} composition spelled four times with the risky copy being the hand-inlined one at postgres.rs's removal leg (a typo silently splits the lock scope — S-1's new git_refs leg included — and nothing errors) — hoist named scope constructors beside advisory_lock_key; (3) the pg recount_sql's replacen string-hack qualification — take the qualified table as a parameter. Small, mechanical, test-visible (the parity batteries + the error-map tests re-pin instantly).

R-3 [booked — head-of-wave-10 test-hygiene task] — one #[cfg(test)] testutil home for the ~25 duplicated harness helpers

Detail per Dimension 2: wait_until ×4, the wake-runner spawn/join sets ×3, unique_db_path ×4, scratch_dir ×4, the record/repo fixtures ×~10 — consolidate into one test-only module (placed per convention 14 as a non-pub sibling, e.g. testutil.rs, or a #[cfg(test)] shared module), which is also the settled home for live_harness::LIVE_GATE (S-2 lands it in live_harness meanwhile — one gate, not per-module growth). Zero runtime coupling; the suites are the consumers.

R-4 [booked — wave 10's freeze-inventory settlement input] — mem_registry is a production-superseded surface whose only consumers are tests

Detail per Dimension 4: the "batteries-included degenerate assembly" (its own doc) lacks the reactor, the record-transaction legs, and the mutation-legs surface that the entry's mem shape (RegistryLegs::mem) composes — no non-test consumer remains. It is on the wave-6 inventory seed, so the not-yet-frozen OQ-03 inventory settles it at wave 10: remove (and update the registry module doc's first-complete-impl narrative) or doc-stamp as the test-only degenerate shape. Not decided here; recorded so the inventory discussion has the facts.

R-5 [booked — F-w10-B1's test-evidence input] — the per-surface coverage table (Dimension 3's) and the thin-arm classes

The table and the residual classes (the adapter's window/pipe io arms and the corrupt-entry mismatch arm, the ingest's guard paths, the pg io/re-key arms) ride into wave 10's decomposition record as the test-evidence inventory's starting column, alongside F-w10-B1's own row list (big-media/ceiling doors, corruption-injection seam, anonymous-refusal row) and F-w10-B3's fleet battery row. Coverage stays review-time evidence (the review-005 disposition), with the live-pg env attached as part of the honest procedure — the env-less run misreads every pg-only surface.

R-6 [booked — F-w10-B2 should absorb the per-piece parity class] — the through-store private-cache instance and the leg-less mem_registry are the class's other two instances

Detail per Dimension 7: the twins carry the VersionedRefTable wrap themselves (the wave-7 refs obligation honored); the LFS twins cannot (the entry owns their counter — F-w10-B2's booked fix), and PackGen::through_store(front)'s default-cache constructor yields a serving shape whose cache no maintenance chain can reach — an unregistered-housekeeping instance the entry's shapes never produce. Recommendation: F-w10-B2's fix (the twins carry the VersionedLfsRows wrap or a doc-stated obligation) extends to the class — the through-store constructor takes an explicit cache or documents the private-cache shape deliberately beside the documented durable() probe surface, and mem_registry's home settles with R-4. Recorded as recommendations; nothing is re-decided against the gate's bookings.

R-7 [booked — rides F-w10-B6's feature-matrix hygiene] — the engine-less combo (--no-default-features) fails -D warnings on the same dead-code class F-w10-B6 names, and is the cheaper fix target

Inventory around the booking (not re-deriving F-w10-B6's lfs-alone combo, whose warnings it subsumes): row_state_from_columns (refs.rs — the engine-consumed-only CAS parser is itself un-gated) and the Path import (compose.rs) are dead in both feature-less combos; the engine-cfg'd test fixtures (scratch_dir, unique_db_path) and two test-target imports ride the combos' test builds. The --no-default-features --features sqlite combo is fully clean; the postgres-alone combo leaves the two lib-side warnings + test fixtures. Fold into wave 10's feature-matrix hygiene (gate the parser or its consumers, drop the import, cfg the fixtures) — one pass closes both combos.

N-1 [observation] — the mem removal's legs are not one atomic span where the twins' removals are single transactions

MemSeededBackend::remove_repo commits the registry row removal, then drops ref rows, LFS rows, and bumps the counter under separate locks — an observer can see resolve return None while list_refs/rows() still serve the removed repo's rows. No corruption is possible (single-process; the same "write after removal commits" window exists on every engine), and the mem reference's atomicity-span honesty is worth knowing when reading parity evidence. Nothing to fix in v1.

N-2 [observation] — the twins' in-SQL removal-leg copies are tripled by design and carry no cross-file parity test

The LFS-removal + refcount recount script exists inline three times (sqlite.rs's and postgres.rs's record-removal transactions + the rows twins' own legs), documented deliberate. The next schema change is the drift risk's window; a small cross-file parity test (the same script asserted against both engines' schemas) is the cheap guard if any schema work enters wave 10. No action forced now.

N-3 [observation] — the core's doc narration has the same mild "later waves" staleness the substrate's had

src/limits.rs ("enforcement lives in the later waves' machines"), src/substrate.rs ("stateless, later wave"), src/duplex.rs / src/stateless.rs ("the later waves' machines to reach"), src/backend.rs ("every later wave") — the consuming machines have all landed; the sentences read as historical stage directions. Out of this review's charter to edit (the substrate is the reviewed object; the core got its doc pass at review-005), recorded so the wave-10 docs task can sweep them in one pass if it touches the core's docs.

N-4 [observation] — the allow inventory is small, justified, and contains no dead surface

The shipped substrate library carries zero #[allow]; the one in the crate is on a test helper; the test harnesses' allows are ergonomics; the core's seven are doc-justified dispatch tuples (review-005's N-1 disposition unchanged). No #[allow(dead_code)] 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 (stated so the next reviewer skips it)

  • The god-file table is measured and dispositioned per candidate (Dimension 1); no second god-file exists (the runner-up is migrations.rs at 401 impl lines, and its bulk is the ADR-024 machinery + twin, correctly placed).
  • The twin-parity sweep (Dimension 6) came back clean on every family after S-1: validation, error mapping, watermark semantics, removal completeness, seeding, recount/refcount logic, epoch wiring, and the serialization shapes are parity-pinned or declared-divergent with no observable difference. The F-S1/F-S2 fixes verified in place on both sources.
  • The wave-9 gate's fixes read as review inputs, stamped: F-B1 complete (the pairing flipped on both sources, the docs coherent after D-2, both straddle regressions genuinely discriminating); F-S3 complete (the mismatch check precedes the last-window yield; the verification statement pinned in three consistent places; the negative row is F-w10-B1's booked seam); F-S4 complete and best-tested (the resolved-span gate pinned including the full-span ranged form); F-S5 complete in code and docs (the battery gap is F-w10-B3's booked row; no non-fleet constructor exists in-tree); F-S6/S7/S8/S9 verified at their sites (guards, orderings, the transfers door duty recorded).
  • The engine-less clippy runs (R-7) are the only feature-matrix residue beyond the booked F-w10-B6; the documented matrix compiles and tests green.
  • Dead surface (Dimension 4): beyond R-4's candidate, the zero-external-consumer re-exports are the documented intentional classes (the door-facing DTOs, the bare-twin probes, the budget-knob constants); every config knob is consumed but F-w10-B4's.
  • Doc honesty (Dimension 5): post-D-1..D-5, the phrase sweeps are clean, the crate docs are structural maps in both crates, the epoch docs are coherent with the corrected mechanism on all four sides, and cargo doc --no-deps is warning-free.

Part D — Recommended disposition order (for the wave-10 decomposition)

  1. R-1 (+ R-2 folded in) — the postgres.rs placement task; full verification set after, the parity batteries re-pin instantly.
  2. R-3 — the test-harness consolidation (S-2's gate is already shared; this is the helpers' home + any LIVE_GATE re-home in the same pass).
  3. R-6 / R-4 — the per-piece parity class (F-w10-B2 absorbed) and the mem_registry disposition ride the OQ-03 freeze-inventory settlement + the F-w10-B1/B3 test-evidence tasks that follow.
  4. R-5 — the coverage table feeds the test-evidence inventory verbatim; coverage stays review-time evidence, live-pg env attached.
  5. R-7 — the engine-less combo joins F-w10-B6's one-pass feature hygiene.
  6. S-1/S-2/D-1..D-5 — landed and stamped; wave 10 composes against the fixed surface.

The substrate's publish posture is otherwise unchanged: no remediation wave is owed, and the wave-10 decomposition proceeds with the refactor-first ordering above.