feat(ai): typed custom events + sandbox file-diff hooks#892
feat(ai): typed custom events + sandbox file-diff hooks#892AlemTuzlak wants to merge 18 commits into
Conversation
…ten wire-projection test
…e.diff stream chunks
Adds a real-handle integration test (localProcessSandbox + a real git repo)
proving withSandbox's fileEvents:{diff:true} emits both sandbox.file and
sandbox.file.diff CUSTOM stream chunks, read back via literal-name narrowing
on the public KnownCustomEvent type without a cast.
Driving a real handle surfaced a genuine bug in buildFileHookEvent's diff()
accessor: it passed the virtual sandbox path straight to `git diff -- <path>`
instead of relativizing it like before() already does, so git resolved the
leading `/` against the filesystem root and failed with "fatal: Invalid
path" on every local-process sandbox. Fixed in file-diff.ts, with updated
unit coverage in file-diff.test.ts.
Lives in ai-sandbox-local-process (not ai-sandbox) so it can use a real
provider via public exports only, avoiding a reverse workspace
devDependency.
🚀 Changeset Version Preview1 package(s) bumped directly, 0 bumped as dependents. 🟩 Patch bumps
|
📝 WalkthroughWalkthroughThis PR adds typed ChangesTyped CUSTOM events and sandbox file-diff hooks
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Watcher
participant withSandbox
participant buildFileHookEvent
participant Hooks
participant SandboxRuntime
participant ChatEngine
Watcher->>withSandbox: raw file change event
withSandbox->>buildFileHookEvent: buildFileHookEvent(handle, watchRoot, baseSha, event)
buildFileHookEvent-->>withSandbox: SandboxFileHookEvent
withSandbox->>Hooks: dispatchDefinitionHooks(enriched)
withSandbox->>SandboxRuntime: emit(enriched)
SandboxRuntime->>ChatEngine: enqueue sandbox.file chunk
alt fileEvents.diff enabled
withSandbox->>buildFileHookEvent: enriched.diff()
buildFileHookEvent-->>withSandbox: unified diff string
withSandbox->>SandboxRuntime: emitFileDiff({ path, diff })
SandboxRuntime->>ChatEngine: enqueue sandbox.file.diff chunk
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
View your CI Pipeline Execution ↗ for commit 40f4295
☁️ 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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
packages/ai-sandbox/src/middleware.ts (1)
127-137: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
git rev-parse HEADruns even whenfileEvents: false.
baseShais computed unconditionally beforeresolveFileEvents(definition.fileEvents)is checked, so sandboxes that explicitly disable file watching still pay for a subprocess exec on every session setup.♻️ Suggested reorder
const watchRoot = definition.workspace?.root ?? DEFAULT_WORKSPACE_ROOT - let baseSha = '' - try { - const shaRes = await handle.process.exec('git rev-parse HEAD', { - cwd: watchRoot, - }) - if (shaRes.exitCode === 0) baseSha = shaRes.stdout.trim() - } catch { - // non-git workspace / exec rejects → baseSha stays '' (accessors fall back) - } + const fe = resolveFileEvents(definition.fileEvents) + let baseSha = '' + if (fe.enabled) { + try { + const shaRes = await handle.process.exec('git rev-parse HEAD', { + cwd: watchRoot, + }) + if (shaRes.exitCode === 0) baseSha = shaRes.stdout.trim() + } catch { + // non-git workspace / exec rejects → baseSha stays '' (accessors fall back) + } + }(remove the later duplicate
const fe = resolveFileEvents(...)at line 165)Also applies to: 165-165
🤖 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/middleware.ts` around lines 127 - 137, The `middleware.ts` setup is always running `git rev-parse HEAD` to compute `baseSha` even when `definition.fileEvents` disables file watching. Move the `resolveFileEvents(definition.fileEvents)` check ahead of the `handle.process.exec('git rev-parse HEAD', ...)` call in the middleware logic, and only compute `baseSha` when file events are enabled; then remove the later duplicate `const fe = resolveFileEvents(...)` so the `watchRoot`/`baseSha` work is skipped entirely for `fileEvents: false`.packages/ai-sandbox/tests/file-diff.test.ts (1)
64-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
diff()onchange/deleteevents with an emptybaseSha.Current tests only exercise
diff()'s empty-base path via acreateevent (Line 69). Given thesynthesizeAddPatchsemantics concern raised infile-diff.ts, tests forchange/delete+ emptybaseShawould help pin down the intended behavior.🤖 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/file-diff.test.ts` around lines 64 - 73, Add test coverage in file-diff.test.ts for FileHookEvent.diff() when baseSha is empty and the event type is change and delete, not just create. Use buildFileHookEvent and the diff() method to verify the intended synthesized patch behavior for empty-base handling, matching the semantics of synthesizeAddPatch in file-diff.ts.packages/ai/src/types.ts (1)
1384-1449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew CUSTOM event interfaces look correct and well-scoped.
Discriminated by literal
name, consistent with howKnownCustomEvent/ChatStreamnarrow downstream. One nit:FileChangedEvent.value(Line 1401) andSandboxFileDiffEvent.value(Line 1395) share the identical{ path: string; diff: string }shape — consider extracting a sharedFileDiffPayloadtype to avoid drift if one evolves independently.🤖 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/src/types.ts` around lines 1384 - 1449, Extract the shared `{ path: string; diff: string }` payload used by SandboxFileDiffEvent.value and FileChangedEvent.value into a reusable type, such as a shared file-diff payload near the existing Sandbox and Harness event interfaces in types.ts. Then update both SandboxFileDiffEvent and FileChangedEvent to reference that shared type so the shape stays aligned and future changes only need to be made in one place.
🤖 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 `@docs/sandbox/observability.md`:
- Around line 141-144: The fallback description is too broad and conflates the
accessor behavior for deleted files, new files, and non-git workspaces. Update
the matrix in the observability docs so it matches the interface semantics: use
the accessor names before(), after(), and diff() explicitly, and show that
deleted files still have before(), new files still have after(), and non-git
workspaces only synthesize diff() from after(). Apply the same wording
correction in SKILL.md so both docs describe the same accessor-specific
fallbacks consistently.
In `@packages/ai-sandbox/src/file-diff.ts`:
- Around line 56-75: The diff() path for non-git workspaces currently treats
every event like a new file, which misrepresents delete and change events.
Update the baseSha === '' branch in diff() to branch on event.type: reserve
synthesizeAddPatch(event.path, await after()) for create only, return an
appropriate removed/empty patch for delete, and avoid using add-patch semantics
for change. Use event.type, after(), and synthesizeAddPatch as the key symbols
to locate the logic.
In `@packages/ai-sandbox/src/middleware.ts`:
- Around line 165-186: Track the pending enriched.diff() work in middleware.ts
so it is awaited before teardown; right now watchWorkspace's onEvent fires the
diff promise without tracking it, and onFinish/onAbort/onError only wait for
watcher.stop(). Update the middleware flow around resolveFileEvents,
buildFileHookEvent, and the fe.diff branch to collect in-flight diff promises
and await them before the session closes so runtime?.emitFileDiff(...) is not
dropped.
In `@packages/ai-sandbox/tests/file-diff.test.ts`:
- Line 1: The import in the file-diff test is out of alphabetical order and
triggers the sort-imports lint rule. Update the import declaration in the test
file so the named imports from vitest are sorted alphabetically, keeping the
existing describe, it, and expect usage intact.
In `@packages/ai/src/activities/chat/index.ts`:
- Around line 714-724: The fire-and-forget call in the sandbox file hook emitter
can create an unhandled rejection because
`this.middlewareRunner.runSandboxFile(...)` is invoked with `void` and no error
handling. Update the `emit` callback in `Chat`’s constructor logic to either
route this work through the existing `ctx.defer()` / `Promise.allSettled`
pattern used elsewhere, or attach a `.catch()` that logs the failure via
`this.logger` so hook errors are handled safely. Use the existing `emit`
callback and `middlewareRunner.runSandboxFile` as the target points for the fix.
- Around line 725-738: The sandbox file diff event can be queued after the final
streamed model chunk and never drained, so make sure `sandbox.file.diff` is
flushed before the run completes. Update the streaming flow in
`streamModelResponse()` and the event handlers around
`emitFileDiff`/`sandboxFileQueue` in `chat/index.ts` so any pending queue
entries are emitted after the `git diff` promise settles, not only while model
chunks are streaming. Also verify the `compose.ts` `.then()` path still routes
its diff output through the same flushing mechanism.
In `@packages/ai/src/activities/chat/middleware/types.ts`:
- Around line 28-35: The SandboxFileHookEvent interface uses shorthand method
signatures for before, after, and diff, which violates the
`@typescript-eslint/method-signature-style` rule in this codebase. Update those
members in SandboxFileHookEvent to function-type properties while keeping the
same Promise<string> return types and semantics, so the interface matches the
project’s preferred style.
In `@packages/ai/tests/known-custom-events.test-d.ts`:
- Around line 5-24: The `KnownCustomEvent` assertions in
`known-custom-events.test-d.ts` are already narrowed to `type: 'CUSTOM'`, so the
`ev.type === 'CUSTOM'` checks are redundant and trigger
`@typescript-eslint/no-unnecessary-condition`. Update the test cases around
`KnownCustomEvent`, `isSessionIdEvent`, and the related `expectTypeOf`
assertions to remove those always-true guards, or alternatively add a targeted
lint disable with a brief explanation if you need to preserve a real-world
narrowing shape.
In `@packages/ai/tests/sandbox-runtime-emit.test.ts`:
- Around line 23-28: The `queue.push` payload in `sandbox-runtime-emit.test.ts`
already matches `StreamChunk`, so the `as StreamChunk` assertion is redundant.
Remove the unnecessary cast from the object literal passed to `queue.push` and
keep the existing `EventType.CUSTOM`/`sandbox.file` structure unchanged.
---
Nitpick comments:
In `@packages/ai-sandbox/src/middleware.ts`:
- Around line 127-137: The `middleware.ts` setup is always running `git
rev-parse HEAD` to compute `baseSha` even when `definition.fileEvents` disables
file watching. Move the `resolveFileEvents(definition.fileEvents)` check ahead
of the `handle.process.exec('git rev-parse HEAD', ...)` call in the middleware
logic, and only compute `baseSha` when file events are enabled; then remove the
later duplicate `const fe = resolveFileEvents(...)` so the `watchRoot`/`baseSha`
work is skipped entirely for `fileEvents: false`.
In `@packages/ai-sandbox/tests/file-diff.test.ts`:
- Around line 64-73: Add test coverage in file-diff.test.ts for
FileHookEvent.diff() when baseSha is empty and the event type is change and
delete, not just create. Use buildFileHookEvent and the diff() method to verify
the intended synthesized patch behavior for empty-base handling, matching the
semantics of synthesizeAddPatch in file-diff.ts.
In `@packages/ai/src/types.ts`:
- Around line 1384-1449: Extract the shared `{ path: string; diff: string }`
payload used by SandboxFileDiffEvent.value and FileChangedEvent.value into a
reusable type, such as a shared file-diff payload near the existing Sandbox and
Harness event interfaces in types.ts. Then update both SandboxFileDiffEvent and
FileChangedEvent to reference that shared type so the shape stays aligned and
future changes only need to be made in one place.
🪄 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: faeff44d-f795-4537-99ea-9911307aa6f5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
docs/adapters/openrouter.mddocs/chat/streaming.mddocs/config.jsondocs/protocol/custom-events.mddocs/sandbox/events.mddocs/sandbox/observability.mdpackages/ai-sandbox-local-process/package.jsonpackages/ai-sandbox-local-process/tests/sandbox-file-diff.integration.test.tspackages/ai-sandbox/src/file-diff.tspackages/ai-sandbox/src/middleware.tspackages/ai-sandbox/src/sandbox.tspackages/ai-sandbox/tests/file-diff.test.tspackages/ai-sandbox/tests/with-sandbox-hooks.test.tspackages/ai/skills/ai-core/ag-ui-protocol/SKILL.mdpackages/ai/skills/ai-core/middleware/SKILL.mdpackages/ai/src/activities/chat/index.tspackages/ai/src/activities/chat/middleware/compose.tspackages/ai/src/activities/chat/middleware/index.tspackages/ai/src/activities/chat/middleware/sandbox-runtime.tspackages/ai/src/activities/chat/middleware/types.tspackages/ai/src/index.tspackages/ai/src/types.tspackages/ai/tests/chat-result-types.test.tspackages/ai/tests/known-custom-events.test-d.tspackages/ai/tests/sandbox-file-dispatch.test.tspackages/ai/tests/sandbox-runtime-emit.test.ts
| const diff = async (): Promise<string> => { | ||
| if (baseSha === '') return synthesizeAddPatch(event.path, await after()) | ||
| // Pathspec must be relative to `root` (like `before()` above) — a bare | ||
| // leading `/` (e.g. the virtual `/workspace/x.ts`) is resolved by git | ||
| // against the filesystem root, not the repo root, and fails with | ||
| // "fatal: Invalid path" whenever the real repo root differs from | ||
| // `/workspace` (e.g. every local-process sandbox). | ||
| const rel = relTo(root, event.path) | ||
| try { | ||
| const res = await handle.process.exec( | ||
| `git diff ${q(baseSha)} -- ${q(rel)}`, | ||
| { | ||
| cwd: root, | ||
| }, | ||
| ) | ||
| return res.exitCode === 0 ? res.stdout : '' | ||
| } catch { | ||
| return '' | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
diff() mishandles change/delete events in non-git workspaces.
When baseSha === '', diff() unconditionally calls synthesizeAddPatch(event.path, await after()) — but that helper is documented as being for "a brand-new file" only (Line 17). For a delete event, after() returns '', so this produces a zero-line "add" patch with a --- /dev/null header, which misrepresents a deletion as a no-op creation. For a change event, it presents the entire current file as newly added rather than reflecting that only some content changed. Only the create path is currently covered by tests.
Consider branching on event.type here (e.g., emit an empty/removed-file patch for delete, and reserve synthesizeAddPatch for create only).
🐛 Proposed fix
const diff = async (): Promise<string> => {
- if (baseSha === '') return synthesizeAddPatch(event.path, await after())
+ if (baseSha === '') {
+ if (event.type === 'delete') return ''
+ return synthesizeAddPatch(event.path, await after())
+ }📝 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.
| const diff = async (): Promise<string> => { | |
| if (baseSha === '') return synthesizeAddPatch(event.path, await after()) | |
| // Pathspec must be relative to `root` (like `before()` above) — a bare | |
| // leading `/` (e.g. the virtual `/workspace/x.ts`) is resolved by git | |
| // against the filesystem root, not the repo root, and fails with | |
| // "fatal: Invalid path" whenever the real repo root differs from | |
| // `/workspace` (e.g. every local-process sandbox). | |
| const rel = relTo(root, event.path) | |
| try { | |
| const res = await handle.process.exec( | |
| `git diff ${q(baseSha)} -- ${q(rel)}`, | |
| { | |
| cwd: root, | |
| }, | |
| ) | |
| return res.exitCode === 0 ? res.stdout : '' | |
| } catch { | |
| return '' | |
| } | |
| } | |
| const diff = async (): Promise<string> => { | |
| if (baseSha === '') { | |
| if (event.type === 'delete') return '' | |
| return synthesizeAddPatch(event.path, await after()) | |
| } | |
| // Pathspec must be relative to `root` (like `before()` above) — a bare | |
| // leading `/` (e.g. the virtual `/workspace/x.ts`) is resolved by git | |
| // against the filesystem root, not the repo root, and fails with | |
| // "fatal: Invalid path" whenever the real repo root differs from | |
| // `/workspace` (e.g. every local-process sandbox). | |
| const rel = relTo(root, event.path) | |
| try { | |
| const res = await handle.process.exec( | |
| `git diff ${q(baseSha)} -- ${q(rel)}`, | |
| { | |
| cwd: root, | |
| }, | |
| ) | |
| return res.exitCode === 0 ? res.stdout : '' | |
| } catch { | |
| return '' | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 64-69: Avoid command injection
Context: handle.process.exec(
git diff ${q(baseSha)} -- ${q(rel)},
{
cwd: root,
},
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-typescript)
🪛 OpenGrep (1.23.0)
[ERROR] 65-70: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 56 - 75, The diff() path
for non-git workspaces currently treats every event like a new file, which
misrepresents delete and change events. Update the baseSha === '' branch in
diff() to branch on event.type: reserve synthesizeAddPatch(event.path, await
after()) for create only, return an appropriate removed/empty patch for delete,
and avoid using add-patch semantics for change. Use event.type, after(), and
synthesizeAddPatch as the key symbols to locate the logic.
| this.sandboxFileQueue.push( | ||
| this.createCustomEventChunk('sandbox.file', { ...event }), | ||
| this.createCustomEventChunk('sandbox.file', { | ||
| type: event.type, | ||
| path: event.path, | ||
| timestamp: event.timestamp, | ||
| }), | ||
| ) | ||
| }, | ||
| emitFileDiff: (value: { path: string; diff: string }) => { | ||
| this.sandboxFileQueue.push( | ||
| this.createCustomEventChunk('sandbox.file.diff', value), | ||
| ) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'Repo root: '; pwd
git ls-files 'packages/ai/src/activities/chat/index.ts' 'packages/ai/src/activities/chat/middleware.ts' | cat
printf '\n-- drainSandboxFileQueue refs --\n'
rg -n "drainSandboxFileQueue|sandboxFileQueue|emitFileDiff|sandbox\.file\.diff|sandbox\.file" packages/ai/src/activities/chat -S
printf '\n-- index.ts outline --\n'
ast-grep outline packages/ai/src/activities/chat/index.ts --view expanded
printf '\n-- middleware.ts outline --\n'
ast-grep outline packages/ai/src/activities/chat/middleware.ts --view expandedRepository: TanStack/ai
Length of output: 10017
🏁 Script executed:
set -euo pipefail
sed -n '1120,1185p' packages/ai/src/activities/chat/index.ts | cat -n
printf '\n----\n'
sed -n '680,770p' packages/ai/src/activities/chat/index.ts | cat -n
printf '\n---- middleware ----\n'
sed -n '1,260p' packages/ai/src/activities/chat/middleware.ts | cat -nRepository: TanStack/ai
Length of output: 7169
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/ai/src/activities/chat/index.ts')
text = p.read_text()
for needle in ['drainSandboxFileQueue(', 'streamModelResponse(', 'processToolCalls(', 'checkForPendingToolCalls(', 'pipeThroughMiddleware(']:
print(f'## {needle}')
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(i, line.strip())
PYRepository: TanStack/ai
Length of output: 1597
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('packages/ai/src/activities/chat/index.ts').read_text().splitlines()
targets = ['deferredPromises', 'await Promise.all', 'pipeThroughMiddleware(', 'runStructuredFinalization(', 'harvestCombinedStructuredOutput(', 'finalStructuredOutput', 'middlewareRunner.runSandboxFile', 'runSandboxFile(']
for needle in targets:
print(f'## {needle}')
for i, line in enumerate(text, 1):
if needle in line:
print(f'{i}: {line.strip()}')
print()
PYRepository: TanStack/ai
Length of output: 4020
🏁 Script executed:
set -euo pipefail
sed -n '780,920p' packages/ai/src/activities/chat/index.ts | cat -n
printf '\n----\n'
sed -n '2528,2608p' packages/ai/src/activities/chat/index.ts | cat -n
printf '\n---- sandbox runtime ----\n'
git ls-files 'packages/ai/src/activities/chat/middleware/*' | cat
printf '\n----\n'
sed -n '1,220p' packages/ai/src/activities/chat/middleware/sandbox-runtime.ts | cat -nRepository: TanStack/ai
Length of output: 11270
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('packages/ai/src/activities/chat/index.ts').read_text().splitlines()
for start, end in [(1000, 1170), (1280, 1615), (1960, 2365), (2540, 2608)]:
print(f'==== {start}-{end} ====')
for i in range(start, min(end, len(text)) + 1):
print(f'{i}: {text[i-1]}')
print()
PYRepository: TanStack/ai
Length of output: 41178
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('packages/ai/src/activities/chat/index.ts').read_text().splitlines()
for needle in ['deferredPromises', 'Promise.allSettled', 'Promise.all', 'await this.deferredPromises', 'void this.middlewareRunner.runSandboxFile', 'runSandboxFile(', 'emitFileDiff:']:
print(f'## {needle}')
for i, line in enumerate(text, 1):
if needle in line:
print(f'{i}: {line.strip()}')
print()
PYRepository: TanStack/ai
Length of output: 851
🏁 Script executed:
set -euo pipefail
sed -n '778,930p' packages/ai/src/activities/chat/index.ts | cat -nRepository: TanStack/ai
Length of output: 7149
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path('packages/ai/src/activities/chat/index.ts').read_text().splitlines()
for start, end in [(778, 930), (930, 1015), (2544, 2578)]:
print(f'==== {start}-{end} ====')
for i in range(start, min(end, len(text)) + 1):
print(f'{i}: {text[i-1]}')
print()
PYRepository: TanStack/ai
Length of output: 11459
🏁 Script executed:
set -euo pipefail
sed -n '650,690p' packages/ai/src/activities/chat/index.ts | cat -nRepository: TanStack/ai
Length of output: 1893
🏁 Script executed:
set -euo pipefail
rg -n "emitFileDiff\(" packages/ai/src -SRepository: TanStack/ai
Length of output: 149
🏁 Script executed:
set -euo pipefail
sed -n '70,150p' packages/ai/src/activities/chat/middleware/compose.ts | cat -nRepository: TanStack/ai
Length of output: 3605
🏁 Script executed:
set -euo pipefail
printf 'Files:\n'
git ls-files 'packages/ai/src/activities/chat/**' | cat
printf '\n-- relevant symbols --\n'
rg -n "sandbox\.file\.diff|emitFileDiff|composeAgentFileTreeWithDiff|git diff|Diff mode|diff mode" packages/ai/src/activities/chat -S
printf '\n-- compose files outline --\n'
fd -a 'compose.ts' packages/ai/src/activities/chat || true
fd -a 'middleware.ts' packages/ai/src/activities/chat || true
fd -a 'sandbox-runtime.ts' packages/ai/src/activities/chat || trueRepository: TanStack/ai
Length of output: 2368
Flush sandbox.file.diff before the run finishes. packages/ai/src/activities/chat/middleware/compose.ts emits diffs from a .then() after git diff, but packages/ai/src/activities/chat/index.ts:725-738 only drains sandboxFileQueue from streamModelResponse(). If that promise settles after the last streamed chunk, the queued sandbox.file.diff can be left behind and never reach the consumer.
🤖 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/src/activities/chat/index.ts` around lines 725 - 738, The sandbox
file diff event can be queued after the final streamed model chunk and never
drained, so make sure `sandbox.file.diff` is flushed before the run completes.
Update the streaming flow in `streamModelResponse()` and the event handlers
around `emitFileDiff`/`sandboxFileQueue` in `chat/index.ts` so any pending queue
entries are emitted after the `git diff` promise settles, not only while model
chunks are streaming. Also verify the `compose.ts` `.then()` path still routes
its diff output through the same flushing mechanism.
|
@AlemTuzlak The before(): Promise<string>
after(): Promise<string>
diff(): Promise<string>This violates the repository's ESLint configuration ( Changing them to function properties resolves the lint error: before: () => Promise<string>
after: () => Promise<string>
diff: () => Promise<string>I verified the fix by running: pnpm eslint packages/ai/src/activities/chat/middleware/types.ts
pnpm test:prAfter applying the change, the full |
…lint method-signature-style)
…on Linux (find . not find /workspace)
…ng diffs, hook-dispatch catch, docs
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/ai/skills/ai-core/middleware/SKILL.md`:
- Around line 451-456: Update the non-git workspace fallback description in
SKILL.md to match the actual behavior of the accessors and diff logic. In the
section describing `before()`, `after()`, and `diff()`, state that `after()`
still returns current on-disk content, only `before()` resolves to `''` in a
non-git workspace, and `diff()` synthesizes an add-patch from `after()` unless
the event is `delete`, in which case it returns `''`.
🪄 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: 588a806e-bd01-4e5a-944c-ddc99900e844
📒 Files selected for processing (8)
docs/sandbox/observability.mdpackages/ai-sandbox/src/file-diff.tspackages/ai-sandbox/src/middleware.tspackages/ai-sandbox/tests/file-diff.test.tspackages/ai/skills/ai-core/middleware/SKILL.mdpackages/ai/src/activities/chat/index.tspackages/ai/tests/known-custom-events.test-d.tspackages/ai/tests/sandbox-runtime-emit.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/sandbox/observability.md
- packages/ai-sandbox/tests/file-diff.test.ts
- packages/ai-sandbox/src/middleware.ts
- packages/ai/src/activities/chat/index.ts
- packages/ai/tests/known-custom-events.test-d.ts
- packages/ai-sandbox/src/file-diff.ts
- packages/ai/tests/sandbox-runtime-emit.test.ts
| accessors throw: a deleted file resolves `after()` to `''` (it still has | ||
| `before()`); a new file resolves `before()` to `''` (it still has `after()`); | ||
| a non-git workspace resolves **both** `before()` and `after()` to `''` and | ||
| makes `diff()` fall back to a synthesized add-patch built from `after()` — | ||
| except for a `delete` event in a non-git workspace, where there's nothing to | ||
| synthesize and `diff()` resolves to `''`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant documentation and inspect the surrounding section.
git ls-files packages/ai/skills/ai-core/middleware/SKILL.md
wc -l packages/ai/skills/ai-core/middleware/SKILL.md
sed -n '380,490p' packages/ai/skills/ai-core/middleware/SKILL.md
# Find nearby references to after(), before(), diff(), git, and non-git fallback.
rg -n "after\(\)|before\(\)|diff\(\)|non-git|git workspace|synthesized add-patch|delete event" packages/ai/skills/ai-core/middleware/SKILL.mdRepository: TanStack/ai
Length of output: 5484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the document structure for the section around the cited lines.
ast-grep outline packages/ai/skills/ai-core/middleware/SKILL.md --view expandedRepository: TanStack/ai
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repo for the implementation or other docs that define the semantics.
rg -n "current on-disk content|resolves `after\(\)`|resolves `before\(\)`|synthesized add-patch|non-git workspace" packages -g '*.md' -g '*.ts' -g '*.js'Repository: TanStack/ai
Length of output: 26859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact section around the cited lines and nearby contract text.
sed -n '420,470p' packages/ai/skills/ai-core/middleware/SKILL.md
sed -n '470,540p' packages/ai/skills/ai-core/middleware/SKILL.mdRepository: TanStack/ai
Length of output: 4255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the authoritative type docs and the sandbox diff implementation.
sed -n '1,120p' packages/ai/src/activities/chat/middleware/types.ts
sed -n '1,120p' packages/ai-sandbox/src/file-diff.ts
sed -n '120,220p' packages/ai-sandbox/src/middleware.tsRepository: TanStack/ai
Length of output: 10702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate tests that cover non-git and delete-event behavior.
rg -n "non-git|delete event|synthesized add-patch|before\\(|after\\(|diff\\(\\)" packages/ai-sandbox/tests packages/ai/tests packages/ai/src -g '*.test.ts' -g '*.ts'Repository: TanStack/ai
Length of output: 2175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sandbox tests covering file diff semantics.
sed -n '1,180p' packages/ai-sandbox/tests/file-diff.test.ts
sed -n '1,220p' packages/ai-sandbox/tests/with-sandbox-hooks.test.tsRepository: TanStack/ai
Length of output: 11141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect specific source files for the runtime semantics.
cat -n packages/ai/src/activities/chat/middleware/types.ts | sed -n '1,120p'
echo '---'
cat -n packages/ai-sandbox/src/file-diff.ts | sed -n '1,220p'
echo '---'
cat -n packages/ai-sandbox/src/middleware.ts | sed -n '110,180p'Repository: TanStack/ai
Length of output: 11949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation that drives the accessors.
cat -n packages/ai-sandbox/src/middleware.ts | sed -n '120,220p'
echo '---'
cat -n packages/ai-sandbox/src/file-diff.ts | sed -n '1,220p'Repository: TanStack/ai
Length of output: 8003
Fix the non-git fallback description. after() still reads current on-disk content; only before() becomes '' in a non-git workspace. diff() then falls back to the synthesized add-patch from after(), with delete returning ''.
Suggested wording tweak
- a non-git workspace resolves **both** `before()` and `after()` to `''` and makes `diff()` fall back to a synthesized add-patch built from `after()`
+ a non-git workspace resolves `before()` to `''`, still reads `after()` from disk, and makes `diff()` fall back to a synthesized add-patch built from `after()`📝 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.
| accessors throw: a deleted file resolves `after()` to `''` (it still has | |
| `before()`); a new file resolves `before()` to `''` (it still has `after()`); | |
| a non-git workspace resolves **both** `before()` and `after()` to `''` and | |
| makes `diff()` fall back to a synthesized add-patch built from `after()` — | |
| except for a `delete` event in a non-git workspace, where there's nothing to | |
| synthesize and `diff()` resolves to `''`. | |
| accessors throw: a deleted file resolves `after()` to `''` (it still has | |
| `before()`); a new file resolves `before()` to `''` (it still has `after()`); | |
| a non-git workspace resolves `before()` to `''`, still reads `after()` from disk, | |
| and makes `diff()` fall back to a synthesized add-patch built from `after()` — | |
| except for a `delete` event in a non-git workspace, where there's nothing to | |
| synthesize and `diff()` resolves to `''`. |
🧰 Tools
🪛 SkillSpector (2.3.7)
[warning] 54: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 58: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 58: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 59: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 60: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 61: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 62: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 63: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 64: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 65: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 66: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 67: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 83: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 85: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 86: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 87: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 88: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 89: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 236: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 241: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
🤖 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/skills/ai-core/middleware/SKILL.md` around lines 451 - 456,
Update the non-git workspace fallback description in SKILL.md to match the
actual behavior of the accessors and diff logic. In the section describing
`before()`, `after()`, and `diff()`, state that `after()` still returns current
on-disk content, only `before()` resolves to `''` in a non-git workspace, and
`diff()` synthesizes an add-patch from `after()` unless the event is `delete`,
in which case it returns `''`.
2 participants
What
Makes every framework-emitted CUSTOM event type-constrained on the chat stream, and gives sandbox file hooks lazy git-backed diffs.
sandbox.file,file.changed,${adapter}.session-id,code_mode:*,skill:registered, plus the already-typed structured-output/tool-interaction/ui events) is now a discriminated interface unified asKnownCustomEvent. The chat stream is retypedChatStream = AsyncIterable<Exclude<StreamChunk, CustomEvent> | KnownCustomEvent>, so a plainif (chunk.type === 'CUSTOM' && chunk.name === '<literal>')narrowschunk.valuewith no helper and no cast. No guard utilities by design.onFile*hooks receive aSandboxFileHookEventwith lazy, git-backedbefore()/after()/diff()(computed on call — path-only hooks pay nothing). The serializedsandbox.filechunk stays path-only.defineSandbox({ fileEvents: { diff: true } })emits a per-filesandbox.file.diff{ path, diff }chunk.Tests / quality
localProcessSandbox+ real git) that caught a genuine bug —diff()was passing the virtual/workspacepath togit diffinstead of relativizing, silently yielding''on every local-process sandbox.gitexec args are shell-quoted.kiiragreen (774/774 snippets);@tanstack/aiand@tanstack/ai-sandboxtypechecks green. Docs +ag-ui-protocol/middlewareskills updated.test:prsweep was not completed locally (monorepo-wide fan-out); CI runs it.Notes for reviewers
testing/e2e/harness has no sandbox wiring (aimock LLM-only), so browser-level sandbox E2E is structurally infeasible here — covered by the real-handle integration test instead. True sandbox E2E infra is a suggested follow-up.docs/adapters/openrouter.md: its cost snippet fence was tagged```typescript ignore, matching the existingstructured-outputs/streaming.mdprecedent — a kiira-env resolution quirk with@ag-ui/corezod types throughExclude<>unions, not a real type regression (packagetscis green; real consumers narrow fine).🤖 Generated with Claude Code
Summary by CodeRabbit
CUSTOMstreaming viaChatStream, including aKnownCustomEventtaxonomy.sandbox.file.diff) with lazy hook accessors (before,after,diff).