fix(background_review): inherit parent's reasoning_config to keep thinking/output_config cache-stable (~38% fewer cache-write tokens on long sessions)

[ziliangpeng]

fix(background_review): inherit parent's reasoning_config to keep thinking/output_config cache-stable

Summary

Fixes #30531. The background skill/memory-review fork (agent/background_review.py) constructs its child AIAgent without propagating reasoning_config from the parent. The fork's default reasoning_config=None causes build_anthropic_kwargs (agent/anthropic_adapter.py) to omit the top-level thinking and output_config fields from the fork's request body. Anthropic's prompt cache is namespaced by thinking presence, so the fork lands in a different cache namespace from the parent and the entire shared prefix is re-written on every review-fork "birth" call.

Same class of bug as #25322 / PR #17276 (system-bytes invariant) and #29567 / PR #29704 (tools[] invariant) — this is the remaining cache-key slot.

Goal

Eliminate cache-write overhead on the background review path. On a captured ~2-day Sonnet-class session, fork-birth requests account for ~38% of the session's total cache_creation_input_tokens even though they share messages[0..N], system, and tools[] with the parent byte-for-byte — the divergence in thinking/output_config alone is enough to push every fork-birth req into a cache miss. After the fix, fork-birth requests read from the parent's warmed cache instead of rewriting.

Direct API verification (independent of Hermes) that thinking is part of the cache key:

T1: same prompt, thinking=enabled, fresh     cache_read=     0   cache_write= 24026
T2: same prompt, thinking=enabled, repeat    cache_read= 24026   cache_write=     0
T3: same prompt, thinking removed            cache_read=     0   cache_write= 24004
T4: same prompt, thinking removed, repeat    cache_read= 24004   cache_write=     0
T5: same prompt, thinking=enabled again      cache_read= 24026   cache_write=     0

The safety contract from #15204 (the review fork must not dispatch terminal / send_message / delegate_task) is preserved: it is enforced by the post-construction set_thread_tool_whitelist({memory, skills, …}) call a few lines below, which gates dispatch, not what the request body transmits.

Implementation

agent/background_review.py — one added kwarg in the AIAgent(...) call inside _spawn_background_review:

reasoning_config=getattr(agent, "reasoning_config", None),

Plus a three-line comment that calls out the cache-key dependency, matching the style of the existing toolset-parity comment one block above (added in PR #29704).

Symmetric inheritance: when the parent's value is None, the fork's is also None and both omit thinking identically; when the parent has reasoning configured, the fork inherits it verbatim.

Reproduction

Captured by routing the Anthropic API through a local HTTP-capture proxy (https://github.com/ziliangpeng/midagent — small FastAPI logger that records request_body + response_body per request) and inspecting outbound /v1/messages traffic during a long real-world session.

Fork-shape requests are identifiable as: (a) messages[-1] carries one of _SKILL_REVIEW_PROMPT / _MEMORY_REVIEW_PROMPT / _COMBINED_REVIEW_PROMPT as the last user message; (b) the top-level thinking and output_config keys are absent (when the parent has reasoning enabled). The first such request after a _spawn_background_review is the "birth" call this PR addresses.

Cost-impact measurement

From the captured 2-day window (~2,040 /v1/messages requests total, Sonnet-class model, parent has reasoning enabled):

Quantity	Main reqs	Review-fork reqs (all)	of which fork "birth"
Request count	1,389	650	72
cache_creation_input_tokens total	8.77 M	11.96 M	7.82 M
cache_read_input_tokens total	133.07 M	96.38 M	0

Fork-birth requests alone account for ~37.7% of the session's total cache_creation_input_tokens — these are exactly the requests this PR converts from cache_write to cache_read. The remaining fork cache-writes (4.14 M) come from the fork's own internal iteration loop and are not affected by this fix.

The percentage is model-pricing-independent. On Opus the same wire pattern costs substantially more per token than on Sonnet, but the structural inefficiency is identical.

Testing

tests/run_agent/test_background_review_cache_parity.py — new positive assertion test_review_fork_inherits_parent_reasoning_config confirms reasoning_config reaches the constructor with the parent's value. The stub gains a non-None reasoning_config so a regression that drops the kwarg surfaces (rather than spuriously passing because both sides default to None) — same pattern PR #29704 used for the toolset stub.

Sanity check: the new positive test fails on origin/main and passes with this PR.

python -m pytest tests/run_agent/test_background_review.py \
                 tests/run_agent/test_background_review_summary.py \
                 tests/run_agent/test_background_review_toolset_restriction.py \
                 tests/run_agent/test_background_review_cache_parity.py -q

Output:

19 passed in 0.79s

Scope

• agent/background_review.py: one added kwarg + 3-line comment.
• tests/run_agent/test_background_review_cache_parity.py: one new positive test, stub updated.
• No production code paths outside the review fork; no schema, public-API, or runtime-whitelist changes.
• No new dependencies.

Related: extends PR #17276 / #25434 (system slot) and PR #29568 / #29704 (tools[] slot) to the thinking / output_config slot — completes the cache-key parity invariant for the background review fork.

[alt-glitch]

Related to #18871 and competing PR #27510 (both propagate reasoning_config to the background review fork). This PR frames the fix specifically around Anthropic cache parity; #27510 targets the same code path for correctness.

[ziliangpeng]

Rebased to latest origin/main (was 1920 commits behind, picked clean with no conflicts). This is still the only unaddressed slot in the background-review cache-key parity invariant — system (#17276), tools[] (#29704), and now thinking/`output_config` (this PR). Mergeable: ✅

@teknium1 @alt-glitch could you take a look?

[ziliangpeng]

Also tagging @liuhao1024 since you reviewed the most recent background-review PR (#41708).

[ziliangpeng]

Just rebased to the latest origin/main (was 1920 commits behind) and cherry-picked cleanly with zero conflicts. Has been verified working on the latest main.

This addresses the last remaining cache-key slot in the background-review fork — system (#17276), tools[] (#29704), and now thinking/output_config (this PR). Without it, every review-fork birth request misses Anthropic's prompt cache and re-writes the entire shared prefix, which is significant for token efficiency.

@teknium1 could you take a look? This is a small, well-tested fix with measurable cost impact.

[teknium1]

Thanks for the focused cache-parity fix. The premise remains valid on current main.

Problems

• The submitted production hunk is stale: git apply --check cannot find its old constructor context. The fork was refactored after the PR base, but current agent/background_review.py:699-715 still constructs AIAgent without reasoning_config.

Suggested changes

• During salvage, add the parent reasoning_config at the current constructor site and retain the regression assertion. AIAgent accepts and forwards this argument (run_agent.py:463, run_agent.py:539), while build_anthropic_kwargs() only emits thinking and output_config when it is configured (agent/anthropic_adapter.py:2642-2660).

Automated hermes-sweeper review.

[ziliangpeng]

Rebased to latest origin/main (was 4,261 commits behind). Cherry-picked cleanly after resolving a conflict in agent/background_review.py — the constructor site was refactored to use a _fork_kwargs dict pattern, so reasoning_config is now added there:

_fork_kwargs["reasoning_config"] = getattr(agent, "reasoning_config", None)

All 4 cache-parity tests pass:

tests/run_agent/test_background_review_cache_parity.py::test_review_fork_inherits_parent_cached_system_prompt PASSED
tests/run_agent/test_background_review_cache_parity.py::test_review_fork_pins_session_start_and_session_id PASSED
tests/run_agent/test_background_review_cache_parity.py::test_review_fork_inherits_parent_toolset_config PASSED
tests/run_agent/test_background_review_cache_parity.py::test_review_fork_inherits_parent_reasoning_config PASSED

Ready for re-review.

[ziliangpeng]

The two failing tests in tests/hermes_cli/test_model_validation.py are pre-existing failures on upstream/main (commit 226e8de82), not introduced by this PR:

FAILED test_probe_sends_client_context_to_gemini
FAILED test_probe_omits_gemini_client_context_for_other_providers

Both fail with TypeError: 'NoneType' object is not subscriptable at mock_urlopen.call_args[0][0] — urlopen is never called, so call_args is None.

[kshitijk4poor]

Merged to main via #64379 — your commit was cherry-picked verbatim (17cfa0f, authorship preserved) with one maintainer follow-up on top (8ef0069) that gates the reasoning_config inheritance on the non-routed path (mirroring the existing not _routed gate on _cached_system_prompt, since a routed aux model has a cold cache and may not accept the parent's effort vocabulary) and adds a routed-path regression test.

Thanks @ziliangpeng for the sharp root-cause analysis and the cache-write measurement — the thinking-presence cache-namespace insight was exactly right. Closing since the change is now on main.