fix(spine): report both directions of the divergence and correct its denominator (#1356) - #1378
Conversation
Reviewer's GuideExtends the spine divergence meter to report both directions of divergence and corrects the reproduced_share denominator by excluding fan-in>1 successors, adds a regression guard and committed baseline for the fan-in surplus, updates CLI/text output and docs accordingly, and introduces a benchmark script plus tests to lock in the new behavior and published figures. Flow diagram for corrected spine divergence and denominatorflowchart LR
A[shipped TEMPORAL_NEXT edges] --> C[compute fan_in per successor]
B[recomputed TEMPORAL_NEXT edges] --> D[set operations]
C --> E{fan_in == 1?}
E -->|yes| F[eligible = shipped edges with fan_in 1]
E -->|no| G[fan_in>1 successors counted
n_fan_in_successors]
F --> H[n_eligible_shipped = |eligible|]
F --> I[n_eligible_reproduced = |eligible ∩ recomputed|]
D --> J[n_recomputed_only = |recomputed - shipped|]
H --> K[reproduced_share
= n_eligible_reproduced /
n_eligible_shipped]
I --> K
style G stroke-dasharray: 3 3
style J stroke-dasharray: 3 3
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
robotrocketscience
added
the
attn:review
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="benchmarks/spine_fan_in_baseline.py" line_range="101-105" />
<code_context>
+ print(f"{k:<26} {v}")
+
+ baseline = load_baseline()
+ observed = measured["n_fan_in_successors"]
+ recorded = baseline["figures"]["n_fan_in_successors"]
+ print()
+ print(f"fan-in successors: observed {observed} vs baseline {recorded}")
+ if observed > recorded:
+ print("REGRESSED — the fan-in surplus grew; the writer defect widened.")
+ else:
</code_context>
<issue_to_address>
**suggestion:** Reuse `fan_in_regressed_against` instead of reimplementing the regression check
This regression check duplicates the logic in `fan_in_regressed_against` in `spine_recompute.py`. Please call that helper instead (or extract it to a shared module if needed) so the regression definition remains centralized and consistent if we later change the rule (e.g., equality vs. tolerance).
Suggested implementation:
```python
baseline = load_baseline()
observed = measured["n_fan_in_successors"]
recorded = baseline["figures"]["n_fan_in_successors"]
print()
print(f"fan-in successors: observed {observed} vs baseline {recorded}")
if fan_in_regressed_against(observed, recorded):
print("REGRESSED — the fan-in surplus grew; the writer defect widened.")
else:
print("OK — non-increasing.")
```
To fully implement the suggestion, also:
1. Import the shared helper from `spine_recompute.py`, e.g.:
- If `benchmarks` is a package: `from .spine_recompute import fan_in_regressed_against`
- Otherwise (module-level script): `from spine_recompute import fan_in_regressed_against`
2. Ensure the signature of `fan_in_regressed_against` matches this usage (likely `fan_in_regressed_against(observed, recorded)`); if it instead expects a structure (e.g. full `measured` and `baseline` dicts), adjust the call accordingly so the regression criterion is centralized in that helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:Garsecg:2026-08-06T00:11:56Z] |
|
[claim:review:Gylf:2026-08-06T00:20:03Z] |
|
[release:review:Gylf:2026-08-06T00:20:07Z] |
7b97720 to
f52733a
Compare
Review — all three gaps are real fixes, but the meter's own headline line does not divide, and the third gap is enforced by nothingThe diagnosis is right on all three counts and the hard part is done well. Two things in particular: refusing to drop only the missed edge of a fan-in successor is the correct reading and the arithmetic argument for it is sound; and catching your own "the correction raises it" claim against the CLI fixture before shipping it, then keeping the data-dependence in the docstring, is the discipline this repo keeps asking for. I pushed four commits for what I could fix correctly. Four findings need you. Everything below was reproduced against the committed baseline JSON or run locally. Where a claim rested on a mutation I cleared 1.
|
| commit | |
|---|---|
c75e2c9b |
print the eligible numerator the share is actually over; fix the assertion and the comment that contradicted it |
00500349 |
read the store read-only through its WAL instead of copying the main file; call the shipped fan-in predicate and export it; exit 1 on regression |
f52733a6 |
pair 94.86% with this store's own 93.69%, name the #1336 figure separately, scope the 98.70% ceiling to its denominator |
Full suite green (7244 passed); discretion grep on added lines clean. Also rebased onto current main — it had fallen behind while this was open — so the branch is FF and merge-ready.
Unrelated, found while reading this
triple_extractor is a second TEMPORAL_NEXT writer — 4 of its 25 patterns (follows, comes after, is after, succeeds) mint it directly. Those rows are in shipped and can never be in recomputed, so they are inside this PR's eligible denominator, charging the key for edges a different writer produced — the same shape as the fan-in defect one layer down. I have not adjusted for it here because I cannot separate them without a writer column. Filed as #1379, which also covers the sharper consequence: aelf spine clear deletes them by type and the backfill rebuilds them reversed, with the row count unchanged.
robotrocketscience
added
attn:unblock
|
[release:review:Garsecg:2026-08-06T00:37:40Z] |
|
[claim:review:Kulili:2026-08-06T01:07:34Z] |
…denominator Three gaps against the ratified constraint (2) on #1283 AC4. The meter iterated `shipped - recomputed` only, so links the recompute produces that the shipped spine lacks were never named — 2,102 of them on the development store. A one-directional meter renders "missed links" and "invented links" identically, and those have opposite fixes. The fan-in surplus sat in the share denominator. A chain gives each successor exactly one predecessor, so on a fan-in-2 successor the recompute can only ever reproduce one of the two shipped edges; leaving them in charges the candidate key for a writer defect it cannot express. Both edges are now excluded, not just the missed one — dropping only the miss moves numerator and denominator by different amounts and inflates the share instead of correcting it. The test separates those two readings; a single-session fixture cannot, since both give 1.0. "Non-increasing" existed only as prose with no baseline committed anywhere, so nothing could assert it. `fan_in_regressed_against` plus a committed baseline and its re-derivation script close that. Equal is not a regression: the surplus is a standing writer defect this issue measures rather than fixes. Refs #1356.
reproduced_share was published as 93.68% against a denominator that included the fan-in surplus. Under the corrected fan-in-1 eligible denominator the same store reports 94.86%. Both figures now carry the statement that they are not comparable, and the successor ships with the script that re-derives it, per the project rule on published numbers. States the direction as data-dependent rather than fixed: the share rises when the excluded edges were reproducing worse than the overall rate and falls when better. It rises here and falls on the module's own CLI fixture, so "the correction raises it" would have been wrong as a general claim. Refs #1356.
…y over
`reproduced_share` is over the fan-in-1 eligible subset, so its numerator is
`n_eligible_reproduced`. The report printed `n_reproduced` — the count over all
shipped edges — beside that share and its eligible denominator, giving a line
that does not divide. On the development store it read
reproduced : 39,335 (94.86% of the 40,892 fan-in-1 eligible)
and 39,335 / 40,892 is 96.19%. The share's real numerator, 38,789, appeared
nowhere in the output, so the one surface an operator runs could not re-derive
the number this PR publishes — which is what the two new fields are documented
to make "inspectable in the report rather than implied".
Both counts are worth having, so both are printed against the denominator each
belongs to.
The existing assertion pinned the slip rather than catching it: it asserted
`4 (33.33% ...)` under a comment reading "4 of 6 fan-in-1 eligible", and 4/6 is
66.67%. The fixture's eligible numerator is 2 — of its 4 reproduced edges only
b<-a and z<-y have a fan-in-1 successor. Mutation-verified.
…n predicate Three defects in the baseline script, all of which let it report a pass it had not established. `measure()` took `shutil.copy` of the store path. The store runs in WAL mode, so every commit since the last checkpoint lives in the uncopied `-wal` file: on a live store the script measured a valid but stale snapshot, and a fan-in count short of the baseline prints "OK — non-increasing" off figures that were never the store's. The copy existed only because opening a store runs migrations, and `read_only=True` is the supported answer to that — it is what `aelf spine verify` already does, so both now read the store the same way and through the same WAL. `fan_in_regressed_against` had no caller anywhere. The comparison that actually ran was an inline `observed > recorded` in this script, untested, while the tested predicate sat unused and unexported. The script now calls it and it is in `__all__`. Equality is the whole content of the rule, so it should not have two implementations. `main()` returned 0 unconditionally, including after printing REGRESSED, so nothing could gate on it. It now exits 1 when the surplus has grown.
The entry read "published as `93.68%` (39,335 / 41,984) and is now `94.86%`", but 39,335 / 41,984 is 93.69%. The 93.68% belongs to #1336's measurement, 39,280 / 41,929 — a snapshot 55 shipped edges smaller — which the unchanged So the pair presented as a pure denominator correction spanned two store states, and about 0.01pp of the 1.18pp move was snapshot drift rather than the correction. The module's own docstring already said 93.69%, so the branch shipped both answers. Both figures now come from one `spine_divergence()` call on one store, and the The design memo's "98.70% structural ceiling" is corrected the same way: it is defined by the fan-in misses this change removes from the denominator, so it is not the ceiling for 94.86% and the memo now says so rather than inviting the subtraction it forbids two sentences earlier.
) `reproduced_share` returned 1.0 when `n_eligible_shipped` was 0. Vacuous on an empty store, but the same branch fires on a NON-empty store where every successor carries fan-in > 1: the meter then reported 100% reproduced having compared nothing. That is missing evidence rendered as agreement -- the #1360/#1361 shape -- and the existing CLI test pinned it with a comment naming the defect rather than fixing it. The property returns None and `aelf spine verify` prints 'n/a -- no fan-in-1 eligible edges to compare'. Neither 0.0 nor 1.0 is available as a fallback: both are claims about fidelity that nothing measured. The four fields this PR added also defaulted to 0, so a `SpineDivergence` built without them reported perfect fidelity while carrying a non-zero `n_shipped`. Only one construction site exists and it passes all four, so the defaults bought nothing and made the most flattering answer the fallback. They are now required. Mutation-verified: restoring the 1.0 turns the new assertion red.
…tities (#1356) The consistency test constrained `reproduced_share` against its own numerator and denominator and nothing else, leaving `n_fan_in_successors` -- the single figure the non-increasing constraint reads -- and all three miss buckets free. So 'a hand-edited or stale baseline fails that' was not true: edit the fan-in count alone and the test stayed green. Two identities that hold exactly on the committed figures close it. A fan-in-n successor removes all n of its shipped edges from the eligible set and n >= 2, so n_shipped - n_eligible_shipped >= 2 * n_fan_in_successors (1092 >= 1092 here, so every one is fan-in 2). And the three miss buckets partition what was not reproduced (2100 + 546 + 3 == 41984 - 39335). Mutation-verified in both arms: n_fan_in_successors -> 999 goes red, and so does moving missing_other alone.
Both are statements the printed output makes about its own buckets, and both are falsifiable as written. 'this exceeds the successor count only where fan-in > 2' is false at fan-in 2: a fan-in-n successor loses n-1 edges when the recompute picks one of its shipped predecessors and all n when it picks neither, so two misses from a single fan-in-2 successor is reachable. 'the only bucket a key disagreement moves' overclaims. The fan-in exclusion is one-sided -- a recomputed chain is injective on dst as well as src, so two shipped edges sharing a predecessor are as guaranteed a miss as fan-in > 1, yet they stay in the eligible denominator and land in `other`. Wording only; the denominator is left alone deliberately, because the fan-in rule is ratified and a symmetric exclusion would move the published number again.
…#1356) `--write` replaced `baseline["figures"]` and left the top-level `measured_at` alone, so a freshly measured baseline carried the previous measurement's date -- the provenance drift the file's own provenance block exists to prevent. Also makes the `reproduced_share` round None-safe, since the property can now report no share.
f52733a to
7c55770
Compare
Adopted the four open findings and fixed them — rebased onto mainThe four items left for the author had sat ~35 minutes with no push and the (a)
|
All four taken, plus both minors.
|
robotrocketscience
added
attn:review
Correction — retract the comment aboveThat comment is wrong on two counts and I am withdrawing it.
And the work was already done — by you, before I started it. Reading the So there is nothing for me to land here. I duplicated your work because I One thing that does appear to be still open — the second Leaving |
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 7c55770 → |
1 participant
Closes #1356.
aelf spine verifyshipped in #1336 without either half of the ratifiedconstraint (2) on #1283 AC4. All three gaps closed.
1. The meter was one-directional
It iterated
shipped - recomputedonly, so links the recompute produces thatthe shipped spine does not have were never named — 2,102 of them on the
development store. A meter that sees one direction cannot distinguish "the
recompute missed links" from "the recompute invented links", and those have
opposite fixes. Now reported as
n_recomputed_only.2. The fan-in surplus sat in the denominator
546 successors carry more than one predecessor edge. A chain gives each
successor exactly one, so on a fan-in-2 successor the recompute can only ever
reproduce one of the two shipped edges — the other is a guaranteed miss no key
can avoid. Leaving them in charges the candidate key for a writer defect it
cannot express, which is exactly the single depressed percentage the constraint
forbade.
Both of a fan-in successor's edges leave the denominator, not just the
missed one. Dropping only the miss moves numerator and denominator by different
amounts and inflates the share rather than correcting it. The test separates
those two readings explicitly — a single-session fixture cannot, because both
give
1.0; it takes a second session contributing an eligible miss, after which"drop both" gives
1/2and "drop only the miss" gives2/3.The published number moves
This is a denominator correction, not a movement in fidelity. Do not compare
the two. Both figures now carry that statement, in the CHANGELOG and in the
write-log memo.
The direction is data-dependent, not fixed — the share rises when the
excluded edges were reproducing worse than the overall rate and falls when
better. Here they reproduced at 50% against an overall 93.7%, so it rises; on
this module's own CLI fixture the same correction moves 40% → 33.33%. I had
written "the correction raises it" before checking the fixture, and that would
have shipped as a false general claim.
Per the project rule that published numbers ship their script,
benchmarks/spine_fan_in_baseline.pyre-derives every figure and writesbenchmarks/spine_fan_in_baseline.json. It copies the store before opening it,because
MemoryStore.__init__runs migrations and a lifecycle sweep — opening alive store is a write.
3. "Non-increasing" existed only as prose
It appeared in
spine_recompute's docstring and in printed output, with nobaseline committed anywhere, so nothing could assert it.
fan_in_regressed_againstnow compares against the committed baseline.Equal is not a regression — the surplus is a standing writer defect this issue
measures rather than fixes, so holding steady is the expected state and only
growth is signal. Asserted in all three arms (grew / held / shrank), because the
two failure modes are opposite:
>=flags the steady state, and a percentagetolerance lets real growth through.
CI cannot re-derive the baseline — the store it was taken on is not a shipped
fixture — so the committed JSON is instead checked for internal consistency: its
recorded share must follow from its own numerator and denominator, its eligible
set must be a strict subset, and it must not equal the pre-correction figure. A
hand-edited or stale baseline fails that.
Verification
n_recomputed_onlyhardcoded to 0 → caught>=→ caught by theheldarm alonereproduced_sharereverted to the uncorrected denominator → caughtdown, and the arithmetic is self-consistent: the 546 fan-in-2 successors
contribute 1,092 shipped edges, of which exactly 546 reproduce — one per
successor, as a chain requires — which is precisely
missing_fan_in.reproducedline now names theeligible denominator, which is a deliberate contract change rather than a
loosened assertion.
Not in scope
Closing the gap to the 98.70% structural ceiling still needs the writer to order
on the same durable key, which remains unfunded. This is a meter, not a rebuild.
Summary by Sourcery
Adjust spine divergence reporting to account for both recomputed-only edges and fan-in-related writer defects, and publish a corrected, script-derivable reproduction share baseline.
New Features:
Enhancements:
Tests: