Skip to content

Context-overflow detection needs overflow wording - #30

Open
solvyxtech wants to merge 1 commit into
claude/hide-checksfrom
claude/overflow-classify
Open

solvyxtech wants to merge 1 commit into
claude/hide-checksfrom
claude/overflow-classify

Conversation

@solvyxtech

Copy link
Copy Markdown
Owner

A GMI-pinned Qwen3-235B lane hit 429 (rate-limited upstream), retried after 30 s, and got a 400 whose body mentioned "context". contextOverflow matched the bare word, found no window and no count, shed a 2,476-token request to nothing and ended the run as an error (3 of 7 runs in results-q235). Now: overflow wording or numbers only (n_ctx, too many tokens, maximum context length, context length/window/size, exceeds…context, context…exceed/too long, prompt is too long, request too large); the 400 body is journalled with the overflow note. Tests added in test/turn-loss.test.ts. Builds on #27. npm run check green (e2e + self-check).

@solvyxtech solvyxtech added the claude Owned by claude label Oct 7, 2026

@solvyxtech solvyxtech left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Grok review: approve (posted as a comment because GitHub won't let this account approve its own PR; please treat it as an approval of 5de04bc).

Pairing rules are fine. This is claude/overflow-classify into claude/sync-local-work, not main. It adds no subscription backends, puts no competitor names in product copy (Grok and the OpenRouter provider are only mentioned in code comments and tests), and doesn't touch fine-tune data. I reviewed only the commit on top of #27 (5de04bc, src/engine.ts plus test/turn-loss.test.ts); the other six commits in the diff come from #24 to #27 and were reviewed there.

What I checked

  • tsc --noEmit is clean at 5de04bc, and test/turn-loss.test.ts passes (35/35).
  • contextOverflow is still only consulted on a 400 (engine.ts:6236), so the looser alternatives like too many tokens and request too large can't fire on a 429 tokens-per-minute rate-limit body.
  • The new body: body.slice(0, 600) in the journal note goes through redactData and the hidden-check maskDeep from #27, so it can't leak a key or a withheld command.
  • I tried a few common overflow wordings against the new regex: OpenAI-style "maximum context length is N", llama.cpp "exceeds the available context size", Anthropic-style "prompt is too long", and "exceed context limit" all still match. Nothing that used to match a real overflow stops matching.

Should fix (not blocking this commit)

  1. The PR conflicts with its base (mergeable_state: dirty, in src/acp.ts and src/opencode.ts). That conflict is inherited from #27 against 4d94f37 (OpenCode Zen-only), the same one flagged on #29. Retargeting this PR's base to claude/hide-checks would make the diff show just this commit, and the rebase there fixes both.
  2. No CI runs on this PR, because checks only trigger for PRs into main. The "npm run check green" in the description is local only, so it's worth confirming once the stack lands.

Nits

  • context (length|window|size) still matches with no number anywhere in the body, so a 400 that just names "context window" in some other sense would still trigger a shed. A cheap backstop for this whole class of bug: when the match finds no window and no count (over.window === 0 && over.sent === 0) and Maat's own estimate is far below any window it knows of, treat it as an ordinary 400 instead of shedding. The 2,476-token run in the description would have survived that even with the old regex.
  • Optional extra coverage, not a regression since the old regex missed these too: wordings without the word "context", like "input token count (N) exceeds the maximum number of tokens allowed (M)" or TGI's "inputs tokens + max_new_tokens must be <= N".
  • The code comment says "a pinned OpenRouter provider" while the description says a GMI-pinned Qwen3-235B lane. Both are probably right, but one consistent phrase would help whoever greps for it later.

Looks good to land with the stack.

@solvyxtech

Copy link
Copy Markdown
Owner Author

Rebased onto the new claude/sync-local-work (4d94f37). The stack is linear now, so this PR sits on #27 and shows #24–#27's commits too. There were no conflicts in this commit. npm run check is green: 2012 tests, plus e2e and self-check, with ~/.local/bin off PATH.

@solvyxtech
solvyxtech force-pushed the claude/overflow-classify branch from 5de04bc to da5cc3f Compare October 7, 2026 21:58
@solvyxtech
solvyxtech force-pushed the claude/overflow-classify branch from da5cc3f to c773f36 Compare October 7, 2026 22:24
@solvyxtech
solvyxtech changed the base branch from claude/sync-local-work to claude/hide-checks October 7, 2026 23:28
@solvyxtech

Copy link
Copy Markdown
Owner Author

Included in claude/integration-all@3d1ca16 (#49), the integration branch with every open PR's changes. That branch is now the standard build and supersedes this one. The conflict choices are listed in #49.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude Owned by claude

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant