Skip to content

feat: request-side overflow guard + uncalibrated-window WARN (#347) - #348

Open
ranxianglei wants to merge 3 commits into
masterfrom
2026-08-28_overflow-guard
Open

feat: request-side overflow guard + uncalibrated-window WARN (#347)#348
ranxianglei wants to merge 3 commits into
masterfrom
2026-08-28_overflow-guard

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Fixes #347

Problem

When a model reports limit.context = 0 (custom OpenAI-compatible providers), state.modelContextLimit is never set — the model-limit catalog drops limit <= 0 and the system hook guards on limit.context. Every percentage threshold (minContextLimit / maxContextLimit / emergencyThresholdPercent) then resolves to undefined and silently no-ops. The session grows past the backend's real window and dies on a provider 400 ("Requested token count exceeds the model's maximum context length…") that opencode swallows — exit 0, no output — a silent, deterministic, unrecoverable death loop.

Changes

Fix 1 — make the blindness visible (lib/messages/uncalibrated-window.ts, new):
trackUncalibratedWindow logs a one-time per-session WARN after 3 consecutive transforms with an unresolved window, telling the user to declare limit in opencode.json or set absolute compress.maxContextLimit/minContextLimit. The counter resets once a window resolves.

Fix 2 — request-side hard guard (lib/messages/prune-to-fit.ts, new):
pruneToFit deterministically clears the oldest compressible (non-protected) tool outputs when the estimated wire size exceeds safeBudget = knownWindow − overflowGuardReserve, independent of model cooperation. This converts a hard 400 into a degraded-but-working turn.

  • knownWindow = modelContextLimit (real window) → else absolute modelMaxLimits[provider/model] → else absolute maxContextLimit; undefined (guard off) when only a percent is set. A percent is a nudge threshold, not a window — using it would massively over-prune.
  • Estimate = O(1) provider-reported usage + an 8192-token margin (covers the new user message + nudges); precise tokenization only when there's no provider token data.
  • Skips the current turn, user messages, the recent-message protection zone, protected tools, and protected file paths (Bug 39 parity). Idempotent. Logs WARN on fit, ERROR if it clears everything but still exceeds the window.

Config (validated + schema'd): compress.overflowGuard (bool, default true), compress.overflowGuardReserve (number, default 32768, covers opencode's 32000 fallback for limit.output = 0).

State: two new transient (non-persisted) fields — uncalibratedWindowTransforms, uncalibratedWindowWarned. Old state files load unchanged.

Not in scope (documented in devlog)

Verification

  • npm run typecheck — clean
  • npm run build — clean
  • npm run test1054/1054 pass (25 new tests in tests/prune-to-fit.test.ts)
  • scripts/ci/check-pr.sh — branch name, devlog, changelog all pass

Devlog: devlog/2026-08-28_overflow-guard/ (REQ + DESIGN + WORKLOG).

When a model reports limit.context=0 (custom providers), state.modelContextLimit
is never set so every percentage threshold resolves to undefined and silently
no-ops; the session then grows past the backend's real window and dies on a
provider 400 that opencode swallows (exit 0, no output) — a silent, deterministic,
unrecoverable death loop.

Two fixes:
- Fix 1 (visibility): trackUncalibratedWindow logs a one-time per-session WARN
  after 3 consecutive transforms with an unresolved window, telling the user to
  declare limit in opencode.json or set absolute compress.maxContextLimit.
- Fix 2 (hard guard): pruneToFit deterministically clears the oldest compressible
  (non-protected) tool outputs when the estimated wire size exceeds
  knownWindow - overflowGuardReserve, independent of model cooperation. knownWindow
  = modelContextLimit, else absolute modelMaxLimits[provider/model], else absolute
  maxContextLimit. Converts a hard 400 into a degraded-but-working turn.

New config: compress.overflowGuard (bool, default true),
compress.overflowGuardReserve (number, default 32768).
New transient (non-persisted) state fields: uncalibratedWindowTransforms,
uncalibratedWindowWarned.

Tests: tests/prune-to-fit.test.ts (25 tests). Full suite 1054/1054 pass.
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

📦 Built Plugin Artifact

Branch: 2026-08-28_overflow-guard (addc530)

Option A — Install from npm PR tag (recommended)

opencode plugin opencode-acp@pr-348 --global

Each push to this PR publishes a new version under the pr-348 npm tag.

Option B — Install from GitHub

opencode plugin "github:ranxianglei/opencode-acp#2026-08-28_overflow-guard" --global

Option C — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf opencode-acp-pr348.tgz
cp -r package/dist ~/.cache/opencode/packages/opencode-acp@latest/node_modules/opencode-acp/dist
  1. Restart opencode to pick up changes.

This comment is automatically updated on each push.

Review round 1 (dual-agent) findings:

- B1 (blocking): estimateWireTokens read only the last assistant's
  provider-reported usage, which is the context size AFTER the last LLM call
  and so omits tool outputs appended after that call. A mid-turn sub-request
  (opencode runs messages.transform on every LLM call) could be
  under-estimated and still 400. Now adds the last assistant's trailing
  completed tool outputs to the estimate (conservative: exact when the last
  step has text, over-counts only for tool-calls-only steps — the safe
  direction). Regression test added and verified to fail without the fix.

- Test hardening: reworked the vacuous recent-zone test (gap now forces the
  guard into the zone; asserts the zone-protected message is skipped and an
  ERROR is logged); added a production-shape test (last assistant carrying a
  trailing completed tool part) covering current-turn protection; added an
  ERROR log for the over-budget-but-nothing-clearable case; added a test
  pinning an explicit overflowGuardReserve: 0 (nullish, not falsy).

- Docs: DESIGN.md + WORKLOG.md updated to reflect the B1 estimator change and
  the review round.

Suite: 1057/1057 pass, typecheck clean, build clean.
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] PR review complete — REQUEST-CHANGES (bounded; 1 source fix + test fixes). Two independent agent reviews per AGENTS.md §5.3/§5.6, with me re-verifying every load-bearing claim against the code.

✅ Verified green (ran locally on the branch)

  • npm run typecheck clean, npm run build clean, npm run test 1054/1054 pass
  • scripts/ci/check-pr.sh 2026-08-28_overflow-guard origin/master — branch name, devlog (REQ+WORKLOG+DESIGN all present and high quality), changelog: all pass
  • Backward compat: the two new state fields are genuinely transient — saveSessionState (lib/state/persistence.ts:118-146) uses an explicit whitelist that excludes them; old state files load with defaults. No dcp tag changes. No as any/@ts-ignore in new lib code.
  • No log injection: all guard log details are numeric/static; tool content never reaches logs.
  • No block-state corruption: prune() (lib/hooks.ts:265) removes block-covered messages before the guard runs (lib/hooks.ts:273); blocks track IDs, not content.
  • Percent values are correctly never used as a window; reserve >= window → no-op; negative reserve rejected by config-validation.

🔴 MAJOR findings

M1. Stale estimate → guard over-fires on the turn after a compresslib/messages/prune-to-fit.ts:88-99,125
estimateWireTokens uses getCurrentTokenUsage = the provider-reported usage of the previous request, but prune() runs first (lib/hooks.ts:265) and may have removed a large compressed range. Empirically demonstrated by the reviewer against the real code: an array whose actual content was ~117k tokens (50k under safeBudget 167232, after a ~40k compress) still produced estimate: 178292 from the stale 170k assistant usage, and the guard cleared a 28k tool output it didn't need to lose. This is the feature's most common trigger path (context grows past budget → model compresses → next transform over-fires), and it destroys context the model deliberately kept. One-shot and self-correcting (re-run the tool), which is why it's MAJOR not BLOCKER — but it should be fixed before merge. Suggested fix: subtract the prune delta (prePruneTokens is already computed at lib/hooks.ts:264) or do a precise count of the current array on the over-budget path (rare, so cost is fine) — plus a regression test.

M2. "respects the recent-message protection zone" test does not verify protectiontests/prune-to-fit.test.ts:443-461 (§5.6 test-fidelity violation)
With preserveRecentMessages: 2 and 7 visible messages, computeProtectedRefs protects only {msg-5, msg-asst}msg-4 is not protected at all. The guard clears msg-0..msg-2 then stops at budget (143277 ≤ 167232), so the assertion that msg-4 survives passes for the wrong reason, and the comment at line 457 is factually wrong. Consequence: the protected-ref skip at lib/messages/prune-to-fit.ts:146 is never the deciding factor in any test — delete that line and the suite still passes. Fix: enlarge the gap (e.g. assistant input ~300k+) so the guard must clear through msg-4 and skip the protected msg-5, asserting msg-5 survives (+ ERROR log), or rename the test and add a dedicated protection-zone case.

🟡 MINOR findings

  • m1. Model identity unknown in a pure [Bug] Silent context-overflow death on resume when model reports limit.context=0 (custom providers): uncalibrated thresholds → 400 → empty exit-0 loop #347 sessionlib/hooks.ts:102-108 writes modelProviderID/modelID only when limit.context is truthy; in a limit=0 session both stay undefined, so (a) the one-time WARN logs provider: undefined, model: undefined (user can't tell which model to configure) and (b) the documented per-model compress.modelMaxLimits is unreachable (lib/messages/prune-to-fit.ts:68 requires both defined) — a user who set modelMaxLimits: {"vllm-local/qwen": 262144} as their fix is silently unprotected. Identity is available from lastUserMessage.info.model (already read at lib/hooks.ts:190-194) or input.model in the system transform.
  • m2. Silent no-op when over budget but nothing is clearablelib/messages/prune-to-fit.ts:181: if estimate > safeBudget but every output is protected / <500 tokens / already cleared, the function returns without logging. That's precisely the "dies on a swallowed 400" scenario this PR targets, with zero diagnostic signal. Add a WARN/ERROR ("over budget, N outputs unclearable").
  • m3. Test name contradicts assertiontests/prune-to-fit.test.ts:552: named "…resets the counter when a window later resolves, then re-warns" but asserts exactly one warn (the warned-flag suppression). Rename to what it actually verifies.
  • m4. Tests use a message shape that can't reach the guard in productionisMessageWithInfo (lib/messages/shape.ts:19) accepts only user/assistant and filterMessagesInPlace (lib/hooks.ts:151) drops everything else; in production tool outputs are parts inside assistant messages. All 25 tests fabricate role: "tool" messages (also missing info.time.created), so the suite never exercises: tool parts in assistant messages, the lastMsgId "never touch the current turn" skip against an assistant carrying the current turn's tool parts, or mixed text+tool messages. The guard logic is role-agnostic for tool parts so the code is likely correct — but the coverage doesn't prove it.
  • m5. Two safety paths untested as deciding factors: (a) the current-turn skip (lib/messages/prune-to-fit.ts:142) — every test ends with the assistant last, so no test has a clearable tool output in the last position; (b) the precise-tokenization fallback (lib/messages/prune-to-fit.ts:94-98, the real first-turn case) is never exercised — all tests supply makeAssistantWithTokens.

⚪ NITs

  • lib/messages/prune-to-fit.ts:89-98: estimate omits the system prompt when getCurrentTokenUsage returns >0 via its internal message-sum fallback (no assistant with tokens yet), while the base === 0 path adds state.systemPromptTokens. Under-count only matters if the system prompt exceeds the reserve — rare, but make the paths consistent.
  • tests/prune-to-fit.test.ts:20: CLEAR_PLACEHOLDER re-declared instead of exported from source. Drift failure mode is safe (positive assertions would fail, not false-pass) — exporting would be cleaner.
  • tests/prune-to-fit.test.ts:50-101: makeConfig omits a few PluginConfig fields (logLevel, top-level allowSubAgents, qualityGate, messageFilters, gc.batchCleanup) — bypassed by the cast and not read by the code under test, but deviates from §5.6 factory conventions.

Verdict

REQUEST-CHANGES. The design is sound and the change set is otherwise clean — persistence, backward compat, type safety, security, performance, and block-state interaction all check out. The fix list is bounded: M1 (post-prune estimate + regression test) is the only source-behavior change; M2 + m3/m4/m5 are test-file fixes; m1/m2 are small source improvements worth doing in the same pass. Happy to implement the fixes on the branch if you'd like — otherwise I'll re-review once updated. (Reminder per AGENTS.md §5.1.1.2: I won't merge the PR — that's yours to click.)

…#347)

- N1: use precise content count after a compress (stale provider base over-cleared)
- N2: count messages after the last assistant (current user turn was uncounted)
- N3: dedup the 'nothing clearable' ERROR via transient overflowGuardStuckLogged
- T1: backward scan instead of array copy; T2: fix misleading docstring
- T3-T6: production-shape clear test, time.created mock shape, non-completed
  trailing-run test, tighten loose assertion
- 33 prune-to-fit tests (5 new), full suite 1062/1062, N1/N2 mutation-verified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant