fix(workspace): a read the harness would cut can no longer invite a full-body overwrite (#417) - #515
Conversation
… it (tinyhumansai#417) build_agent never called AgentBuilder::context_config, so the vendored ContextConfig::default() supplied the 16 KiB per-tool-result cut and no OpenCompany source mentioned the number. Tools that must size their results against it had no way to read it, and workspace_read capped itself at 64 KiB instead. Mint TOOL_RESULT_BUDGET_BYTES from the vendored default and pass it explicitly. Behaviour-identical today; the number is now chosen here.
…inyhumansai#417) workspace_read capped a body at 64 KiB and treated its own dropped==0 as proof the model had the whole note, emitting the write-eligible branch: 'call workspace_write with the complete new body'. The harness then cut the result to 16 KiB. For a note between the two, the agent wrote back what it had seen, the 64 KiB write gate accepted it, and the remainder of an operator's note was destroyed with nothing reporting a loss. MAX_CONTENT_BYTES is now TOOL_RESULT_BUDGET_BYTES minus the framing a read wraps a body in, so a full result always fits and the outer cut never fires on this tool. A const assertion fails the build if a later edit separates the two again. The read header states 'returned X of Y bytes' so a partial read is legible from the first line rather than only from a marker at the end, and echoed paths are bounded so an unbounded name cannot push the header past its reservation. Notes over 12 KiB become agent-read-only via the existing current_len gate. Operator edits are unaffected: the console and REST handlers call the WorkspaceStore port and never enter this module.
…the header (tinyhumansai#417) MAX_LIST_ENTRIES is 300 but an entry renders at ~90-105 bytes, so the harness budget bit at ~184 entries — below the count bound. The 'narrow with prefix' marker was therefore never generated, and the unaddressable notice sat below it; both were at the end of the body, which is the end an outer cut takes first. The advice disappeared exactly on the listings long enough to need it. Stop appending entries when the rendered bytes would exceed the budget minus a header reserve, keeping 300 as an absolute cap, and emit the header and the unaddressable notice above the entries. A trailer the model must act on now sits where an outer cut cannot remove it.
…al turn (tinyhumansai#417) Unit tests can only assert a read renders under some number; they cannot see ToolOutputMiddleware, which cuts every tool result on its way into the model's context. That second bound is what made the old 64 KiB cap a data-loss bug. Read a 20 KiB note through the whole pipeline and assert on the bytes the model actually received: no write invitation, no tool_result_budget marker, and the closing fence last. Under the old cap the model received a [tool_result_preview] envelope carrying 'the complete new body' instruction over an unterminated fence — which is the bug, verbatim.
tinyhumansai#417) The 64 KiB figure is stale, and the paragraph did not say that operator edits never enter the agent tool path — the property that makes a note going agent-read-only a containment change and not a loss of access.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe harness now sets an explicit tool-result budget. Workspace listings and reads derive limits from that budget, preserve required guidance, and reject writes based on partial reads. Unit and end-to-end tests cover truncation, boundaries, and output framing. ChangesWorkspace budget alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/harness/workspace_tools.rs`:
- Around line 672-681: Update the listing loop around the line construction and
MAX_LIST_BYTES check so an entry whose rendered path exceeds the limit does not
terminate the entire listing. Bound the displayed path before constructing the
line, or skip that entry with a clear notice, then continue processing
subsequent entries while retaining the node ID; add coverage for an oversized
path followed by normal entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d47069eb-ff6c-4b45-bc6a-f79a7b71f928
📒 Files selected for processing (4)
docs/spec/runtime/manifest.mdsrc/harness/build.rssrc/harness/workspace_tools.rssrc/harness/workspace_turn_test.rs
…ing (tinyhumansai#417) A node name is operator-supplied and no backend length-caps it, so a single deep path could render a line larger than the byte budget and break the render loop on its first iteration — reporting a near-empty listing for a workspace that is almost entirely listable. Bound the echoed path with the same clamp the header already uses. It announces its own drop, and the id stays unclamped so a bounded entry is still addressable.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Addressed in 18f90d7. The finding was valid. The render loop bounds the listing by bytes and Fixed by bounding the echoed path with
New test Negative control run — reverting the clamp fails that test alone (37 passed / 1 failed), restoring it returns 38. Scope is now 50 tests, both clippy lanes and fmt clean. Not applied: skipping the entry instead of bounding it. That would drop a real node from the listing to save display width, and a silent omission mid-listing is the failure mode this issue exists to remove. |
2 participants
Summary
Closes #417.
workspace_readcapped a note atMAX_CONTENT_BYTES(64 KiB) and emitted itswrite-eligible branch whenever its own
dropped == 0— telling the agent tocall
workspace_writewith "the complete new body". But the harness applies asecond, smaller bound to every tool result (
DEFAULT_TOOL_RESULT_BUDGET_BYTES,16 KiB), and
build_agentnever calledcontext_config, so that default wasinherited silently.
For a note between the two numbers the module believed nothing was dropped, the
model saw roughly the first 16 KiB, and it complied — writing back the fragment
it had seen.
MAX_WRITE_BYTESaccepted it. The rest of the note was destroyedand nothing in the loop reported a loss.
This restores the invariant the module documents for itself — a truncated read
can never become a write — by making the module's own bound the effective one,
so its gate and the model's actual view agree.
The same shape was fixed in
workspace_list: its "narrow withprefix" guidancesat at the end of the body, so it was cut away precisely when the listing was
long enough to need it. Trailers the model must act on now live in the header,
where an outer cut cannot remove them.
API Or Behavior Changes
Notes between 12 KiB and 64 KiB become agent-readable but not agent-writable.
That window is the data-loss window itself, so the change is intended, but it is
user-visible: an agent that could previously revise such a note now receives
Refused: … larger than the 12288-byte read limit.Operator edits are unaffected — the console and REST write path
(
server::ops::workspace::write_file) does not go throughworkspace_tools.rs.Verified directly rather than assumed: that module has no reference to either cap,
both are private consts with no re-export, and the frontend applies no size check.
workspace_read's header now reportsreturned X of Y byteswhen truncated.workspace_listrenders size-aware, keeping 300 as an absolute cap, with thenarrowing guidance and the
unaddressablenotice above the entries.The tool-result budget is now stated explicitly in
build_agentrather thaninherited. Behaviour is identical today; the point is that the number is chosen
here instead of by accident.
Tests
49 tests in scope, up from 44, including a turn-level test through the real
middleware rather than a mock.
Every negative control was executed — break the fix, run, observe, restore:
MAX_CONTENT_BYTES→ 64 KiBrendered 66505 bytes, over the 16384-byte harness budget, the othera partial read still invited a full-body overwriteREAD_OVERHEAD_BYTES→ 0current_lenwrite gaterendered 23806 bytes, over the 16384-byte budget[tool_result_preview]envelope carrying the write-eligible instruction, an unterminated--- BEGIN WORKSPACE NOTE, and[… 573 bytes truncated by tool_result_budget …]with no closing fenceThat last one is the defect reproduced verbatim in the real pipeline.
Worth recording: the pre-existing test for this area passed with the bug present.
It exercised cap + 4 KiB, so it was structurally blind to the 16–64 KiB window.
cargo fmt --all -- --checkcargo clippy --locked --all-targets -- -D warningscargo clippy --locked --no-deps --features openhuman,tinycortex --all-targets -- -D warningscargo test --features openhuman,tinycortex --lib harness::workspace— 49 passedAdjacent scopes re-run green:
harness::build19,policy::92,runtime::builder23,harness::toolbelt15.cargo build --all-targets: N/A — the two clippy lanes above build all targets inboth feature configurations.
Documentation
docs/spec/runtime/manifest.md— the 64 KiB figure was stale; the caps are nowdocumented as derived from the harness budget.
Open question for review
READ_OVERHEAD_BYTESis 4096, but measured framing is ~969 bytes — about 4xheadroom, and every byte of it comes out of what an agent may write. Tightening to
2048 would still double the measured worst case and would raise the writable
ceiling from 12 KiB to 14 KiB. Kept at 4096 here; happy to change it if reviewers
prefer the tighter number.
Separately, and out of scope: the operator write path has no size limit of its
own — no handler validation, no body-limit layer, no store cap. The only backstop
is axum's implicit 2 MiB JSON extractor limit. Pre-existing, worth its own issue.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation