fix(sandbox): make file-diff failures observable and correct (#914)#917
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR improves sandbox diff correctness by synthesizing git-style add-patches for untracked files, withholding diffs for git-ignored files, and making watcher and teardown paths log failures instead of silently dropping them. ChangesSandbox observability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
🚀 Changeset Version Preview6 package(s) bumped directly, 39 bumped as dependents. 🟥 Major bumps
🟨 Minor bumps
🟩 Patch bumps
|
|
View your CI Pipeline Execution ↗ for commit a45094c
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-codex
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-mcp
@tanstack/ai-mistral
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ai-sandbox/src/file-diff.ts`:
- Around line 189-192: The baseSha === '' branch in file-diff.ts bypasses the
ignore filtering used elsewhere, so create/change events can leak git-ignored
contents through the diff stream. Update the diff() flow to reuse the same git
check-ignore logic here, or delegate this path through
synthesizeIfUntracked(rel) before calling synthesizeAddPatch, and keep the
handling aligned with the existing baseSha !== '' branch.
In `@packages/ai-sandbox/tests/with-sandbox-hooks.test.ts`:
- Around line 7-8: The test file has duplicate type-only imports from the same
`@tanstack/ai` module, which triggers `import/no-duplicates`. Consolidate the
`SandboxFileEvent` and `ChatMiddlewareContext` imports into a single type import
at the top of the file, keeping the existing identifiers unchanged so the rest
of `with-sandbox-hooks.test.ts` continues to compile.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9dcb09fe-1f2d-40de-8c32-6457f7701312
📒 Files selected for processing (11)
.changeset/sandbox-file-diff-observability.mddocs/config.jsondocs/sandbox/observability.mdpackages/ai-sandbox/src/file-diff.tspackages/ai-sandbox/src/middleware.tspackages/ai-sandbox/src/watch.tspackages/ai-sandbox/tests/fakes.tspackages/ai-sandbox/tests/file-diff.test.tspackages/ai-sandbox/tests/watch.test.tspackages/ai-sandbox/tests/with-sandbox-hooks.test.tspackages/ai/skills/ai-core/middleware/SKILL.md
Comment on lines
189
to
+192
| const diff = async (): Promise<string> => { | ||
| if (baseSha === '') { | ||
| if (event.type === 'delete') return '' | ||
| return synthesizeAddPatch(event.path, await after()) | ||
| return synthesizeAddPatch(relTo(root, event.path), await after()) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Git-ignored file contents can leak when baseSha === '' in a fresh git init repo with no HEAD. This branch emits full add-patches for create/change events without the git check-ignore gate used in the baseSha !== '' path, so an ignored .env could be streamed into the diff feed. Reuse that ignore check here, or route this case through synthesizeIfUntracked(rel).
🤖 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/ai-sandbox/src/file-diff.ts` around lines 189 - 192, The baseSha ===
'' branch in file-diff.ts bypasses the ignore filtering used elsewhere, so
create/change events can leak git-ignored contents through the diff stream.
Update the diff() flow to reuse the same git check-ignore logic here, or
delegate this path through synthesizeIfUntracked(rel) before calling
synthesizeAddPatch, and keep the handling aligned with the existing baseSha !==
'' branch.
Comment on lines
7
to
8
| import type { SandboxFileEvent } from '@tanstack/ai' | ||
| import type { ChatMiddlewareContext } from '@tanstack/ai' |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Merge the duplicate @tanstack/ai type imports — ESLint fails the build.
import/no-duplicates reports both lines as errors. Combine them into a single type import.
♻️ Proposed fix
-import type { SandboxFileEvent } from '`@tanstack/ai`'
-import type { ChatMiddlewareContext } from '`@tanstack/ai`'
+import type { SandboxFileEvent, ChatMiddlewareContext } from '`@tanstack/ai`'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { SandboxFileEvent } from '@tanstack/ai' | |
| import type { ChatMiddlewareContext } from '@tanstack/ai' | |
| import type { SandboxFileEvent, ChatMiddlewareContext } from '`@tanstack/ai`' |
🧰 Tools
🪛 ESLint
[error] 7-7: '@tanstack/ai' imported multiple times.
(import/no-duplicates)
[error] 8-8: '@tanstack/ai' imported multiple times.
(import/no-duplicates)
🤖 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/ai-sandbox/tests/with-sandbox-hooks.test.ts` around lines 7 - 8, The
test file has duplicate type-only imports from the same `@tanstack/ai` module,
which triggers `import/no-duplicates`. Consolidate the `SandboxFileEvent` and
`ChatMiddlewareContext` imports into a single type import at the top of the
file, keeping the existing identifiers unchanged so the rest of
`with-sandbox-hooks.test.ts` continues to compile.
Source: Linters/SAST tools
Follow-up to #892. The sandbox file-diff subsystem was built on a "never throws, fall back to ''/empty, don't log" contract, so a git/exec/fs failure silently became empty/wrong data. - diff(): synthesize an add-patch for any file git isn't tracking (a created file AND every later edit), keyed on tracked-ness at the baseline rather than event.type; a tracked file identical to the baseline still diffs empty, and a transient git-show probe failure no longer fabricates a bogus add-patch. Synthesized patches follow git's add-file shape (diff --git header, new file mode, no-newline marker). - git-ignored files are withheld from the diff feed (the event still fires; diff() returns '') so a .env/secret is never surfaced. - exec-poll watcher no longer fabricates phantom create/delete storms: a failed poll preserves the previous snapshot, a failed/partial initial poll seeds without diffing (re-baselining on the first complete poll), and a partial (permission-denied) poll is merged, not diffed. - watcher watches the definition workspace.root (was defaulting to /workspace, diverging from enrichment); native watch re-seeds if its initial listing fails; teardown always destroys even if stop() rejects. - every swallowed git/exec/fs failure now logs (real anomalies under errors/warn, expected-empty under the sandbox debug category).
Cover the git-ignored-file withholding, the untracked-file synthesized diff, and that swallowed failures now log; align the middleware SKILL hook-error claim with the actual errors category.
tombeckenham
force-pushed
the
fix/sandbox-file-diff-observability
branch
from
63f58af to
ea598c7
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/ai-sandbox/tests/with-sandbox-hooks.test.ts (1)
7-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate
@tanstack/aitype imports still present — ESLintimport/no-duplicatesfails.Previously flagged and not yet merged. Combine into a single import to clear the lint error.
♻️ Proposed fix
-import type { SandboxFileEvent } from '`@tanstack/ai`' -import type { ChatMiddlewareContext } from '`@tanstack/ai`' +import type { SandboxFileEvent, ChatMiddlewareContext } from '`@tanstack/ai`'🤖 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/ai-sandbox/tests/with-sandbox-hooks.test.ts` around lines 7 - 8, The test file still has duplicate type-only imports from the same module, causing the import/no-duplicates lint error. Update the import section in with-sandbox-hooks.test.ts by combining SandboxFileEvent and ChatMiddlewareContext into a single `@tanstack/ai` type import, and keep the rest of the test code unchanged.Source: Linters/SAST tools
packages/ai-sandbox/src/file-diff.ts (1)
189-192: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
baseSha === ''path still bypassesgit check-ignore— ignored-file contents can leak.This was flagged previously and remains unresolved. When
baseSha === ''(freshgit initwith no commits),git check-ignoreis functional, but this branch callssynthesizeAddPatchdirectly without the ignore gate used in thesynthesizeIfUntrackedpath. An ignored.envcreated in such a repo would have its contents streamed into the diff feed.Consider extracting the
git check-ignorelogic into a shared helper and applying it here before synthesizing.🔒 Proposed fix: add ignore check to the `baseSha === ''` path
const diff = async (): Promise<string> => { if (baseSha === '') { if (event.type === 'delete') return '' - return synthesizeAddPatch(relTo(root, event.path), await after()) + const rel = relTo(root, event.path) + const content = await after() + if (content === '') return '' + try { + const ignored = await handle.process.exec( + `git check-ignore -q -- ${q(rel)}`, + { cwd: root }, + ) + if (ignored.exitCode === 0) { + logger?.sandbox('sandbox diff() withheld for git-ignored file', { + path: event.path, + }) + return '' + } + } catch (error) { + logger?.warn('sandbox diff() git check-ignore failed', { + path: event.path, + error, + }) + } + return synthesizeAddPatch(rel, content) }🤖 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/ai-sandbox/src/file-diff.ts` around lines 189 - 192, The `baseSha === ''` branch in `diff` bypasses the same ignore filtering used by `synthesizeIfUntracked`, so ignored files can still be included. Update the `diff` function in `file-diff.ts` to run `git check-ignore` before calling `synthesizeAddPatch` when there are no commits, ideally by extracting the existing ignore check into a shared helper and reusing it in both paths. Ensure the delete case still returns early, but all non-delete additions in this branch go through the ignore gate first.
🤖 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.
Duplicate comments:
In `@packages/ai-sandbox/src/file-diff.ts`:
- Around line 189-192: The `baseSha === ''` branch in `diff` bypasses the same
ignore filtering used by `synthesizeIfUntracked`, so ignored files can still be
included. Update the `diff` function in `file-diff.ts` to run `git check-ignore`
before calling `synthesizeAddPatch` when there are no commits, ideally by
extracting the existing ignore check into a shared helper and reusing it in both
paths. Ensure the delete case still returns early, but all non-delete additions
in this branch go through the ignore gate first.
In `@packages/ai-sandbox/tests/with-sandbox-hooks.test.ts`:
- Around line 7-8: The test file still has duplicate type-only imports from the
same module, causing the import/no-duplicates lint error. Update the import
section in with-sandbox-hooks.test.ts by combining SandboxFileEvent and
ChatMiddlewareContext into a single `@tanstack/ai` type import, and keep the rest
of the test code unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37861f75-cdc2-4acc-b6a5-b403cdcd8f9a
📒 Files selected for processing (11)
.changeset/sandbox-file-diff-observability.mddocs/config.jsondocs/sandbox/observability.mdpackages/ai-sandbox/src/file-diff.tspackages/ai-sandbox/src/middleware.tspackages/ai-sandbox/src/watch.tspackages/ai-sandbox/tests/fakes.tspackages/ai-sandbox/tests/file-diff.test.tspackages/ai-sandbox/tests/watch.test.tspackages/ai-sandbox/tests/with-sandbox-hooks.test.tspackages/ai/skills/ai-core/middleware/SKILL.md
✅ Files skipped from review due to trivial changes (3)
- docs/config.json
- .changeset/sandbox-file-diff-observability.md
- docs/sandbox/observability.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ai-sandbox/tests/fakes.ts
- packages/ai-sandbox/tests/watch.test.ts
- packages/ai-sandbox/tests/file-diff.test.ts
- packages/ai-sandbox/src/middleware.ts
A steady-state `find` poll throw was logged only under `sandbox` (off by default). A *persistent* mid-run exec failure (seam wedged, `find` gone) then leaves the watcher silently dead for the rest of the run with no signal at default verbosity — the silent-degradation class this change set out to eliminate. Distinguish teardown (`controller.signal.aborted` → quiet `sandbox`) from a genuine failure, and escalate to `warn` after 3 consecutive non-teardown throws so a wedged watcher is observable while a single transient blip stays quiet. Honors the observability doc's "real anomalies → errors category (on by default)" contract on the one path that hid one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tombeckenham
approved these changes
Jul 9, 2026
tombeckenham
left a comment
tombeckenham
left a comment
Contributor
There was a problem hiding this comment.
All good. Small changes only
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
2 participants
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.
Follow-up to #892, closing #914. A post-merge review of the sandbox file-diff subsystem found a cluster of correctness + observability gaps sharing one root cause:
buildFileHookEventand the watcher were built on a "never throws, fall back to''/empty, don't log" contract, so a git/exec/fs failure silently became empty (or wrong) data — the worst failure mode for an observability feature.Severity: medium — the feature is new and opt-in (no prod consumers yet), no data-loss/security angle beyond the git-ignored-content fix below.
Correctness
git diff <sha> -- <path>shows nothing for a file git isn't tracking, so a file the agent creates — and every later edit to it — used to emit an empty diff.diff()now synthesizes an add-patch keyed on tracked-ness at the baseline (probed via thegit showexit code), not onevent.type. A tracked file identical to the baseline still diffs empty, and a transient probe failure no longer fabricates a bogus add-patch. Synthesized patches follow git's add-file shape (diff --githeader,new file mode,\ No newline at end of filemarker, header-only for empty files).create/deletestorms (sandbox file-diff: make failures observable + correct (follow-up to #892) #914.2). A failedfindpoll now preserves the previous snapshot instead of collapsing to empty; a failed/partial initial poll seeds without diffing (re-baselining on the first complete poll); a partial (permission-denied) poll is merged, not diffed, so a transiently-unreadable subtree isn't reported as deletions.definition.workspace.root(it was defaulting to/workspacewhile enrichment relativized againstworkspace.root).watcher.stop()can no longer skipdefinition.destroy()on the abort path; the native watcher re-seeds if its initial listing fails.Observability (#914.3)
Every swallowed git/exec/fs failure — across the diff accessors, both watcher paths (exec-poll + native
fs.watch), the git-baseline capture, and per-hook dispatch — now logs before falling back: real anomalies undererrors/warn(on by default), expected-empty conditions under thesandboxdebug category. This also aligns the middleware SKILL's hook-error claim with actual behavior (it logs undererrors, matching the engine's run-scoped path).Security-adjacent
.env/credentials file still fires its file event (you're notified it changed) butdiff()returns'', so its contents are never surfaced in the stream.Tests (#914.4)
pendingDiffsteardown drain now has deferred-promise tests foronFinish/onAbort/onError(verified to fail if the drain is removed).Review
Ran through a 3-round automated review-fix loop (correctness / silent-failure / test-coverage / comment / type-design lenses) to convergence — every finding fixed and re-confirmed, all fixes covered by tests verified to fail on revert.
Verification
test:types,test:eslint(src),test:build, andtest:docsclean; 189/190 ai-sandbox unit tests pass (the 1 failure is a pre-existing Windows path-separator issue inharness-cwd.test.ts, unrelated to this change). Changeset included (@tanstack/ai-sandboxpatch).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation