Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

### Fixed

- **The frontend now reads the `msg_limit` ceiling from `/api/session` metadata instead of hand-mirroring the constant.** Following the ceiling clamp shipped in exp-v0.52.98, the backend advertises its `_MAX_MSG_LIMIT` as `_msg_limit_max` in every `/api/session` response, and the frontend reads it dynamically (`_msgLimitMax`, a module-scope value defaulting to the static fallback), so the two can no longer drift and the mirrored constant's drift-guard test is no longer needed. Older backends that omit the field fall back to the built-in default, so mixed-version deployments are unaffected. Thanks @webtecnica. (#6214, #6177)

- **`GET /api/session?msg_limit=` is now bounded by a server-side ceiling, and the frontend paginates around it without dropping rows.** A client could request `msg_limit=1000000` (or an outline-jump path asked for `9999`) and force the server to assemble and serialize an unbounded message payload. The backend now clamps `?msg_limit=` to `[1, 500]` via a dedicated `_parse_msg_limit()` helper and sets the existing `_messages_truncated` signal when it clamps; the bare no-`msg_limit` path (branch/undo/jump-to-start) still returns the full transcript. The frontend mirrors the ceiling: `_loadOlderMessages` grows its tail window below the ceiling and switches to bounded `msg_before` backward paging once the server would clamp, and the outline jump uses the bare full-transcript path instead of the old `msg_limit=9999` hack. Two silent row-loss regressions found during review were fixed before ship: a raw-row-heavy `msg_before` page that textually repeated the current tail could be misclassified as the cumulative tail and wholesale-replace it (losing older rows), and a same-session refresh above the ceiling could shrink an already-loaded >500-row transcript to the last 500 — both now route through the correct paging/full-transcript path. Thanks @rh-id. (#6152, #6154, #6177)

- **Background subagents no longer fail to persist when the parent's cached agent is reused mid-turn.** A parent session's `SessionDB` handle is shared by reference with the background subagents it spawns (`delegate_tool`), but the cached-agent reuse path unconditionally closed that handle when the parent got a new turn / server-side wakeup — so every still-running child then failed `append_message` with `'NoneType' object has no attribute 'execute'` (observed 600+ times under one parent). The reuse path now keeps a still-open handle (closing only the unused fresh one, preserving the #1421 FD-leak fix) and only replaces a dead/missing one. The credential self-heal path was hardened the same way: if rebuilding the handle fails it degrades cleanly to a lazy reinit instead of reusing a closed handle. Thanks @carlotestor. (#6143)
Expand Down
1 change: 1 addition & 0 deletions api/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -12769,6 +12769,7 @@ def handle_get(handler, parsed) -> bool:
_truncated = load_messages and msg_limit is not None and _messages_offset > 0
raw["_messages_truncated"] = _truncated
raw["_messages_offset"] = _messages_offset
raw["_msg_limit_max"] = _MAX_MSG_LIMIT
_t4 = _time.monotonic()
if _diag: _diag.stage("t4_after_compact_and_merge")
if effective_model:
Expand Down
20 changes: 13 additions & 7 deletions static/sessions.js
Original file line number Diff line number Diff line change
Expand Up @@ -2935,12 +2935,17 @@ const _INITIAL_MSG_LIMIT = 30;
// ceiling. _loadOlderMessages grows its msg_limit tail window by
// +_INITIAL_MSG_LIMIT each load; once growth would exceed this ceiling the
// server clamps and the tail stops growing, so we switch to msg_before paging
// (a fixed-size backward page keyed off _oldestIdx) instead. If you change the
// backend _MAX_MSG_LIMIT, change this value to match — otherwise load-older
// silently stalls again for long sessions. The durable fix (so this mirror
// isn't needed) is to expose the ceiling in the /api/session metadata response
// and read it here; see tracking issue for that follow-up.
// (a fixed-size backward page keyed off _oldestIdx) instead. This const is the
// static FALLBACK default only — the live ceiling is read from the /api/session
// `_msg_limit_max` metadata into _msgLimitMax below (#6177), so the two can no
// longer drift. Keep this fallback value roughly in sync with the backend
// _MAX_MSG_LIMIT for the mixed-version case where the server omits the field.
const _MSG_LIMIT_MAX = 500;
// Live server-advertised msg_limit ceiling. Declared at module scope with the
// static fallback so the reload-width paths (_ensureMessagesLoaded /
// _loadOlderMessages) always read a defined value even before the first
// /api/session response lands; refreshed from `_msg_limit_max` on each load.
let _msgLimitMax = _MSG_LIMIT_MAX;
let _sameSessionForceReloadHint = null;

function _currentLoadedRenderableMessageCount(){
Expand Down Expand Up @@ -3052,7 +3057,7 @@ async function _ensureMessagesLoaded(sid, opts) {
// window exceeds the ceiling, fall back to the bare full-transcript request
// (no msg_limit / no expand_renderable) so a same-session refresh never drops
// already-loaded older rows (Codex gate #6154, silent row-loss).
const boundedReloadLimit = (reloadLimit && reloadLimit <= _MSG_LIMIT_MAX) ? reloadLimit : null;
const boundedReloadLimit = (reloadLimit && reloadLimit <= _msgLimitMax) ? reloadLimit : null;
const reloadLimitParam = boundedReloadLimit ? `&msg_limit=${boundedReloadLimit}` : '';
// Older frontends used expand_renderable=1 to request visible-row expansion.
// The server now counts msg_limit by visible transcript rows by default; keep
Expand All @@ -3072,6 +3077,7 @@ async function _ensureMessagesLoaded(sid, opts) {
if (!data || !data.session) return;
_messagesTruncated = !!data.session._messages_truncated;
_oldestIdx = data.session._messages_offset || 0;
_msgLimitMax = data.session._msg_limit_max || _MSG_LIMIT_MAX;
// #3162: `msgs` is reassigned below by the #3018 ephemeral-field carry-forward,
// so it must be `let`, not `const`. The `const` form threw a TypeError inside
// _ensureMessagesLoaded() that surfaced as a "Failed to load conversation messages"
Expand Down Expand Up @@ -3570,7 +3576,7 @@ async function _loadOlderMessages() {
// arbitrarily long transcripts. (This is the same paging request the
// race-fallback below uses, proven correct there.)
const requestedLimit = Math.max(_INITIAL_MSG_LIMIT, (S.messages || []).length + _INITIAL_MSG_LIMIT);
const useBeforePaging = requestedLimit >= _MSG_LIMIT_MAX;
const useBeforePaging = requestedLimit >= _msgLimitMax;
const data = useBeforePaging
? await api(
`/api/session?session_id=${encodeURIComponent(sid)}&messages=1&resolve_model=0&msg_before=${_oldestIdx}&msg_limit=${_INITIAL_MSG_LIMIT}`,
Expand Down
6 changes: 6 additions & 0 deletions tests/test_cross_session_message_load_isolation.py
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@ def test_ensure_messages_loaded_ownership_guard_pre_and_post_await():
// the fetch URL drops msg_limit/expand_renderable and mismatches the
// enqueued buildMessageUrl(), stalling the ordered api() harness.
globalThis._MSG_LIMIT_MAX = 500;
// #6177: _msgLimitMax is a module-scope `let` (live server-advertised ceiling,
// defaulting to _MSG_LIMIT_MAX). It's read by _ensureMessagesLoaded's
// boundedReloadLimit and _loadOlderMessages's useBeforePaging; the harness
// injects only the extracted functions, not module-level lets, so define it
// here or those reads resolve undefined -> wrong fetch URL -> ordered-api stall.
globalThis._msgLimitMax = 500;
globalThis._currentMessageRenderWindowSize = () => 1;
globalThis._messageRenderableMessageCount = () => 2;

Expand Down
71 changes: 0 additions & 71 deletions tests/test_msg_limit_ceiling_drift.py

This file was deleted.

31 changes: 31 additions & 0 deletions tests/test_session_msg_limit_ceiling.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,34 @@ def test_parse_msg_limit_zero_and_negative_clamp_to_one():
for msg_limit=0 gets a 1-row window, not the full transcript."""
assert _parse_msg_limit("0") == 1
assert _parse_msg_limit("-5") == 1


# ── #6177: metadata-decoupling — the frontend reads the ceiling from the
# /api/session `_msg_limit_max` field instead of a hand-mirrored constant. ──

from pathlib import Path

_ROUTES_SRC = (Path(__file__).resolve().parents[1] / "api" / "routes.py").read_text(encoding="utf-8")
_SESSIONS_JS = (Path(__file__).resolve().parents[1] / "static" / "sessions.js").read_text(encoding="utf-8")
Comment on lines +72 to +75

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Module-level file reads at collection time

_ROUTES_SRC and _SESSIONS_JS are populated at import time, not inside test functions. If either read_text() call raises (e.g. a permission error or an unexpected working directory), pytest reports the entire module as a collection error — silently preventing test_max_msg_limit_constant_is_reasonable and the other pre-existing unit tests from running at all, with no signal that those tests were skipped rather than passing. Moving the reads inside each test function (or into a pytest.fixture) would isolate the failure to only the affected tests. The from pathlib import Path import mid-file rather than at the top is also worth tidying.



def test_backend_exposes_msg_limit_max_in_session_response():
"""The /api/session handler advertises the ceiling as `_msg_limit_max` so the
frontend never has to hand-mirror _MAX_MSG_LIMIT (#6177 decoupling)."""
assert 'raw["_msg_limit_max"] = _MAX_MSG_LIMIT' in _ROUTES_SRC


def test_frontend_declares_live_ceiling_at_module_scope_with_fallback():
"""`_msgLimitMax` MUST be declared at module scope (not an implicit global)
with the static fallback, so the reload-width paths read a DEFINED value
before the first /api/session response lands — otherwise a cold load reads
`undefined`, drops msg_limit, and full-loads every session."""
assert "let _msgLimitMax = _MSG_LIMIT_MAX;" in _SESSIONS_JS
# refreshed from the response metadata, falling back when the server omits it
assert "_msgLimitMax = data.session._msg_limit_max || _MSG_LIMIT_MAX;" in _SESSIONS_JS


def test_frontend_reload_width_paths_read_the_live_ceiling():
"""Both reload-width decisions read the live `_msgLimitMax`, not the mirror."""
assert "reloadLimit <= _msgLimitMax" in _SESSIONS_JS # _ensureMessagesLoaded
assert "requestedLimit >= _msgLimitMax" in _SESSIONS_JS # _loadOlderMessages
Comment on lines +78 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Source-string assertions rather than behavior tests

All three new tests assert that specific literal strings appear somewhere in the source files — they pass equally whether the string is in live code, a comment, or dead code. AGENTS.md guideline 6 asks for assertions on observable behavior rather than source strings. The existing codebase already uses this pattern (the deleted drift-guard test did the same), so this is a known tradeoff, but the new tests would silently pass even if the string were in a commented-out block or unreachable branch. A complement that validates _msgLimitMax after a simulated _ensureMessagesLoaded response would give stronger guarantees.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2 changes: 2 additions & 0 deletions tests/test_session_unread_dot_on_visit.py
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ def _hidden_completion_script(*, hidden: bool) -> str:
let _messagesTruncated = false;
let _oldestIdx = 0;
let _messageRenderWindowSize = 0;
const _MSG_LIMIT_MAX = 500;
let _msgLimitMax = _MSG_LIMIT_MAX;
let _pendingCarryForwardSnapshot = null;
let _loadingSessionId = 'open';
let _loadSessionGeneration = 0;
Expand Down
4 changes: 3 additions & 1 deletion tests/test_webui_external_refresh_frontend.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,9 @@ def test_same_session_force_reload_keeps_loaded_transcript_width_hint():
# ceiling; an over-ceiling hint would be clamped by the backend and could
# silently shrink an already-loaded transcript, so it falls back to the bare
# full-transcript path (#6152/#6154 ceiling; Codex gate silent row-loss fix).
assert "const boundedReloadLimit = (reloadLimit && reloadLimit <= _MSG_LIMIT_MAX) ? reloadLimit : null;" in SESSIONS_JS
# #6177: the ceiling is now read from /api/session metadata into _msgLimitMax
# (module-scope let, default _MSG_LIMIT_MAX) instead of the mirrored const.
assert "const boundedReloadLimit = (reloadLimit && reloadLimit <= _msgLimitMax) ? reloadLimit : null;" in SESSIONS_JS
assert "const reloadLimitParam = boundedReloadLimit ? `&msg_limit=${boundedReloadLimit}` : '';" in SESSIONS_JS
assert "if (_ownsLoad()) _clearSameSessionForceReloadHint(sid);" in SESSIONS_JS

Expand Down
Loading