feat: temporal spine — ingest-time chronological edges + dedicated retrieval lane, default-off (#1064) - #1069
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
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR introduces a "temporal spine" feature: ingest-time chronological belief chaining via TEMPORAL_NEXT edges, a default-off retrieval lane, a backfill/doctor CLI subcommand, config flags, an ablation benchmark, design documentation, and unit tests. ChangesTemporal Spine Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/aelfrice/cli.py (1)
4590-4618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
--jsonfor scriptability.Other operator-facing subcommands (
doctor,health,review) support--jsonfor machine consumption. Sincespine backfillis meant to be invoked by the default-ON flip release (per the docstring), a JSON output mode would make it easier to script/verify programmatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/cli.py` around lines 4590 - 4618, Add a machine-readable output mode to `_cmd_spine` so it matches other operator-facing commands like `doctor`, `health`, and `review`. Update the `aelf spine backfill` CLI wiring to accept `--json`, and in `_cmd_spine` emit the `backfill_temporal_spine` report as JSON when that flag is set instead of the current human-readable summary. Keep the existing text output as the default, and make sure the new option is discoverable alongside the existing `dry_run` behavior.src/aelfrice/temporal_spine.py (1)
302-318: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBackfill commits one edge at a time; consider a single transaction for large stores.
backfill_temporal_spinecallsstore.insert_edgeper consecutive pair, and eachinsert_edgeruns its owncommit()(plus invalidation-callback fire). On a large pre-existing store this is O(N) fsync-bearing commits for a one-shot migration, which can makeaelf spine backfillnoticeably slow. Since this isMemoryStore-internal, batching all inserts into one transaction/commit would need a small store-side helper — worth considering if the migration is expected to run against sizable on-disk DBs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/temporal_spine.py` around lines 302 - 318, The backfill in backfill_temporal_spine is issuing store.insert_edge for each adjacent belief pair, which triggers a commit and callback per edge. Update the migration to batch these inserts into a single store-side transaction/commit path for large runs, ideally by adding or using a MemoryStore helper that can insert multiple Edge records at once. Keep the existing logic in backfill_temporal_spine that iterates session_belief_ids_ordered, but route writes through the batch helper instead of calling insert_edge repeatedly.benchmarks/temporal_spine_ablation.py (1)
160-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit
strict=tozip(B905).
srcs/dsts/weightsare built from the samerowsso lengths match today;strict=Truedocuments that invariant and fails loudly if the extraction ever diverges.♻️ Suggested tweak
- for src, dst, weight in zip(srcs, dsts, weights): + for src, dst, weight in zip(srcs, dsts, weights, strict=True):The other Ruff/ast-grep hints here (
randomS311,/tmpdefault S108, path-traversal on--out) are expected for a deterministic, dev-only benchmark and don't warrant changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/temporal_spine_ablation.py` around lines 160 - 168, Add an explicit strict argument to the zip loop in the temporal spine ablation benchmark. In the edge-writing loop that iterates over srcs, dsts, and weights, update the zip call to use strict=True so the invariant is enforced and any length mismatch fails loudly. Use the existing loop in the benchmark function that writes into edges to locate the change.Source: Linters/SAST tools
src/aelfrice/retrieval.py (1)
3281-3286: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid counting all edge types just to check for
TEMPORAL_NEXT.
store.count_edges_by_type()runsSELECT type, COUNT(*) FROM edges GROUP BY type, and there’s noedges(type)index, so this guard still scans the whole table. Use anEXISTS/LIMIT 1probe ontype = EDGE_TEMPORAL_NEXThere instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aelfrice/retrieval.py` around lines 3281 - 3286, The temporal spine guard in retrieval logic is still doing a full grouped edge count through store.count_edges_by_type() just to verify EDGE_TEMPORAL_NEXT exists. Update the condition in the retrieval path to use a direct existence probe against the edges table for type = EDGE_TEMPORAL_NEXT (for example via an EXISTS or LIMIT 1 helper) so the check avoids scanning all edge types. Keep the change localized around the is_temporal_spine_enabled/query/l1_packed guard and reuse the existing EDGE_TEMPORAL_NEXT symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@benchmarks/temporal_spine_ablation.py`:
- Around line 160-168: Add an explicit strict argument to the zip loop in the
temporal spine ablation benchmark. In the edge-writing loop that iterates over
srcs, dsts, and weights, update the zip call to use strict=True so the invariant
is enforced and any length mismatch fails loudly. Use the existing loop in the
benchmark function that writes into edges to locate the change.
In `@src/aelfrice/cli.py`:
- Around line 4590-4618: Add a machine-readable output mode to `_cmd_spine` so
it matches other operator-facing commands like `doctor`, `health`, and `review`.
Update the `aelf spine backfill` CLI wiring to accept `--json`, and in
`_cmd_spine` emit the `backfill_temporal_spine` report as JSON when that flag is
set instead of the current human-readable summary. Keep the existing text output
as the default, and make sure the new option is discoverable alongside the
existing `dry_run` behavior.
In `@src/aelfrice/retrieval.py`:
- Around line 3281-3286: The temporal spine guard in retrieval logic is still
doing a full grouped edge count through store.count_edges_by_type() just to
verify EDGE_TEMPORAL_NEXT exists. Update the condition in the retrieval path to
use a direct existence probe against the edges table for type =
EDGE_TEMPORAL_NEXT (for example via an EXISTS or LIMIT 1 helper) so the check
avoids scanning all edge types. Keep the change localized around the
is_temporal_spine_enabled/query/l1_packed guard and reuse the existing
EDGE_TEMPORAL_NEXT symbol.
In `@src/aelfrice/temporal_spine.py`:
- Around line 302-318: The backfill in backfill_temporal_spine is issuing
store.insert_edge for each adjacent belief pair, which triggers a commit and
callback per edge. Update the migration to batch these inserts into a single
store-side transaction/commit path for large runs, ideally by adding or using a
MemoryStore helper that can insert multiple Edge records at once. Keep the
existing logic in backfill_temporal_spine that iterates
session_belief_ids_ordered, but route writes through the batch helper instead of
calling insert_edge repeatedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ba172a1-a0d8-40d3-ac7a-309bad92a706
📒 Files selected for processing (11)
CHANGELOG/v3.mdbenchmarks/temporal_spine_ablation.pydocs/design/feature-temporal-spine.mddocs/user/CONFIG.mdsrc/aelfrice/cli.pysrc/aelfrice/ingest.pysrc/aelfrice/retrieval.pysrc/aelfrice/store.pysrc/aelfrice/temporal_spine.pytests/test_slash_commands.pytests/test_temporal_spine.py
|
[claim:review:Kulili:2026-07-04T07:05:14Z] |
|
Reviewed the full diff (4 atomic signed commits, 11 files). Verified:
Non-blocking notes:
FF on main, all commits signed. Labeling |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Kulili:2026-07-04T07:09:33Z] |
|
[claim:review:garsecg:2026-07-04T07:10:00Z] |
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
robotrocketscience
left a comment
robotrocketscience
left a comment
There was a problem hiding this comment.
Reviewed the full diff, verifying claims at call sites:
- Lane guard order is right:
is_temporal_spine_enabled(...)short-circuits before any store call, so the default-off path pays zero; the no-op guard (count_edges_by_type) keeps spineless stores byte-identical when the flag IS on. Seeds are the top-5 packed L1 beliefs; spine hits append under the token budget after L1 (never displacing), dedup against locked/L2.5/L1/HRR-expand, and deliberately don't seed BFS — all matching the #1064 lane-not-gate rationale. - Bidirectional traversal is sound:
edges_for_beliefs(pre-existing) matchessrc OR dst, so both successor and predecessor directions resolve; ordering (successors-then-predecessor, id-sorted within group) is deterministic; budget counts emitted only. - Writer/backfill: idempotent per
(src,dst,type);session_predecessor_idorders by(created_at, rowid)with correct tie-break SQL; backfill==writer equivalence is pinned by test;--dry-runtouches nothing. - Wiring complete: env → kwarg → TOML → False on both flags (unrecognised env falls through, tested);
HIDDEN_SUBCOMMANDSregistration present; doctor row; CONFIG.md keys; telemetry carries candidates-vs-packed so the G2 trim question is readable per call. - Independent verification: fetched the PR head;
test_temporal_spine.py+test_slash_commands.pypass locally in a fresh dev venv (194 passed, 1 skipped). CI matrix green; 0 unresolved review threads; discretion grep on the diff clean (one hit is a pre-existing context line).
Two non-blocking notes for the flip-time revisit, not this PR:
- At the default depth-1, skip-but-continue can't actually traverse past a soft-deleted neighbour (it joins the next frontier but the loop ends). Chain integrity across GC'd segments effectively needs depth ≥2 — worth one line in the design doc when the budget/depth knobs get revisited at flip time.
benchmarks/temporal_spine_ablation.pyimports the privateingest._ingest_turn_ids; fine for a bench, but if that seam ever changes the bench is the only breakage surface — a public re-export would decouple it.
LGTM on content — but hold the label: main moved 4 commits under this branch (the #1068 hook landed), so the FF-only merge-train will bounce it as-is. After a rebase onto current github/main + green rerun, add ready-to-merge; happy for anyone to label it then, no re-review needed for a clean rebase.
|
[release:review:garsecg:2026-07-04T07:14:09Z] |
|
[claim:review:Gylf:2026-07-04T07:18:01Z] |
|
Incremental review of 0110e5e (pushed one minute before the prior review landed, so pinning down that the delta is covered):
One flip-time note (non-blocking, same bucket as the prior review's two): Status: content approved — prior review plus this delta covers the full branch. |
robotrocketscience
added
attn:merge-conflict
|
[release:review:Gylf:2026-07-04T07:22:20Z] |
|
[claim:review:Kulili:2026-07-04T07:25:30Z] |
0110e5e to
d4897d5
Compare
…t, default-off (#1064) Ingest-time chronological spine: each newly inserted belief links to its session predecessor (created_at order, insertion-order tie-break) with a TEMPORAL_NEXT edge, src=successor dst=predecessor weight 0.8. New store.session_predecessor_id() accessor does the O(log n) indexed lookup. Wired into _ingest_turn_ids behind is_temporal_spine_write_enabled (AELFRICE_TEMPORAL_SPINE_WRITE env > kwarg > [ingest] write_temporal_spine TOML > False) so the off-path is byte-identical to today. Idempotent per (src,dst,type); soft-deleted predecessors stay eligible so chain integrity survives GC.
backfill_temporal_spine walks every session's beliefs in (created_at, rowid) order and links consecutive pairs with the same TEMPORAL_NEXT edge the ingest writer produces — idempotent per (src,dst,type), with a --dry-run counting mode. Hidden 'aelf spine' subcommand (migration surface, not a workflow verb; registered in HIDDEN_SUBCOMMANDS). aelf health / doctor graph scope gains a 'temporal spine: present (N edges)/absent' row. Equivalence test pins backfill output == writer output on the same corpus.
…bench, default-off (#1064) Additive candidate source after L1 in retrieve_with_tiers: traverses TEMPORAL_NEXT chains from the top-5 packed L1 seeds via temporal_spine.spine_neighbors (bidirectional, depth 1 default, node budget 32 default; deterministic ordering, soft-deleted beliefs skip-but-continue). Appended after L1, never displacing it pre-packing; no-op guard skips the traversal on stores with zero TEMPORAL_NEXT edges so spineless output stays byte-identical. Spine hits do not seed BFS (unmeasured surface; the #1064 evidence ran depth-1 append-after-L1). Flag: AELFRICE_TEMPORAL_SPINE env > use_temporal_spine kwarg > [retrieval] use_temporal_spine TOML > False; node budget tunable via AELFRICE_TEMPORAL_SPINE_BUDGET / temporal_spine_budget. LaneTelemetry gains temporal_spine (packed survivors) + temporal_spine_candidates (pre-pack discoveries) so the G2 trim-loss question is readable per call. benchmarks/temporal_spine_ablation.py scores gold-evidence coverage on LoCoMo across baseline / +spine / seeded shuffled-control arms (the chronology-vs-density isolate).
docs/design/feature-temporal-spine.md records the mechanism, the dev/confirmatory coverage evidence, the lane-not-gate rationale vs G1-G5. CONFIG.md documents [ingest] write_temporal_spine and [retrieval] use_temporal_spine / temporal_spine_budget (schema block + detail sections). CHANGELOG [Unreleased] entry for the feature wave.
…1064) count_edges_by_type() is a full GROUP BY over the edges table — 4.15ms per call measured on a 24k-edge production store, nearly the whole G3 p50 gate (5ms) spent before the lane does any work. New store.has_edge_type() is a LIMIT-1 existence probe: ~0ms when a spine edge exists (the lane's common on-path), bounded by one scan when none does. Paired alternating min-of-7 measurement on a 26k-belief store copy: lane delta within ±5ms on the heaviest-firing queries (8-10 packed spine hits).
d4897d5 to
5dac4de
Compare
robotrocketscience
added
the
ready-to-merge
5dac4de to
fe3f29c
Compare
|
merge-train: blocked branch head moved during merge-train queue (event= The |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged fe3f29c → |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Kulili:2026-07-04T07:37:48Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged fe3f29c → |
1 participant
Lands deliverables 1–4 of #1064 — the temporal spine, everything default-OFF. Four atomic commits:
feat(temporal_spine)— spine writer. After each belief insert, link to the session predecessor (created_atorder, insertion-order tie-break) withTEMPORAL_NEXT, src = successor, dst = predecessor, weight 0.8. Newstore.session_predecessor_id()does the indexed O(log n) lookup. Wired into_ingest_turn_ids(covers bothingest_turnandingest_jsonl) behindAELFRICE_TEMPORAL_SPINE_WRITEenv → kwarg →[ingest] write_temporal_spineTOML → False, mirroring the feat(ingest): build the semantic-edge substrate via deterministic relationship-detection at ingest (default-off) — the real LoCoMo lever; reframes #981/#977 #988 writer's posture: off-path ingest is byte-identical. Idempotent per(src, dst, type); soft-deleted predecessors stay eligible so chains never sever under GC.feat(cli)—aelf spine backfill+ doctor row. Idempotent per-session chain build over existing stores (--dry-runsupported), hidden subcommand (migration surface, registered inHIDDEN_SUBCOMMANDS).aelf health/doctorgraph scope reportstemporal spine: present (N edges) / absent. Equivalence pinned by test: backfill output == writer output on the same corpus. Live-store dry-run: 21,476 edges across 521 sessions ≈ 0.98 edges/belief, matching the issue's ~1.0 estimate.feat(retrieval)— dedicated lane + telemetry + ablation bench. Additive candidate source after L1 inretrieve_with_tiers: traversesTEMPORAL_NEXTfrom the top-5 packed L1 seeds viaspine_neighbors(bidirectional, depth 1, node budget 32; deterministic ordering;valid_toskip-but-continue). Appended after L1, never displacing it pre-packing; no-op guard viacount_edges_by_type()keeps spineless stores byte-identical. Flag:AELFRICE_TEMPORAL_SPINE→use_temporal_spinekwarg →[retrieval] use_temporal_spine→ False; budget viaAELFRICE_TEMPORAL_SPINE_BUDGET/temporal_spine_budget.LaneTelemetrygainstemporal_spine(packed survivors) +temporal_spine_candidates(pre-pack) so the G2 trim-loss question is readable per call.benchmarks/temporal_spine_ablation.pyscores gold-evidence coverage on LoCoMo (baseline / +spine / seeded shuffled-control — the chronology-vs-density isolate).docs—docs/design/feature-temporal-spine.md(mechanism, evidence, lane-not-gate rationale vs Adaptive expansion-gate: skip BFS/HRR-expensive lanes on broad prompts #741/Benchmark the gated retrieval lanes ON; revisit conservative default-off flags (BFS, type-aware compression, γ/ζ rerank) #977/decide: denser/typed semantic-edge substrate beyond CONTRADICTS-only for HRR-expand/BFS lanes #998-A4, pre-registered flip-gate criteria G1–G5), CONFIG.md keys, CHANGELOG[Unreleased]entry.Two deliberate deviations from strict #981-lane symmetry, both documented in code: spine hits do not seed BFS (the confirmatory evidence measured depth-1 append-after-L1 with BFS untouched; feeding BFS is unmeasured surface), and the lane lives in
retrieve_with_tiers/retrieve_v2only (same scope as the #981 lane — hook-path wiring is G2 flip-time work).Deliverable 5 (default-ON flip) is intentionally not here: gated on G2–G5 per the issue.
Tests: 35 new in
tests/test_temporal_spine.py(flag precedence, ordering contract incl. rowid tie-break, writer/backfill idempotency + equivalence, traversal budget/depth/soft-delete, lane on/off/no-op-guard/env/budget viaretrieve_v2); full suite 5601 passed locally.Part of #1064 — deliberately not
Closes: the issue stays open tracking the G2–G5 flip gate (deliverable 5). If the operator prefers close-on-merge + a fresh flip-gate issue, say the word and I'll file it.Summary by CodeRabbit
New Features
Bug Fixes