fix(background_review): inherit parent's reasoning_config for Anthropic cache parity (salvage #30532) - #64379
Merged
kshitijk4poor merged 2 commits intoJul 14, 2026
Conversation
… Anthropic cache namespace PR NousResearch#17276 painstakingly pinned `_cached_system_prompt`, `session_start`, `session_id`, and the toolset config on the background-review fork so its outbound request body would byte-match the parent's and hit Anthropic's exact-prefix cache. The contributor measured a ~26% end-to-end cost reduction on Sonnet 4.5. That optimization is currently being silently undone by a missing `reasoning_config` kwarg. The fork's `AIAgent(...)` call omits it, so the fork's `reasoning_config` defaults to `None`. `anthropic_adapter.build_anthropic_kwargs` (line ~2165) then short-circuits the `thinking` / `output_config` block, and the fork's request body lands in a DIFFERENT Anthropic cache namespace from the parent's. Result on the wire: 0 `cache_read_input_tokens`, full `cache_creation_input_tokens` of the entire parent prefix — every single background review. 7 days of midagent.db traffic from one host running stock Hermes against Anthropic Sonnet: ``` Background-review FIRST calls (the moment a review fork is born): count = 68 cache_write tokens = 7,004,297 cache_read tokens = 1,016,335 Cost on Sonnet ($3.75/M write vs $0.30/M read): Spent on these writes: $26.27 Cost if they had hit parent cache instead: $2.10 WASTED: $24.16 / week / user ``` That is from one user. Multiply by Hermes's installed base for the full impact. Tested against api.anthropic.com directly (see refs/api-tests/ in the attached investigation repo if needed): | pair | cache_r | cache_w | |---------------------------------------------|---------|---------| | parent fresh | 0 | 24,047 | | parent same again | 24,047 | 0 | | fork: appends 2 new tail msgs, thinking ON | 24,047 | 22 | | fork: appends 2 new tail msgs, thinking OFF | 0 | 24,047 | Same fork-shape request, only difference is `thinking`. With the fix, the fork hits the parent's full prefix and only writes the delta (the `Review the conversation above…` prompt block, ~3-5K tokens). One line in `agent/background_review.py`: pass `reasoning_config=getattr(agent, "reasoning_config", None)` to the `AIAgent(...)` constructor of the review fork. A short comment block above it explains why so the next person who reads this code doesn't re-introduce the regression. `tests/run_agent/test_background_review_cache_parity.py` already covers the system-prompt / session-id / toolset-config parity contracts that PR NousResearch#17276 introduced. I added: * a `reasoning_config` attribute to `_make_agent_stub` so the stub has a non-None parent value the test can verify is propagated. * `test_review_fork_inherits_parent_reasoning_config()` — asserts the fork's `AIAgent(...)` kwargs carry the parent's `reasoning_config`. Pre-fix this test fails with `None vs expected {'enabled': True, 'effort': 'medium'}`; post-fix all 4 tests in the file pass. ``` $ python -m pytest tests/run_agent/test_background_review_cache_parity.py -v test_review_fork_inherits_parent_cached_system_prompt PASSED test_review_fork_pins_session_start_and_session_id PASSED test_review_fork_inherits_parent_toolset_config PASSED test_review_fork_inherits_parent_reasoning_config PASSED ← new ``` Also runs against the broader background-review test suite: `test_background_review.py` (4), `test_background_review_summary.py` (8), `test_background_review_toolset_restriction.py` (3) — 19/19 pass. `agent/curator.py:1691` has the same omission for the umbrella-curation fork, but curator's prompt is "curate all skills" — it shares no prefix with any user conversation, so cache-parity is a non-issue there. Worth auditing if the curator ever takes a parent conversation as input, but not part of this PR. The `agent/auxiliary_client.py:1006` `reasoning_config=None` hardcode is intentional (title/summary one-shots on short prompts — per-call cost of namespace flip is negligible) and is also out of scope.
…ed + dedupe recorder stubs
Review follow-up to the reasoning_config cache-parity fix:
- Only inherit the parent's reasoning_config when the fork runs on the
parent's model (not routed). On the routed aux path
(auxiliary.background_review.{provider,model}) the cache is cold
regardless, so parity buys nothing, and the parent's effort vocabulary
can be invalid for the routed model/provider: OpenRouter
extra_body.reasoning.effort is forwarded unclamped
(chat_completions.py) and codex_responses only maps max/ultra for
gpt-5.6 — an exotic parent effort routed to a strict provider could
400 the review. Mirrors the existing 'not _routed' gate on
_cached_system_prompt / session_start three lines below.
- Add a routed-path regression test asserting reasoning_config is
omitted from the fork kwargs when _resolve_review_runtime returns
routed=True.
- Extract the four copy-pasted recorder stubs in
test_background_review_cache_parity.py into a single
_make_recorder_class() factory so a new fork attribute needs one stub
edit, not four.
kshitijk4poor
enabled auto-merge (rebase)
July 14, 2026 11:48
alt-glitch
added
type/perf
This was referenced Jul 14, 2026
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
3 participants
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.
Summary
Salvage of #30532 by @ziliangpeng — the background review fork now inherits the parent's
reasoning_configso the request body'sthinking/output_configstay byte-identical with the parent's and the fork hits the same Anthropic prompt-cache namespace (Anthropic's cache key is namespaced bythinkingpresence). Contributor measured ~38% fewer cache-write tokens on long sessions.Contributor's commit is cherry-picked verbatim (authorship preserved). One maintainer follow-up commit on top addresses review findings.
Contributor commit (48d7681, @ziliangpeng)
agent/background_review.py: passreasoning_config=getattr(agent, "reasoning_config", None)into the review-forkAIAgent.tests/run_agent/test_background_review_cache_parity.py: regression test asserting the fork inherits the parent's non-Nonereasoning_config.Mechanism verified against source:
anthropic_adapter.pyemitsthinking+output_configonly whenreasoning_configis a truthy dict withenabled is not False. Pre-fix, a parent running with thinking sentthinkingwhile the fork (configNone) omitted it — different request-body shape, cache-namespace split on every review nudge. Post-fix both sides match in both directions (None↔None, dict↔dict).Maintainer follow-up (review findings)
not _routed. The contributor's assignment was unconditional, but whenauxiliary.background_review.{provider,model}routes the review to a different model (routed=Truein_resolve_review_runtime), the cache is cold regardless — parity buys nothing — and the parent's effort vocabulary may be invalid for the routed model/provider: OpenRouterextra_body.reasoning.effortis forwarded unclamped (chat_completions.py), and codex_responses passesmax/ultrathrough unmapped except on gpt-5.6/xAI — an exotic parent effort routed to a strict provider could 400 the review. The gate mirrors the existingnot _routedgate on_cached_system_prompt/session_startinheritance a few lines below, so routed forks now consistently get no parent prompt bytes, no parentsession_start, and no parentreasoning_config._resolve_review_runtimeto returnrouted=Trueand assertsreasoning_configis omitted from the fork kwargs._make_recorder_class()factory so a new fork attribute needs one stub edit, not four. Assertion strength preserved (the factory captures full constructor kwargs, enabling presence checks, which is stricter than the old per-kwarg.get()recorders).Also verified: the curator's sibling fork (
agent/curator.py) correctly omitsreasoning_config— it is a fresh independent session with no shared prefix, so cache parity does not apply there; no parallel change needed.Review process
Two independent 4-angle reviews (code reuse / quality / efficiency / Hermes-specific cache invariants) on the contributor SHA, plus a full transport-matrix analysis of
reasoning_confighandling across api_modes (anthropic, chat_completions incl. OpenRouter/GitHub Models/LM Studio/Kimi/Gemini, codex_responses), plus a re-review on the exact final SHA after the follow-up. Verdict on final SHA: approve; non-routed path preserved byte-for-byte (baseline comparison at the contributor commit: all pre-existing tests pass at both SHAs).Testing
tests/run_agent/test_background_review_cache_parity.py— 5 passed (4 original-parity + new routed test)tests/run_agent/test_background_review_toolset_restriction.py— 5 passedtests/run_agent/ -k background_review— 41 passedtests/agent/test_compression_concurrent_fork.py— 23 passed (one known pre-existing threading flake observed once, passes in isolation and on reruns; unrelated to this diff)Related
Supersedes-candidates on the same underlying issue (#18871): #18973, #20674, #27510, #36995 — this PR (#30532's approach) was selected for salvage as it pairs the fix with cache-parity framing and a regression test in the existing parity suite. Closes #30532's intent; original PR to be closed with credit after merge.
Credit: @ziliangpeng for the root-cause analysis, fix, and measurement.