feat(agent): read_file line offset + limit window - #690
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Implement-plan for #689. Window lives only in
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #690
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/read-file-line-window · 7 files · read_file line window + edit-grant predicate
Lenses run: L1, L5, L6, L8 (skip: L2 no new trust boundary / finalize redaction unchanged; L3 no DOM/Wasm chat; L4 no CI/wasm; L7 no deploy-host bind; L9 no UI)
AGENTS.md read: yes
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | lib/agent/tools.ts · read_file always recordRead({ truncated: true }) on a non-full window. createRunFileFreshness.recordRead is last-write-wins, so a later peek revokes a valid full-file grant. Tool description + docs/sandbox.md say a full read authorizes edits until the on-disk file changes / is required in this agent run — not “until the next windowed read.” No test covers full → window → edit. |
1. Default-read an 800-line file (offset=1 limit=1000 → grant, no (truncated)). 2. Peek with offset=400 limit=40 (the feature this PR adds). 3. str_replace / overwrite → truncated read_file is not enough. File did not change. |
Defender: last-observation is the existing ledger; a truncated view should not authorize edit; error is recoverable. Fails: (1) this PR’s own contract text is “until disk changes,” not “last read must be full”; (2) windowed peek is the new product path, so full-then-zoom-then-edit is the likely medium-file workflow; (3) the model already has the full file in context; (4) plan locked recordRead(truncated) without carving out “do not downgrade a fresh grant.” |
high |
| Minor | L8 | DEFAULT_AGENT_SYSTEM (lib/agent/runAgent.ts) still says “Must read_file a path in this agent run before str_replace or overwriting.” After the default-1000 behavior change, that sentence is false for any file whose split('\n') length is > 1000. Standing orders never mention offset / limit / “full window.” |
Model follows the system prompt on a 1400-line file, default-reads, then edits. Status is lines=1000/1400 (truncated); edit denied. Re-read with the same defaults (prompt says “read_file again”) loops. |
Defender: read_file tool description + status + edit-gate error all state the full-window rule. Fails as a complete defense: the standing order is injected every FS turn and still describes the pre-#689 rule. Tool schema is a long paragraph the model can ignore in favor of the short system line. |
high |
| Minor | L1 + L6 | Exact-1000 grant test is a non-POSIX fixture. nLines(1000) is L1\n…\nL1000 with no trailing newline, so split('\n').length === 1000 and the test grants. A real 1000-line source file ends in \n → totalLines === 1001 → default read is lines=1000/1001 (truncated) and denies. DoD / plan table “exactly 1000-line file grants” is unproven for editor/wc -l files. |
Write 1000 newline-terminated lines (normal wc -l = 1000). Default read_file. Status lines=1000/1001 (truncated). str_replace denied. Bump limit to 1001 to grant. |
Defender: plan locked split('\n') trailing '' as a line; status is honest. Fails as a test of the claimed cap boundary: the only exact-1000 case uses a fixture real files do not match. |
high |
Residual risk
Windowing still happens after a byte-0 maxBytes prefix (16 MiB). A deep offset past that prefix is empty + truncated — accepted non-goal. limit has no ceiling; formatLineWindow will number every line in the prefix (worst case ~16M \ns) before finalize clips to 2M chars — working-set spike, not a payload leak. truncateForModel can still clip a granted body on a few huge lines (pre-existing 2M vs 16 MiB hole); the new lines=N/N status can then claim completeness. Full vitest run was not executed here (author: --changed only). Live sandbox / daemon not exercised.
Merge guidance
- CONCERNS: do not merge until the Major is fixed or explicitly accepted.
- Fix path for the Major: do not let a windowed
recordRead({ truncated: true })downgrade an existing fresh grant when the fingerprint is unchanged (or stop recording truncated when a prior full grant exists). Update the tool description /docs/sandbox.mdto match whichever rule you keep. Addfull → offset>1 → str_replaceas a regression test. - Minors: teach
DEFAULT_AGENT_SYSTEMthe full-window rule; add a trailing-\n1000-line grant/deny case.
What was not attacked
Live DO runner, prod Gateway, BYO daemon /v1/read_file over the network, Vercel Sandbox SDK path, Wasm/bridge paint of L2 numbered previews, full vitest suite, in-process agent loop with a real model.
Plan to address feedback on PR #690 (Status: DONE)Last updated: 2026-08-19 Feedback items (most complex → least)
#1 —
|
… now teaches full-window rule; add POSIX trailing-newline grant/deny tests
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #690 (round 2)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/read-file-line-window @ e49c201 (fixup on 2631e65)
Lenses run: L1, L5, L6, L8 (skip L2: no new trust boundary; L3: no DOM/Wasm chat; L4: no CI/wasm; L7: no deploy bind; L9: no UI)
AGENTS.md read: yes
Prior: CONCERNS on 2631e65 — peek revoked grant (Major); stale DEFAULT_AGENT_SYSTEM (Minor); POSIX 1000-line fixture (Minor)
Findings
No surviving Major/Blocker.
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L6 | DEFAULT_AGENT_SYSTEM standing-order test (runAgent.test.ts) still only locks “read_file a path” / “read again” / create exception. The new offset=1 / full-window clause can be deleted and that describe stays green. |
Revert the parenthetical in runAgent.ts. vitest still passes. |
Tool description remains the real contract; this is lock-rot, not a behavior hole. | high |
Closed from round 1
| Was | Now |
|---|---|
Major: recordRead({truncated:true}) last-write-wins revokes a full grant |
fileFreshness.ts skips truncated write when grants.has(path). Full read still upgrades. Tools test: full → offset=40 peek → str_replace ok. Gate 2 still stale-checks live mtime/size at edit. |
| Minor: standing orders described the pre-1000 rule | DEFAULT_AGENT_SYSTEM now requires offset 1 covering every line, not clipped by limit/maxBytes. |
| Minor: exact-1000 grant used a non-POSIX fixture | Kept; added trailing-\n → lines=1000/1001 (truncated) deny, and limit=1001 grant. |
Residual risk
truncateForModel (2M chars) can still clip a granted body on a few huge lines; the new lines=N/N status then claims completeness (pre-existing 2M vs 16 MiB hole). limit has no ceiling — formatLineWindow will number every line in the maxBytes prefix before finalize clips. totalLines is lines in the backend prefix, not a disk recount (locked). The standing-order parenthetical hangs grammatically off write_file; models still have the read_file schema. Full vitest run not executed here (author: --changed 168). Live sandbox / daemon not exercised.
Merge guidance
PASS WITH NOTES — safe to merge from this attack. The Nit is optional (runAgent.test.ts should match /offset=1/ / limit / maxBytes if you want the standing-order fix to stay locked).
Do not treat this as --approve unless you ask for an approve after PASS.
What was not attacked
Live sandbox /v1/read_file, Vercel Sandbox SDK path, in-process agent loop with a real model, full vitest suite, Wasm L2 preview of numbered bodies.
|
R2 Nit on
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #690 (round 3)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/read-file-line-window @ d2ed092 (standing-order lock on e49c201)
Lenses run: L1, L5, L6, L8 (skip L2: no new trust boundary; finalize redaction still wraps the numbered body. skip L3: no DOM/Wasm chat. skip L4: no CI/wasm. skip L7: no deploy bind. skip L9: no UI)
AGENTS.md read: yes
Prior: R1 CONCERNS on 2631e65 (peek revoked grant) · R2 PASS WITH NOTES on e49c201 (standing-order test did not lock the new clause)
Findings
No surviving Blocker / Major / Minor after self-refutation.
R2’s Nit is closed: runAgent.test.ts now requires /offset=1/, /limit/, /maxBytes/. Deleting the parenthetical fails the describe.
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| — | — | — | — | — | — |
Closed from rounds 1–2
| Was | Now |
|---|---|
R1 Major: windowed recordRead({truncated:true}) last-write-wins revoked a full grant |
fileFreshness.ts skips truncated when grants.has(path). Tools test: full → offset=40 peek → str_replace ok. Gate 2 still stale-checks live mtime/size. |
R1 Minor: DEFAULT_AGENT_SYSTEM described the pre-1000 rule |
Standing order requires offset 1 covering every line, not clipped by limit/maxBytes. Parenthetical hangs off read_file. |
| R1 Minor: exact-1000 grant used a non-POSIX fixture | Kept (split semantics); added trailing-\n → 1000/1001 (truncated) deny and limit=1001 grant. |
| R2 Nit: standing-order test still only locked “read_file a path” | Locks offset=1 / limit / maxBytes. |
Residual risk
Attack did not breach a runtime path with current evidence.
- Strategy:
totalLinesis lines in themaxBytesprefix (split('\\n')), not a disk recount — locked. A POSIX trailing newline on an otherwise-1000-line file still needslimit=1001to grant. truncateForModel(2M chars) can still clip a granted body on a few huge lines;lines=N/Nthen claims completeness (pre-existing 2M vs 16 MiB hole).limithas no ceiling —formatLineWindowwill number every line in the prefix before finalize clips. Deepoffsetstill scans from byte 0 (non-goal).- Never-downgrade after a stale fingerprint: a default 1000-line re-read on a longer file does not refresh the grant; the model must pass a limit that covers every line (same rule as first grant). Error stays
stalerather than flipping totruncated. fileFreshness.test.tsdoes not itself lock never-downgrade (the tools execute path does).- Full
vitest runnot executed here (author:--changed). Live sandbox / daemon not exercised.
Merge guidance
PASS WITH NOTES — safe to merge from this attack. Residual items are accepted ceilings / test-layer optional, not merge gates.
Do not treat this as --approve.
What was not attacked
Live BYO /v1/read_file, Vercel Sandbox SDK path, in-process agent loop with a real model, full vitest suite, Wasm L2 preview of numbered bodies, di-gate.mjs.
Summary
Implements plan #689: Grok Build-style line
offset+limiton the existingread_filetool. No second tool. No sandbox client / daemon signature change.Closes #689
Refs #688
Behavior
offsetlimitStatus:
read_file path offset=N limit=N lines=returned/totalInContent (truncated)?:Body is
N→line. Default 1000 is a behavior change vs today’s full-file read.Edit grant only when
offset === 1and the window reached EOF of the backend content andmaxBytesdid not clip.offset > 1never grants, even if the window hits EOF.What changed (7 files)
lib/agent/tools.tsformatLineWindow+ grant predicate + status linelib/agent/agentStream.tssalientToolBitsregex accepts the new prefix; L1 stillpath · N lines · M Blib/agent/tools.test.tslib/agent/agentStream.test.tsdocs/sandbox.mddocs/harness-limits.mddocs/agent-stream.mdGates
npm run typecheckvitest run --changedtools.test.tsagentStream.test.tsvitest runbuild-harnessCaps
limitoffsetminmaxBytes