Repository navigation
meta(#532): built-in meta sandbox tools on /api/agent - #535
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Implented per
Gates (agent workspace): typecheck ✅ · di-gate ✅ · full Not merged. CI: Vercel = pending (deploying), Preview Comments = pass → |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #535
Verdict: BLOCK
Repo: btipling/invincible
Scope: main ← 9e774da · 8 files · phase 2 meta_sandbox_* (#532)
Lenses run: L1, L2, L6, L8 (skip L3: no protocol; L4: no workflow; L5: tiny inventory; L7: no UI)
AGENTS.md read: yes · SECURITY.md: tokens never client/Wasm
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Blocker | L1 | Switch does not bind the next turn and is overwritten by the host PUT. meta_sandbox_switch writes meta.activeSandboxId on the envelope and preserves updatedAt. Next /api/agent resolve uses only parsed.sandboxId from the client (route.ts ~415) — it never reads the envelope. The host sends session.activeSandboxId from local state (harnessChat.ts ~983) and on success adopts agentResult.sandboxId, which is the start-of-turn resolve id (runAgent.ts sandboxOut = hasFsTools ? params.sandboxId), not the switched id. Then PUT folds that old id with a newer updatedAt → LWW wipes the tool write. Docs claim the switch “survives even without a host PUT” — true only if the host never PUTs. It always does. |
User: “switch to staging.” Tool: switched active sandbox to id=stg. This turn still uses prod FS. After persist, local + Redis are prod again. Next turn still prod. |
(1) Route resolve must treat envelope meta.activeSandboxId as requestedSandboxId (same as persona snapshot). (2) Surface the new id on the JSON/SSE result (skill_attached-style) so the host adopts it before PUT. (3) Check upsertEnvelope status (below). |
high |
| Major | L1 | upsertEnvelope conflict is ignored. Result is {status:'stored'|'conflict'}; switch always returns switched…. Same updatedAt write loses to a concurrent host PUT (input.updatedAt < existing.updatedAt). Model is told it switched; store unchanged. |
Mid-turn host PUT (cwd / attachedSkills). Switch reports success. Next resolve still old. | if (result.status !== 'stored') return ERROR … (no write). |
high |
| Major | L1 | The case switch exists for never reaches the tool. SANDBOX_SELECTION_REQUIRED (multiple usable, no preferred / stale active) is ok: false without softContinue. Route hard-returns 403 before runAgent unless BUILTIN_HTTP_FETCH is on. meta_* are excluded from nonSkillToolCount on purpose. |
User with two grants, no preferred: every agent turn 403s. Cannot meta_sandbox_switch. Feature dead unless HTTP is enabled. |
Soft-continue (or run with meta tools only) on selection-required so switch can persist, then next turn resolves. UI-only select is not this PR’s job if the addendum says the agent can switch. | high |
| Nit | L2 | Always-on switch: prompt injection can retarget a sandbox the user already has a grant on (read-only session → writable). Same product accept as #533; lower blast radius than delete-all-skills. | “Summarize https://evil” → meta_sandbox_switch to the write grant. |
Residual / #534-class visibility. Not the merge gate. | med |
Checked and not findings:
- L2 IDOR / secrets:
userId/sessionIdclosed over; foreign id not inlistUserSandboxChoices→ no write.projectSandboxOptionhas nobase_url/ token. Key is{tenantId, caller userId, caller sessionId}. Redis-safe id checked. Tests cover unusable / foreign / no sessionId / non-envelope store. - L6: DI +
sessionStoreSeam; no in-body I/O.isMetaToolNamestill excludesmeta_sandbox_*from the workspace 403 mask (correct, but it causes the selection-required hard-403 above).
Residual risk
Stale logicalCwd after a bind change (docs mention fail-closed abs paths). List unbounded (sandbox count is small). Switch this turn does not rebuild FS tools (documented subsequent-turn).
Merge guidance
Do not merge until a switched id is what resolve and the host session see on the next turn (envelope read + result fold), and selection-required can reach the tool. Conflict must fail closed.
What was not attacked
Live Redis + host PUT after a real switch; two-tenant grant row in a real DB.
Block-response plan — sandbox bind switch must actually persist, honor store conflicts, and be reachable for selectionStatus: DONE (all increments landed; committed to this PR branch) — living comment; amended as I iterate. Increments 1 (B2), 2 (A1/A2 wire), 3 (A1 resolve seed + host fold), and 4 (B3 — selection reachability) are DONE and committed; docs updated. Decisions resolved (you approved my recommendations):
The block (from adversarial review) — three coupled correctness bugs, not scope/wont-fix
Strategy — A (server-authoritative envelope bind; approved)
Change set (all landed)
Test plan (all landed)
Progress log
Gates & notes
Living comment. |
…retry, fail-closed no false success) Adversarial-review block B2: the tool discarded the upsertEnvelope result, so a store-rejected write (concurrent host PUT bumping updatedAt) was reported as a false "switched" success. Now retryPersistActiveSandbox preserves the stored updatedAt, re-reads + retries ONCE on a conflict, and returns true ONLY for a stored write; a still-conflicting retry fails closed with an honest error. Tests: +2 (concurrent-bump recover via bound retry; persistent conflict honest error). metaSandboxTools.test.ts 16 -> 18 passing.
…locker B1 A2 wire)
…ost-turn effective bind (blocker B1 A1)
…ls (blocker B3 reachability)
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #535 (re-run)
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: a3a345c on plan/meta-sandbox-tools
Lenses run: L1, L2, L6, L8
AGENTS.md read: yes
Prior BLOCK
| Prior | Status |
|---|---|
B1 Switch lost: resolve ignored envelope; host folded pre-turn sandboxId; PUT LWW-wiped |
Fixed. Route seeds requestedSandboxId from envelope; envelope wins over body when sessionId is set (route.ts). runAgent emits post-turn activeSandboxId from a successful meta_sandbox_switch result (typed parse, not the truncated summary). Host folds activeSandboxId ?? sandboxId (harnessChat.ts). Tests: route B1 A1 + envelope-wins; harness JSON/stream fold. |
B2 upsertEnvelope conflict reported as switched |
Fixed. retryPersistActiveSandbox: honor status, retry once on conflict, true only for stored. Tests for recover + honest fail. |
B3 SANDBOX_SELECTION_REQUIRED hard-403 before tools |
Fixed. selectionRequired on resolve; route soft-paths when meta_sandbox_* present. Hard SANDBOX_FORBIDDEN (no usable grant) still 403. Route test: 200 + tools vs still-403 forbidden. |
| Nit L2 always-on switch | Unchanged product accept (same class as #533). |
Findings (this pass)
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Nit | L1 | Envelope bind always wins over the body when sessionId is present. A host UI change that has not yet PUT the envelope is ignored on the next /api/agent. |
Operator picks another sandbox in chrome, sends immediately, no PUT yet → still old bind. | Host already PUTs session mutations first in the normal path. Document “PUT then POST.” | med |
| Nit | L8 | docs/session-model.md table still says the host applies the server’s resolved sandboxId on success. The wire is now activeSandboxId (switch target). |
Next implementer reverts the fold. | One-line doc fix. | med |
Checked and not re-opened:
- IDOR / no
base_url/ token: unchanged. - Switch-id parse:
^switched active sandbox to id=(\S+)\s+tools=;ERRORignored; host re-checks Redis-safe. - Workspace-not-running still does not treat meta tools as a substitute (
metaSelectionDeferredis selection-only). - This turn still does not rebuild FS tools after switch (next turn + envelope seed). Documented.
Residual risk
Always-on meta_sandbox_switch (prompt injection can retarget a grant the user already has). One conflict retry can still lose to two rapid host PUTs (honest error). Stale logicalCwd after bind change.
Merge guidance
OK to merge for #532. Optional doc + “PUT then POST” note.
What was not attacked
Live Redis + host PUT after a real switch on two devices.
…inding precedence
meta(#532): built-in meta sandbox tools on
/api/agentPhase 2 of the built-in meta tool surface (#332, parent #530): first-party sandbox bind tools — non-secret inventory, current-bind surface, and a mid-session switch that persists
meta.activeSandboxIdvia the existing session envelope seam. Built on the stable sandbox API shipped by #330.Closes #532 · Refs #530 (parent epic) · Refs #332 (product)
What ships
lib/agent/metaSandboxTools.ts—createMetaSandboxTools({userId, sessionId, userPreferredSandbox, sessionStoreSeam}), always-on in-process AI SDK tools:meta_sandbox_list— non-secret projection mirroringGET /api/sandboxes.options(id/name/slug/backend/status/image/canRead/canWrite/usable/granted). Neverbase_url/token.meta_sandbox_active— the persistedmeta.activeSandboxIdwhen it is a usable grant, else null, + itsdescribeSandboxToolssurface (mirrors the route'sactive). A set-but-unusable id is reported honestly, fail-closed.meta_sandbox_switch— validate Redis-safe + usable-grant to the caller; persistmeta.activeSandboxIdon the caller's own session via the envelope seam (resolveSessionStore→isEnvelopeStore→readEnvelope/upsertEnvelope,updatedAtpreserved likeattachedSkills). Fail-closed, no partial write on a non-Redis-safe id, unusable/ungranted/wrong-tenant grant, missingsessionId, or an unavailable / non-envelope store.app/api/agent/route.ts— assemble intoextraToolsalways; seam closed over the existingresolveSessionStore()+services.harnessSessionsRedis.resolveTenantIdForUser. Covered by themeta_*soft-path guard (isMetaToolName).lib/agent/runAgent.ts— appendMETA_SANDBOX_SYSTEM_ADDENDUMwhen ameta_sandbox_*tool is present.docs/mcp.md,docs/sandbox.md,docs/session-model.md,AGENTS.mdrow. Cloud ops N/A (no Production mutate, no GHA).Tests
New
lib/agent/metaSandboxTools.test.ts— 16 cases (store-fakes mirroring themetaTools.test.tspattern), covering: non-secret projection (nobase_url/token), active-descriptor resolution incl. set-but-unusable honesty, switch success (persistsactiveSandboxId, preservesupdatedAt+ stored fields, fresh-envelope create), and the full fail-closed matrix (unusable/ungranted/wrong-tenant/foreign id, missingsessionId, store unavailable, non-envelopeisEnvelopeStoreguard, non-Redis-safe id) with no partial write assertions.Gates
npm run typechecknode scripts/di-gate.mjsnode_modules/vitest/vitest.mjs runnpm run buildpublic/harnesspresent)Not merged. Ready for review.