From 9649475cb5e7da80d6931042ac18fb2e176761f7 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 2 Sep 2026 07:37:03 -0700 Subject: [PATCH] guard the user-action ledger's read path, not just its write path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #5627 landed the #5605 structural guard on the file backend's `record` path, so an un-redirected suite can no longer write user-action-events.json into the developer's live data/ tree. `list` was left unguarded, so such a suite could still READ the live ledger — machine-local operator records the privacy ADR keeps off the wire and out of untethered test processes. Also hardens the guard test's own cleanup: it snapshotted the pre-existing file's bytes but only ever deleted a file it had created, so a guard regression on a MEMORY_BACKEND=file install left the developer's real ledger with the probe event appended. It now restores the snapshot. The guard message takes the attempted operation as an argument so the read path doesn't report itself as a write. --- server/lib/README.md | 2 +- server/services/userActions.js | 35 ++++++--- .../services/userActionsDataRootGuard.test.js | 76 ++++++++++--------- 3 files changed, 66 insertions(+), 47 deletions(-) diff --git a/server/lib/README.md b/server/lib/README.md index cd8284e1e3..692009dd5a 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -218,7 +218,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `collectionStore.js` | Per-type, per-record JSON storage with explicit type-level `schemaVersion` stamping. Use for collections that have outgrown a monolithic JSON file. `createCollectionStore({ dir, type, schemaVersion, sanitizeRecord })` returns `loadOne` / `saveOne` / `saveOneNow` / `listIds` / `loadAll` / `loadAllResult` / `deleteOne` / `loadTypeIndex` / `saveTypeIndex` / `verifySchemaVersion`. `loadAll` silently drops corrupt records; `loadAllResult()` → `{ records, failedIds }` keeps the "which ids failed to load" signal so a caller can tell a partial set from a complete one. Per-id write queue means writes to different records don't serialize; `saveOneNow` is for callers already inside a collection write queue. Boot-time `verifyCollectionVersions([store, ...])` logs schema-version mismatches. **Type-index `config` slot** holds cross-record state (see the `TypeIndexConfig` typedef + header convention): `{ runs?: [], featureFlags?: {}, lockPolicies?: {} }` — `runs` is the shipped slot (universeBuilder's capped history log), the other two are reserved names; consumers may add their own keys but should reuse a reserved name when it fits and document the shape next to the consumer. `saveTypeIndex({ config })` shallow-merges `config` one level deep (a patched `runs` replaces the whole array), so a read-modify-write of a slot must load → mutate a copy → write inside `queueTypeIndexWrite(fn)`. | | `conflictJournal.js` | Non-blocking edit-conflict journal for cross-install LWW merges. `maybeJournalBeforeOverwrite({kind,id,local,remote,source})` (call right before a merge overwrite) archives the losing local version when a true 3-way divergence is detected (`detectConflict` via per-record `syncBaseHash` + `contentHashForRecord`), then advances the base hash; `flushBaseHashes()` persists the batched base-hash side store; `withBaseHashFlushBatch(fn)` defers every interior flush so an await-separated multi-record loop — the peer:online convergence push and every base-hash-evicting `pruneTombstoned*` loop — collapses N `sync_base_hashes.json` rewrites into one terminal write (depth-counted, so concurrent batches merge too; flushes in `finally`). `deleteSyncBaseHash(kind,id)` evicts a record's base hash when its tombstone is hard-pruned, so those paths don't let the side store grow without bound; `pruneOrphanedBaseHashes(resolves)` is the backstop sweep (`resolves(kind,id) => bool`; unknown kinds kept) that drops keys whose record no longer resolves, wired into the tombstone GC sweep. `conflictJournalStore()` is the `pending`/`resolved` entry store (discard resolves an entry; DELETE hard-removes it — there is no `dismissed` status). Local-only — never crosses the wire. | | `schemaVersions.js` | Cross-instance sync version contract. `PORTOS_SCHEMA_VERSIONS` (frozen map of `{ category: layoutVersion }`), `RECORD_KIND_SCHEMA_CATEGORIES` (frozen map of federated record kind → the schema categories it writes), `buildPortosMeta()` (envelope for every outbound sync payload), `compareSchemaVersions(sender, receiver)` returning `{ ahead, behind, compatible }`, `scopeVersionDiff(diff, categories)` (restrict that diff to the categories a specific transfer touches), and `formatVersionGap()` for UI/log lines. Receivers gate `applyIncomingPush` / share-bucket import / snapshot apply per-category on the scoped comparator result so an upgraded sender can't corrupt a downstream peer — and a bump to one category doesn't sever sync of the others. | -| `dataRoot.js` | Data-root resolution + worktree-checkout detection (#1947). `resolveInstallRoot(fallbackRoot)` prefers the `PORTOS_DATA_ROOT` env var (pinned at real launch in `ecosystem.config.cjs`) over an `import.meta.url`-derived fallback, so a process booted from inside a CoS agent git worktree still resolves `data/`/`data.reference/` to the real install instead of the worktree's empty tree. `isWorktreeRoot(rootDir)` is the boot-migration backstop — true when `rootDir` lives under `data/cos/worktrees/` (keyed on the path segment only, so a fresh install's empty `data/` isn't a false positive). `DATA_ROOT_ENV` is the env-var name constant. Consumed by `fileUtils.js` (`PATHS`), `server/index.js`, and `scripts/run-migrations.js`. | +| `dataRoot.js` | Data-root resolution + worktree-checkout detection (#1947). `resolveInstallRoot(fallbackRoot)` prefers the `PORTOS_DATA_ROOT` env var (pinned at real launch in `ecosystem.config.cjs`) over an `import.meta.url`-derived fallback, so a process booted from inside a CoS agent git worktree still resolves `data/`/`data.reference/` to the real install instead of the worktree's empty tree. `isWorktreeRoot(rootDir)` is the boot-migration backstop — true when `rootDir` lives under `data/cos/worktrees/` (keyed on the path segment only, so a fresh install's empty `data/` isn't a false positive). `resolveCodeRootForModule(moduleUrl)` is the single source of truth for the "two directories above this file" depth assumption — `paths.js`'s `CODE_ROOT` and `services/userActions.js`'s data-root guard both derive through it so they cannot silently drift apart. `DATA_ROOT_ENV` is the env-var name constant. Consumed by `fileUtils.js` (`PATHS`), `server/index.js`, and `scripts/run-migrations.js`. | | `agentInstructionsFile.js` | The `AGENTS.md` + bridge `CLAUDE.md` pair a repo carries (#4852). `writeAgentInstructions(repoPath, content)` writes the body to `AGENTS.md` and the one-line `@AGENTS.md` import beside it — use it in scaffolders instead of a bare `writeFile(join(repoPath, 'CLAUDE.md'), …)`, since a generated repo carrying only one name is unreadable to half the CLIs PortOS can point at it. Constants: `AGENT_INSTRUCTIONS_FILENAME`, `CLAUDE_BRIDGE_FILENAME`, `AGENT_INSTRUCTIONS_IMPORT`. | | `fileCore.js` | Cross-cutting filesystem primitives (`atomicWrite`, directory helpers, bounded tail reads/watchers), time/format helpers, directory sizing, and SHA-256 helpers. | | `fileUtils.js` | Backward-compatible facade re-exporting the focused file utility modules so existing deep imports need no caller changes. | diff --git a/server/services/userActions.js b/server/services/userActions.js index 6e68fb3147..9bd9e5533e 100644 --- a/server/services/userActions.js +++ b/server/services/userActions.js @@ -71,20 +71,27 @@ const REAL_REPO_DATA_DIR = join(resolveInstallRoot(resolveCodeRootForModule(impo * `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. + * Fires only under the test runner, and only at the moment the file backend + * actually touches the ledger, so a suite that never reaches `recordUserAction` + * / `listUserActions` is unaffected either way. + * + * Reads are guarded as well as writes: the live ledger holds machine-local + * operator records (ADR docs/decisions/2026-08-08-privacy-records-machine-local.md), + * so an untethered suite must not pull them into the test process either. + * + * @param {string} attempted what the file backend was about to do, e.g. + * `'recordUserAction attempted a write of'` */ -function assertTestDataRootRedirected() { +function assertTestDataRootRedirected(attempted) { 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.', + `${attempted} user-action-events.json in the repo's real data/ tree. ` + + 'This suite exercises the user-action ledger 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 file backend touch the ' + + 'real tree.', ); } @@ -320,7 +327,7 @@ function makeFileBackend() { // 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(); + assertTestDataRootRedirected('recordUserAction attempted a file-backend write of'); const events = await loadFileEvents(); if (events.some((row) => row.type === event.type && row.dedupeKey === event.dedupeKey)) return null; await ensureDir(PATHS.data); @@ -328,6 +335,10 @@ function makeFileBackend() { return event; }), list: async (filters) => { + // Same guard as the write path: an un-redirected suite must not get to READ + // the developer's live ledger either — those rows are machine-local operator + // records (docs/decisions/2026-08-08-privacy-records-machine-local.md). + assertTestDataRootRedirected('listUserActions attempted a file-backend read of'); const events = await loadFileEvents(); return events .filter((event) => matchesFilters(event, filters)) diff --git a/server/services/userActionsDataRootGuard.test.js b/server/services/userActionsDataRootGuard.test.js index be961d4824..5ea4a51cb3 100644 --- a/server/services/userActionsDataRootGuard.test.js +++ b/server/services/userActionsDataRootGuard.test.js @@ -11,46 +11,54 @@ * 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 { describe, it, expect, afterEach } from 'vitest'; +import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { PATHS } from '../lib/fileUtils.js'; -import { recordUserAction } from './userActions.js'; +import { listUserActions, 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; +// Snapshot ONCE, before any case runs. An install using the documented +// MEMORY_BACKEND=file escape hatch may legitimately already have this file as +// its real ledger, so every case must prove the guard leaves it untouched +// rather than assert it is absent — and `afterEach` must put back exactly what +// it found, including when a regression mutated the bytes. +const EXISTED_BEFORE = existsSync(REAL_EVENTS_FILE); +const CONTENT_BEFORE = EXISTED_BEFORE ? 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 real tree must come back exactly as this file found it — present and byte-identical, or still absent. */ +function expectRealTreeUntouched() { + expect(existsSync(REAL_EVENTS_FILE)).toBe(EXISTED_BEFORE); + if (EXISTED_BEFORE) expect(readFileSync(REAL_EVENTS_FILE, 'utf8')).toBe(CONTENT_BEFORE); +} - // 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 }); - } - } +afterEach(() => { + // Repair anything a GUARD REGRESSION did, never anything the developer had: + // delete only a file this run leaked, and restore only bytes this run changed. + if (!EXISTED_BEFORE) { + if (existsSync(REAL_EVENTS_FILE)) rmSync(REAL_EVENTS_FILE, { force: true }); + return; + } + if (readFileSync(REAL_EVENTS_FILE, 'utf8') !== CONTENT_BEFORE) { + writeFileSync(REAL_EVENTS_FILE, CONTENT_BEFORE); + } +}); + +describe('user-action ledger — data-root guard (#5605)', () => { + it('recordUserAction throws instead of writing into the real data/ tree', async () => { + 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/); + expectRealTreeUntouched(); + }); + + it('listUserActions throws instead of reading the real ledger', async () => { + // The read path matters too: those rows are machine-local operator records + // (privacy ADR), so an untethered suite must not pull them into the process. + await expect(listUserActions()).rejects.toThrow(/real data\/ tree/); + expectRealTreeUntouched(); }); });