Bound tool results, add grep and windowed readFile - #494
AshishKumar4 wants to merge 5 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}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 4 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| // least one line is always included; a single over-cap line is left to the generic bound. | ||
| let budget = MAX_TOOL_RESULT_CHARS - note(lines.length - 1).length - 2; | ||
| let chars = lines[first - 1].length; |
There was a problem hiding this comment.
🟡 Long lines cannot be continued
When one line exceeds 32K characters, readFileWindow always sends its prefix through the generic bound. startLine selects only whole lines, so no read can expose the remainder.
Learn more
The implementation guarantees at least one complete source line before the generic bound runs. A source line larger than the result cap therefore gets character-truncated, but the API offers only line-based offsets. Grep has the same limitation because a matching result contains the full source line before generic truncation.
Example: A minified client.js consists of one 100 KiB line. Reading line 1 always returns its first 32 KiB. Reading line 2 fails as past EOF, so the final 68 KiB is inaccessible to the agent.
Recommended fix: Add a character or column continuation for over-cap individual lines, or return numbered chunks that can resume within a line. Ensure the continuation metadata itself fits inside the cap.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Known limit, left as is: a single line longer than the cap is cut by the generic bound, and the doc says so. Minified one-line files aren't something the agent can work with line by line anyway; a column offset can come when there's a real need.
![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.
| } | ||
| markFileRead(resolved.workpieceId, filename, head); | ||
| return toolResult(fileContent, {observedCommit: head}); | ||
| return toolResult(readFileWindow(fileContent, window), {observedCommit: head}); |
There was a problem hiding this comment.
readFileWindow() can throw for an out-of-range startLine, but this branch marks the file as read immediately before calling it (and the session-content branch does the same). The failed read therefore leaves filesRead populated, so a subsequent editFile bypasses the read-before-edit gate without the model ever seeing file content. Render the window successfully before calling markFileRead().
There was a problem hiding this comment.
Agreed. Both readFile paths now render the window before markFileRead, so a failed read leaves the gate closed.
| let output = formatGrep(scan, re); | ||
| return toolResult(output, {output} as Partial<AiToolCall>); | ||
| } catch (error) { | ||
| toolCallNotes.set(toolCallId, {error: toolErrorText(error)}); |
There was a problem hiding this comment.
Failure results bypass the new bound because this catch records and rethrows the raw error rather than returning through toolResult(). For example, new RegExp("[" + "a".repeat(40000)) produces a roughly 40K SyntaxError; pi sends that full error to the live model and persists it, while replay later applies boundToolResultText(). This violates the cap and makes live/replayed history differ. The central failure-result path also needs to apply the bound.
There was a problem hiding this comment.
Agreed. The live bound moved from toolResult() to pi's afterToolCall, which sees thrown errors after pi has rendered them as text, so error results are bounded the same way live and on replay.
| let result = await webFetchImpl(hooks.getWebFetchEnv(), {url, raw}); | ||
| // Cut the body here, not in the generic bound, so the frontmatter's `truncated` stays | ||
| // true to the text and the recorded output is what the model saw. | ||
| if (result.body.length > MAX_TOOL_RESULT_CHARS) { |
There was a problem hiding this comment.
The cap is applied to the body before the frontmatter is added, so a body at or just below 32K is subsequently cut by toolResult() even though this condition leaves truncated: false. The model then sees truncated content with contradictory frontmatter. Reserve space for the formatted header (including the final URL) when slicing the body, and set truncated whenever that final formatting requires a cut.
There was a problem hiding this comment.
Agreed. webFetch now formats first and cuts the body by the overflow of the formatted whole, so the header counts and truncated is set exactly when a cut happened.
|
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. |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| case "grep": | ||
| // A search over content the user later reverted would replay as current-looking | ||
| // source; elide it the way a reverted readFile is. | ||
| if (chatMessageStatus.get(msg.sequence) === "reverted") { |
There was a problem hiding this comment.
🟡 Same-step grep survives reverted edits
When grep follows an edit in one tool batch, reverting that batch leaves its search output visible. The tool-call message precedes the changes message, so chatMessageStatus never marks it reverted. Replay presents matches from code that no longer exists.
Learn more
An agent step stores its assistant message, including every tool call, before storing the step's changes message. This ordering is enforced by commitAgentStep. Sequential tool execution lets an edit update session content and a later grep in that same assistant message search the updated content. Reverting starts at the changes message, so the preceding assistant message remains outside the reverted range. Checking only the assistant message's status therefore misses the exact search output produced from that reverted edit.
Example: One tool batch writes secret = 42 and then runs grep for secret. The step stores the tool-call message at sequence 10 and its changes at sequence 11. Reverting from sequence 11 removes the edit, but replay retains the sequence-10 grep match and presents secret = 42 as source.
Recommended fix: During replay, associate each persisted assistant step with its following changes message. Elide grep when either the tool-call message or that associated changes message is reverted. Preserve the existing check for searches performed in later messages inside a reverted range.
Was this helpful? React with 👍 or 👎 to provide feedback.
| */ | ||
| export function matchLines(text: string, pattern: RegExp): { line: number, text: string }[] { | ||
| let re = new RegExp(pattern.source, pattern.flags); | ||
| let lines = text.split("\n"); |
There was a problem hiding this comment.
CRLF files keep the carriage return in every value passed to the regex, so end-anchored searches fail. Repro: write notes.txt as "alpha\r\nneedle\r\n", then call the new agent grep with pattern: "needle$" and that file path; this returns (no matches) because the tested line is actually "needle\r". The added tests use only LF input, so they do not catch this. Strip one terminal \r from each line produced by this \n split (and cover both matching and rendered output for CRLF).
|
@AshishKumar4 Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
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.
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.
8bf39cb to
70b3e4e
Compare
|
Bonk's last run timed out before it could post its one finding, but the finding was right: |
1 participant
A single
readFileorwebFetchcould put up to a mebibyte of text into the model's context; with no way to search a workpiece or read part of a file, reading whole was also the only option. This lands the cap together with the two tools that make a cap workable, so it never ships without its recovery path.What the model sees of one tool result is now capped at 32K characters (about 8k tokens), in one place:
boundToolResultText, applied in pi'safterToolCall(so thrown errors are bounded too), at the replay of recorded results, and at the syntheticobserveUserChangesresult a user's diff becomes. It keeps the head and the tail and notes the elided middle,[... 380112 of 412880 characters elided ...], since the end of a result often carries what matters most, such as the exception at the end of anexecuteCodelog. Recorded outputs are unchanged, exceptgrep's, which is recorded already bounded.webFetchcuts its body, header included, before formatting so the frontmatter'struncatedfield stays true.readFiletakes optionalstartLineandlineCount. A windowed read, and an unwindowed read of a file over the cap, returns whole lines and ends with[lines 1-620 of 3800; next startLine: 621];lineCountis an upper bound, so the note always says where to continue. Small unwindowed reads return the file verbatim as before, so existing behavior and tests hold. Edits still anchor on text, so theeditFilegate is unchanged.grep(workpiece, pattern, path?)searches a gadget or a worktree with a JavaScript regular expression and returnspath:line:text. Worktrees go through the scan the binding'sgrep()already used, moved togrep.tsand reached through one new hook; a gadget's files are in hand, so that half is a filter. Output is recorded on the tool call and replayed as recorded, likewebFetch, elided if the user reverted the batch, and stripped from client deliveries: for a worktree it is repository content.Why 32K: the shipped
client.jsblueprints run 70K to 150K characters, so any cap the agent can live with needs windows and search first. Kenton's scratch-worktree spill for large results can point at the same continuation line and windows when it lands.Commits are in dependency order and reviewable alone: the pure extraction, the tool, the windows, then the cap.
Tests: boundary tests for the bound and the windows, and one scripted-model integration test that creates a gadget, writes a file, searches it two ways, reads a window, writes a file past the cap and reads it back, then replays the turn and asserts the model sees the same text. Backend suite 50 files, integration agent tests, frontend build and lint clean.