fix(#3263): apply Codex dual-gate MUST-FIX — recompute persisted stale cap + rescale terminal threshold
Codex regression gate (+Opus, both independently) found the default-only guard dropped the stale compressor cap but two sibling paths stayed inconsistent: 1. Per-turn persistence: fallback resolver only ran when context_length was falsy, so a previously-persisted stale 232K survived forever on non-default sessions. Now also runs when _skip_cc_cl, and rescales threshold_tokens to the recomputed real cap (or clears it). 2. Terminal SSE 'done' payload: re-emitted the stale compressor threshold, so messages.js overwrote S.lastUsage and the indicator reverted on stream end. Now rescales threshold to the resolved window when the stale cap was dropped. Added 3 source-structure regression tests pinning both fixes; bumped the brittle test_pr1341 distance limit 13000→15000 (+ noted it should become structural). Co-authored-by: allenliang2022 <allenliang2022@users.noreply.github.com>
This commit is contained in:
1 parent
7a734462b0
commit
18a459726b
3 files changed
+90
-10
No files matched your search
+42
-1
@@ -5917,7 +5917,14 @@ def _run_agent_streaming(
|
||||
# window in the persisted session and the SSE payload —
|
||||
# which then trips LCM auto-compress at ~25% of the wrong
|
||||
# value, cascading into 429 floods.
|
||||
if not getattr(s, 'context_length', 0):
|
||||
#
|
||||
# #3256/#3263: ALSO run this fallback when _skip_cc_cl is true
|
||||
# (non-default model whose compressor carried the stale global
|
||||
# cap). Without this, a session that already had a stale 232K
|
||||
# context_length persisted keeps it forever — skipping the
|
||||
# compressor write removes the re-clobber but never recomputes
|
||||
# the real per-model window. Recompute and overwrite in that case.
|
||||
if (not getattr(s, 'context_length', 0)) or _skip_cc_cl:
|
||||
try:
|
||||
from agent.model_metadata import get_model_context_length
|
||||
_cfg_ctx_len = None
|
||||
@@ -5980,6 +5987,23 @@ def _run_agent_streaming(
|
||||
# Older hermes-agent builds may not expose this helper.
|
||||
# Better to leave context_length=0 than crash the save.
|
||||
pass
|
||||
# #3256/#3263: when we skipped the stale compressor cap for a
|
||||
# non-default model and recomputed the real per-model window
|
||||
# above, rescale the persisted threshold_tokens to that real cap
|
||||
# so the auto-compress trigger and the reloaded context-ring
|
||||
# match the live snapshot (which already rescales). Without this,
|
||||
# a reload shows a smaller compression trigger than streaming did.
|
||||
# Only rescale when both the original cap and threshold are
|
||||
# positive; otherwise clear the threshold to 0 (consistent with
|
||||
# the live-snapshot path) rather than leave a stale value.
|
||||
if _skip_cc_cl:
|
||||
_orig_cap = _cc_cl # the stale global cap the compressor reported
|
||||
_orig_thresh = getattr(s, 'threshold_tokens', 0) or 0
|
||||
_real_cap = getattr(s, 'context_length', 0) or 0
|
||||
if _real_cap > 0 and _orig_cap > 0 and _orig_thresh > 0:
|
||||
s.threshold_tokens = int(_orig_thresh * _real_cap / _orig_cap)
|
||||
else:
|
||||
s.threshold_tokens = 0
|
||||
if not ephemeral and s.messages:
|
||||
_latest_assistant_idx = next(
|
||||
(idx for idx in range(len(s.messages) - 1, -1, -1)
|
||||
@@ -6109,6 +6133,13 @@ def _run_agent_streaming(
|
||||
_cc = getattr(agent, 'context_compressor', None)
|
||||
if _cc:
|
||||
_cc_cl_sse = getattr(_cc, 'context_length', 0) or 0
|
||||
# #3256/#3263: remember the original compressor cap + threshold
|
||||
# so that if we drop the stale cap below and the fallback
|
||||
# resolves the real per-model window, we can rescale the
|
||||
# threshold consistently (the live snapshot already does this).
|
||||
_orig_cc_cl_sse = _cc_cl_sse
|
||||
_orig_cc_thresh_sse = getattr(_cc, 'threshold_tokens', 0) or 0
|
||||
_dropped_stale_cap_sse = False
|
||||
# Default-only guard (#3256): the agent-side context_compressor
|
||||
# is constructed in agent_init with the global model.context_length
|
||||
# applied unconditionally, so for non-default models its
|
||||
@@ -6134,6 +6165,7 @@ def _run_agent_streaming(
|
||||
and _cfg_default_sse != _sess_model_sse
|
||||
):
|
||||
_cc_cl_sse = 0
|
||||
_dropped_stale_cap_sse = True
|
||||
except Exception:
|
||||
pass
|
||||
if _cc_cl_sse:
|
||||
@@ -6198,6 +6230,15 @@ def _run_agent_streaming(
|
||||
)
|
||||
if _fb_cl:
|
||||
usage['context_length'] = _fb_cl
|
||||
# #3256/#3263: if we dropped the stale compressor cap
|
||||
# for a non-default model, the threshold_tokens written
|
||||
# above is still the stale compressor value. Rescale it
|
||||
# to the real resolved window so the terminal `done`
|
||||
# payload matches the live snapshot (which rescales) —
|
||||
# otherwise messages.js overwrites S.lastUsage with the
|
||||
# stale threshold and the indicator reverts on stream end.
|
||||
if _dropped_stale_cap_sse and _orig_cc_cl_sse > 0 and _orig_cc_thresh_sse > 0:
|
||||
usage['threshold_tokens'] = int(_orig_cc_thresh_sse * _fb_cl / _orig_cc_cl_sse)
|
||||
except Exception:
|
||||
pass
|
||||
# Fallback: when last_prompt_tokens is missing (no compressor), use the
|
||||
|
||||
@@ -13,6 +13,7 @@ the real agent metadata catalog.
|
||||
"""
|
||||
import sys
|
||||
import types
|
||||
from pathlib import Path as _Path
|
||||
|
||||
|
||||
def _install_fake_get_model_context_length(monkeypatch, recorder):
|
||||
@@ -93,3 +94,42 @@ def test_empty_model_returns_zero(monkeypatch):
|
||||
_install_fake_get_model_context_length(monkeypatch, rec)
|
||||
assert _resolver()("") == 0
|
||||
assert _resolver()(None) == 0
|
||||
|
||||
|
||||
# --- #3263 dual-gate MUST-FIX invariants (Codex regression gate, v0.51.192) ---
|
||||
# These pin the two consistency fixes applied after the gate found that the
|
||||
# default-only guard dropped the stale cap but didn't (a) recompute a persisted
|
||||
# stale context_length, or (b) rescale the terminal SSE threshold. Both live
|
||||
# deep inside _run_agent_streaming, so we pin them at the source-structure level
|
||||
# (the live-snapshot path already had behavioral coverage; these guard the two
|
||||
# sibling paths from silently regressing back to the stale value).
|
||||
_STREAMING_SRC = (_Path(__file__).resolve().parent.parent / "api" / "streaming.py").read_text(encoding="utf-8")
|
||||
|
||||
|
||||
def test_persistence_fallback_also_runs_when_skip_cc_cl():
|
||||
"""The per-turn persistence fallback must recompute the real cap when the
|
||||
stale compressor cap was skipped — not only when context_length is falsy.
|
||||
Otherwise a previously-persisted stale 232K survives forever."""
|
||||
assert "(not getattr(s, 'context_length', 0)) or _skip_cc_cl:" in _STREAMING_SRC, (
|
||||
"persistence fallback gate must also fire on _skip_cc_cl (#3263 MUST-FIX 1)"
|
||||
)
|
||||
|
||||
|
||||
def test_persistence_rescales_threshold_when_cap_skipped():
|
||||
"""When the stale cap is skipped and the real cap recomputed, the persisted
|
||||
threshold_tokens must be rescaled to the real cap (or cleared), so a reload
|
||||
matches the live snapshot."""
|
||||
assert "if _skip_cc_cl:" in _STREAMING_SRC
|
||||
assert "s.threshold_tokens = int(_orig_thresh * _real_cap / _orig_cap)" in _STREAMING_SRC, (
|
||||
"persistence path must rescale threshold_tokens to the real cap (#3263 MUST-FIX 2)"
|
||||
)
|
||||
|
||||
|
||||
def test_sse_done_payload_rescales_threshold_when_cap_dropped():
|
||||
"""The terminal SSE usage payload must rescale threshold_tokens when it
|
||||
dropped the stale compressor cap, so the indicator doesn't revert on stream
|
||||
end (messages.js overwrites S.lastUsage with this payload)."""
|
||||
assert "_dropped_stale_cap_sse" in _STREAMING_SRC
|
||||
assert "usage['threshold_tokens'] = int(_orig_cc_thresh_sse * _fb_cl / _orig_cc_cl_sse)" in _STREAMING_SRC, (
|
||||
"SSE done payload must rescale threshold_tokens to the resolved window (#3263 MUST-FIX 3)"
|
||||
)
|
||||
@@ -38,15 +38,14 @@ def test_streaming_persists_context_fields_on_session_before_save():
|
||||
# Save call follows shortly after
|
||||
save_call = src.find("\n s.save()", block_start)
|
||||
assert save_call != -1, "s.save() not found after the post-merge marker"
|
||||
# Limit bumped to 13000 by the #3256/#3263 default-only context_length guard:
|
||||
# the pre-save block now also skips the agent compressor's stale global
|
||||
# context_length cap for non-default models (e.g. a 1M-context variant when
|
||||
# model.default carries a 232K cap) so it doesn't clobber the real per-model
|
||||
# window on disk. Earlier the limit was 9000 (cancellation finalization
|
||||
# guards). The context_length fallback is still a single focused resolver
|
||||
# call with arg-prep scaffold and commentary explaining the failure mode it
|
||||
# prevents.
|
||||
assert save_call - block_start < 13000, (
|
||||
# Limit bumped to 15000 by the #3256/#3263 default-only context_length guard
|
||||
# plus its dual-gate consistency fixes (recompute persisted stale cap +
|
||||
# rescale threshold_tokens). The pre-save block legitimately grew here. NOTE:
|
||||
# this byte-distance assertion is itself brittle (it must be bumped whenever a
|
||||
# legitimate pre-save mutation block is added) — a structural check (presence
|
||||
# of s.save() shortly after the post-merge marker) would be more durable; left
|
||||
# as a follow-up. Earlier limits: 9000 (cancellation guards) → 13000 (#3263 v1).
|
||||
assert save_call - block_start < 15000, (
|
||||
"s.save() should be close to the post-merge marker — block expanded unexpectedly. "
|
||||
"If you've added a new pre-save mutation block here, bump this limit."
|
||||
)
|
||||
|
||||
Reference in new issue
Block a user