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.
44 KiB
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. |
|
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:
- God-files — measured impl/test split, seam quality, split/no-split.
- Duplication — the engine-twin families plus the cross-module sweep.
- Coverage — per-surface numbers as F-w10-B1's test-evidence input.
- Dead code / unused surface.
- Doc drift.
- Engine-twin parity — the mem reference vs. the twins, both directions.
- 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 postgreswith 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)]); oneexpectoutside 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 streamverified_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_casin refs; validation + the epoch wrapper in lfs) with the reference impl and both engine twins inline as siblingmod sqlite_twin/mod pg_twinfeature-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.rssibling (~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
CoTenantPghandle, 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::Invalidon both twins) with zero engine dependency. This is thecas_decide-quality second candidate: move toregistry.rsbesidevalidate_record. Into R-2.- The advisory-lock scope strings — the
{schema}\0{scope}composition is spelled four times (postgres.rs'sSTAMP_TABLElock, refs.rs'sgit_refs, lfs.rs'slfs_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 toadvisory_lock_keyand 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 areplacenstring-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 aDisplaybound 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_repospelled 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_blockinghelpers — 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 surfacemem_registrylacks); 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'srun_push; thetests/harnesses carry their own benign ergonomics allows). The core's seventoo_many_argumentssites are doc-justified dispatch surfaces (the review-005 N-1 disposition unchanged).- Markers/panics: zero TODO/FIXME/HACK; zero
unwrap/expectoutside tests after D-5 (the oneexpectlived inverified_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) + thePathimport (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_bytesdead seed (F-w10-B4 — confirmed dead at lfs_adapter.rs, built into anOpsContextthat only ever invokesblobs/stat; not re-decided here).LfsAdapterConfig,AssemblyOptions,MaintenanceOpts,PgRegistryOptions,RegistryLegsall 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 thelfs/lfs_adaptermodules — 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_recordcalled identically; ids un-validated on the remove path in all three), same typed business denials, same corrupt-row →RegistryError::Invalidmapping, 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 theGitRegistryStoreseam (twins in-SQL; mem viaMemSeededBackend), same HEAD seeding (HEAD_SEED_TARGETshared). Locking is per-engine but equivalent-serializing: mem oneparking_lotlock, 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_seqrow lock (which serializes every registry transaction cross-instance) + advisory locks where engine twins write the same tables cross-connection.
- 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
- Refs family: PARITY structurally — the shared
cas_decide/run_caskernel 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_refsadvisory lock; the registry transactions touchinggit_refsdid not — S-1 fixed that (the F-S2 sibling: the wave-9 gate added the lock to thelfs_objectsleg of the removal and missed thegit_refslegs). 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 (everyPgLfsRowswrite transaction takes the{schema}\0lfs_objectsadvisory 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,Errbumps 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 intogit_refs, postgres.rs) anddurable_remove(the repo'sgit_refsdelete leg) touched the refs table without the{schema}\0git_refsadvisory 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 thelfs_objectsleg 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 theregistry_seqrow 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-depsis warning-free.
Part D — Recommended disposition order (for the wave-10 decomposition)
- R-1 (+ R-2 folded in) — the postgres.rs placement task; full verification set after, the parity batteries re-pin instantly.
- 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).
- R-6 / R-4 — the per-piece parity class (F-w10-B2 absorbed) and
the
mem_registrydisposition ride the OQ-03 freeze-inventory settlement + the F-w10-B1/B3 test-evidence tasks that follow. - R-5 — the coverage table feeds the test-evidence inventory verbatim; coverage stays review-time evidence, live-pg env attached.
- R-7 — the engine-less combo joins F-w10-B6's one-pass feature hygiene.
- 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.