feat(retrieval): use_intentional_clustering flips to default-on (#436) - #577
Conversation
Reviewer's GuideFlips the retrieval feature flag Sequence diagram for resolve_use_intentional_clustering with default-on behaviorsequenceDiagram
participant Caller
participant Retrieval as RetrievalModule
participant Env as EnvSubsystem
participant Toml as TomlConfig
Caller->>Retrieval: resolve_use_intentional_clustering(explicit, start)
Retrieval->>Env: _env_intentional_clustering_override()
Env-->>Retrieval: env_value or None
alt env_value is not None
Retrieval-->>Caller: env_value
else env_value is None
alt explicit is not None
Retrieval-->>Caller: explicit
else explicit is None
Retrieval->>Toml: _read_toml_flag_for(INTENTIONAL_CLUSTERING_FLAG, start)
Toml-->>Retrieval: toml_value or None
alt toml_value is not None
Retrieval-->>Caller: toml_value
else toml_value is None
Retrieval-->>Caller: True
end
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✨ 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 |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_env_var_alone_enables_compression, consider using the existingENV_INTENTIONAL_CLUSTERINGconstant instead of the raw"AELFRICE_INTENTIONAL_CLUSTERING"string so env key usage stays consistent and refactor-safe. - Now that
use_intentional_clusteringdefaults toTrueand is mutex withuse_type_aware_compression, it may be worth enforcing this at theretrieve_v2boundary (e.g., raising or logging when both are enabled) instead of relying on scattered call-site overrides to avoid accidental co-activation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_env_var_alone_enables_compression`, consider using the existing `ENV_INTENTIONAL_CLUSTERING` constant instead of the raw `"AELFRICE_INTENTIONAL_CLUSTERING"` string so env key usage stays consistent and refactor-safe.
- Now that `use_intentional_clustering` defaults to `True` and is mutex with `use_type_aware_compression`, it may be worth enforcing this at the `retrieve_v2` boundary (e.g., raising or logging when both are enabled) instead of relying on scattered call-site overrides to avoid accidental co-activation.
## Individual Comments
### Comment 1
<location path="tests/test_compression_integration.py" line_range="190-196" />
<code_context>
monkeypatch: pytest.MonkeyPatch, _isolated_cwd: Path
) -> None:
monkeypatch.setenv(ENV_TYPE_AWARE_COMPRESSION, "1")
+ # Post-#436 default-flip, AELFRICE_INTENTIONAL_CLUSTERING must also
+ # be disabled in this scope to satisfy the v2.0.0 mutex (the cluster
+ # pack accounts in raw tokens; composing it with compressed cost is
+ # tracked as a v2.x follow-up). The test still meaningfully exercises
+ # "env var alone enables compression" — it just makes the clustering
+ # env-disable explicit instead of relying on the (now-flipped) default.
+ monkeypatch.setenv("AELFRICE_INTENTIONAL_CLUSTERING", "0")
s = _populate_store()
result = retrieve_v2(s, "sqlite system") # no explicit kwarg
</code_context>
<issue_to_address>
**nitpick:** Avoid hard-coding the clustering env var name in tests
Use the same constant the implementation relies on (e.g. `ENV_INTENTIONAL_CLUSTERING`) instead of the string literal. This keeps the test aligned with production behavior and ensures it fails if the env var name changes, rather than silently diverging.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Post-#436 default-flip, AELFRICE_INTENTIONAL_CLUSTERING must also | ||
| # be disabled in this scope to satisfy the v2.0.0 mutex (the cluster | ||
| # pack accounts in raw tokens; composing it with compressed cost is | ||
| # tracked as a v2.x follow-up). The test still meaningfully exercises | ||
| # "env var alone enables compression" — it just makes the clustering | ||
| # env-disable explicit instead of relying on the (now-flipped) default. | ||
| monkeypatch.setenv("AELFRICE_INTENTIONAL_CLUSTERING", "0") |
There was a problem hiding this comment.
nitpick: Avoid hard-coding the clustering env var name in tests
Use the same constant the implementation relies on (e.g. ENV_INTENTIONAL_CLUSTERING) instead of the string literal. This keeps the test aligned with production behavior and ensures it fails if the env var name changes, rather than silently diverging.
|
[claim:review:maxwell:2026-05-10T06:41:24Z] |
|
[claim:review:faraday:2026-05-10T06:42:54Z] |
|
[release:review:faraday:2026-05-10T06:42:58Z] |
|
[claim:review:leibniz:2026-05-10T06:43:37Z] |
|
[release:review:leibniz:2026-05-10T06:43:42Z] |
e1d8a29 to
5248011
Compare
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
5248011 to
5e93400
Compare
|
[claim:review:leibniz:2026-05-10T06:49:43Z] |
|
[release:review:leibniz:2026-05-10T06:49:47Z] |
Substrate landed in v2.0/v2.1 (#496 module + corpus, #498 retrieve_v2 wiring, #504 bench-gate scorer). Bench gate cleared on the production multi-store sweep (R6 60/60 PASS at p99 0.328ms, 15-30x margin under the 5ms A4 budget). Per the resolve_use_intentional_clustering() docstring contract ("the bench gate flips the default after lab-side benchmark evidence clears"), the default is unblocked. Precedence (env > kwarg > TOML > default) is unchanged; only the default value flips False -> True. Reversible via [retrieval] use_intentional_clustering = false in .aelfrice.toml or AELFRICE_INTENTIONAL_CLUSTERING=0 for v2.0.x parity. Tests updated: - test_default_is_off -> test_default_is_on - test_env_garbage_falls_through default-arm assertion flipped - test_default_call_byte_identical_to_explicit_off -> test_default_call_byte_identical_to_explicit_on (ON-byte-identity invariant supersedes the earlier OFF-byte-identity invariant) Floor parameter (DEFAULT_CLUSTER_EDGE_FLOOR = 0.4) is unchanged in this commit. Lab finding (raising to 0.6 triples uplift on 8 rows by excluding CITES inter-cluster edges) is queued as a separate follow-up so this PR stays minimal and reviewable.
Mirrors the format of the v2.1.0 #154 default-flip entry: substrate, evidence, contract, precedence, opt-out, separable follow-up. Cites lab-side R6 numbers as the bench-gate verdict.
…flip (#436) Four compression tests implicitly relied on use_intentional_clustering defaulting to False to satisfy the v2.0.0 mutex (retrieval.py:1567). Post-#436 default-flip, the implicit assumption breaks and tests trip the mutex. Fix: add explicit `use_intentional_clustering=False` to the three direct retrieve_v2() callers, and `AELFRICE_INTENTIONAL_CLUSTERING=0` to test_env_var_alone_enables_compression. Each preserves test intent (compression-only behavior); the change just makes the mutex precondition explicit instead of leaning on the (now-flipped) default. Same wart applies user-side: any caller that opts into compression via TOML / env / kwarg without also disabling clustering will hit the mutex ValueError. Documented in the changelog entry; mutex removal is a v2.x compose-them-together follow-up tracked separately.
5e93400 to
fdf31f4
Compare
|
[release:review:maxwell:2026-05-10T06:53:20Z] |
Summary
Flips
use_intentional_clusteringdefaultFalse→Trueper the explicit ship-criteria contract inresolve_use_intentional_clustering():That gate cleared on the production multi-store sweep (lab-side, #436 R6 run 2026-05-09): 60/60 PASS at p99 0.328ms — ~15-30× margin under the 5ms A4 latency budget across the top-2 production stores.
Substrate (already on main)
RetrievalClustermodule + multi-fact corpus mountretrieve_v2run_clustering_upliftbench-gate scorerWhat changes
src/aelfrice/retrieval.py:923—return False→return Trueinresolve_use_intentional_clustering(). Precedence (env > kwarg > TOML > default) is unchanged.tests/test_clustering_integration.py— three test updates:test_default_is_off→test_default_is_ontest_env_garbage_falls_throughdefault-arm assertion flippedtest_default_call_byte_identical_to_explicit_off→test_default_call_byte_identical_to_explicit_on(the ON-byte-identity invariant supersedes the OFF-byte-identity invariant)CHANGELOG.md—[Unreleased] Changedentry mirroring the v2.1.0 [retrieval] Pipeline composition tracker — unified retrieve() with feature-flag gate #154 default-flip format.Opt-out
Reversible for v2.0.x parity:
…or
AELFRICE_INTENTIONAL_CLUSTERING=0.Out of scope
The lab-finding that raising
DEFAULT_CLUSTER_EDGE_FLOORfrom 0.4 → 0.6 triples uplift on 8 rows (by excludingEDGE_CITESinter-cluster edges) is queued as a separate follow-up so this PR stays minimal and reviewable.Test plan
pytest tests/test_clustering_integration.py— 10/10 PASSpytest tests/ -k 'clustering or retrieve_v2 or context_rebuilder'— 130/130 PASS, 4 skippedSummary by Sourcery
Flip intentional clustering retrieval behavior to default-on while preserving existing override precedence and update tests and changelog accordingly.
New Features:
Enhancements:
Documentation:
use_intentional_clusteringbehavior and opt-out path in the changelog.Tests: