fix(retrieval): demote or exclude superseded beliefs, both arms behind a flag (#1187) - #1191
Conversation
Reviewer's GuideImplements a supersession-aware retrieval lane that can either demote or exclude beliefs retired by SUPERSEDES edges, wires it through the production retrieval path behind a default-off flag with env/kwarg/TOML resolution, adds a batched store query and configuration/docs/changelog updates, and thoroughly tests both arms and resolver behavior. Sequence diagram for supersession-aware retrieval lanesequenceDiagram
actor User
participant Client
participant retrieve_v2
participant retrieve_with_tiers
participant _l1_hits
participant Store
User->>Client: call retrieve()
Client->>retrieve_v2: retrieve_v2(query, use_supersession_demote, supersession_treatment, supersession_factor)
retrieve_v2->>is_supersession_demote_enabled: use_supersession_demote
retrieve_v2->>resolve_supersession_treatment: supersession_treatment
retrieve_v2->>resolve_supersession_factor: supersession_factor
retrieve_v2->>retrieve_with_tiers: use_supersession_demote, supersession_treatment, supersession_factor
retrieve_with_tiers->>_l1_hits: use_supersession_demote, supersession_treatment, supersession_factor
alt use_supersession_demote
_l1_hits->>Store: superseded_belief_ids(candidate_ids)
Store-->>_l1_hits: superseded_ids
alt supersession_treatment == SUPERSESSION_TREATMENT_EXCLUDE
_l1_hits->>_l1_hits: filter out superseded beliefs
else supersession_treatment == SUPERSESSION_TREATMENT_DEMOTE
_l1_hits->>_supersession_penalty: superseded_ids, belief_id, supersession_factor
_supersession_penalty-->>_l1_hits: log(factor) penalty
_l1_hits->>_l1_hits: s += _supersession_penalty(...)
end
else not use_supersession_demote
_l1_hits-->>retrieve_with_tiers: ranking unchanged by supersession
end
retrieve_with_tiers-->>retrieve_v2: ranked beliefs
retrieve_v2-->>Client: results
Client-->>User: surface ranked beliefs
Flow diagram for supersession treatment demote vs excludeflowchart TD
A["Supersession lane enabled?"] -->|No| B["Return original ranking"]
A -->|Yes| C["Fetch superseded_belief_ids from Store"]
C --> D["supersession_treatment"]
D -->|SUPERSESSION_TREATMENT_EXCLUDE| E["Filter out superseded beliefs from candidate set"]
D -->|SUPERSESSION_TREATMENT_DEMOTE| F["Compute penalty = log(clamped supersession_factor)"]
F --> G["Add _supersession_penalty to rerank score for superseded beliefs"]
E --> H["Continue retrieval with pruned candidates"]
G --> H
H --> I["Return supersession-aware ranking"]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 29 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 Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe retrieval pipeline adds a default-off supersession lane. It resolves configuration from environment, kwargs, TOML, and defaults; identifies superseded candidates through a batched store query; then demotes or excludes them during L1 scoring. ChangesSupersession retrieval
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RetrieveV2
participant RetrieveWithTiers
participant L1Hits
participant MemoryStore
RetrieveV2->>RetrieveV2: Resolve supersession configuration
RetrieveV2->>RetrieveWithTiers: Pass lane parameters
RetrieveWithTiers->>L1Hits: Pass scoring parameters
L1Hits->>MemoryStore: Query SUPERSEDES destinations
MemoryStore-->>L1Hits: Return superseded belief IDs
L1Hits-->>RetrieveWithTiers: Demote or exclude candidates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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 |
![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 found 2 issues, and left some high level feedback:
- In
_l1_hitsyou recomputesuperseded_belief_idsseparately on the BM25F and FTS5 paths; consider factoring this into a single helper or computing it once per call to avoid duplicated work on larger candidate sets. - The resolver functions (
resolve_supersession_treatment,resolve_supersession_factor,_read_toml_str_for) emit diagnostics viaprintto stderr; it may be more consistent with the rest of the codebase to route these through a logging facility so callers can control verbosity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_l1_hits` you recompute `superseded_belief_ids` separately on the BM25F and FTS5 paths; consider factoring this into a single helper or computing it once per call to avoid duplicated work on larger candidate sets.
- The resolver functions (`resolve_supersession_treatment`, `resolve_supersession_factor`, `_read_toml_str_for`) emit diagnostics via `print` to stderr; it may be more consistent with the rest of the codebase to route these through a logging facility so callers can control verbosity.
## Individual Comments
### Comment 1
<location path="tests/test_supersession_lane.py" line_range="369-378" />
<code_context>
+# --- Reachable from the production path ----------------------------------
+
+
+def test_retrieve_v2_threads_the_lane(
+ store: MemoryStore, monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """The lane has to be reachable from `retrieve_v2`, not just `_l1_hits`.
+
+ `uri_baki.apply_supersession_demote` had no importer for exactly this
+ reason — a primitive nothing calls is not a fix. Uses the exclusion arm
+ because it is unambiguous end-to-end (the demote arm composes with the
+ default-ON entity lane, per the composition test above).
+ """
+ monkeypatch.delenv(retrieval.ENV_SUPERSESSION_DEMOTE, raising=False)
+
+ off = retrieve_v2(store, "deploy target")
+ on = retrieve_v2(
+ store, "deploy target", use_supersession_demote=True,
+ supersession_treatment=SUPERSESSION_TREATMENT_EXCLUDE,
+ )
+
+ assert _order(off.beliefs) == ["superseded", "current"]
+ assert _order(on.beliefs) == ["current"]
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that exercises `retrieve()` itself so the supersession lane is validated all the way through the default public API.
`test_retrieve_v2_threads_the_lane` verifies wiring through `retrieve_v2`, but most callers use `retrieve()`. Without a test for `retrieve()`, the lane could be dropped or mis-threaded there without detection. Please add a test that calls `retrieve()` with `use_supersession_demote=True` and `supersession_treatment=SUPERSESSION_TREATMENT_EXCLUDE`, and asserts the same belief ordering, to lock in this behavior across the default API surface.
Suggested implementation:
```python
assert _order(off.beliefs) == ["superseded", "current"]
assert _order(on.beliefs) == ["current"]
def test_retrieve_threads_the_lane(
store: MemoryStore, monkeypatch: pytest.MonkeyPatch,
) -> None:
"""The supersession lane must also be threaded through `retrieve()`.
Mirrors `test_retrieve_v2_threads_the_lane` but exercises the default
public retrieval API, so that changes to `retrieve()` wiring cannot drop
or mis-thread the supersession demote lane without test failures.
"""
monkeypatch.delenv(retrieval.ENV_SUPERSESSION_DEMOTE, raising=False)
off = retrieve(store, "deploy target")
on = retrieve(
store, "deploy target", use_supersession_demote=True,
supersession_treatment=SUPERSESSION_TREATMENT_EXCLUDE,
)
assert _order(off.beliefs) == ["superseded", "current"]
assert _order(on.beliefs) == ["current"]
```
1. Ensure `retrieve` is imported into `tests/test_supersession_lane.py` alongside `retrieve_v2` from the appropriate module (likely the same module where `retrieve_v2` is imported).
2. If `retrieve` returns a different shape than `retrieve_v2` (e.g., the beliefs are on `result.beliefs` vs. directly on the result), adjust `off`/`on` handling accordingly while keeping the assertions on the belief ordering identical.
</issue_to_address>
### Comment 2
<location path="tests/test_supersession_lane.py" line_range="305-311" />
<code_context>
+ assert is_supersession_demote_enabled(start=tmp_path) is True
+
+
+def test_treatment_defaults_to_the_safer_arm(
+ tmp_path: Path, monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A wrong demote leaves a ranking signal; a wrong exclusion does not."""
+ monkeypatch.delenv(retrieval.ENV_SUPERSESSION_TREATMENT, raising=False)
+
+ assert resolve_supersession_treatment(start=tmp_path) == (
+ SUPERSESSION_TREATMENT_DEMOTE
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Precedence for supersession treatment/factor resolution is only partially tested; consider explicit precedence tests similar to the lane flag.
Current tests cover `is_supersession_demote_enabled` precedence and the normalisation/error paths of `resolve_supersession_treatment` and `resolve_supersession_factor`, but not their precedence behaviour when multiple sources are set. Please add parametrized tests that assert the documented order (env > kwarg > TOML > default) for both resolvers—for example, "env overrides kwarg", "kwarg overrides TOML"—so configuration resolution precedence is fully exercised and guarded against regressions.
Suggested implementation:
```python
def test_kwarg_beats_toml(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
monkeypatch.delenv(retrieval.ENV_SUPERSESSION_DEMOTE, raising=False)
(tmp_path / ".aelfrice.toml").write_text(
"[retrieval]\nuse_supersession_demote = true\n", encoding="utf-8",
)
assert is_supersession_demote_enabled(False, start=tmp_path) is False
assert is_supersession_demote_enabled(start=tmp_path) is True
from aelfrice.retrieval import (
SUPERSESSION_DEMOTE_FACTOR,
SUPERSESSION_TREATMENT_DEMOTE,
SUPERSESSION_TREATMENT_EXCLUDE,
_l1_hits,
_supersession_penalty,
is_supersession_demote_enabled,
resolve_supersession_factor,
resolve_supersession_treatment,
retrieve_v2,
)
@pytest.mark.parametrize(
"env_value, kwarg_value, toml_value, expected",
[
# env overrides kwarg and TOML
(
SUPERSESSION_TREATMENT_EXCLUDE,
SUPERSESSION_TREATMENT_DEMOTE,
SUPERSESSION_TREATMENT_DEMOTE,
SUPERSESSION_TREATMENT_EXCLUDE,
),
# kwarg overrides TOML when env is unset
(
None,
SUPERSESSION_TREATMENT_EXCLUDE,
SUPERSESSION_TREATMENT_DEMOTE,
SUPERSESSION_TREATMENT_EXCLUDE,
),
# TOML used when env and kwarg are unset
(
None,
None,
SUPERSESSION_TREATMENT_EXCLUDE,
SUPERSESSION_TREATMENT_EXCLUDE,
),
# default used when no sources are set
(
None,
None,
None,
SUPERSESSION_TREATMENT_DEMOTE,
),
],
)
def test_resolve_supersession_treatment_precedence(
tmp_path: Path,
monkeypatch: pytest.MonkeyPatch,
env_value: str | None,
kwarg_value: str | None,
toml_value: str | None,
expected: str,
) -> None:
"""Ensure env > kwarg > TOML > default precedence for supersession treatment."""
if env_value is None:
monkeypatch.delenv(retrieval.ENV_SUPERSESSION_TREATMENT, raising=False)
else:
monkeypatch.setenv(retrieval.ENV_SUPERSESSION_TREATMENT, env_value)
if toml_value is not None:
(tmp_path / ".aelfrice.toml").write_text(
"[retrieval]\nsupersession_treatment = \""
+ toml_value
+ "\"\n",
encoding="utf-8",
)
assert resolve_supersession_treatment(kwarg_value, start=tmp_path) == expected
@pytest.mark.parametrize(
"env_value, kwarg_value, toml_value, expected",
[
# env overrides kwarg and TOML
("0.5", 0.1, 0.2, 0.5),
# kwarg overrides TOML when env is unset
(None, 0.3, 0.1, 0.3),
# TOML used when env and kwarg are unset
(None, None, 0.4, 0.4),
# default used when no sources are set
(None, None, None, SUPERSESSION_DEMOTE_FACTOR),
],
)
def test_resolve_supersession_factor_precedence(
tmp_path: Path,
monkeypatch: pytest.MonkeyPatch,
env_value: str | None,
kwarg_value: float | None,
toml_value: float | None,
expected: float,
) -> None:
"""Ensure env > kwarg > TOML > default precedence for supersession demote factor."""
if env_value is None:
monkeypatch.delenv(retrieval.ENV_SUPERSESSION_DEMOTE_FACTOR, raising=False)
else:
monkeypatch.setenv(retrieval.ENV_SUPERSESSION_DEMOTE_FACTOR, env_value)
if toml_value is not None:
(tmp_path / ".aelfrice.toml").write_text(
"[retrieval]\nsupersession_demote_factor = "
+ str(toml_value)
+ "\n",
encoding="utf-8",
)
assert resolve_supersession_factor(kwarg_value, start=tmp_path) == expected
```
These changes assume:
1. `ENV_SUPERSESSION_TREATMENT` and `ENV_SUPERSESSION_DEMOTE_FACTOR` exist on the `aelfrice.retrieval` module and are the correct environment variable names; if they differ, update the references accordingly.
2. The TOML keys `supersession_treatment` and `supersession_demote_factor` match the actual configuration schema; if different, adjust the keys in the `.write_text(...)` calls.
3. `SUPERSESSION_TREATMENT_DEMOTE` and `SUPERSESSION_TREATMENT_EXCLUDE` are the string values expected by `resolve_supersession_treatment`. If the resolvers operate on enums or other types, adapt the parameter types and TOML/env string representations to match.
4. If type annotations such as `str | None` and `float | None` are inconsistent with the project's minimum Python version or style, you may need to switch to `Optional[str]` / `Optional[float]` or omit annotations to align with the rest of the test module.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
28a959e to
a3e2295
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aelfrice/retrieval.py (1)
2966-3033: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring gap: new supersession params undocumented.
Every other
_l1_hitsflag (use_bm25f_anchors,gamma_temperature,zeta_params,heat_kernel_on, etc.) gets a dedicated paragraph in the docstring, butuse_supersession_demote/supersession_treatment/supersession_factor(added at Lines 2980-2982) aren't mentioned there at all — the behavior is only documented via inline comments deeper in the function body. Worth a short paragraph for discoverability, matching this file's own documentation convention.🤖 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 2966 - 3033, Document the supersession options in the _l1_hits docstring: describe use_supersession_demote, supersession_treatment, and supersession_factor, including their role in reranking and the available treatment behavior as established by the function implementation. Match the concise dedicated-paragraph style used for the other scoring flags, without changing runtime logic.
🤖 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 `@src/aelfrice/retrieval.py`:
- Around line 2966-3033: Document the supersession options in the _l1_hits
docstring: describe use_supersession_demote, supersession_treatment, and
supersession_factor, including their role in reranking and the available
treatment behavior as established by the function implementation. Match the
concise dedicated-paragraph style used for the other scoring flags, without
changing runtime logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa0b5bf6-2925-45ba-a025-a507abacf3b9
📒 Files selected for processing (5)
CHANGELOG/v4.mddocs/user/CONFIG.mdsrc/aelfrice/retrieval.pysrc/aelfrice/store.pytests/test_supersession_lane.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/user/CONFIG.md
- CHANGELOG/v4.md
a3e2295 to
6c9495f
Compare
|
[claim:review:Setr:2026-07-30T17:00:35Z] |
|
Not approving yet. The mechanism, the resolvers and finding 1 are all correct — and finding 1 is the right call, independently corroborated below. But the exclusion arm has a defect that would make the bench it exists to serve unreadable, and it should be fixed before the operator commissions that bench. The blocker: exclusion shrinks the pack, it does not backfill
Reproduced on this branch. Seven beliefs match One belief instead of four, with three perfectly good current beliefs available and unreached. In the degenerate case — every top- Two consequences, and the second is the one that matters for this PR's purpose:
The fix is contained: on the exclusion arm, over-fetch ( I'd rather this land in this PR than as a follow-up, because a bench run against the current exclusion arm produces a number that looks authoritative and isn't. Finding 1 is right, and it is already established hereThe multiplicative-inversion analysis is correct and matches a prior finding in this codebase: the composite rerank score is log-domain and routinely negative, so a multiplicative demote inverts into a promotion.
Finding 2 is right, and the recommendation should be strongerAgreed that Verified
Smaller notes
Requested: the over-fetch fix on the exclusion arm. Everything else here I'd approve as-is. Re-ping me and I'll re-review promptly; the reproduction above is three beliefs and a loop if you want it as a test. |
robotrocketscience
added
attn:unblock
|
[release:review:Setr:2026-07-30T17:04:04Z] |
6c9495f to
ba067c4
Compare
|
[claim:review:Setr:2026-07-30T18:20:22Z] |
ba067c4 to
6562ccb
Compare
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
|
Rebased onto Exclusion now backfills instead of shrinking. Measured on a 20-belief matching set with the strongest 8 retired: The arm widens the candidate fetch and retries, stopping as soon as it has On the BM25F path the refetch reuses the already-computed Three tests, each load-bearing — removing the widening fails the backfill test:
Checked the #1195 gate. That merged while this was under review and now binds any PR touching The two findings in the PR body still stand and are still the right advice for the bench. The multiplicative-inversion analysis is correct, and the factor-0.5 weakness is real — sweep the factor, don't pin it. What changes is that a loss for exclusion is now attributable: previously the arms differed in pack size as well as in treatment, so the bench could not have separated 'removing retired content hurt' from 'the pack got smaller'. Labelling once CI settles. |
6562ccb to
0ed7302
Compare
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
|
Both Sourcery threads were valid — added, and rebased onto current main. Now
I checked what it actually catches rather than assuming, and the docstring now says so: it fails if Precedence. Added for both Labelling once CI settles. |
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
0ed7302 to
5e72138
Compare
robotrocketscience
added
the
ready-to-merge
|
[release:review:Setr:2026-07-30T18:36:49Z] |
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
One query returning which of a candidate set a SUPERSEDES edge points at. Direction is the producers' canonical one (#1170): src is the newer belief, dst the one it retires, so a belief is superseded when it appears as a dst. The candidate set binds as a single JSON array read through json_each rather than an interpolated IN list, following ca97776 — the SQL text stays static and placeholder count cannot drift. Batched because the caller runs it inside the L1 rerank.
Retrieval had no notion of supersession: grep SUPERSED over retrieval.py returned nothing, and uri_baki.apply_supersession_demote had no importer. So correcting "deploy target is heroku" to "fly.io" left the next prompt injecting heroku first, ahead of its own replacement. Ships both ratified arms behind one default-OFF use_supersession_demote flag — demote by log(factor), or exclude from the candidate set before the heat-kernel seeds are computed — selected by supersession_treatment and resolved env > kwarg > TOML > default like every neighbouring lane. The default stays off: unlike the #1170 BFS fix this changes retrieve() output on the default path, so the three-arm bench picks the winner. The demote is additive, which corrects the issue's suggested wiring. The composite rerank score is log-domain and routinely negative (-13.08 on the reproduction), so score * 0.5 raises it — importing the multiplicative primitive would have promoted the superseded belief to the top, the exact inversion this fixes. Adding log(factor) is the log-domain equivalent of scaling a probability, so the factor semantics survive intact.
26 tests: edge direction (dst not src, so the #1170 inversion cannot reappear one layer down), candidate scoping, other edge types ignored, both arms through the L1 rerank, exclusion emptying the pack, and reachability from retrieve_v2 — the gap that left the uri_baki primitive importerless. Two pin findings rather than behaviour. One asserts the negative-score premise directly, so the additive penalty is not "simplified" back into a multiplication. The other records the measured composition with the default-ON entity-persistence lane: at factor 0.5 the two penalties are the same order of magnitude and on this corpus they cancel, which is why the bench needs to sweep the factor.
CONFIG.md gains the knob trio with the demote-vs-exclude trade-off, the reason the penalty is additive, and the two measured calibration facts an operator running the three-arm bench needs.
…king it The candidate limit is applied by the search — SQL LIMIT on the FTS5 path, top_k on BM25F — so filtering superseded beliefs afterwards dropped the pack size by however many were retired and left current, relevant beliefs stranded just below the cutoff: l1_limit=4, three of the top four retired -> 1 belief returned l1_limit=10, eight of the top ten retired -> 2 beliefs returned Same shape as the lock-budget starvation fixed in #1014/#1015: a filter applied after the budget starves the pack. In the degenerate case every top-l1_limit candidate is retired and the arm returned nothing while the store held the answer. The exclusion arm now widens the fetch and retries, stopping as soon as it has l1_limit survivors or the search runs out of matches, bounded at three rounds. The demote arm is untouched — it reorders a fixed candidate set, which is why it always measured full. This also matters for the ratified three-arm bench: with the arms differing in pack size as well as in treatment, a loss for exclusion could not have been attributed to either. Raised in review on #1191.
…pin precedence Two gaps Sourcery flagged, both real. The lane was only exercised through retrieve_v2. Production goes through retrieve(), and 'staged lane wired into retrieve_v2 only' is a defect class this repo has hit before — a lane reachable from tests and the bench harness and from nothing a user runs. The new test drives it by env override, which is the only handle retrieve() exposes. Treatment and factor resolution had normalisation and error-path tests but no precedence tests. Each layer is now asserted while the layer below it disagrees, so a resolver reading one source would fail rather than coincide. Swapping env and kwarg fails it. The retrieve() test's docstring states what it does not catch: a hardcoded False in retrieve() is unobservable because the resolver reads env before kwarg. It guards resolver consultation and _l1_hits wiring, which is what neutering either does fail.
5e72138 to
fc6f736
Compare
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged fc6f736 → |
1 participant
Closes #1187. The retrieval half of #1170, split out because it changes
retrieve()output on the default path.Builds both arms behind one default-OFF flag, per the shape you ratified on 2026-07-29. Nothing here presumes a winner; the three-arm bench picks the default.
The defect, reproduced
grep SUPERSED src/aelfrice/retrieval.pyreturns zero hits — retrieval had no notion of supersession — anduri_baki.apply_supersession_demotehad no importer anywhere insrc/. Measured on this branch's reproduction:What ships
use_supersession_demotefalsesupersession_treatmentdemotedemote|excludesupersession_demote_factor0.5Both resolve env > kwarg > TOML > default, mirroring
use_entity_persist_demote. One batchedSELECT DISTINCT dst … WHERE type='SUPERSEDES'over the candidate set, via thejson_eachbinding idiom from ca97776 rather than an interpolatedINlist. Exclusion is applied before the heat-kernel seeds are computed, so a retired belief does not seed the graph lane either. Threaded through_l1_hits→retrieve_with_tiers→retrieve_v2→retrieve(), resolver-driven, so the lane is reachable from the production path — the gap that left the primitive importerless.Two findings you should read before commissioning the bench
1. The issue's suggested wiring would have inverted the fix. #1187 says to give
uri_baki.apply_supersession_demotea real importer for the demote arm. That primitive multiplies:score * factor. But the composite rerank score is a log-domain quantity fromcombine_log_scores/partial_bayesian_scoreand is routinely negative — measured-13.08on the two-belief reproduction. Multiplying-13.08by0.5gives-6.54, which is higher: it would have promoted the superseded belief to the top of the pack.So the demote is additive:
s += log(factor). That is the log-domain equivalent of scaling a probability byfactor, so your "factor 0.5" semantics survive exactly, and it matches_entity_persist_penalty, which is log-additive for the same reason.uri_baki.apply_supersession_demoteis left unimported — its contract is correct only on a non-negative score scale, which is not what this rerank produces. A test asserts the negative-score premise directly so nobody folds the addition back into a multiplication. Per the issue, the primitive is a delete candidate if the bench picks exclusion; I have not deleted it, since that decision is downstream of the bench.2. At factor 0.5 the demote arm is weak enough that composition can cancel it. Measured on the reproduction with the default-ON entity-persistence lane:
fly.io(current)heroku(superseded)The two nearly cancel, the pre-existing bm25 gap survives, and the order does not change.
log(0.5) = −0.69is the same order of magnitude as the entity penalty and far weaker than itslog(ε) = −6.91floor. The demote is applied correctly — verified in isolation — it is simply a weak term in composition.Recommendation: the bench should sweep
supersession_demote_factor, not test 0.5 alone. A three-arm demote-vs-exclude-vs-control run pinned at 0.5 risks reading "demotion doesn't work" when what it measured was "0.5 is too small to outrun a co-resident lane."Verification
6202 passed, 69 skipped(26 new tests).deptryclean;vulturereports only the pre-existingingest.py:113.retrieve_v2, not just the primitive.superseded_belief_idsto raise and asserts the lane-off path never calls it, so the short-circuit that skips the rerank stays reachable.Out of scope
edge_rerank.py, the other importerless module (belongs with [Umbrella] Inert, unreachable, and decorative mechanisms — graph substrate and the delete list #1162).Unrelated observation
tests/test_promotion_adversarial.pyC6-01…C6-04 now xpass on baremain(verified atca97776, independent of this branch): #1189's all-stopword promotion fix made them pass without un-marking thexfail. Harmless today, but the markers now hide a working guard rather than a known bug. Not touched here.Summary by Sourcery
Introduce a configurable supersession lane in retrieval that can demote or exclude beliefs marked as superseded, wired through the production retrieval path but shipped behind a default-off flag.
New Features:
use_supersession_demote,supersession_treatment,supersession_demote_factor) with env/TOML/kwarg resolution to support demote or exclude behaviour for superseded beliefs.Enhancements:
Tests:
_l1_hitsandretrieve_v2, resolver precedence, factor clamping, and interaction with entity-persistence demotion.Summary by CodeRabbit