Skip to content

fix(lean): lower offloader default limit below client output cap - #184

Merged
MementoRC merged 2 commits into
developmentfrom
fix/offloader-default-limit
Jun 19, 2026
Merged

fix(lean): lower offloader default limit below client output cap#184
MementoRC merged 2 commits into
developmentfrom
fix/offloader-default-limit

Conversation

@MementoRC

Copy link
Copy Markdown
Owner

Summary

Follow-up to the ResponseOffloader wiring fix (#182). Live testing showed the offloader default sat above Claude Code's client-side output cap, defeating its purpose.

Problem

GitLeanInterface builds its default MCPTokenLimiter from settings.unknown_token_limit (was 25000 tokens). At the offloader's 3.5 chars/token estimate that's ~88KB. But Claude Code's client MAX_MCP_OUTPUT_TOKENS truncates around ~60KB for git-hash-heavy text. So outputs in the ~60–88KB band slip past the server offloader (returned inline) and get truncated-to-file by the client instead of cleanly offloaded to /tmp — the opposite of what the offloader is for.

Observed live: a ~60KB git_log was returned inline and hit the client cap, while ~88KB git_diff outputs offloaded correctly.

Change

Lower the default unknown_token_limit 25000 → 12000 tokens (~42KB at 3.5 ch/tok), a safe margin under the client cap, so the offloader spills before the client truncates. The MCP_GIT_UNKNOWN_TOKEN_LIMIT env override still raises it for anyone who wants the old behavior. (BALANCED profile updated to match; explicit high-limit profiles AGGRESSIVE/DEVELOPMENT left as-is.)

  • src/mcp_server_git/config/token_limits.py: default + BALANCED profile 25000 → 12000
  • tests/unit/lean/test_offloader_token_limit.py: +5 regression tests (default is 12000, default < client cap, env override still works, new default truncates a ~50KB payload, old default did not — regression anchor)

Test plan

  • pixi run -e quality test-unit — passes (exit 0)
  • pixi run -e quality lint (ruff F,E9) — clean

⚠️ Requires an MCP server restart to take effect at runtime.

🤖 Generated with Claude Code

The offloader default (unknown_token_limit=25000 tokens, ~88KB) sat ABOVE
Claude Code's client MAX_MCP_OUTPUT_TOKENS (~60KB for git output), so
60-88KB results slipped past the server offloader and were truncated by the
client instead of offloaded to /tmp. Lower the default to 12000 tokens
(~42KB) so the offloader spills before the client truncates. Env override
(MCP_GIT_UNKNOWN_TOKEN_LIMIT) still raises it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude finished @MementoRC's task —— View job


PR Review

  • Read repository guidelines (CLAUDE.md)
  • Read changed files
  • Analyze code quality and best practices
  • Check test coverage
  • Post review feedback

Overall: Clean, well-motivated fix with good regression coverage. A few observations below.


src/mcp_server_git/config/token_limits.py

CONSERVATIVE profile anomaly (token_limits.py:109)

TokenLimitProfile.CONSERVATIVE: cls(
    llm_token_limit=15000,
    unknown_token_limit=18000,   # ← higher than BALANCED (12000) and the default (12000)
    ...
),

The CONSERVATIVE profile has unknown_token_limit=18000, which is above the new default of 12000 and exceeds the PR's stated client cap threshold (~17000 tokens ≈ 60 KB). This means a user selecting "CONSERVATIVE" would still be in the problematic zone the PR aims to fix. The name implies "safe/restrictive", but this limit is looser than both the default and BALANCED. Consider lowering it to ≤12000, or at least below _CLIENT_CAP_APPROX_TOKENS.

Inline comment placement (token_limits.py:36-37)

# kept below client MAX_MCP_OUTPUT_TOKENS so the offloader spills before the client truncates
unknown_token_limit: int = 12000

Good addition — the comment explains the invariant. It could also mention the approximate KB value (42 KB) to make the relationship to the ~60 KB cap immediately legible without mental arithmetic.


tests/unit/lean/test_offloader_token_limit.py

Hardcoded client cap estimate (test_offloader_token_limit.py:98-99)

_CLIENT_CAP_APPROX_TOKENS = 17_000  # ~60 KB / 3.5 ch per token, rounded down
_CHARS_PER_TOKEN_ESTIMATE = 3.5

_CHARS_PER_TOKEN_ESTIMATE is defined but only used in an f-string in a test assertion message (test_offloader_token_limit.py:150), not in the actual calculation of _CLIENT_CAP_APPROX_TOKENS. The cap value is computed manually and written in. If either constant changes independently, the comment and the assertion message can drift from the actual value. Consider deriving it: _CLIENT_CAP_APPROX_TOKENS = int(60 * 1024 / _CHARS_PER_TOKEN_ESTIMATE) (≈17,554) for consistency.

Test class isolation (test_offloader_token_limit.py:102-165)

The TestOffloaderDefaultBelowClientCap tests instantiate TokenLimitSettings() directly (not through config_manager) so they correctly bypass the singleton reset in the _reset_config_manager fixture. This is fine for these tests, but worth noting: the fixture is autouse=True and applies to both test classes even though the second class doesn't use config_manager. No bug, just slightly over-broad fixture scope.

Missing profile coverage

There's no test that asserts the CONSERVATIVE profile's unknown_token_limit is also below the client cap. Given the anomaly noted above, a test like test_balanced_profile_below_client_cap or test_all_safe_profiles_below_client_cap would catch future regressions across profiles.

Test for BALANCED profile

The PR mentions "BALANCED profile updated to match", but there's no regression test asserting TokenLimitSettings.from_profile(TokenLimitProfile.BALANCED).unknown_token_limit == 12000. Minor gap given the existing default test, but worth adding for completeness.


Summary

Area Status
Core fix (25000 → 12000) ✅ Correct and well-explained
Env override still works ✅ Tested
New default regression tests ✅ Solid
CONSERVATIVE profile consistency ⚠️ Anomaly — 18000 > client cap threshold
_CHARS_PER_TOKEN_ESTIMATE usage ⚠️ Minor — unused in derivation
Profile-level test coverage ℹ️ Optional improvement

The CONSERVATIVE profile issue is the most substantive one since it partially undermines the goal of the PR for users who explicitly choose "conservative" behavior.

…ive test cap

- CONSERVATIVE profile unknown_token_limit 18000 -> 8000 (was above the new
  default and the client cap, undermining the 'conservative' intent).
- Derive _CLIENT_CAP_APPROX_TOKENS from _CHARS_PER_TOKEN_ESTIMATE instead of
  hardcoding, so they can't drift.
- Add profile-coverage tests asserting CONSERVATIVE/BALANCED stay below the
  client cap and BALANCED==12000.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@MementoRC

Copy link
Copy Markdown
Owner Author

Thanks for the review — addressed in faf40b9:

  • CONSERVATIVE profile anomaly (substantive): lowered unknown_token_limit 18000 → 8000, so it's now the most restrictive profile (below the 12000 default and well under the client cap), matching the 'conservative' intent.
  • Hardcoded cap: _CLIENT_CAP_APPROX_TOKENS is now derived — int(60 * 1024 / _CHARS_PER_TOKEN_ESTIMATE) (≈17476) — so the two constants can't drift.
  • Profile coverage: added TestProfilesBelowClientCap asserting CONSERVATIVE and BALANCED stay < _CLIENT_CAP_APPROX_TOKENS, and BALANCED == 12000.
  • Added the ~42 KB note to the default's inline comment.

The over-broad autouse fixture is a no-op for the direct-instantiation tests, so I left it as-is. test-unit green, lint clean.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude encountered an error —— View job


I'll analyze this and get back to you.

@MementoRC
MementoRC merged commit f44a84e into development Jun 19, 2026
15 of 16 checks passed
@MementoRC
MementoRC deleted the fix/offloader-default-limit branch June 19, 2026 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant