Skip to content

feat(tools): make tool-result persistence threshold configurable - #86171

Open
x1051445024 wants to merge 2 commits into
NousResearch:mainfrom
x1051445024:pr/tool-result-persist-threshold-v2
Open

feat(tools): make tool-result persistence threshold configurable#86171
x1051445024 wants to merge 2 commits into
NousResearch:mainfrom
x1051445024:pr/tool-result-persist-threshold-v2

Conversation

@x1051445024

Copy link
Copy Markdown
Contributor

Problem

tools/tool_result_storage.py persists oversized tool results (file reads,
searches, script output) to the sandbox and replaces them in-context with a
preview + path. The per-result persistence threshold is hardcoded at
100K chars
for large-context models (tools/budget_config.py), scaled down
only for small models.

In long tool loops (agentic group chats, deep investigation turns) this is
wasteful: medium-sized results (30–50K chars of search/script output) stay
fully in history and are re-sent on every subsequent API call. Measured on
real logs (one room, one day, see A/B below): per-call input grew monotonically
from ~31K to ~193K as results accumulated — with no compression firing because
usage never approached the 50% threshold of a 1M-token model.

Proposed change (minimal, default-preserving)

New config key tools.tool_result_persist_threshold_chars:

  • None (default) — today's behavior, byte-identical (context-scaled budget).
  • int > 0 — explicit per-result cap in chars. Smaller values (e.g. 20000)
    persist medium results early: preview + path in context, full original on
    disk for audit/replay. Values are still bounded by each tool's registry cap
    and read_file stays pinned (no persist→read→persist loops).
  • The explicit cap overrides only the per-result size. The per-turn
    aggregate budget and the preview size still come from the model's context
    window scaling, so a small-window model keeps its small turn budget (it must
    not silently reset back to the 200K default — regression [Bug]: Hermes sends oversized prompts after switching to lower-context local model; token estimation undercounts and compression can increase prompt size #23767).
  • Validation funnels through one function,
    tools/budget_config.normalize_persist_threshold, used by all three layers
    (config parsing, factory, executor) so accept/reject rules cannot drift.
    Strict type whitelist — only non-bool int and whole-number strings are
    accepted:
    • None = unset; booleans rejected (bool is an int subclass —
      int(True) == 1 would persist almost every result, even when set
      programmatically by plugins/tests, not just via YAML);
    • floats and other non-integer numerics rejected (int(1.5) == 1,
      int(Decimal("1.5")) == 1, int(Fraction(3, 2)) == 1 truncation traps);
    • bytes and arbitrary objects rejected — no coercion;
    • non-whole-number strings ("20.5", "1e4") rejected up front;
    • non-positive ints = unset with a warning — never clamped to 1.

Files (8 files total; diff vs baseline fe1b5d8: +477/-5)

File Change
hermes_cli/config_defaults.py default (None) + docs for the new key under tools:
tools/budget_config.py normalize_persist_threshold() (single source of truth, strict whitelist) + budget_with_persist_threshold(threshold, context_length=None) — explicit value overrides only default_result_size; turn budget/preview keep context-scaled values
agent/tool_executor.py _budget_for_agent() normalizes the explicit value through the same function; booleans/non-integer numerics/garbage set programmatically fall back to context scaling instead of crashing or persisting everything
agent/agent_init.py parse/validate the key onto agent._tool_result_persist_threshold_chars via the shared normalizer; warning + fallback on rejection
tests/tools/test_budget_config.py factory: small-model turn-budget protection, context caps, registry cap, pinned exemption, non-positive = unset, normalize whitelist incl. bool/float/Decimal/Fraction/bytes (33 cases)
tests/tools/test_tool_result_storage.py config-driven persist triggers / stays inline (28 cases)
tests/agent/test_tool_budget_explicit_threshold.py executor precedence incl. small-window turn-budget regression + programmatic-fallback regression (11 cases)
website/docs/user-guide/configuration.md "Tool Result Persistence Threshold" section (updated semantics)

Relationship to #85479 (draft, tool-result pruner in compression)

Orthogonal by design:

  • feat(compression): prune oversized tool results in place before summarization #85479 prunes oversized tool results during compression (head 4096 +
    marker + tail 1024, 8192 threshold) — rewrites already-sent history, prompt
    cache prefix broken on each commit.
  • This PR only makes the first-touch persistence threshold configurable —
    append-only, never rewrites history, cache-prefix safe. A tool result is
    persisted once at production time; nothing later modifies it.
  • File-level overlap: both touch agent/agent_init.py +
    hermes_cli/config_defaults.py in different regions (compression vs
    tools sections); no merge conflict. If both land, users can combine them
    (early persist + late prune).

Expected benefit (offline A/B, real room logs)

This is a logical-input upper-bound estimate, NOT a billing-saving promise.
Actual billed savings depend on the provider's prompt-cache pricing; with a
94% cached-input ratio the billed saving is far smaller than the input
reduction.

  • Window: 2026-08-14 00:00:00 .. 2026-08-14 18:03:33 (local, frozen,
    inclusive)
    ; both bounds are fixed: frozen_since_ms=1786636800000,
    frozen_until_ms=1786701813000. Log calls and DB tool results are filtered
    by the same since_ms <= ts <= until_ms bounds — no hardcoded date strings.
    Re-runs produce identical numbers even though the data sources keep growing
    (verified: two consecutive runs diff only in the informational
    extracted_at line). The lower-bound fix changed nothing for this dataset
    (the room was created on 2026-08-14; 0 log rows and 0 DB rows exist before
    the start of window — verified separately), so all numbers below are
    unchanged by the round-5 fix.
  • Room: single agentic group-chat room (all agents, 25 turn-segments).
  • Inputs: 376 calls / 36,986,120 input tokens; 415 tool results
    (4,378,343 chars)
    ; cached input 34,820,480 = 94.1% of total input
    (token ratio, not a per-call ratio).
  • Attribution (heuristic — there is no direct run_id → thread mapping):
    • 351/415 (84.6%) via run_id-grouped unique-window heuristic: results
      grouped by gc_messages.run_id; a run whose tool-message timestamp median
      falls inside exactly one turn-segment's call interval is assigned to that
      segment.
    • 64/415 (15.4%) via nearest-next-call fallback heuristic (ambiguous runs,
      run_id NULL).
    • Both scenarios are reported below as a sensitivity range.
  • Reproduce (run from the repo root; read-only, deterministic across runs):
    .venv/Scripts/python.exe ../hermes-agent-evidence/ab_simulate.py \
        --since "2026-08-14 00:00:00" --until "2026-08-14 18:03:33"
    The script lives in the local evidence directory
    (hermes-agent-evidence/ab_simulate.py), not inside the repo; it
    imports tools.budget_config from the sibling checkout (default
    ../hermes-agent-main, overridable with --repo).

Per-tool thresholds are resolved from the production budget objects
(DEFAULT_BUDGET.resolve_threshold(tool) / budget_with_persist_threshold(T).resolve_threshold(tool)),
so PINNED_THRESHOLDS and registry per-tool caps are applied exactly as at
runtime. In the frozen data, all 15 tool names resolve to base=100,000
(no tool registers a smaller max_result_size_chars; the registry cap branch
is therefore a no-op for this dataset), and read_file resolves to
(pinned — never counted as newly persisted).

threshold scenario input saved (tok) % uncached portion cached portion
8_000 main heuristic only 2,809,030 7.59% 208,915 2,600,115
8_000 with fallback 3,050,352 8.25% 253,435 2,796,917
12_000 main heuristic only 2,068,925 5.59% 173,042 1,895,883
12_000 with fallback 2,220,232 6.00% 207,644 2,012,588
20_000 main heuristic only 1,596,658 4.32% 120,979 1,475,679
20_000 with fallback 1,658,273 4.48% 141,517 1,516,756
30_000 main heuristic only 1,071,906 2.90% 79,626 992,280
30_000 with fallback 1,133,521 3.06% 100,164 1,033,356
50_000 main heuristic only 266,598 0.72% 44,041 222,558
50_000 with fallback 328,213 0.89% 64,579 263,634
100_000 (default) both 0 0% 0 0

Method (incremental-cut simulation, conservative):

  • Each thread's calls are split into turn segments at every per-thread call
    counter reset to Terminal tool #1; each segment's carried set starts empty (no cross-turn
    leakage). Per call, saving = Σ (len − 1500 preview) chars over results newly
    persisted at the candidate threshold but not at baseline
    (candidate_threshold < len ≤ baseline_threshold), chars→tokens at chars/4.
    First carry would be uncached (full price); re-carry on later calls of the
    same turn would be cache-read (discounted price).
  • Pinned tools and registry caps handled via real resolve_threshold
    (see above) — not hardcoded; the script prints the resolved per-tool
    thresholds as evidence.
  • Savings are an upper bound: assumes model behavior unchanged; real savings
    depend on the model re-reading persisted files.

Tests (revision 8 — accurate scope; NOT a full-suite run)

Three runs on the feature branch 881c79a (Python 3.12 venv, pytest 9.1.1,
ruff 0.16.3; workdir = repo root, all commands with -p no:cacheprovider):

# 1) PR feature files (3 new/updated test files)
pytest tests/agent/test_tool_budget_explicit_threshold.py \
      tests/tools/test_budget_config.py tests/tools/test_tool_result_storage.py \
      -v            →  72 passed in 5.68s

# 2) Adjacent core (PR files + accretion_caps, ansi_strip, approval)
pytest tests/tools/test_budget_config.py tests/tools/test_tool_result_storage.py \
      tests/agent/test_tool_budget_explicit_threshold.py \
      tests/tools/test_accretion_caps.py tests/tools/test_ansi_strip.py \
      tests/tools/test_approval.py -q
                    →  195 passed, 2 failed in 35.93s
      (2 failures are baseline platform issues in tests/tools/test_approval.py:
       TestDetectDangerousRm /tmp canonical-path semantics under git-bash on
       Windows — unrelated to this change, pre-existing on the baseline)

# 3) Isolated HERMES_HOME rerun (7 approval/config/allowlist test files,
#    fresh tempdir seeded with the pristine backup config)
pytest tests/tools/test_approval.py tests/tools/test_approval_config_readonly.py \
      tests/tools/test_command_guards.py tests/tools/test_approval_deny_rules.py \
      tests/tools/test_approval_plugin_hooks.py \
      tests/gateway/test_approvals_command.py \
      tests/gateway/test_allowlist_startup_check.py -q
                    →  162 passed, 2 failed in 27.87s (same 2 baseline failures)
  • Isolation proof (run 3): the seeded config (17455 B, LF, sha
    9cc77f4c) was byte-identical after the run (ISO == BACKUP: True) — the
    suite cannot touch a production config; conftest also rewires any
    production-pointing HERMES_HOME to a per-session tempdir.
  • ruff check clean (All checks passed!) on all 7 touched Python files;
    the eighth touched file is Markdown documentation.
  • Scope honesty: this is NOT a full-suite pass. The full collection is
    ~32,000 tests (incl. web/browser/gateway suites that hang or are
    platform-specific on Windows; tests/hermes_cli/test_doctor_journal_modes.py
    fails collection on Windows via os.geteuid). Full-suite CI is only
    meaningful on the project's Linux runners. Precise terminal commands for
    runs 1 and 2, the session-log-recovered core script for run 3, and the
    result lines from the session log are archived in the local evidence
    directory
    (token-save-audit/test-commands-round8.md, not included in the
    PR; note these are session-log records, not raw stdout files);
    programmatic-coercion verification archived as
    hermes-agent-evidence/bool_verification.txt.

Compatibility & rollback

Open questions / follow-ups

  1. Should the explicit threshold also feed the per-turn aggregate budget at a
    fixed ratio (e.g. ×10)? Currently the turn budget stays context-scaled,
    independent of the explicit value.
  2. Live A/B plan (needs user approval + 1-2 real task runs):
    • run one representative long task with threshold=20000 vs baseline;
    • compare: total input tokens, cache-read vs uncached split, call count,
      completion quality (evidence preserved on disk).
  3. If feat(compression): prune oversized tool results in place before summarization #85479 lands first, this draft's wording should reference it as landed
    (same regions, no conflict).

Add tools.tool_result_persist_threshold_chars (default None = current
behavior) so operators can persist oversized tool results earlier than the
100K-char default, reclaiming medium-sized results from re-sent history on
long tool loops.

The explicit value is normalized through a single source of truth
(tools/budget_config.normalize_persist_threshold, strict whitelist: non-bool
int and whole-number strings only) used by config parsing, the budget factory
and the tool executor, and overrides only the per-result size: the per-turn
budget and preview keep their context-window scaling, so small models keep
their small-window turn-budget protection (NousResearch#23767). Booleans, floats,
Decimal/Fraction, bytes and non-whole strings are rejected at every layer
instead of silently clamping to 1. Per-tool registry caps and the read_file
pin still apply through resolve_threshold.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets area/config Config system, migrations, profiles labels Aug 14, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

feat(tools): make tool-result persistence threshold configurable

  1. agent/agent_init.py normalizes the config once at startup and stores it on the agent; agent/tool_executor.py::_budget_for_agent only reads that stored attribute. A value changed in config.yaml (or set by a plugin) after startup is never re-read, so the knob is effectively read-once per session. Consider documenting that, or re-resolving when the agent config reloads, so the executor and the config never disagree mid-conversation.
  2. Diagnostics are asymmetric: an invalid value from the config path logs a warning in agent_init.py, but an invalid value set programmatically on agent._tool_result_persist_threshold_chars is silently ignored in _budget_for_agent (it just falls back to context scaling). Logging the same warning in the executor path would make the two layers' behavior consistent and debuggable.
  3. tools/budget_config.py::budget_with_persist_threshold is annotated threshold_chars: int, yet normalize_persist_threshold accepts whole-number strings and the factory is called with the raw programmatic value in _budget_for_agent. The double/triple normalization (config parse → executor → factory) is harmless but means the annotation doesn't describe what the factory actually accepts; consider narrowing the annotation or documenting that strings are intentionally accepted.
  4. Minor: several tests (tests/agent/test_tool_budget_explicit_threshold.py) assert exact products like int(65_536 * 4 * 0.30) and int(65_536 * 4 * 0.15), which inline-replicate the scaling formula. If the formula in budget_config.py changes, these tests fail for the wrong reason. Asserting relative contracts (explicit cap wins; turn budget stays below the 200K default; non-positives fall back) would be more robust to scaling changes.

@x1051445024

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed all four suggestions in commit b69cce5:

  • Documented that tools.tool_result_persist_threshold_chars is read at agent initialization and remains fixed for the conversation.
  • Added a warning for invalid programmatic values, emitted at most once per agent.
  • Updated the budget_with_persist_threshold annotation and documentation to reflect supported integer and whole-number string inputs.
  • Replaced formula-dependent assertions in the executor tests with behavioral comparisons against the context-scaled budget, and added coverage for the warning behavior.

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

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants