Cap tool results at 32K; add grep and line ranges to 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}
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
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. |
d58d5d5 to
fea24f1
Compare
AshishKumar4
changed the title
|
Performance is inconclusive because every cohort is marked non-comparable due to changed eval definitions. Pass rates were unchanged at 0/3, 0/3, and 3/3; durations fell 11%, 5%, and 12%, while project-doc cost fell 44% despite more tool calls. Candidate project-doc runs used grep and targeted line reads instead of full client.js reads. With only three stochastic trials, these shifts may reflect run variance. |
|
Findings
|
|
Performance is inconclusive because all cohorts are non-comparable due to changed eval definitions. Pass rates stayed 0/3, 0/3, and 3/3, while project-doc duration rose 105%, tool calls 92%, and cost 87%; other cohorts were nearly flat. Candidate project-doc runs used grep and targeted line reads of client.js, but required substantially more steps than baseline runs. With only three stochastic trials, the overhead cannot be confidently attributed to the change. |
fea24f1 to
d9b582a
Compare
|
LGTM! |
|
Performance is inconclusive because all cohorts are non-comparable due to changed eval definitions. Across 10 trials each, appointment stayed 10/10 but cost rose 15%; expense fell from 7/10 to 6/10 despite 36% faster runs and 20% more tool calls. Project-doc improved from 8/10 to 9/10, with duration down 7% and cost down 28%; bounded large-file reads reduced context while preserving its five-call flow. With only 10 stochastic trials per cohort, these shifts may reflect variance rather than the change. |
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.
d9b582a to
ec318a6
Compare
| let worktreeBase = worktreePinBases.get(workpieceId); | ||
| if (worktreeBase !== undefined && pinnedGadgets.has(workpieceId)) { | ||
| scan = await hooks.grepWorktree(worktreeTurnAccess, workpieceId, worktreeBase, path); | ||
| } else { | ||
| // The same source readFile reads: committed code at the observed head for an | ||
| // unpinned gadget, else the session content. | ||
| let head = pinnedGadgets.has(workpieceId) ? undefined : observeHead(workpieceId); | ||
| let files = head !== undefined | ||
| ? await hooks.readCommitFiles(head) : sessionContent.get(workpieceId) ?? new Map(); | ||
| scan = scanGadgetForGrep(files, path); |
There was a problem hiding this comment.
🔴 Unpinned worktree searches return empty
When grep targets an unmodified worktree, it skips grepWorktree because only pinned bases qualify. observeHead has no worktree head, so the fallback scans an empty session map.
Learn more
Worktrees begin unpinned and read untouched paths from their accepted commit. worktreePinBases contains only worktrees modified in the current chat, while worktreeBase also falls back to the accepted commit. The current branch therefore excludes every newly created or otherwise unmodified worktree. Since observeHead calls getGadgetHead, the fallback cannot recover worktree files and scans an empty map.
Example: A new worktree contains src/main.js with needle. Calling grep({workpiece: "REPO", pattern: "needle"}) returns (no matches) until any worktree modification pins it.
Recommended fix: Resolve the base with worktreeBase(workpieceId) and use grepWorktree whenever it returns a commit. Keep the gadget fallback only when no worktree base exists.
| let worktreeBase = worktreePinBases.get(workpieceId); | |
| if (worktreeBase !== undefined && pinnedGadgets.has(workpieceId)) { | |
| scan = await hooks.grepWorktree(worktreeTurnAccess, workpieceId, worktreeBase, path); | |
| } else { | |
| // The same source readFile reads: committed code at the observed head for an | |
| // unpinned gadget, else the session content. | |
| let head = pinnedGadgets.has(workpieceId) ? undefined : observeHead(workpieceId); | |
| let files = head !== undefined | |
| ? await hooks.readCommitFiles(head) : sessionContent.get(workpieceId) ?? new Map(); | |
| scan = scanGadgetForGrep(files, path); | |
| 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. | |
| let head = pinnedGadgets.has(workpieceId) ? undefined : observeHead(workpieceId); | |
| let files = head !== undefined | |
| ? await hooks.readCommitFiles(head) : sessionContent.get(workpieceId) ?? new Map(); | |
| scan = scanGadgetForGrep(files, path); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
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.
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.Tool results are now capped at 32K characters. The cap keeps the start and the end and drops the middle, since the end is often the useful part (the exception at the end of an
executeCodelog). It applies to live results and to replayed history. Stored outputs are unchanged, exceptgrep's, which is stored already capped. The shippedclient.jsblueprints are 70K to 150K characters, so the cap is only workable with the two tools below.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. Worktrees use the scan the binding'sgrep()already had, moved togrep.ts. 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.AiToolCallinworkshop-sharedgains the tworeadFileinputs and agrepvariant.Commits: move the scan (fixing CRLF handling in the moved matcher), add
grep, add ranges, add the cap, add tests. Tests are unit tests for the cap and the windows, and two scripted-model runs: one covering grep, ranges and the cap with a replay check, and #522's revert scenario extended to search as well as read.