[electrolobzik]

Summary

Fixes two related bugs in the truncate-and-cache pipeline that together broke read_cache pagination for any response large enough to need it. Two commits, each independently testable.

1. forwardContentResult never stored the truncated payload

When an upstream tool call exceeded tool_response_limit (default 20 KB), the truncator generated a CacheKey and embedded it in a "use read_cache key=..." banner. But the full pre-truncation payload was never written to the BBolt cache backend — only the banner was emitted. Every subsequent read_cache call returned "cache key not found". The cache + pagination plumbing (cache.Manager.Store and Manager.GetRecords) was already there and correct — the truncation path simply skipped the Store step.

Real-world impact: github:pull_request_read get_reviews on a PR with a long CodeRabbit review (~70 KB body) gives a truncated banner pointing at a cache key that resolves to nothing.

forwardContentResult now takes an optional CacheStore (a small local interface mirroring cache.Manager.Store) and persists the original text under the truncator-generated key whenever TruncationResult.CacheAvailable is true. Both call sites in internal/server/mcp.go now thread p.cacheManager through.

The interface is defined locally to keep internal/server/content_forward.go decoupled from internal/cache. Callers that don't have a cache plumbed through (e.g. existing tests) can pass nil — that path is exercised by TestForwardContentResult_NoCacheStoreNoPanic.

2. read_cache's own output bypassed the truncator

Once commit 1 made read_cache actually work, the next-level bug surfaced: read_cache itself returns mcp.NewToolResultText(jsonResult) directly, bypassing forwardContentResult entirely. So an oversized page (50 records totalling 200 KB) was emitted unbounded — the very thing the truncator is supposed to prevent. The fix on naturally-large responses (CodeRabbit reviews, log queries, big diffs) was incomplete.

This is the recursive truncate-and-cache contract: when handleReadCache produces an oversized response, it now gets truncated, the full body stored under a fresh cache key, and the agent sees the standard banner pointing at the new key. Subsequent reads against the new key paginate normally — recursion is bounded (each level emits a distinct key) and idempotent.

A new helper maybeTruncateAndCacheText lives next to forwardContentResult so other built-in tools that bypass the upstream pipeline can adopt the same contract in a one-liner.

Single-record infinite-recursion guard. If read_cache(limit=1) returns one record that itself exceeds the limit, recursively caching it would produce a new key resolving to the exact same oversized payload — an inescapable loop. The helper short-circuits when paginableUnits <= 1: the oversize text passes through unchanged. The agent already has the data they explicitly asked for; nothing more can be done at this layer.

Tests

All in internal/server/content_forward_test.go:

Commit 1 (forwardContentResult caches):

• TestForwardContentResult_StoresFullContentInCache — single TextContent block triggers exactly one Store call with the full payload, correct key, tool name, args, and totalRecords
• TestForwardContentResult_NoCacheStoreNoPanic — nil cache store stays safe for legacy callers
• TestForwardContentResult_FallbackPathStoresCache — the non-*CallToolResult JSON-wrap fallback caches too
• TestForwardContentResult_RoundTripViaRealCacheManager — real BBolt-backed cache.Manager end-to-end; GetRecords(0,10) and GetRecords(10,10) return distinct, correctly-ordered pages by id

Commit 2 (read_cache recursive):

• TestMaybeTruncateAndCacheText_HappyPathStores — 30 records over the limit triggers exactly one Store carrying the full body
• TestMaybeTruncateAndCacheText_NoRecurseOnSingleRecord — single oversize record short-circuits; no Store call, body returned as-is
• TestMaybeTruncateAndCacheText_NoOpUnderLimit — under the limit, helper is a no-op (no banner, no cache churn)
• TestMaybeTruncateAndCacheText_RecursiveRoundTripViaRealCacheManager — wires the real BBolt cache.Manager: a 40-record response gets truncated, the new key is parsed out of the banner, GetRecords(newKey, 0, 5) returns the same records 0..4 in order. Proves recursion is stable.

All 4 pre-existing TestForwardContentResult_* tests still pass (the new CacheStore param is nil for them).

Test plan

• go build ./...
• go test ./internal/server/ -run 'TestForwardContentResult|TestMaybeTruncateAndCacheText' -v — 12 tests, all green
• go test ./internal/cache/... ./internal/truncate/... — unchanged, still pass
• Manual live verification (already done against PR Enhance macOS support in GitHub Actions workflow and add autostart functionality #5 of HaloCollar/halo-tools): build, restart proxy, call github:pull_request_read get_reviews on a PR with a >20 KB review body, then call read_cache with the embedded key — returns the full record body. Call read_cache again with a high limit on a many-record payload — gets re-truncated with a fresh key, follow-up walks paginate correctly.

Notes

No MCP API surface change. No config flag. No backwards-compat shim — the previous behavior was straight-up broken in both layers, and the fix only affects the truncation/pagination path.

The local CacheStore interface and maybeTruncateAndCacheText helper give other built-in tools (tail_log, retrieve_tools, etc.) a one-line adoption path if they want the same guarantee. Not done here to keep scope tight.

🤖 Generated with Claude Code

[electrolobzik]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[Dumbris]

Thank you @electrolobzik for the thorough fix and excellent test coverage! 🙏

Verified locally: go build ./... clean, targeted tests green. The bug analysis is spot-on — the cache plumbing existed and only the Store call was missing on the truncation path. The local CacheStore interface keeps package coupling clean, graceful degradation on Store failure is the right tradeoff, and the two-level recursion round-trip test is great.

A few minor, non-blocking follow-ups I'll open a separate PR for:

• Multi-TextContent-block results can collide on a single cache key (GenerateKey is second-granularity on toolName:args), so later blocks overwrite earlier ones.
• "Idempotent" / "bounded recursion" wording is slightly stronger than the timestamped-key behavior guarantees.
• read_cache recursive truncation isn't fed into the truncation token metrics.

None of these block the fix. Merging.