From dcc23a8b4ef9a5e33b602b92e6e87cea7889ba38 Mon Sep 17 00:00:00 2001 From: Alem Tuzlak Date: Wed, 8 Jul 2026 12:13:11 +0200 Subject: [PATCH 1/4] fix(sandbox): make file-diff failures observable and correct (#914) 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). --- .changeset/sandbox-file-diff-observability.md | 12 + packages/ai-sandbox/src/file-diff.ts | 168 +++++++++- packages/ai-sandbox/src/middleware.ts | 106 +++++- packages/ai-sandbox/src/watch.ts | 209 ++++++++++-- packages/ai-sandbox/tests/fakes.ts | 27 ++ packages/ai-sandbox/tests/file-diff.test.ts | 314 ++++++++++++++++++ packages/ai-sandbox/tests/watch.test.ts | 291 ++++++++++++++++ .../tests/with-sandbox-hooks.test.ts | 270 ++++++++++++++- 8 files changed, 1345 insertions(+), 52 deletions(-) create mode 100644 .changeset/sandbox-file-diff-observability.md diff --git a/.changeset/sandbox-file-diff-observability.md b/.changeset/sandbox-file-diff-observability.md new file mode 100644 index 000000000..77b527dc7 --- /dev/null +++ b/.changeset/sandbox-file-diff-observability.md @@ -0,0 +1,12 @@ +--- +'@tanstack/ai-sandbox': patch +--- + +Make sandbox file-diff correct and observable (follow-up to #892): + +- `diff()` now synthesizes an add-patch for any file git isn't tracking (a file the agent created **and every later edit to it**), keyed on tracked-ness at the baseline rather than on the event being a `create` — so agent-created files no longer stream empty diffs. A tracked file identical to the baseline still diffs empty, and a transient git-show probe failure no longer fabricates a bogus add-patch. +- The synthesized patch now matches `git diff`'s add-file shape (`diff --git` header, `new file mode`, repo-relative paths). +- **git-ignored files are withheld from the diff feed**: the file event still fires (you're notified it changed) but `diff()` returns `''`, so a secret like a `.env` never has its contents surfaced. +- The native `fs.watch` watcher re-seeds lazily if its initial workspace listing fails, so a pre-existing file is correctly reported as a `change` (not a `create`) on first edit. +- The exec-poll watcher no longer fabricates phantom `create`/`delete` storms: a failed poll (thrown exec, or non-zero exit with no output) preserves the previous snapshot, a failed initial poll seeds without diffing, and a partial (`find` permission-denied) poll is merged rather than diffed so transiently-unreadable files aren't reported as deleted. +- Every swallowed git/exec/fs failure — in the diff accessors, both watcher paths (exec-poll and native `fs.watch`), the git-baseline capture, and per-hook dispatch — is now logged (real anomalies under `errors`, expected-empty conditions under the `sandbox` debug category) instead of silently becoming empty data. diff --git a/packages/ai-sandbox/src/file-diff.ts b/packages/ai-sandbox/src/file-diff.ts index 174c37395..bb0f1ffee 100644 --- a/packages/ai-sandbox/src/file-diff.ts +++ b/packages/ai-sandbox/src/file-diff.ts @@ -1,4 +1,5 @@ import type { SandboxFileEvent, SandboxFileHookEvent } from '@tanstack/ai' +import type { InternalLogger } from '@tanstack/ai/adapter-internals' import type { SandboxHandle } from './contracts' /** Path relative to the repo/workspace root, POSIX form. */ @@ -14,29 +15,58 @@ function q(value: string): string { return `'${value.replace(/'/g, `'\\''`)}'` } -/** Minimal unified add-patch for a brand-new file (non-git workspaces). */ -function synthesizeAddPatch(path: string, content: string): string { - const lines = content === '' ? [] : content.replace(/\n$/, '').split('\n') +/** + * Unified add-patch for a brand-new file, closely following the shape `git + * diff` produces for an added file (`diff --git` header + `new file mode` + + * `--- /dev/null` + `+++ b/`), so synthesized `create` diffs align with + * the real `git diff` output emitted for `change` events. `rel` must be the + * repo-root-relative POSIX path (like git's). Reproduces git's `\ No newline + * at end of file` marker and the header-only form for a zero-byte file, so a + * consumer applying the patch reconstructs the file byte-for-byte. It is not + * byte-identical to git — it omits the `index ..` line and always + * writes the `+1,N` hunk count (git omits `,1`) — but both are valid + * unified-diff and accepted by `git apply`/`patch`. + */ +function synthesizeAddPatch(rel: string, content: string): string { + const header = `diff --git a/${rel} b/${rel}\nnew file mode 100644\n` + // A zero-byte new file has no hunk in git's output — just the header. + if (content === '') return header + const hasFinalNewline = content.endsWith('\n') + const lines = content.replace(/\n$/, '').split('\n') const body = lines.map((l) => `+${l}`).join('\n') - return `--- /dev/null\n+++ ${path}\n@@ -0,0 +1,${lines.length} @@\n${body}${body ? '\n' : ''}` + return ( + header + + `--- /dev/null\n` + + `+++ b/${rel}\n` + + `@@ -0,0 +1,${lines.length} @@\n` + + body + + (hasFinalNewline ? '\n' : '\n\\ No newline at end of file\n') + ) } /** * Wrap a raw {@link SandboxFileEvent} with lazy git-backed accessors bound to * the live handle. `baseSha` is the session baseline (`''` when the workspace - * isn't a git repo). Never throws. + * isn't a git repo). Never throws — every git/fs failure falls back to `''` + * (or a synthesized add-patch), but is logged first via `logger` so a failure + * is observable instead of silently becoming empty data. */ export function buildFileHookEvent( handle: SandboxHandle, root: string, baseSha: string, event: SandboxFileEvent, + logger?: InternalLogger, ): SandboxFileHookEvent { const after = async (): Promise => { if (event.type === 'delete') return '' try { return await handle.fs.read(event.path) - } catch { + } catch (error) { + logger?.warn('sandbox after() failed to read file', { + path: event.path, + error, + }) return '' } } @@ -48,15 +78,118 @@ export function buildFileHookEvent( `git show ${q(baseSha)}:${q(rel)}`, { cwd: root }, ) - return res.exitCode === 0 ? res.stdout : '' - } catch { + if (res.exitCode === 0) return res.stdout + // Non-zero exit is EXPECTED when the file didn't exist at the baseline + // (a newly created file) — git exits 128 with "exists on disk, but not + // in ". Log under `sandbox` (off by default) rather than warn so a + // create event's before() doesn't spam a warning on every new file. + logger?.sandbox('before() git show non-zero exit', { + path: event.path, + exitCode: res.exitCode, + stderr: res.stderr, + }) + return '' + } catch (error) { + logger?.warn('sandbox before() git show failed', { + path: event.path, + error, + }) + return '' + } + } + // Empty `git diff` fallback: `git diff -- ` shows nothing for a + // file git isn't tracking, so an untracked file (the common agent action — + // and every subsequent edit to it, which arrives as a `change`, not just the + // first `create`) yields empty stdout even though it has content. Synthesize + // an add-patch, but ONLY when the file is genuinely untracked at the + // baseline — an empty diff for a *tracked* file means "identical to + // baseline", a real no-op that must stay empty. Presence at `baseSha` + // distinguishes them, probed below via the `git show` EXIT CODE (NOT + // `before()`'s `''`, which also means "git show threw" — see the inline note). + // ponytail: reached only when `git diff` came back empty — the common case + // for an untracked file (and every edit to it), rare for a tracked one. It + // spends up to two extra subprocesses (`git check-ignore`, then `git show`) + // plus one `after()` read per such event; both verdicts are invariant per + // path across a run, but memoizing them would need cross-event state the + // per-event accessor doesn't hold. Add a per-path cache in the watcher if + // edit-burst latency matters. + const synthesizeIfUntracked = async (rel: string): Promise => { + const content = await after() + if (content === '') return '' // deleted / empty / unreadable — nothing to add + // Don't expose the CONTENTS of a git-ignored file (`.env`, credentials, + // build artifacts, …) in the diff feed. The file event still fires so + // consumers are notified it changed — only its diff is withheld. A + // force-added, ignored-yet-TRACKED file produces a non-empty `git diff` + // above and never reaches here, so its real diff is unaffected. + try { + const ignored = await handle.process.exec( + `git check-ignore -q -- ${q(rel)}`, + { cwd: root }, + ) + // check-ignore: exit 0 ⇒ path is ignored; 1 ⇒ not ignored; 128 ⇒ error. + if (ignored.exitCode === 0) { + logger?.sandbox('sandbox diff() withheld for git-ignored file', { + path: event.path, + }) + return '' + } + if (ignored.exitCode !== 1) { + // Not the expected "not ignored" (1) — a real check-ignore error (128: + // corrupt repo, bad invocation). Same anomaly class as the throw below, + // so `warn`. We still fall through and diff, so a broken probe never + // silently withholds; but log it, or an error here would look exactly + // like "not ignored" and could expose a would-be-withheld file's diff. + logger?.warn('sandbox diff() git check-ignore non-zero exit', { + path: event.path, + exitCode: ignored.exitCode, + stderr: ignored.stderr, + }) + } + } catch (error) { + // check-ignore threw (git/exec broken) — same anomaly class as the other + // git execs here, so `warn`. We fall through and diff as usual rather + // than withhold, so a broken probe can't hide a legitimate diff. + logger?.warn('sandbox diff() git check-ignore failed', { + path: event.path, + error, + }) + } + // Distinguish "absent at the baseline (untracked)" from "present at the + // baseline" by the git-show EXIT CODE, not by before()'s `''` — which also + // means "git show threw". Conflating a transient git-show failure with + // untracked would fabricate a full-file add-patch for an unchanged tracked + // file the agent never touched. + try { + const res = await handle.process.exec( + `git show ${q(baseSha)}:${q(rel)}`, + { cwd: root }, + ) + // exit 0 ⇒ tracked; `git diff` was already empty ⇒ identical to baseline + // ⇒ genuine no-op. + if (res.exitCode === 0) return '' + // Non-zero is the EXPECTED "absent at baseline ⇒ untracked" case (exit + // 128), but a genuine git-show error (bad object, corrupt repo, invalid + // sha) also exits non-zero and would silently fabricate a full-file + // add-patch. Log it under `sandbox` (like `before()` does) so a + // persistent probe error is greppable, then fall back to synthesize. + logger?.sandbox( + 'sandbox diff() tracked-ness probe non-zero exit (treating as untracked)', + { path: event.path, exitCode: res.exitCode, stderr: res.stderr }, + ) + return synthesizeAddPatch(rel, content) + } catch (error) { + // Uncertain — don't fabricate a full-file add-patch on a probe failure. + logger?.warn('sandbox diff() tracked-ness probe failed', { + path: event.path, + error, + }) return '' } } const diff = async (): Promise => { if (baseSha === '') { if (event.type === 'delete') return '' - return synthesizeAddPatch(event.path, await after()) + return synthesizeAddPatch(relTo(root, 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 @@ -71,8 +204,21 @@ export function buildFileHookEvent( cwd: root, }, ) - return res.exitCode === 0 ? res.stdout : '' - } catch { + if (res.exitCode !== 0) { + logger?.warn('sandbox diff() git diff non-zero exit', { + path: event.path, + exitCode: res.exitCode, + stderr: res.stderr, + }) + return '' + } + if (res.stdout !== '') return res.stdout + return synthesizeIfUntracked(rel) + } catch (error) { + logger?.warn('sandbox diff() git diff failed', { + path: event.path, + error, + }) return '' } } diff --git a/packages/ai-sandbox/src/middleware.ts b/packages/ai-sandbox/src/middleware.ts index 83dfe7cd4..d25314429 100644 --- a/packages/ai-sandbox/src/middleware.ts +++ b/packages/ai-sandbox/src/middleware.ts @@ -29,6 +29,7 @@ import { ProjectionCapability, provideWorkspaceProjection } from './projection' import { resolveSecret } from './secrets' import { watchWorkspace } from './watch' import { DEFAULT_WORKSPACE_ROOT } from './bootstrap' +import type { InternalLogger } from '@tanstack/ai/adapter-internals' import type { AbortInfo, ChatMiddlewareContext, @@ -53,10 +54,34 @@ interface SandboxRunState { * watcher callback, awaited before teardown so a pending diff isn't * dropped when the run finishes/aborts/errors mid-computation. */ pendingDiffs: Array> + /** Logger captured at setup, so terminal hooks can log watcher teardown. */ + logger?: InternalLogger } const runState = new WeakMap() +/** + * Stop the watcher and drain any in-flight `diff()` promises before teardown, + * so the final file's diff isn't dropped when a run finishes/aborts/errors + * mid-computation. The `pendingDiffs` await is the load-bearing line — without + * it a deferred diff resolves after the run is gone and its chunk is lost. + */ +async function drainWatcher( + state: SandboxRunState, + phase: 'finish' | 'abort' | 'error', +): Promise { + // Guard `stop()`: a rejecting watcher teardown must NOT propagate out of + // here, or the caller skips the `definition.destroy(...)` that follows — + // leaking the sandbox on exactly the abort path that must ALWAYS tear down. + try { + await state.watcher?.stop() + } catch (error) { + state.logger?.warn('sandbox watcher stop failed', { phase, error }) + } + await Promise.allSettled(state.pendingDiffs) + if (state.watcher) state.logger?.sandbox('sandbox watcher stopped', { phase }) +} + /** Defensively pull tenant scoping out of the runtime context, if present. */ function tenantFrom( context: unknown, @@ -83,11 +108,14 @@ function buildEnsureCtx(ctx: ChatMiddlewareContext): SandboxEnsureContext { /** * Dispatch a sandbox file event to the per-type hooks declared on the * definition. Errors in individual hooks are swallowed so one bad hook - * cannot break the run. + * cannot break the run — but are logged under the `errors` category first, so + * a throwing hook is observable (matching the run-scoped path in the engine + * and the behavior the observability docs promise). */ async function dispatchDefinitionHooks( hooks: SandboxHooks | undefined, event: SandboxFileHookEvent, + logger?: InternalLogger, ): Promise { if (!hooks) return const typed = ( @@ -101,8 +129,14 @@ async function dispatchDefinitionHooks( if (!fn) continue try { await fn(event) - } catch { - // swallowed — one bad hook must not break the run + } catch (error) { + // swallowed — one bad hook must not break the run — but logged so the + // failure isn't invisible. + logger?.errors('sandbox file hook failed', { + path: event.path, + type: event.type, + error, + }) } } } @@ -128,15 +162,43 @@ export function withSandbox( provideSandbox(ctx, handle) if (definition.policy) provideSandboxPolicy(ctx, definition.policy) + // Pull the runtime (and its logger) up front so `baseSha` capture and + // hook dispatch below can log through the same `sandbox`/`errors` + // categories the engine uses. + const runtime = getSandboxRuntime(ctx, { optional: true }) + const logger = runtime?.logger + 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) + if (shaRes.exitCode === 0) { + baseSha = shaRes.stdout.trim() + logger?.sandbox('sandbox git baseline captured', { + root: watchRoot, + baseSha, + }) + } else { + // Non-zero exit: either not a git repository (non-git workspace) or a + // repo with no commits (no HEAD). Expected, but it silently degrades + // every subsequent diff to a full-file add-patch, so surface it + // under `sandbox` (with stderr) rather than leaving nothing to grep. + logger?.sandbox('sandbox git baseline unavailable (non-zero exit)', { + root: watchRoot, + exitCode: shaRes.exitCode, + stderr: shaRes.stderr, + }) + } + } catch (error) { + // exec rejected (git not on PATH, exec seam broken) → baseSha stays '' + // and accessors fall back, but this is a real anomaly, not a plain + // non-git workspace, so warn. + logger?.warn('sandbox git baseline capture failed', { + root: watchRoot, + error, + }) } const workspace = definition.workspace @@ -170,7 +232,6 @@ export function withSandbox( const pendingDiffs: Array> = [] let watcher: SandboxWatchHandle | undefined if (fe.enabled) { - const runtime = getSandboxRuntime(ctx, { optional: true }) watcher = await watchWorkspace(handle, { onEvent: (event: SandboxFileEvent) => { const enriched = buildFileHookEvent( @@ -178,8 +239,9 @@ export function withSandbox( watchRoot, baseSha, event, + logger, ) - void dispatchDefinitionHooks(hooks, enriched) + void dispatchDefinitionHooks(hooks, enriched, logger) runtime?.emit(enriched) if (fe.diff) { pendingDiffs.push( @@ -188,11 +250,27 @@ export function withSandbox( .then((diff) => { runtime?.emitFileDiff({ path: event.path, diff }) }) - .catch(() => undefined), + .catch((error: unknown) => { + logger?.warn('sandbox file diff emit failed', { + path: event.path, + error, + }) + }), ) } }, + // Watch the SAME root the enrichment layer relativizes against + // (`buildFileHookEvent(handle, watchRoot, …)` and the `baseSha` + // capture). Without this the watcher defaults to `/workspace` while + // enrichment uses `watchRoot`, so a custom `workspace.root` makes the + // two look at different directories and git pathspecs break. + root: watchRoot, ...(ctx.signal !== undefined ? { signal: ctx.signal } : {}), + ...(logger !== undefined ? { logger } : {}), + }) + logger?.sandbox('sandbox watcher started', { + root: watchRoot, + diff: fe.diff, }) } @@ -201,6 +279,7 @@ export function withSandbox( ensureCtx, pendingDiffs, ...(watcher ? { watcher } : {}), + ...(logger !== undefined ? { logger } : {}), }) }, @@ -209,8 +288,7 @@ export function withSandbox( if (!state) return const { handle, ensureCtx } = state - await state.watcher?.stop() - await Promise.allSettled(state.pendingDiffs) + await drainWatcher(state, 'finish') const lifecycle = definition.lifecycle @@ -244,8 +322,7 @@ export function withSandbox( const state = runState.get(ctx) if (!state) return - await state.watcher?.stop() - await Promise.allSettled(state.pendingDiffs) + await drainWatcher(state, 'abort') // ALWAYS tear down on an explicit abort, regardless of `destroyOnComplete`. // The in-sandbox agent process is not killed by closing its IO stream @@ -261,8 +338,7 @@ export function withSandbox( const state = runState.get(ctx) if (!state) return - await state.watcher?.stop() - await Promise.allSettled(state.pendingDiffs) + await drainWatcher(state, 'error') await definition.hooks?.onError?.(info.error) // On failure, only tear down when the lifecycle says so; otherwise leave diff --git a/packages/ai-sandbox/src/watch.ts b/packages/ai-sandbox/src/watch.ts index 71743ae29..22665afaf 100644 --- a/packages/ai-sandbox/src/watch.ts +++ b/packages/ai-sandbox/src/watch.ts @@ -19,6 +19,7 @@ import { DEFAULT_WORKSPACE_ROOT } from './bootstrap' import type { SandboxHandle } from './contracts' import type { SandboxFileEvent } from '@tanstack/ai' +import type { InternalLogger } from '@tanstack/ai/adapter-internals' export type { SandboxFileEvent } from '@tanstack/ai' /** @deprecated alias retained for the low-level watch API. */ @@ -39,6 +40,12 @@ export interface WatchOptions { ignore?: Array /** Stop watching when this signal aborts. */ signal?: AbortSignal + /** + * Optional logger. When present, a failed `find` poll (non-zero exit or a + * thrown exec) is logged instead of silently degrading the snapshot — the + * failure mode a plain exec-poll watcher hides. + */ + logger?: InternalLogger } export interface SandboxWatchHandle { @@ -144,17 +151,46 @@ async function startNativeWatch( handle: SandboxHandle, options: WatchOptions & { root: string; ignore: Array }, ): Promise { - const { onEvent, root, ignore } = options + const { onEvent, root, ignore, logger } = options const watch = handle.fs.watch if (!watch) throw new Error('native watch is unavailable on this provider') // Seed the set of existing files so the first event per path is classified // correctly (create vs change). - const known = await collectPaths(handle, root, ignore) + const seed = await collectPaths(handle, root, ignore, logger) + const known = seed.files + // If the ROOT list failed, `known` is untrustworthy — every pre-existing + // file would misclassify as `create` on its first edit. Re-seed lazily on + // the next event(s): by the time real activity arrives the fs has usually + // recovered, and re-listing then establishes the baseline. Dedupe concurrent + // re-seeds behind a single in-flight promise. + // ponytail: a file genuinely CREATED in the narrow window between the failed + // seed and the first event gets picked up by the re-seed and so mislabels as + // `change` once. That's strictly better than the whole-run mislabel a + // never-recovered empty seed causes, and `diff()` is correct regardless. + let seeded = seed.rootOk + let reseeding: Promise | null = null + const ensureSeeded = (): Promise => { + if (seeded) return Promise.resolve() + if (!reseeding) { + reseeding = collectPaths(handle, root, ignore, logger).then((r) => { + if (r.rootOk) { + for (const p of r.files) known.add(p) + seeded = true + logger?.sandbox('sandbox watch: re-seeded after failed initial seed', { + root, + }) + } + reseeding = null + }) + } + return reseeding + } const subscription = await watch(root, (raw) => { const path = raw.path if (isIgnored(path, ignore)) return void (async () => { + await ensureSeeded() const exists = await handle.fs.exists(path) const timestamp = Date.now() if (!exists) { @@ -166,14 +202,28 @@ async function startNativeWatch( known.add(path) onEvent({ type: 'create', path, timestamp }) } - })().catch(() => undefined) + })().catch((error: unknown) => { + // A failed classify (e.g. `fs.exists` threw) drops this file's event — + // log it so a missing diff isn't silent (the whole point of the watcher). + logger?.warn('sandbox watch: native event classify failed', { + path, + error, + }) + }) }) - const onAbort = (): void => void subscription.stop().catch(() => undefined) + // A failed `subscription.stop()` can leak an OS-level watch — log rather + // than swallow it silently. + const logStopFailure = (error: unknown): void => + logger?.warn('sandbox watch: native subscription.stop() failed', { + root, + error, + }) + const onAbort = (): void => void subscription.stop().catch(logStopFailure) options.signal?.addEventListener('abort', onAbort, { once: true }) // The signal may have aborted during the awaits above (the once-listener // would have missed it) — tear down now if so. - if (options.signal?.aborted) void subscription.stop().catch(() => undefined) + if (options.signal?.aborted) void subscription.stop().catch(logStopFailure) return { stop: async () => { @@ -192,33 +242,129 @@ async function startPollWatch( intervalMs: number }, ): Promise { - const { onEvent, root, ignore, intervalMs } = options + const { onEvent, root, ignore, intervalMs, logger } = options const command = buildFindCommand(ignore) const controller = new AbortController() - const snapshot = async (): Promise> => { - const result = await handle.process.exec(command, { - cwd: root, - signal: controller.signal, + // A poll result: the parsed snapshot plus whether `find` completed cleanly. + // `null` means the poll produced no usable output at all (thrown exec, or a + // non-zero exit with empty stdout) — callers preserve the previous snapshot. + // Collapsing a failed poll to `{}` would make the next diff fabricate a + // `delete` for every tracked file (and a `create` for each on recovery) — + // one transient `find` blip would fan a phantom storm out to hooks/stream. + interface Poll { + map: Map + /** `false` when `find` exited non-zero but still printed rows (partial). */ + complete: boolean + } + const snapshot = async (isInitial = false): Promise => { + let result + try { + result = await handle.process.exec(command, { + cwd: root, + signal: controller.signal, + }) + } catch (error) { + // Thrown exec — container not ready, `find` seam rejects, or a + // mid-teardown abort. Treat as a failed poll so BOTH the initial seed + // and every tick preserve `previous` instead of rejecting setup (which + // would crash the run and leak the sandbox) or the interval. A steady- + // state throw is usually a transient/teardown blip → `sandbox`. But the + // INITIAL poll can't be a teardown (a pre-aborted signal is guarded in + // `watchWorkspace`), so a throw there is an unambiguous anomaly — `find` + // missing, container never ready — that would leave the watcher dead for + // the whole run, so surface it at `warn`. + if (isInitial) { + logger?.warn('sandbox watch: initial `find` poll threw', { root, error }) + } else { + logger?.sandbox('sandbox watch: `find` poll threw', { root, error }) + } + return null + } + if (result.exitCode === 0) { + return { map: parseFindOutput(result.stdout, root), complete: true } + } + // Non-zero exit doesn't mean "no data": GNU `find` exits >0 on the first + // permission-denied entry it hits mid-traversal (common in containers, and + // the ignore list is a `-not -path` filter, not `-prune`, so `find` still + // descends into unreadable dirs) yet still prints every readable file. Use + // that partial output — marked `complete: false` so the tick merges rather + // than diffs it — instead of blinding the watcher for the whole run. Only a + // non-zero exit with NO output is a truly failed poll. + if (result.stdout !== '') { + logger?.sandbox( + 'sandbox watch: `find` non-zero exit with partial output', + { root, exitCode: result.exitCode, stderr: result.stderr }, + ) + return { map: parseFindOutput(result.stdout, root), complete: false } + } + logger?.warn('sandbox watch: `find` poll exited non-zero with no output', { + root, + exitCode: result.exitCode, + stderr: result.stderr, }) - return result.exitCode === 0 - ? parseFindOutput(result.stdout, root) - : new Map() + return null } - let previous = await snapshot() + // `null` until the first poll that yields usable output. A failed INITIAL + // poll must NOT seed an empty baseline — the first successful poll would then + // diff against `{}` and fabricate a `create` for every pre-existing file. So + // the first non-null snapshot is adopted as the baseline WITHOUT diffing. + let previous: Map | null = null + // Whether `previous` was established from a COMPLETE poll. A baseline seeded + // from a PARTIAL poll is provisional — files unreadable during that poll are + // absent from it and would later fabricate `create`s when they recover — so + // the first complete poll re-baselines without diffing. + let seededFromComplete = false + { + const poll = await snapshot(true) + if (poll) { + previous = poll.map + seededFromComplete = poll.complete + } + } const state = { running: true } const tick = async (): Promise => { if (!state.running) return try { - const next = await snapshot() + const poll = await snapshot() + // Failed poll — keep `previous` and retry next tick (see `snapshot`). + if (poll === null) return + if (previous === null) { + // First usable snapshot after a failed initial poll — seed, don't diff. + previous = poll.map + seededFromComplete = poll.complete + return + } + if (!seededFromComplete && poll.complete) { + // First complete poll after a provisional (partial) seed — re-baseline + // WITHOUT diffing, so files merely unreadable at seed time don't + // fabricate `create`s. (Real creates during this degraded-startup + // window are missed — an acceptable trade for not fabricating events.) + logger?.sandbox( + 'sandbox watch: re-baselined after provisional partial seed', + { root }, + ) + previous = poll.map + seededFromComplete = true + return + } + // A partial (non-`complete`) poll can't distinguish "deleted" from + // "transiently unreadable this poll", so MERGE it over `previous`: pick + // up new/changed files without fabricating a `delete` for a path this + // poll simply couldn't see. A real deletion still surfaces on the next + // complete poll. + const next = poll.complete + ? poll.map + : new Map([...previous, ...poll.map]) for (const event of diffSnapshots(previous, next, Date.now())) { onEvent(event) } previous = next - } catch { - // transient exec failure (e.g. mid-teardown) — try again next tick + } catch (error) { + // Defensive: a throw from diff dispatch — preserve `previous`, retry. + logger?.sandbox('sandbox watch: tick failed', { root, error }) } } @@ -244,26 +390,41 @@ async function startPollWatch( return { stop } } -/** Recursively collect file paths under `root`, honoring `ignore`. */ +/** + * Recursively collect file paths under `root`, honoring `ignore`. `rootOk` is + * `false` when the ROOT `list` itself failed — the seed is then untrustworthy + * (empty/partial), which the native watcher uses to trigger a lazy re-seed. A + * failed *subdirectory* list is logged but doesn't flip `rootOk` (its files are + * simply absent, a smaller misclassification surface). + */ async function collectPaths( handle: SandboxHandle, root: string, ignore: Array, -): Promise> { + logger?: InternalLogger, +): Promise<{ files: Set; rootOk: boolean }> { const files = new Set() - const walk = async (dir: string): Promise => { + let rootOk = true + const walk = async (dir: string, isRoot: boolean): Promise => { let entries: Awaited> try { entries = await handle.fs.list(dir) - } catch { + } catch (error) { + // A dir we can't list is seeded as empty, so its existing files would + // later misclassify as `create` on first edit — log rather than hide it. + if (isRoot) rootOk = false + logger?.warn('sandbox watch: failed to list directory while seeding', { + dir, + error, + }) return } for (const entry of entries) { if (ignore.includes(entry.name)) continue - if (entry.type === 'dir') await walk(entry.path) + if (entry.type === 'dir') await walk(entry.path, false) else files.add(entry.path) } } - await walk(root) - return files + await walk(root, true) + return { files, rootOk } } diff --git a/packages/ai-sandbox/tests/fakes.ts b/packages/ai-sandbox/tests/fakes.ts index f4ae8ea9d..db4772908 100644 --- a/packages/ai-sandbox/tests/fakes.ts +++ b/packages/ai-sandbox/tests/fakes.ts @@ -1,3 +1,5 @@ +import { resolveDebugOption } from '@tanstack/ai/adapter-internals' +import type { InternalLogger } from '@tanstack/ai/adapter-internals' import type { ExecResult, SandboxCapabilities, @@ -226,3 +228,28 @@ export function makeFakeProvider( } return provider } + +/** + * An `InternalLogger` (all categories on) that records every emitted call by + * its underlying level, for asserting that a code path logs rather than + * silently swallows. `sandbox`/other debug categories route to `debug`, + * `warn` to `warn`, and `errors` to `error`. + */ +export function captureLogger(): { + logger: InternalLogger + calls: Array<{ level: string; msg: string }> +} { + const calls: Array<{ level: string; msg: string }> = [] + const rec = + (level: string) => + (msg: string): void => void calls.push({ level, msg }) + const logger = resolveDebugOption({ + logger: { + debug: rec('debug'), + info: rec('info'), + warn: rec('warn'), + error: rec('error'), + }, + }) + return { logger, calls } +} diff --git a/packages/ai-sandbox/tests/file-diff.test.ts b/packages/ai-sandbox/tests/file-diff.test.ts index 217949010..a581df5ac 100644 --- a/packages/ai-sandbox/tests/file-diff.test.ts +++ b/packages/ai-sandbox/tests/file-diff.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import { buildFileHookEvent } from '../src/file-diff' +import { captureLogger } from './fakes' import type { SandboxHandle } from '../src/contracts' function fakeHandle( @@ -119,4 +120,317 @@ describe('buildFileHookEvent', () => { expect(await e.diff()).toBe('DIFF') expect(calls[0]).toBe("git diff 'sha1' -- 'src/a.ts'") }) + + it('diff() with a base synthesizes an add-patch when git diff is empty for a create (untracked file)', async () => { + // `git diff -- ` shows nothing for an untracked file, so a + // freshly created file yields exitCode 0 + empty stdout even though it has + // content. The git-show probe exits non-zero (absent at baseline), so the + // create must still get a real diff. + const h = fakeHandle({ + read: async () => 'line1\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } // not ignored + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent(h, '/workspace', 'sha1', { + type: 'create', + path: '/workspace/new.ts', + timestamp: 1, + }) + const patch = await e.diff() + expect(patch).toContain('--- /dev/null') + expect(patch).toContain('+line1') + }) + + it('diff() returns "" for a TRACKED file whose git diff is empty (identical to baseline, no bogus add-patch)', async () => { + // A file present at the baseline (before() non-empty) whose content now + // matches it legitimately diffs empty — it must NOT be synthesized into a + // full-file add-patch. Only a file ABSENT at the baseline synthesizes. + const h = fakeHandle({ + read: async () => 'X', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } // not ignored + if (cmd.startsWith('git show')) + return { stdout: 'X', stderr: '', exitCode: 0 } // tracked at baseline + return { stdout: '', stderr: '', exitCode: 0 } // git diff: identical + }, + }) + const e = buildFileHookEvent(h, '/workspace', 'sha1', { + type: 'change', + path: '/workspace/a.ts', + timestamp: 1, + }) + expect(await e.diff()).toBe('') + }) + + it('diff() synthesizes an add-patch for a CHANGE to an untracked file (the post-create edit case)', async () => { + // The dominant #914 case: an agent creates a file then edits it. The edit + // arrives as a `change`, `git diff` is still empty (git ignores untracked + // files), and before() is '' (absent at baseline) — so it must synthesize, + // not stream an empty diff. This is what keys on tracked-ness, not on the + // event being a `create`. + const h = fakeHandle({ + read: async () => 'hello\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } // not ignored + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent(h, '/workspace', 'sha1', { + type: 'change', + path: '/workspace/new.ts', + timestamp: 1, + }) + const patch = await e.diff() + expect(patch).toContain('diff --git a/new.ts b/new.ts') + expect(patch).toContain('+hello') + }) + + it('diff() withholds content for a git-ignored file — notify-only, empty diff', async () => { + // `git check-ignore` reports the path as ignored (exit 0). Even though the + // file has content and is untracked, diff() returns '' so the file's + // contents (e.g. a .env / secret) never reach the diff feed. The file event + // itself still fires elsewhere to notify that it changed. + const h = fakeHandle({ + read: async () => 'SECRET=1\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 0 } // ignored + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent(h, '/workspace', 'sha1', { + type: 'create', + path: '/workspace/.env', + timestamp: 1, + }) + expect(await e.diff()).toBe('') + }) + + it('diff() falls through to a normal diff (does NOT withhold) when git check-ignore itself throws', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + read: async () => 'x\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) throw new Error('check-ignore boom') + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'create', path: '/workspace/n.ts', timestamp: 1 }, + logger, + ) + const patch = await e.diff() + expect(patch).toContain('+x') // synthesized, not withheld + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('check-ignore')), + ).toBe(true) + }) + + it('logs a warning and still diffs when git check-ignore errors (exit 128)', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + read: async () => 'y\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // error, not "1" + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'create', path: '/workspace/z.ts', timestamp: 1 }, + logger, + ) + const patch = await e.diff() + expect(patch).toContain('+y') // not withheld on a check-ignore error + expect( + calls.some( + (c) => c.level === 'warn' && c.msg.includes('check-ignore non-zero'), + ), + ).toBe(true) + }) + + it('logs (sandbox) when the tracked-ness probe exits non-zero before synthesizing', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + read: async () => 'q\n', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } // not ignored + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } + return { stdout: '', stderr: '', exitCode: 0 } + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'create', path: '/workspace/q.ts', timestamp: 1 }, + logger, + ) + const patch = await e.diff() + expect(patch).toContain('+q') + expect( + calls.some( + (c) => c.level === 'debug' && c.msg.includes('tracked-ness probe non-zero'), + ), + ).toBe(true) + }) + + it('logs a warning when git diff itself throws', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + exec: async (cmd: string) => { + if (cmd.startsWith('git diff')) throw new Error('diff boom') + return { stdout: '', stderr: '', exitCode: 0 } + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'change', path: '/workspace/a.ts', timestamp: 1 }, + logger, + ) + expect(await e.diff()).toBe('') + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('git diff')), + ).toBe(true) + }) + + it('synthesizes a git-shaped add-patch for a multi-line untracked file, with the no-newline marker', async () => { + const h = fakeHandle({ + read: async () => 'a\nb\nc', // 3 lines, NO trailing newline + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } + if (cmd.startsWith('git show')) + return { stdout: '', stderr: 'fatal', exitCode: 128 } + return { stdout: '', stderr: '', exitCode: 0 } + }, + }) + const e = buildFileHookEvent(h, '/workspace', 'sha1', { + type: 'create', + path: '/workspace/m.ts', + timestamp: 1, + }) + const patch = await e.diff() + expect(patch).toContain('new file mode 100644') + expect(patch).toContain('@@ -0,0 +1,3 @@') + expect(patch).toContain('+a\n+b\n+c') + expect(patch).toContain('\\ No newline at end of file') + }) + + it('synthesized add-patch omits the no-newline marker for content that ends in a newline', async () => { + const e = buildFileHookEvent( + fakeHandle({ read: async () => 'one\ntwo\n' }), + '/workspace', + '', + { type: 'create', path: '/workspace/a.ts', timestamp: 1 }, + ) + const patch = await e.diff() + expect(patch).toContain('@@ -0,0 +1,2 @@') + expect(patch).toContain('+one\n+two\n') + expect(patch).not.toContain('No newline at end of file') + }) + + it('synthesized add-patch for an empty file is header-only (no hunk)', async () => { + const e = buildFileHookEvent( + fakeHandle({ read: async () => '' }), + '/workspace', + '', + { type: 'create', path: '/workspace/empty.ts', timestamp: 1 }, + ) + const patch = await e.diff() + expect(patch).toBe( + 'diff --git a/empty.ts b/empty.ts\nnew file mode 100644\n', + ) + }) + + it('diff() does NOT synthesize when the tracked-ness probe throws (no bogus add-patch on a git hiccup)', async () => { + // git diff comes back empty (looks like a no-op), but the git-show probe + // that would confirm tracked-ness rejects transiently. We must NOT read + // that as "untracked" and fabricate a full-file add-patch for a file that + // is (probably) a tracked, unchanged file. + const { logger, calls } = captureLogger() + const h = fakeHandle({ + read: async () => 'X', + exec: async (cmd: string) => { + if (cmd.startsWith('git check-ignore')) + return { stdout: '', stderr: '', exitCode: 1 } // not ignored + if (cmd.startsWith('git show')) throw new Error('git hiccup') + return { stdout: '', stderr: '', exitCode: 0 } // git diff empty + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'change', path: '/workspace/a.ts', timestamp: 1 }, + logger, + ) + expect(await e.diff()).toBe('') + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('tracked-ness')), + ).toBe(true) + }) + + it('logs a warning when git diff exits non-zero', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + exec: async () => ({ stdout: '', stderr: 'boom', exitCode: 128 }), + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'change', path: '/workspace/a.ts', timestamp: 1 }, + logger, + ) + expect(await e.diff()).toBe('') + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('git diff')), + ).toBe(true) + }) + + it('logs a warning when after() fails to read the file', async () => { + const { logger, calls } = captureLogger() + const h = fakeHandle({ + read: async () => { + throw new Error('nope') + }, + }) + const e = buildFileHookEvent( + h, + '/workspace', + 'sha1', + { type: 'change', path: '/workspace/a.ts', timestamp: 1 }, + logger, + ) + expect(await e.after()).toBe('') + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('after()')), + ).toBe(true) + }) }) diff --git a/packages/ai-sandbox/tests/watch.test.ts b/packages/ai-sandbox/tests/watch.test.ts index 8e748b1b9..a73e14990 100644 --- a/packages/ai-sandbox/tests/watch.test.ts +++ b/packages/ai-sandbox/tests/watch.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { diffSnapshots, watchWorkspace } from '../src/watch' +import { captureLogger } from './fakes' import type { SandboxHandle } from '../src/contracts' import type { FileEvent } from '../src/watch' @@ -110,6 +111,235 @@ describe('watchWorkspace (exec-poll)', () => { ]) }) + it('preserves the previous snapshot when a `find` poll fails (no phantom delete/create storm)', async () => { + vi.useFakeTimers() + // initial (a, b present) → transient non-zero poll → recovery (unchanged). + const results = [ + { stdout: '1.0\t10\t./a.js\n2.0\t20\t./b.js\n', stderr: '', exitCode: 0 }, + { stdout: '', stderr: 'find: not found', exitCode: 127 }, + { stdout: '1.0\t10\t./a.js\n2.0\t20\t./b.js\n', stderr: '', exitCode: 0 }, + ] + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => + Promise.resolve(results[Math.min(call++, results.length - 1)]!) + + const events: Array = [] + const { logger, calls } = captureLogger() + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + logger, + }) + + // Tick 1 = the failed poll (preserve a,b), tick 2 = recovery (unchanged). + await vi.advanceTimersByTimeAsync(250) + await watcher.stop() + + // Collapsing the failed poll to {} would emit delete a/b then re-create + // a/b on recovery. The preserved snapshot emits nothing. + expect(events).toEqual([]) + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('non-zero')), + ).toBe(true) + }) + + it('seeds (does not diff) after a failed INITIAL poll, so recovery emits no phantom creates', async () => { + vi.useFakeTimers() + // The very first poll fails (container warming up) → then recovers with two + // pre-existing files. They must be adopted as the baseline, NOT reported as + // freshly created. + const results = [ + { stdout: '', stderr: 'find: not ready', exitCode: 127 }, // initial fails + { stdout: '1.0\t10\t./a.js\n2.0\t20\t./b.js\n', stderr: '', exitCode: 0 }, + { stdout: '1.0\t10\t./a.js\n2.0\t20\t./b.js\n', stderr: '', exitCode: 0 }, + ] + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => + Promise.resolve(results[Math.min(call++, results.length - 1)]!) + + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + }) + await vi.advanceTimersByTimeAsync(250) + await watcher.stop() + + // Seeding an empty baseline would fabricate a `create` for a.js and b.js. + expect(events).toEqual([]) + }) + + it('does not crash when the INITIAL find exec rejects; recovers on the next poll', async () => { + vi.useFakeTimers() + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => { + call++ + if (call === 1) return Promise.reject(new Error('container not ready')) + return Promise.resolve({ + stdout: '1.0\t10\t./a.js\n', + stderr: '', + exitCode: 0, + }) + } + const events: Array = [] + // Must resolve (not reject) despite the initial exec throwing — otherwise + // middleware setup crashes and leaks the sandbox. + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + }) + await vi.advanceTimersByTimeAsync(250) + await watcher.stop() + // a.js is seeded on recovery, not fabricated as a `create`. + expect(events).toEqual([]) + }) + + it('a partial (non-zero) poll does not fabricate deletes for transiently-missing files', async () => { + vi.useFakeTimers() + // full → partial (c transiently unreadable, exit 1) → full without c (real delete). + const results = [ + { + stdout: '1\t1\t./a.js\n2\t2\t./b.js\n3\t3\t./c.js\n', + stderr: '', + exitCode: 0, + }, + { stdout: '1\t1\t./a.js\n2\t2\t./b.js\n', stderr: 'denied', exitCode: 1 }, + { stdout: '1\t1\t./a.js\n2\t2\t./b.js\n', stderr: '', exitCode: 0 }, + ] + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => + Promise.resolve(results[Math.min(call++, results.length - 1)]!) + + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + }) + await vi.advanceTimersByTimeAsync(120) // tick 1: the partial poll + expect(events).toEqual([]) // c NOT reported deleted despite being absent + + await vi.advanceTimersByTimeAsync(100) // tick 2: complete poll, c really gone + await watcher.stop() + expect(events).toEqual([ + { type: 'delete', path: '/workspace/c.js', timestamp: expect.any(Number) }, + ]) + }) + + it('uses partial `find` output when it exits non-zero but still printed files', async () => { + vi.useFakeTimers() + // `find` hits a permission-denied dir (exit 1) but still prints readable + // files. The watcher must parse that output, not blind itself for the run. + const results = [ + { stdout: '1.0\t10\t./a.js\n', stderr: 'permission denied', exitCode: 1 }, + { + stdout: '1.0\t10\t./a.js\n2.0\t20\t./b.js\n', + stderr: 'permission denied', + exitCode: 1, + }, + ] + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => + Promise.resolve(results[Math.min(call++, results.length - 1)]!) + + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + }) + await vi.advanceTimersByTimeAsync(120) + await watcher.stop() + + // Baseline (a.js) parsed from the non-zero poll; b.js then seen as created. + expect(events).toEqual([ + { type: 'create', path: '/workspace/b.js', timestamp: expect.any(Number) }, + ]) + }) + + it('re-baselines on the first complete poll after a PARTIAL seed, so recovered files are not fabricated as creates', async () => { + vi.useFakeTimers() + // Partial initial seed (b transiently unreadable) → first complete poll + // sees b → it must be adopted as baseline, NOT reported as a create. + const results = [ + { stdout: '1\t1\t./a.js\n', stderr: 'denied', exitCode: 1 }, // partial seed + { stdout: '1\t1\t./a.js\n2\t2\t./b.js\n', stderr: '', exitCode: 0 }, // complete + { stdout: '1\t1\t./a.js\n2\t2\t./b.js\n', stderr: '', exitCode: 0 }, + ] + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => + Promise.resolve(results[Math.min(call++, results.length - 1)]!) + + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + }) + await vi.advanceTimersByTimeAsync(250) + await watcher.stop() + + // b was merely unreadable at seed time; the first complete poll re-baselines + // rather than diffing, so no phantom `create` for b. + expect(events).toEqual([]) + }) + + it('logs a warning when the INITIAL find exec throws', async () => { + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => { + call++ + return call === 1 + ? Promise.reject(new Error('boom')) + : Promise.resolve({ stdout: '', stderr: '', exitCode: 0 }) + } + const { logger, calls } = captureLogger() + // Large interval so no tick fires before we stop — isolates the seed throw. + const watcher = await watchWorkspace(handle, { + onEvent: () => undefined, + intervalMs: 10000, + logger, + }) + await watcher.stop() + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('initial')), + ).toBe(true) + }) + + it('logs (sandbox) and preserves previous when a steady-state tick exec throws', async () => { + vi.useFakeTimers() + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => { + call++ + // initial + tick1 + tick3 succeed with {a}; tick2 (call 3) throws. + return call === 3 + ? Promise.reject(new Error('tick boom')) + : Promise.resolve({ + stdout: '1\t1\t./a.js\n', + stderr: '', + exitCode: 0, + }) + } + const events: Array = [] + const { logger, calls } = captureLogger() + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + logger, + }) + await vi.advanceTimersByTimeAsync(350) + await watcher.stop() + // The throw preserved `previous`; no fabricated delete/create for a.js. + expect(events).toEqual([]) + expect( + calls.some((c) => c.level === 'debug' && c.msg.includes('poll threw')), + ).toBe(true) + }) + it('does not start polling when the signal is already aborted', async () => { const controller = new AbortController() controller.abort() @@ -186,6 +416,67 @@ describe('watchWorkspace (native fs.watch)', () => { expect(events).toEqual([]) }) + it('logs a warning when native event classification fails (no silent drop)', async () => { + let onRaw: (e: { type: string; path: string }) => void = () => undefined + const handle = fakeHandle({ + list: () => Promise.resolve([]), + exists: () => Promise.reject(new Error('exists boom')), + watch: (_p, cb) => { + onRaw = cb + return Promise.resolve({ stop: () => Promise.resolve() }) + }, + }) + const { logger, calls } = captureLogger() + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + logger, + }) + + onRaw({ type: 'change', path: '/workspace/x.js' }) + await flush() + await watcher.stop() + + expect(events).toEqual([]) // classify failed → no event + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('classify')), + ).toBe(true) + }) + + it('re-seeds after a failed initial root list, so a pre-existing file is a change (not a create)', async () => { + let onRaw: (e: { type: string; path: string }) => void = () => undefined + let listCalls = 0 + const handle = fakeHandle({ + // First seed (during watchWorkspace) throws; the lazy re-seed on the + // first event succeeds and lists the pre-existing file. + list: () => { + listCalls++ + return listCalls === 1 + ? Promise.reject(new Error('list not ready')) + : Promise.resolve([ + { name: 'a.ts', path: '/workspace/a.ts', type: 'file' as const }, + ]) + }, + exists: () => Promise.resolve(true), + watch: (_p, cb) => { + onRaw = cb + return Promise.resolve({ stop: () => Promise.resolve() }) + }, + }) + const events: Array = [] + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + }) + + onRaw({ type: 'change', path: '/workspace/a.ts' }) + await flush() + await watcher.stop() + + // Empty seed would classify the edit as `create`; the re-seed makes a.ts + // known, so it's correctly a `change`. + expect(events.map((e) => e.type)).toEqual(['change']) + }) + it('honors a custom root when classifying native events', async () => { const present = new Set() let onRaw: (e: { type: string; path: string }) => void = () => undefined diff --git a/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts b/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts index d941496f5..0ad85d639 100644 --- a/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts +++ b/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts @@ -3,6 +3,7 @@ import { provideSandboxRuntime } from '@tanstack/ai/adapter-internals' import { resolveDebugOption } from '@tanstack/ai/adapter-internals' import { defineSandbox } from '../src/sandbox' import { withSandbox } from '../src/middleware' +import { captureLogger } from './fakes' import type { SandboxFileEvent } from '@tanstack/ai' import type { ChatMiddlewareContext } from '@tanstack/ai' import type { SandboxHandle, SandboxProvider } from '../src/contracts' @@ -10,6 +11,7 @@ import type { SandboxHandle, SandboxProvider } from '../src/contracts' // Fake handle with a native fs.watch we can fire. function fakeHandleAndFire(present: Set) { let onRaw: (e: { type: string; path: string }) => void = () => undefined + let watchedRoot: string | undefined const handle: SandboxHandle = { id: 'fake', provider: 'fake', @@ -34,7 +36,8 @@ function fakeHandleAndFire(present: Set) { remove: () => Promise.resolve(), rename: () => Promise.resolve(), exists: (p) => Promise.resolve(present.has(p)), - watch: (_p, cb) => { + watch: (p, cb) => { + watchedRoot = p onRaw = cb return Promise.resolve({ stop: () => Promise.resolve() }) }, @@ -48,7 +51,11 @@ function fakeHandleAndFire(present: Set) { env: { set: () => Promise.resolve() }, destroy: () => Promise.resolve(), } - return { handle, fire: (e: { type: string; path: string }) => onRaw(e) } + return { + handle, + fire: (e: { type: string; path: string }) => onRaw(e), + watchedRoot: () => watchedRoot, + } } // Fake handle whose `fs.list` seeds the watcher's known-path set (so the @@ -114,6 +121,73 @@ function fakeHandleWithGit( return { handle, fire: (e: { type: string; path: string }) => onRaw(e) } } +type ExecResult = { stdout: string; stderr: string; exitCode: number } + +// Native-watch handle whose `git diff` exec stays PENDING until `releaseDiff` +// is called — lets a test hold a `diff()` in flight across a teardown hook to +// prove the hook awaits `pendingDiffs` (the drain) before returning. +function fakeHandleDeferredDiff(knownPath: string) { + let onRaw: (e: { type: string; path: string }) => void = () => undefined + let releaseDiff: (patch: string) => void = () => undefined + const pendingDiff = new Promise((resolve) => { + releaseDiff = (patch) => + resolve({ stdout: patch, stderr: '', exitCode: 0 }) + }) + const handle: SandboxHandle = { + id: 'fake', + provider: 'fake', + capabilities: { + fs: true, + exec: true, + env: true, + ports: false, + backgroundProcesses: false, + writableStdin: true, + snapshots: false, + networkPolicy: false, + durableFilesystem: false, + fork: false, + }, + fs: { + read: () => Promise.resolve('AFTER'), + readBytes: () => Promise.reject(new Error('x')), + write: () => Promise.resolve(), + list: (dir) => + Promise.resolve( + dir === '/workspace' + ? [{ name: 'x.ts', path: knownPath, type: 'file' as const }] + : [], + ), + mkdir: () => Promise.resolve(), + remove: () => Promise.resolve(), + rename: () => Promise.resolve(), + exists: () => Promise.resolve(true), + watch: (_p, cb) => { + onRaw = cb + return Promise.resolve({ stop: () => Promise.resolve() }) + }, + }, + git: {} as SandboxHandle['git'], + process: { + exec: (cmd: string) => { + if (cmd.startsWith('git rev-parse')) + return Promise.resolve({ stdout: 'sha1\n', stderr: '', exitCode: 0 }) + if (cmd.startsWith('git diff')) return pendingDiff + return Promise.resolve({ stdout: '', stderr: '', exitCode: 0 }) + }, + spawn: () => Promise.reject(new Error('x')), + }, + ports: { connect: () => Promise.reject(new Error('x')) }, + env: { set: () => Promise.resolve() }, + destroy: () => Promise.resolve(), + } + return { + handle, + fire: (e: { type: string; path: string }) => onRaw(e), + releaseDiff: (patch: string) => releaseDiff(patch), + } +} + function fakeProvider(handle: SandboxHandle): SandboxProvider { return { name: 'fake', @@ -135,6 +209,23 @@ function makeCtx(): ChatMiddlewareContext { const flush = () => new Promise((r) => setTimeout(r, 5)) +type ReturnedMiddleware = ReturnType + +/** Invoke the terminal hook for a given lifecycle phase. */ +function invokeTerminal( + mw: ReturnedMiddleware, + ctx: ChatMiddlewareContext, + phase: 'finish' | 'abort' | 'error', +): Promise { + if (phase === 'finish') + return Promise.resolve( + mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }), + ) + if (phase === 'abort') + return Promise.resolve(mw.onAbort!(ctx, { reason: 'x', duration: 0 })) + return Promise.resolve(mw.onError!(ctx, { error: new Error('x'), duration: 0 })) +} + describe('withSandbox hooks', () => { it('fires defineSandbox file hooks and emits via the runtime sink', async () => { const present = new Set() @@ -250,4 +341,179 @@ describe('withSandbox hooks', () => { await mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }) }) + + // The drain (`await Promise.allSettled(state.pendingDiffs)`) exists so a + // diff still in flight when the run ends isn't dropped. Deleting it from any + // teardown path would silently lose the final file's diff — these tests fail + // if that happens, by holding a `diff()` pending across the hook. + for (const phase of ['finish', 'abort', 'error'] as const) { + it(`awaits an in-flight diff before ${phase} teardown returns`, async () => { + const path = '/workspace/x.ts' + const { handle, fire, releaseDiff } = fakeHandleDeferredDiff(path) + const diffs: Array<{ path: string; diff: string }> = [] + const sandbox = defineSandbox({ + id: 's', + provider: fakeProvider(handle), + fileEvents: { diff: true }, + }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger: resolveDebugOption(false), + emit: () => undefined, + emitFileDiff: (v) => void diffs.push(v), + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + + fire({ type: 'change', path }) + await flush() // event dispatched, diff() called — git-diff exec pending + expect(diffs).toEqual([]) // diff not resolved yet + + let done = false + const terminal = invokeTerminal(mw, ctx, phase).then(() => { + done = true + }) + await flush() + // Without the drain, the hook resolves here — before the pending diff. + expect(done).toBe(false) + expect(diffs).toEqual([]) + + releaseDiff('PATCH') + await terminal + expect(done).toBe(true) + expect(diffs).toEqual([{ path, diff: 'PATCH' }]) // drained, not dropped + }) + } + + it('logs (sandbox) when git baseline capture exits non-zero (non-git repo)', async () => { + const { handle } = fakeHandleWithGit('/workspace/x.ts', { + 'git rev-parse HEAD': { + stdout: '', + stderr: 'fatal: not a git repository', + exitCode: 128, + }, + }) + const { logger, calls } = captureLogger() + const sandbox = defineSandbox({ id: 's', provider: fakeProvider(handle) }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger, + emit: () => undefined, + emitFileDiff: () => undefined, + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + // Non-zero exit (git ran, no repo/HEAD) logs under `sandbox` (debug level). + expect( + calls.some( + (c) => c.level === 'debug' && c.msg.includes('baseline unavailable'), + ), + ).toBe(true) + await mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }) + }) + + it('logs a warning when git baseline capture fails', async () => { + // fakeHandleAndFire's process.exec rejects, so `git rev-parse HEAD` throws. + const { handle } = fakeHandleAndFire(new Set()) + const { logger, calls } = captureLogger() + const sandbox = defineSandbox({ id: 's', provider: fakeProvider(handle) }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger, + emit: () => undefined, + emitFileDiff: () => undefined, + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + expect( + calls.some((c) => c.level === 'warn' && c.msg.includes('baseline')), + ).toBe(true) + await mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }) + }) + + it('logs (does not silently swallow) a throwing file hook', async () => { + const present = new Set() + const { handle, fire } = fakeHandleAndFire(present) + const { logger, calls } = captureLogger() + const sandbox = defineSandbox({ + id: 's', + provider: fakeProvider(handle), + hooks: { + onFileCreate: () => { + throw new Error('bad hook') + }, + }, + }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger, + emit: () => undefined, + emitFileDiff: () => undefined, + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + + present.add('/workspace/new.ts') + fire({ type: 'rename', path: '/workspace/new.ts' }) + await flush() + + expect( + calls.some((c) => c.level === 'error' && c.msg.includes('hook failed')), + ).toBe(true) + await mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }) + }) + + it('watches the definition workspace.root (not the default) so the watcher and enrichment agree', async () => { + const { handle, watchedRoot } = fakeHandleAndFire(new Set()) + const sandbox = defineSandbox({ + id: 's', + provider: fakeProvider(handle), + workspace: { root: '/repo', source: { type: 'none' } }, + }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger: resolveDebugOption(false), + emit: () => undefined, + emitFileDiff: () => undefined, + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + // Without passing `root`, the watcher would default to '/workspace' while + // enrichment relativizes against '/repo' — the two would diverge. + expect(watchedRoot()).toBe('/repo') + await mw.onFinish!(ctx, { finishReason: 'stop', duration: 0, content: '' }) + }) + + it('still destroys the sandbox on abort when the watcher stop() rejects', async () => { + const { handle, fire } = fakeHandleAndFire(new Set()) + // Make the watcher's subscription.stop() reject. + const origWatch = handle.fs.watch! + handle.fs.watch = (p, cb) => + origWatch(p, cb).then(() => ({ + stop: () => Promise.reject(new Error('stop boom')), + })) + let destroyed = 0 + const sandbox = defineSandbox({ + id: 's', + provider: { + ...fakeProvider(handle), + destroy: () => { + destroyed++ + return Promise.resolve() + }, + }, + }) + const ctx = makeCtx() + provideSandboxRuntime(ctx, { + logger: resolveDebugOption(false), + emit: () => undefined, + emitFileDiff: () => undefined, + }) + const mw = withSandbox(sandbox) + await mw.setup!(ctx) + void fire // watcher active + // onAbort must ALWAYS destroy — a rejecting stop() must not skip it. + await mw.onAbort!(ctx, { reason: 'x', duration: 0 }) + expect(destroyed).toBe(1) + }) }) From 5635eb50cf067ba72f1c48ba99042ae0a34fe760 Mon Sep 17 00:00:00 2001 From: Alem Tuzlak Date: Wed, 8 Jul 2026 12:13:11 +0200 Subject: [PATCH 2/4] docs(sandbox): document file-diff observability behavior 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. --- docs/config.json | 2 +- docs/sandbox/observability.md | 19 ++++++++++++++++- .../ai/skills/ai-core/middleware/SKILL.md | 21 ++++++++++++++++--- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/docs/config.json b/docs/config.json index ceb80c4f7..c30c1ad2b 100644 --- a/docs/config.json +++ b/docs/config.json @@ -386,7 +386,7 @@ "label": "Observability", "to": "sandbox/observability", "addedAt": "2026-06-29", - "updatedAt": "2026-07-03" + "updatedAt": "2026-07-08" }, { "label": "Cloudflare (Edge)", diff --git a/docs/sandbox/observability.md b/docs/sandbox/observability.md index 950413629..2b7039d0a 100644 --- a/docs/sandbox/observability.md +++ b/docs/sandbox/observability.md @@ -144,7 +144,24 @@ 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 `''`. +resolves to `''`. In a git workspace, a file git **isn't tracking yet** — one +the agent just created, and every later edit to it — diffs empty because +`git diff` ignores untracked files, so `diff()` falls back to that same +synthesized add-patch whenever the file is absent at the baseline. A file the +agent creates (and keeps editing) therefore never streams an empty diff, while +a **tracked** file that's identical to the baseline correctly stays empty. +A **git-ignored** file (e.g. a `.env` or a credentials file) is the exception: +the file event still fires so you're notified it changed, but `diff()` returns +`''` so its contents are never surfaced in the diff feed. + +Every git/exec/fs failure behind these accessors — and behind the `find`-poll +watcher — still falls back to `''` (or preserves the last snapshot), but is +**logged first** so a failure is observable rather than a silent empty value: +real anomalies (a failed `git diff`, an unreadable file, a `find` poll that +exits non-zero, a lost git baseline) under the `errors` category (on by +default), and expected-empty conditions (a new file's `before()`) under the +`sandbox` debug category. Enable `debug: { sandbox: true }` (see +[Debugging](#debugging)) to see the latter. ## Disabling file watching diff --git a/packages/ai/skills/ai-core/middleware/SKILL.md b/packages/ai/skills/ai-core/middleware/SKILL.md index 16fc91413..65c6bf7b8 100644 --- a/packages/ai/skills/ai-core/middleware/SKILL.md +++ b/packages/ai/skills/ai-core/middleware/SKILL.md @@ -453,11 +453,26 @@ accessors throw: a deleted file resolves `after()` to `''` (it still has 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 `''`. +synthesize and `diff()` resolves to `''`. In a git workspace a file git +**isn't tracking yet** (a file the agent created, and every later edit to it) +diffs empty because `git diff` ignores untracked files, so `diff()` falls +back to the same synthesized add-patch whenever the file is absent at the +baseline — a create-or-edit of an untracked file never streams an empty diff. +An empty diff for a **tracked** file (identical to the baseline) stays empty, +as it should. A **git-ignored** file is withheld: the file event still fires +(you're notified it changed) but `diff()` returns `''`, so a secret like a +`.env` never has its contents surfaced in the diff feed. + +**Failures are logged, not silent.** Every git/exec/fs failure behind these +accessors (and behind the `find`-poll watcher) still falls back to `''`/an +empty snapshot, but logs first: real anomalies (a failed `git diff`, an +unreadable file, a lost `find` poll) under the `errors` category (on by +default); expected-empty conditions (a new file's `before()`, a non-git +baseline) under the `sandbox` debug category. **Hook errors are swallowed per hook.** A throwing `sandbox` hook is caught -and logged under the `sandbox` debug category — it cannot break the run or -stop other hooks (or the `sandbox.file` chunk) from continuing. +and logged under the `errors` category (on by default) — it cannot break the +run or stop other hooks (or the `sandbox.file` chunk) from continuing. Source: docs/sandbox/observability.md From ea598c76f0b315134e0dbd788829c9f7d755d6bd Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:15:01 +0000 Subject: [PATCH 3/4] ci: apply automated fixes --- packages/ai-sandbox/src/watch.ts | 14 ++++++++++---- packages/ai-sandbox/tests/fakes.ts | 3 ++- packages/ai-sandbox/tests/file-diff.test.ts | 6 ++++-- packages/ai-sandbox/tests/watch.test.ts | 12 ++++++++++-- .../ai-sandbox/tests/with-sandbox-hooks.test.ts | 7 ++++--- 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/packages/ai-sandbox/src/watch.ts b/packages/ai-sandbox/src/watch.ts index 22665afaf..56b6d241e 100644 --- a/packages/ai-sandbox/src/watch.ts +++ b/packages/ai-sandbox/src/watch.ts @@ -176,9 +176,12 @@ async function startNativeWatch( if (r.rootOk) { for (const p of r.files) known.add(p) seeded = true - logger?.sandbox('sandbox watch: re-seeded after failed initial seed', { - root, - }) + logger?.sandbox( + 'sandbox watch: re-seeded after failed initial seed', + { + root, + }, + ) } reseeding = null }) @@ -275,7 +278,10 @@ async function startPollWatch( // missing, container never ready — that would leave the watcher dead for // the whole run, so surface it at `warn`. if (isInitial) { - logger?.warn('sandbox watch: initial `find` poll threw', { root, error }) + logger?.warn('sandbox watch: initial `find` poll threw', { + root, + error, + }) } else { logger?.sandbox('sandbox watch: `find` poll threw', { root, error }) } diff --git a/packages/ai-sandbox/tests/fakes.ts b/packages/ai-sandbox/tests/fakes.ts index db4772908..bfae86670 100644 --- a/packages/ai-sandbox/tests/fakes.ts +++ b/packages/ai-sandbox/tests/fakes.ts @@ -242,7 +242,8 @@ export function captureLogger(): { const calls: Array<{ level: string; msg: string }> = [] const rec = (level: string) => - (msg: string): void => void calls.push({ level, msg }) + (msg: string): void => + void calls.push({ level, msg }) const logger = resolveDebugOption({ logger: { debug: rec('debug'), diff --git a/packages/ai-sandbox/tests/file-diff.test.ts b/packages/ai-sandbox/tests/file-diff.test.ts index a581df5ac..0d025faff 100644 --- a/packages/ai-sandbox/tests/file-diff.test.ts +++ b/packages/ai-sandbox/tests/file-diff.test.ts @@ -222,7 +222,8 @@ describe('buildFileHookEvent', () => { const h = fakeHandle({ read: async () => 'x\n', exec: async (cmd: string) => { - if (cmd.startsWith('git check-ignore')) throw new Error('check-ignore boom') + if (cmd.startsWith('git check-ignore')) + throw new Error('check-ignore boom') if (cmd.startsWith('git show')) return { stdout: '', stderr: 'fatal', exitCode: 128 } // untracked return { stdout: '', stderr: '', exitCode: 0 } // git diff empty @@ -293,7 +294,8 @@ describe('buildFileHookEvent', () => { expect(patch).toContain('+q') expect( calls.some( - (c) => c.level === 'debug' && c.msg.includes('tracked-ness probe non-zero'), + (c) => + c.level === 'debug' && c.msg.includes('tracked-ness probe non-zero'), ), ).toBe(true) }) diff --git a/packages/ai-sandbox/tests/watch.test.ts b/packages/ai-sandbox/tests/watch.test.ts index a73e14990..d8d210dec 100644 --- a/packages/ai-sandbox/tests/watch.test.ts +++ b/packages/ai-sandbox/tests/watch.test.ts @@ -225,7 +225,11 @@ describe('watchWorkspace (exec-poll)', () => { await vi.advanceTimersByTimeAsync(100) // tick 2: complete poll, c really gone await watcher.stop() expect(events).toEqual([ - { type: 'delete', path: '/workspace/c.js', timestamp: expect.any(Number) }, + { + type: 'delete', + path: '/workspace/c.js', + timestamp: expect.any(Number), + }, ]) }) @@ -256,7 +260,11 @@ describe('watchWorkspace (exec-poll)', () => { // Baseline (a.js) parsed from the non-zero poll; b.js then seen as created. expect(events).toEqual([ - { type: 'create', path: '/workspace/b.js', timestamp: expect.any(Number) }, + { + type: 'create', + path: '/workspace/b.js', + timestamp: expect.any(Number), + }, ]) }) diff --git a/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts b/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts index 0ad85d639..bba885bdf 100644 --- a/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts +++ b/packages/ai-sandbox/tests/with-sandbox-hooks.test.ts @@ -130,8 +130,7 @@ function fakeHandleDeferredDiff(knownPath: string) { let onRaw: (e: { type: string; path: string }) => void = () => undefined let releaseDiff: (patch: string) => void = () => undefined const pendingDiff = new Promise((resolve) => { - releaseDiff = (patch) => - resolve({ stdout: patch, stderr: '', exitCode: 0 }) + releaseDiff = (patch) => resolve({ stdout: patch, stderr: '', exitCode: 0 }) }) const handle: SandboxHandle = { id: 'fake', @@ -223,7 +222,9 @@ function invokeTerminal( ) if (phase === 'abort') return Promise.resolve(mw.onAbort!(ctx, { reason: 'x', duration: 0 })) - return Promise.resolve(mw.onError!(ctx, { error: new Error('x'), duration: 0 })) + return Promise.resolve( + mw.onError!(ctx, { error: new Error('x'), duration: 0 }), + ) } describe('withSandbox hooks', () => { From a45094ce9e6baf570290ba3e6e3ee7223eccc50e Mon Sep 17 00:00:00 2001 From: Tom Beckenham <34339192+tombeckenham@users.noreply.github.com> Date: Thu, 9 Jul 2026 20:58:04 +1000 Subject: [PATCH 4/4] fix(sandbox): escalate persistent find-poll throws to warn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- packages/ai-sandbox/src/watch.ts | 36 ++++++++++++++++++++----- packages/ai-sandbox/tests/watch.test.ts | 32 ++++++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/packages/ai-sandbox/src/watch.ts b/packages/ai-sandbox/src/watch.ts index 56b6d241e..bf7c858d4 100644 --- a/packages/ai-sandbox/src/watch.ts +++ b/packages/ai-sandbox/src/watch.ts @@ -260,6 +260,9 @@ async function startPollWatch( /** `false` when `find` exited non-zero but still printed rows (partial). */ complete: boolean } + // Escalate a steady-state poll throw to `warn` after this many in a row. + const STEADY_STATE_THROW_WARN_AFTER = 3 + let consecutiveThrows = 0 const snapshot = async (isInitial = false): Promise => { let result try { @@ -267,23 +270,42 @@ async function startPollWatch( cwd: root, signal: controller.signal, }) + consecutiveThrows = 0 // exec returned (any exit code) — the seam is alive } catch (error) { // Thrown exec — container not ready, `find` seam rejects, or a // mid-teardown abort. Treat as a failed poll so BOTH the initial seed // and every tick preserve `previous` instead of rejecting setup (which - // would crash the run and leak the sandbox) or the interval. A steady- - // state throw is usually a transient/teardown blip → `sandbox`. But the - // INITIAL poll can't be a teardown (a pre-aborted signal is guarded in - // `watchWorkspace`), so a throw there is an unambiguous anomaly — `find` - // missing, container never ready — that would leave the watcher dead for - // the whole run, so surface it at `warn`. + // would crash the run and leak the sandbox) or the interval. if (isInitial) { + // The INITIAL poll can't be a teardown (a pre-aborted signal is guarded + // in `watchWorkspace`), so a throw here is an unambiguous anomaly (`find` + // missing, container never ready) that leaves the watcher dead for the + // whole run — surface it at `warn`. logger?.warn('sandbox watch: initial `find` poll threw', { root, error, }) + } else if (controller.signal.aborted) { + // Mid-teardown abort — expected, stay quiet. + logger?.sandbox('sandbox watch: `find` poll threw during teardown', { + root, + error, + }) } else { - logger?.sandbox('sandbox watch: `find` poll threw', { root, error }) + // Steady-state throw while NOT tearing down. One is usually a transient + // blip (→ `sandbox`), but a run of them means the exec seam is wedged: + // every poll returns null and the watcher emits nothing for the rest of + // the run. That silent-death case escalates to `warn` (on by default). + consecutiveThrows += 1 + if (consecutiveThrows >= STEADY_STATE_THROW_WARN_AFTER) { + logger?.warn('sandbox watch: `find` poll threw repeatedly', { + root, + error, + consecutiveThrows, + }) + } else { + logger?.sandbox('sandbox watch: `find` poll threw', { root, error }) + } } return null } diff --git a/packages/ai-sandbox/tests/watch.test.ts b/packages/ai-sandbox/tests/watch.test.ts index d8d210dec..ec2321164 100644 --- a/packages/ai-sandbox/tests/watch.test.ts +++ b/packages/ai-sandbox/tests/watch.test.ts @@ -346,6 +346,38 @@ describe('watchWorkspace (exec-poll)', () => { expect( calls.some((c) => c.level === 'debug' && c.msg.includes('poll threw')), ).toBe(true) + // A single transient throw must NOT escalate to warn. + expect(calls.some((c) => c.level === 'warn')).toBe(false) + }) + + it('escalates to a warning when `find` polls throw persistently (watcher wedged)', async () => { + vi.useFakeTimers() + let call = 0 + const handle = fakeHandle({}) + handle.process.exec = () => { + call++ + // Initial poll seeds {a}; every steady-state tick then throws (seam wedged). + return call === 1 + ? Promise.resolve({ stdout: '1\t1\t./a.js\n', stderr: '', exitCode: 0 }) + : Promise.reject(new Error('exec wedged')) + } + const events: Array = [] + const { logger, calls } = captureLogger() + const watcher = await watchWorkspace(handle, { + onEvent: (e) => events.push(e), + intervalMs: 100, + logger, + }) + // 4 ticks all throw → crosses the 3-in-a-row escalation threshold. + await vi.advanceTimersByTimeAsync(450) + await watcher.stop() + // Still no fabricated events (previous preserved), but now surfaced at warn. + expect(events).toEqual([]) + expect( + calls.some( + (c) => c.level === 'warn' && c.msg.includes('poll threw repeatedly'), + ), + ).toBe(true) }) it('does not start polling when the signal is already aborted', async () => {