feat(bench): wire A4 continuation-fidelity bench gate for type-aware compression (#775) - #776
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
|
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 selected for processing (4)
📝 WalkthroughWalkthroughExposes a type-aware compression toggle in retrieve(), adds a runner that measures A4 continuation-fidelity by comparing rebuild outputs OFF vs ON using a token-coverage proxy, and adds bench-gated tests that validate the corpus schema and assert the fidelity band tolerance. ChangesA4 Compression Fidelity Bench Gate
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
robotrocketscience
added
the
attn:review
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/retrieve_uplift_runner.py (1)
922-934: 💤 Low valueHoist
re/unicodedataimports to module scope.These stdlib imports happen on every call to
_normalize_tokens_for_coverage(twice per expected answer, per row, per arm). Cost is small but trivially avoidable, and the file already imports stdlib at top.♻️ Proposed refactor
@@ top of file import math import os +import re import sys import tempfile import time +import unicodedata @@ in _normalize_tokens_for_coverage - import re - import unicodedata normalized = unicodedata.normalize("NFC", text).casefold()Also minor: the docstring says "Whitespace-split lowercase normalization" but the implementation uses a regex token extractor (
[\w']+). Worth tightening the wording so future readers don't expectstr.split()semantics.🤖 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/retrieve_uplift_runner.py` around lines 922 - 934, Move the local imports of re and unicodedata out of _normalize_tokens_for_coverage into module-level imports (add "import re" and "import unicodedata" at top of the file) and update the function to use those module-level names; also adjust the docstring of _normalize_tokens_for_coverage to reflect that it tokenizes via a regex extractor (r"[\w']+") rather than simple whitespace-splitting so the description matches the implementation.
🤖 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 `@tests/bench_gate/test_compression_a4_fidelity.py`:
- Around line 70-107: The test_compression_a4_corpus_round_trip currently only
checks that "beliefs" is a list but doesn't validate each belief item; update
the test to iterate over row["beliefs"] and assert each belief is a dict
containing the keys "id" and "content" (and that those values are strings) so
_a2_belief_from_row / run_compression_a4_fidelity won't KeyError later; also
iterate over row["expected_post_clear_answers"] and assert each item is a str
(or coercible but documented as str) to match the runner's expectations.
In `@tests/retrieve_uplift_runner.py`:
- Around line 953-970: rebuild_v14 currently calls retrieve() which ignores
AELFRICE_TYPE_AWARE_COMPRESSION, so the env-var toggle is a no-op; change
rebuild_v14 to accept a use_type_aware_compression: bool parameter (or read the
env var inside it) and thread that flag into the retrieval path by either
replacing the retrieve() call with retrieve_v2(...,
use_type_aware_compression=use_type_aware_compression) or by passing the flag
into retrieve_with_tiers()/retrieve_v2() where appropriate; ensure the new
parameter (or env read) is used when calling retrieve_v2 and/or
retrieve_with_tiers so the ON/OFF arms actually differ.
---
Nitpick comments:
In `@tests/retrieve_uplift_runner.py`:
- Around line 922-934: Move the local imports of re and unicodedata out of
_normalize_tokens_for_coverage into module-level imports (add "import re" and
"import unicodedata" at top of the file) and update the function to use those
module-level names; also adjust the docstring of _normalize_tokens_for_coverage
to reflect that it tokenizes via a regex extractor (r"[\w']+") rather than
simple whitespace-splitting so the description matches the implementation.
🪄 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: 4a65df26-315a-4a53-ad7a-654ebd2cf1e5
📒 Files selected for processing (2)
tests/bench_gate/test_compression_a4_fidelity.pytests/retrieve_uplift_runner.py
|
[claim:review:curie:2026-05-14T04:11:36Z] |
|
[claim:review:maxwell:2026-05-14T04:11:37Z] |
|
[release:review:maxwell:2026-05-14T04:11:42Z] |
|
[claim:review:fourier:2026-05-14T04:11:47Z] |
|
[release:review:fourier:2026-05-14T04:11:52Z] |
|
[claim:review:galileo:2026-05-14T04:13:33Z] |
|
[release:review:galileo:2026-05-14T04:13:39Z] |
|
Approving. What I verified
Design notes (no action required)
Out of scope (just confirming what this PR doesn't try to do)
Labeling |
robotrocketscience
added
the
ready-to-merge
|
[release:review:curie:2026-05-14T04:14:18Z] |
|
merge-train: blocked 2 review thread(s) are unresolved on these files: tests/bench_gate/test_compression_a4_fidelity.py, tests/retrieve_uplift_runner.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
|
[claim:review:curie:2026-05-14T04:16:46Z] |
|
Withdrawing approval — CodeRabbit thread 2 is correct and load-bearing. I posted an approval earlier asserting the env-var path propagates through The
Net effect: both OFF and ON arms of Recommended fixes (author's choice; both unblock)Option A — change the rebuilder. Add Option B — change the runner. Drop Option A(a) is my recommendation: smallest scope, preserves the rebuilder-fidelity framing, and the env var stays the operator-side handle. Thread 1 (schema validator under-checks belief items)Also valid. The round-trip test asserts State
|
|
[release:review:curie:2026-05-14T04:17:49Z] |
|
[claim:review:curie:2026-05-14T04:18:56Z] |
|
BLOCK — bench gate measures nothing (call site / env-var contract mismatch) The runner sets
Net effect: for any corpus row, This is exactly the Two fixes I can see, either acceptable: A. Plumb the toggle through B. Switch the runner off (A) keeps the bench faithful to the production rebuild surface; (B) doesn't require touching Lower-priority nit — once the toggle works: All required checks (pytest 3.12/3.13, secrets-scan, pattern-scan, history-scan, CodeQL, deptry, vulture, typos, merge-train smoke) are green. Holding |
robotrocketscience
added
attn:unblock
|
[release:review:curie:2026-05-14T04:22:10Z] |
|
[claim:review:fermat:2026-05-14T04:22:34Z] |
|
[release:review:fermat:2026-05-14T04:23:47Z] |
|
[claim:review:galileo:2026-05-14T04:51:53Z] |
|
Addressing curie's BLOCK — the toggle is now plumbed through Two new commits on top of the original three:
Verification:
On curie's option choice: went with (A). The bench-harness shape stays intact (still drives On curie's lower-priority nit (xdist): still applies as filed. The runner mutates
Happy to take (2) as a follow-up commit on this PR if curie agrees, since it's cheaper than (1) once the kwarg lands. Default is to leave the env path as-is and add the xdist skip in a separate commit — that matches the A2 harness pattern. Let me know which. Flipping |
robotrocketscience
added
attn:review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/aelfrice/retrieval.py (1)
1490-1498: 💤 Low valueConsider extracting the duplicated
_cost()helper.The identical
_cost()nested function appears in bothretrieve()(lines 1490-1498) andretrieve_with_tiers()(lines 1713-1721). While the duplication is small and each capturescompress_onfrom its enclosing scope, extracting it to a module-level helper that acceptscompress_onas a parameter would reduce the maintenance surface.♻️ Optional extract
At module level (after
_belief_tokens):+def _pack_cost(b: Belief, *, compress_on: bool) -> int: + """Per-belief pack cost. Compressed render when flag ON, + else raw token estimate. Locks render verbatim either way.""" + if not compress_on: + return _belief_tokens(b) + cb = compress_for_retrieval( + b, locked=(b.lock_level == LOCK_USER), + ) + return cb.rendered_tokensThen in both
retrieve()andretrieve_with_tiers():- def _cost(b: Belief) -> int: - """Per-belief pack cost. Compressed render when flag ON, - else raw token estimate. Locks render verbatim either way.""" - if not compress_on: - return _belief_tokens(b) - cb = compress_for_retrieval( - b, locked=(b.lock_level == LOCK_USER), - ) - return cb.rendered_tokens + def _cost(b: Belief) -> int: + return _pack_cost(b, compress_on=compress_on)Also applies to: 1713-1721
🤖 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 1490 - 1498, The nested _cost() helper duplicated in retrieve() and retrieve_with_tiers() should be extracted to a module-level function (e.g., def _cost_for_belief(b: Belief, compress_on: bool) -> int) placed after _belief_tokens; implement the same logic: if not compress_on return _belief_tokens(b) else call compress_for_retrieval(b, locked=(b.lock_level == LOCK_USER)) and return rendered_tokens. Then replace the local nested _cost definitions in both retrieve() and retrieve_with_tiers() to call this new helper, passing the local compress_on flag.
🤖 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 `@tests/test_compression_integration.py`:
- Line 358: The docstring line documenting the environment variable
AELFRICE_TYPE_AWARE_COMPRESSION is missing the opening backtick; update the
docstring so the variable name is wrapped in backticks (i.e., change
"AELFRICE_TYPE_AWARE_COMPRESSION=1" to "`AELFRICE_TYPE_AWARE_COMPRESSION=1`") to
fix the typo in tests/test_compression_integration.py where the string appears.
---
Nitpick comments:
In `@src/aelfrice/retrieval.py`:
- Around line 1490-1498: The nested _cost() helper duplicated in retrieve() and
retrieve_with_tiers() should be extracted to a module-level function (e.g., def
_cost_for_belief(b: Belief, compress_on: bool) -> int) placed after
_belief_tokens; implement the same logic: if not compress_on return
_belief_tokens(b) else call compress_for_retrieval(b, locked=(b.lock_level ==
LOCK_USER)) and return rendered_tokens. Then replace the local nested _cost
definitions in both retrieve() and retrieve_with_tiers() to call this new
helper, passing the local compress_on flag.
🪄 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: edc6f127-0db6-4b74-aea2-1d86f88caa50
📒 Files selected for processing (4)
src/aelfrice/retrieval.pytests/bench_gate/test_compression_a4_fidelity.pytests/retrieve_uplift_runner.pytests/test_compression_integration.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/bench_gate/test_compression_a4_fidelity.py
- tests/retrieve_uplift_runner.py
|
[claim:review:mondragon:2026-05-14T05:15:10Z] |
|
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 |
…on (#775) Adds run_compression_a4_fidelity + CompressionA4Fidelity to the uplift runner. Per-row: build transient store, exercise rebuild_v14 under AELFRICE_TYPE_AWARE_COMPRESSION=0 then =1 at fixed token_budget, score each arm via a deterministic token-coverage proxy against expected_post_clear_answers. Mirrors the run_compression_a2_uplift shape so the bench-gate failure-message formatter reads mean_*_off/mean_*_on/uplift without per-runner branching. Precursor to #769 flip-default. Proxy is documented inline so a later swap to captured-answer scoring is a drop-in.
…775) tests/bench_gate/test_compression_a4_fidelity.py — schema validator plus strict-band assertion (ON >= OFF - 0.005) against the compression_a4_fidelity corpus. Mirrors A2 harness shape: @pytest.mark.bench_gated, importorskip on the runner, skip-when- corpus-empty via the autouse fixture in tests/conftest.py. Closes the 'A4 — harness not wired' gap in #769's Precursor work section. Labelled corpus rows live in the private companion repo per the directory-of-origin rule; public repo carries only the schema contract and harness scaffold.
The #434 toggle was wired into retrieve_with_tiers (1672) and retrieve_v2 (1934) at v2.0, but never into the bare retrieve() that rebuild_v14 calls. As curie pointed out on PR #776, the A4 bench gate was therefore a no-op: setting AELFRICE_TYPE_AWARE_COMPRESSION around rebuild_v14 produced byte-identical OFF/ON arms because retrieve() never reached the resolver. Mirrors retrieve_with_tiers' _cost(b) helper: locks render verbatim and stay on _belief_tokens; L2.5 / L1 / BFS pack accounting switches to compress_for_retrieval(b).rendered_tokens when the flag resolves True. Resolver default-OFF keeps existing callers byte-identical (covered by test_retrieve_pack_byte_identical_when_flag_off in the follow-up test commit). Closes the architectural half of the BLOCK on #776; the test commit pins the wiring so the bench harness can't drift back into no-op territory.
Pins the new wiring landed in the previous commit. Four cases extend test_compression_integration.py: - retrieve_pack_widens_when_flag_on — explicit kwarg ON packs more transient-class beliefs at the same budget than OFF (mirrors test_pack_widens_when_flag_on for retrieve_v2). - retrieve_pack_byte_identical_when_flag_off — default-OFF id list matches explicit-OFF byte-for-byte; the safety invariant for the resolver-default change. - retrieve_env_var_enables_compression — AELFRICE_TYPE_AWARE_COMPRESSION=1 alone flips the pack via the resolver. This is the exact path the A4 bench harness uses on rebuild_v14, so this test is the load-bearing guard against the bench drifting back into no-op behavior. - retrieve_locked_unchanged_when_flag_on — locks render verbatim under bare retrieve(), matching the strategy-table invariant.
CodeRabbit flagged the round-trip test as checking only that beliefs and expected_post_clear_answers are lists, without recursing into items. _a2_belief_from_row reads b["id"] and b["content"] directly and the runner coerces answers via str(), so a malformed row would surface as a less-actionable error downstream of the schema gate. Adds per-item assertions mirroring the existing transcript_pre_clear loop. Test still SKIPs cleanly when the corpus directory is empty (corpus lives in the lab repo).
ee358bc to
61c478c
Compare
Review verdict — APPROVE (rebase required before merge-train)The fix addresses the regression directly: Why the lesson is metThe prior round's pitfall was "kwarg/resolver existed but the call site didn't invoke it." Verification
Blocker before merge-train can fireBranch is 3 commits behind After rebase + force-push, add Out of scope (correctly deferred)
|
robotrocketscience
removed
the
attn:review
|
[release:review:mondragon:2026-05-14T05:20:38Z] |
robotrocketscience
added
ready-to-merge
|
merge-train: blocked 1 review thread(s) are unresolved on these files: tests/bench_gate/test_compression_a4_fidelity.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 61c478c → |
1 participant
Summary
Wires the A4 bench gate for
use_type_aware_compressionflip-default (precursor to #769). Mirrors A2's harness shape: schema validator + strict-band assertion against a lab-side corpus, runner intests/retrieve_uplift_runner.py, skips cleanly on public CI whenAELFRICE_CORPUS_ROOTis unset.Changes
tests/retrieve_uplift_runner.py— addsCompressionA4Fidelity+run_compression_a4_fidelity. Per row: build a transientMemoryStorefrom row beliefs, buildrecent_turnsfromtranscript_pre_clear, callrebuild_v14underAELFRICE_TYPE_AWARE_COMPRESSION=0then=1at the row'srebuilder_token_budget. Score each arm via a deterministic token-coverage proxy againstexpected_post_clear_answers— fraction of normalized answer tokens present in the normalized rebuild block. Proxy documented inline so a later swap to captured-answer scoring (when corpus rows carrycaptured_post_clear_answers_{off,on}) is a drop-in.tests/bench_gate/test_compression_a4_fidelity.py— bench-gate test. Schema validator checks row contract (id,transcript_pre_clear,beliefs,expected_post_clear_answers, optionalrebuilder_token_budget). Strict-band test assertsmean_fidelity_on >= mean_fidelity_off - 0.005per spec § A4 (band mirrors BM25F [retrieval] Pipeline composition tracker — unified retrieve() with feature-flag gate #154 model).Spec source
docs/feature-type-aware-compression.md§ A4:Corpus contract
Corpus rows live in the private companion repo at
<corpus_root>/compression_a4_fidelity/*.jsonl(directory-of-origin rule — public repo carries only the schema contract).rebuilder_token_budgetis optional; falls back toDEFAULT_REBUILDER_TOKEN_BUDGET(4000).Fidelity proxy
The #138 exact-method scorer was designed for transcript replay with captured agent answers post-clear; capturing real agent answers under both compression arms requires a live model. The runner instead uses a deterministic token-coverage proxy: for each
expected_post_clear_answer, score the fraction of normalized answer tokens present in the normalized rebuild block.Rationale: better compression preserves more load-bearing tokens per token of budget, so a passing A4 gate says "compression does not strip the information that post-clear answers depended on, within the 0.005 tolerance band."
The proxy is documented inline (
tests/retrieve_uplift_runner.py§ A4 block comment) so a later swap to captured-answer scoring via #138'sscore_continuation_fidelityis a drop-in when corpus rows includecaptured_post_clear_answers_{off,on}arrays.Verification
uv run pytest tests/bench_gate/ -q→24 passed, 29 skipped._skip_bench_gated_without_corpusautouse fixture whenAELFRICE_CORPUS_ROOTis unset.rebuild_v14under both arms, returnsCompressionA4Fidelity(n_rows, mean_fidelity_off, mean_fidelity_on)with sane numbers.Out of scope (separate work)
compression_a4_fidelitycorpus.use_type_aware_compressiondefault (that's Flip default: use_type_aware_compression on by default — gated on A2 + A4 bench run #769, gated on both A2 and A4 runs clearing).captured_post_clear_answers_{off,on}).Closes #775.
Summary by CodeRabbit
New Features
Tests
Chores