fix(agent): honor prompt_caching.cache_ttl=off on MoA and fallback stub paths - #76113
fix(agent): honor prompt_caching.cache_ttl=off on MoA and fallback stub paths#76113686f6c61 wants to merge 4 commits into
Conversation
Blank SimpleNamespace stubs used by MoA decoration and plan_cache_sections_for_destination never set _cache_disabled, so anthropic_prompt_cache_policy re-injected cache_control markers after operators turned caching off. Stamp the disable onto those stubs from an explicit flag or the live config, and pass the agent flag from the MoA aggregator path. Fixes NousResearch#76085
Avoid F401 from ruff/pyflakes on the NousResearch#76085 regression file.
cc71231 to
e16781f
Compare
|
Rebased onto current Follow-up from review hygiene elsewhere:
Tip is |
|
Note for maintainers: #76121 opened later against the same issue (#76085) with a similar approach (stamp This PR was first (opened ~11:07 UTC; #76121 ~11:35 UTC). Happy to fold any extra coverage from #76121 (e.g. auxiliary client tests) into this branch if useful, or to stand down if you prefer consolidating there — just say which to keep. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the blank-stub policy paths; the current-main premise is real: agent/agent_runtime_helpers.py:1889 and agent/moa_loop.py:409 both pass a stub without _cache_disabled to the policy.
Problems
agent/moa_loop.py:1698accessesself._agentdirectly and coerces an unavailable agent toFalse. The existing prepared-aggregator test builds the facade viaMoAChatCompletions.__new__without setting_agent(tests/agent/test_moa_aggregator_cache_control.py:133-153), so this raises inside the plannertry, skips planning, and breaks its expected tool marker. An explicitlyNoneagent would also suppress the new planner's config fallback.
Suggested changes
- Preserve a tri-state value here: use
getattr(self, "_agent", None)and passNonewhen no live_cache_disabledsnapshot exists, allowingplan_cache_sections_for_destination()to read config as intended. - Add a regression for the prepared-aggregator no-agent/config-off path.
Automated hermes-sweeper review.
Prepared-aggregator facades built via __new__ lack _agent. Accessing self._agent raised inside the planner try and bool-coercion of a missing snapshot forced False, suppressing config fallback for cache_ttl=off. Pass a tri-state value and add a no-agent/config-off regression.
|
Thanks — addressed on the latest tip.
Docker: |
|
@JoaoMarcos44 thanks for the careful side-by-side — that is exactly the coordination I was hoping for. I ported the pieces from #76121 that were not yet here onto this tip:
Docker: If maintainers are happy merging this branch as the single path, you can close #76121 as superseded; if they prefer yours as the merge target I am fine either way. Thanks again for the comparison. |
|
Closed #76121 as superseded — verified your ported changes match (pulled the branch, diffed, reran the tests locally, 15/15 + 193 passed on the broader suite). Appreciate you folding those in cleanly. One ask: since That's the standard way GitHub attributes a merged commit to both authors. Thanks again for coordinating on this. |
Absorb the useful deltas from the parallel NousResearch#76121 approach: a single blank_cache_policy_stub factory so _cache_disabled cannot be left off hand-rolled SimpleNamespaces, and pin the live agent disable onto MoA advisor fan-out and one-shot aggregate_moa_context decoration so those paths track conversation state rather than a fresh config re-read. Keeps the earlier tri-state prepared-aggregator no-agent fix. Adds factory and synthesis/advisor regressions. Coordinates with NousResearch#76121 / NousResearch#76085. Co-authored-by: JoaoMarcos44 <87440198+JoaoMarcos44@users.noreply.github.com>
1559981 to
ebf54b1
Compare
|
@JoaoMarcos44 done — amended the consolidate commit with your trailer and force-pushed. Tip is now Thanks again for closing #76121 as superseded and for the careful verification. |
Attribution entry for PR #76113 salvage (github@00b.tech -> 686f6c61).
…d stub paths Follow-ups from review of NousResearch#76113: - Extract cache_ttl_means_disabled() as the single disable-synonym predicate; agent_init and prompt_caching_disabled_from_config both use it so the two detection sites can no longer drift (drift would recreate the NousResearch#76085 bug class). - Mirror _run_reference's not-None injection guard in aggregate_moa_context (stamping None was a harmless no-op copy). - Replace a vacuous trailing test assertion with the intended input-non-mutation check; drop a stray blank line. - Add a predicate-parity regression test (unknown TTL values keep caching enabled, matching historical agent_init semantics).
…d stub paths Follow-ups from review of #76113: - Extract cache_ttl_means_disabled() as the single disable-synonym predicate; agent_init and prompt_caching_disabled_from_config both use it so the two detection sites can no longer drift (drift would recreate the #76085 bug class). - Mirror _run_reference's not-None injection guard in aggregate_moa_context (stamping None was a harmless no-op copy). - Replace a vacuous trailing test assertion with the intended input-non-mutation check; drop a stray blank line. - Add a predicate-parity regression test (unknown TTL values keep caching enabled, matching historical agent_init semantics).
|
Merged via #76622 — your four commits were cherry-picked onto current main with authorship preserved (rebase merge, no squash), including the Co-authored-by credit for @JoaoMarcos44 from the #76121 consolidation. Commits on main: 8a29703, 67db870, 8e1a351, 8ee5174 (+ 88a629b, a small follow-up from review: shared Review verification before merge: caching-enabled output verified byte-identical to main across native Anthropic / OpenRouter Claude / non-caching routes (the disable now strips markers on all stub paths), plus a mutation check proving the new tests fail when the stub flag is defeated. Solid fix and excellent coordination with the duplicate PR — thanks! |
|
Thanks @kshitijk4poor — glad the four commits landed via #76622 with authorship preserved, and the follow-up ( |
Attribution entry for PR NousResearch#76113 salvage (github@00b.tech -> 686f6c61).
…d stub paths Follow-ups from review of NousResearch#76113: - Extract cache_ttl_means_disabled() as the single disable-synonym predicate; agent_init and prompt_caching_disabled_from_config both use it so the two detection sites can no longer drift (drift would recreate the NousResearch#76085 bug class). - Mirror _run_reference's not-None injection guard in aggregate_moa_context (stamping None was a harmless no-op copy). - Replace a vacuous trailing test assertion with the intended input-non-mutation check; drop a stray blank line. - Add a predicate-parity regression test (unknown TTL values keep caching enabled, matching historical agent_init semantics).
Attribution entry for PR NousResearch#76113 salvage (github@00b.tech -> 686f6c61).
…d stub paths Follow-ups from review of NousResearch#76113: - Extract cache_ttl_means_disabled() as the single disable-synonym predicate; agent_init and prompt_caching_disabled_from_config both use it so the two detection sites can no longer drift (drift would recreate the NousResearch#76085 bug class). - Mirror _run_reference's not-None injection guard in aggregate_moa_context (stamping None was a harmless no-op copy). - Replace a vacuous trailing test assertion with the intended input-non-mutation check; drop a stray blank line. - Add a predicate-parity regression test (unknown TTL values keep caching enabled, matching historical agent_init semantics).
What does this PR do?
prompt_caching.cache_ttl: offsets_cache_disabledon the realAIAgentininit_agent. Main-loop requests honor that flag. MoA advisor/aggregator decoration and the sharedplan_cache_sections_for_destination()helper do not: they resolve policy against a blankSimpleNamespacestub, sogetattr(stub, "_cache_disabled", False)is always false andcache_controlmarkers still get injected.That breaks the documented contract from #33555 that the config disable is global. Operators who turn caching off (providers that mishandle markers, OAuth extra-usage concerns, debugging) still saw markers on:
plan_cache_sections_for_destinationFix
prompt_caching_disabled_from_config()with the same TTL disable detection asinit_agent._cache_disabledonto the policy stub in:plan_cache_sections_for_destination()(optionalcache_disabledparam; defaults to config)_maybe_apply_moa_cache_control()(same)getattr(self._agent, "_cache_disabled", False)so a live agent disable wins when available.Caching-enabled routes are unchanged: with
cache_disabled=False/ default TTL, native Anthropic destinations still receive breakpoints.Related Issue
Fixes #76085
Type of Change
Changes Made
agent/agent_runtime_helpers.py— config helper + stub flag inplan_cache_sections_for_destinationagent/moa_loop.py— stub flag in_maybe_apply_moa_cache_control; pass agent flag from prepared aggregatortests/agent/test_cache_disabled_on_stubs.py— config detection, plan-path strip, MoA decoration strip, enabled path still marksHow to Test
Docker (
ghcr.io/astral-sh/uv:python3.12-bookworm-slim+uv pip install -e ".[dev]"): 7/7 passed.Manual:
cache_controlmarkersChecklist
Code
fix(agent): …)Documentation & Housekeeping
Notes
I defaulted the stub paths to read config when the caller does not pass an explicit flag so auxiliary fallback does not need a full agent object. When MoA has
self._agent, the agent flag is still threaded for consistency with runtime state after init.