diff --git a/server/lib/dataRoot.js b/server/lib/dataRoot.js index 304710072a..aae52f132f 100644 --- a/server/lib/dataRoot.js +++ b/server/lib/dataRoot.js @@ -15,11 +15,28 @@ * worktree-rooted process so callers (boot migrations) can skip work that * assumes the real install tree instead of crashing. */ -import { isAbsolute, resolve as resolvePath, sep } from 'path'; +import { dirname, isAbsolute, join, resolve as resolvePath, sep } from 'path'; +import { fileURLToPath } from 'url'; /** Env var a real launch sets to pin the install root (see ecosystem.config.cjs). */ export const DATA_ROOT_ENV = 'PORTOS_DATA_ROOT'; +/** + * The executing checkout's root, derived from a caller's `import.meta.url` + * (two directories up from wherever that module physically lives). Single + * source of truth for this depth assumption — `lib/paths.js`'s `CODE_ROOT` + * and any code that must independently re-derive the real (non-test-mocked) + * install root, such as `userActions.js`'s data-root guard, both resolve it + * through here so the two can never silently drift apart if this formula + * ever changes. + * + * @param {string} moduleUrl the caller's `import.meta.url` + * @returns {string} absolute path two directories above the caller's file + */ +export function resolveCodeRootForModule(moduleUrl) { + return join(dirname(fileURLToPath(moduleUrl)), '../..'); +} + /** * Resolve the PortOS install root (the parent of `data/` and `data.reference/`). * diff --git a/server/lib/paths.js b/server/lib/paths.js index d44ff67d6e..e6b74acf5f 100644 --- a/server/lib/paths.js +++ b/server/lib/paths.js @@ -1,16 +1,12 @@ /** Canonical install/source roots and path construction helpers. */ import { homedir } from 'os'; -import { dirname, join } from 'path'; -import { fileURLToPath } from 'url'; -import { resolveInstallRoot } from './dataRoot.js'; - -const __lib_filename = fileURLToPath(import.meta.url); -const __lib_dirname = dirname(__lib_filename); +import { join } from 'path'; +import { resolveCodeRootForModule, resolveInstallRoot } from './dataRoot.js'; // The executing checkout's root (where THIS file physically lives). Code/source // paths — the repo root for git ops, `lib/slashdo` — must stay anchored here so // they always point at the checkout that loaded the code. -const CODE_ROOT = join(__lib_dirname, '../..'); +const CODE_ROOT = resolveCodeRootForModule(import.meta.url); // The install root that holds the runtime `data/` tree. Prefer an explicit // PORTOS_DATA_ROOT env var over the executing-file location so a process booted diff --git a/server/services/userActions.js b/server/services/userActions.js index d9542157d1..6e68fb3147 100644 --- a/server/services/userActions.js +++ b/server/services/userActions.js @@ -47,6 +47,7 @@ import { createFileWriteQueue } from '../lib/fileWriteQueue.js'; import { isPlainObject } from '../lib/objects.js'; import { createPgFileFacade, resolvePgBackend } from '../lib/pgFileFacade.js'; import { isTestRunner } from '../lib/db.js'; +import { resolveCodeRootForModule, resolveInstallRoot } from '../lib/dataRoot.js'; import { isUserActionActor, isUserActionType } from '../lib/userActionTypes.js'; import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } from './userActionsDb.js'; @@ -55,6 +56,38 @@ import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } fr // pre-mock value and crash any suite whose fileUtils stub omits PATHS.data. const eventsFile = () => join(PATHS.data, 'user-action-events.json'); +// The REAL repo data/ dir, computed independently of the (possibly test-mocked) +// `PATHS` import above via the same resolveCodeRootForModule/resolveInstallRoot +// technique lib/paths.js itself uses (both go through the shared helper, so +// they can't silently drift apart) — so a suite that redirects PATHS.data to a +// temp root can't accidentally spoof this comparison too. `dataRoot.js` reads +// its own env var directly rather than through anything a PATHS mock would touch. +const REAL_REPO_DATA_DIR = join(resolveInstallRoot(resolveCodeRootForModule(import.meta.url)), 'data'); + +/** + * Structural guard against the bug class in #3683/#3687/#5605: a suite that + * exercises a route wired to `recordUserAction` without redirecting + * PATHS.data to a temp root would otherwise silently write + * `user-action-events.json` into the developer's live `data/` tree the next + * time such a route gets exercised — #5594 patched three known offenders + * one at a time, which is a per-suite fix, not a guard against the next one. + * Fires only under the test runner, and only at the moment a write is + * actually attempted, so a suite that merely reads (or never triggers a + * `recordUserAction` call) is unaffected either way. + */ +function assertTestDataRootRedirected() { + if (!isTestRunner() || PATHS.data !== REAL_REPO_DATA_DIR) return; + throw new Error( + 'recordUserAction attempted a file-backend write of user-action-events.json ' + + "into the repo's real data/ tree. This suite exercises a route wired to " + + 'recordUserAction but never redirected PATHS.data to a temp root - mock ' + + "`../lib/fileUtils.js` with lib/mockPathsDataRoot.js's makePathsProxy/" + + 'createTempDataRoot (the same fix #5594 applied to cos.test.js / ' + + 'cosTaskRoutes.test.js / cosAgentFeedback.test.js) rather than letting the ' + + 'write land here.', + ); +} + /** LLM-readable one-liner cap — the ledger is read by a model, not paged through. */ export const SUMMARY_MAX_CHARS = 240; /** Per-string payload cap; the containing object gets `truncated: true`. */ @@ -281,6 +314,13 @@ function makeFileBackend() { return { name: 'file', record: (event) => queueWrite(async () => { + // Hoisted above loadFileEvents()/the dedupe check on purpose (#5627 + // review): an un-redirected suite replaying an existing + // (type, dedupeKey) used to hit the dedupe short-circuit's `return null` + // BEFORE this guard ever ran, silently no-op'ing past it with no throw — + // and by then loadFileEvents() had already read the real ledger into the + // test process regardless. Running the guard first closes both holes. + assertTestDataRootRedirected(); const events = await loadFileEvents(); if (events.some((row) => row.type === event.type && row.dedupeKey === event.dedupeKey)) return null; await ensureDir(PATHS.data); diff --git a/server/services/userActionsDataRootGuard.test.js b/server/services/userActionsDataRootGuard.test.js new file mode 100644 index 0000000000..be961d4824 --- /dev/null +++ b/server/services/userActionsDataRootGuard.test.js @@ -0,0 +1,56 @@ +/** + * Regression coverage for the #5605 structural guard: a suite that exercises + * a route wired to `recordUserAction` without redirecting PATHS.data to a + * temp root must fail loudly instead of writing user-action-events.json into + * the repo's real data/ tree (the bug class #5594 patched per-suite for + * cos.test.js / cosTaskRoutes.test.js / cosAgentFeedback.test.js). + * + * Deliberately does NOT mock `../lib/fileUtils.js` — this is the one test in + * the suite that is SUPPOSED to run with PATHS.data unredirected, so it can + * prove the guard rejects that exact condition. If the guard regresses, this + * would otherwise be the test that writes a real file into the repo's data/ + * tree, so it also asserts that never happens. + */ +import { describe, it, expect } from 'vitest'; +import { existsSync, readFileSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { PATHS } from '../lib/fileUtils.js'; +import { recordUserAction } from './userActions.js'; + +const REAL_EVENTS_FILE = join(PATHS.data, 'user-action-events.json'); + +describe('recordUserAction — data-root guard (#5605)', () => { + it('throws instead of writing user-action-events.json into the real data/ tree', async () => { + // Snapshot whatever is already there BEFORE exercising the guard. An + // install using the documented MEMORY_BACKEND=file escape hatch may + // legitimately already have this file as its real ledger — this test + // must prove the guard leaves it untouched, not assert it's absent (the + // previous version's `expect(existsSync(...)).toBe(false)` failed + // spuriously on exactly that install shape, and its unconditional + // afterEach `rmSync` then deleted the developer's real ledger). + const existedBefore = existsSync(REAL_EVENTS_FILE); + const contentBefore = existedBefore ? readFileSync(REAL_EVENTS_FILE, 'utf8') : null; + + try { + await expect(recordUserAction({ + type: 'cos.task.create', + summary: 'Guard regression probe', + dedupeKey: `guard-probe-${Math.random().toString(36).slice(2)}`, + })).rejects.toThrow(/real data\/ tree/); + + // The guard must leave the real tree exactly as it found it — present + // and unchanged, or still absent — never newly created or modified. + expect(existsSync(REAL_EVENTS_FILE)).toBe(existedBefore); + if (existedBefore) { + expect(readFileSync(REAL_EVENTS_FILE, 'utf8')).toBe(contentBefore); + } + } finally { + // Only clean up a file THIS run's own guard regression created — never + // touch one that was already there before it ran (which could be a + // developer's real MEMORY_BACKEND=file ledger). + if (!existedBefore && existsSync(REAL_EVENTS_FILE)) { + rmSync(REAL_EVENTS_FILE, { force: true }); + } + } + }); +});