6 Commits

Author SHA1 Message Date
HaotianChen616
756eac0d1a
feat(tool):Add structured synopsis for oversized tool output previews (#3377)
* Improve tool output preview synopsis

* Add JSON path anchors to tool output synopsis

* Fix JSON synopsis line anchors

* fix(synopsis): tighten detectors and fix CSV first-row join

Address review feedback from @willem-bd on PR #3377.

Detectors:
- _looks_yaml now requires >=3 key-shaped lines and refuses bare
  uppercase-tag lines ('INFO: ...', 'ERROR: ...') that look like log
  lines and would round-trip into a flat string dict via safe_load.
  Previously a 200-line log file was classified as 'YAML object with
  3 top-level keys' and lost every line, count, and middle signal.
- _try_yaml refuses payloads that safe_load collapses to a dict of
  all strings (the shape tracebacks and log lines collapse into).
- _try_table applies the header-must-look-like-identifiers and
  minimum-row-count guards only to TSV, since the same safeguards
  would reject legitimate small CSVs. Refuses tab-indented bash
  output, ls -l listings, and tree dumps.

Rendering:
- CSV first data row is now rendered as a key=value list joined by
  ' | ' (e.g. 'name=Ada | description="a fine, brilliant logician"
  | score=98'). The previous delimiter.join(rows[1]) silently
  re-split cells that contained the delimiter inside a quoted cell,
  which made the synopsis report a 3-column table as 5 columns and
  misled the model about column count and content.

Text summary:
- _summarize_text now omits the closing excerpt entirely when the
  input is shorter than 2 * _TEXT_EXCERPT_CHARS, since the previous
  opener/closer slices overlapped and duplicated text for short
  inputs (build_tool_output_synopsis is reachable directly from
  tests and other callers that pass small inputs).

Tests:
- Update test_table_preview_extracts_columns to assert the new
  key=value list format.

* fix(synopsis): drop JSON path line/byte offset hints

The path-location hint was computed by string-searching for the
quoted key in the original content and reporting its byte offset and
line number. This anchors at the first textual occurrence of the
key string, which is wrong when the key also appears as a value
earlier in the document, or when the same key recurs at multiple
depths. With nested paths the anchor drifts further on every step
because the search cursor is advanced past each previous match.

Concrete cases:
  content = '{"label": "items", "items": {"id": 1}}'
  _json_path_location(content, ['items'])
  -> ' (line 1, byte offset 10)'  # the value, not the key

  content = '{"data": {"info": 1, "data": {"info": 2}}}'
  _json_path_location(content, ['data','data','info'])
  -> ' (line 1, byte offset 30)'  # the inner first 'info', not the second

The synopsis instructed the model to 'Start near the line hints
above when present', so a wrong anchor would send read_file into
the wrong region of the persisted .tool-results file.

Drop the hint entirely. The path itself ('$.data.items') is
already useful navigation; the agent uses read_file with start_line
based on its own judgement of where the relevant slice is.

Tests:
- Update test_json_preview_reports_nested_paths to assert no 'line '
  or 'byte offset ' appears in the body before the Access section.
- Rename test_json_line_hints_use_original_content_offsets to
  test_json_paths_are_emitted_without_line_hints and invert the
  assertions to check the hints are absent.

* fix(synopsis): bound _scalar_examples recursion depth

Mirror the _JSON_STRUCTURE_DEPTH cap used by _json_container_paths
and _json_shape so that deeply nested JSON cannot trigger
RecursionError inside build_tool_output_synopsis.

In ToolOutputBudgetMiddleware.awrap_tool_call the synopsis is built
inside asyncio.to_thread(_patch_result, ...); a RecursionError
would surface as a tool-call failure and the user would lose the
entire output. 300-level nested JSON is well inside what an
attacker-controlled MCP tool, a JSON-RPC-over-JSON-RPC chain, or a
buggy serializer can produce.

* feat(synopsis): restore inline raw head/tail sample

The synopsis-only preview silently dropped the raw head/tail bytes
that preview_head_chars / preview_tail_chars used to inline. For
text/code/log outputs the agent lost first/last KB of the actual
content and had to issue a follow-up read_file round-trip to see
the trailing region (last paragraph of a fetched article, final
error line in a traceback, closing diagnostics of a bash run).

Restore an inline 'Raw sample (head + tail)' section in the preview.
The section is composed by slicing head_chars from the start and
tail_chars from the end of the content (with a '...' separator
between them, and the tail suppressed when it would overlap the
head). For binary-like output, the synopsis's own sample is reused
unchanged.

This makes preview_head_chars / preview_tail_chars operational
again for every kind except binary, which already had a sample
channel.

Tests:
- Rename test_json_preview_extracts_structure_instead_of_head_tail
  to test_json_preview_includes_structure_and_raw_sample and assert
  the raw sample section is present and the payload is reachable
  in the head slice.

* test(synopsis): add regression tests for willem-bd review findings

Add 8 regression tests under TestToolOutputSynopsis, one per
finding in @willem-bd's review of PR #3377:

- test_review_5_log_lines_are_not_misclassified_as_yaml
  Pins the YAML detector to refuse 'LEVEL: message' log lines.
- test_review_6_json_paths_are_emitted_without_byte_offset
  Pins the removal of byte/line hint from JSON path descriptions.
- test_review_7_scalar_examples_respects_depth_cap
  Pins that 500-deep nested JSON does not raise.
- test_review_8_csv_first_row_quoted_cells_round_trip
  Pins the new key=value list format for CSV first-row rendering
  and asserts that quoted cells with embedded delimiters survive.
- test_review_9_tsv_detector_rejects_tab_indented_bash
  Pins that tab-indented bash output is not classified as TSV.
- test_review_10_preview_includes_raw_head_and_tail_sample
  Pins the restored inline 'Raw sample (head + tail)' section.
- test_review_11_short_text_does_not_duplicate_excerpts
  Pins that closer is suppressed for inputs shorter than
  2 * _TEXT_EXCERPT_CHARS.
- test_review_12_preview_head_tail_chars_are_operational
  Pins that head_chars / tail_chars are wired into the rendered
  preview and not silently dropped.

Also removes the now-stale 'byte offsets are approximate anchors'
sentence from render_tool_output_preview's Access block; the
synopsis no longer emits byte/line hints, so the guidance to
'start near the line hints' was misleading.

* fix(synopsis): resolve lint errors on tool output budget tests

Local 'make lint' on feat/tool-output-synopsis-preview (after fast-forward
to current main) failed with three errors in tests added by PR #3377:

- E501: 307-char bash_out literal in test_review_9_tsv_detector_rejects_tab_indented_bash
- E741: ambiguous single-letter 'l' in test_review_11_short_text_does_not_duplicate_excerpts
- E741: same ambiguous 'l' on the closing assert

Replace the long literal with a join of per-row entries, rename the loop
variable from 'l' to 'ln', and run ruff format on the two touched files
to absorb the formatting drift introduced by the merge with main.

Verification:
- make lint   -> All checks passed; 643 files already formatted
- pytest tests/test_tool_output_budget_middleware.py -> 110 passed

* fix: address willem-bd review findings (code/csv misclassification, text duplication, line snapping, dead constant, xml hardening, depth consistency)

- _CODE_HINTS: require stronger signals for use/fn (trailing ; or parenthesised)
- _try_table: apply _TABLE_MIN_DATA_ROWS gate to CSV too (not just TSV)
- config.example.yaml: correct misleading comment about preview_head/tail_chars
- _summarize_text: skip opener/closer excerpts when raw sample will be appended
- _build_raw_sample: snap to line boundaries for clean truncation
- Remove dead constant _TABLE_FIRST_ROW_CHARS
- Prefer defusedxml for XML parsing (billion-laughs protection), fallback to stdlib
- Replace _json_shape magic number 2 with named _JSON_SHAPE_MAX_DEPTH constant
- Update tests to match new CSV gate (>=5 rows) and line-snapped sample counts

* style: ruff format fix for tool_output_synopsis.py and test_tool_output_budget_middleware.py

* fix(tool-output): address 4 review comments - DoS hardening + size cap

1. XML entity-expansion DoS: skip _try_xml when defusedxml is not
   available (SafeET is None), falling through to text + raw sample.
   (cid=3587721336)

2. YAML alias-bomb DoS: refuse to parse YAML content > 500 KB.
   (cid=3587721340)

3. Unbounded content parse: add _MAX_SYNOPSIS_INPUT_BYTES=5MB cap;
   oversized output falls back to raw head/tail sample instead of full
   parse. (cid=3587721346)

4. Scalar examples surface mid-document values: add docstring note
   that the synopsis is a structural summary, not a confidentiality
   filter. (cid=3587721353)

* fix(tool-output): ruff format the synopsis string to one line

---------

Co-authored-by: qinchenghan <qinchenghan@huawei.com>
2026-07-16 16:41:04 +08:00
Aari
3be3969f8f
fix(agent): snap tool-output tail forward so fallback truncation respects max_chars (#4017)
* fix(agent): snap tool-output tail forward so fallback truncation respects max_chars

_snap_to_line_boundary() moves an offset backwards to the preceding newline.
That is correct for the head's end offset, but _build_fallback() also applied it
to the tail's start offset, where moving backwards lengthens the tail. On output
whose last newline sits in the second half (log lines followed by one long
unbroken line: minified JSON, base64 artifacts), the returned string exceeded
max_chars by up to 17x, defeating the guard that exists to stop a single large
tool result from blowing the model context.

Add _snap_start_to_line_boundary(), the forward-snapping mirror, and use it for
the tail offset. The existing head_end guard becomes redundant because a forward
snap can only move the tail start away from the head.

test_result_never_exceeds_max_chars already asserted this invariant but passed
newline-free content, so the snapping branch was never exercised.

* test(agent): pin the forward-snap direction of the fallback tail

The bound test only asserts that the result fits max_chars. Its content has
no newline inside the tail's snap window, so _snap_start_to_line_boundary
returns pos unchanged and the test stays green even with the snap removed.

Place a newline in the window so the snap has to fire: the tail must begin
after it, which the pre-fix backward snap does not do.
2026-07-10 08:07:25 +08:00
Xinmin Zeng
09988caf95
feat(skills): request-scoped secrets for skills (closes #3861) (#3871)
* feat(sandbox): per-call env injection + platform-secret scrubbing for skills

Add an env parameter to Sandbox.execute_command (abstract + local + AIO) so request-scoped secrets can be injected into skill subprocesses, and scrub platform credentials (*KEY*/*SECRET*/*TOKEN*/*PASSWORD*/*CREDENTIAL*) from the inherited environment by default so scoped injection is not security theatre. LocalSandbox always passes an explicit scrubbed env; AioSandbox routes env-bearing commands through bash.exec(env=) on a fresh session and leaves the legacy persistent-shell path unchanged. Part of #3861. BEHAVIOR CHANGE: execute_command no longer inherits the full os.environ; Windows encoding tests updated to assert the scrubbed dict.

* feat(skills): parse required-secrets frontmatter declaration

Add SecretRequirement and Skill.required_secrets, and parse the required-secrets SKILL.md frontmatter field (a string list or {name, optional} mappings), dropping malformed entries with a warning so one bad declaration does not invalidate the skill. The declared name is both the context.secrets key and the env var injected at activation. Part of #3861.

* feat(runtime): request-scoped secret carrier (context.secrets)

Add SECRETS_CONTEXT_KEY + extract_request_secrets, centralising the context.secrets carrier contract. The existing context passthrough (build_run_config -> _build_runtime_context) already carries the sub-key to runtime.context without mirroring it into configurable; characterization tests lock that behaviour. Part of #3861.

* feat(skills): inject declared secrets at slash-activation into bash env

Binding point A: when a skill is slash-activated, SkillActivationMiddleware resolves its declared required-secrets against the request's context.secrets and writes the per-run injection set to runtime.context. The bash tool forwards that set to execute_command(env=). A skill cannot harvest a host platform credential (is_host_platform_secret guard, cf. GHSA-rhgp-j443-p4rf), and injected values are redacted from bash output (mask_secret_values) so an echoed secret never re-enters the prompt/trace. Part of #3861.

* test(skills): lock the five secret leak surfaces + add trace redaction helper

Regression tests assert the secret value is absent from all five surfaces: prompt (activation message), checkpoint (graph state vs context separation), audit (journal records names only), trace (metadata builder never copies context; never mirrored to configurable), and stdout (mask_secret_values). Add redact_secret_context_keys as a defensive helper for any context serialization. Part of #3861.

* docs(backend): document request-scoped secrets for skills

Add Request-Scoped Secrets subsection (Skills) + env policy note (Sandbox) and the execute_command(env=) signature change, per the doc-sync policy. Part of #3861.

* fix(skills): close gaps found by end-to-end verification of request-scoped secrets

Real-gateway e2e + independent review of #3861 surfaced three defects, now fixed:

1. Slash activation never fired in the live chain. InputSanitizationMiddleware
   wraps user input in BEGIN/END markers before SkillActivationMiddleware sees it,
   and the original text was only preserved when an upload or IM channel set it.
   For a plain text message the slash command became undetectable, so no secret
   was ever resolved. Fix: the sanitizer now setdefaults the pre-wrap text into
   ORIGINAL_USER_CONTENT_KEY (additive; sanitization behaviour unchanged), so
   slash activation works for all messages. Pre-existing latent bug surfaced here.

2. The raw request config (with context.secrets) was persisted to runs.kwargs_json
   and echoed by the run API (RunResponse.kwargs). Fix: redact_config_secrets()
   strips secret-bearing context keys from the persisted/echoed copy in start_run;
   the live config that drives the run keeps them. build_run_config now also sets
   configurable.thread_id on the context path (the checkpointer requires it).

3. Connection-string credentials (DATABASE_URL, REDIS_URL, SENTRY_DSN, GH_PAT, ...)
   were not scrubbed from the inherited sandbox env. Fix: env_policy adds a *DSN*
   pattern plus an explicit connection-string denylist (no blanket *URL* — benign
   service URLs stay readable).

Verified end-to-end via a real gateway run (real LLM + skill activation + bash):
the secret reaches the sandbox subprocess and appears in NONE of prompt, trace,
checkpoint, audit, stdout, runs.kwargs_json, or the run API. Part of #3861.

* docs(backend): document the env scrub, persistence redaction, and sanitizer interaction

Sync the Request-Scoped Secrets section with the verification-driven fixes: inherited-env scrub (incl. connection-string denylist), run-record/run-API redaction as the 6th sealed leak surface, and the sanitizer preserving original content so slash activation fires. Part of #3861.

* fix(skills): inject caller secret over scrubbed host value; drop redundant host-name guard

A real-world demo (a skill calling a third-party cloud API with a request-scoped
key) exposed that the is_host_platform_secret guard was both wrong and harmful:
it refused to inject a caller-supplied secret whenever a same-named variable
existed in the Gateway env — which is exactly the #3861 use case (a per-user key
overriding a shared platform key). The guard was also redundant: build_sandbox_env
already scrubs secret-looking names from the inherited env before injection, so a
skill can never read a host credential — it only ever receives the caller's value.

Remove the guard; the injected (caller) value simply wins over the scrubbed host
value. Verified end-to-end: the agent called the real cloud API successfully with
the caller's key, the host's same-named key was scrubbed and never used, and the
caller's key leaked to none of the surfaces. Part of #3861.

* fix(skills): address review on request-scoped secrets (#3861)

Review fixes from PR #3871:

- E2BSandbox.execute_command now accepts env/timeout and routes them to
  commands.run(envs=, timeout=). The bash tool passes env= unconditionally,
  so the prior signature (command only) raised TypeError on every e2b bash
  call and broke e2b deployments entirely. env=None stays backward-compatible.
- SkillActivationMiddleware clears the active-secret set before resolving each
  activation, so a later skill in the same run never inherits an earlier
  skill's injection set (the #3861 contract: a skill only receives what the
  caller supplied AND that skill declared).
- AioSandbox env path uses a dedicated _DEFAULT_HARD_TIMEOUT — bash.exec exposes
  no idle/no-change timeout, so the prior reuse of the legacy idle constant
  conflated wall-clock vs idle semantics. The env path also retries on the
  ErrorObservation signature now, sharing the legacy persistent-shell recovery
  contract.
- mask_secret_values skips values below a minimum length floor so a short
  declared secret (e.g. "42") cannot shred unrelated bytes (exit codes,
  timestamps, sizes) of tool output. The secret is still injected into the
  subprocess; only the output mask skips it.

session_id reuse on the env path is intentionally NOT added: a shared session
could let request-scoped secrets ride the session env into later commands,
which the SDK does not contractually forbid. The fresh-session choice matches
the LocalSandbox model (each call is a fresh subprocess); the trade-off
(consecutive env-bearing calls do not share cwd/venv/exports) is documented on
_execute_with_env.
2026-07-03 07:51:22 +08:00
Zhipeng Zheng
e7b88a97ed
fix(security): add input sanitization middleware for prompt-injection defense (#3630) (#3662)
InputSanitizationMiddleware: escapes blocked XML tags in user messages (<system> -> &lt;system&gt;) — de-identify, don't reject (AWS PII-style). Wraps user input in plain-text boundary markers (--- BEGIN/END USER INPUT ---) per OWASP structured-prompt guidance. System-Context Confidentiality clause prevents LLM from revealing internal instructions. Boundary-marker layer hardened against delimiter injection (self-suppression + break-out). Multimodal content blocks preserved during sanitization.

Addresses review feedback from @fancyboi999 and @WillemJiang:

1. Boundary-marker injection: strict startswith+endswith idempotency, neutralize user-supplied BEGIN/END tokens, forged-idempotency bypass fixed

2. Multimodal data loss: _rebuild_content preserves interleaved non-text blocks

3. CI fixes: replay_provider strips boundary markers before hashing, middleware chain assertions updated

4. Lint: ruff format applied on Linux (slice spacing + f-string collapse)

Test: 71 input_sanitization + 1 replay_golden + 14 tool_error + 93 tool_output_budget = 179/179 passed
2026-06-21 19:01:43 +08:00
Huixin615
64d923b0fd
fix(middleware): externalize oversized tool output into sandbox for non-mounted sandboxes (#3417)
* fix(middleware): externalize oversized tool output into sandbox for non-mounted sandboxes

ToolOutputBudgetMiddleware persisted oversized tool results to the host
filesystem and returned a /mnt/user-data/outputs virtual path. For sandboxes
that do not use thread-data mounts (e.g. remote AIO sandbox), that virtual
path does not exist inside the sandbox, so the model's read_file tool could
not read it back and reported 'file not found'.

Branch on SandboxProvider.uses_thread_data_mounts:

- Mounted sandboxes (local Docker, AIO + LocalContainerBackend) keep the
  original host-disk path; the host outputs dir is bind-mounted to the same
  virtual path inside the sandbox, so behavior is unchanged.

- Non-mounted (remote) sandboxes externalize into the sandbox itself via
  execute_command('mkdir -p ...') + write_file + 'test -s' validation. The
  validation step is required because AIO sandbox execute_command returns
  'Error: ...' as a string on failure instead of raising, so a silent mkdir
  failure would otherwise leak through.

Any failure (rejected subdir, mkdir/write/validate error) falls back to the
existing inline head+tail truncation, so an unreadable path is never returned
to the model.

The sandbox resolver reads the sandbox_id that SandboxMiddleware already
writes into runtime.state['sandbox']; it never calls provider.acquire(),
keeping the tool-call hot path free of blocking I/O. Tools that do not use a
sandbox (web_search, MCP, ...) resolve to None and fall through to inline
truncation, which is the safe behavior for them.

Fixes #3416

* fix(middleware): address Copilot review feedback on sandbox externalization

- Make get_sandbox_provider() lookup best-effort in _budget_content: only
  query when outputs_path or sandbox is available, and fall back to inline
  truncation if provider initialization raises rather than propagating
  the error. A resolved sandbox instance is sufficient on its own to take
  the non-mounted externalization branch.
- Strict-match the sandbox post-write validation echo
  (check.strip() == 'OK') to avoid false positives if execute_command
  ever surfaces unrelated stdout/stderr containing 'OK' as a substring.

Refs: #3417

* test: fix flaky tests relying on /nonexistent/... path under container root

Two tests in this module (test_returns_none_on_invalid_path and
test_fallback_when_disk_write_fails) used paths like
'/nonexistent/impossible/path' to trigger _externalize's OSError
fallback. These paths are creatable when the test process runs as root
inside the CI container: os.makedirs(..., exist_ok=True) successfully
creates the entire chain under /, so the OSError branch is never hit
and the tests fail. Reproducible on main independently of this PR.

Switch to '/dev/null/cannot-mkdir-here'. /dev/null is a character
device on both Linux and macOS, so os.makedirs always fails with
NotADirectoryError regardless of privileges, reliably exercising the
OSError fallback.

* fix(tool-output-budget): only consult sandbox provider when a sandbox is resolved

The previous revision called get_sandbox_provider() whenever externalization
was triggered, including on the legacy host-disk path. Environments without
a configured sandbox -- in particular CI runners without a config.yaml --
would raise FileNotFoundError there, get caught, and silently fall back to
inline truncation. That defeated the host-disk externalization path that
predates this PR and was the root cause of the regressing legacy tests.

Restructure the branching so the provider is only consulted when a sandbox
has actually been resolved for the current tool call:

  - sandbox resolved + provider.uses_thread_data_mounts: host-disk write
    (bind-mounted into the sandbox, equivalent to a sandbox-side write).
  - sandbox resolved + non-mounted provider:             sandbox write (#3416).
  - no sandbox + outputs_path:                           host-disk write
    (legacy / non-sandbox tools, no provider call at all).
  - otherwise:                                           inline fallback.

No test changes; the legacy externalization tests are provider-agnostic by
construction and now pass without monkeypatching.

Refs: #3416

* test(tool-output-budget): assert legacy path does not call sandbox provider

Lock in the contract introduced by d6e2d25b: when no sandbox is resolved
for a tool call, _budget_content must externalize to the host outputs
directory without consulting get_sandbox_provider(). Regressing this would
re-break legacy / non-sandbox tools in environments without a configured
sandbox (e.g. CI without config.yaml), which is the failure mode #3416's
fix avoids.

The test injects a get_sandbox_provider that raises on call, so any
future refactor that moves the provider lookup out of the sandbox-only
branch will fail loudly.

Refs: #3416
2026-06-08 12:24:48 +08:00
Xinmin Zeng
ca487578a4
feat(agent): add ToolOutputBudgetMiddleware for oversized tool output protection (#3303)
* feat(agent): add ToolOutputBudgetMiddleware for oversized tool output protection

Closes #3289. Adds a unified middleware that enforces per-result budgets
on ALL tool outputs (MCP, sandbox, community, custom), preventing
oversized external tool results from blowing the model context window.

Design informed by claude-code (persistToolResult), hermes-agent
(tool_result_storage), and pi (OutputAccumulator) — the three most
mature implementations in production coding-agent frameworks.

Key features:
- Disk externalization: oversized outputs written to thread-local
  .tool-results/ directory, replaced with compact preview + file
  reference. Model can read full output via read_file with offset/limit.
- Fallback truncation: head+tail truncation when disk is unavailable
  (no thread_data, write failure), ensuring the context is always
  protected.
- read_file exemption: prevents persist-read-persist infinite loops
  (independently discovered by claude-code, hermes-agent, and pi).
- Per-tool threshold overrides via config.
- Line-boundary-aware truncation (no partial lines in previews).
- Multimodal content passthrough (images/structured blocks skip budget).
- Historical ToolMessage patching in wrap_model_call for checkpoint
  recovery scenarios.

Related: #3222 (design RFC), #1844 (comprehensive context management),
#3137 (write_file args compaction), #1677 (sandbox tool truncation).

* test: add MCP content_and_artifact format coverage

Add 5 tests for MCP tool output format (list of content blocks):
- text content blocks are extracted and budgeted
- multiple text blocks are joined and budgeted
- image content blocks are skipped (multimodal passthrough)
- mixed text+image blocks are skipped
- small text blocks pass through unchanged

Total test count: 59 (was 54).

* fix(agent): address Codex review findings for ToolOutputBudgetMiddleware

Three issues identified by Codex code review, all fixed:

1. `enabled` config field was unused — middleware now checks
   `config.enabled` and skips all processing when disabled.

2. `_build_fallback` could exceed `fallback_max_chars` — the marker
   text itself (~139 chars) was not deducted from the budget. Now
   pre-computes marker overhead and falls back to hard slice when
   max_chars is smaller than the marker.

3. Sync file I/O in async path — `awrap_tool_call` now delegates
   `_patch_result` to `asyncio.to_thread` to avoid blocking the
   event loop during disk writes.

Tests updated to use realistic fallback_max_chars values (500+)
that can accommodate the marker overhead, plus two new tests:
- `test_result_never_exceeds_max_chars` (parametric across sizes)
- `test_very_small_max_chars_does_not_crash`

* fix(agent): address Copilot review — path traversal, async perf, shared config

1. Path traversal defense: sanitize tool_name via _sanitize_tool_name()
   (strips separators, .., absolute paths), validate storage_subdir is
   relative, and verify resolved filepath stays inside storage_dir.

2. Async hot-path optimization: add _needs_budget() cheap check before
   asyncio.to_thread offload — small outputs (99% of calls) skip the
   thread overhead entirely.

3. Replace shared module-level _DEFAULT_CONFIG with _default_config()
   factory to prevent cross-instance mutation of mutable fields.

12 new tests: TestSanitizeToolName (5), TestExternalizePathTraversal (3),
TestNeedsBudget (4).

* fix(agent): correct preview hint to match read_file actual API

read_file uses start_line/end_line (1-indexed line numbers), not
offset/limit. The previous wording was copied from hermes-agent
which has a different read_file interface.

* perf(agent): hoist hot-path imports, add model-call pre-scan (review #3303)

Address maintainer review feedback:

1. Hoist inline imports to module level — `import asyncio` (was in
   awrap_tool_call hot path) and `from dataclasses import replace`
   (was in _patch_result) now live at module top.

2. Add a cheap pre-scan to _patch_model_messages so the historical
   message list is not rebuilt on every model call when nothing is
   oversized (the common case once results are budgeted at tool-call
   time). Also adds the same _needs_budget gate to the sync
   wrap_tool_call for symmetry with awrap_tool_call.

The pre-scan is refactored into per-tool-aware helpers
(_effective_trigger / _tool_message_over_budget) that mirror the exact
trigger conditions in _budget_content — including tool_overrides — so
the fast-path can never produce a false negative (silently skipping
budgeting for a tool with a low per-tool threshold).

7 new regression tests lock the per-tool-override-through-pre-scan path
and the model-call early return.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-05-29 22:59:26 +08:00