fix(sandbox): charge each file ceiling once per execution, not once per source - #7288
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR makes sandbox file ceilings apply once per execution rather than independently per source.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/execution/remote-sandbox/index.ts | Combines declared and harvested files under one execution-wide output count and avoids re-harvesting exact declared paths. |
| apps/sim/lib/function-execution/sandbox-mounts.ts | Deduplicates input mounts by storage key while preserving deterministic first-occurrence naming. |
| apps/sim/lib/execution/remote-sandbox/conformance.test.ts | Adds provider conformance tests for combined ceilings and exact-path deduplication. |
| apps/sim/lib/function-execution/sandbox-mounts.test.ts | Adds duplicate-key coverage and corrects fixture keys for tests representing distinct files. |
| apps/sim/executor/variables/resolver.ts | Clarifies that marker reuse limits request variables while mount planning enforces storage-key deduplication. |
Reviews (2): Last reviewed commit: "fix(sandbox): spend each file ceiling on..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Two ceilings on a Function run's sandbox files were charged per source rather than per execution. Mounts: planUserFileMounts assigned a path per element, so one storage key named by two sources became two mounts. `files` is `user-or-llm` and deduped nowhere, so a model repeating an id — or naming a file the code also references with `<block.file.path>` — produced a duplicate that cost a presign, a second transfer of identical bytes, and a second charge against both the byte budget and the 20-file mount ceiling, either of which then refuses a request that fits. Collapse by storage key, first occurrence wins. The contract already requires a non-empty key, so there is no keyless case to carry. Exports: MAX_SANDBOX_OUTPUT_FILES is documented as what one execution may export "whether declared by path or discovered by harvesting", and collectExportedFiles already runs the byte ceiling that way. The count ceiling did not, so a request declaring paths and harvesting a directory could export 20 of each. Count declared and discovered together, with a declared path inside the directory dropped from the discovered set so it is not billed on both sides. With no declared paths — every call execute-request makes, since it sets outputSandboxDir only when nothing declares a sandboxPath — the check and its message are unchanged. The resolver's marker reuse is no longer what keeps a twice-referenced file to one mount; its comment said otherwise. Fixture keys in sandbox-mounts.test.ts were identical across files the tests meant to be distinct; they now differ, which is what those tests always claimed to set up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a9ac0ae to
2f3b704
Compare
|
@cubic review |
@icecrasher321 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Two ceilings on a Function run's sandbox files were charged per source rather than per execution. Both came out of review comments on #7251; the behavior is real in each case, though not for the reason the comments gave.
Mounts — a storage key mounts once
planUserFileMountsassigned a path per array element, so one storage key named by two sources became two mounts.The comment pointed at
collectSandboxFileMountRefsappending a file once per marker position. That half was already handled upstream —resolveSandboxFilePathReferencereuses the marker standing for a givenfile.key, so<block.file.path>twice in one block yields one marker. And the stated consequence, "the key-based path map makes every reference use the last mount", is harmless on its own: both mounts hold identical bytes.What is actually reachable is the other input to the same list.
mountedUserFilescomes from thefilesparam, which isvisibility: 'user-or-llm'and deduped nowhere. A model repeating a file id, or passing a file the code also references with<block.file.path>, produces a real duplicate — costing a presign, a second transfer of identical bytes, and a second charge against both the mount byte budget andMAX_BLOCK_MOUNTED_FILES, either of which then refuses a request that fits.So the fix goes at
planUserFileMountsrather than in the collector: it is the one place the whole mount set is assembled, so it covers the explicit list, the markers, and the overlap between them in a single pass. Keyed on the storage key, since that is the object's identity and already whatmountPathsByKeylooks paths up by. First occurrence wins, so the name listed first is the one the code sees.No keyless branch:
userFileSchemadeclareskey: z.string().min(1), and the resolver drops a file without a key before it can become a marker, so there is no empty-key case to carry.This only ever removes work. Fewer files reach
MAX_BLOCK_MOUNTED_FILESand the byte budget, so it cannot newly refuse anything; the surviving mount is the first rather than the last, which is the un-suffixed name.Exports — one 20-file ceiling per run, not per source
MAX_SANDBOX_OUTPUT_FILESis documented as how many files one execution may export "whether declared by path or discovered by harvesting the output directory".collectExportedFilesalready runs the byte ceiling that way — one running total across both, with a declared path inside the harvest directory dropped from the discovered set rather than billed twice. The count ceiling did not follow:listOutputDirectoryFileschecked the harvest alone, so a request could export 20 declared plus 20 discovered.Declared and discovered are now counted together, and the declared-path filter moved into the listing so count and bytes apply the same rule.
Scope of the behavior change:
execute-requestsetsoutputSandboxDironly when no output declares asandboxPath, so on every call it makes,declaredPathsis empty — the comparison and the error message are exactly what they are today. The copilot doc tools never set the directory at all. The combination is admissible through the contract (outputs.filesalongside the legacy top-leveloutputSandboxPath), which is what makes this worth closing rather than leaving to the next caller, but nothing on a normal path changes.The naive form of this fix would over-refuse: a run whose 20 harvested files include the one declared path would count as 21. A test pins that case.
Tests
planUserFileMounts: one storage key mounts once across sources.sandbox-mounts.test.tswere identical across files the tests meant to be distinct — the collision-suffix and inline-budget tests both passed only because their fixtures shared one key. They now differ, which is what those tests always claimed to set up.bun run test(function-execution, remote-sandbox, executor/variables): 823 passed, 40 skipped.bunx turbo run type-check: 26/26.bun run check:api-validationclean.🤖 Generated with Claude Code