From a48abd0a0b7d1eed5188e1746948acbe9730dbd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Sun, 19 Jul 2026 18:27:32 -0300 Subject: [PATCH] refactor(core): centralize verified run catalog --- docs/releases/UNRELEASED.md | 4 + packages/cli/src/commands/artifacts.ts | 58 +-- packages/cli/test/commands.test.ts | 24 + packages/core/src/evidence.ts | 117 ++--- packages/core/src/index.ts | 9 +- packages/core/src/run-catalog.ts | 495 +++++++++++++++++++++ packages/core/src/run.ts | 58 +-- packages/core/test/evidence.test.ts | 41 ++ packages/core/test/run-catalog.test.ts | 216 +++++++++ packages/mcp-server/src/resources.ts | 300 ++++--------- packages/mcp-server/src/tools/artifacts.ts | 65 ++- packages/mcp-server/test/tools.test.ts | 18 + 12 files changed, 974 insertions(+), 431 deletions(-) create mode 100644 packages/core/src/run-catalog.ts create mode 100644 packages/core/test/run-catalog.test.ts diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 9df5fbc..8acf629 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -32,6 +32,9 @@ release description, then reset it after the release is published. - Centralized session lifecycle composition in core and routed dead-session reaping through typed desktop, Android, and browser teardown owners, removing duplicate CLI/MCP orchestration and core PID/profile stop implementations. +- Added one verified run catalog for deterministic discovery, directory-manifest + identity binding, corrupt-entry handling, and symlink-safe root-file reads + across core retention, CLI artifacts, and MCP tools/resources. - Centralized CLI provisioning policy for plan classification, consent, dry-runs, ordered preflight failures/skips, adapter-owned sudo routing, cancellation, redacted presentation plans, and partial results. @@ -56,6 +59,7 @@ release description, then reset it after the release is published. - `bun install --frozen-lockfile` - `bun run typecheck` +- Focused run catalog, run, evidence, CLI artifact, and MCP resource/tool suites. - `bun run test` (79 files, 958 passed / 2 skipped) - `bun run test:coverage` (79 files, 958 passed / 2 skipped; all thresholds met) - `bun run build` diff --git a/packages/cli/src/commands/artifacts.ts b/packages/cli/src/commands/artifacts.ts index 3ee9ed8..9ad2d56 100644 --- a/packages/cli/src/commands/artifacts.ts +++ b/packages/cli/src/commands/artifacts.ts @@ -1,12 +1,13 @@ import { spawn } from "node:child_process"; -import path from "node:path"; import { + EVIDENCE_ACTION_LOG, isEvidenceRun, - listRuns, - readActions, + openRunCatalog, + parseActionsJournal, renderRunReport, runsDir, - type RunManifest, + type RunCatalog, + type RunCatalogEntry, } from "@pickforge/picklab-core"; import { findOnPath } from "@pickforge/picklab-desktop-linux"; import { @@ -18,8 +19,9 @@ import { export async function runArtifactsList(opts: BaseCliOptions): Promise { return runReported(opts, async () => { const projectDir = resolveProjectDir(opts); - const manifests = await listRuns(projectDir); - const runs = manifests.map((manifest) => ({ + const catalog = await openRunCatalog(projectDir); + const entries = await catalog.list(); + const runs = entries.map(({ manifest }) => ({ runId: manifest.runId, slug: manifest.slug, createdAt: manifest.createdAt, @@ -39,32 +41,28 @@ export async function runArtifactsList(opts: BaseCliOptions): Promise { }); } -const RUN_ID_PATTERN = /^[A-Za-z0-9._-]+$/; - -function isSafeRunId(runId: string): boolean { - return RUN_ID_PATTERN.test(runId) && runId !== "." && runId !== ".."; -} - async function findRun( projectDir: string, runId: string | undefined, -): Promise<{ manifest: RunManifest; dir: string }> { - const manifests = (await listRuns(projectDir)).filter((candidate) => - isSafeRunId(candidate.runId), - ); - let manifest: RunManifest | undefined; - if (runId === undefined) { - manifest = manifests[0]; - if (manifest === undefined) { +): Promise<{ catalog: RunCatalog; entry: RunCatalogEntry }> { + const catalog = await openRunCatalog(projectDir); + const entry = await catalog.find(runId); + if (entry === undefined) { + if (runId === undefined) { throw new Error(`No runs found under ${runsDir(projectDir)}`); } - } else { - manifest = manifests.find((candidate) => candidate.runId === runId); - if (manifest === undefined) { - throw new Error(`Run not found: ${runId} (see: picklab artifacts list)`); - } + throw new Error(`Run not found: ${runId} (see: picklab artifacts list)`); } - return { manifest, dir: path.join(runsDir(projectDir), manifest.runId) }; + return { catalog, entry }; +} + +async function readCatalogActions( + catalog: RunCatalog, + entry: RunCatalogEntry, +): Promise> { + if (!isEvidenceRun(entry.manifest)) return []; + const raw = await catalog.readRootTextIfPresent(entry, EVIDENCE_ACTION_LOG); + return raw === undefined ? [] : parseActionsJournal(raw, entry.dir); } export async function runArtifactsOpen( @@ -73,7 +71,8 @@ export async function runArtifactsOpen( ): Promise { return runReported(opts, async () => { const projectDir = resolveProjectDir(opts); - const { manifest, dir } = await findRun(projectDir, runId); + const { entry } = await findRun(projectDir, runId); + const { manifest, dir } = entry; let opened = false; const display = process.env.DISPLAY; if (opts.json !== true && display !== undefined && display !== "") { @@ -101,8 +100,9 @@ export async function runArtifactsReport( ): Promise { return runReported(opts, async () => { const projectDir = resolveProjectDir(opts); - const { manifest, dir } = await findRun(projectDir, runId); - const records = isEvidenceRun(manifest) ? await readActions(dir) : []; + const { catalog, entry } = await findRun(projectDir, runId); + const { manifest, dir } = entry; + const records = await readCatalogActions(catalog, entry); return { data: { runId: manifest.runId, dir, manifest }, lines: renderRunReport(manifest, dir, records), diff --git a/packages/cli/test/commands.test.ts b/packages/cli/test/commands.test.ts index 99d9dc1..6449f82 100644 --- a/packages/cli/test/commands.test.ts +++ b/packages/cli/test/commands.test.ts @@ -1508,6 +1508,30 @@ describe("picklab artifacts", () => { expect(report.errors[0]).toContain("Corrupt evidence journal"); }); + it("fails closed for an unsafe evidence journal", async () => { + const env = makeEnv(); + const projectDir = makeProjectDir(); + const runId = "20260609-120000-unsafe"; + writeSyntheticRun(projectDir, runId, { + evidenceVersion: 1, + actionLog: "actions.jsonl", + }); + const runDir = path.join(projectDir, ".picklab", "runs", runId); + fs.symlinkSync( + path.join(runDir, "manifest.json"), + path.join(runDir, "actions.jsonl"), + ); + + const result = await runCli( + ["artifacts", "report", runId, "--project-dir", projectDir, "--json"], + env, + ); + expect(result.code).toBe(1); + expect(parseJson(result).errors.join("\n")).toContain( + "Unsafe run catalog file", + ); + }); + it("fails for unknown run ids", async () => { const env = makeEnv(); const projectDir = makeProjectDir(); diff --git a/packages/core/src/evidence.ts b/packages/core/src/evidence.ts index 73a3dd4..e6b9b15 100644 --- a/packages/core/src/evidence.ts +++ b/packages/core/src/evidence.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { ensureDir, runsDir, writeFileAtomic } from "./paths.js"; +import { openRunCatalog } from "./run-catalog.js"; import { isPidAlive, processIdentityMatches, @@ -1604,69 +1605,6 @@ function isFinalizedStatus(status: RunStatus): boolean { return status === "completed" || status === "failed"; } -interface EvidenceRunEntry { - /** The actual directory-entry name — authoritative for what gets deleted. */ - dirName: string; - manifest: RunManifest; -} - -/** - * Enumerate evidence-run directories, binding each manifest to the directory it - * physically lives in. Applies the same runs-root confinement as `listRuns` - * (rejecting a symlinked `.picklab` or `.picklab/runs`) and, critically, only - * yields an entry when the manifest's declared `runId` matches its own - * directory name. A manifest that names a *different* directory is never used to - * decide a deletion, so a spoofed or corrupt `runId` can never redirect a - * removal at another run's directory. - */ -async function listEvidenceRunEntries( - projectDir: string, -): Promise { - const parent = runsDir(projectDir); - try { - const realProject = await fs.promises.realpath(projectDir); - const realParent = await fs.promises.realpath(parent); - if (realParent !== path.join(realProject, ".picklab", "runs")) return []; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; - throw error; - } - - let entries: fs.Dirent[]; - try { - entries = await fs.promises.readdir(parent, { withFileTypes: true }); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return []; - throw error; - } - - const out: EvidenceRunEntry[] = []; - for (const entry of entries) { - if (entry.isSymbolicLink() || !entry.isDirectory()) continue; - const dir = path.join(parent, entry.name); - const manifestPath = path.join(dir, "manifest.json"); - try { - const manifestStat = await fs.promises.lstat(manifestPath); - if (manifestStat.isSymbolicLink()) continue; - } catch { - continue; - } - const manifest = await readManifest(dir); - if ( - manifest === undefined || - typeof manifest.runId !== "string" || - typeof manifest.createdAt !== "string" || - !Array.isArray(manifest.artifacts) - ) { - continue; - } - // Bind: the manifest must declare the directory it actually lives in. - if (manifest.runId !== entry.name) continue; - out.push({ dirName: entry.name, manifest }); - } - return out; -} - /** Collect run ids currently referenced by any session's active pointer. */ async function collectActiveRunIds(parent: string): Promise> { const active = new Set(); @@ -1707,30 +1645,28 @@ export async function pruneFinalizedEvidenceRuns( opts: PruneEvidenceOptions = {}, ): Promise { const keep = opts.keep ?? EVIDENCE_RETENTION_KEEP; - const parent = runsDir(projectDir); - // Enumerate directory entries with their bound manifests (runId === dirName), - // so every deletion decision targets the directory the manifest lives in — a - // spoofed runId can never point the removal at another run. - const entries = await listEvidenceRunEntries(projectDir); - const activeRunIds = await collectActiveRunIds(parent); - - const finalized = entries - .filter( - (entry) => - isEvidenceRun(entry.manifest) && - isFinalizedStatus(entry.manifest.status) && - !activeRunIds.has(entry.dirName), - ) - .sort((a, b) => - b.manifest.createdAt.localeCompare(a.manifest.createdAt), - ); + const catalog = await openRunCatalog(projectDir); + const entries = await catalog.list(); + const activeByRoot = new Map>(); + for (const entry of entries) { + if (!activeByRoot.has(entry.rootDir)) { + activeByRoot.set(entry.rootDir, await collectActiveRunIds(entry.rootDir)); + } + } + + const finalized = entries.filter( + (entry) => + isEvidenceRun(entry.manifest) && + isFinalizedStatus(entry.manifest.status) && + !activeByRoot.get(entry.rootDir)?.has(entry.dirName), + ); const removed: string[] = []; - for (const { dirName } of finalized.slice(keep)) { + for (const entry of finalized.slice(keep)) { + const { dirName, dir, rootDir } = entry; if (!isSafeRunId(dirName)) continue; - const dir = path.join(parent, dirName); - // Confinement: only a real, non-symlink directory directly under the runs - // root is a removal candidate. + // Confinement: only a real, non-symlink directory directly under its + // catalog root is a removal candidate. let stat: fs.Stats; try { stat = await fs.promises.lstat(dir); @@ -1738,14 +1674,13 @@ export async function pruneFinalizedEvidenceRuns( continue; } if (stat.isSymbolicLink() || !stat.isDirectory()) continue; - if (path.dirname(dir) !== parent) continue; - // Re-read and re-verify the *same* manifest immediately before removal, so a - // run that was concurrently re-activated, mutated, or whose manifest now - // disagrees with its directory is never deleted (TOCTOU guard). - const fresh = await readManifest(dir); + if (path.dirname(dir) !== rootDir) continue; + // Re-read through the catalog immediately before removal. A run that was + // swapped, mutated, or whose manifest no longer binds to its directory is + // never deleted. + const fresh = await catalog.refresh(entry); if ( fresh === undefined || - fresh.runId !== dirName || !isEvidenceRun(fresh) || !isFinalizedStatus(fresh.status) ) { @@ -1753,7 +1688,7 @@ export async function pruneFinalizedEvidenceRuns( } // Re-check the active pointers last: an owner may have re-claimed this run // between the first scan and now. - const activeNow = await collectActiveRunIds(parent); + const activeNow = await collectActiveRunIds(rootDir); if (activeNow.has(dirName)) continue; await fs.promises.rm(dir, { recursive: true, force: true }); forgetRunByteCache(dir); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 17fb944..e073783 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -30,7 +30,6 @@ export { createRun, EVIDENCE_ACTION_LOG, EVIDENCE_VERSION, - listRuns, RunHandle, type ArtifactType, type CreateRunOptions, @@ -39,6 +38,14 @@ export { type RunStatus, } from "./run.js"; +export { + listRuns, + openRunCatalog, + RunCatalog, + type RunCatalogEntry, + type RunCatalogRoot, +} from "./run-catalog.js"; + export { activePointerPath, appendAction, diff --git a/packages/core/src/run-catalog.ts b/packages/core/src/run-catalog.ts new file mode 100644 index 0000000..df33714 --- /dev/null +++ b/packages/core/src/run-catalog.ts @@ -0,0 +1,495 @@ +import fs from "node:fs"; +import path from "node:path"; +import { runsDir } from "./paths.js"; +import type { RunManifest } from "./run.js"; + +const SAFE_ENTRY_PATTERN = /^[A-Za-z0-9._-]+$/; +const MANIFEST_FILE = "manifest.json"; + +/** + * One trusted run-storage root. Roots are read in array order; the first valid + * occurrence of a run id wins. `expectedRealDir` lets the resolver authorize a + * canonical location without making the catalog trust symlinked ancestors. + */ +export interface RunCatalogRoot { + dir: string; + expectedRealDir: string; +} + +/** A manifest bound to the real directory entry it was read from. */ +export interface RunCatalogEntry { + dirName: string; + dir: string; + rootDir: string; + rootPrecedence: number; + manifest: RunManifest; +} + +interface CatalogIdentity { + root: fs.Stats; + run: fs.Stats; +} + +const ENTRY_IDENTITY = Symbol("runCatalogIdentity"); +type BoundRunCatalogEntry = RunCatalogEntry & { + [ENTRY_IDENTITY]: CatalogIdentity; +}; + +class RunCatalogAccessError extends Error { + readonly missing: boolean; + + constructor(message: string, missing = false) { + super(message); + this.missing = missing; + } +} + +function isMissing(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return code === "ENOENT" || code === "ENOTDIR"; +} + +function sameIdentity(left: fs.Stats, right: fs.Stats): boolean { + return left.dev === right.dev && left.ino === right.ino; +} + +function compareText(left: string, right: string): number { + return left < right ? -1 : left > right ? 1 : 0; +} + +function isSafeEntryName(name: string): boolean { + return ( + SAFE_ENTRY_PATTERN.test(name) && + name !== "." && + name !== ".." && + !name.includes("..") + ); +} + +function parseManifest(raw: string, dirName: string): RunManifest | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + if (typeof parsed !== "object" || parsed === null) return undefined; + const manifest = parsed as RunManifest; + if ( + typeof manifest.runId !== "string" || + typeof manifest.createdAt !== "string" || + !Array.isArray(manifest.artifacts) + ) { + return undefined; + } + // Identity is fail-closed: a manifest can describe only the directory entry + // that physically contains it. Mismatches are corrupt catalog entries. + if (manifest.runId !== dirName) return undefined; + return manifest; +} + +async function verifiedRoot( + root: RunCatalogRoot, +): Promise<{ stat: fs.Stats; realDir: string } | undefined> { + let stat: fs.Stats; + let realDir: string; + try { + stat = await fs.promises.lstat(root.dir); + if (stat.isSymbolicLink() || !stat.isDirectory()) return undefined; + realDir = await fs.promises.realpath(root.dir); + } catch (error) { + if (isMissing(error)) return undefined; + throw error; + } + if (realDir !== root.expectedRealDir) return undefined; + return { stat, realDir }; +} + +async function rootAndRunStillMatch( + root: RunCatalogRoot, + dirName: string, + rootStat: fs.Stats, + runStat: fs.Stats, +): Promise { + try { + const currentRoot = await verifiedRoot(root); + if ( + currentRoot === undefined || + !sameIdentity(rootStat, currentRoot.stat) + ) { + return false; + } + const runDir = path.join(root.dir, dirName); + const currentRun = await fs.promises.lstat(runDir); + return ( + !currentRun.isSymbolicLink() && + currentRun.isDirectory() && + sameIdentity(runStat, currentRun) && + (await fs.promises.realpath(runDir)) === + path.join(currentRoot.realDir, dirName) + ); + } catch { + return false; + } +} + +async function readVerifiedRootFile( + root: RunCatalogRoot, + dirName: string, + fileName: string, + readContents: boolean, + expectedIdentity?: CatalogIdentity, +): Promise<{ value: Buffer | true; identity: CatalogIdentity }> { + if (!isSafeEntryName(dirName) || !isSafeEntryName(fileName)) { + throw new RunCatalogAccessError("Unsafe run catalog entry"); + } + + const rootBefore = await verifiedRoot(root); + if (rootBefore === undefined) { + throw new RunCatalogAccessError("Unsafe run catalog root"); + } + if ( + expectedIdentity !== undefined && + !sameIdentity(rootBefore.stat, expectedIdentity.root) + ) { + throw new RunCatalogAccessError("Run catalog root changed"); + } + + const runDir = path.join(root.dir, dirName); + const filePath = path.join(runDir, fileName); + let runBefore: fs.Stats | undefined; + let fileBefore: fs.Stats; + try { + runBefore = await fs.promises.lstat(runDir); + if (runBefore.isSymbolicLink() || !runBefore.isDirectory()) { + throw new RunCatalogAccessError("Unsafe run catalog directory"); + } + if ( + expectedIdentity !== undefined && + !sameIdentity(runBefore, expectedIdentity.run) + ) { + throw new RunCatalogAccessError("Run catalog directory changed"); + } + if ( + (await fs.promises.realpath(runDir)) !== + path.join(rootBefore.realDir, dirName) + ) { + throw new RunCatalogAccessError("Unsafe run catalog directory"); + } + fileBefore = await fs.promises.lstat(filePath); + if (fileBefore.isSymbolicLink() || !fileBefore.isFile()) { + throw new RunCatalogAccessError("Unsafe run catalog file"); + } + if ( + (await fs.promises.realpath(filePath)) !== + path.join(rootBefore.realDir, dirName, fileName) + ) { + throw new RunCatalogAccessError("Unsafe run catalog file"); + } + } catch (error) { + if (error instanceof RunCatalogAccessError) throw error; + if (isMissing(error)) { + const unchanged = + runBefore !== undefined && + (await rootAndRunStillMatch( + root, + dirName, + rootBefore.stat, + runBefore, + )); + throw new RunCatalogAccessError( + unchanged + ? "Run catalog file not found" + : "Run catalog directory changed", + unchanged, + ); + } + throw new RunCatalogAccessError("Could not verify run catalog file"); + } + + if (runBefore === undefined) { + throw new RunCatalogAccessError("Run catalog directory not found"); + } + + let handle: fs.promises.FileHandle; + try { + handle = await fs.promises.open( + filePath, + fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW, + ); + } catch (error) { + const missing = isMissing(error); + const unchanged = + missing && + (await rootAndRunStillMatch( + root, + dirName, + rootBefore.stat, + runBefore, + )); + throw new RunCatalogAccessError( + missing && unchanged + ? "Run catalog file not found" + : "Could not open run catalog file", + missing && unchanged, + ); + } + + try { + const opened = await handle.stat(); + if (!opened.isFile() || !sameIdentity(opened, fileBefore)) { + throw new RunCatalogAccessError("Run catalog file changed during read"); + } + const value = readContents ? await handle.readFile() : true; + + const rootAfter = await verifiedRoot(root); + if ( + rootAfter === undefined || + !sameIdentity(rootBefore.stat, rootAfter.stat) + ) { + throw new RunCatalogAccessError("Run catalog root changed during read"); + } + const runAfter = await fs.promises.lstat(runDir); + const fileAfter = await fs.promises.lstat(filePath); + if ( + runAfter.isSymbolicLink() || + !runAfter.isDirectory() || + !sameIdentity(runBefore, runAfter) || + fileAfter.isSymbolicLink() || + !fileAfter.isFile() || + !sameIdentity(opened, fileAfter) + ) { + throw new RunCatalogAccessError("Run catalog entry changed during read"); + } + if ( + (await fs.promises.realpath(runDir)) !== + path.join(rootAfter.realDir, dirName) || + (await fs.promises.realpath(filePath)) !== + path.join(rootAfter.realDir, dirName, fileName) + ) { + throw new RunCatalogAccessError("Run catalog entry escaped during read"); + } + return { + value, + identity: { root: rootBefore.stat, run: runBefore }, + }; + } catch (error) { + if (error instanceof RunCatalogAccessError) throw error; + throw new RunCatalogAccessError("Could not read run catalog file"); + } finally { + await handle.close(); + } +} + +async function readBoundManifest( + root: RunCatalogRoot, + dirName: string, + expectedIdentity?: CatalogIdentity, +): Promise<{ manifest: RunManifest; identity: CatalogIdentity } | undefined> { + try { + const result = await readVerifiedRootFile( + root, + dirName, + MANIFEST_FILE, + true, + expectedIdentity, + ); + if (!Buffer.isBuffer(result.value)) return undefined; + const manifest = parseManifest(result.value.toString("utf8"), dirName); + return manifest === undefined + ? undefined + : { manifest, identity: result.identity }; + } catch { + return undefined; + } +} + +/** + * Verified run catalog over an ordered set of storage roots. Corrupt entries, + * symlinks, directory/manifest identity mismatches, and duplicate ids from + * lower-precedence roots are omitted. Ordering is newest-first with run-id and + * root-precedence tie breakers, independent of filesystem enumeration order. + */ +function identityOf(entry: RunCatalogEntry): CatalogIdentity | undefined { + return (entry as BoundRunCatalogEntry)[ENTRY_IDENTITY]; +} + +export class RunCatalog { + readonly roots: readonly RunCatalogRoot[]; + + constructor(roots: readonly RunCatalogRoot[]) { + this.roots = roots.map((root) => ({ + dir: path.resolve(root.dir), + expectedRealDir: path.resolve(root.expectedRealDir), + })); + } + + async list(): Promise { + const entries: BoundRunCatalogEntry[] = []; + const seenRunIds = new Set(); + + for ( + let rootPrecedence = 0; + rootPrecedence < this.roots.length; + rootPrecedence += 1 + ) { + const root = this.roots[rootPrecedence]!; + if ((await verifiedRoot(root)) === undefined) continue; + + let dirEntries: fs.Dirent[]; + try { + dirEntries = await fs.promises.readdir(root.dir, { + withFileTypes: true, + }); + } catch (error) { + if (isMissing(error)) continue; + throw error; + } + dirEntries.sort((left, right) => compareText(left.name, right.name)); + + for (const dirEntry of dirEntries) { + if ( + dirEntry.isSymbolicLink() || + !dirEntry.isDirectory() || + !isSafeEntryName(dirEntry.name) || + seenRunIds.has(dirEntry.name) + ) { + continue; + } + const bound = await readBoundManifest(root, dirEntry.name); + if (bound === undefined) continue; + seenRunIds.add(dirEntry.name); + entries.push({ + dirName: dirEntry.name, + dir: path.join(root.dir, dirEntry.name), + rootDir: root.dir, + rootPrecedence, + manifest: bound.manifest, + [ENTRY_IDENTITY]: bound.identity, + }); + } + } + + entries.sort( + (left, right) => + compareText(right.manifest.createdAt, left.manifest.createdAt) || + compareText(left.manifest.runId, right.manifest.runId) || + left.rootPrecedence - right.rootPrecedence, + ); + return entries; + } + + async find(runId?: string): Promise { + if (runId !== undefined && !isSafeEntryName(runId)) return undefined; + const entries = await this.list(); + return runId === undefined + ? entries[0] + : entries.find((entry) => entry.manifest.runId === runId); + } + + async refresh(entry: RunCatalogEntry): Promise { + const root = this.roots[entry.rootPrecedence]; + const identity = identityOf(entry); + if ( + root === undefined || + identity === undefined || + entry.rootDir !== root.dir || + entry.dir !== path.join(root.dir, entry.dirName) + ) { + return undefined; + } + return (await readBoundManifest(root, entry.dirName, identity))?.manifest; + } + + async hasRootFile(entry: RunCatalogEntry, fileName: string): Promise { + const root = this.roots[entry.rootPrecedence]; + const identity = identityOf(entry); + if ( + root === undefined || + identity === undefined || + (await this.refresh(entry)) === undefined + ) { + return false; + } + try { + await readVerifiedRootFile( + root, + entry.dirName, + fileName, + false, + identity, + ); + return (await this.refresh(entry)) !== undefined; + } catch { + return false; + } + } + + async readRootFile(entry: RunCatalogEntry, fileName: string): Promise { + const root = this.roots[entry.rootPrecedence]; + const identity = identityOf(entry); + if ( + root === undefined || + identity === undefined || + (await this.refresh(entry)) === undefined + ) { + throw new RunCatalogAccessError(`Run not found: ${entry.dirName}`); + } + const result = await readVerifiedRootFile( + root, + entry.dirName, + fileName, + true, + identity, + ); + if ( + !Buffer.isBuffer(result.value) || + (await this.refresh(entry)) === undefined + ) { + throw new RunCatalogAccessError(`Run not found: ${entry.dirName}`); + } + return result.value; + } + + async readRootText(entry: RunCatalogEntry, fileName: string): Promise { + return (await this.readRootFile(entry, fileName)).toString("utf8"); + } + + async readRootTextIfPresent( + entry: RunCatalogEntry, + fileName: string, + ): Promise { + try { + return await this.readRootText(entry, fileName); + } catch (error) { + if (error instanceof RunCatalogAccessError && error.missing) { + return undefined; + } + throw error; + } + } +} + +/** Current storage seam. #34 can replace this one-root resolution with modes. */ +export async function openRunCatalog(projectDir: string): Promise { + let realProject: string; + try { + realProject = await fs.promises.realpath(projectDir); + } catch (error) { + if (isMissing(error)) return new RunCatalog([]); + throw error; + } + return new RunCatalog([ + { + dir: runsDir(projectDir), + expectedRealDir: path.join(realProject, ".picklab", "runs"), + }, + ]); +} + +/** Compatibility projection for callers that only need manifests. */ +export async function listRuns(projectDir: string): Promise { + return (await (await openRunCatalog(projectDir)).list()).map( + (entry) => entry.manifest, + ); +} diff --git a/packages/core/src/run.ts b/packages/core/src/run.ts index 5c261e1..8388bd7 100644 --- a/packages/core/src/run.ts +++ b/packages/core/src/run.ts @@ -175,60 +175,4 @@ export async function createRun( return new RunHandle(runDir, manifest); } -export async function listRuns(projectDir: string): Promise { - const parent = runsDir(projectDir); - - // Confine the runs root to the real `.picklab/runs` under the real project - // dir. This rejects a symlinked `.picklab` or `.picklab/runs` (which could - // redirect reads to outside runs) while still allowing the project dir - // itself to be a symlink. - try { - const realProject = await fs.promises.realpath(projectDir); - const realParent = await fs.promises.realpath(parent); - if (realParent !== path.join(realProject, ".picklab", "runs")) { - return []; - } - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") { - return []; - } - throw error; - } - - let entries: fs.Dirent[]; - try { - entries = await fs.promises.readdir(parent, { withFileTypes: true }); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") { - return []; - } - throw error; - } - - const manifests: RunManifest[] = []; - for (const entry of entries) { - // Skip symlinked run entries; only follow real directories. - if (entry.isSymbolicLink() || !entry.isDirectory()) continue; - const manifestPath = path.join(parent, entry.name, "manifest.json"); - try { - const manifestStat = await fs.promises.lstat(manifestPath); - if (manifestStat.isSymbolicLink()) continue; - const raw = await fs.promises.readFile(manifestPath, "utf8"); - const parsed: unknown = JSON.parse(raw); - if ( - typeof parsed === "object" && - parsed !== null && - typeof (parsed as RunManifest).runId === "string" && - typeof (parsed as RunManifest).createdAt === "string" && - Array.isArray((parsed as RunManifest).artifacts) - ) { - manifests.push(parsed as RunManifest); - } - } catch { - continue; - } - } - - manifests.sort((a, b) => b.createdAt.localeCompare(a.createdAt)); - return manifests; -} +export { listRuns } from "./run-catalog.js"; diff --git a/packages/core/test/evidence.test.ts b/packages/core/test/evidence.test.ts index c55e472..f4c167c 100644 --- a/packages/core/test/evidence.test.ts +++ b/packages/core/test/evidence.test.ts @@ -1197,6 +1197,47 @@ describe("finalized-run retention", () => { expect(removed).not.toContain(victim); }); + it("does not prune a real directory replacement after catalog discovery", async () => { + const ids: string[] = []; + for (let i = 0; i < 21; i += 1) { + const minute = String(i).padStart(2, "0"); + ids.push(await finalizedEvidenceRun(`2026-06-09T10:${minute}:00Z`)); + } + const victim = ids[0]!; + const victimDir = path.join(runsRoot(), victim); + const replacementManifest = await fs.promises.readFile( + path.join(victimDir, "manifest.json"), + "utf8", + ); + const realReaddir = fs.promises.readdir.bind(fs.promises); + let rootReads = 0; + const spy = vi.spyOn(fs.promises, "readdir").mockImplementation((async ( + ...args: Parameters + ) => { + if (path.resolve(String(args[0])) === runsRoot()) { + rootReads += 1; + if (rootReads === 2) { + await fs.promises.rename(victimDir, `${victimDir}-original`); + await fs.promises.mkdir(victimDir); + await fs.promises.writeFile( + path.join(victimDir, "manifest.json"), + replacementManifest, + ); + } + } + return (realReaddir as typeof fs.promises.readdir)(...args); + }) as typeof fs.promises.readdir); + + try { + expect(await pruneFinalizedEvidenceRuns(project, { keep: 20 })).toEqual( + [], + ); + expect(fs.existsSync(victimDir)).toBe(true); + } finally { + spy.mockRestore(); + } + }); + it("resists fake-inflation manifests that point at a real run", async () => { // One real finalized run we intend to keep. const real = await finalizedEvidenceRun("2026-06-09T12:00:00Z"); diff --git a/packages/core/test/run-catalog.test.ts b/packages/core/test/run-catalog.test.ts new file mode 100644 index 0000000..ca8fa7d --- /dev/null +++ b/packages/core/test/run-catalog.test.ts @@ -0,0 +1,216 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + RunCatalog, + createRun, + openRunCatalog, + type RunManifest, +} from "../src/index.js"; + +let root: string; + +beforeEach(async () => { + root = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-run-catalog-"), + ); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +async function writeRun( + runsRoot: string, + runId: string, + overrides: Partial = {}, +): Promise { + const runDir = path.join(runsRoot, runId); + await fs.promises.mkdir(runDir, { recursive: true }); + const manifest: RunManifest = { + runId, + slug: runId, + createdAt: "2026-06-09T12:00:00.000Z", + status: "completed", + artifacts: [], + ...overrides, + }; + await fs.promises.writeFile( + path.join(runDir, "manifest.json"), + `${JSON.stringify(manifest)}\n`, + ); + return runDir; +} + +describe("RunCatalog", () => { + it("uses root precedence for duplicate ids and deterministic tie ordering", async () => { + const primary = path.join(root, "primary"); + const fallback = path.join(root, "fallback"); + await writeRun(primary, "same", { slug: "primary" }); + await writeRun(fallback, "same", { slug: "fallback" }); + await writeRun(fallback, "z-run"); + await writeRun(fallback, "a-run"); + await writeRun(fallback, "B-run"); + + const catalog = new RunCatalog([ + { dir: primary, expectedRealDir: primary }, + { dir: fallback, expectedRealDir: fallback }, + ]); + const entries = await catalog.list(); + + expect(entries.map((entry) => entry.manifest.runId)).toEqual([ + "B-run", + "a-run", + "same", + "z-run", + ]); + expect(entries.find((entry) => entry.dirName === "same")?.manifest.slug).toBe( + "primary", + ); + }); + + it("fails closed when a manifest run id differs from its directory", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project); + const run = await createRun(project, "mismatch"); + const manifestPath = path.join(run.dir, "manifest.json"); + const manifest = JSON.parse( + await fs.promises.readFile(manifestPath, "utf8"), + ) as RunManifest; + manifest.runId = "different-run"; + await fs.promises.writeFile(manifestPath, JSON.stringify(manifest)); + + expect(await (await openRunCatalog(project)).list()).toEqual([]); + }); + + it("rejects a real directory replacement after discovery", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project); + const run = await createRun(project, "replace", { evidence: true }); + const catalog = await openRunCatalog(project); + const entry = await catalog.find(run.runId); + expect(entry).toBeDefined(); + if (entry === undefined) throw new Error("expected catalog entry"); + + await fs.promises.rename(run.dir, `${run.dir}-original`); + await writeRun(path.dirname(run.dir), path.basename(run.dir), { + evidenceVersion: 1, + actionLog: "actions.jsonl", + }); + await fs.promises.writeFile( + path.join(run.dir, "actions.jsonl"), + '{"actionId":"replacement"}\n', + ); + + await expect(catalog.readRootText(entry, "actions.jsonl")).rejects.toThrow( + /not found|changed/i, + ); + }); + + it("does not classify a replacement directory's missing file as absent", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project); + const run = await createRun(project, "replace-missing", { evidence: true }); + const catalog = await openRunCatalog(project); + const entry = await catalog.find(run.runId); + expect(entry).toBeDefined(); + if (entry === undefined) throw new Error("expected catalog entry"); + + const replacement = path.join(root, "replacement-run"); + await writeRun(path.dirname(replacement), path.basename(replacement), { + runId: run.runId, + evidenceVersion: 1, + actionLog: "actions.jsonl", + }); + const target = path.join(run.dir, "actions.jsonl"); + const realLstat = fs.promises.lstat.bind(fs.promises); + let swapped = false; + vi.spyOn(fs.promises, "lstat").mockImplementation(async (...args) => { + if (!swapped && path.resolve(String(args[0])) === target) { + swapped = true; + await fs.promises.rename(run.dir, `${run.dir}-original`); + await fs.promises.rename(replacement, run.dir); + } + return realLstat(...args); + }); + + await expect( + catalog.readRootTextIfPresent(entry, "actions.jsonl"), + ).rejects.toThrow(/changed|open/i); + expect(swapped).toBe(true); + }); + + it("distinguishes a missing root file from an unsafe symlink", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project); + const run = await createRun(project, "optional"); + const catalog = await openRunCatalog(project); + const entry = await catalog.find(run.runId); + expect(entry).toBeDefined(); + if (entry === undefined) throw new Error("expected catalog entry"); + + expect(await catalog.readRootTextIfPresent(entry, "actions.jsonl")).toBeUndefined(); + await fs.promises.symlink( + path.join(run.dir, "manifest.json"), + path.join(run.dir, "actions.jsonl"), + ); + await expect( + catalog.readRootTextIfPresent(entry, "actions.jsonl"), + ).rejects.toThrow(/unsafe/i); + }); + + it("rejects a run-directory swap while reading a verified root file", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project); + const run = await createRun(project, "swap", { evidence: true }); + await fs.promises.writeFile( + path.join(run.dir, "actions.jsonl"), + '{"actionId":"safe"}\n', + ); + const catalog = await openRunCatalog(project); + const entry = await catalog.find(run.runId); + expect(entry).toBeDefined(); + if (entry === undefined) throw new Error("expected catalog entry"); + + const backup = `${run.dir}-backup`; + const outsideRun = path.join(root, "outside-run"); + await writeRun(path.dirname(outsideRun), path.basename(outsideRun), { + runId: run.runId, + }); + await fs.promises.writeFile( + path.join(outsideRun, "actions.jsonl"), + '{"actionId":"outside"}\n', + ); + + const target = path.join(run.dir, "actions.jsonl"); + const realOpen = fs.promises.open.bind(fs.promises); + let swapped = false; + vi.spyOn(fs.promises, "open").mockImplementation(async (...args) => { + if (!swapped && path.resolve(String(args[0])) === target) { + swapped = true; + await fs.promises.rename(run.dir, backup); + await fs.promises.symlink(outsideRun, run.dir); + } + return realOpen(...args); + }); + + await expect(catalog.readRootText(entry, "actions.jsonl")).rejects.toThrow( + /changed|not found/i, + ); + expect(swapped).toBe(true); + }); + + it("never follows a symlinked catalog root", async () => { + const realRoot = path.join(root, "real-runs"); + const linkedRoot = path.join(root, "linked-runs"); + await writeRun(realRoot, "hidden"); + await fs.promises.symlink(realRoot, linkedRoot); + + const catalog = new RunCatalog([ + { dir: linkedRoot, expectedRealDir: realRoot }, + ]); + expect(await catalog.list()).toEqual([]); + }); +}); diff --git a/packages/mcp-server/src/resources.ts b/packages/mcp-server/src/resources.ts index 7c13dc1..a75cf05 100644 --- a/packages/mcp-server/src/resources.ts +++ b/packages/mcp-server/src/resources.ts @@ -10,15 +10,15 @@ import { EVIDENCE_REPORT, getSession, isEvidenceRun, - listRuns, listSessions, + openRunCatalog, parseActionsJournal, redactSecrets, - runsDir, sortEvidenceRecords, + type RunCatalog, + type RunCatalogEntry, } from "@pickforge/picklab-core"; import type { ServerContext } from "./context.js"; -import { isSafeRunId } from "./tools/artifacts.js"; import { sessionStatusEntry } from "./tools/session.js"; const SAFE_NAME_PATTERN = /^[A-Za-z0-9._-]+$/; @@ -49,12 +49,11 @@ function decodeVariable(variables: Variables, label: string): string { } function runFilePath( - ctx: ServerContext, - runId: string, + entry: RunCatalogEntry, subdir: "screenshots" | "logs", name: string, ): string { - const base = path.join(runsDir(ctx.projectDir), runId, subdir); + const base = path.join(entry.dir, subdir); const resolved = path.resolve(base, name); if (resolved !== path.join(base, name)) { throw new Error(`Invalid resource path: ${name}`); @@ -66,24 +65,17 @@ function runFilePath( // outside the runs root). Returns true when the run dir is safe, false when it // is missing or escapes the runs root via symlinks. async function isRunDirSafe( - ctx: ServerContext, - runId: string, + catalog: RunCatalog, + entry: RunCatalogEntry, ): Promise { - const root = runsDir(ctx.projectDir); - const runDir = path.join(root, runId); + if ((await catalog.refresh(entry)) === undefined) return false; + const root = catalog.roots[entry.rootPrecedence]; + if (root === undefined) return false; try { - // The real runs root must be exactly `.picklab/runs` under the real - // project dir. This rejects a symlinked `.picklab` or `.picklab/runs` - // ancestor that would redirect reads to outside runs (core listRuns - // applies the same confinement), while allowing the project dir itself to - // be a symlink. - const realProject = await fs.promises.realpath(ctx.projectDir); - const realRoot = await fs.promises.realpath(root); - if (realRoot !== path.join(realProject, ".picklab", "runs")) { - return false; - } - const realRunDir = await fs.promises.realpath(runDir); - return realRunDir === path.join(realRoot, runId); + const realRoot = await fs.promises.realpath(entry.rootDir); + if (realRoot !== root.expectedRealDir) return false; + const realRunDir = await fs.promises.realpath(entry.dir); + return realRunDir === path.join(realRoot, entry.dirName); } catch { return false; } @@ -93,13 +85,13 @@ async function isRunDirSafe( // the file (or subdir) does not exist, return so the caller's read produces its // usual not-found error. async function assertWithinSubdir( - ctx: ServerContext, - runId: string, + catalog: RunCatalog, + entry: RunCatalogEntry, subdir: "screenshots" | "logs", filePath: string, notFound: () => Error, ): Promise { - if (!(await isRunDirSafe(ctx, runId))) { + if (!(await isRunDirSafe(catalog, entry))) { throw notFound(); } // Reject a symlinked artifact file even when its target stays inside the run @@ -115,7 +107,7 @@ async function assertWithinSubdir( } return; } - const runDir = path.join(runsDir(ctx.projectDir), runId); + const runDir = entry.dir; const base = path.join(runDir, subdir); let realRunDir: string; let realBase: string; @@ -138,87 +130,6 @@ async function assertWithinSubdir( } } -// Reject a missing fixed run file, a symlink, or a real location that escapes -// the run directory. -async function assertRootFileWithinRun( - ctx: ServerContext, - runId: string, - fileName: string, - filePath: string, - notFound: () => Error, -): Promise { - if (!(await isRunDirSafe(ctx, runId))) { - throw notFound(); - } - let expectedEntry: fs.Stats; - try { - expectedEntry = await fs.promises.lstat(filePath); - if (expectedEntry.isSymbolicLink() || !expectedEntry.isFile()) { - throw notFound(); - } - } catch (error) { - if ((error as NodeJS.ErrnoException).code === undefined) { - throw error; - } - throw notFound(); - } - const runDir = path.join(runsDir(ctx.projectDir), runId); - let realRunDir: string; - let realFile: string; - try { - realRunDir = await fs.promises.realpath(runDir); - realFile = await fs.promises.realpath(filePath); - } catch { - throw notFound(); - } - if (realFile !== path.join(realRunDir, fileName)) { - throw notFound(); - } - return expectedEntry; -} - -function sameFileIdentity(left: fs.Stats, right: fs.Stats): boolean { - return left.dev === right.dev && left.ino === right.ino; -} - -// Open the validated file without following its final component, bind the -// descriptor to the validated inode, and reject a pathname swap during read. -async function readTextFileNoFollow( - filePath: string, - expected: fs.Stats, - notFound: () => Error, -): Promise { - let handle: fs.promises.FileHandle; - try { - handle = await fs.promises.open( - filePath, - fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW, - ); - } catch { - throw notFound(); - } - try { - const opened = await handle.stat(); - if (!opened.isFile() || !sameFileIdentity(opened, expected)) { - throw notFound(); - } - const raw = await handle.readFile("utf8"); - const current = await fs.promises.lstat(filePath); - if ( - current.isSymbolicLink() || - !current.isFile() || - !sameFileIdentity(opened, current) - ) { - throw notFound(); - } - return raw; - } catch { - throw notFound(); - } finally { - await handle.close(); - } -} - // Read only the trailing `maxBytes` of a file (tail semantics), so an // oversized log never gets pulled fully into memory just to redact secrets // from it. Drops a partial leading line so the returned text starts cleanly. @@ -245,48 +156,16 @@ async function readTailUtf8( } } -async function isRootFileSafe( - ctx: ServerContext, - runId: string, - fileName: string, -): Promise { - if (!(await isRunDirSafe(ctx, runId))) return false; - const runDir = path.join(runsDir(ctx.projectDir), runId); - const filePath = path.join(runDir, fileName); - try { - const entry = await fs.promises.lstat(filePath); - if (entry.isSymbolicLink() || !entry.isFile()) return false; - const realRunDir = await fs.promises.realpath(runDir); - const realFile = await fs.promises.realpath(filePath); - return realFile === path.join(realRunDir, fileName); - } catch { - return false; - } -} - -// List runs whose run id is safe and whose manifest passes realpath -// confinement, so symlinked manifests are never exposed via listings. -async function listSafeRuns( - ctx: ServerContext, -): Promise>> { - const safe: Awaited> = []; - for (const manifest of await listRuns(ctx.projectDir)) { - if (!isSafeRunId(manifest.runId)) continue; - if (!(await isRootFileSafe(ctx, manifest.runId, "manifest.json"))) continue; - safe.push(manifest); - } - return safe; -} - async function listEvidenceRunFiles( ctx: ServerContext, fileName: typeof EVIDENCE_ACTION_LOG | typeof EVIDENCE_REPORT, ): Promise { + const catalog = await openRunCatalog(ctx.projectDir); const runIds: string[] = []; - for (const manifest of await listSafeRuns(ctx)) { - if (!isEvidenceRun(manifest)) continue; - if (!(await isRootFileSafe(ctx, manifest.runId, fileName))) continue; - runIds.push(manifest.runId); + for (const entry of await catalog.list()) { + if (!isEvidenceRun(entry.manifest)) continue; + if (!(await catalog.hasRootFile(entry, fileName))) continue; + runIds.push(entry.manifest.runId); } return runIds; } @@ -296,8 +175,9 @@ async function listRunFiles( subdir: "screenshots" | "logs", ): Promise> { const entries: Array<{ runId: string; name: string }> = []; - for (const manifest of await listSafeRuns(ctx)) { - const runDir = path.join(runsDir(ctx.projectDir), manifest.runId); + const catalog = await openRunCatalog(ctx.projectDir); + for (const entry of await catalog.list()) { + const runDir = entry.dir; const dir = path.join(runDir, subdir); try { const realRunDir = await fs.promises.realpath(runDir); @@ -315,14 +195,14 @@ async function listRunFiles( } for (const name of names) { if (!SAFE_NAME_PATTERN.test(name) || name.includes("..")) continue; - let entry: fs.Stats; + let fileStat: fs.Stats; try { - entry = await fs.promises.lstat(path.join(dir, name)); + fileStat = await fs.promises.lstat(path.join(dir, name)); } catch { continue; } - if (entry.isSymbolicLink()) continue; - entries.push({ runId: manifest.runId, name }); + if (fileStat.isSymbolicLink()) continue; + entries.push({ runId: entry.manifest.runId, name }); } } return entries; @@ -338,7 +218,8 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { mimeType: "application/json", }, async (uri) => { - const runs = (await listSafeRuns(ctx)).map((manifest) => ({ + const catalog = await openRunCatalog(ctx.projectDir); + const runs = (await catalog.list()).map(({ manifest }) => ({ runId: manifest.runId, slug: manifest.slug, createdAt: manifest.createdAt, @@ -361,11 +242,13 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { "run-manifest", new ResourceTemplate("picklab://runs/{runId}/manifest", { list: async () => ({ - resources: (await listSafeRuns(ctx)).map((manifest) => ({ - uri: `picklab://runs/${manifest.runId}/manifest`, - name: `Run ${manifest.runId} manifest`, - mimeType: "application/json", - })), + resources: ( + await (await openRunCatalog(ctx.projectDir)).list() + ).map(({ manifest }) => ({ + uri: `picklab://runs/${manifest.runId}/manifest`, + name: `Run ${manifest.runId} manifest`, + mimeType: "application/json", + })), }), }), { @@ -375,23 +258,15 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const manifestPath = path.join( - runsDir(ctx.projectDir), - runId, - "manifest.json", - ); - const expected = await assertRootFileWithinRun( - ctx, - runId, - "manifest.json", - manifestPath, - () => new Error(`Run not found: ${runId}`), - ); - const raw = await readTextFileNoFollow( - manifestPath, - expected, - () => new Error(`Run not found: ${runId}`), - ); + const catalog = await openRunCatalog(ctx.projectDir); + const entry = await catalog.find(runId); + if (entry === undefined) throw new Error(`Run not found: ${runId}`); + let raw: string; + try { + raw = await catalog.readRootText(entry, "manifest.json"); + } catch { + throw new Error(`Run not found: ${runId}`); + } return { contents: [ { uri: uri.href, mimeType: "application/json", text: raw }, @@ -420,28 +295,19 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const manifest = (await listSafeRuns(ctx)).find( - (candidate) => candidate.runId === runId, - ); - if (manifest === undefined || !isEvidenceRun(manifest)) { + const catalog = await openRunCatalog(ctx.projectDir); + const entry = await catalog.find(runId); + if (entry === undefined || !isEvidenceRun(entry.manifest)) { + throw new Error(`Actions not found: ${runId}`); + } + let raw: string; + try { + raw = await catalog.readRootText(entry, EVIDENCE_ACTION_LOG); + } catch { throw new Error(`Actions not found: ${runId}`); } - const runDir = path.join(runsDir(ctx.projectDir), runId); - const actionsPath = path.join(runDir, EVIDENCE_ACTION_LOG); - const expected = await assertRootFileWithinRun( - ctx, - runId, - EVIDENCE_ACTION_LOG, - actionsPath, - () => new Error(`Actions not found: ${runId}`), - ); let records; try { - const raw = await readTextFileNoFollow( - actionsPath, - expected, - () => new Error(`Actions not found: ${runId}`), - ); records = parseActionsJournal(raw, `run ${runId}`); } catch (error) { throw new Error( @@ -482,29 +348,17 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const manifest = (await listSafeRuns(ctx)).find( - (candidate) => candidate.runId === runId, - ); - if (manifest === undefined || !isEvidenceRun(manifest)) { + const catalog = await openRunCatalog(ctx.projectDir); + const entry = await catalog.find(runId); + if (entry === undefined || !isEvidenceRun(entry.manifest)) { + throw new Error(`Report not found: ${runId}`); + } + let html: string; + try { + html = await catalog.readRootText(entry, EVIDENCE_REPORT); + } catch { throw new Error(`Report not found: ${runId}`); } - const reportPath = path.join( - runsDir(ctx.projectDir), - runId, - EVIDENCE_REPORT, - ); - const expected = await assertRootFileWithinRun( - ctx, - runId, - EVIDENCE_REPORT, - reportPath, - () => new Error(`Report not found: ${runId}`), - ); - const html = await readTextFileNoFollow( - reportPath, - expected, - () => new Error(`Report not found: ${runId}`), - ); return { contents: [ { @@ -539,10 +393,15 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { if (!name.endsWith(".png")) { throw new Error(`Not a PNG screenshot: ${name}`); } - const filePath = runFilePath(ctx, runId, "screenshots", name); + const catalog = await openRunCatalog(ctx.projectDir); + const entry = await catalog.find(runId); + if (entry === undefined) { + throw new Error(`Screenshot not found: ${runId}/${name}`); + } + const filePath = runFilePath(entry, "screenshots", name); await assertWithinSubdir( - ctx, - runId, + catalog, + entry, "screenshots", filePath, () => new Error(`Screenshot not found: ${runId}/${name}`), @@ -604,10 +463,15 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { async (uri, variables) => { const runId = decodeVariable(variables, "runId"); const name = decodeVariable(variables, "name"); - const filePath = runFilePath(ctx, runId, "logs", name); + const catalog = await openRunCatalog(ctx.projectDir); + const entry = await catalog.find(runId); + if (entry === undefined) { + throw new Error(`Log not found: ${runId}/${name}`); + } + const filePath = runFilePath(entry, "logs", name); await assertWithinSubdir( - ctx, - runId, + catalog, + entry, "logs", filePath, () => new Error(`Log not found: ${runId}/${name}`), diff --git a/packages/mcp-server/src/tools/artifacts.ts b/packages/mcp-server/src/tools/artifacts.ts index 39a8547..437cc9b 100644 --- a/packages/mcp-server/src/tools/artifacts.ts +++ b/packages/mcp-server/src/tools/artifacts.ts @@ -1,49 +1,42 @@ -import path from "node:path"; import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; import { + EVIDENCE_ACTION_LOG, isEvidenceRun, - listRuns, - readActions, + openRunCatalog, + parseActionsJournal, renderRunReport, runsDir, - type RunManifest, + type RunCatalog, + type RunCatalogEntry, } from "@pickforge/picklab-core"; import { runTool, type ServerContext } from "../context.js"; -const RUN_ID_PATTERN = /^[A-Za-z0-9._-]+$/; - -export function isSafeRunId(runId: string): boolean { - return ( - RUN_ID_PATTERN.test(runId) && - runId !== "." && - runId !== ".." && - !runId.includes("..") - ); -} - export async function findRun( projectDir: string, runId: string | undefined, -): Promise<{ manifest: RunManifest; dir: string }> { - const manifests = (await listRuns(projectDir)).filter((candidate) => - isSafeRunId(candidate.runId), - ); - let manifest: RunManifest | undefined; - if (runId === undefined) { - manifest = manifests[0]; - if (manifest === undefined) { +): Promise<{ catalog: RunCatalog; entry: RunCatalogEntry }> { + const catalog = await openRunCatalog(projectDir); + const entry = await catalog.find(runId); + if (entry === undefined) { + if (runId === undefined) { throw new Error(`No runs found under ${runsDir(projectDir)}`); } - } else { - manifest = manifests.find((candidate) => candidate.runId === runId); - if (manifest === undefined) { - throw new Error( - `Run not found: ${runId} (see the artifact_list tool)`, - ); - } + throw new Error(`Run not found: ${runId} (see the artifact_list tool)`); } - return { manifest, dir: path.join(runsDir(projectDir), manifest.runId) }; + return { catalog, entry }; +} + +async function readCatalogActions( + catalog: RunCatalog, + entry: RunCatalogEntry, +): Promise> { + if (!isEvidenceRun(entry.manifest)) return []; + const raw = await catalog.readRootTextIfPresent( + entry, + EVIDENCE_ACTION_LOG, + ); + return raw === undefined ? [] : parseActionsJournal(raw, entry.dir); } export function registerArtifactTools( @@ -61,8 +54,9 @@ export function registerArtifactTools( }, () => runTool(async () => { - const manifests = await listRuns(ctx.projectDir); - const runs = manifests.map((manifest) => ({ + const catalog = await openRunCatalog(ctx.projectDir); + const entries = await catalog.list(); + const runs = entries.map(({ manifest }) => ({ runId: manifest.runId, slug: manifest.slug, createdAt: manifest.createdAt, @@ -86,8 +80,9 @@ export function registerArtifactTools( }, (args) => runTool(async () => { - const { manifest, dir } = await findRun(ctx.projectDir, args.runId); - const records = isEvidenceRun(manifest) ? await readActions(dir) : []; + const { catalog, entry } = await findRun(ctx.projectDir, args.runId); + const { manifest, dir } = entry; + const records = await readCatalogActions(catalog, entry); return { data: { runId: manifest.runId, diff --git a/packages/mcp-server/test/tools.test.ts b/packages/mcp-server/test/tools.test.ts index c66697a..d5a4413 100644 --- a/packages/mcp-server/test/tools.test.ts +++ b/packages/mcp-server/test/tools.test.ts @@ -262,6 +262,24 @@ describe("artifact report", () => { expect(report).toContain("[REDACTED]"); expect(report).not.toContain(PLANTED_TOKEN); }); + + it("fails closed for an unsafe evidence journal", async () => { + const runId = "20260609-120000-unsafe-evidence"; + const { dir } = writeSyntheticRun(dirs.projectDir, runId); + const manifestPath = path.join(dir, "manifest.json"); + const manifest = JSON.parse(fs.readFileSync(manifestPath, "utf8")); + manifest.evidenceVersion = 1; + manifest.actionLog = "actions.jsonl"; + fs.writeFileSync(manifestPath, `${JSON.stringify(manifest, null, 2)}\n`); + fs.symlinkSync(manifestPath, path.join(dir, "actions.jsonl")); + + const result = await lab.client.callTool({ + name: "artifact_report", + arguments: { runId }, + }); + expect(result.isError).toBe(true); + expect(JSON.stringify(result)).toContain("Unsafe run catalog file"); + }); }); describe("empty lab", () => {