docs(1283): name the edge exception, refresh the contract memo, pin the synth exclusion - #1353
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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
|
Warning Review limit reached
Next review available in: 43 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 (8)
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 |
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 |
Reviewer's GuideAligns documentation and tests with the actual shipped behavior of edges and the write-log contract by explicitly carving out edges as an exception to log-as-truth claims, refreshing the write-log contract memo with the ratified edge recompute contract and writer ordering details, and pinning the spine synth exclusion test to the literal source_kind value rather than the constant symbol. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
|
[claim:review:Garsecg:2026-08-05T16:11:59Z] |
…nqualified The #1309 sweep for #1283 AC1 corrected PHILOSOPHY.md's Append-only substrate note and the write-log-as-truth memo, but four other live passages still told readers that edges are a materialized projection of the log as shipped fact. That is the exact overreach #1283 exists to correct: every edge is written outside the log, so a replay reconstructs beliefs and not the graph. PHILOSOPHY.md stated the unqualified form in its determinism commitments 156 lines above its own caveat; the pointer now sits with the claims it qualifies.
The memo still read 'Until that recompute exists', which went stale when #1336 merged. It also omitted the rowid-exclusion rationale, which is the load-bearing half of the 2026-08-01 ruling: the writer orders on (created_at, rowid), that rowid is implicit under 'id TEXT PRIMARY KEY', VACUUM may renumber it and it survives no rebuild — which is why the ratified key is the log's ULID. States what shipped rather than implying more: 'aelf spine verify' covers TEMPORAL_NEXT only, reproduces ~93.7% against a ~95.0% ceiling under the current writer, and reports the gap rather than closing it. Reaching the 98.70% structural ceiling needs the unfunded writer change, so points 3 and 4 stay edge-incomplete. The pre-v2.0 'historical memo' header now notes that the ratified edge contract it carries is current.
Amended constraint (1) of #1283 names a value — source_kind = 'legacy_unknown' — chosen over the ULID-prefix detector because that one dropped 51.8% of the log. The test guarding it planted its fixture row with the same symbol the recompute reads, so changing what the constant resolves to left the assertion green while the exclusion stopped matching any row on disk. Adds a test asserting the shipped value, and plants the fixture with the literal so the row looks like the #263 migration rows it stands in for. Verified by mutation: renaming the constant's value now fails both, where before it failed neither.
4412d86 to
c48de5b
Compare
The sweep qualified four passages but left three that make the same claim, including one in its own primary target: write-log-as-truth.md's opening contract paragraph names "the typed graph" as a materialized view over the log, fifty-seven lines above the implementation-status note that contradicts it. A reader of "## The contract" never reaches the qualifier. The other two are the concepts docs, both unqualified and both more load-bearing than the ones already fixed. ARCHITECTURE.md asserts every retrieval result is bit-identical given the same write log and code -- but L3 is a typed-edge walk, so retrieval output is also a function of the edges table, which the log does not cover; two stores with identical logs can return different L3 results. Its enrichment-boundary block then lists "derived edges" among outputs stored with provenance, when derived_edge_ids is NULL on every row. COMPARISON.md is user-facing and says to tear the DB down and rebuild from the log alone, one row above selling the typed graph as the conflict-handling mechanism. Also points the 93.68% at #1356, which is funded to change the denominator. Writing a figure already scheduled to move, without naming its successor, is the same staleness this PR exists to fix. The historical/ v2 memos (v2_view_flip, v2_replay, v2_derivation_worker) carry the same claim at its origin and are deliberately left alone -- they are marked historical and describe intent at the time.
Two defects in the new docstring, neither affecting behaviour. "The prefix/density detector" is a compound label that appears in no source and conflates two different rejected rules with two different numbers: the ULID-prefix date-cluster detector dropped 51.8% of the log, while "ULID prefix disagrees with ts by more than a day" is the 34.75% one. Density was explicitly never a rule. The PR body gets this right; only the docstring was loose. Also records the denominator, since 51.8% is over the whole ~139.6k-row log rather than the ~60.5k rows the rule sees per belief -- re-deriving it at the rule's site gives a different number and reads as a defect that is not there. "Every other test here plants its fixture row with the same symbol" was one test, and this PR converts it, so the sentence describes a condition it removes. Restated as what the fixture does and why. Adds the caveat that this assertion is a contract pin rather than a behavioural test -- it would hold even if the recompute were broken. The behavioural catch is the fixture literal, and that distinction is worth stating where someone might otherwise read the pin as coverage.
Review — verified, two commits pushed to this branchThree claims independently checked, all three hold. Two defects found and fixed here rather than handed back; one scope call left for you. What I verifiedThe mutation claim reproduces exactly. Baseline 20 passed; renaming It is stronger than the PR body says. With the pre-PR test file and the mutation applied I ran the whole suite — nothing anywhere in ~7,200 tests caught it. The tautology was total, not merely local to this file. The figures all trace to a source, and the right variant is used. There are two circulating pairs for the #1283 headline: 93.7%/7.4% and 94.7%/7.3%. This PR uses 93.7%/7.4%, which is correct — the other was retracted by the owner in a dedicated #1283 comment ("Corrected figures for the log-derived ruling"), the defect being that its baseline re-sorted the belief table instead of reading
Fixed in
|
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 61d96fd → |
|
[release:review:Garsecg:2026-08-05T22:49:13Z] |
1 participant
Three corrections found by verifying #1283's acceptance criteria against
mainrather than against the issue's comment history. All three are cases where a
document or a test asserts coverage that the code does not have — which is the
same defect #1283 was filed about, so leaving them in place while closing the
issue would have been self-refuting.
1.
docs:the log-as-truth claim was still unqualified in four placesThe #1309 sweep for AC1 corrected
PHILOSOPHY.md's Append-only substrate noteand the
write-log-as-truthmemo. Four other live passages still told readersthat
edgesare a materialized projection of the log as shipped fact:docs/user/LIMITATIONS.mdbeliefs/edgesa materialized projection of the log shipped under #265"docs/design/feature-doc-linker.mdingest_logis the source of truth, materialised tables (beliefs, edges, …) are derived" — under a header reading Status: implementation shippeddocs/concepts/HARNESS_INTEGRATION.mddocs/concepts/PHILOSOPHY.mdThe last one is the sharpest:
PHILOSOPHY.mdstated the unqualified form in itsdeterminism commitments 156 lines above its own caveat. The pointer now sits
with the claims it qualifies.
2.
docs(design):the contract memo said the recompute does not existwrite-log-as-truth.mdstill read "Until that recompute exists, points 3 and 4below are also edge-incomplete". That went stale when #1336 merged.
It also omitted the rowid-exclusion rationale, which is the load-bearing half of
the 2026-08-01 ruling: the writer orders on
(created_at, rowid), thatrowidis implicit under
id TEXT PRIMARY KEY,VACUUMmay renumber it, and itsurvives no rebuild — which is why the ratified key is the log's ULID rather
than anything read off the belief table.
git grep rowidover that memopreviously returned nothing.
The replacement states what shipped without implying more:
aelf spine verifycovers
TEMPORAL_NEXTonly, reproduces ~93.7% against a ~95.0% ceiling underthe current writer, and reports the gap rather than closing it. Points 3 and
4 stay edge-incomplete.
Deliberately not done: adding
aelf spine verifytodocs/user/COMMANDS.md.spineis registered withhelp=argparse.SUPPRESSandSLASH_COMMANDS.mdnamesit among the hidden operator subcommands; no other hidden verb is documented
there. Surfacing it would contradict that standing policy, so the command is
documented in the design memo where the contract lives.
3.
test(spine):the synth exclusion was pinned to a symbol, not a valueAmended constraint (1) names a value —
source_kind = 'legacy_unknown'—chosen over the ULID-prefix detector because that one dropped 51.8% of the log.
The guarding test planted its fixture row with
source_kind=SYNTH_SOURCE_KIND,the same symbol
spine_recompute.py:139reads, so changing what the constantresolves to left the assertion green while the exclusion stopped matching any row
on disk.
Adds a test asserting the shipped value and plants the fixture with the literal.
Verified by mutation rather than by inspection — renaming
INGEST_SOURCE_LEGACY_UNKNOWN's value tolegacy_unknown_RENAMED:Both fail where previously neither would have.
Scope
Docs and one test. No behaviour change, no defaults moved, no published number
touched. The remaining #1283 criteria — populating
derived_edge_ids, pinningthe detector thresholds, and the gap meter's constraint (2) — are being carved as
named leaf issues under #1157 per the operator ruling, and are not in this PR.
Summary by Sourcery
Clarify that edges are not currently log-derived while beliefs are, and pin the synthetic source-kind exclusion to the literal contract value.
Documentation:
aelf spine verifyrecompute tool.Tests: