refuse test writes into the install's real data/ tree at runtime - #6202
Merged
Conversation
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. |
Owner
Author
Local review statusConfigured reviewers for this run:
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
force-pushed
the
claim/issue-6176
branch
from
September 4, 2026 05:14
f612163 to
3d2914d
Compare
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.
This was referenced Sep 4, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.jswrote its fixture quota cards over the machine's realdata/provider-quotas.json, then invalidated the sync checksum so peers pulled the fabricated values.This closes that half at runtime, the way
db.jsalready refuses row writes to a non-test database.server/lib/testDataIsolation.jsthrows when a write resolves inside the real data root under the test runner, naming the offending path and thecreateTempDataRoot()escape hatch. It fires fromatomicWrite,ensureDir's create path (an existing directory is a no-op, so read-only suites stay quiet), newwriteFile/appendFile/copyFilewrappers infileCore.js, andcollectionStore'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.await import()behindisTestRunner(), so a production process never loads it at all. The tree-wide static-import budget inimportScoping.test.jsis unchanged — no ceiling was raised.atomicWritereplaces a symlink;writeFile/appendFile/copyFilefollow one. Checking only the link's own location would have left the guard's own hole open.Leaks it found on its first run
backup.test.jsvi.doUnmockalso drops the file-levelvi.mock, so every later suite resolvedPATHS.datato the real tree andrunBackuprewrote the user's genuinedata/backup/state.jsonon every runproviders.readiness.test.js,loraTraining/captionLeakStaging.test.jssharing/integration.test.jsdata/images/videosby hand, so the later-addedimageRefsstill pointed atdata/image-refsand the importer copied bundled reference sheets thereassetHash.test.js,modelAbuseGuard.materialize.test.jsEach is fixed by redirecting the data root, not by silencing the guard.
Supporting changes
isTestRunnermoves out ofdb.jsinto a dependency-freelib/runtimeEnv.js. The file primitives must not pull inpg, and the many suites spellingvi.mock('../lib/db.js', () => ({ query }))were stripping it out of the module graph for every other consumer.lib/pathContainment.jsowns root-inclusive containment and ancestor-canonicalization, kept a leaf sofileCorereaches it without draggingerrorHandler/pathsinto every suite's closure.mockPathsDataRoot.jsgainslazyTempDataRoot/cleanupTempDataRoots, so thevi.mock-hoisting hazard is explained once instead of in each suite that redirects a root.agentRunTracking.js,autonomousJobs/store.js,cosState.js,sharing/importer.jsandrunner.jsmove from rawfswrites 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 rawfs, and on a populated install their directories already exist, soensureDir'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
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 adata-archivesibling, a..climb, a relative path, a symlinked destination and a filesystem root all resolve correctly.origin/main— verified against a pristine baseline worktree.importScoping.test.js,index.test.js(barrel/README catalogs) and the existing statictestDataIsolation.guards.test.jsall pass.Closes #6176