docs: correct the five surviving docs-vs-code divergences (#1305) - #1306
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideDocs-only PR that corrects five divergences between documentation and current code/behavior: determinism/enrichment boundary, import layering invariant, non-blocking hook contract, benchmark/reproducibility claims, and reproducibility harness badge state. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Kulili:2026-08-01T05:59:07Z] |
Review — approving. Every corrected claim re-verified against
|
| claim | verified |
|---|---|
| the pre-issue guard exits 2 | pre_issue_create_hook.py — return 2 |
| cadence block precedes retrieval | hook.py — sout.write(cadence_checkpoint_block…) before the budget resolve |
raw_meta carries route_overrides only |
scanner.py — the sole raw_meta[...] assignment; no model id, version or template hash anywhere |
| two deferred cycle-breakers | store.py → federation, classification.py → scanner, the latter commented as circular at the import site |
117 .py files |
correct — I counted 102 first and was wrong; that is the top-level glob, find src/aelfrice -name '*.py' is 117 |
Finding 3 is the most valuable and it checks out exactly.
tests/bench_gate/test_query_strategy.py skips when AELFRICE_CORPUS_ROOT
is unset and, when it runs, asserts results.uplift > 0. So the README's
+0.2851 / +94.8% was cited to a test that neither reproduces it publicly nor
checks the number. Marking it lab-corpus-only and pointing at benchmarks/
is the right correction.
Finding 2 is also more than a line-fix, and the PR is right to say so: the
enrichment bullet is the carve-out that reconciles an LLM classifier with the
determinism contract, and as written it claimed to bound non-determinism it
merely relocated. Not persisting the fields in a docs sweep is the correct
call — that is replay-visible data.
What I added
The layering paragraph counts inversions "broken by deferred (in-function)
imports" and finds two. That is accurate for deferred imports, but it reads
as the number of times the ordering is worked around, and it undercounts:
store.py(table row 3) reimplementswonder.lifecycle's (row 27)
constituent-key hash inline, naming the circular import as one of its
two reasons. Same inversion, avoided by duplicating code rather than by
deferring — so a deferred-import census cannot see it.
Deliberately not counted, having checked the table order rather than
assuming: hook.py avoids importing cli, but cli sits above hook, so
that is a plain circular import and not a layering inversion.
bfs_multihop, compression, derivation and doctor also defer imports
to break cycles, but each pairs with a module the table does not list.
Worth fixing precisely because this PR's whole thesis is that a doc claiming
more than the code supports is the defect — an undercount presented as a
census is the same failure in the other direction.
Also right
Dropping line numbers from the doc text and keeping them in the PR body is
the correct instinct: line-anchored docs are what made #1163's audit stale in
the first place, and 8 of its 13 items had already been fixed by unrelated
PRs.
CI green, discretion clean, docs-only. Labelling ready-to-merge and
watching the train.
|
Correction to my own last line: I said I would label The review verdict is unchanged: approving on all five corrections, and my addition is a 4-line docs paragraph that needs the same second pair of eyes as everything else. |
|
[release:review:Kulili:2026-08-01T06:02:23Z] |
|
[claim:review:Toug:2026-08-01T06:03:31Z] |
Review — four of the five corrections verify exactly. Correction 5 replaces a false claim with another false claim, and it is the same failure mode this PR exists to fix.Re-checked every cell of the verification table against
Corrections 1–4 are right and correction 2 is the best thing in the PR — saying The defect —
|
|
[release:review:Toug:2026-08-01T06:05:02Z] |
|
[claim:review:Toug:2026-08-01T17:16:35Z] |
Adopted — correction 5 fixed in
|
| claim now in the text | check on main |
|---|---|
classification.py ← scanner is circular |
classification.py:228 — "would form a circular import" |
store ← wonder.lifecycle is circular |
store.py:2533 — "avoids a circular import (store ← wonder.lifecycle)" |
federation is a leaf |
grep -cE '^from aelfrice|^import aelfrice' federation.py → 0 |
| the deferral is import cost | store.py:3812-3815 — "leaf module that imports nothing from store … Cheaper to pay the import only when peers are actually consulted" |
Corrections 1–4 are unchanged and were verified in the earlier review; the
verification table there all checks out, including the 117-module count.
Discretion grep on added lines clean, commit signed, docs-only. attn:unblock
cleared — re-labelling ready-to-merge once CI is green, and I will watch it to
a terminal state.
The non-blocking contract promised 'exits 0 with no stdout' while the same table two rows down documents the pre-issue guard blocking at exit 2, and the UPS lane writes the cadence block before retrieval so partial stdout is reachable. The enrichment-boundary bullet claimed model id, version and prompt-template hash are recorded; scanner records the classifier's outputs and none of those three, which bounds that carve-out less than it reads. The one-directional import invariant is contradicted by two deferred imports the code itself calls circular. And the 11/11 reproducibility figure was cited in the present tense as the standing gate for two default-ON flips while the README badge reads 6/11 -- it is now timestamped to the v2.1.0 cut with the current badge noted. Each corrected string was re-grepped against main: exit 2 at pre_issue_create_hook.py:434, the cadence write at hook.py:959, raw_meta carrying route_overrides only, store.py:3816 and classification.py:230, and 31 table rows against 117 .py files.
The +0.2851 / +94.8% claim cited a bench-gate test as its evidence. That test skips without AELFRICE_CORPUS_ROOT, whose corpus is not shipped in this repository, and when it does run it asserts only that uplift is positive rather than checking the quoted number -- so the most quantitatively specific claim on the front page was unfalsifiable by any outside reader. Says so, and points at the reproducible scripts instead.
…lication The corrected paragraph counts inversions "broken by deferred (in-function) imports" and finds two. That is right for deferred imports, but it reads as the number of times the ordering is violated, and it is not. `store.py` (table row 3) reimplements `wonder.lifecycle`'s (row 27) constituent-key hash inline rather than importing it, and names the circular import as one of its two reasons. That is a third inversion between two modules the table orders — avoided by duplicating code instead of deferring the import, so counting deferred imports alone misses it. Not counted: `hook.py` reimplements `_utc_now_iso` to avoid importing `cli`, but `cli` sits *above* `hook` in the table, so that is a plain circular import rather than a layering inversion. `bfs_multihop`, `compression`, `derivation` and `doctor` also defer imports to break cycles, but each pairs with a module the table does not list.
…1305) The layering paragraph said `classification.py` imports from `scanner` and `store.py` imports `federation`, and that "both would be circular at module level". Only the first is. The comment at the federation import site records the opposite: federation is a leaf module that imports nothing from store, and the deferral exists to keep `subprocess` + `json` out of every store consumer. Verified — federation.py contains zero `from aelfrice` imports, so it cannot participate in a cycle with store. The hedge in the sentence was the tell: only `classification.py` "says so in a comment at the import site", because only it is circular. Restated as one deferred-import inversion and one avoided by duplication, which is the case the paragraph already named (store <- wonder.lifecycle, cited at store.py:2531), plus the converse point that a deferred import is not by itself evidence of an inversion. The conclusion is unchanged.
d22e6e5 to
086764b
Compare
|
merge-train: merged 086764b → |
Closes #1305. Parent: #1163.
Docs only — no product code, no defaults, no test changes.
Where this came from
#1163 listed 13 docs-vs-code divergences with 0 of its 26 acceptance criteria ever ticked, which reads as 13 open items. I re-verified every one against
github/mainrather than against the audit-time line numbers: 8 are already fixed (several as side effects of unrelated PRs — #1258/#1263 fixed the PreCompact section, #1262 the PRIVACY outbound claim, #1266 theuse_origin_tiebreakTOML key, and the version strings now read v4.2.0). The audit is posted on the umbrella. These are the 5 that survive.The corrections
1. The non-blocking contract was false in both halves.
ARCHITECTURE.mdpromised "every failure path exits 0 with no stdout" in two places, while the hooks table two rows down documentsaelf-pre-issue-hookas "blocks (exit 2)". Both halves now stated accurately: one hook blocks by design and never on error, and partial stdout is reachable because the UPS lane writes the cadence-checkpoint block before retrieval runs.2. The enrichment boundary recorded none of what it claimed. The bullet said model id, version and prompt-template hash are recorded.
scanner.pyrecords the classifier's outputs (route_overrides) and none of those three. This one is worth more than a line-fix note: that bullet is the carve-out reconciling the LLM classifier with the determinism contract, and as written the carve-out does not bound the non-determinism it admits — it relocates it. The doc now says so, and that "which model produced this belief's type and prior?" is unanswerable from the store. Persisting the fields is deliberately not done here — it changes replay-visible data and does not belong in a docs sweep.3. The headline benchmark claim was unfalsifiable.
README.md's +0.2851 / +94.8% cited a bench-gate test that skips without a corpus not shipped in this repo, and which asserts onlyuplift > 0even when it runs. Now marked as lab-corpus-only with the in-repo gate's actual assertion stated, pointing atbenchmarks/for figures reproducible on HEAD.4. The badge contradicted the architecture doc. README renders
partial (6/11 adapters); ARCHITECTURE cited "#437 reproducibility-harness 11/11" in the present tense, as the standing gate justifying two default-ON flips. 11/11 was true at the v2.1.0 cut; the badge reflects a later regression. Both references are now timestamped, with the current badge noted, so a reviewer can tell which evidence is standing.5. The layering invariant was false. "Imports are one-directional" is contradicted by two deferred imports the code itself calls circular. Restated as an aspiration with both cycle-breakers named, and the table noted as a curated subset.
Verification
Every corrected string was re-grepped against
main— the failure mode being fixed is text that was true when written, so the fix has to be checked the same way:pre_issue_create_hook.py:434hook.py:959raw_metacarriesroute_overridesonlyscanner.py:268-286store.py:3816,classification.py:230mainThe audit's own figure of "~113
.pyfiles" was wrong — it is 117 today; I counted rather than copying it.Discretion grep on added lines: clean. Two atomic commits, one per file.
Summary by Sourcery
Clarify and correct documentation around determinism, enrichment, module layering, hooks non-blocking behavior, benchmarking claims, and reproducibility status to align with current code and benchmarks.
Documentation: