fix(agents): oldest-first tool result compaction with configurable ratios#726
fix(agents): oldest-first tool result compaction with configurable ratios#726Cstewart-HC wants to merge 5 commits intomoltis-org:mainfrom
Conversation
Greptile SummaryThis PR resolves the oldest-first compaction direction bug in long agent loops, makes the compaction and overflow thresholds configurable via Confidence Score: 5/5Safe to merge — all previously flagged blockers are resolved; only P2 polish items remain. All five P0/P1 findings from prior review rounds have been correctly addressed: ratio=0 disable path, off-by-one in min-iterations semantics, ordering validation in check_semantic_warnings, oldest-first test coverage, and overflow still firing during the early-iteration window. Remaining findings are three P2 issues (diagnostic format string, misleading error message text, missing >100 validation) that do not affect correctness or safety. crates/config/src/validate/semantic.rs (format string), crates/config/src/schema/tools.rs (upper-bound validation)
|
| Filename | Overview |
|---|---|
| crates/agents/src/runner/helpers.rs | Reverses compaction direction to oldest-first, adds configurable ratio/overflow params, handles ratio=0 disable path, and retains post-compaction overflow guard — all correct. |
| crates/agents/src/runner/non_streaming.rs | Wires in new config fields; effective_ratio=0 during early iterations correctly delegates overflow checking to enforce_tool_result_context_budget without skipping it. |
| crates/agents/src/runner/streaming.rs | Mirrors non_streaming.rs changes exactly; same effective_ratio guard pattern, no issues. |
| crates/config/src/schema/tools.rs | Adds three config fields with correct defaults; missing upper-bound (>100) validation for the two percentage fields. |
| crates/config/src/validate/semantic.rs | Adds ordering constraint (overflow_ratio > compaction_ratio); diagnostic message has a literal newline/indentation inside the format string. |
| crates/agents/src/runner/tests/basic.rs | Adds 12 new unit tests covering oldest-first direction, edge cases (already-compacted, too-small, zero tokens, zero context window), and enforce_budget path (ratio=0, overflow, compaction fire, no tool results). |
| crates/config/src/validate/schema_map.rs | Registers the three new config keys; straightforward and correct. |
| crates/config/src/template.rs | Template comments added for all three new fields with correct defaults; no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["enforce_tool_result_context_budget\n(messages, tool_schemas, context_window,\ncompaction_ratio, overflow_ratio)"] --> B{context_window == 0\nor no tool results?}
B -- Yes --> C[return Ok]
B -- No --> D{compaction_ratio == 0?}
D -- Yes --> E[compute overflow_budget\ncheck current_tokens]
E --> F{current_tokens >\noverflow_budget?}
F -- Yes --> G[return Err\nContextWindowExceeded\n'compaction disabled']
F -- No --> C
D -- No --> H[compute compaction_budget\nand overflow_budget]
H --> I{current_tokens >\ncompaction_budget?}
I -- Yes --> J["compact_tool_results_oldest_first_in_place\n(oldest → newest order)"]
J --> K[log debug]
I -- No --> L[re-estimate post_compaction_tokens]
K --> L
L --> M{post_compaction_tokens >\noverflow_budget?}
M -- Yes --> N[return Err\nContextWindowExceeded\n'after compaction']
M -- No --> C
subgraph Callers["Callers (non_streaming / streaming)"]
R["iterations > compaction_min_iterations?"]
R -- Yes --> S["effective_ratio = compaction_ratio"]
R -- No --> T["effective_ratio = 0\n(overflow still checked)"]
end
S --> A
T --> A
Reviews (5): Last reviewed commit: "fix(agents): don't skip overflow check d..." | Re-trigger Greptile
|
@greptileai review |
P1: compaction_ratio=0 now truly disables compaction instead of clamping to 1%. Early-return with overflow-only guard when ratio is 0. P2: streaming.rs min-iterations guard changed from >= to > to match non_streaming.rs - consistent skip first N iterations semantics. P2: added config validation warning when preemptive_overflow_ratio <= tool_result_compaction_ratio (prevents infinite error loops). Refs: moltis-org#726
48b7325 to
3dd6b92
Compare
…et enforcement Addresses PR moltis-org#726 review feedback: core behavioral change had no test coverage. 11 new tests covering: - Oldest-first compaction direction (not newest) - Already-compacted results are skipped - Results below TOOL_RESULT_COMPACTION_MIN_BYTES are skipped - Zero tokens_needed early return - Compaction stops once budget freed - compaction_ratio=0 disables compaction (overflow-only guard) - compaction_ratio=0 still errors on overflow - Compaction fires when over compaction threshold - ContextWindowExceeded when overflow unreachable after compaction - Noop when no tool results present - Noop when context_window is zero Also fixes function rename: compact_tool_results_newest_first_in_place → compact_tool_results_oldest_first_in_place (name was stale after rebase conflict resolution changed iteration direction to forward).
|
@greptileai review |
…tios Issue moltis-org#1 from moltis-stream-issues investigation. Three changes to per-iteration tool-result compaction: A) Reverse compaction direction: compact oldest results first instead of newest. The model needs its most recent tool outputs to maintain coherent operation in long agent loops. C) Make compaction/overflow ratios configurable via ToolsConfig: - tool_result_compaction_ratio (default 75, set to 0 to disable) - preemptive_overflow_ratio (default 90) D) Add compaction_min_iterations (default 3): skip compaction for the first N iterations to prevent premature context destruction in short loops. All defaults match the previous hardcoded values for backward compatibility. No moltis.toml changes required for existing deploys.
yes Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Christopher L Stewart <100775598+Cstewart-HC@users.noreply.github.com>
P1: compaction_ratio=0 now truly disables compaction instead of clamping to 1%. Early-return with overflow-only guard when ratio is 0. P2: streaming.rs min-iterations guard changed from >= to > to match non_streaming.rs - consistent skip first N iterations semantics. P2: added config validation warning when preemptive_overflow_ratio <= tool_result_compaction_ratio (prevents infinite error loops). Refs: moltis-org#726
…et enforcement Addresses PR moltis-org#726 review feedback: core behavioral change had no test coverage. 11 new tests covering: - Oldest-first compaction direction (not newest) - Already-compacted results are skipped - Results below TOOL_RESULT_COMPACTION_MIN_BYTES are skipped - Zero tokens_needed early return - Compaction stops once budget freed - compaction_ratio=0 disables compaction (overflow-only guard) - compaction_ratio=0 still errors on overflow - Compaction fires when over compaction threshold - ContextWindowExceeded when overflow unreachable after compaction - Noop when no tool results present - Noop when context_window is zero Also fixes function rename: compact_tool_results_newest_first_in_place → compact_tool_results_oldest_first_in_place (name was stale after rebase conflict resolution changed iteration direction to forward).
dc1a278 to
bbf19d3
Compare
…et enforcement Addresses PR moltis-org#726 review feedback: core behavioral change had no test coverage. 11 new tests covering: - Oldest-first compaction direction (not newest) - Already-compacted results are skipped - Results below TOOL_RESULT_COMPACTION_MIN_BYTES are skipped - Zero tokens_needed early return - Compaction stops once budget freed - compaction_ratio=0 disables compaction (overflow-only guard) - compaction_ratio=0 still errors on overflow - Compaction fires when over compaction threshold - ContextWindowExceeded when overflow unreachable after compaction - Noop when no tool results present - Noop when context_window is zero Also fixes function rename: compact_tool_results_newest_first_in_place → compact_tool_results_oldest_first_in_place (name was stale after rebase conflict resolution changed iteration direction to forward).
|
@greptileai review |
…ns guard Pass ratio=0 (overflow-only path) instead of skipping the call entirely during the first N iterations. This ensures ContextWindowExceeded is raised even when compaction is deferred. Fixes greptile P1 review feedback on moltis-org#726.
|
@greptileai review |
…tios Three changes to per-iteration tool-result compaction: A) Reverse compaction direction: compact oldest results first instead of newest. The model needs its most recent tool outputs to maintain coherent operation in long agent loops. B) Make compaction/overflow ratios configurable via ToolsConfig: - tool_result_compaction_ratio (default 75, set to 0 to disable) - preemptive_overflow_ratio (default 90) C) Add compaction_min_iterations (default 3): skip compaction for the first N iterations to prevent premature context destruction in short loops. All defaults match the previous hardcoded values for backward compatibility. No moltis.toml changes required for existing deploys. Refs: moltis-org#726
Summary
Fix for Issue #1 from the stream investigation report — per-iteration tool-result compaction was destroying agent context in long loops.
Changes
A) Reverse compaction direction: Compact oldest results first instead of newest. The model needs its most recent tool outputs to maintain coherent operation in long agent loops.
B) Configurable ratios via
ToolsConfig:tool_result_compaction_ratio(default 75, set to 0 to disable)preemptive_overflow_ratio(default 90)C) Minimum iteration guard:
compaction_min_iterations(default 3) — skip compaction for the first N iterations to prevent premature context destruction in short loops.Behavior
All defaults match the previous hardcoded values for backward compatibility. No
moltis.tomlchanges required for existing deploys.Files changed (7 files, +71 −20)
crates/agents/src/runner/mod.rs— reverse iteration order, read config valuescrates/agents/src/runner/streaming.rs— pass config throughcrates/agents/src/runner/non_streaming.rs— pass config throughcrates/agents/src/runner/tests/helpers.rs— test helper updatecrates/config/src/schema/tools.rs— 3 new config fields with defaultscrates/config/src/template.rs— template commentscrates/config/src/validate/schema_map.rs— schema key registrationValidation
cargo fmt --check✅cargo clippy --workspace --all-targets -- -D warnings✅cargo test --workspace✅ (344 passed, 1 timing-flaky unrelated failure)Session context
Built during an extended investigation session into stream compaction issues. Redacted — no secrets or tokens in session.