feat(#154): flip use_heat_kernel + use_hrr_structural to default-on - #523
Merged
Conversation
Closes the #154 composition-tracker default-on flip for the HRR structural-query lane. The lane has shipped since v1.7.0 (#152) but stayed opt-in pending the calibrated reproducibility-harness gate. That gate cleared at 11/11 yesterday via PR #489 closing #437, so the default flips per the tracker's status banner. Behavior: precedence (env > kwarg > toml > default) is unchanged; only the default value flips False → True. Opt-out paths are preserved — env var AELFRICE_HRR_STRUCTURAL=0, the kwarg, and the TOML key all still let users pin to the v2.0.x ranking. Tests: full suite (3065 passed, 51 skipped) green with the flip. The default-off assertion is replaced with a default-on assertion plus an explicit opt-out coverage test for the env / kwarg / TOML paths.
Closes the second of the two #154 composition-tracker default-on flips. The heat-kernel authority-scoring lane (#150) shipped v1.5.0–v1.7.0 but stayed opt-in pending the calibrated reproducibility-harness gate. That gate cleared at 11/11 yesterday via PR #489 closing #437, so the default flips per the tracker's status banner. Behavior: precedence (env > kwarg > toml > default) is unchanged; only the default value flips False → True. Opt-out paths are preserved — env var AELFRICE_HEAT_KERNEL=0, the kwarg, and the TOML key all still let users pin to the v2.0.x ranking. Tests: full suite (3066 passed, 51 skipped) green with both flips landed. The default-off assertion is replaced with default-on + explicit opt-out coverage.
Reviewer's GuideFlips the retrieval flags Sequence diagram for retrieval flag resolution precedencesequenceDiagram
participant RetrievalClient
participant RetrievalModule
participant EnvConfig
participant TomlConfig
RetrievalClient->>RetrievalModule: call retrieve_v2(use_heat_kernel, use_hrr_structural)
activate RetrievalModule
Note over RetrievalModule: Resolve use_hrr_structural
RetrievalModule->>EnvConfig: read AELFRICE_HRR_STRUCTURAL
alt env var set
EnvConfig-->>RetrievalModule: return truthy_or_falsy
RetrievalModule-->>RetrievalClient: structural_enabled = env_value
else env var not set
RetrievalModule-->>RetrievalClient: structural_explicit_kwarg?
alt explicit kwarg provided
RetrievalModule-->>RetrievalClient: structural_enabled = kwarg_value
else no explicit kwarg
RetrievalModule->>TomlConfig: read [retrieval] use_hrr_structural
alt toml key set
TomlConfig-->>RetrievalModule: return toml_value
RetrievalModule-->>RetrievalClient: structural_enabled = toml_value
else toml key not set
RetrievalModule-->>RetrievalClient: structural_enabled = True (default_on)
end
end
end
Note over RetrievalModule: Resolve use_heat_kernel
RetrievalModule->>EnvConfig: read AELFRICE_HEAT_KERNEL
alt env var set
EnvConfig-->>RetrievalModule: return truthy_or_falsy
RetrievalModule-->>RetrievalClient: heat_kernel_enabled = env_value
else env var not set
RetrievalModule-->>RetrievalClient: heat_kernel_explicit_kwarg?
alt explicit kwarg provided
RetrievalModule-->>RetrievalClient: heat_kernel_enabled = kwarg_value
else no explicit kwarg
RetrievalModule->>TomlConfig: read [retrieval] use_heat_kernel
alt toml key set
TomlConfig-->>RetrievalModule: return toml_value
RetrievalModule-->>RetrievalClient: heat_kernel_enabled = toml_value
else toml key not set
RetrievalModule-->>RetrievalClient: heat_kernel_enabled = True (default_on)
end
end
end
RetrievalModule-->>RetrievalClient: run lanes with resolved flags
deactivate RetrievalModule
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
author-rogue1
📝 WalkthroughWalkthroughThis PR flips two retrieval feature flags from opt-in (default false) to opt-in (default true): ChangesDefault-on flip for HRR structural and heat kernel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 left some high level feedback:
- In the new
test_use_heat_kernel_opt_out_paths_intactandtest_use_hrr_structural_opt_out_paths_intacttests, you mention env-var opt-out in the docstrings but only assert kwarg/TOML behavior; consider adding an explicit env-var override assertion to keep coverage aligned with the described opt-out surface. - The detailed historical explanation of the #154/#437 gate and 11/11 status is repeated in several docstrings and docs; consider tightening or centralizing this context to keep the configuration docs focused on current behavior and reduce repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `test_use_heat_kernel_opt_out_paths_intact` and `test_use_hrr_structural_opt_out_paths_intact` tests, you mention env-var opt-out in the docstrings but only assert kwarg/TOML behavior; consider adding an explicit env-var override assertion to keep coverage aligned with the described opt-out surface.
- The detailed historical explanation of the #154/#437 gate and 11/11 status is repeated in several docstrings and docs; consider tightening or centralizing this context to keep the configuration docs focused on current behavior and reduce repetition.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/CONFIG.md (1)
12-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale "opt-in" labels in the summary blurb after the flip.
The detailed sections at lines 68-80 and 294-298 / 300-326 now describe
use_heat_kernelanduse_hrr_structuralas default-ON, but the top-level[retrieval]knob list at line 12 still labels both as "opt-in". A reader skimming the summary will get the wrong default.📝 Suggested rewording
-`use_heat_kernel` (authority scoring lane, opt-in), `use_hrr_structural` (HRR structural-query lane, opt-in), `use_type_aware_compression` (per-belief retention-class compression, opt-in since v2.1), `use_vocab_bridge` (HRR query-side vocabulary bridge, opt-in since v2.1). +`use_heat_kernel` (authority scoring lane, default-on since `#154` bench gate), `use_hrr_structural` (HRR structural-query lane, default-on since `#154` bench gate), `use_type_aware_compression` (per-belief retention-class compression, opt-in since v2.1), `use_vocab_bridge` (HRR query-side vocabulary bridge, opt-in since v2.1).🤖 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 `@docs/CONFIG.md` at line 12, The top-level [retrieval] summary incorrectly labels use_heat_kernel and use_hrr_structural as "opt-in" despite detailed sections showing they are now default-ON; update the summary blurb for the retrieval knob list (the line containing use_heat_kernel and use_hrr_structural) to remove the "(opt-in)" qualifier and, if useful, annotate them as "default-ON" or include the version where the flip occurred to match the detailed sections; ensure you touch the text that lists the flags (including use_signed_laplacian and use_posterior_ranking) so the summary and the detailed descriptions are consistent.tests/test_heat_kernel.py (1)
15-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale acceptance-criteria docstring.
AC7: feature-flag default is False (use_heat_kernel)directly contradictstest_use_heat_kernel_default_onbelow.📝 Suggested fix
-- AC7: feature-flag default is False (use_heat_kernel) +- AC7: feature-flag default is True (use_heat_kernel) since `#154` + flipped the default after the `#437` 11/11 reproducibility gate🤖 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_heat_kernel.py` at line 15, Update the stale acceptance-criteria docstring that reads "AC7: feature-flag default is False (use_heat_kernel)" to match the actual test behavior in test_use_heat_kernel_default_on (i.e., change it to indicate the default is True or remove AC7 entirely); locate the docstring in tests/test_heat_kernel.py and edit the AC7 text so it no longer contradicts the test_use_heat_kernel_default_on test.
🤖 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/retrieval.py`:
- Line 805: The docstring describing structural-marker behavior is stale because
_route_structural_query now returns early (via RetrievalResult(beliefs=out,
locked_ids=[...]) when structural markers are hit) and the structural lane
defaults to ON, so callers using temporal_sort, with_doc_anchors, or
use_type_aware_compression will receive an unprocessed result; update the
docstring on the public API that calls _route_structural_query to state the
structural lane default is ON, that structural hits bypass post-processing steps
(temporal_sort, compressed_beliefs, doc_anchors) and which flags are ignored,
and add/update tests covering a query matching the structural pattern
(^[A-Z][A-Z_]*:\S) combined with temporal_sort=True / with_doc_anchors=True /
use_type_aware_compression=True to assert the returned RetrievalResult is the
early structural form or change the implementation if a different behavior is
desired.
In `@tests/test_heat_kernel.py`:
- Around line 256-276: The tests rely on ambient env/cwd and must explicitly
isolate environment and working directory: in both
test_use_heat_kernel_default_on and test_use_heat_kernel_opt_out_paths_intact,
call monkeypatch.delenv("AELFRICE_HEAT_KERNEL", raising=False) at the start and
pass start=tmp_path into is_heat_kernel_enabled so the resolver doesn't pick up
ancestor TOML; also add a type annotation tmp_path: Path on the second test and
add from pathlib import Path to the module imports to keep signatures
consistent.
In `@tests/test_hrr_struct_index.py`:
- Around line 226-246: Tests can flake because they don't isolate environment
variables or current working directory; update both tests to ensure
AELFRICE_HRR_STRUCTURAL is absent (use
monkeypatch.delenv("AELFRICE_HRR_STRUCTURAL", raising=False)) and pass
start=tmp_path to is_hrr_structural_enabled so _read_toml_flag_for walks from
the temporary dir rather than the test runner's cwd; specifically modify
test_use_hrr_structural_default_on to delete the env var and call
is_hrr_structural_enabled(start=tmp_path) and modify
test_use_hrr_structural_opt_out_paths_intact so the explicit-kwarg assertion
runs after monkeypatch.delenv and uses start=tmp_path for calls that rely on
TOML lookup.
---
Outside diff comments:
In `@docs/CONFIG.md`:
- Line 12: The top-level [retrieval] summary incorrectly labels use_heat_kernel
and use_hrr_structural as "opt-in" despite detailed sections showing they are
now default-ON; update the summary blurb for the retrieval knob list (the line
containing use_heat_kernel and use_hrr_structural) to remove the "(opt-in)"
qualifier and, if useful, annotate them as "default-ON" or include the version
where the flip occurred to match the detailed sections; ensure you touch the
text that lists the flags (including use_signed_laplacian and
use_posterior_ranking) so the summary and the detailed descriptions are
consistent.
In `@tests/test_heat_kernel.py`:
- Line 15: Update the stale acceptance-criteria docstring that reads "AC7:
feature-flag default is False (use_heat_kernel)" to match the actual test
behavior in test_use_heat_kernel_default_on (i.e., change it to indicate the
default is True or remove AC7 entirely); locate the docstring in
tests/test_heat_kernel.py and edit the AC7 text so it no longer contradicts the
test_use_heat_kernel_default_on test.
🪄 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: 34dccebb-59ac-404a-9c8c-5669a48eb9e4
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (6)
docs/CONFIG.mddocs/feature-hrr-vocab-bridge.mdsrc/aelfrice/hrr_index.pysrc/aelfrice/retrieval.pytests/test_heat_kernel.pytests/test_hrr_struct_index.py
| if toml_value is not None: | ||
| return toml_value | ||
| return False | ||
| return True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Surface every retrieve_v2 call site to see whether any combine the
# bypassed kwargs with structural-shaped queries.
rg -nP --type=py -C4 '\bretrieve_v2\s*\('
# Look for any test that exercises the marker + temporal_sort/with_doc_anchors combo.
rg -nP --type=py -C3 'temporal_sort\s*=\s*True|with_doc_anchors\s*=\s*True'Repository: robotrocketscience/aelfrice
Length of output: 45576
🏁 Script executed:
#!/bin/bash
# First, get the retrieve_v2 implementation and check line 1797
head -1850 src/aelfrice/retrieval.py | tail -100 | cat -nRepository: robotrocketscience/aelfrice
Length of output: 5490
🏁 Script executed:
#!/bin/bash
# Look at the actual retrieve_v2 function signature and implementation
sed -n '1699,1850p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 8448
🏁 Script executed:
#!/bin/bash
# Find _route_structural_query definition and understand the early return
rg -n '_route_structural_query' src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Look at _route_structural_query to see what RetrievalResult it returns
sed -n '808,950p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 5981
🏁 Script executed:
#!/bin/bash
# Check if there's a test that combines structural queries with temporal_sort or with_doc_anchors
rg -n 'CONTRADICTS|CITES|SUPPORTS' tests/ | grep -E 'temporal_sort|with_doc_anchors|use_type_aware_compression' | head -20Repository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Get the full retrieve_v2 function to see compressed_beliefs and doc_anchors handling
sed -n '1699,1850p' src/aelfrice/retrieval.py | tail -50 | cat -nRepository: robotrocketscience/aelfrice
Length of output: 2267
🏁 Script executed:
#!/bin/bash
# Check RetrievalResult dataclass to see default values
rg -n 'class RetrievalResult' src/aelfrice/retrieval.py -A 30Repository: robotrocketscience/aelfrice
Length of output: 1777
🏁 Script executed:
#!/bin/bash
# Get the remaining lines of retrieve_v2 to see doc_anchors and compressed_beliefs handling
sed -n '1800,1900p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 4862
🏁 Script executed:
#!/bin/bash
# Check for any test that uses structural queries (CONTRADICTS, CITES, SUPPORTS, etc.)
# combined with temporal_sort, with_doc_anchors, or use_type_aware_compression
rg -n 'retrieve_v2.*temporal_sort|retrieve_v2.*with_doc_anchors|retrieve_v2.*use_type_aware_compression' tests/Repository: robotrocketscience/aelfrice
Length of output: 1187
🏁 Script executed:
#!/bin/bash
# Check if use_hrr_structural is on by default
rg -n 'is_hrr_structural_enabled' src/aelfrice/retrieval.py -A 5Repository: robotrocketscience/aelfrice
Length of output: 551
🏁 Script executed:
#!/bin/bash
# Check the is_hrr_structural_enabled implementation
rg -n 'def is_hrr_structural_enabled' src/aelfrice/ -A 10Repository: robotrocketscience/aelfrice
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Look for any existing tests that explicitly combine structural markers with temporal_sort, with_doc_anchors, or compression
rg -n 'use_hrr_structural.*True' tests/ -A 10 | head -100Repository: robotrocketscience/aelfrice
Length of output: 4472
🏁 Script executed:
#!/bin/bash
# Get the full is_hrr_structural_enabled function to see the default
sed -n '775,806p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 1488
🏁 Script executed:
#!/bin/bash
# Check the retrieve_v2 docstring about defaults for use_hrr_structural
sed -n '1776,1790p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 1020
🏁 Script executed:
#!/bin/bash
# Check if there's a test combining structural marker + temporal_sort explicitly
rg -n 'CONTRADICTS|CITES|SUPPORTS' tests/test_retrieve_v2_hrr_structural.py -A 5 | grep -i 'temporal_sort\|with_doc_anchors\|compression'Repository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Get absolute line numbers for _route_structural_query and its return
sed -n '808,875p' src/aelfrice/retrieval.py | grep -n 'return'Repository: robotrocketscience/aelfrice
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Confirm: on a structural marker hit, the RetrievalResult skips all post-processing
# by showing the exact return statement at line 61-64
sed -n '861,864p' src/aelfrice/retrieval.py | cat -nRepository: robotrocketscience/aelfrice
Length of output: 209
The docstring at line 1776 is stale; the structural lane now defaults to ON (not OFF), confirming the concern.
On a structural-marker hit, _route_structural_query (lines 808–864) returns early with a minimal RetrievalResult(beliefs=out, locked_ids=[...]) that bypasses all post-processing: temporal_sort (line 1848), compressed_beliefs (lines 1852–1856), and doc_anchors (lines 1863–1866) remain empty. Pre-flip this only affected callers who explicitly set use_hrr_structural=True; post-flip (per line 31: default return True), any caller combining temporal_sort=True / with_doc_anchors=True / use_type_aware_compression=True with a query matching ^[A-Z][A-Z_]*:\S that resolves to a known kind will silently receive the unprocessed structural result. No existing tests cover this combination; the contract should be documented and the docstring corrected to reflect the current default-ON state.
🤖 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` at line 805, The docstring describing
structural-marker behavior is stale because _route_structural_query now returns
early (via RetrievalResult(beliefs=out, locked_ids=[...]) when structural
markers are hit) and the structural lane defaults to ON, so callers using
temporal_sort, with_doc_anchors, or use_type_aware_compression will receive an
unprocessed result; update the docstring on the public API that calls
_route_structural_query to state the structural lane default is ON, that
structural hits bypass post-processing steps (temporal_sort, compressed_beliefs,
doc_anchors) and which flags are ignored, and add/update tests covering a query
matching the structural pattern (^[A-Z][A-Z_]*:\S) combined with
temporal_sort=True / with_doc_anchors=True / use_type_aware_compression=True to
assert the returned RetrievalResult is the early structural form or change the
implementation if a different behavior is desired.
Comment on lines
+256
to
+276
| def test_use_heat_kernel_default_on() -> None: | ||
| """Per #154 the default flipped to ON after the #437 11/11 gate | ||
| cleared. No env, no kwarg, no toml → True.""" | ||
| from aelfrice.retrieval import is_heat_kernel_enabled | ||
|
|
||
| # No env, no kwarg, no toml → default False | ||
| assert is_heat_kernel_enabled() is False | ||
| assert is_heat_kernel_enabled() is True | ||
|
|
||
|
|
||
| def test_use_heat_kernel_opt_out_paths_intact(tmp_path) -> None: | ||
| """The opt-out surface (kwarg, TOML key) remains reachable for | ||
| users who want the pre-flip ranking. Replaces the v1.7-era | ||
| default-off check.""" | ||
| from aelfrice.retrieval import is_heat_kernel_enabled | ||
|
|
||
| # Explicit kwarg | ||
| assert is_heat_kernel_enabled(False) is False | ||
|
|
||
| # TOML key | ||
| cfg = tmp_path / ".aelfrice.toml" | ||
| cfg.write_text("[retrieval]\nuse_heat_kernel = false\n") | ||
| assert is_heat_kernel_enabled(start=tmp_path) is False |
There was a problem hiding this comment.
Same env/cwd-isolation concern as tests/test_hrr_struct_index.py.
test_use_heat_kernel_default_on calls is_heat_kernel_enabled() with no start= and no env scrub — ambient AELFRICE_HEAT_KERNEL=0 or an ancestor .aelfrice.toml containing [retrieval] use_heat_kernel = false will fail the assertion. The kwarg branch at line 271 is similarly vulnerable to AELFRICE_HEAT_KERNEL=1 because env beats kwarg in the resolver. Apply the same monkeypatch.delenv(...) + start=tmp_path treatment as suggested in the parallel test file. Also consider type-annotating tmp_path: Path for consistency with the rest of the file.
🛡️ Suggested isolation
-def test_use_heat_kernel_default_on() -> None:
+def test_use_heat_kernel_default_on(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
"""Per `#154` the default flipped to ON after the `#437` 11/11 gate
cleared. No env, no kwarg, no toml → True."""
from aelfrice.retrieval import is_heat_kernel_enabled
- assert is_heat_kernel_enabled() is True
+ monkeypatch.delenv("AELFRICE_HEAT_KERNEL", raising=False)
+ assert is_heat_kernel_enabled(start=tmp_path) is True
-def test_use_heat_kernel_opt_out_paths_intact(tmp_path) -> None:
+def test_use_heat_kernel_opt_out_paths_intact(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
"""The opt-out surface (kwarg, TOML key) remains reachable for
users who want the pre-flip ranking. Replaces the v1.7-era
default-off check."""
from aelfrice.retrieval import is_heat_kernel_enabled
+ monkeypatch.delenv("AELFRICE_HEAT_KERNEL", raising=False)
+
# Explicit kwarg
- assert is_heat_kernel_enabled(False) is False
+ assert is_heat_kernel_enabled(False, start=tmp_path) is False
# TOML key
cfg = tmp_path / ".aelfrice.toml"
cfg.write_text("[retrieval]\nuse_heat_kernel = false\n")
assert is_heat_kernel_enabled(start=tmp_path) is Falsefrom pathlib import Path will need to be added to the module imports if you adopt the annotation.
🤖 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_heat_kernel.py` around lines 256 - 276, The tests rely on ambient
env/cwd and must explicitly isolate environment and working directory: in both
test_use_heat_kernel_default_on and test_use_heat_kernel_opt_out_paths_intact,
call monkeypatch.delenv("AELFRICE_HEAT_KERNEL", raising=False) at the start and
pass start=tmp_path into is_heat_kernel_enabled so the resolver doesn't pick up
ancestor TOML; also add a type annotation tmp_path: Path on the second test and
add from pathlib import Path to the module imports to keep signatures
consistent.
Comment on lines
+226
to
+246
| def test_use_hrr_structural_default_on() -> None: | ||
| """Per #154 the default flipped to ON after the #437 11/11 gate | ||
| cleared. No env, no kwarg, no toml → True.""" | ||
| from aelfrice.retrieval import is_hrr_structural_enabled | ||
|
|
||
| assert is_hrr_structural_enabled() is False | ||
| assert is_hrr_structural_enabled() is True | ||
|
|
||
|
|
||
| def test_use_hrr_structural_opt_out_paths_intact(tmp_path: Path) -> None: | ||
| """The opt-out surface (env var, kwarg, TOML key) remains | ||
| reachable for users who want the pre-flip ranking. Replaces the | ||
| v1.7-era default-off check.""" | ||
| from aelfrice.retrieval import is_hrr_structural_enabled | ||
|
|
||
| # Explicit kwarg | ||
| assert is_hrr_structural_enabled(False) is False | ||
|
|
||
| # TOML key | ||
| cfg = tmp_path / ".aelfrice.toml" | ||
| cfg.write_text("[retrieval]\nuse_hrr_structural = false\n") | ||
| assert is_hrr_structural_enabled(start=tmp_path) is False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether a project-wide autouse fixture already clears AELFRICE_* env
# vars before each test (would invalidate the concern above).
fd -t f conftest.py tests
rg -nP -C3 'AELFRICE_(HEAT_KERNEL|HRR_STRUCTURAL|BM25F|BFS|VOCAB_BRIDGE)' testsRepository: robotrocketscience/aelfrice
Length of output: 872
🏁 Script executed:
cat -n tests/conftest.pyRepository: robotrocketscience/aelfrice
Length of output: 3160
🏁 Script executed:
cat -n tests/test_hrr_struct_index.py | sed -n '220,260p'Repository: robotrocketscience/aelfrice
Length of output: 1743
🏁 Script executed:
fd -t f "retrieval.py" srcRepository: robotrocketscience/aelfrice
Length of output: 97
🏁 Script executed:
cat -n src/aelfrice/retrieval.py | head -100Repository: robotrocketscience/aelfrice
Length of output: 4732
🏁 Script executed:
rg -n "def is_hrr_structural_enabled" -A 20 src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 1064
🏁 Script executed:
rg -n "def is_hrr_structural_enabled" -A 40 src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 1603
🏁 Script executed:
rg -n "_env_hrr_structural_override\|_read_toml_flag_for" -B 2 -A 10 src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
rg -n "_env_hrr_structural_override|_read_toml_flag_for" src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 925
🏁 Script executed:
rg -n "def _env_hrr_structural_override\|def _read_toml_flag_for" -A 15 src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
sed -n '345,365p' src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 768
🏁 Script executed:
sed -n '405,435p' src/aelfrice/retrieval.pyRepository: robotrocketscience/aelfrice
Length of output: 1203
🏁 Script executed:
grep -n "monkeypatch\|delenv\|environ" tests/conftest.pyRepository: robotrocketscience/aelfrice
Length of output: 115
🏁 Script executed:
sed -n '1,100p' tests/test_hrr_struct_index.py | grep -i "memorystorm\|mock\|patch"Repository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
rg -n "MemoryStore\|unittest.mock" tests/test_hrr_struct_index.py | head -20Repository: robotrocketscience/aelfrice
Length of output: 53
🏁 Script executed:
head -50 tests/test_hrr_struct_index.pyRepository: robotrocketscience/aelfrice
Length of output: 1597
🏁 Script executed:
sed -n '1,30p' tests/test_hrr_struct_index.pyRepository: robotrocketscience/aelfrice
Length of output: 1161
🏁 Script executed:
grep -n "def _toy_store\|MemoryStore" tests/test_hrr_struct_index.py | head -10Repository: robotrocketscience/aelfrice
Length of output: 256
Tests don't isolate env/cwd; can flake on a developer or CI environment that sets AELFRICE_HRR_STRUCTURAL or has an ambient .aelfrice.toml.
is_hrr_structural_enabled() consults os.environ[AELFRICE_HRR_STRUCTURAL] first and _read_toml_flag_for walks upward from Path.cwd(). Both test_use_hrr_structural_default_on (no start=, no env clear) and the kwarg branch of test_use_hrr_structural_opt_out_paths_intact (env beats kwarg) will produce wrong results if the ambient env or any ancestor .aelfrice.toml of the test runner pins the flag.
Use monkeypatch.delenv(...) and pass start=tmp_path consistently:
🛡️ Suggested isolation
-def test_use_hrr_structural_default_on() -> None:
+def test_use_hrr_structural_default_on(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
"""Per `#154` the default flipped to ON after the `#437` 11/11 gate
cleared. No env, no kwarg, no toml → True."""
from aelfrice.retrieval import is_hrr_structural_enabled
- assert is_hrr_structural_enabled() is True
+ monkeypatch.delenv("AELFRICE_HRR_STRUCTURAL", raising=False)
+ assert is_hrr_structural_enabled(start=tmp_path) is True
-def test_use_hrr_structural_opt_out_paths_intact(tmp_path: Path) -> None:
+def test_use_hrr_structural_opt_out_paths_intact(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
"""The opt-out surface (env var, kwarg, TOML key) remains
reachable for users who want the pre-flip ranking. Replaces the
v1.7-era default-off check."""
from aelfrice.retrieval import is_hrr_structural_enabled
+ monkeypatch.delenv("AELFRICE_HRR_STRUCTURAL", raising=False)
+
# Explicit kwarg
- assert is_hrr_structural_enabled(False) is False
+ assert is_hrr_structural_enabled(False, start=tmp_path) is False
# TOML key
cfg = tmp_path / ".aelfrice.toml"
cfg.write_text("[retrieval]\nuse_hrr_structural = false\n")
assert is_hrr_structural_enabled(start=tmp_path) is False🤖 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_hrr_struct_index.py` around lines 226 - 246, Tests can flake
because they don't isolate environment variables or current working directory;
update both tests to ensure AELFRICE_HRR_STRUCTURAL is absent (use
monkeypatch.delenv("AELFRICE_HRR_STRUCTURAL", raising=False)) and pass
start=tmp_path to is_hrr_structural_enabled so _read_toml_flag_for walks from
the temporary dir rather than the test runner's cwd; specifically modify
test_use_hrr_structural_default_on to delete the env var and call
is_hrr_structural_enabled(start=tmp_path) and modify
test_use_hrr_structural_opt_out_paths_intact so the explicit-kwarg assertion
runs after monkeypatch.delenv and uses start=tmp_path for calls that rely on
TOML lookup.
Owner
Author
|
[claim:review:Kulili:2026-05-09T16:48:40Z] |
Owner
Author
|
[release:review:Kulili:2026-05-09T16:50:13Z] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #154.
Summary
use_hrr_structuraldefaultFalse→True(1st atomic commit).use_heat_kerneldefaultFalse→True(2nd atomic commit).[Unreleased]entry (3rd atomic commit).The #154 composition tracker has gated these two default flips on the calibrated reproducibility-harness reaching 11/11. That gate cleared yesterday via PR #489 closing #437, so the tracker's status banner ("
use_heat_kernel,use_hrr_structuralstay opt-in pending the calibrated #437 reproducibility harness") no longer applies and the flips are due.Behavior
Precedence (env > kwarg > TOML > default) is unchanged. Only the default value flips. The opt-out surface remains reachable for users who want parity with the v2.0.x ranking:
use_heat_kernelAELFRICE_HEAT_KERNEL=0[retrieval] use_heat_kernel = falseuse_heat_kernel=Falseuse_hrr_structuralAELFRICE_HRR_STRUCTURAL=0[retrieval] use_hrr_structural = falseuse_hrr_structural=FalseEach replaced default-off test is now a default-on assertion plus an explicit opt-out coverage test for the env / kwarg / TOML paths.
Test plan
tests/test_heat_kernel.py,tests/test_hrr_struct_index.py,tests/test_retrieve_v2_hrr_structural.py,tests/test_composition_tracker.py— greenuv run pytest tests/ --ignore=tests/e2e→ 3066 passed, 51 skippedNotes
Summary by Sourcery
Flip the retrieval flags for the heat-kernel authority lane and HRR structural-query lane to be enabled by default, while preserving existing opt-out mechanisms and documenting the behavior change.
Enhancements:
use_heat_kernelanduse_hrr_structuralnow default totruewhen no env, kwarg, or TOML override is provided.Documentation:
use_heat_kernelanduse_hrr_structural.Summary by CodeRabbit
New Features
<KIND>:<belief-id>syntax for targeted retrieval.Behavior Changes
Documentation