fix(tools): stabilize sandbox read_file results

[Christopher-Schulze]

What does this PR do?

Makes hermes_tools.read_file() a stable programmatic API inside execute_code.

Sandbox reads now return raw content without chat display gutters on every call, including repeated reads. They still traverse the standard tool dispatcher, middleware, hooks, registry, file safety checks, redaction, and pagination. The chat-facing read_file behavior remains unchanged.

Related Issue

Fixes #93749

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✅ Tests (adding or improving test coverage)

Changes Made

• Add a scoped programmatic read mode to tools/file_tools.py that disables chat-only line gutters and repeated-read dedup responses.
• Route local UDS and remote file-RPC sandbox reads through that mode while preserving the standard dispatcher pipeline. Session-kernel cells copy Context via ctx.run(), which drops ContextVars, so CellAuthority._invoke applies the same programmatic wrap on the per-call path.
• Native no-gutter fast path: ShellFileOperations.read_file(..., line_numbers=False) builds raw content directly (same per-line clamp, no gutter) instead of add-then-strip; the old _strip_read_file_gutter helper is removed.
• Canonical newline semantics: the structured-document early branch terminates its raw page with \n, byte-identical to the sed/cut output of the native file_ops path on the same window.
• Documented backstop: with deduplicate=False the consecutive-loop breaker does not apply to sandbox RPC callers — the RPC call limit and per-script timeout remain the guard.
• End-to-end and focused regression coverage for repeated reads, literal gutter-like content, errors, pagination, chat dedup isolation, dispatcher routing, and a three-way byte-identity pinning test (chat early branch vs. programmatic vs. real ShellFileOperations.read_file(line_numbers=False)).

How to Test

1. Run scripts/run_tests.sh tests/tools/test_code_execution_programmatic_read.py tests/tools/test_code_execution.py tests/tools/test_file_operations.py -q.
2. Run ruff check . and uv lock --check.
3. Execute a script that calls hermes_tools.read_file(path) twice and confirm both dicts contain identical raw content.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS arm64, Python 3.11

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Focused programmatic-read suite: 9 passed. CI surfaced three rounds of test fakes/assertions still matching the old read_file signature (no line_numbers kwarg) in test_file_read_guards.py, test_file_staleness.py, test_read_loop_detection.py, and test_file_tools.py — all updated; the final head passes the full project checker with every blocking gate green. The broader file-tools suite otherwise shows only the known macOS /tmp-vs-/private/tmp path-expectation failures that reproduce identically on origin/main.

The complete suite was also started and reached 1,170 passes before being stopped after three unrelated failures caused by the optional anthropic SDK being absent from the locked all/dev environment. The first failure reproduces identically on origin/main (tests/agent/test_auxiliary_transport_autodetect.py: 15 passed, 1 failed).

[Enough1122]

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

The core diagnosis is right and the fix is well-scoped: a programmatic RPC caller should get idempotent, display-free reads, and the chat-facing dedup/line-number contract is preserved verbatim (the test_programmatic_read_does_not_change_chat_dedup_contract test pinning "1|alpha\n2|beta\n3|" and the unchanged second read is the correct guardrail). Routing both RPC loops through _dispatch_sandbox_tool_call (tools/code_execution_tool.py:888-902) with a ContextVar is a clean, thread-safe mechanism, and keyword-only params keep every existing caller bit-compatible.

Three concerns, mostly about the gutter-stripping path:

1. Add-then-strip is a fragile round trip (tools/file_tools.py:1649-1658, applied at :1896-1902). _strip_read_file_gutter re-derives raw content by pattern-matching the exact f"{line_number}|" prefix per line, duplicating knowledge of _add_line_numbers's output format in a second place. If the formatter ever changes (padding, sentinels), stripping silently passes numbered content through — and no test would notice, since the suite only strips content the current formatter produced. Worse, if file_ops.read_file internally clamps an out-of-range offset to a real line, the returned gutters start at a different number than start_line and the prefixes survive into "stable" programmatic content. The fast path at :1754-1761 already builds raw content natively; giving file_ops.read_file a native no-numbering mode (or always routing through the fast path) would remove the whole class.

2. Two construction paths can now drift — the early result_dict branch (:1754) natively omits numbers while the file_ops branch adds-and-strips. Same request, different synthesis. A test that forces each path (e.g. mocked file_ops) and asserts byte-identical programmatic output would pin them together.

3. The sandbox loses its read-loop breaker (:1934-1962). With deduplicate=False, nothing records read_history/consecutive, so a sandbox script looping read_file forever is no longer caught by the consecutive-read guard. That's presumably intentional — stability trumps — but it leans entirely on the RPC call limit in code_execution_tool as the remaining backstop; worth a comment confirming that limit covers this, so a future refactor doesn't leave zero protection.

Solid tests; the pagination-without-gutters case (:90-102) is the right shape to keep.

[Christopher-Schulze]

Thanks for the structured review — all three points are addressed on the updated branch (head d45a861a7f).

1. Add-then-strip is fragile → native no-gutter path.
ShellFileOperations.read_file() now takes line_numbers=False and builds the raw content natively — the same per-line clamp as _add_line_numbers, but without ever materializing the gutter. The _strip_read_file_gutter helper is removed entirely (clean cutover), so offset-clamped gutters can no longer survive into "stable" RPC content.

2. Two construction paths could drift → byte-identity pinned by test.
The structured-document early branch and the file_ops path now share canonical newline semantics (a raw page ends with \n, matching sed/cut's always-newline-terminated output). A three-way pinning test reads the same window through the chat early branch, the programmatic path, and a real ShellFileOperations.read_file(line_numbers=False) call and asserts byte-identical content.

3. No read-loop breaker with deduplicate=False → documented backstop.
Correct that nothing is recorded in read_history/consecutive on this path; a comment at the tracking block now states explicitly that sandbox RPC callers are bounded by the RPC call limit and per-script timeout instead, while the consecutive-loop guard only protects the chat-facing model loop.

Verification: focused programmatic-read suite 9 passed; ruff clean; project checker passes all blocking gates on the final head.

[Christopher-Schulze]

Follow-up on the CI runs after the last update: three rounds of test fakes/assertions were still matching the old read_file signature (no line_numbers kwarg) — test_file_read_guards.py, test_file_staleness.py, test_read_loop_detection.py, and the call assertion in test_file_tools.py::TestReadFileHandler. All updated; final head is e68b8d13f0, and CI is now fully green (22 passed checks, 0 failures). The production changes are unchanged from what I described above.

[Christopher-Schulze]

Rebased onto current main (25fcc8ad1) after the branch went dirty.

Conflict in tools/code_execution_tool.py: kept main's session-kernel dispatch= override and routed it through _dispatch_sandbox_tool_call so programmatic read_file wrapping still applies on the per-call path. allowed_tools deny-all from #86148 is preserved.

Session kernels copy cell Context via ctx.run(), which drops ContextVars set on the RPC thread. CellAuthority._invoke now applies programmatic_read_context so sandbox reads stay raw even inside session cells.

Head a6a98f3d3. Focused programmatic-read suite 9/9; all contributor blocking gates passed. The two remaining file-tools failures are the known macOS /tmp vs /private/tmp path-expectation mismatches that reproduce identically on origin/main.

[kvnloo]

Scripts calling hermes_tools.read_file inside execute_code got three different shapes back for the same call — raw content, line-numbered display text, or a dedup stub with no content key at all (#93749). The numbered-text case is the nasty one: parsing 21|text as data corrupts silently instead of erroring. This pins the sandbox read to one stable raw shape while the chat path keeps its gutters and dedup. Approving.

Mechanics verified on the code and by running the suites. The fork is a ContextVar (_programmatic_read) consulted in the dispatcher (_handle_read_file), applied at all three sandbox entry points: the RPC handler (code_execution_rpc.py), _dispatch_sandbox_tool_call (per-call sandboxes), and CellAuthority._invoke — the last one is load-bearing, because session kernels run handle_function_call inside ctx.run() which copies the cell context and drops RPC-thread ContextVars, so the flag has to be re-applied per call (verified: the outer wrap in _dispatch_sandbox_tool_call does not propagate into the cell copy; _invoke's inner wrap does the work). The native no-gutter path is add-not-strip: _clamp_read_file_lines applies the identical per-line clamp as _add_line_numbers (same max_line_length, same "... [truncated]" marker — checked character for character) without the gutter, and the structured-document early branch now \n-terminates its raw page to match sed/cut output. Test runs on the PR head: 10/10 new programmatic-read tests, plus 63 read-guard/staleness/loop, 44 code-execution, 71 file-operations — all green, including the three-way byte-identity pinning test.

Non-blocking, the sharpest one: deduplicate=False skips the whole _record_successful_read, not just the loop breaker the body documents. That function also establishes full_write_baselines, updates read_timestamps, and calls file_state.record_read. So a script that reads a file via hermes_tools.read_file no longer counts as "this task has seen the file" — a later chat write_file on that path now hits the "exists but this task has not seen its full current content" refusal where it previously passed (admittedly on a guttered baseline). If the refusal is the intended new contract, name it in the body; otherwise consider keeping the loop-breaker bypass but still recording the baseline and timestamps.

Non-blocking: _read_extracted_document builds the chat page with page_text.rstrip("\n"), which strips all trailing newlines — but _add_line_numbers deliberately drops exactly one terminator so a genuinely selected trailing blank line keeps its gutter number. A chat read of a structured document whose page ends with a blank line now loses that blank line's number (base rendered N|a\n(N+1)|). One-line fix: strip one, not all, before the gutter join.

Non-blocking: tests/tools/test_programmatic_read_raw_repeat.py duplicates test_programmatic_read_returns_raw_content_on_repeated_calls in the main test file, and its if programmatic is not None fallback branch can never fire post-merge. Dead weight; fold it into the main file.

One pattern thread, offered not required: the raw-read contract now lives in an ambient ContextVar with three wiring sites that must each remember to set it. The next consumer of "read with the raw contract" (MCP server reads, a future batch tool) grows a fourth. If the programmatic variant were a first-class registry entry instead of a flag on the dispatcher, the contract would travel with the call instead of the thread.