fix(chat): stop classifying secret-free binary sandbox exports as unknown - #6349
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Copilot function execution only attaches the mounted-files provenance bundle when every sandbox mount came from this resolver. Pre-existing VFS reads clamp Also fixes an embeddings import ( Reviewed by Cursor Bugbot for commit 287995f. Configure here. |
icecrasher321
changed the title
Greptile SummaryThe PR allows secret-free binary sandbox exports to receive exact-empty provenance while retaining unknown provenance whenever mounted secrets cannot be ruled out.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/function/execute/route.ts | Classifies binary exports as exact-empty only when environment and mounted-file provenance establish that no cataloged secret material was in scope. |
| apps/sim/lib/copilot/tools/handlers/function-execute.ts | Couples resolved mounts with a whole-set provenance envelope and deliberately omits it when preserved mounts cannot be attested. |
| apps/sim/lib/execution/mounted-file-secret-provenance.ts | Exposes envelope-level secret presence independently from whether entries produce scannable plaintext. |
| apps/sim/lib/copilot/tools/handlers/vfs.ts | Applies line windows using actual content length while preserving attachment labels. |
| apps/sim/lib/embeddings/client.ts | Updates the utility import to the established helpers subpath. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Sandbox execution exports binary file] --> B{Environment secrets in scope?}
B -->|Yes| U[Persist unknown provenance]
B -->|No| C{Mounted files present?}
C -->|No| E[Persist exact-empty provenance]
C -->|Yes| D{Complete mount envelope available?}
D -->|No| U
D -->|Yes| F{Envelope contains secret entries?}
F -->|Yes| U
F -->|No| E
Reviews (7): Last reviewed commit: "Revert "fix(execution): count the runtim..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ce envelope The binary classifier read an absent mounted-file scanner as "no mounted secrets". That is absence of evidence, not evidence of absence: the request contract permits _sandboxFiles without the provenance envelope, so a caller that mounts secret-bearing bytes and omits the envelope would have a derived binary persisted as provably secret-free. Not reachable today — the route is internal-JWT-only and its one file-mounting caller always emits the envelope — but the classification rested on an invariant nothing enforced. - the copilot handler emits the envelope on the same condition that produces the mount, so tables ship one too and the two cannot drift apart - a mount with no verified scanner now counts as secret material in scope, so the classification is never stronger than what the caller attested to Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
356b79e to
59afafa
Compare
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…nknown Two ways the envelope could read as stronger evidence than it was. The copilot handler preserved `_sandboxFiles` that arrived on the params and then exported provenance from `mountedRegistry`, which knows only about the files it resolved itself. The route would have read that partial envelope as a complete attestation over every mounted byte. The envelope now covers the whole mounted set or is not emitted at all, and a mount with no envelope already fails closed. `hasSecrets` was derived from whether entries produced scannable literals, so an envelope listing entries that all failed to decrypt reported false and let a derived binary be marked exact-empty. It now reflects what the envelope attested to: entries that yield no plaintext make the mount less classifiable, not more. Neither was reachable — `_sandboxFiles` is absent from the copilot tool schema, so nothing can populate the preserved-mount branch — but both had the classification resting on a property nothing enforced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ca769. Configure here.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14ca769. Configure here.
`bun run tool-metadata:check` fails on origin/staging as well as here, so this is not from this branch — #6317 landed the artifact generated from a factory that still built a per-provider apiKey description, and the source was later genericized without regenerating. Regenerating changes exactly the five embeddings entries' apiKey description to the text `tools/embeddings/factory.ts:74` actually produces. The per-provider strings appear nowhere in source. Included here only because the gate is red on every branch cut from staging until someone lands it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ddc1c1a. Configure here.
An execution with no mounted files and no env secret still carries `params` and `contextVariables` into the sandbox — the runtime payload is serialized into a private-input file, so resolved block outputs and workflow variables land as plaintext regardless of `_sandboxFiles`. The scope predicate only looked at mounts and env secrets, so a binary derived from them was classified exact-empty. The route has no catalog for those values and cannot tell a secret-bearing one from an ordinary one, so they count as in scope. Only an execution with nothing at all in scope earns an exact-empty binary. This narrows where the relaxation applies rather than regressing anything: every binary export was unknown before this branch, so a workflow Function block carrying block references keeps exactly the behavior it has today. The mothership path is unaffected — its tool sets no contextVariables, blockData, or workflowVariables, which is the case this branch exists to fix. Values, not keys, for the params check: `executionParams._context` is set to undefined before the context is built, so a key count reads every execution as carrying params. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 754e37c. Configure here.
…in scope" This reverts commit 754e37c. The classifier's secret catalog is the Secrets feature and nothing else: `outputSecretNamesByScanLiteral` and `outputSecretPlaintextsByName` are built only from `envVars`, and mounted-file entries trace back to the same place. `contextVariables`, `blockData`, and `workflowVariables` are ordinary workflow data — resolved block outputs the user already sees in logs — and the text export path does not scan them either. Treating their mere presence as secret material was a heuristic, not a security property, and it created exactly the asymmetry rejected two rounds earlier: a binary derived from a context variable would be `unknown` while a text export of the same bytes stays exact-empty. Stricter than the text path for the same content is not a boundary. It was also nearly inert. `scopeEnvironmentVariables` returns every workspace secret when scope is `all` (the default), so any workflow Function block with secrets configured already trips the env branch. The only slice it changed was executions with no env vars at all, where the workspace has no secret for a context variable to carry. A Secret resolved into an upstream block's output and arriving here through blockData is a real gap, but it is pre-existing, identical for text exports, and belongs at the executor -> route boundary as a provenance envelope for params — not as a presence check in this classifier. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 287995f. Configure here.
1 participant
Summary
Binary outputs from sandboxes need to be trusted in the right manner.
Type of Change
Testing
Tested manually
Checklist