Skip to content

refuse test writes into the install's real data/ tree at runtime - #6202

Merged
atomantic merged 6 commits into
mainfrom
claim/issue-6176
Sep 4, 2026
Merged

refuse test writes into the install's real data/ tree at runtime#6202
atomantic merged 6 commits into
mainfrom
claim/issue-6176

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

A test that writes into the developer's live data/ tree is invisible: unlike a read leak, persisting fixture bytes over their records changes nothing about whether assertions pass. #6171 was the proof — providerUsage.test.js wrote its fixture quota cards over the machine's real data/provider-quotas.json, then invalidated the sync checksum so peers pulled the fabricated values.

This closes that half at runtime, the way db.js already refuses row writes to a non-test database.

  • server/lib/testDataIsolation.js throws when a write resolves inside the real data root under the test runner, naming the offending path and the createTempDataRoot() escape hatch. It fires from atomicWrite, ensureDir's create path (an existing directory is a no-op, so read-only suites stay quiet), new writeFile/appendFile/copyFile wrappers in fileCore.js, and collectionStore's record delete — so the next raw writer inherits the guard rather than having to remember an import. aiToolkit/ is untouched, per its self-containment rule.
  • Inert in production, and cheaper than that: the guard module loads through a memoized await import() behind isTestRunner(), so a production process never loads it at all. The tree-wide static-import budget in importScoping.test.js is unchanged — no ceiling was raised.
  • Symlinked destinations are judged at both landing sites. atomicWrite replaces a symlink; writeFile/appendFile/copyFile follow one. Checking only the link's own location would have left the guard's own hole open.

Leaks it found on its first run

Suite What it was writing into the live install
backup.test.js vi.doUnmock also drops the file-level vi.mock, so every later suite resolved PATHS.data to the real tree and runBackup rewrote the user's genuine data/backup/state.json on every run
providers.readiness.test.js, loraTraining/captionLeakStaging.test.js took the machine-wide heavy-local-job claim — the file that gates the user's real local renders
sharing/integration.test.js a hand-rolled PATHS proxy listed data/images/videos by hand, so the later-added imageRefs still pointed at data/image-refs and the importer copied bundled reference sheets there
assetHash.test.js, modelAbuseGuard.materialize.test.js wrote fixtures into the real tree deliberately, swept up only when nothing above them threw

Each is fixed by redirecting the data root, not by silencing the guard.

Supporting changes

  • isTestRunner moves out of db.js into a dependency-free lib/runtimeEnv.js. The file primitives must not pull in pg, and the many suites spelling vi.mock('../lib/db.js', () => ({ query })) were stripping it out of the module graph for every other consumer.
  • lib/pathContainment.js owns root-inclusive containment and ancestor-canonicalization, kept a leaf so fileCore reaches it without dragging errorHandler/paths into every suite's closure.
  • mockPathsDataRoot.js gains lazyTempDataRoot / cleanupTempDataRoots, so the vi.mock-hoisting hazard is explained once instead of in each suite that redirects a root.
  • agentRunTracking.js, autonomousJobs/store.js, cosState.js, sharing/importer.js and runner.js move from raw fs writes onto the guarded primitives.

Scope, stated rather than implied

Writes only — reads stay covered by the two-run probe. Roughly forty services still reach PATHS.* with raw fs, and on a populated install their directories already exist, so ensureDir's create-path check is a no-op for them. Both limits are written into the module header rather than glossed over; the sweep is tracked separately.

Test plan

  • New server/lib/testDataIsolation.test.js (22 cases) asserts the thrown message, not merely that something threw: a temp root writes fine, the real root refuses with the path and the escape hatch named, the guard is inert outside the runner, and a data-archive sibling, a .. climb, a relative path, a symlinked destination and a filesystem root all resolve correctly.
  • Full server suite: 38,945 passing. Full client suite re-run. The handful of remaining failures are timeouts under load that reproduce identically on origin/main — verified against a pristine baseline worktree.
  • importScoping.test.js, index.test.js (barrel/README catalogs) and the existing static testDataIsolation.guards.test.js all pass.

Closes #6176

@atomantic

Copy link
Copy Markdown
Owner Author

Required code review was not completed before publication. This PR is intentionally left open and will not be merged until the required review completes.

@atomantic

atomantic commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

Local review status

Configured reviewers for this run:claude.

  • claude — completed. Returned three findings, all confirmed against the code and fixed in f612163:
    1. writeFile/appendFile/copyFile follow a symlinked destination, but the guard judged only the link's own location — a fixture symlink in a temp root could write straight through into live data. Both landing sites are now checked, resolved via readlink + canonicalizePath so a dangling link still reports its target (realpathSync throws on those).
    2. isPathAtOrInsideDir reported nothing as inside a filesystem/drive root — those already end in the separator, so the anchor compared against //.
    3. The ..-climb case in the guard's own test was vacuous: path.join normalizes its arguments, so the assertion held even with resolve() removed.

The claude reviewer's first invocation was killed by system memory pressure with no output; it was retried once and completed normally.

A suite that writes into the developer's live `data/` is invisible: unlike a
read leak, persisting fixture bytes over their records changes nothing about
whether assertions pass. #6171 was the proof — `providerUsage.test.js` wrote its
fixture quota cards over the machine's real `data/provider-quotas.json`, then
invalidated the sync checksum so peers pulled the fabricated values.

Close that half at runtime, the way `db.js` already refuses row writes to a
non-test database. `lib/testDataIsolation.js` throws when a write resolves
inside the real data root under the test runner, naming the path and the
`createTempDataRoot()` escape hatch. It fires from `atomicWrite`, `ensureDir`'s
create path (an existing dir is a no-op, so read-only suites stay quiet), new
`writeFile`/`appendFile`/`copyFile` wrappers in `fileCore.js`, and
`collectionStore`'s record delete — so the next raw writer inherits the guard
instead of having to remember an import. `aiToolkit/` is untouched, per its
self-containment rule.

It found six real leaks on its first run:

  - `backup.test.js` — `vi.doUnmock` also drops the file-level `vi.mock`, so
    every later suite resolved PATHS.data to the real tree and `runBackup`
    rewrote the user's genuine `data/backup/state.json` on every run.
  - `providers.readiness.test.js` and `loraTraining/captionLeakStaging.test.js`
    — took the MACHINE-WIDE heavy-local-job claim in the live tree, the file
    that gates the user's real local renders.
  - `sharing/integration.test.js` — a hand-rolled PATHS proxy listed `data`,
    `images` and `videos` by hand, so the later-added `imageRefs` still pointed
    at `data/image-refs` and the importer copied bundled sheets there.
  - `assetHash.test.js` and `modelAbuseGuard.materialize.test.js` — wrote
    fixtures into the real tree deliberately, swept up only when nothing threw.

Supporting changes: `isTestRunner` moves out of `db.js` into a dependency-free
`lib/runtimeEnv.js` (the file primitives must not pull in `pg`, and the many
suites spelling `vi.mock('../lib/db.js', () => ({ query }))` were stripping it
out of the graph for every other consumer); `lib/pathContainment.js` owns the
root-inclusive containment and ancestor-canonicalizing helpers, kept a leaf so
`fileCore` reaches them without dragging `errorHandler`/`paths` into every
suite's closure; the guard itself loads through a memoized `await import()`
behind `isTestRunner()`, so production never loads it and the tree-wide import
budget is unchanged. `mockPathsDataRoot.js` gains `lazyTempDataRoot` /
`cleanupTempDataRoots` so the hoisting hazard is explained once rather than in
each suite that redirects a data root.

Writes only: reads stay covered by the two-run probe, and roughly forty services
still reach `PATHS.*` with raw `fs`. Both limits are stated in the module header
rather than implied away, with the sweep tracked separately.
…ment

Local review found three defects in the guard shipped in the previous commit.

The one that mattered: `writeFile`, `appendFile` and `copyFile` FOLLOW a
symlinked destination, but the guard judged only the link's own location. A
fixture symlink inside a temp root pointing at `data/provider-quotas.json`
therefore wrote straight through into live data — the exact leak the guard
exists to stop. Both landing sites are now checked, resolved through `readlink`
+ `canonicalizePath` rather than `realpathSync` so a DANGLING link (one naming a
file the real tree has not created yet) still reports its target.

`isPathAtOrInsideDir` also reported nothing as inside a filesystem or drive
root: those already end in the separator, so anchoring on `root + sep` compared
against `//`. Unreachable from today's `<install>/data` root, but the helper is
a new public export whose contract promises otherwise.

And the `..`-climb case in the guard's own test was vacuous — `path.join`
normalizes its arguments, so the value under test arrived already collapsed and
the assertion held even with `resolve()` removed. Rebuilt by concatenation,
alongside new relative-path, symlink and filesystem-root cases.

Also routes `services/runner.js`'s raw write of `data/runs/<id>/output.txt`
through the guarded wrapper (`agentRunTracking.js` writes the same file), and
exempts the guard's contract test from the static isolation rule: it has to name
the real root to prove a refusal, and every assertion against a real path
asserts the call rejects.
The rebase onto main tightened importScoping.test.js's tree-wide budget
just enough that this file's static import of testDataIsolation.js (and
its pathContainment.js closure) pushed the suite to 85,129 instantiations,
85 over the 85,000 ceiling. fileCore.js already lazy-loads the same guard
behind isTestRunner() for exactly this reason; mirror that pattern here
instead of raising the budget.
@atomantic
atomantic enabled auto-merge September 4, 2026 05:16
…/ leak

CI caught this after the rebase: the annotated-regen route stages its
init-image snapshot under PATHS.imageRefs (ensureDir + write), and this
suite never redirected PATHS away from the real install tree, so it wrote
that snapshot into the developer's live data/image-refs on every run. It
only looked green locally because that directory already existed from
prior runs — ensureDir's create-path guard is a no-op against an existing
dir, so the write went unnoticed until a fresh checkout (or a first-time
directory) exposed it. This is the same class of leak #6176 already fixed
in six other files; this one's #7.

Also mocks lib/paths.js alongside lib/fileUtils.js: pathSafety.js's
resolveGalleryImage/resolveImageRef/resolveImageInputPath read PATHS from
paths.js directly, so the fileUtils.js redirect alone left the runner's
own re-validation of the staged path checking against the real root.
CI's smoke-boot step (npm run smoke) starts the real server and
deliberately sets NODE_ENV=test to select the file-backend escape hatch
documented in AGENTS.md — it is not a Vitest suite. The #6176 write guard
gated on isTestRunner() (NODE_ENV==='test' OR VITEST), so that legitimate
real boot got treated as a test writing into its own data/ tree and every
startup write (usage.json, instances.json, cos/, brain/, voice-timers.json,
loops) was refused, crashing the smoke-boot job in CI.

Add isVitestRunner() (VITEST only) and use it everywhere the guard decides
whether to fire: fileCore.js's ensureDir/atomicWrite/writeFile/appendFile/
copyFile, collectionStore's record delete, and userActions.js's local
read+write guard. Left isTestRunner() itself, and the backend-selection
call sites that key on it (postRunStore.js, userActions.js's isFile,
sprites/records.js, db.js's non-test-database refusal), unchanged — those
correctly want the broader "NODE_ENV=test OR VITEST" signal.

Verified: `npm run smoke` now boots clean, and the guard still throws for
an actual Vitest suite writing outside its redirected data root.
@atomantic
atomantic merged commit 0e78ab7 into main Sep 4, 2026
12 checks passed
@atomantic
atomantic deleted the claim/issue-6176 branch September 4, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Guard against tests WRITING into the real data/ tree (the half the isolation probe can't see)

1 participant