Cap tool results at 32K; add grep and line ranges to readFile - #494
AshishKumar4 wants to merge 6 commits into
Conversation
github-actions
Bot
added
workshop/frontend
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| /** A file to search, or a directory to search recursively. Absent means the whole workpiece. */ | ||
| path?: string; | ||
| }; | ||
| output?: string; |
There was a problem hiding this comment.
This puts grep matches in the client-facing AiToolCall. For worktrees those matches are source text fetched through the creating chat’s private repository capability, but hydrateChatMessageForClient() only strips worktree change entries and pins; history reads and subscriber delivery will return this output unchanged. That violates the existing invariant that worktree content never reaches clients. Please keep replay-only grep data server-side or explicitly remove it on every client delivery path.
There was a problem hiding this comment.
Agreed. grep output is now stripped in hydrateChatMessageForClient, the one funnel every client delivery goes through; the field's doc says so. The integration test asserts delivered history carries the call and no output.
There was a problem hiding this comment.
Worktrees are now visible to clients, so bonk's complaint no longer applies. (Bonk was also incorrectly interpreting a missing feature as an intentional security decision.)
I think it's useful to have the tool output visible to the user, should they choose to expand it. It could make sense to truncate if it's very long, perhaps... but I actually wouldn't worry too much about that.
|
Posted 4 actionable inline findings. |
f9a6cf7 to
c4e333b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Findings
|
c4e333b to
9d75eaa
Compare
|
Findings
|
9d75eaa to
8bf39cb
Compare
|
On the two later Bonk batches: Fixed: the synthetic observeUserChanges result now goes through the bound (a user's diff can be a whole file); a grep recorded in a batch the user later reverted is elided on replay the way a reverted readFile is; the webFetch cut steps back off a surrogate; the prompt now names the file tools' parameter Not changing: describeBinding output over 32K is a gatekeeper with an outsized surface, and windowing it is that gatekeeper's problem to shape, not the bound's. Regex backtracking in the Overseer is the same exposure the binding's grep() has had since it took a RegExp, in the user's own workspace. A windowed read not preserving the final-newline distinction is by design; the unwindowed read is verbatim. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@AshishKumar4 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
8bf39cb to
70b3e4e
Compare
|
Bonk's last run timed out before it could post its one finding, but the finding was right: |
|
Findings
|
70b3e4e to
7775df1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Findings
|
|
Performance is inconclusive because all cohorts are non-comparable due to changed eval definitions. Across three trials, appointment-desk improved from 0/3 to 1/3 passes but mean duration and cost rose 14% and 54%; expense-ledger remained 0/3, while project-doc remained 3/3 with duration up 12% and cost up 35%. The successful appointment trial used 16 tool calls, including grep and a windowed read to recover from a failed edit, versus zero tool calls in every baseline trial. With only three stochastic trials and the gain confined to one run, neither the success nor the overhead can be confidently attributed to the change. |
7775df1 to
d58d5d5
Compare
|
Medium |
|
Performance is inconclusive because all cohorts are non-comparable due to changed eval definitions. Project-doc improved from 2/3 to 3/3 passes, with duration down 83%, cost down 75%, and tool errors from 2 to 0; the other cohorts remained 0/3. Candidate project-doc runs used grep and targeted windowed reads instead of consuming or rewriting the full large file. With only three stochastic trials, these gains cannot be confidently attributed to the change. |
|
The bonk comments are to be ignored ^ the evals are not to be used for now. |
d9b582a to
ec318a6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Finding
|
|
Performance is inconclusive because every cohort is non-comparable due to changed eval definitions. Appointment fell from 10/10 to 9/10 while 16% faster; expense fell from 9/10 to 4/10 despite 24% fewer tool calls and 15% lower cost; project rose from 7/10 to 8/10 with 28% lower cost. In failed expense trajectories, temporary verification helpers cleared persisted expenses, directly causing continuity failures. With 10 stochastic trials per cohort, these shifts cannot be confidently attributed to the change. |
| /** | ||
| * The grep tool's worktree half: the searchable files under `path` in the worktree's | ||
| * overlay-over-base view at `base`, with missing blobs pulled in one batch (see | ||
| * scanWorktreeForGrep). The gadget half needs no hook: a gadget's files are already in hand. |
There was a problem hiding this comment.
This is actually something I intend to clean up -- gadget code should be treated the same as worktree code, lazily. Some such cleanup has already happened in #513.
Let's rename this to grepWorkpiece and use it for both worktrees and gadgets, on the assumption that gadget code won't be loaded eagerly anymore in the future.
| }; | ||
| break; | ||
| } | ||
| // fallthrough |
There was a problem hiding this comment.
This is a super-ugly fallthrough. It's pure coincidence that the webFetch code below happens to also fit as the completion of the implementation of grep. It would be very easy for a future change to the webFetch tool to break the grep tool due to not noticing this. Much better to copy-paste the lines below.
| /** A file to search, or a directory to search recursively. Absent means the whole workpiece. */ | ||
| path?: string; | ||
| }; | ||
| output?: string; |
There was a problem hiding this comment.
Worktrees are now visible to clients, so bonk's complaint no longer applies. (Bonk was also incorrectly interpreting a missing feature as an intentional security decision.)
I think it's useful to have the tool output visible to the user, should they choose to expand it. It could make sense to truncate if it's very long, perhaps... but I actually wouldn't worry too much about that.
|
The grep tool is broken after rebasing on my worktree UI PR: it now fails on worktrees that have no changes, since such worktrees are no longer marked as "pinned" (for consistency with gadgets). |
|
Fix for grep tool: diff --git a/packages/workshop-backend/src/agent.ts b/packages/workshop-backend/src/agent.ts
index 82afccda..b888af57 100644
--- a/packages/workshop-backend/src/agent.ts
+++ b/packages/workshop-backend/src/agent.ts
@@ -2992,9 +2992,11 @@ async function runAgentPass(
hooks.resolveWorkpieceRoot(resolveToolWorkpieceId(workpiece), true, chatId);
let re = new RegExp(pattern);
let scan: GrepScan;
- let worktreeBase = worktreePinBases.get(workpieceId);
- if (worktreeBase !== undefined && pinnedGadgets.has(workpieceId)) {
- scan = await hooks.grepWorktree(worktreeTurnAccess, workpieceId, worktreeBase, path);
+ // A worktree, pinned or not, scans its overlay over its base: the pin's base while
+ // pinned, else the accepted commit (the same view the Worktree binding's grep() sees).
+ let base = worktreeBase(workpieceId);
+ if (base !== undefined) {
+ scan = await hooks.grepWorktree(worktreeTurnAccess, workpieceId, base, path);
} else {
// The same source readFile reads: committed code at the observed head for an
// unpinned gadget, else the session content. |
| // The live half of the tool-result bound (replay applies the same function to recorded | ||
| // results). This runs for thrown errors too, which pi has already rendered as text content, | ||
| // so an error message the model sees is bounded like any other result. | ||
| afterToolCall: async ({result}) => ({ |
There was a problem hiding this comment.
Hmm, I'm not a big fan of this approach of splicing every possible tool call result arbitrarily without regard to its content or structure.
Like, some tools return JSON, and this will still just treat it as a string as splice it anyway. Sure, nothing parses the JSON -- it's just fed into the LLM -- but this seems very weird.
It seems like this could be quite confusing for the model in a lot of cases, since there's no way for it to know unambiguously that the marker we spliced in was not part of the original content. And what if it happens to splice over actual structure of the JSON rather than just string content?
I think we really need to decide on a tool-by-tool basis how to handle oversized output.
The scan that resolves a path argument to searchable files, batch-pulls missing blobs, and matches lines now lives in grep.ts, so the agent's grep tool can call it without importing the RpcTarget. The binding's grep() and structuredGrep() call the same functions they did before. One fix while it moved: lines are split on `\r?\n`, so on a CRLF file `$` anchors at the end of the line rather than before a stray `\r`, and the rendered match carries none.
The agent could only search a worktree from executeCode, through the binding's grep(), and could not search a gadget at all. The new tool takes a workpiece, a JavaScript regular expression and an optional file or directory, and returns matches as `path:line:text`. Worktrees go through the scan the binding already uses, via one new hook on AgentHooks; a gadget's files are in hand, so that half is a filter over them. The output is recorded on the tool call and replayed as recorded, like webFetch, since a re-run could pull blobs or return something different. Replay elides a search whose content the user later reverted, using the same check as a reverted read.
readFile takes optional `startLine` (1-based) and `lineCount`. A read with neither returns the file verbatim as before. A windowed read returns the selected lines followed by `[lines A-B of N; next startLine: B+1]`, so the agent can page through a file it cannot or should not read whole. One function renders the window for the live tool and for history replay, so replayed reads show the model exactly what it saw. The editFile gate is unchanged: edits anchor on text, not line numbers, so a windowed read still counts as having read the file.
A single readFile or webFetch could put up to a mebibyte of text into the model's context. Every tool result the model sees now passes through boundToolResultText: live in pi's afterToolCall, which covers thrown errors as well as results, at the replay of recorded results, and at the synthetic observeUserChanges result a user's diff becomes. It keeps the head and the tail of the text and notes what it elided between them, since the end of a result often carries what matters most, such as the uncaught exception at the end of an executeCode log. The note fits inside the cap, so a bounded text bounds to itself. Recorded outputs are unchanged, except grep's, which is recorded already bounded: a broad match over several large files could otherwise exceed a storage record, and replay shows the model this text anyway. readFile treats lineCount as an upper bound and an unwindowed read of a large file as a window from line 1: whole lines up to the cap, then `[lines 1-620 of 3800; next startLine: 621]`, so a file is never cut mid-line and the agent always knows how to go on. webFetch cuts its body, header included, before formatting, so the frontmatter's `truncated` field stays true to the text.
A scripted model creates a gadget, writes a file, searches it two ways, reads a window of it, writes a file past the cap and reads it back. The test asserts the exact tool results the model received, then runs a second turn and asserts the replayed history shows the same text. The revert-elision scenario also searches the file in the reverted step and asserts the search is elided along with the read.
ec318a6 to
eb94da5
Compare
| let last = first; | ||
| let chars = lines[first - 1].length; | ||
| while (last < limit && chars + 1 + lines[last].length <= budget) { | ||
| chars += 1 + lines[last].length; | ||
| ++last; | ||
| } | ||
| return `${lines.slice(first - 1, last).join("\n")}\n\n${note(last)}`; |
There was a problem hiding this comment.
🟡 Single-line reads bypass result cap
When a selected line exceeds 32K, readFileWindow returns it whole. A valid 512K one-line file can overflow the model context.
Learn more
The window starts with one line before checking the budget. This guarantees progress, but it also lets the largest valid file bypass the new tool-result limit when that file has no newline. The code-change contract permits file text up to 512K UTF-16 units, so this input is valid and can be produced by minified source.
Example: A 400K-character minified client.js contains one line. readFile({filename: "client.js"}) returns all 400K characters plus the range note, rather than at most 32K characters.
Recommended fix: Define an oversized-line representation that remains within MAX_TOOL_RESULT_CHARS, such as a character slice with an explicit continuation marker. Ensure callers can continue within a line, or return an actionable error directing the model to grep rather than emitting the full line.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (chars + line.length + 1 > maxChars) { | ||
| out.push(`(${lines.length - index} more matches not shown; narrow the pattern or path)`); | ||
| break; | ||
| } | ||
| out.push(line); | ||
| chars += line.length + 1; | ||
| } | ||
| if (lines.length === 0) out.push("(no matches)"); | ||
| out.push(...scan.errors.map(error => `(skipped: ${error.error})`)); |
There was a problem hiding this comment.
🟡 No-match grep exceeds result cap
When formatGrep finds no matches but truncates skipped files, (no matches) is omitted from the character budget. The tool result can exceed maxChars by 12 characters, violating the 32K cap.
Was this helpful? React with 👍 or 👎 to provide feedback.
eb94da5 to
7e7043c
Compare
|
Findings
|
|
Finding
|
|
Performance is inconclusive because every cohort is non-comparable due to changed eval definitions. Across 10 trials each, appointment improved from 8/10 to 10/10 but was 7% slower and costlier; expense improved from 4/10 to 5/10 and was 57% faster; project-doc fell from 10/10 to 9/10, was 19% slower, but cost 28% less. Project-doc retained essentially the same five-call flow, while candidate |
grep now scans every workpiece the same way, as an overlay over a base commit: the hook is grepWorkpiece, its base is the one readFile reads from (the pin base while pinned, else a gadget's head or a worktree's accepted commit, or none for a gadget with no committed code), and the in-memory gadget filter is gone. This also fixes grep on a worktree with no changes, which #513 no longer marks as pinned, and on a gadget with no files yet, whose root scope now exists and reports no matches. The blind cap on every tool result is gone. Each tool that can produce more than MAX_TOOL_RESULT_CHARS decides how to stay under it in a way the model can read: readFile already returned a window of whole lines with a continuation note; grep now drops whole matches, then whole skipped-file lines, and one line each says how many were left out, with the notes budgeted so the whole result fits; webFetch already cut its body and said so in the frontmatter. Nothing else is cut, and nothing splices a marker into text whose shape it doesn't know. grep's replay case no longer falls through into webFetch's.
7e7043c to
5dd2674
Compare
|
Finding
|
|
Performance is inconclusive because every cohort is non-comparable due to changed eval definitions. Across 10 trials, appointment stayed 10/10 while 13% faster; expense fell from 7/10 to 6/10 despite 27% fewer tool calls and 19% lower cost; project-doc rose from 9/10 to 10/10 with 28% lower cost. Project-doc kept the same five-call flow, but its large file read was bounded to a line window, reducing context without changing strategy. These mixed shifts may reflect stochastic variance or eval drift rather than the change. |
2 participants
Based on #522; merge that first.
A
readFileorwebFetchresult could be a megabyte of text, and the agent had no way to search a workpiece or read part of a file. This addsgrepand line ranges onreadFile, and has each tool that can produce a large result keep it under about 32K characters in a way the model can read.readFiletakes two optional parameters:startLineis 1-based.lineCountis an upper bound: the window also stops where the next line would cross the cap. A windowed read, or an unwindowed read of a file over the cap, returns whole lines and ends with[lines 1-620 of 3800; next startLine: 621]. Small reads with no window return the file as before.grepis new:patternis a JavaScript regex, matched per line.pathis a file or a directory (searched recursively); omitted means the whole workpiece. Output ispath:line:textper match, likegrep -n, plus a(skipped: ...)line for each file that couldn't be searched. Past the cap it drops whole matches and ends with a line saying how many. It scans every workpiece the same way, as the turn's edits over a base commit: the pin base while pinned, else a gadget's head or a worktree's accepted commit, the same basereadFilereads from. Output is recorded on the tool call and replayed from the record, likewebFetch. Replay elides it if the user reverted the edits it saw, with the check from #522.webFetchcuts its body at the cap and says so in the frontmatter'struncatedfield, as before. Nothing else is cut.AiToolCallinworkshop-sharedgains the tworeadFileinputs and agrepvariant.The shipped
client.jsblueprints are 70K to 150K characters, so any cap the agent can live with needs the search and range tools first.Commits: move the scan (fixing CRLF handling in the moved matcher), add
grep, add ranges, add the cap, add tests, then one commit for the review: one grep path for gadgets and worktrees, per-tool caps in place of a blind one, and no fallthrough in replay. Tests are unit tests for the window and the grep cut, and two scripted-model runs: one for grep, ranges and the cap with a replay check, and #522's revert scenario extended to search as well as read.