test: guard user-action-events file-backend writes against leaking to real data/

[Bryandero98]

Summary

• server/services/userActions.js writes through createPgFileFacade, which selects the file backend under NODE_ENV=test. Any suite that exercises an instrumented route (recordUserAction) without redirecting PATHS.data to a temp root writes data/user-action-events.json into the developer's live data/ tree — the same bug class as unlockPass.test.js reads the developer's real data/ directory — 2 failures on any checkout with real series #3683/Audit the whole server suite for tests that read the checkout's real data/ directory #3687.
• Add a machine-local user_action_events store and instrument core CoS, schedule, and settings operator actions #5594 patched the three suites that tripped on it, but that's a per-suite fix: the next hook added to a route an untethered suite exercises silently re-opens the hole (epic Add a queryable user-action log so Persistent Mind can observe operator work and propose automations #5593 phase 3/Grow the user-action allowlist and detect leftover branches after idle agent runs #5596 is explicitly about growing that allowlist).
• Adds a structural guard directly on the write path (option 1 from the issue): right before the file-backend's atomicWrite, compares the live PATHS.data against the real repo data/ dir — computed independently via the same fileURLToPath/resolveInstallRoot technique lib/paths.js itself uses, so a suite's PATHS mock can't spoof the comparison too. A write attempted against the unredirected real path now throws a clear, actionable error (naming the fix and pointing at lib/mockPathsDataRoot.js) instead of landing on disk.
• Scoped to the recordUserAction write chokepoint specifically (not a blanket wrap of atomicWrite, which is used by dozens of unrelated stores repo-wide) — every route that ever attaches a recordUserAction hook funnels through this one function regardless of which suite exercises it, so growing the hook allowlist (Grow the user-action allowlist and detect leftover branches after idle agent runs #5596) stays covered automatically.

Test plan

• New userActionsDataRootGuard.test.js proves the guard fires — deliberately does NOT mock fileUtils.js (the one test meant to run unredirected), asserts recordUserAction throws before any write, and asserts no file lands in the real data/ tree
• userActions.test.js (own suite, already correctly redirected) — 13/13 pass, guard stays silent
• Every known recordUserAction caller + its test suite — routes/cos.test.js, routes/cosTaskRoutes.test.js, services/cos.test.js, services/cosAgentFeedback.test.js, routes/settings.test.js, services/settings.test.js, services/taskSchedule.test.js — 655/655 pass
• Manually confirmed no data/user-action-events.json existed before or after the guard test run

Fixes #5605

[Bryandero98]

Thanks for leaving the guard mechanism up to the implementer — went with option 1 since it protects the next route/hook automatically rather than needing another per-suite patch. Glad to switch to one of the other two approaches if you have a preference I'm not seeing.

[atomantic]

Reviewed by /do:review (security-focused pass, external contributor) — 2 critical, 1 improvement.

The approach is right and the ticket is satisfied in spirit. Option 1 from #5605, correctly narrowed to the recordUserAction write chokepoint instead of blanket-wrapping atomicWrite — that scoping call is good, and it means #5596's growing hook allowlist stays covered for free. Deriving REAL_REPO_DATA_DIR independently of the mockable PATHS is the right technique; I verified it agrees with lib/paths.js for a plain install, a PORTOS_DATA_ROOT pin, and a CoS worktree checkout.

Verified against the branch (worktree, isolated):

• Full cd server && npm test — 1809 files / 36801 tests pass, exit 0. No data/user-action-events.json after the run. AC #2 ✔
• The guard does fire on an unredirected write. AC #1 ✔
• Security scan: diff is pure ASCII apart from em-dashes (no zero-width/bidi), no network calls, no process.env reads, no eval/exec/child_process, no dependency or CI changes, production behavior unchanged (isTestRunner()-gated). Single commit, single author. Nothing injection-shaped in comments or prose.

Blocking (please address):

1. The new test deletes real user data (userActionsDataRootGuard.test.js:22-26). The afterEach rmSync is unconditional, and this is the one test that deliberately runs with PATHS.data unredirected — so it targets the live tree. On an install using the documented MEMORY_BACKEND=file escape hatch, npm test both fails spuriously (line 36 asserts the file is absent) and then permanently removes the developer's ledger. Reproduced end to end. A guard against leaking into data/ must not itself destroy data/.

2. The guard is bypassed by the dedupe short-circuit (userActions.js:321-322). It sits after loadFileEvents() and after the return null, so an un-redirected suite replaying an existing (type, dedupeKey) silently no-ops with no throw — reproduced. The real ledger has also already been read into the test process by that point. Hoisting the assertion to the top of the queued callback closes both.

Non-blocking: the REAL_REPO_DATA_DIR derivation is a second copy of paths.js's INSTALL_ROOT formula and fails open if that formula ever changes — worth collapsing into one exported helper in lib/dataRoot.js.

Coherence check: the PR description matches the diff; the test-plan claims hold up (I re-ran them). One AC gap worth noting: #5605 asks that the guard "names the offending suite" — the message names the fix but not the suite. Vitest's stack trace covers it in practice, so I'd call that satisfied, but a one-line mention would close it cleanly.

Thanks for taking this on — the structural instinct here is exactly right, and with the two fixes above it's a clear improvement over the per-suite patching in #5594.

Generated by /do:review

[Bryandero98]

Addressed both blocking findings, plus the non-blocking suggestion:

1. The destructive afterEach — rewrote the guard test to snapshot the real file's presence/content before exercising the guard, assert it comes back unchanged (not merely absent, which was the source of the spurious failure on a MEMORY_BACKEND=file install), and only clean up a file this run's own regression would have created.
2. The dedupe bypass — hoisted assertTestDataRootRedirected() above both loadFileEvents() and the dedupe short-circuit's return null, so neither path can slip past it anymore.
3. REAL_REPO_DATA_DIR drift risk — extracted the fileURLToPath/dirname/../.. depth formula into a new resolveCodeRootForModule() in lib/dataRoot.js. Both paths.js's CODE_ROOT and userActions.js's REAL_REPO_DATA_DIR now derive through it — one source of truth, paths.js's own exported values unchanged.

Verified: full cd server && npm test (1793/1810 files pass), and confirmed via git stash that the 16 files / 58 tests that do fail are identical with or without this change — all pre-existing Windows-only noise (symlink creation needs Developer Mode privileges, a couple of EBUSY temp-file races, one missing local Python module), none touching paths.js/dataRoot.js/userActions.js. Also re-ran the guard test and userActions.test.js in isolation.

🤖 Generated with Claude Code

[atomantic]

Re-reviewed after the fix commit — both blocking findings are resolved, and I verified each one independently rather than taking the description's word for it. Approving.

Verified against the branch (isolated worktree):

1. Destructive cleanup — fixed. Seeded a real data/user-action-events.json (the MEMORY_BACKEND=file install shape that broke the previous version) and ran the guard test: it now passes and leaves the file byte-identical. The snapshot-then-compare rewrite plus the !existedBefore cleanup gate is exactly right, and the try/finally is a better shape than the afterEach I suggested.
2. Dedupe bypass — fixed. Reproduced the original hole's setup: seeded {type:'cos.task.create', dedupeKey:'preexisting-key'} and replayed the same pair. Before the hoist this returned null silently; now it throws, and loadFileEvents() never reads the real ledger.
3. Bypass probe. Removed assertTestDataRootRedirected() and re-ran — the guard test goes red with the leaked event visible in the diff. The test genuinely pins the contract rather than passing vacuously.
4. Shared helper (non-blocking) — addressed. resolveCodeRootForModule collapses the duplicated derivation, and paths.js's CODE_ROOT is expression-identical to before, so the fail-open drift risk is gone.
5. Full cd server && npm test — no data/user-action-events.json after the run, and zero guard firings across all 36.5k tests (so no existing suite is falsely tripped). CI's Server tests job is green on this head.
6. Security pass: diff is pure ASCII apart from em-dashes (no zero-width/bidi), no child_process/eval/network/process.env additions, no dependency or workflow changes, production behavior unchanged (isTestRunner()-gated early return). The only destructive call is the now-correctly-gated rmSync.

Ticket alignment (#5605): option 1, scoped to the recordUserAction chokepoint. AC #1 ✔ (bypass probe), AC #2 ✔ (clean tree after a full run), AC #3 — the message names the fix but not the offending suite; Vitest's stack covers that in practice, so I'm calling it satisfied.

Follow-ups I'll take on our side (all minor, none worth another round-trip here):

• list is still unguarded — an un-redirected suite can read the live ledger through listUserActions. Read-only, so outside this ticket's AC.
• If the guard ever regresses on a machine that already has the ledger, the test leaves the probe event appended (it snapshots contentBefore but doesn't restore it).
• lib/README.md's dataRoot.js row doesn't mention the new export.

Thanks for the thorough turnaround on this — the structural fix is a real improvement over the per-suite patching in #5594, and it keeps #5596's growing hook allowlist covered for free.