feat(agent): per-session scratch directory - #919
Merged
Conversation
Reserve a per-session, ephemeral OS-temp scratch dir the agent reaches through the shell — its working area and the default home for produced / intermediate files, distinct from the user's workspace (WG scratch.md, RFD #916). Deliberately minimal and tool-agnostic; the deferred image/video gen tool is a future consumer, not a dependency. - session/scratch.ts: pure derivation (scratchRootFor / assertOutsideSecretsRoot) + thin I/O (ensure/remove/sweep) + defaultScratchBase. Per-session isolation (S1) and "outside the GRIDA-SEC-004 secret root" (S4) are package-owned invariants; the base location is host-injected (filesystem location is host-owned I/O), defaulted at the server boundary to <os.tmpdir()>/grida-agent. - shell: additionalAllowedRoots lets a cwd sit inside scratch though it is NOT a workspace (S5). Reach rides the existing run_command path — no routing fs backend, no /scratch namespace. - runtime/host: derive + ensure scratch per turn (gated on shell + workspace), thread scratch_base; startup sweep + per-delete cleanup bound the lifetime (S2). - prompt: scratch_capability tells the agent its path, that it is ephemeral, and that keeping a file means promoting it into the workspace. Tests pin the contract by name (S1/S2/S4): scratch derivation + I/O, the shell allowed-root gate, capability-hint gating, and a REAL-shell integration proving extract-into-scratch and promotion end-to-end. Refs #916, #912
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds host-managed per-session scratch directories, threads scratch paths through runtime and shell validation, and exposes a scratch capability hint when command execution is available. Session cleanup and server startup now reclaim scratch state, and tests cover the new scratch path and allowlisting behavior. ChangesPer-session scratch directory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 820b2c8e8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/grida-ai-agent/src/test/browser-harness.global.ts (1)
46-57: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTeardown no longer removes the new scratch temp dirs.
These hosts now use
${harnessDir}-scratchand${foreignDir}-scratch, butcleanup()still only deletesharnessDirandforeignDir. Any browser test that allocates scratch will leak temp directories across runs.Suggested fix
const cleanup = async () => { await harness.stop(); await foreign.stop(); await fs.rm(harnessDir, { recursive: true, force: true }); await fs.rm(foreignDir, { recursive: true, force: true }); + await fs.rm(`${harnessDir}-scratch`, { recursive: true, force: true }); + await fs.rm(`${foreignDir}-scratch`, { recursive: true, force: true }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/grida-ai-agent/src/test/browser-harness.global.ts` around lines 46 - 57, The browser harness teardown is leaving behind the new scratch temp directories created for the AgentHost instances. Update the cleanup logic in browser-harness.global.ts so the teardown removes both the main user data dirs and the associated scratch dirs for the harness and foreign hosts. Use the existing AgentHost setup symbols and the cleanup() routine to locate the matching temp paths, and ensure the scratch bases derived from harnessDir and foreignDir are included in deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/grida-ai-agent/src/http/server.ts`:
- Around line 195-200: The default scratch base in server startup is shared
across hosts, so a second process can sweep another host’s session directories.
Update the scratch base selection in server.ts around defaultScratchBase() and
sweepScratch() so the fallback path is namespaced per host before cleanup, using
host-specific information from the server options or environment. Keep the sweep
behavior the same, but ensure each host gets its own isolated base directory
before invoking sweepScratch().
In `@packages/grida-ai-agent/src/session/scratch.ts`:
- Around line 83-95: The containment check in assertOutsideSecretsRoot is only
lexical and can be bypassed by symlinked scratch bases. Update
assertOutsideSecretsRoot and the ensureScratch flow to validate the physical
location using realpaths (or perform the authoritative check during the I/O path
after directory creation) so a scratchRoot that resolves inside secretsRoot is
rejected even when the input path appears outside. Use the existing
assertOutsideSecretsRoot, ensureScratch, scratchRoot, and secretsRoot symbols to
wire the realpath-based check consistently before the shell accepts the
directory as a cwd.
---
Outside diff comments:
In `@packages/grida-ai-agent/src/test/browser-harness.global.ts`:
- Around line 46-57: The browser harness teardown is leaving behind the new
scratch temp directories created for the AgentHost instances. Update the cleanup
logic in browser-harness.global.ts so the teardown removes both the main user
data dirs and the associated scratch dirs for the harness and foreign hosts. Use
the existing AgentHost setup symbols and the cleanup() routine to locate the
matching temp paths, and ensure the scratch bases derived from harnessDir and
foreignDir are included in deletion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2038d10-4a5c-41ac-8fdb-39881ec233ab
📒 Files selected for processing (18)
packages/grida-ai-agent/src/agent-host.test.tspackages/grida-ai-agent/src/agent-host.tspackages/grida-ai-agent/src/agent/index.tspackages/grida-ai-agent/src/agent/scratch-capability.test.tspackages/grida-ai-agent/src/daemon.test.tspackages/grida-ai-agent/src/http/routes/sessions.tspackages/grida-ai-agent/src/http/server.tspackages/grida-ai-agent/src/prompts.tspackages/grida-ai-agent/src/runtime/command-backend.tspackages/grida-ai-agent/src/runtime/index.tspackages/grida-ai-agent/src/runtime/workspace-agent-bindings.test.tspackages/grida-ai-agent/src/runtime/workspace-agent-bindings.tspackages/grida-ai-agent/src/session/scratch.test.tspackages/grida-ai-agent/src/session/scratch.tspackages/grida-ai-agent/src/shell/runner.test.tspackages/grida-ai-agent/src/shell/runner.tspackages/grida-ai-agent/src/test/browser-harness.global.tspackages/grida-ai-agent/src/tools/index.ts
/simplify cleanup: - Share the path.sep-prefix `containsPath` (new src/path-contains.ts) between the shell runner and the scratch containment assert — was a 3rd divergent copy. - `ensureScratch` now takes the already-derived scratch dir instead of re-deriving it from base+sessionId (the runtime already holds it); drops the double gate. - Inline the one-use `sessionDirFor` into `removeScratch`. Codex P2 review fixes: - Create scratch dirs owner-only (mode 0700) so other local accounts on a shared machine can't read produced/extracted artifacts under a world-traversable /tmp. - Make the host-start sweep SYNCHRONOUS and run it before serving, so a resumed session's ensureScratch can't race a still-in-flight async sweep that would delete the dir underneath a running command. - Drop the prompt's "without asking" claim — in accept-edits mode a mutating scratch command still hits the supervised Allow/Deny gate, so the unconditional promise was false. - Register session/scratch.ts (+ path-contains.ts) in SECURITY.md's GRIDA-SEC-004 file list. Tests updated for the new ensureScratch(dir) signature and sync sweep; adds an owner-only (0700) mode assertion. typecheck + 649 tests + lint + build green.
CodeRabbit: defaultScratchBase() resolved to one global <os.tmpdir()>/grida-agent and the start-time sweep deletes every <base>/sessions/*, so a second default-configured host on the same machine (e.g. a `cli serve` alongside the desktop sidecar) could wipe a running host's live session scratch. Namespace the default base per host by a short hash of its user_data_path: <os.tmpdir()>/grida-agent-<16-hex>. Stable across restarts of the same host (so the sweep still reclaims that host's prior-run scratch), distinct between hosts. Also tightens the predictable-shared-path angle from the earlier 0700 fix.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/grida-ai-agent/src/session/scratch.test.ts (1)
128-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a real path-containment check here.
startsWith(os.tmpdir())is only a string-prefix test, so a sibling like/tmp-evil/grida-agentwould still satisfy this assertion whenos.tmpdir()is/tmp. That weakens the contract this test is meant to pin. Please switch this to the same containment semantics used elsewhere in the PR (or apath.relative-based check).Suggested change
const b = defaultScratchBase(); - expect(b.startsWith(os.tmpdir())).toBe(true); + const rel = path.relative(os.tmpdir(), b); + expect(path.isAbsolute(rel) || rel.startsWith("..")).toBe(false); expect(b.endsWith("grida-agent")).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/grida-ai-agent/src/session/scratch.test.ts` around lines 128 - 130, The test for defaultScratchBase() is using a string prefix check, which can pass for sibling paths that merely start with the temp directory text. Update the assertion in scratch.test.ts to use a real path-containment check consistent with the rest of the PR, such as a path.relative-based validation, while still verifying the result ends with grida-agent.
♻️ Duplicate comments (1)
packages/grida-ai-agent/src/session/scratch.ts (1)
87-97: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-check the symlink containment hardening.
This still appears to compare
path.resolve(...)values lexically, so a scratch base symlink that resolves undersecretsRootcan pass the pre-I/O check. If this was fixed elsewhere, please ensure this helper or theensureScratchI/O path performs the authoritative realpath-based check before exposing the directory to shell cwd allowlisting.Also applies to: 108-113
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/grida-ai-agent/src/session/scratch.ts` around lines 87 - 97, The symlink containment check in assertOutsideSecretsRoot is still using path.resolve/containsPath, which can miss a scratchRoot symlink that resolves inside secretsRoot. Update this helper, or the ensureScratch path that consumes it, to perform the authoritative check using realpath-based resolution before the directory is exposed for shell cwd allowlisting. Make sure the fix is applied at the scratchRoot/secretsRoot boundary so the decision is based on the actual resolved filesystem locations, not lexical paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/grida-ai-agent/src/session/scratch.ts`:
- Around line 108-114: The ensureScratch helper currently only creates the
directory with SCRATCH_DIR_MODE, but existing session/scratch paths can keep
unsafe permissions. Update ensureScratch to explicitly tighten both the session
directory and the scratch directory after mkdir, using chmod on the relevant
paths so pre-existing directories are forced to 0700. If either chmod fails,
make the operation fail closed by surfacing the error instead of continuing.
---
Outside diff comments:
In `@packages/grida-ai-agent/src/session/scratch.test.ts`:
- Around line 128-130: The test for defaultScratchBase() is using a string
prefix check, which can pass for sibling paths that merely start with the temp
directory text. Update the assertion in scratch.test.ts to use a real
path-containment check consistent with the rest of the PR, such as a
path.relative-based validation, while still verifying the result ends with
grida-agent.
---
Duplicate comments:
In `@packages/grida-ai-agent/src/session/scratch.ts`:
- Around line 87-97: The symlink containment check in assertOutsideSecretsRoot
is still using path.resolve/containsPath, which can miss a scratchRoot symlink
that resolves inside secretsRoot. Update this helper, or the ensureScratch path
that consumes it, to perform the authoritative check using realpath-based
resolution before the directory is exposed for shell cwd allowlisting. Make sure
the fix is applied at the scratchRoot/secretsRoot boundary so the decision is
based on the actual resolved filesystem locations, not lexical paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 359b6d8c-09a4-461f-b811-fb271319585f
📒 Files selected for processing (8)
SECURITY.mdpackages/grida-ai-agent/src/http/server.tspackages/grida-ai-agent/src/path-contains.tspackages/grida-ai-agent/src/prompts.tspackages/grida-ai-agent/src/runtime/index.tspackages/grida-ai-agent/src/session/scratch.test.tspackages/grida-ai-agent/src/session/scratch.tspackages/grida-ai-agent/src/shell/runner.ts
✅ Files skipped from review due to trivial changes (1)
- packages/grida-ai-agent/src/path-contains.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/grida-ai-agent/src/prompts.ts
- packages/grida-ai-agent/src/http/server.ts
- packages/grida-ai-agent/src/shell/runner.ts
- packages/grida-ai-agent/src/runtime/index.ts
CodeRabbit (Major): assertOutsideSecretsRoot was lexical only — a scratch base that is a symlink resolving back inside `userData` (the secret root) would pass, ensureScratch would create the tree there, and the shell would later realpath that location and accept it as an allowed cwd (GRIDA-SEC-004 bypass). ensureScratch now adds an authoritative PHYSICAL check before creating anything: realpath the nearest existing ancestor of both the scratch dir and the secret root, then containment-test. A symlinked base that resolves into the secret root is rejected and nothing is created. Adds a symlink-bypass test (skipped on Windows). Also: browser-harness teardown now removes the per-host `-scratch` bases too (leak-free even though those hosts allocate no scratch today).
CodeRabbit (Major): mkdir(mode: 0o700) only sets the mode on dirs it CREATES, so a pre-existing (e.g. attacker-pre-created world-readable) scratch or session dir keeps its permissive mode and can still leak produced/extracted artifacts. ensureScratch now chmods both the scratch dir and its parent session dir to 0700 after mkdir, and FAILS CLOSED — a dir it can't restrict (not owner → EPERM) throws rather than silently serving artifacts to other local accounts. Adds a test that a pre-created 0755 scratch dir is tightened to 0700.
CodeRabbit (minor): the test asserted `b.startsWith(os.tmpdir())`, a string-prefix
check a sibling like `${tmpdir}-evil/grida-agent` would also pass. Use the PR's
own `containsPath` primitive so the assertion pins true containment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/grida-ai-agent/src/session/scratch.test.ts`:
- Around line 105-115: The scratch-dir test setup is still dependent on process
umask, so it may not actually start from a permissive mode and won’t exercise
the tightening behavior in ensureScratch. In scratch.test.ts, update the test
around ensureScratch, scratchRootFor, and fs.stat so the session directory and
root are explicitly chmod’d to 0755 before calling ensureScratch, then keep the
assertions that both the root and its parent are tightened to 0700.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90ebd458-d048-4436-bcfe-e53668822c3b
📒 Files selected for processing (3)
packages/grida-ai-agent/src/session/scratch.test.tspackages/grida-ai-agent/src/session/scratch.tspackages/grida-ai-agent/src/test/browser-harness.global.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/grida-ai-agent/src/test/browser-harness.global.ts
CodeRabbit (minor): `fs.mkdir(..., { mode: 0o755 })` can still yield 0700 under a
restrictive umask, so the pre-existing-permissive-dir test might not start from a
permissive mode and would skip the tightening path it exists to exercise.
Explicitly chmod the scratch dir and its parent to 0755 first, then assert
ensureScratch forces both to 0700.
The deliverable verification for the scratch substrate, as a gated live test (GRIDA_LIVE_AGENT, excluded from CI like the other *.live.test.ts). Real provider/model (OpenRouter, claude-sonnet-4.6), real shell, real per-session scratch dir. Drives one turn: "extract archive.zip into your scratch (not the project), show me the contents, then copy the file into the workspace to keep it." Asserts the model — told ONLY the scratch capability — extracts into scratch, inspects it, and promotes the file out. Verified live: $ unzip <ws>/archive.zip -d <scratch> # into scratch, not the project $ cat <scratch>/inside.txt # inspect $ cp <scratch>/inside.txt <ws>/inside.txt # promote to keep Pins: no command rejected by the shell gate, a command names the real scratch path, the entry lands in scratch, and the promoted file reaches the workspace with contents intact. Proves both the system/arch and that the agent utilizes scratch as intended.
pull Bot
pushed a commit
to A-Archives-and-Forks/grida
that referenced
this pull request
Jun 30, 2026
…omote Wraps the existing in-process BYOK image generation (resolveImageModel + generateImage, gridaco#908) behind an agent tool mirroring the AgentVision / view_image pattern: a capability-gated, server-resolved `generate_image` that writes produced bytes into the per-session scratch dir (gridaco#919) and lowers the result to a media block so the model SEES its own output. - New neutral `AgentGen` namespace (gen/index.ts): narrow ImageGenerator outcome interface, tool schema, dispatcher, media-block + path lowering (folds perception; reuses the view_image retention/elision window). - Generator binding built in workspace-agent-bindings where SecretsStore (host) meets the per-turn scratch dir; gated on host `images` capability + scratch sink + a present provider key (vision-style "don't advertise an unanswerable capability"). Writes via writeScratchFile (owner-only 0600, filename sanitized). BYOK-paid — no providerOptions.grida. - Threaded: tools/index capability, prompts.image_gen_capability, buildCapabilityHints, run-agent, runtime deps, http/server (image_gen_enabled = capabilities.images), message-view elision generalized to generate_image. Tests: pure AgentGen contract, capability-hint gating, binding presence gating, retention elision. Live (gated): agent told only the capability generates a red circle into scratch, perceives it, and cp-promotes it into the workspace — passed against OpenRouter seedream-4.5. GRIDA-SEC-004: key read in-process only, returns bytes never the key (Layer 5 intact); no new egress; SECURITY.md descriptions updated.
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the Scratch WG RFC (
docs/wg/ai/agent/scratch.md, #915) in@grida/agent: a per-session, system-managed, ephemeral filesystem area the agent uses as working space and the default home for what it produces — distinct from the durable workspace. Closes the gap where the agent's only filesystem surface was the user's project.Deliberately minimal and tool-agnostic per the approved plan ("reserve an OS temp dir + tell the agent"). The deferred image/video gen tool is a future consumer of this substrate, not a dependency.
What it does
<base>/sessions/<id>/scratchper session (base defaults to<os.tmpdir()>/grida-agent, outside the GRIDA-SEC-004 secret root), creates it on demand, and tells the agent its path via ascratchcapability prompt.unzip … -d <scratch>,cp <scratch>/x <ws>/(promotion),ls <scratch>all work — no routing fs backend, no/scratchnamespace.SDK-design calls (deciding table)
os.tmpdir()call baked into the runtime core. A speculativeScratchProviderinterface was deliberately not built (one filesystem host today → promote-on-dogfooding).Tests (names = the invariant, grep-able)
session/scratch.test.ts— derivation + I/O (S1/S2/S4).shell/runner.test.ts— theadditionalAllowedRootscwd gate; scratch arg not flagged protected.agent/scratch-capability.test.ts— the capability hint is advertised only when wired.runtime/workspace-agent-bindings.test.ts— a real-shell integration proving extract-into-scratch + promotion end-to-end.pnpm typecheckclean; 648 package tests pass; build + lint clean.Verification still owed (needs credentials)
The live BYOK zip-extraction run via the daemon (the plan's deliverable) needs a real provider key + a running daemon, so it's left for a manual pass. The real-shell integration test above is its automated proxy and covers the same chain.
Notes
Refs #916, #912
Summary by CodeRabbit
scratch_base.cwdvalidation to accept explicitly sanctioned scratch directories, with stronger path-containment safeguards.