feat(temporal_spine): flip defaults ON (#1064) — HOLD: merge after #1090 + operator go - #1092
robotrocketscience wants to merge 4 commits into
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
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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR flips two temporal-spine feature flags from default-off to default-on: ChangesTemporal spine default-on flip
Estimated code review effort: 2 (Simple) | ~12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/aelfrice/temporal_spine.py (1)
124-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfusing parenthetical contradicts the preceding sentence.
The docstring states "Default-ON since the
#1064flip" and then immediately adds "(Landed default-off; the flip is that issue's deliverable 5...)" in the same paragraph. Read at a glance this reads as self-contradictory — a future maintainer skimming the docstring could misread the current default as off. Consider rewording to make clear this parenthetical is purely historical (original landing) vs. current state (this release).✏️ Suggested rewording
Default-ON since the `#1064` flip: every evidence gate (G1–G5) passed, so the ingest writer chains new beliefs by default. Opt out with ``AELFRICE_TEMPORAL_SPINE_WRITE=0`` or ``[ingest] write_temporal_spine - = false``. (Landed default-off; the flip is that issue's deliverable - 5, shipped as a release default change plus the auto-once backfill for - existing stores.) + = false``. (`#1064` originally landed default-off; this default-ON flip + is that issue's deliverable 5, shipped as a release default change + plus the auto-once backfill for existing stores.)🤖 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 124 - 131, The docstring in temporal_spine is confusing because the “Default-ON since the `#1064` flip” sentence is immediately followed by a parenthetical that sounds contradictory. Reword the documentation around the ingest writer default so it clearly separates the current behavior from the historical landing note, and keep the explanation near the existing AELFRICE_TEMPORAL_SPINE_WRITE / [ingest] write_temporal_spine references.src/aelfrice/retrieval.py (1)
3270-3277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "default-OFF" comment after the flip.
This block-comment above the temporal-spine traversal still says
(additive, default-OFF), butis_temporal_spine_enablednow defaults toTrueper the updated docstring/behavior in this same PR. Left as-is, it will mislead future readers about the lane's default state.📝 Suggested comment fix
- # `#1064` temporal-spine lane (additive, default-OFF). Traverses + # `#1064` temporal-spine lane (additive, default-ON since the flip). + # Traverses🤖 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 3270 - 3277, Update the block comment above the temporal-spine traversal so it no longer says “default-OFF”; align it with the current behavior where is_temporal_spine_enabled defaults to True. Keep the rest of the `#1064` commentary intact, and make the wording in retrieval.py consistent with the nearby docstring and traversal logic so future readers don’t get conflicting guidance.tests/test_temporal_spine.py (1)
487-489: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing isolation in
test_lane_default_on_flag.Unlike the sibling flag tests in this file, this test doesn't accept
monkeypatch/tmp_pathand callsis_temporal_spine_enabled()/is_temporal_spine_enabled(explicit=False)without pinningstart=or clearingAELFRICE_TEMPORAL_SPINE. A leaked env var from another test/CI config, or a real.aelfrice.tomlfound while walking up from cwd, would make this assertion non-deterministic.🧪 Suggested isolation fix
-def test_lane_default_on_flag() -> None: - assert is_temporal_spine_enabled() is True # default-ON (`#1064` flip) - assert is_temporal_spine_enabled(explicit=False) is False - assert resolve_temporal_spine_budget() == 32 - assert resolve_temporal_spine_budget(explicit=7) == 7 +def test_lane_default_on_flag(monkeypatch: pytest.MonkeyPatch, tmp_path) -> None: + monkeypatch.delenv(ENV_TEMPORAL_SPINE, raising=False) + assert is_temporal_spine_enabled(start=tmp_path) is True # default-ON (`#1064` flip) + assert is_temporal_spine_enabled(explicit=False, start=tmp_path) is False + assert resolve_temporal_spine_budget() == 32 + assert resolve_temporal_spine_budget(explicit=7) == 7🤖 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 `@tests/test_temporal_spine.py` around lines 487 - 489, `test_lane_default_on_flag` is not isolated because it relies on ambient configuration when calling `is_temporal_spine_enabled()`. Update this test to accept the same isolation fixtures used by the sibling flag tests, then explicitly clear `AELFRICE_TEMPORAL_SPINE` and pin the search root/start directory so the check cannot pick up a leaked env var or an on-disk `.aelfrice.toml`. Keep the assertions in `test_lane_default_on_flag` but make them deterministic by controlling the environment and lookup scope.
🤖 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.
Inline comments:
In `@src/aelfrice/temporal_spine.py`:
- Line 141: The stale ingest comment in the `is_temporal_spine_write_enabled()`
flow no longer matches the implementation because the fallback now returns True
instead of Default-OFF. Update the comment in `ingest.py` around the
`is_temporal_spine_write_enabled` call site to reflect the current default
state, keeping the wording aligned with the behavior in `temporal_spine.py`.
---
Nitpick comments:
In `@src/aelfrice/retrieval.py`:
- Around line 3270-3277: Update the block comment above the temporal-spine
traversal so it no longer says “default-OFF”; align it with the current behavior
where is_temporal_spine_enabled defaults to True. Keep the rest of the `#1064`
commentary intact, and make the wording in retrieval.py consistent with the
nearby docstring and traversal logic so future readers don’t get conflicting
guidance.
In `@src/aelfrice/temporal_spine.py`:
- Around line 124-131: The docstring in temporal_spine is confusing because the
“Default-ON since the `#1064` flip” sentence is immediately followed by a
parenthetical that sounds contradictory. Reword the documentation around the
ingest writer default so it clearly separates the current behavior from the
historical landing note, and keep the explanation near the existing
AELFRICE_TEMPORAL_SPINE_WRITE / [ingest] write_temporal_spine references.
In `@tests/test_temporal_spine.py`:
- Around line 487-489: `test_lane_default_on_flag` is not isolated because it
relies on ambient configuration when calling `is_temporal_spine_enabled()`.
Update this test to accept the same isolation fixtures used by the sibling flag
tests, then explicitly clear `AELFRICE_TEMPORAL_SPINE` and pin the search
root/start directory so the check cannot pick up a leaked env var or an on-disk
`.aelfrice.toml`. Keep the assertions in `test_lane_default_on_flag` but make
them deterministic by controlling the environment and lookup scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ab80cbf-020e-4a5e-95fa-7f83c6252d4c
📒 Files selected for processing (5)
CHANGELOG/v3.mddocs/design/feature-temporal-spine.mdsrc/aelfrice/retrieval.pysrc/aelfrice/temporal_spine.pytests/test_temporal_spine.py
d36e5c6 to
03c8327
Compare
All pre-registered evidence gates passed (G1-G3, G5) and G4 migration is resolved (#1090 auto-once backfill + spine clear), so both temporal-spine flags flip default-ON: - is_temporal_spine_write_enabled default False -> True (ingest chains new beliefs by default; opt out AELFRICE_TEMPORAL_SPINE_WRITE=0 / toml). - is_temporal_spine_enabled (lane) default False -> True (retrieval traverses the spine; opt out AELFRICE_TEMPORAL_SPINE=0 / toml). A store with no TEMPORAL_NEXT edges keeps the lane a no-op, so retrieval is byte-identical until a spine exists. Tests that pinned the default-off posture flip to default-on (flag defaults, lane default, ingest off-path now an explicit opt-out + a new default-on ingest test); the 'unrecognised/malformed value not decisive' tests now prove non-decisiveness against a lower rung rather than the default. CHANGELOG Changed entry added.
Status -> default-ON (flipped); flip-gate section retained as the pre-registration record.
The flip changed is_temporal_spine_write_enabled to default True, so the sentinel-gated auto-once backfill now fires on the unset/default path. test_auto_backfill_default_gate_reads_writer_flag still encoded the old default-off expectation (ran is False / no sentinel); update it to the flipped default (ran is True / sentinel written), matching the ratified #1064 G4 behavior — the backfill fires exactly once the write flag is enabled, which it now is by default. The sibling explicit-kwarg tests (runs-when-enabled, rearms-after-flip) already cover the gate both ways.
03c8327 to
51df323
Compare
…-flip (#1064) The defaults flip left the writer and lane doc-comments describing the pre-flip posture: ingest call-site, the module docstring, the backfill short-circuit note, and the two retrieval-lane comments still read Default-OFF. Update them to the post-flip default-ON wording (opt-out via env/toml). Comment/docstring-only; no logic change. Resolves the CodeRabbit staleness thread on this PR.
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked FF push to The |
github-actions
Bot
removed
the
ready-to-merge
|
Operator-directed split (2026-07-06): the writer half is being carved out into #1111. Per operator decision, the temporal-spine flip is split so the write-side ships now while the read-side waits for the retrieval cutover:
Rationale in #1111's description. Flagging so this HOLD draft isn't merged as 'both flags' — the writer half is now #1111. |
1 participant
Temporal spine → default-ON (the #1064 flip)
main— otherwise the flip delivers no historical spine for existing stores; andThis PR is opened as a draft to enforce (1)+(2). It will need a rebase once #1090 merges (the design-doc Status line reconciles against #1090's G4 edit).
What it does
Every pre-registered evidence gate passed — G1 (+14.6pp LoCoMo), G2 (production-budget trim survival + 1,986/1,986 top-rank invariance), G3 (latency delta in band), G5 (two-build determinism + ablation-green) — and G4 migration is resolved (#1090). So both flags flip default-ON:
is_temporal_spine_write_enableddefaultFalse → True— ingest chains new beliefs by default.is_temporal_spine_enabled(lane) defaultFalse → True— retrieval traverses the spine by default.Per-flag opt-out is unchanged (
AELFRICE_TEMPORAL_SPINE_WRITE=0/AELFRICE_TEMPORAL_SPINE=0, or the.aelfrice.tomlkeys). Existing stores get their historical spine via #1090's sentinel-gated auto-backfill on the firstaelf setupafter upgrade.Blast radius (measured)
Full suite: 5651 passed after updating the tests that pinned the default-off posture. The lane flip is contained because a store with no
TEMPORAL_NEXTedges makes the lane a no-op (empty-lane guard) — every retrieval test on a spineless store is byte-identical. The only fallout was intest_temporal_spine.py: default-flag assertions flipped, the ingest off-path became an explicit opt-out (+ a new default-on ingest test), and the two "unrecognised/malformed value not decisive" tests now prove non-decisiveness against a lower rung rather than the default.Verification
pytest: 5651 passed / 69 skipped / 75 xfailed.Summary by CodeRabbit