Skip to content

Cap persisted tool results before rehydration#1089

Open
s-salamatov wants to merge 1 commit into
moltis-org:mainfrom
s-salamatov:codex/cap-persisted-tool-results
Open

Cap persisted tool results before rehydration#1089
s-salamatov wants to merge 1 commit into
moltis-org:mainfrom
s-salamatov:codex/cap-persisted-tool-results

Conversation

@s-salamatov

Copy link
Copy Markdown
Contributor

Summary

  • cap persisted tool and tool_result content when session history is rehydrated into provider-bound ChatMessages
  • apply the capped conversion to normal chat, streaming chat, retry-after-compaction, prompt inspection, silent memory turns, and LLM-backed compaction prompts
  • keep provider-format conversion unbounded so hook-modified provider payloads are preserved exactly

Testing

  • RED: cargo test -p moltis-agents model::tests::convert_caps_persisted_string_tool_result_when_limit_is_set -- --nocapture failed before implementation with missing values_to_chat_messages_with_tool_result_limit
  • cargo test -p moltis-agents model::tests
  • cargo test -p moltis-agents
  • cargo test -p moltis-chat
  • cargo fmt --check
  • git diff --check

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 318e64e828

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/chat/src/compaction_run/llm_replace.rs Outdated
Comment thread crates/chat/src/compaction_run/structured.rs Outdated
@greptile-apps

greptile-apps Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a values_to_chat_messages_with_tool_result_limit conversion function that caps persisted tool/tool_result content at config.tools.max_tool_result_bytes (default 50 KB) before feeding session history to an LLM provider. All relevant production rehydration callsites — normal chat, streaming, retry-after-compaction, silent memory turns, prompt inspection, LLM-backed compaction, session summary, title generation, and quick actions — are updated. The provider_values_to_chat_messages path is intentionally left unbounded to preserve hook-modified provider payloads.

  • Core truncation is delegated to the existing sanitize_tool_result helper (strips base64/hex blobs, then byte-caps at a char boundary), keeping logic DRY.
  • Seven new unit tests cover string results, structured JSON results, errors, multi-byte char boundary safety, orphan-filtering preservation, and the provider conversion path.
  • Two new integration tests (llm_replace, structured) verify that the provider-level byte cap — not tool_prune_char_threshold — is applied when building LLM summary prompts.

Confidence Score: 5/5

Safe to merge — the change is additive and all production rehydration paths now apply a consistent byte cap with no missed call sites.

Every callsite that converts persisted session JSON to ChatMessages for provider consumption is updated. The core truncation is delegated to the already-tested sanitize_tool_result helper. The intentional exception (provider_values_to_chat_messages) is clearly documented and left unbounded by design. Unit and integration tests cover the new code path end-to-end, including char-boundary safety and orphan-filter preservation.

No files require special attention.

Important Files Changed

Filename Overview
crates/agents/src/model/convert.rs Adds values_to_chat_messages_with_tool_result_limit and maybe_limit_tool_result_content; applies capping to both tool and tool_result roles, preserves orphan filtering and char-boundary safety.
crates/agents/src/model/mod.rs Exports new public API and adds seven well-structured unit tests covering string, JSON, error, char-boundary, orphan-filter, and provider-path scenarios.
crates/chat/src/compaction_run/llm_replace.rs Threads max_tool_result_bytes through to the summary-input conversion; new integration test correctly distinguishes the provider byte cap from tool_prune_char_threshold.
crates/chat/src/compaction_run/structured.rs Threads max_tool_result_bytes into the structured summary-prompt build and adds a parallel integration test; recency-preserving path stays JSON-native and is correctly left unchanged.
crates/chat/src/run_with_tools.rs Applies the capped conversion at both initial chat-history load and retry-after-compaction reload, sourcing the limit from persona config each time.
crates/chat/src/service/chat_impl.rs Updates silent memory turn and prompt-inspection paths; minor refactor avoids calling discover_and_load() twice by storing the config in a local binding.
crates/chat/src/streaming.rs Switches the streaming history-to-messages conversion to the capped variant; single-line, low-risk change.
crates/gateway/src/session/summary.rs Session summary memory write now uses the bounded conversion; straightforward call-site update.
crates/gateway/src/session/title.rs Title-generation history conversion switched to the capped variant; no logic change, just call-site update.
crates/chat/src/compaction_run/runner.rs Threads max_tool_result_bytes into structured::run and llm_replace::run; recency_preserving::run correctly omits it since it operates directly on raw JSON values.

Reviews (2): Last reviewed commit: "Cap persisted tool results before rehydr..." | Re-trigger Greptile

@s-salamatov s-salamatov force-pushed the codex/cap-persisted-tool-results branch from 318e64e to 869fe56 Compare June 3, 2026 02:52
@s-salamatov

s-salamatov commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback in 869fe56:

  • llm_replace and structured compaction now use the configured provider-bound tools.max_tool_result_bytes cap for summary input history instead of chat.compaction.tool_prune_char_threshold, so the summarizer no longer sees only the first ~200 bytes by default.
  • Threaded max_tool_result_bytes through run_compaction / compact_session so LLM-backed compaction has access to the same operator-configured tool-result cap as normal chat paths.
  • Updated the missed LLM-bound history paths to use capped rehydration: periodic memory extraction, session-end summary, /btw quick action context, and auto-title generation.
  • Added regression coverage for both LLM-backed compaction modes to verify summary prompts preserve tool-result content beyond the stored-history prune threshold when the provider cap allows it.

Verification:

  • cargo test -p moltis-chat -> 118/118
  • cargo test -p moltis-gateway -> 445/445 plus 14/14 integration tests
  • cargo test -p moltis-agents model::tests -> 58/58
  • cargo fmt --check
  • git diff --check

@greptile

@s-salamatov

Copy link
Copy Markdown
Contributor Author

@greptile Pls review

@s-salamatov

Copy link
Copy Markdown
Contributor Author

@codex advise tests set, docs

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@s-salamatov

Copy link
Copy Markdown
Contributor Author

@codex advise tests set, docs

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

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