From 0dbff62e69940f0c687e304f16b98778107d02e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 01:06:26 -0300 Subject: [PATCH 1/5] feat(core): default run storage outside the project (home mode) Screenshots, logs, manifests, and evidence journals now default to ~/.pickforge/picklab/projects//runs/ instead of /.picklab/runs/, so a default run never appears in `git status`. PICKLAB_HOME now defaults to ~/.pickforge/picklab (was ~/.picklab); the old default is still read non-destructively as a fallback for existing global config, agent state, and sessions. - Add a single storage resolver (resolveRunStorage) for home, project-local, and custom modes, with a stable per-project id derived from the canonical project path. Route run creation, the run catalog, and active-evidence-pointer resolution in core, CLI, and MCP through it. - openRunCatalog layers the resolved primary root with a read-only legacy project-local fallback root, so pre-existing project-local runs stay discoverable without migration. - Add PICKLAB_STORAGE_MODE / PICKLAB_STORAGE_PATH env overrides and a project/global config `storage` field. - Add a non-destructive legacy read fallback (resolveReadablePath, per-entry) for global config, agent state, and sessions across the ~/.picklab -> ~/.pickforge/picklab default-root change; picklab doctor flags a detected legacy home. - Update README/SECURITY docs and record the default change in docs/releases/UNRELEASED.md. Closes pickforge/picklab#34 --- README.md | 53 ++++- SECURITY.md | 10 +- docs/releases/UNRELEASED.md | 36 ++- packages/android/test/reaper.test.ts | 7 +- packages/browser/src/devtools-evidence.ts | 9 +- .../browser/test/devtools-evidence.test.ts | 5 +- .../browser/test/evidence-integration.test.ts | 10 +- packages/cli/src/commands/artifacts.ts | 9 +- packages/cli/src/program.ts | 6 +- packages/cli/src/provision/checks.ts | 12 + packages/cli/src/provision/detect.ts | 11 + packages/cli/test/checks.test.ts | 19 +- packages/cli/test/commands.test.ts | 4 + packages/core/src/config.ts | 26 ++- packages/core/src/evidence.ts | 140 +++++++----- packages/core/src/index.ts | 17 ++ packages/core/src/paths.ts | 77 +++++++ packages/core/src/run-catalog.ts | 93 ++++++-- packages/core/src/run.ts | 6 +- packages/core/src/session.ts | 38 ++-- packages/core/src/storage.ts | 123 +++++++++++ packages/core/src/target.ts | 2 + packages/core/test/config.test.ts | 82 ++++++- packages/core/test/evidence-render.test.ts | 6 +- .../core/test/evidence.concurrency.test.ts | 7 +- packages/core/test/evidence.test.ts | 41 ++-- packages/core/test/paths.test.ts | 101 ++++++++- packages/core/test/run-catalog.test.ts | 110 ++++++++++ packages/core/test/run.test.ts | 7 +- packages/core/test/session.test.ts | 80 ++++++- packages/core/test/storage.test.ts | 206 ++++++++++++++++++ packages/mcp-server/src/context.ts | 1 + packages/mcp-server/src/evidence.ts | 10 +- packages/mcp-server/src/resources.ts | 20 +- packages/mcp-server/src/tools/artifacts.ts | 22 +- packages/mcp-server/test/android.test.ts | 2 +- packages/mcp-server/test/evidence.test.ts | 11 +- packages/mcp-server/test/helpers.ts | 9 +- 38 files changed, 1271 insertions(+), 157 deletions(-) create mode 100644 packages/core/src/storage.ts create mode 100644 packages/core/test/storage.test.ts diff --git a/README.md b/README.md index 58d919d..f8830f7 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,52 @@ picklab artifacts report # render the latest run picklab session destroy --all ``` -Every screenshot, log, and action lands in `.picklab/runs//` with a manifest, so a run is inspectable and reproducible after the fact. +Every screenshot, log, and action lands in a run directory with a manifest, so a run is inspectable and reproducible after the fact. By default that run directory lives outside your project — see [Run storage](#run-storage) below. + +### Run storage + +By default, run artifacts (screenshots, logs, manifests, evidence journals) +are written under the shared Pickforge company root, **not** inside your +project — a default screenshot or run never shows up in `git status`: + +```text +~/.pickforge/picklab/projects//runs// +``` + +`` is a stable id derived from the project's canonical (symlink-resolved) +path: the same project always resolves to the same id, and different projects +never collide. Use the platform home-directory equivalent on non-Linux systems. +`PICKLAB_HOME` overrides the PickLab home root (default `~/.pickforge/picklab`); +`picklab doctor` reports the resolved path. + +Two other modes are available via `storage` in `.picklab/config.json` +(project-level) or the global config, or the `PICKLAB_STORAGE_MODE` / +`PICKLAB_STORAGE_PATH` environment overrides for automation and tests: + +```json +{ + "storage": { "mode": "project-local" } +} +``` + +- `home` (default) — the layout above. +- `project-local` — restores the previous default: `.picklab/runs/` inside the + project. Generated files then do appear in the project's source-control + view; add `.picklab/runs/` to `.gitignore` if you opt into this mode. +- `custom` — an explicit absolute path: `{ "storage": { "mode": "custom", "path": "/abs/path" } }` + writes runs under `/runs/`. A relative path, or `custom` mode with no + path, is rejected. + +`.picklab/config.json` itself always stays project-local regardless of +`storage` mode — only generated runtime artifacts move. + +**Upgrading from an earlier version:** existing runs already written under a +project's `.picklab/runs/` remain discoverable by `artifact_list` / +`artifact_report` / MCP resources without any migration step — nothing is +moved or deleted. Likewise, an existing `~/.picklab` global config, agent +state, or session registry (the previous default PickLab home) is still read +as a non-destructive fallback if the new `~/.pickforge/picklab` default has +nothing yet; `picklab doctor` flags a detected legacy home. ### Evidence recording @@ -68,7 +113,8 @@ desktop, Android, and session actions share the same append-only timeline as browser DevTools actions. Destroying a session, or reaping a dead one, finalizes the run and writes a static `report.html` filmstrip. -A finalized evidence run under `.picklab/runs//` contains: +A finalized evidence run directory (see [Run storage](#run-storage) for where +it lives) contains: - `manifest.json` — run identity, status, and evidence metadata - `actions.jsonl` — authoritative, append-only sanitized action timeline @@ -144,7 +190,8 @@ For any other agent, add the stdio server yourself: `picklab-browser` is static. Each invocation discovers the one live browser session for the agent's project and derives its loopback CDP URL in memory, so recreating a session never requires an agent config edit. The relay runs the bundled, exact `chrome-devtools-mcp@1.5.0`; it does not use `npx` or connect to a personal browser. -Custom agents can be stored under `~/.picklab/agents`: +Custom agents can be stored under the PickLab home's `agents/` dir (default +`~/.pickforge/picklab/agents`, override via `PICKLAB_HOME`): ```sh picklab agents add --name my-agent --mcp-command "picklab mcp serve" diff --git a/SECURITY.md b/SECURITY.md index 3cca796..2a4d57d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -20,10 +20,12 @@ fuller model. ## Recorded evidence and screenshots -Computer-use evidence is stored as local project data under -`.picklab/runs//`. `actions.jsonl` is the authoritative sanitized -timeline; finalization produces an escaped, no-script `report.html` with a -restrictive content security policy and no external requests. +Computer-use evidence is stored as local run data, by default under +`~/.pickforge/picklab/projects//runs//` (outside the +project; see the README's "Run storage" section for storage modes). +`actions.jsonl` is the authoritative sanitized timeline; finalization +produces an escaped, no-script `report.html` with a restrictive content +security policy and no external requests. The recorder persists only allowlisted metadata. Typed and filled text becomes length plus input type. Failed network records keep method, URL origin/path diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 8acf629..3a45c06 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -5,6 +5,19 @@ release description, then reset it after the release is published. ## User-facing changes +- **Default run storage changed.** Screenshots, logs, manifests, and evidence + journals now default to `~/.pickforge/picklab/projects//runs/` + (outside the project) instead of `/.picklab/runs/`, so a default + run no longer shows up in `git status`. `PICKLAB_HOME` now defaults to + `~/.pickforge/picklab` (was `~/.picklab`); the old default is still read + non-destructively as a fallback for existing global config, agent state, + and sessions — nothing is moved or deleted. `project-local` (restores the + previous layout) and `custom` (explicit absolute path) storage modes are + available via `.picklab/config.json`'s `storage` field or the + `PICKLAB_STORAGE_MODE` / `PICKLAB_STORAGE_PATH` env overrides. Existing + project-local runs remain discoverable by `artifact_list` / + `artifact_report` / MCP resources without migration. See the README's "Run + storage" section. - Added isolated headed Chrome/Chromium sessions, including CLI and MCP session lifecycle support and a static `picklab-browser` DevTools MCP entry. - Added `picklab watch` plus configurable manual/automatic VNC viewer attachment @@ -26,6 +39,15 @@ release description, then reset it after the release is published. ## Internal/release changes +- Added a single storage resolver (`resolveRunStorage`) covering home, + project-local, and custom modes with stable per-project id derivation + (sha256 of the canonical project path); routed run creation, the run + catalog, and active-evidence-pointer resolution in core, CLI, and MCP + through it. `openRunCatalog` now layers the resolved primary root with a + read-only legacy project-local fallback root for non-destructive discovery. + Added a legacy read-fallback (`resolveReadablePath`, per-entry) for global + config, agent state, and sessions across the `~/.picklab` → + `~/.pickforge/picklab` default-root change. - Added a private browser lifecycle package with confined ephemeral profiles, loopback-only CDP discovery, verified process-group teardown, concurrent session safety, and conservative dead-session reaping. @@ -60,12 +82,22 @@ 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) +- New `storage.test.ts` (project id derivation, all three storage modes, env + overrides) and `run-catalog.test.ts`'s "openRunCatalog storage modes" suite + (home default, legacy project-local discovery, project isolation, custom + mode), plus a `git status --porcelain` repo-cleanliness smoke test. +- `bun run test` — same pre-existing failure set as unmodified `main` on this + (macOS) sandbox (Darwin lacks `/proc`, so PID-identity/Xvfb/x11vnc-driven + tests fail there and on CI's Linux runners this class does not apply); + verified test-by-test against `main` on this branch's dev sandbox. - `bun run build` ### Not tested yet +- `bun run test:coverage` on a machine where the full suite runs clean (this + dev sandbox can't produce a coverage summary because the pre-existing + Darwin-only failures above stop the process before the v8 coverage + provider flushes it — reproduced identically on unmodified `main`). - Platform smoke checks outside Linux. - Live remote SSH-tunnel smoke test. - Tag-triggered npm publish and draft-release creation (runs only after merge and diff --git a/packages/android/test/reaper.test.ts b/packages/android/test/reaper.test.ts index 30c43ae..bec4dc6 100644 --- a/packages/android/test/reaper.test.ts +++ b/packages/android/test/reaper.test.ts @@ -85,7 +85,12 @@ describe("android reaper tracking", () => { bootTimeoutMs: 5_000, }); - const { run } = await beginEvidenceRun(projectDir, session.id); + const { run } = await beginEvidenceRun( + projectDir, + session.id, + {}, + registryEnv, + ); forceStopFailure = true; try { diff --git a/packages/browser/src/devtools-evidence.ts b/packages/browser/src/devtools-evidence.ts index 5470557..976f63c 100644 --- a/packages/browser/src/devtools-evidence.ts +++ b/packages/browser/src/devtools-evidence.ts @@ -401,9 +401,12 @@ export async function createDevtoolsEvidenceRecorder( ): Promise { const config = await loadConfig(opts.projectDir, opts.env); if (!isEvidenceEnabled(config)) return undefined; - const { run } = await beginEvidenceRun(opts.projectDir, opts.sessionId, { - slug: "computer-use", - }); + const { run } = await beginEvidenceRun( + opts.projectDir, + opts.sessionId, + { slug: "computer-use" }, + opts.env, + ); const runStat = await fs.promises.lstat(run.dir); if (!runStat.isDirectory() || runStat.isSymbolicLink()) { throw new Error("Evidence run directory is not a real directory"); diff --git a/packages/browser/test/devtools-evidence.test.ts b/packages/browser/test/devtools-evidence.test.ts index fddb0f3..21d74d8 100644 --- a/packages/browser/test/devtools-evidence.test.ts +++ b/packages/browser/test/devtools-evidence.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { listRuns, readActions, @@ -19,9 +19,12 @@ beforeEach(async () => { projectDir = await fs.promises.mkdtemp( path.join(os.tmpdir(), "picklab-devtools-evidence-"), ); + // These tests assert against the literal `.picklab/runs` layout. + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(async () => { + vi.unstubAllEnvs(); await fs.promises.rm(projectDir, { recursive: true, force: true }); }); diff --git a/packages/browser/test/evidence-integration.test.ts b/packages/browser/test/evidence-integration.test.ts index 708e49e..9daf6ab 100644 --- a/packages/browser/test/evidence-integration.test.ts +++ b/packages/browser/test/evidence-integration.test.ts @@ -9,7 +9,7 @@ import { destroySessionRecord, listRuns, readActions, - runsDir, + resolveRunStorage, type EnvLike, type SessionRecord, } from "@pickforge/picklab-core"; @@ -158,6 +158,7 @@ describe("browser evidence integration", () => { const evidence = await createDevtoolsEvidenceRecorder({ projectDir, sessionId, + env: registryEnv, }); expect(evidence).toBeDefined(); const requests = [ @@ -205,9 +206,12 @@ describe("browser evidence integration", () => { expect(Buffer.concat(output).toString()).toContain(QUERY_TOKEN); await destroySessionRecord(sessionId, registryEnv, "failed"); - const [finalized] = await listRuns(projectDir); + const [finalized] = await listRuns(projectDir, registryEnv); expect(finalized).toBeDefined(); - const runDir = path.join(runsDir(projectDir), finalized!.runId); + const runDir = path.join( + (await resolveRunStorage(projectDir, registryEnv)).runsDir, + finalized!.runId, + ); const reportPath = path.join(runDir, "report.html"); const records = await readActions(runDir); const html = await fs.promises.readFile(reportPath, "utf8"); diff --git a/packages/cli/src/commands/artifacts.ts b/packages/cli/src/commands/artifacts.ts index 9ad2d56..16ebf5f 100644 --- a/packages/cli/src/commands/artifacts.ts +++ b/packages/cli/src/commands/artifacts.ts @@ -5,7 +5,7 @@ import { openRunCatalog, parseActionsJournal, renderRunReport, - runsDir, + resolveRunStorage, type RunCatalog, type RunCatalogEntry, } from "@pickforge/picklab-core"; @@ -32,7 +32,9 @@ export async function runArtifactsList(opts: BaseCliOptions): Promise { data: { projectDir, runs }, lines: runs.length === 0 - ? [`no runs found under ${runsDir(projectDir)}`] + ? [ + `no runs found under ${(await resolveRunStorage(projectDir)).runsDir}`, + ] : runs.map( (run) => `${run.runId} ${run.status} ${run.artifacts} artifact(s)`, @@ -49,7 +51,8 @@ async function findRun( const entry = await catalog.find(runId); if (entry === undefined) { if (runId === undefined) { - throw new Error(`No runs found under ${runsDir(projectDir)}`); + const { runsDir } = await resolveRunStorage(projectDir); + throw new Error(`No runs found under ${runsDir}`); } throw new Error(`Run not found: ${runId} (see: picklab artifacts list)`); } diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index cdadc61..0b7c929 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -483,7 +483,9 @@ export function buildProgram(): Command { const artifacts = program .command("artifacts") - .description("Inspect run artifacts recorded under .picklab/runs"); + .description( + "Inspect recorded run artifacts (default storage: ~/.pickforge/picklab)", + ); withJson( withProjectDir( @@ -588,7 +590,7 @@ export function buildProgram(): Command { withJson( agents .command("add") - .description("Store a custom agent MCP config snippet under ~/.picklab/agents") + .description("Store a custom agent MCP config snippet under the PickLab home's agents dir") .requiredOption("--name ", "custom agent name") .requiredOption( "--mcp-command ", diff --git a/packages/cli/src/provision/checks.ts b/packages/cli/src/provision/checks.ts index 99c8fe7..b48b567 100644 --- a/packages/cli/src/provision/checks.ts +++ b/packages/cli/src/provision/checks.ts @@ -99,6 +99,18 @@ export function evaluateChecks(s: DetectionSnapshot): DoctorCheck[] { }); } + if (s.legacyHome !== null) { + checks.push({ + id: "legacy-home", + title: "Legacy PickLab home", + status: "warn", + detail: `${s.legacyHome.path} still exists (pre-#34 default)`, + hint: + "config, agent state, and sessions there are still read " + + "non-destructively as a fallback; nothing was moved or deleted", + }); + } + if (s.config.ok) { checks.push({ id: "config", diff --git a/packages/cli/src/provision/detect.ts b/packages/cli/src/provision/detect.ts index e510c75..920fe83 100644 --- a/packages/cli/src/provision/detect.ts +++ b/packages/cli/src/provision/detect.ts @@ -8,6 +8,7 @@ import { type SystemImage, } from "@pickforge/picklab-android"; import { + legacyPicklabHome, loadConfig, picklabHome, resolvedDefaults, @@ -22,6 +23,10 @@ import { export interface DetectionSnapshot { picklabHome: { path: string; exists: boolean; writable: boolean }; + /** Present only when the pre-#34 `~/.picklab` root still exists and + * differs from the current default (never when `PICKLAB_HOME` is set + * explicitly — that is the user's own root, not a legacy one). */ + legacyHome: { path: string } | null; config: { ok: boolean; error: string | null; profile: PicklabProfile | null }; desktop: { xvfb: string | null; @@ -121,6 +126,11 @@ export async function collectSnapshot( const homePath = picklabHome(env); const homeExists = dirExists(homePath); + const legacyPath = legacyPicklabHome(env); + const legacyHome = + legacyPath !== undefined && legacyPath !== homePath && dirExists(legacyPath) + ? { path: legacyPath } + : null; const androidEnv = detectAndroidEnvironment({ env, @@ -138,6 +148,7 @@ export async function collectSnapshot( exists: homeExists, writable: homeExists && isWritable(homePath), }, + legacyHome, config, desktop: { xvfb: findOnPath("Xvfb", env), diff --git a/packages/cli/test/checks.test.ts b/packages/cli/test/checks.test.ts index 02333ca..6642110 100644 --- a/packages/cli/test/checks.test.ts +++ b/packages/cli/test/checks.test.ts @@ -9,6 +9,7 @@ import type { DetectionSnapshot } from "../src/provision/detect.js"; function snapshot( overrides: { picklabHome?: Partial; + legacyHome?: DetectionSnapshot["legacyHome"]; config?: Partial; desktop?: Partial; android?: Partial; @@ -18,11 +19,12 @@ function snapshot( ): DetectionSnapshot { return { picklabHome: { - path: "/home/u/.picklab", + path: "/home/u/.pickforge/picklab", exists: true, writable: true, ...overrides.picklabHome, }, + legacyHome: overrides.legacyHome ?? null, config: { ok: true, error: null, profile: null, ...overrides.config }, desktop: { xvfb: "/usr/bin/Xvfb", @@ -94,6 +96,21 @@ describe("evaluateChecks", () => { expect(check.detail).toContain("not writable"); }); + it("omits the legacy-home check when there is nothing to report", () => { + const checks = evaluateChecks(snapshot()); + expect(checks.some((check) => check.id === "legacy-home")).toBe(false); + }); + + it("surfaces a detected legacy ~/.picklab home as a non-blocking warning", () => { + const check = checkById( + snapshot({ legacyHome: { path: "/home/u/.picklab" } }), + "legacy-home", + ); + expect(check.status).toBe("warn"); + expect(check.detail).toContain("/home/u/.picklab"); + expect(check.hint).toContain("non-destructively"); + }); + it("flags a broken config with its parse error", () => { const check = checkById( snapshot({ config: { ok: false, error: "Invalid PickLab config" } }), diff --git a/packages/cli/test/commands.test.ts b/packages/cli/test/commands.test.ts index 6449f82..bf60970 100644 --- a/packages/cli/test/commands.test.ts +++ b/packages/cli/test/commands.test.ts @@ -82,6 +82,10 @@ function makeEnv(opts: EnvOptions = {}): Record { return { HOME: home, PICKLAB_HOME: path.join(home, ".picklab"), + // Default new runs to the pre-#34 project-local layout so existing + // fixtures/assertions in this file keep working; tests that exercise the + // new "home" default explicitly override this via opts.extra. + PICKLAB_STORAGE_MODE: "project-local", PATH: pathParts.join(":"), ...(opts.extra ?? {}), }; diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 1e2751d..240213f 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -3,7 +3,9 @@ import path from "node:path"; import { ensureDir, globalConfigPath, + legacyGlobalConfigPath, projectConfigPath, + resolveReadablePath, writeFileAtomic, type EnvLike, } from "./paths.js"; @@ -16,12 +18,29 @@ export type PicklabProfile = export type ViewerMode = "manual" | "auto"; +/** + * Where new run artifacts (screenshots, manifests, evidence journals) are + * written. `home` (default) keeps them under the shared Pickforge company + * root, isolated per project; `project-local` restores the pre-#34 layout + * under `/.picklab/runs`; `custom` targets an explicit absolute + * path. See `resolveRunStorage` in `storage.ts`. + */ +export type StorageMode = "home" | "project-local" | "custom"; + +export interface StorageConfig { + mode?: StorageMode; + /** Required (and must be absolute) when `mode` is `"custom"`. */ + path?: string; + [key: string]: unknown; +} + export interface PicklabConfig { profile?: PicklabProfile; android?: { avdName?: string; [key: string]: unknown }; labUser?: { name?: string; home?: string; [key: string]: unknown }; viewer?: { mode?: ViewerMode; [key: string]: unknown }; evidence?: { enabled?: boolean; [key: string]: unknown }; + storage?: StorageConfig; [key: string]: unknown; } @@ -30,6 +49,7 @@ export const resolvedDefaults = { labUser: { name: "picklab-lab", home: "/var/lib/picklab/lab-home" }, viewer: { mode: "manual" }, evidence: { enabled: true }, + storage: { mode: "home" }, } as const satisfies PicklabConfig; /** @@ -95,7 +115,11 @@ export async function loadConfig( projectDir: string, env: EnvLike = process.env, ): Promise { - const global = await readConfigFile(globalConfigPath(env)); + const globalPath = await resolveReadablePath( + globalConfigPath(env), + legacyGlobalConfigPath(env), + ); + const global = await readConfigFile(globalPath); const project = await readConfigFile(projectConfigPath(projectDir)); return deepMerge( deepMerge(deepMerge({}, resolvedDefaults), global), diff --git a/packages/core/src/evidence.ts b/packages/core/src/evidence.ts index e6b9b15..e4fcaa3 100644 --- a/packages/core/src/evidence.ts +++ b/packages/core/src/evidence.ts @@ -1,8 +1,9 @@ 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 { ensureDir, writeFileAtomic, type EnvLike } from "./paths.js"; import { openRunCatalog } from "./run-catalog.js"; +import { resolveRunStorage } from "./storage.js"; import { isPidAlive, processIdentityMatches, @@ -233,12 +234,14 @@ function manifestMatchesPointer( } /** Absolute path of the active-run pointer for a session. */ -export function activePointerPath( +export async function activePointerPath( projectDir: string, sessionId: string, -): string { + env: EnvLike = process.env, +): Promise { assertSafeSessionId(sessionId); - return path.join(runsDir(projectDir), `.active-${sessionId}.json`); + const parent = (await resolveRunStorage(projectDir, env)).runsDir; + return path.join(parent, `.active-${sessionId}.json`); } function parsePointer(raw: string): ActiveEvidencePointer | undefined { @@ -351,8 +354,11 @@ async function readManifest( export async function resolveActivePointer( projectDir: string, sessionId: string, + env: EnvLike = process.env, ): Promise { - const pointerPath = activePointerPath(projectDir, sessionId); + assertSafeSessionId(sessionId); + const parent = (await resolveRunStorage(projectDir, env)).runsDir; + const pointerPath = path.join(parent, `.active-${sessionId}.json`); let raw: string; try { raw = await fs.promises.readFile(pointerPath, "utf8"); @@ -366,9 +372,7 @@ export async function resolveActivePointer( if (claim !== undefined) return { status: "claiming", raw, claim }; const pointer = parsePointer(raw); if (pointer === undefined) return { status: "corrupt", raw }; - const manifest = await readManifest( - path.join(runsDir(projectDir), pointer.runId), - ); + const manifest = await readManifest(path.join(parent, pointer.runId)); if ( manifest === undefined || manifest.status !== "running" || @@ -396,8 +400,11 @@ export async function clearActivePointer( projectDir: string, sessionId: string, opts: { expectRaw?: string; force?: boolean } = {}, + env: EnvLike = process.env, ): Promise { - const pointerPath = activePointerPath(projectDir, sessionId); + assertSafeSessionId(sessionId); + const parent = (await resolveRunStorage(projectDir, env)).runsDir; + const pointerPath = path.join(parent, `.active-${sessionId}.json`); if (opts.force === true) { return unlinkIfPresent(pointerPath); } @@ -413,7 +420,7 @@ export async function clearActivePointer( return unlinkIfMatches(pointerPath, current); } // Default: only clear a pointer that is genuinely stale/corrupt. - const resolution = await resolveActivePointer(projectDir, sessionId); + const resolution = await resolveActivePointer(projectDir, sessionId, env); if (resolution.status === "stale" || resolution.status === "corrupt") { return unlinkIfMatches(pointerPath, current); } @@ -612,11 +619,12 @@ export async function beginEvidenceRun( projectDir: string, sessionId: string, opts: BeginEvidenceRunOptions = {}, + env: EnvLike = process.env, ): Promise { assertSafeSessionId(sessionId); - const parent = runsDir(projectDir); + const parent = (await resolveRunStorage(projectDir, env)).runsDir; await ensureDir(parent); - const pointerPath = activePointerPath(projectDir, sessionId); + const pointerPath = path.join(parent, `.active-${sessionId}.json`); const ownerPid = process.pid; const ownerStartTicks = readProcessStartTicks(ownerPid); const deadline = Date.now() + CLAIM_TOTAL_DEADLINE_MS; @@ -635,7 +643,7 @@ export async function beginEvidenceRun( handle = await fs.promises.open(pointerPath, "wx"); } catch (error) { if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; - const resolution = await resolveActivePointer(projectDir, sessionId); + const resolution = await resolveActivePointer(projectDir, sessionId, env); if (resolution.status === "active") { return adopt(resolution.pointer, resolution.manifest); } @@ -652,15 +660,21 @@ export async function beginEvidenceRun( await delay(claimBackoff(attempt)); continue; } - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }); + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ); continue; } if (attempt >= EMPTY_CLAIM_GRACE_ATTEMPTS) { - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }); + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ); } if (Date.now() >= deadline) break; await delay(claimBackoff(attempt)); @@ -671,9 +685,12 @@ export async function beginEvidenceRun( continue; } // stale or corrupt: clear exactly this content, then retry the claim. - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }); + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ); continue; } @@ -700,9 +717,12 @@ export async function beginEvidenceRun( } } catch (error) { await handle.close().catch(() => {}); - await clearActivePointer(projectDir, sessionId, { - expectRaw: claimContent, - }).catch(() => {}); + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: claimContent }, + env, + ).catch(() => {}); // Fall back to unconditional cleanup if the claim was never readable. await unlinkIfMatches(pointerPath, "").catch(() => {}); throw error; @@ -713,12 +733,12 @@ export async function beginEvidenceRun( let run: RunHandle | undefined; try { if (opts._afterClaim !== undefined) await opts._afterClaim(); - run = await createRun(projectDir, opts.slug ?? "evidence", { - now: opts.now, - sessionId, - meta: opts.meta, - evidence: true, - }); + run = await createRun( + projectDir, + opts.slug ?? "evidence", + { now: opts.now, sessionId, meta: opts.meta, evidence: true }, + env, + ); const pointer: ActiveEvidencePointer = { evidenceVersion: EVIDENCE_VERSION, @@ -762,14 +782,17 @@ export async function beginEvidenceRun( if (run !== undefined) { await run.finish("failed").catch(() => {}); } - await clearActivePointer(projectDir, sessionId, { - expectRaw: claimContent, - }).catch(() => {}); + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: claimContent }, + env, + ).catch(() => {}); if (error instanceof ClaimLostError) { // Another owner took the session. Adopt it if it is active; otherwise // fall through to retry within the remaining budget. - const resolution = await resolveActivePointer(projectDir, sessionId); + const resolution = await resolveActivePointer(projectDir, sessionId, env); if (resolution.status === "active") { return adopt(resolution.pointer, resolution.manifest); } @@ -794,9 +817,11 @@ export async function finalizeActiveEvidenceRun( projectDir: string, sessionId: string, status: RunStatus = "completed", + env: EnvLike = process.env, ): Promise { + const parent = (await resolveRunStorage(projectDir, env)).runsDir; for (let attempt = 0; attempt < MAX_CLAIM_ATTEMPTS; attempt += 1) { - const resolution = await resolveActivePointer(projectDir, sessionId); + const resolution = await resolveActivePointer(projectDir, sessionId, env); if (resolution.status === "absent") return undefined; if (resolution.status === "claiming") { @@ -813,9 +838,12 @@ export async function finalizeActiveEvidenceRun( continue; } if ( - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }) + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ) ) { return undefined; } @@ -823,9 +851,12 @@ export async function finalizeActiveEvidenceRun( } if (resolution.status === "corrupt") { if ( - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }) + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ) ) { return undefined; } @@ -834,7 +865,7 @@ export async function finalizeActiveEvidenceRun( const pointer = resolution.pointer; if (pointer === undefined) continue; - const runDir = path.join(runsDir(projectDir), pointer.runId); + const runDir = path.join(parent, pointer.runId); const manifest = resolution.status === "active" ? resolution.manifest @@ -845,9 +876,12 @@ export async function finalizeActiveEvidenceRun( !manifestMatchesPointer(manifest, pointer, sessionId) ) { if ( - await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - }) + await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + ) ) { return undefined; } @@ -859,13 +893,16 @@ export async function finalizeActiveEvidenceRun( await new RunHandle(runDir, manifest).finish(status); } if ( - !(await clearActivePointer(projectDir, sessionId, { - expectRaw: resolution.raw, - })) + !(await clearActivePointer( + projectDir, + sessionId, + { expectRaw: resolution.raw }, + env, + )) ) { continue; } - await pruneFinalizedEvidenceRuns(projectDir); + await pruneFinalizedEvidenceRuns(projectDir, {}, env); return manifest; } throw new Error( @@ -1643,9 +1680,10 @@ async function collectActiveRunIds(parent: string): Promise> { export async function pruneFinalizedEvidenceRuns( projectDir: string, opts: PruneEvidenceOptions = {}, + env: EnvLike = process.env, ): Promise { const keep = opts.keep ?? EVIDENCE_RETENTION_KEEP; - const catalog = await openRunCatalog(projectDir); + const catalog = await openRunCatalog(projectDir, env); const entries = await catalog.list(); const activeByRoot = new Map>(); for (const entry of entries) { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index e073783..fdd0b09 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -5,8 +5,14 @@ export { ensureDir, globalConfigPath, isProfileConfined, + legacyAgentsDir, + legacyGlobalConfigPath, + legacyPicklabHome, + legacySessionsDir, + listDirSafe, picklabHome, projectConfigPath, + resolveReadablePath, runsDir, sessionsDir, writeFileAtomic, @@ -23,9 +29,20 @@ export { saveProjectConfig, type PicklabConfig, type PicklabProfile, + type StorageConfig, + type StorageMode, type ViewerMode, } from "./config.js"; +export { + canonicalProjectPath, + deriveProjectId, + projectId, + resolveRunStorage, + StorageConfigError, + type ResolvedRunStorage, +} from "./storage.js"; + export { createRun, EVIDENCE_ACTION_LOG, diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts index 6cc2331..c5d57d2 100644 --- a/packages/core/src/paths.ts +++ b/packages/core/src/paths.ts @@ -4,11 +4,33 @@ import path from "node:path"; export type EnvLike = Record; +/** + * The PickLab home root, shared by every product under the Pickforge company + * root (`~/.pickforge//`). `PICKLAB_HOME` remains the override + * for automation/tests/custom installs. + */ export function picklabHome(env: EnvLike = process.env): string { const fromEnv = env.PICKLAB_HOME; if (fromEnv !== undefined && fromEnv !== "") { return fromEnv; } + return path.join(os.homedir(), ".pickforge", "picklab"); +} + +/** + * The pre-#34 PickLab home (`~/.picklab`). Only meaningful when the caller is + * on the default root (no explicit `PICKLAB_HOME`): once a user sets + * `PICKLAB_HOME` themselves they have taken explicit control and no legacy + * fallback applies. Existing global config, agent state, and sessions under + * this path are never migrated or deleted — callers that read single files or + * list directories fall back to this path only when the new default has + * nothing yet, so nothing already there is silently orphaned. + */ +export function legacyPicklabHome(env: EnvLike = process.env): string | undefined { + const fromEnv = env.PICKLAB_HOME; + if (fromEnv !== undefined && fromEnv !== "") { + return undefined; + } return path.join(os.homedir(), ".picklab"); } @@ -16,10 +38,20 @@ export function sessionsDir(env: EnvLike = process.env): string { return path.join(picklabHome(env), "sessions"); } +export function legacySessionsDir(env: EnvLike = process.env): string | undefined { + const legacyHome = legacyPicklabHome(env); + return legacyHome === undefined ? undefined : path.join(legacyHome, "sessions"); +} + export function agentsDir(env: EnvLike = process.env): string { return path.join(picklabHome(env), "agents"); } +export function legacyAgentsDir(env: EnvLike = process.env): string | undefined { + const legacyHome = legacyPicklabHome(env); + return legacyHome === undefined ? undefined : path.join(legacyHome, "agents"); +} + export function projectConfigPath(projectDir: string): string { return path.join(projectDir, ".picklab", "config.json"); } @@ -28,6 +60,14 @@ export function globalConfigPath(env: EnvLike = process.env): string { return path.join(picklabHome(env), "config.json"); } +export function legacyGlobalConfigPath(env: EnvLike = process.env): string | undefined { + const legacyHome = legacyPicklabHome(env); + return legacyHome === undefined ? undefined : path.join(legacyHome, "config.json"); +} + +/** The project-local runs layout (`/.picklab/runs`), used by the + * `project-local` storage mode and kept, unwritten, as a non-destructive + * legacy read fallback for every other mode. */ export function runsDir(projectDir: string): string { return path.join(projectDir, ".picklab", "runs"); } @@ -37,6 +77,43 @@ export async function ensureDir(dir: string): Promise { return dir; } +/** + * Resolve which of a primary path and an optional legacy path to read: the + * primary wins whenever it exists; otherwise the legacy path is used verbatim + * so an existing single-file read (global config, agent state) keeps working + * across the `~/.picklab` → `~/.pickforge/picklab` default-root change without + * a migration step. Writes always target the primary path (callers never pass + * this through a writer), so nothing legacy is ever mutated. + */ +export async function resolveReadablePath( + primaryPath: string, + legacyPath: string | undefined, +): Promise { + if (legacyPath === undefined) return primaryPath; + try { + await fs.promises.access(primaryPath, fs.constants.F_OK); + return primaryPath; + } catch { + try { + await fs.promises.access(legacyPath, fs.constants.F_OK); + return legacyPath; + } catch { + return primaryPath; + } + } +} + +/** Directory listing that returns `[]` instead of throwing when missing. */ +export async function listDirSafe(dir: string): Promise { + try { + return await fs.promises.readdir(dir); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT" || code === "ENOTDIR") return []; + throw error; + } +} + let atomicTmpCounter = 0; /** diff --git a/packages/core/src/run-catalog.ts b/packages/core/src/run-catalog.ts index df33714..25f66b7 100644 --- a/packages/core/src/run-catalog.ts +++ b/packages/core/src/run-catalog.ts @@ -1,6 +1,7 @@ import fs from "node:fs"; import path from "node:path"; -import { runsDir } from "./paths.js"; +import { picklabHome, runsDir, type EnvLike } from "./paths.js"; +import { resolveRunStorage } from "./storage.js"; import type { RunManifest } from "./run.js"; const SAFE_ENTRY_PATTERN = /^[A-Za-z0-9._-]+$/; @@ -470,26 +471,90 @@ export class RunCatalog { } } -/** Current storage seam. #34 can replace this one-root resolution with modes. */ -export async function openRunCatalog(projectDir: string): Promise { - let realProject: string; +async function realpathIfExists(dir: string): Promise { try { - realProject = await fs.promises.realpath(projectDir); + return await fs.promises.realpath(dir); } catch (error) { - if (isMissing(error)) return new RunCatalog([]); + if (isMissing(error)) return undefined; throw error; } - return new RunCatalog([ - { - dir: runsDir(projectDir), - expectedRealDir: path.join(realProject, ".picklab", "runs"), - }, - ]); +} + +/** The project-local `.picklab/runs` root, verified against the project + * directory's own real path. Used as the `project-local` mode's root, and as + * a non-destructive legacy discovery root for every other mode so runs + * written before #34 (or by an explicit project-local opt-out) stay + * discoverable without a migration. */ +async function projectLocalRoot( + projectDir: string, +): Promise { + const realProject = await realpathIfExists(projectDir); + if (realProject === undefined) return undefined; + return { + dir: runsDir(projectDir), + expectedRealDir: path.join(realProject, ".picklab", "runs"), + }; +} + +/** + * Open the run catalog for a project: the resolved storage mode's root as the + * highest-precedence source, plus (unless that root already is the + * project-local layout) the legacy project-local root as a read-only + * fallback. A root whose directory does not exist yet is simply absent from + * the catalog, not an error — the same behavior `RunCatalog.list()` already + * has for a missing root. + */ +export async function openRunCatalog( + projectDir: string, + env: EnvLike = process.env, +): Promise { + const resolved = await resolveRunStorage(projectDir, env); + const roots: RunCatalogRoot[] = []; + + if (resolved.mode === "project-local") { + const root = await projectLocalRoot(projectDir); + if (root !== undefined) roots.push(root); + } else if (resolved.mode === "home") { + const homeReal = await realpathIfExists(picklabHome(env)); + if (homeReal !== undefined && resolved.projectId !== undefined) { + roots.push({ + dir: resolved.runsDir, + expectedRealDir: path.join( + homeReal, + "projects", + resolved.projectId, + "runs", + ), + }); + } + } else { + // custom: the configured absolute path is the trusted ancestor to verify + // the runs dir against (there is no project- or home-rooted ancestor to + // lean on for a fully user-specified location). + const customBase = path.dirname(resolved.runsDir); + const baseReal = await realpathIfExists(customBase); + if (baseReal !== undefined) { + roots.push({ + dir: resolved.runsDir, + expectedRealDir: path.join(baseReal, "runs"), + }); + } + } + + if (resolved.mode !== "project-local") { + const legacyRoot = await projectLocalRoot(projectDir); + if (legacyRoot !== undefined) roots.push(legacyRoot); + } + + return new RunCatalog(roots); } /** Compatibility projection for callers that only need manifests. */ -export async function listRuns(projectDir: string): Promise { - return (await (await openRunCatalog(projectDir)).list()).map( +export async function listRuns( + projectDir: string, + env: EnvLike = process.env, +): Promise { + return (await (await openRunCatalog(projectDir, env)).list()).map( (entry) => entry.manifest, ); } diff --git a/packages/core/src/run.ts b/packages/core/src/run.ts index 8388bd7..c76a31e 100644 --- a/packages/core/src/run.ts +++ b/packages/core/src/run.ts @@ -1,6 +1,7 @@ import fs from "node:fs"; import path from "node:path"; -import { ensureDir, runsDir, writeFileAtomic } from "./paths.js"; +import { ensureDir, writeFileAtomic, type EnvLike } from "./paths.js"; +import { resolveRunStorage } from "./storage.js"; export type RunStatus = "running" | "completed" | "failed"; export type ArtifactType = "screenshot" | "log" | "report" | "other"; @@ -126,11 +127,12 @@ export async function createRun( projectDir: string, slug: string, opts: CreateRunOptions = {}, + env: EnvLike = process.env, ): Promise { assertValidSlug(slug); const now = opts.now ?? new Date(); const baseName = `${formatTimestamp(now)}-${slug}`; - const parent = runsDir(projectDir); + const parent = (await resolveRunStorage(projectDir, env)).runsDir; await ensureDir(parent); let runDir: string | undefined; diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 710765f..3c4cf95 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -5,12 +5,15 @@ import { finalizeActiveEvidenceRun } from "./evidence.js"; import { writeEvidenceReport } from "./evidence-render.js"; import { ensureDir, + legacySessionsDir, + listDirSafe, + resolveReadablePath, sessionsDir, - runsDir, writeFileAtomic, type EnvLike, } from "./paths.js"; import { isPidAlive, processIdentityMatches } from "./proc.js"; +import { resolveRunStorage } from "./storage.js"; export type SessionType = "desktop" | "android" | "desktop+android" | "browser"; export type SessionStatus = "starting" | "running" | "stopped" | "error"; @@ -151,7 +154,11 @@ export async function getSession( if (!isValidSessionId(id)) { return undefined; } - const filePath = sessionPath(id, env); + const legacyDir = legacySessionsDir(env); + const filePath = await resolveReadablePath( + sessionPath(id, env), + legacyDir === undefined ? undefined : path.join(legacyDir, `${id}.json`), + ); let raw: string; try { raw = await fs.promises.readFile(filePath, "utf8"); @@ -174,15 +181,10 @@ export async function getSession( export async function listSessions( env: EnvLike = process.env, ): Promise { - let entries: string[]; - try { - entries = await fs.promises.readdir(sessionsDir(env)); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") { - return []; - } - throw error; - } + const legacyDir = legacySessionsDir(env); + const primaryEntries = await listDirSafe(sessionsDir(env)); + const legacyEntries = legacyDir === undefined ? [] : await listDirSafe(legacyDir); + const entries = [...new Set([...primaryEntries, ...legacyEntries])]; const records: SessionRecord[] = []; for (const entry of entries) { @@ -304,13 +306,23 @@ export async function destroySessionRecord( record.projectDir, record.id, evidenceStatus, + env, ).catch(() => undefined); if (finalized !== undefined) { + const parent = (await resolveRunStorage(record.projectDir, env)).runsDir; await writeEvidenceReport( - path.join(runsDir(record.projectDir), finalized.runId), + path.join(parent, finalized.runId), finalized, ).catch(() => {}); } } - await fs.promises.rm(sessionPath(id, env), { force: true }); + // Remove from wherever the record actually lives (new home, or a legacy + // `~/.picklab` record read through the fallback) so an explicitly destroyed + // session never keeps reappearing in listSessions. + const legacyDir = legacySessionsDir(env); + const legacyPath = + legacyDir === undefined ? undefined : path.join(legacyDir, `${id}.json`); + await fs.promises.rm(await resolveReadablePath(sessionPath(id, env), legacyPath), { + force: true, + }); } diff --git a/packages/core/src/storage.ts b/packages/core/src/storage.ts new file mode 100644 index 0000000..ce76d34 --- /dev/null +++ b/packages/core/src/storage.ts @@ -0,0 +1,123 @@ +import crypto from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; +import { loadConfig, type StorageConfig, type StorageMode } from "./config.js"; +import { picklabHome, runsDir, type EnvLike } from "./paths.js"; + +export type { StorageConfig, StorageMode } from "./config.js"; + +const PROJECT_ID_HASH_LENGTH = 16; + +function sanitizeSlug(name: string): string { + const slug = name + .toLowerCase() + .replace(/[^a-z0-9]+/g, "-") + .replace(/^-+|-+$/g, ""); + return (slug === "" ? "project" : slug).slice(0, 40); +} + +/** + * Canonical form of a project path used for stable project-id derivation. + * Resolves symlinks (so a project reached through different paths still gets + * one id); falls back to the lexically resolved path when the directory does + * not exist yet (e.g. a not-yet-created target), so id derivation never + * throws and stays stable once the directory does appear. + */ +export async function canonicalProjectPath(projectDir: string): Promise { + const resolved = path.resolve(projectDir); + try { + return await fs.promises.realpath(resolved); + } catch { + return resolved; + } +} + +/** + * Stable per-project id derived from a canonical project path: a sha256 + * digest (load-bearing for uniqueness) prefixed with a human-readable slug of + * the directory's basename purely for debuggability (`ls + * ~/.pickforge/picklab/projects` stays legible). The same canonical path + * always yields the same id; different paths practically never collide. + */ +export function deriveProjectId(canonicalPath: string): string { + const hash = crypto + .createHash("sha256") + .update(canonicalPath) + .digest("hex") + .slice(0, PROJECT_ID_HASH_LENGTH); + return `${sanitizeSlug(path.basename(canonicalPath))}-${hash}`; +} + +export async function projectId(projectDir: string): Promise { + return deriveProjectId(await canonicalProjectPath(projectDir)); +} + +export class StorageConfigError extends Error {} + +export interface ResolvedRunStorage { + mode: StorageMode; + /** Where new runs are written, e.g. `.../runs//`. */ + runsDir: string; + /** Present only for `mode: "home"`. */ + projectId?: string; +} + +function envStorageMode(env: EnvLike): StorageMode | undefined { + const value = env.PICKLAB_STORAGE_MODE; + if (value === "home" || value === "project-local" || value === "custom") { + return value; + } + return undefined; +} + +/** + * Resolve where new run artifacts should be written for a project. Reads + * `storage` from project/global config (`.picklab/config.json` / + * `/config.json`), then an explicit `PICKLAB_STORAGE_MODE` / + * `PICKLAB_STORAGE_PATH` environment override for automation and tests. Env + * wins over config so a CI job or test harness can force a mode without + * touching project files. + * + * - `home` (default): `/projects//runs`, isolated per + * project and outside every target repository. + * - `project-local`: the pre-#34 `/.picklab/runs` layout. + * - `custom`: `/runs` under an explicit absolute path. + * + * This is the single resolver every run-creation and artifact-lookup path + * (core, CLI, MCP) goes through, so they always agree on where runs live. + */ +export async function resolveRunStorage( + projectDir: string, + env: EnvLike = process.env, +): Promise { + const config = await loadConfig(projectDir, env); + const storage: StorageConfig = config.storage ?? {}; + const mode = envStorageMode(env) ?? storage.mode ?? "home"; + const customPath = env.PICKLAB_STORAGE_PATH ?? storage.path; + + if (mode === "project-local") { + return { mode, runsDir: runsDir(projectDir) }; + } + + if (mode === "custom") { + if (customPath === undefined || customPath === "") { + throw new StorageConfigError( + 'storage mode "custom" requires a storage path ' + + "(storage.path in config, or PICKLAB_STORAGE_PATH)", + ); + } + if (!path.isAbsolute(customPath)) { + throw new StorageConfigError( + `storage path must be an absolute path, got "${customPath}"`, + ); + } + return { mode, runsDir: path.join(path.resolve(customPath), "runs") }; + } + + const id = await projectId(projectDir); + return { + mode: "home", + runsDir: path.join(picklabHome(env), "projects", id, "runs"), + projectId: id, + }; +} diff --git a/packages/core/src/target.ts b/packages/core/src/target.ts index f2f3834..6707f44 100644 --- a/packages/core/src/target.ts +++ b/packages/core/src/target.ts @@ -145,6 +145,7 @@ export interface ResolveScreenshotTargetOptions { defaultSlug: string; sessionId?: string; conflictError: string; + env?: EnvLike; } export async function resolveScreenshotTarget( @@ -202,6 +203,7 @@ export async function resolveScreenshotTarget( opts.projectDir, opts.runSlug ?? opts.defaultSlug, opts.sessionId === undefined ? {} : { sessionId: opts.sessionId }, + opts.env, ); return { outPath: path.join(run.dir, "screenshots", "screenshot.png"), diff --git a/packages/core/test/config.test.ts b/packages/core/test/config.test.ts index 49cd321..ceb1bb1 100644 --- a/packages/core/test/config.test.ts +++ b/packages/core/test/config.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -124,6 +124,86 @@ describe("resolvedDefaults", () => { expect(resolvedDefaults.labUser.home).toBe("/var/lib/picklab/lab-home"); expect(resolvedDefaults.viewer.mode).toBe("manual"); expect(resolvedDefaults.evidence.enabled).toBe(true); + expect(resolvedDefaults.storage.mode).toBe("home"); + }); +}); + +describe("loadConfig legacy home fallback", () => { + it("reads an existing ~/.picklab global config when PICKLAB_HOME is unset and the new default has nothing yet", async () => { + const fakeHome = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-fakehome-"), + ); + const homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + try { + const legacyConfigDir = path.join(fakeHome, ".picklab"); + await fs.promises.mkdir(legacyConfigDir, { recursive: true }); + await fs.promises.writeFile( + path.join(legacyConfigDir, "config.json"), + JSON.stringify({ profile: "android" }), + ); + + const config = await loadConfig(project, {}); + expect(config.profile).toBe("android"); + // Non-destructive: nothing was written to the new default location. + expect( + fs.existsSync(path.join(fakeHome, ".pickforge", "picklab")), + ).toBe(false); + } finally { + homedirSpy.mockRestore(); + await fs.promises.rm(fakeHome, { recursive: true, force: true }); + } + }); + + it("prefers the new default over the legacy home once the new one has a config", async () => { + const fakeHome = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-fakehome-"), + ); + const homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + try { + await fs.promises.mkdir(path.join(fakeHome, ".picklab"), { + recursive: true, + }); + await fs.promises.writeFile( + path.join(fakeHome, ".picklab", "config.json"), + JSON.stringify({ profile: "android" }), + ); + await fs.promises.mkdir( + path.join(fakeHome, ".pickforge", "picklab"), + { recursive: true }, + ); + await fs.promises.writeFile( + path.join(fakeHome, ".pickforge", "picklab", "config.json"), + JSON.stringify({ profile: "generic" }), + ); + + const config = await loadConfig(project, {}); + expect(config.profile).toBe("generic"); + } finally { + homedirSpy.mockRestore(); + await fs.promises.rm(fakeHome, { recursive: true, force: true }); + } + }); + + it("does not fall back to ~/.picklab once PICKLAB_HOME is set explicitly", async () => { + const fakeHome = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-fakehome-"), + ); + const homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + try { + await fs.promises.mkdir(path.join(fakeHome, ".picklab"), { + recursive: true, + }); + await fs.promises.writeFile( + path.join(fakeHome, ".picklab", "config.json"), + JSON.stringify({ profile: "android" }), + ); + + const config = await loadConfig(project, env); + expect(config.profile).toBeUndefined(); + } finally { + homedirSpy.mockRestore(); + await fs.promises.rm(fakeHome, { recursive: true, force: true }); + } }); }); diff --git a/packages/core/test/evidence-render.test.ts b/packages/core/test/evidence-render.test.ts index 36d5b1c..3987d37 100644 --- a/packages/core/test/evidence-render.test.ts +++ b/packages/core/test/evidence-render.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { EVIDENCE_REPORT, appendAction, @@ -24,9 +24,13 @@ beforeEach(() => { root = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-evidence-render-")); projectDir = path.join(root, "project"); fs.mkdirSync(projectDir, { recursive: true }); + // Isolate createRun's default storage resolution from the real developer + // home; the exact mode does not matter to these render-only assertions. + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(() => { + vi.unstubAllEnvs(); fs.rmSync(root, { recursive: true, force: true }); }); diff --git a/packages/core/test/evidence.concurrency.test.ts b/packages/core/test/evidence.concurrency.test.ts index 90ddfe1..8f457c2 100644 --- a/packages/core/test/evidence.concurrency.test.ts +++ b/packages/core/test/evidence.concurrency.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { spawn } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; @@ -52,11 +52,16 @@ function run(args: string[]): Promise { let project: string; +// Pin storage to `project-local` (the layout these concurrency tests assert +// against and spawn separate `bun` worker processes into via inherited +// `process.env`) rather than the new `home` default. beforeEach(async () => { project = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-evc-")); + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(async () => { + vi.unstubAllEnvs(); await fs.promises.rm(project, { recursive: true, force: true }); }); diff --git a/packages/core/test/evidence.test.ts b/packages/core/test/evidence.test.ts index f4c167c..824a14f 100644 --- a/packages/core/test/evidence.test.ts +++ b/packages/core/test/evidence.test.ts @@ -22,11 +22,18 @@ import { createRun, listRuns, RunHandle } from "../src/run.js"; let project: string; +// This file exercises the concurrency/security invariants of the run +// catalog and claim protocol directly against `.picklab/runs`, so it pins +// storage to `project-local` (the pre-#34 layout it was written against) +// rather than the new `home` default. `home`-mode default behavior itself is +// covered by storage.test.ts and run-catalog.test.ts. beforeEach(async () => { project = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-evi-")); + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(async () => { + vi.unstubAllEnvs(); await fs.promises.rm(project, { recursive: true, force: true }); }); @@ -110,8 +117,8 @@ describe("beginEvidenceRun and the active pointer", () => { const a = await beginEvidenceRun(project, "desk-aaaaaa"); const b = await beginEvidenceRun(project, "andr-bbbbbb"); expect(a.run.runId).not.toBe(b.run.runId); - expect(fs.existsSync(activePointerPath(project, "desk-aaaaaa"))).toBe(true); - expect(fs.existsSync(activePointerPath(project, "andr-bbbbbb"))).toBe(true); + expect(fs.existsSync(await activePointerPath(project, "desk-aaaaaa"))).toBe(true); + expect(fs.existsSync(await activePointerPath(project, "andr-bbbbbb"))).toBe(true); }); it("rejects unsafe session ids", async () => { @@ -223,7 +230,7 @@ describe("pointer resolution and clearing", () => { it("treats an empty pointer as a peer mid-claim", async () => { await fs.promises.mkdir(runsRoot(), { recursive: true }); - await fs.promises.writeFile(activePointerPath(project, "desk-claim0"), ""); + await fs.promises.writeFile(await activePointerPath(project, "desk-claim0"), ""); expect((await resolveActivePointer(project, "desk-claim0")).status).toBe( "claiming", ); @@ -232,7 +239,7 @@ describe("pointer resolution and clearing", () => { it("reports corrupt for unparseable pointer content", async () => { await fs.promises.mkdir(runsRoot(), { recursive: true }); await fs.promises.writeFile( - activePointerPath(project, "desk-corr00"), + await activePointerPath(project, "desk-corr00"), "{ not json", ); expect((await resolveActivePointer(project, "desk-corr00")).status).toBe( @@ -250,11 +257,11 @@ describe("pointer resolution and clearing", () => { it("clears only stale/corrupt pointers by default, not active ones", async () => { const { run } = await beginEvidenceRun(project, "desk-keep00"); expect(await clearActivePointer(project, "desk-keep00")).toBe(false); - expect(fs.existsSync(activePointerPath(project, "desk-keep00"))).toBe(true); + expect(fs.existsSync(await activePointerPath(project, "desk-keep00"))).toBe(true); await run.finish("failed"); expect(await clearActivePointer(project, "desk-keep00")).toBe(true); - expect(fs.existsSync(activePointerPath(project, "desk-keep00"))).toBe(false); + expect(fs.existsSync(await activePointerPath(project, "desk-keep00"))).toBe(false); }); it("force-clears an active pointer when asked", async () => { @@ -262,12 +269,12 @@ describe("pointer resolution and clearing", () => { expect( await clearActivePointer(project, "desk-force0", { force: true }), ).toBe(true); - expect(fs.existsSync(activePointerPath(project, "desk-force0"))).toBe(false); + expect(fs.existsSync(await activePointerPath(project, "desk-force0"))).toBe(false); }); it("compare-and-clears with expectRaw", async () => { await beginEvidenceRun(project, "desk-cas000"); - const pointerPath = activePointerPath(project, "desk-cas000"); + const pointerPath = await activePointerPath(project, "desk-cas000"); const raw = await fs.promises.readFile(pointerPath, "utf8"); expect( await clearActivePointer(project, "desk-cas000", { expectRaw: "other" }), @@ -308,7 +315,7 @@ describe("pointer resolution and clearing", () => { }), ], ] as const) { - await fs.promises.writeFile(activePointerPath(project, session), content); + await fs.promises.writeFile(await activePointerPath(project, session), content); expect((await resolveActivePointer(project, session)).status).toBe( "corrupt", ); @@ -324,7 +331,7 @@ describe("pointer resolution and clearing", () => { ownerPid: 4_194_304, createdAt: new Date().toISOString(), }); - await fs.promises.writeFile(activePointerPath(project, "desk-gone00"), raw); + await fs.promises.writeFile(await activePointerPath(project, "desk-gone00"), raw); expect((await resolveActivePointer(project, "desk-gone00")).status).toBe( "stale", ); @@ -362,7 +369,7 @@ describe("pointer resolution and clearing", () => { await expect( finalizeActiveEvidenceRun(project, sessionId), ).resolves.toBeUndefined(); - expect(fs.existsSync(activePointerPath(project, sessionId))).toBe(false); + expect(fs.existsSync(await activePointerPath(project, sessionId))).toBe(false); expect(fs.existsSync(path.join(project, "outside", "report.html"))).toBe( false, ); @@ -371,7 +378,7 @@ describe("pointer resolution and clearing", () => { it("recovers a corrupt pointer by starting a fresh run", async () => { await fs.promises.mkdir(runsRoot(), { recursive: true }); await fs.promises.writeFile( - activePointerPath(project, "desk-corr01"), + await activePointerPath(project, "desk-corr01"), "{ garbage", ); const { run, adopted } = await beginEvidenceRun(project, "desk-corr01"); @@ -396,7 +403,7 @@ describe("pointer resolution and clearing", () => { ownerStartTicks: 1, // and no /proc match, so identity is provably dead createdAt: new Date().toISOString(), })}\n`; - await fs.promises.writeFile(activePointerPath(project, "desk-dead00"), raw); + await fs.promises.writeFile(await activePointerPath(project, "desk-dead00"), raw); const resolution = await resolveActivePointer(project, "desk-dead00"); expect(resolution.status).toBe("stale"); if (resolution.status !== "stale") throw new Error("expected stale"); @@ -416,7 +423,7 @@ describe("pointer resolution and clearing", () => { const first = await beginEvidenceRun(project, "desk-dead01"); // Simulate the creating process dying: rewrite the pointer's owner identity // to a dead/reused one while the manifest is still `running`. - const pointerPath = activePointerPath(project, "desk-dead01"); + const pointerPath = await activePointerPath(project, "desk-dead01"); const pointer = JSON.parse(await fs.promises.readFile(pointerPath, "utf8")); pointer.ownerPid = 4_194_304; pointer.ownerStartTicks = 1; @@ -1037,7 +1044,7 @@ describe("truncation marker durability", () => { describe("begin claim recovery", () => { it("reclaims an owner-unknown (empty) claim and starts a run", async () => { await fs.promises.mkdir(runsRoot(), { recursive: true }); - await fs.promises.writeFile(activePointerPath(project, "desk-empt00"), ""); + await fs.promises.writeFile(await activePointerPath(project, "desk-empt00"), ""); const { run, adopted } = await beginEvidenceRun(project, "desk-empt00"); expect(adopted).toBe(false); const resolution = await resolveActivePointer(project, "desk-empt00"); @@ -1056,7 +1063,7 @@ describe("begin claim recovery", () => { claim: true, claimedAt: new Date().toISOString(), })}\n`; - await fs.promises.writeFile(activePointerPath(project, "desk-dclm00"), claim); + await fs.promises.writeFile(await activePointerPath(project, "desk-dclm00"), claim); // A claim record with owner identity resolves as `claiming`. const pre = await resolveActivePointer(project, "desk-dclm00"); expect(pre.status).toBe("claiming"); @@ -1407,7 +1414,7 @@ describe("defensive filesystem errors propagate", () => { } // No active pointer is left behind. - expect(fs.existsSync(activePointerPath(project, "desk-pubfail"))).toBe(false); + expect(fs.existsSync(await activePointerPath(project, "desk-pubfail"))).toBe(false); // The just-created run is finalized (failed), never a permanent running // orphan. Exactly one run exists and none are still running. const runs = await listRuns(project); diff --git a/packages/core/test/paths.test.ts b/packages/core/test/paths.test.ts index c918d96..0ef62ae 100644 --- a/packages/core/test/paths.test.ts +++ b/packages/core/test/paths.test.ts @@ -5,7 +5,13 @@ import path from "node:path"; import { agentsDir, isProfileConfined, + legacyAgentsDir, + legacyGlobalConfigPath, + legacyPicklabHome, + legacySessionsDir, + listDirSafe, picklabHome, + resolveReadablePath, sessionsDir, } from "../src/paths.js"; @@ -14,14 +20,103 @@ describe("picklabHome", () => { expect(picklabHome({ PICKLAB_HOME: "/custom/home" })).toBe("/custom/home"); }); - it("falls back to ~/.picklab when PICKLAB_HOME is empty", () => { + it("falls back to ~/.pickforge/picklab when PICKLAB_HOME is empty", () => { expect(picklabHome({ PICKLAB_HOME: "" })).toBe( + path.join(os.homedir(), ".pickforge", "picklab"), + ); + }); + + it("falls back to ~/.pickforge/picklab when PICKLAB_HOME is unset", () => { + expect(picklabHome({})).toBe(path.join(os.homedir(), ".pickforge", "picklab")); + }); +}); + +describe("legacyPicklabHome", () => { + it("returns ~/.picklab when PICKLAB_HOME is unset", () => { + expect(legacyPicklabHome({})).toBe(path.join(os.homedir(), ".picklab")); + }); + + it("returns ~/.picklab when PICKLAB_HOME is empty", () => { + expect(legacyPicklabHome({ PICKLAB_HOME: "" })).toBe( path.join(os.homedir(), ".picklab"), ); }); - it("falls back to ~/.picklab when PICKLAB_HOME is unset", () => { - expect(picklabHome({})).toBe(path.join(os.homedir(), ".picklab")); + it("is undefined once PICKLAB_HOME is set explicitly (the user's own root)", () => { + expect(legacyPicklabHome({ PICKLAB_HOME: "/custom/home" })).toBeUndefined(); + }); +}); + +describe("legacy subdir helpers", () => { + it("derive from legacyPicklabHome, undefined once PICKLAB_HOME is explicit", () => { + expect(legacySessionsDir({})).toBe( + path.join(os.homedir(), ".picklab", "sessions"), + ); + expect(legacyAgentsDir({})).toBe( + path.join(os.homedir(), ".picklab", "agents"), + ); + expect(legacyGlobalConfigPath({})).toBe( + path.join(os.homedir(), ".picklab", "config.json"), + ); + expect(legacySessionsDir({ PICKLAB_HOME: "/lab" })).toBeUndefined(); + expect(legacyAgentsDir({ PICKLAB_HOME: "/lab" })).toBeUndefined(); + expect(legacyGlobalConfigPath({ PICKLAB_HOME: "/lab" })).toBeUndefined(); + }); +}); + +describe("resolveReadablePath", () => { + it("returns the primary path verbatim when there is no legacy path", async () => { + expect(await resolveReadablePath("/a/primary.json", undefined)).toBe( + "/a/primary.json", + ); + }); + + it("prefers the primary path when it exists", async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-readable-")); + const primary = path.join(root, "primary.json"); + const legacy = path.join(root, "legacy.json"); + fs.writeFileSync(primary, "{}"); + fs.writeFileSync(legacy, "{}"); + try { + expect(await resolveReadablePath(primary, legacy)).toBe(primary); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it("falls back to the legacy path when the primary is missing", async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-readable-")); + const primary = path.join(root, "primary.json"); + const legacy = path.join(root, "legacy.json"); + fs.writeFileSync(legacy, "{}"); + try { + expect(await resolveReadablePath(primary, legacy)).toBe(legacy); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it("returns the primary path when neither exists", async () => { + expect( + await resolveReadablePath("/nope/primary.json", "/nope/legacy.json"), + ).toBe("/nope/primary.json"); + }); +}); + +describe("listDirSafe", () => { + it("returns [] for a missing directory instead of throwing", async () => { + expect(await listDirSafe("/definitely/does/not/exist")).toEqual([]); + }); + + it("lists real entries", async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "picklab-listdir-")); + fs.writeFileSync(path.join(root, "a.json"), "{}"); + fs.writeFileSync(path.join(root, "b.json"), "{}"); + try { + expect((await listDirSafe(root)).sort()).toEqual(["a.json", "b.json"]); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } }); }); diff --git a/packages/core/test/run-catalog.test.ts b/packages/core/test/run-catalog.test.ts index ca8fa7d..ec18519 100644 --- a/packages/core/test/run-catalog.test.ts +++ b/packages/core/test/run-catalog.test.ts @@ -11,14 +11,21 @@ import { let root: string; +// These tests build `RunCatalog` roots directly, or exercise `createRun` + +// `openRunCatalog` against the literal `.picklab/runs` layout, so they pin +// storage to `project-local`. Home-mode default resolution and the +// home-primary + legacy-project-local layering are covered below in +// "openRunCatalog storage modes". beforeEach(async () => { root = await fs.promises.mkdtemp( path.join(os.tmpdir(), "picklab-run-catalog-"), ); + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(async () => { vi.restoreAllMocks(); + vi.unstubAllEnvs(); await fs.promises.rm(root, { recursive: true, force: true }); }); @@ -214,3 +221,106 @@ describe("RunCatalog", () => { expect(await catalog.list()).toEqual([]); }); }); + +describe("openRunCatalog storage modes", () => { + // These tests set env explicitly per call (not via the file-level + // PICKLAB_STORAGE_MODE stub) so they exercise the real "home" default. + it("defaults new runs under the home root, isolated per project, with no files in the project dir", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { PICKLAB_HOME: home }; + + const run = await createRun(project, "smoke", {}, env); + + expect(run.dir.startsWith(path.join(home, "projects"))).toBe(true); + expect(fs.existsSync(path.join(project, ".picklab"))).toBe(false); + + const entries = await (await openRunCatalog(project, env)).list(); + expect(entries.map((entry) => entry.manifest.runId)).toEqual([run.runId]); + }); + + it("keeps pre-existing project-local runs discoverable without migration", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { PICKLAB_HOME: home }; + const legacyRoot = path.join(project, ".picklab", "runs"); + await writeRun(legacyRoot, "legacy-run"); + + const newRun = await createRun(project, "fresh", {}, env); + + const entries = await (await openRunCatalog(project, env)).list(); + const ids = entries.map((entry) => entry.manifest.runId).sort(); + expect(ids).toEqual(["legacy-run", newRun.runId].sort()); + // Legacy data was only read, never written or moved. + expect(fs.existsSync(path.join(legacyRoot, "legacy-run"))).toBe(true); + }); + + it("keeps two different project paths fully isolated under the same home", async () => { + const home = path.join(root, "home"); + const projectA = path.join(root, "project-a"); + const projectB = path.join(root, "project-b"); + await fs.promises.mkdir(projectA, { recursive: true }); + await fs.promises.mkdir(projectB, { recursive: true }); + const env = { PICKLAB_HOME: home }; + + const runA = await createRun(projectA, "a-run", {}, env); + const runB = await createRun(projectB, "b-run", {}, env); + + expect(runA.dir).not.toBe(runB.dir); + const entriesA = await (await openRunCatalog(projectA, env)).list(); + const entriesB = await (await openRunCatalog(projectB, env)).list(); + expect(entriesA.map((e) => e.manifest.runId)).toEqual([runA.runId]); + expect(entriesB.map((e) => e.manifest.runId)).toEqual([runB.runId]); + }); + + it("restores the project-local layout when explicitly configured", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { PICKLAB_HOME: home, PICKLAB_STORAGE_MODE: "project-local" }; + + const run = await createRun(project, "local", {}, env); + + expect(run.dir).toBe( + path.join(project, ".picklab", "runs", run.runId), + ); + expect(fs.existsSync(home)).toBe(false); + }); + + it("rejects custom mode without a path", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await expect( + createRun(project, "x", {}, { PICKLAB_STORAGE_MODE: "custom" }), + ).rejects.toThrow(/storage path/i); + }); + + it("rejects custom mode with a relative path", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await expect( + createRun(project, "x", {}, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: "relative/artifacts", + }), + ).rejects.toThrow(/absolute/i); + }); + + it("writes under an explicit custom path", async () => { + const project = path.join(root, "project"); + const custom = path.join(root, "custom-artifacts"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: custom, + }; + + const run = await createRun(project, "x", {}, env); + + expect(run.dir).toBe(path.join(custom, "runs", run.runId)); + const entries = await (await openRunCatalog(project, env)).list(); + expect(entries.map((e) => e.manifest.runId)).toEqual([run.runId]); + }); +}); diff --git a/packages/core/test/run.test.ts b/packages/core/test/run.test.ts index 7023e5f..3d0d698 100644 --- a/packages/core/test/run.test.ts +++ b/packages/core/test/run.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -6,11 +6,16 @@ import { createRun, listRuns } from "../src/run.js"; let project: string; +// These tests assert against the literal `.picklab/runs` layout, so they pin +// storage to `project-local` explicitly rather than the new `home` default +// (covered by storage.test.ts). beforeEach(async () => { project = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-run-")); + vi.stubEnv("PICKLAB_STORAGE_MODE", "project-local"); }); afterEach(async () => { + vi.unstubAllEnvs(); await fs.promises.rm(project, { recursive: true, force: true }); }); diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 8fd2fc0..f836742 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -280,7 +280,7 @@ describe("session registry", () => { { type: "desktop", projectDir, status: "running" }, env, ); - const { run } = await beginEvidenceRun(projectDir, session.id); + const { run } = await beginEvidenceRun(projectDir, session.id, {}, env); await destroySessionRecord(session.id, env); @@ -295,7 +295,9 @@ describe("session registry", () => { ).toMatchObject({ status: "completed" }); expect(await fs.promises.readFile(path.join(run.dir, "report.html"), "utf8")) .toContain("completed"); - expect(fs.existsSync(activePointerPath(projectDir, session.id))).toBe(false); + expect(fs.existsSync(await activePointerPath(projectDir, session.id, env))).toBe( + false, + ); }); it("finalizes active evidence when reaping a dead session", async () => { @@ -310,7 +312,7 @@ describe("session registry", () => { }, env, ); - const { run } = await beginEvidenceRun(projectDir, stale.id); + const { run } = await beginEvidenceRun(projectDir, stale.id, {}, env); await appendAction(run.dir, { actionId: "before-reap", source: "mcp", @@ -335,7 +337,9 @@ describe("session registry", () => { expect(await readActions(run.dir)).toHaveLength(1); expect(await fs.promises.readFile(path.join(run.dir, "report.html"), "utf8")) .toContain("desktop_click"); - expect(fs.existsSync(activePointerPath(projectDir, stale.id))).toBe(false); + expect(fs.existsSync(await activePointerPath(projectDir, stale.id, env))).toBe( + false, + ); }); it("stops recorded helper pids before deleting dead running records", async () => { @@ -1131,3 +1135,71 @@ describe("session registry", () => { } }); }); + +describe("legacy session home fallback", () => { + let fakeHome: string; + let homedirSpy: ReturnType; + + beforeEach(async () => { + fakeHome = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-legacy-fakehome-"), + ); + homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fakeHome); + }); + + afterEach(async () => { + homedirSpy.mockRestore(); + await fs.promises.rm(fakeHome, { recursive: true, force: true }); + }); + + function writeLegacySession(id: string): void { + const dir = path.join(fakeHome, ".picklab", "sessions"); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync( + path.join(dir, `${id}.json`), + JSON.stringify({ + id, + type: "desktop", + createdAt: "2026-01-01T00:00:00.000Z", + status: "stopped", + projectDir: "/legacy/project", + }), + ); + } + + it("reads a legacy ~/.picklab session when PICKLAB_HOME is unset", async () => { + writeLegacySession("desk-1eaac1"); + + const record = await getSession("desk-1eaac1", {}); + expect(record?.projectDir).toBe("/legacy/project"); + }); + + it("lists legacy sessions alongside new-home sessions", async () => { + writeLegacySession("desk-1eaac2"); + const created = await createSession( + { type: "desktop", projectDir: "/new/project" }, + {}, + ); + + const ids = (await listSessions({})).map((record) => record.id).sort(); + expect(ids).toEqual(["desk-1eaac2", created.id].sort()); + }); + + it("does not fall back once PICKLAB_HOME is set explicitly", async () => { + writeLegacySession("desk-1eaac3"); + + expect(await getSession("desk-1eaac3", { PICKLAB_HOME: "/other" })).toBeUndefined(); + }); + + it("destroying a session found via the legacy fallback removes it there, not just the new home", async () => { + writeLegacySession("desk-1eaac4"); + + await destroySessionRecord("desk-1eaac4", {}); + + expect( + fs.existsSync( + path.join(fakeHome, ".picklab", "sessions", "desk-1eaac4.json"), + ), + ).toBe(false); + }); +}); diff --git a/packages/core/test/storage.test.ts b/packages/core/test/storage.test.ts new file mode 100644 index 0000000..41714a1 --- /dev/null +++ b/packages/core/test/storage.test.ts @@ -0,0 +1,206 @@ +import { execFile } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { promisify } from "node:util"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + canonicalProjectPath, + deriveProjectId, + projectId, + resolveRunStorage, + StorageConfigError, +} from "../src/storage.js"; +import { saveProjectConfig } from "../src/config.js"; +import { createRun } from "../src/run.js"; + +const execFileAsync = promisify(execFile); + +let root: string; + +beforeEach(async () => { + root = await fs.promises.mkdtemp(path.join(os.tmpdir(), "picklab-storage-")); +}); + +afterEach(async () => { + await fs.promises.rm(root, { recursive: true, force: true }); +}); + +describe("deriveProjectId", () => { + it("is stable for the same canonical path", () => { + const id = deriveProjectId("/home/u/projects/widget"); + expect(deriveProjectId("/home/u/projects/widget")).toBe(id); + }); + + it("differs for different canonical paths", () => { + expect(deriveProjectId("/home/u/projects/widget")).not.toBe( + deriveProjectId("/home/u/projects/gadget"), + ); + }); + + it("includes a debuggable slug derived from the basename", () => { + const id = deriveProjectId("/home/u/projects/My Cool App!"); + expect(id.startsWith("my-cool-app-")).toBe(true); + }); + + it("falls back to a generic slug for an unrepresentable basename", () => { + const id = deriveProjectId("/home/u/projects/___"); + expect(id.startsWith("project-")).toBe(true); + }); +}); + +describe("projectId / canonicalProjectPath", () => { + it("resolves the same id for the same directory reached through a symlink", async () => { + const real = path.join(root, "real-project"); + const link = path.join(root, "linked-project"); + await fs.promises.mkdir(real); + await fs.promises.symlink(real, link); + + expect(await projectId(link)).toBe(await projectId(real)); + }); + + it("resolves different ids for different projects", async () => { + const a = path.join(root, "a"); + const b = path.join(root, "b"); + await fs.promises.mkdir(a); + await fs.promises.mkdir(b); + + expect(await projectId(a)).not.toBe(await projectId(b)); + }); + + it("does not throw for a project directory that does not exist yet", async () => { + const missing = path.join(root, "not-created-yet"); + await expect(canonicalProjectPath(missing)).resolves.toBeTruthy(); + await expect(projectId(missing)).resolves.toBeTruthy(); + }); +}); + +describe("resolveRunStorage", () => { + it("defaults to home mode under the resolved PickLab home", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { PICKLAB_HOME: home }; + + const resolved = await resolveRunStorage(project, env); + + expect(resolved.mode).toBe("home"); + expect(resolved.projectId).toBeDefined(); + expect(resolved.runsDir).toBe( + path.join(home, "projects", resolved.projectId!, "runs"), + ); + }); + + it("resolves project-local mode from PICKLAB_STORAGE_MODE", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "project-local", + }); + + expect(resolved.mode).toBe("project-local"); + expect(resolved.runsDir).toBe(path.join(project, ".picklab", "runs")); + }); + + it("resolves project-local mode from project config", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { storage: { mode: "project-local" } }); + + const resolved = await resolveRunStorage(project, {}); + + expect(resolved.mode).toBe("project-local"); + expect(resolved.runsDir).toBe(path.join(project, ".picklab", "runs")); + }); + + it("lets an env override win over project config", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { storage: { mode: "project-local" } }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_HOME: home, + PICKLAB_STORAGE_MODE: "home", + }); + + expect(resolved.mode).toBe("home"); + }); + + it("resolves custom mode from config with an absolute path", async () => { + const project = path.join(root, "project"); + const custom = path.join(root, "custom-artifacts"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { + storage: { mode: "custom", path: custom }, + }); + + const resolved = await resolveRunStorage(project, {}); + + expect(resolved.mode).toBe("custom"); + expect(resolved.runsDir).toBe(path.join(custom, "runs")); + }); + + it("rejects custom mode with no path", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + + await expect( + resolveRunStorage(project, { PICKLAB_STORAGE_MODE: "custom" }), + ).rejects.toThrow(StorageConfigError); + }); + + it("rejects custom mode with a relative path", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + + await expect( + resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: "relative/path", + }), + ).rejects.toThrow(/absolute/i); + }); +}); + +describe("repo cleanliness smoke", () => { + it("leaves a target git repo's working tree clean after a default run", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "target-repo"); + await fs.promises.mkdir(project, { recursive: true }); + const env = { ...process.env, PICKLAB_HOME: home }; + + await execFileAsync("git", ["init", "-q"], { cwd: project }); + await execFileAsync("git", ["config", "user.email", "test@example.com"], { + cwd: project, + }); + await execFileAsync("git", ["config", "user.name", "Test"], { + cwd: project, + }); + await fs.promises.writeFile(path.join(project, "README.md"), "# repo\n"); + await execFileAsync("git", ["add", "-A"], { cwd: project }); + await execFileAsync("git", ["commit", "-q", "-m", "init"], { + cwd: project, + }); + + const run = await createRun(project, "smoke", {}, env); + await fs.promises.writeFile( + path.join(run.dir, "screenshots", "screenshot.png"), + "fake-png", + ); + await run.addArtifact( + "screenshot", + "screenshot.png", + path.join(run.dir, "screenshots", "screenshot.png"), + ); + await run.finish("completed"); + + const status = await execFileAsync("git", ["status", "--porcelain"], { + cwd: project, + }); + expect(status.stdout.trim()).toBe(""); + expect(fs.existsSync(path.join(project, ".picklab"))).toBe(false); + expect(run.dir.startsWith(home)).toBe(true); + }); +}); diff --git a/packages/mcp-server/src/context.ts b/packages/mcp-server/src/context.ts index 5b77094..5ce452b 100644 --- a/packages/mcp-server/src/context.ts +++ b/packages/mcp-server/src/context.ts @@ -144,5 +144,6 @@ export async function resolveScreenshotTarget( defaultSlug, sessionId, conflictError: 'Use either "out" or "runSlug", not both', + env: ctx.env, }); } diff --git a/packages/mcp-server/src/evidence.ts b/packages/mcp-server/src/evidence.ts index 06346f7..ef30599 100644 --- a/packages/mcp-server/src/evidence.ts +++ b/packages/mcp-server/src/evidence.ts @@ -53,8 +53,14 @@ async function evidenceRun( ): Promise { const config = await loadConfig(ctx.projectDir, ctx.env); if (!isEvidenceEnabled(config)) return undefined; - return (await beginEvidenceRun(ctx.projectDir, sessionId, { slug: "computer-use" })) - .run; + return ( + await beginEvidenceRun( + ctx.projectDir, + sessionId, + { slug: "computer-use" }, + ctx.env, + ) + ).run; } async function refreshFinalizedReport(run: RunHandle): Promise { diff --git a/packages/mcp-server/src/resources.ts b/packages/mcp-server/src/resources.ts index a75cf05..5dbaf02 100644 --- a/packages/mcp-server/src/resources.ts +++ b/packages/mcp-server/src/resources.ts @@ -160,7 +160,7 @@ async function listEvidenceRunFiles( ctx: ServerContext, fileName: typeof EVIDENCE_ACTION_LOG | typeof EVIDENCE_REPORT, ): Promise { - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const runIds: string[] = []; for (const entry of await catalog.list()) { if (!isEvidenceRun(entry.manifest)) continue; @@ -175,7 +175,7 @@ async function listRunFiles( subdir: "screenshots" | "logs", ): Promise> { const entries: Array<{ runId: string; name: string }> = []; - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); for (const entry of await catalog.list()) { const runDir = entry.dir; const dir = path.join(runDir, subdir); @@ -214,11 +214,11 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { "picklab://runs", { title: "PickLab runs", - description: "Index of recorded runs under .picklab/runs", + description: "Index of recorded runs (see picklab doctor for the resolved storage path)", mimeType: "application/json", }, async (uri) => { - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const runs = (await catalog.list()).map(({ manifest }) => ({ runId: manifest.runId, slug: manifest.slug, @@ -243,7 +243,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { new ResourceTemplate("picklab://runs/{runId}/manifest", { list: async () => ({ resources: ( - await (await openRunCatalog(ctx.projectDir)).list() + await (await openRunCatalog(ctx.projectDir, ctx.env)).list() ).map(({ manifest }) => ({ uri: `picklab://runs/${manifest.runId}/manifest`, name: `Run ${manifest.runId} manifest`, @@ -258,7 +258,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entry = await catalog.find(runId); if (entry === undefined) throw new Error(`Run not found: ${runId}`); let raw: string; @@ -295,7 +295,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entry = await catalog.find(runId); if (entry === undefined || !isEvidenceRun(entry.manifest)) { throw new Error(`Actions not found: ${runId}`); @@ -348,7 +348,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { }, async (uri, variables) => { const runId = decodeVariable(variables, "runId"); - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entry = await catalog.find(runId); if (entry === undefined || !isEvidenceRun(entry.manifest)) { throw new Error(`Report not found: ${runId}`); @@ -393,7 +393,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { if (!name.endsWith(".png")) { throw new Error(`Not a PNG screenshot: ${name}`); } - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entry = await catalog.find(runId); if (entry === undefined) { throw new Error(`Screenshot not found: ${runId}/${name}`); @@ -463,7 +463,7 @@ export function registerResources(server: McpServer, ctx: ServerContext): void { async (uri, variables) => { const runId = decodeVariable(variables, "runId"); const name = decodeVariable(variables, "name"); - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entry = await catalog.find(runId); if (entry === undefined) { throw 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 437cc9b..7f32be0 100644 --- a/packages/mcp-server/src/tools/artifacts.ts +++ b/packages/mcp-server/src/tools/artifacts.ts @@ -6,7 +6,8 @@ import { openRunCatalog, parseActionsJournal, renderRunReport, - runsDir, + resolveRunStorage, + type EnvLike, type RunCatalog, type RunCatalogEntry, } from "@pickforge/picklab-core"; @@ -15,12 +16,14 @@ import { runTool, type ServerContext } from "../context.js"; export async function findRun( projectDir: string, runId: string | undefined, + env: EnvLike = process.env, ): Promise<{ catalog: RunCatalog; entry: RunCatalogEntry }> { - const catalog = await openRunCatalog(projectDir); + const catalog = await openRunCatalog(projectDir, env); const entry = await catalog.find(runId); if (entry === undefined) { if (runId === undefined) { - throw new Error(`No runs found under ${runsDir(projectDir)}`); + const { runsDir } = await resolveRunStorage(projectDir, env); + throw new Error(`No runs found under ${runsDir}`); } throw new Error(`Run not found: ${runId} (see the artifact_list tool)`); } @@ -48,13 +51,14 @@ export function registerArtifactTools( { title: "List runs", description: - "List recorded runs (screenshots, logs, reports) under " + - ".picklab/runs in the project directory.", + "List recorded runs (screenshots, logs, reports). Runs default to " + + "the shared Pickforge home outside the project directory; see " + + "picklab doctor or the storage config docs for the resolved path.", inputSchema: {}, }, () => runTool(async () => { - const catalog = await openRunCatalog(ctx.projectDir); + const catalog = await openRunCatalog(ctx.projectDir, ctx.env); const entries = await catalog.list(); const runs = entries.map(({ manifest }) => ({ runId: manifest.runId, @@ -80,7 +84,11 @@ export function registerArtifactTools( }, (args) => runTool(async () => { - const { catalog, entry } = await findRun(ctx.projectDir, args.runId); + const { catalog, entry } = await findRun( + ctx.projectDir, + args.runId, + ctx.env, + ); const { manifest, dir } = entry; const records = await readCatalogActions(catalog, entry); return { diff --git a/packages/mcp-server/test/android.test.ts b/packages/mcp-server/test/android.test.ts index f3ac336..c854b80 100644 --- a/packages/mcp-server/test/android.test.ts +++ b/packages/mcp-server/test/android.test.ts @@ -399,7 +399,7 @@ describe("android_start (fake sdk)", () => { ), ).toContain("session_destroy"); expect( - fs.existsSync(activePointerPath(startDirs.projectDir, session.id)), + fs.existsSync(await activePointerPath(startDirs.projectDir, session.id)), ).toBe(false); } finally { killFakeEmulator(pidFile); diff --git a/packages/mcp-server/test/evidence.test.ts b/packages/mcp-server/test/evidence.test.ts index 1084e1a..6e987b8 100644 --- a/packages/mcp-server/test/evidence.test.ts +++ b/packages/mcp-server/test/evidence.test.ts @@ -4,7 +4,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { listRuns, readActions, + resolveRunStorage, saveProjectConfig, + type EnvLike, } from "@pickforge/picklab-core"; import { withMcpEvidence } from "../src/evidence.js"; import { @@ -15,10 +17,12 @@ import { } from "./helpers.js"; let dirs: LabDirs; +let env: EnvLike; const sessionId = "desk-evidence"; beforeEach(() => { dirs = makeLabDirs(); + env = { PICKLAB_HOME: dirs.home }; }); afterEach(() => { @@ -26,11 +30,10 @@ afterEach(() => { }); async function evidenceRecords() { - const [manifest] = await listRuns(dirs.projectDir); + const [manifest] = await listRuns(dirs.projectDir, env); expect(manifest).toBeDefined(); - return readActions( - path.join(dirs.projectDir, ".picklab", "runs", manifest!.runId), - ); + const { runsDir } = await resolveRunStorage(dirs.projectDir, env); + return readActions(path.join(runsDir, manifest!.runId)); } describe("MCP evidence producer", () => { diff --git a/packages/mcp-server/test/helpers.ts b/packages/mcp-server/test/helpers.ts index 6860cb7..79b40c1 100644 --- a/packages/mcp-server/test/helpers.ts +++ b/packages/mcp-server/test/helpers.ts @@ -44,7 +44,14 @@ export async function connectLab(opts: { projectDir: string; env: Record; }): Promise { - const server = createMcpServer(opts); + // Default new runs to the pre-#34 project-local layout so existing + // fixtures (`writeSyntheticRun`, hardcoded `.picklab/runs` assertions) + // keep working; individual tests can still override PICKLAB_STORAGE_MODE + // via opts.env to exercise the new "home" default explicitly. + const server = createMcpServer({ + ...opts, + env: { PICKLAB_STORAGE_MODE: "project-local", ...opts.env }, + }); const client = new Client({ name: "picklab-test", version: "0.0.0" }); const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); From 3a49d3b690361b8fce873a5c71ab38da1570fd9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 01:09:35 -0300 Subject: [PATCH 2/5] fix(agent-installers): apply the legacy home fallback to agent state readAgentsState previously only checked the new PICKLAB_HOME default, so "is this agent registered?" silently forgot pre-#34 ~/.picklab state instead of falling back to it non-destructively, unlike global config and sessions. --- packages/agent-installers/src/state.ts | 25 ++++- packages/agent-installers/test/state.test.ts | 109 +++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 packages/agent-installers/test/state.test.ts diff --git a/packages/agent-installers/src/state.ts b/packages/agent-installers/src/state.ts index 3270e18..1e8eab9 100644 --- a/packages/agent-installers/src/state.ts +++ b/packages/agent-installers/src/state.ts @@ -1,6 +1,12 @@ import fs from "node:fs"; import path from "node:path"; -import { agentsDir, writeFileAtomic, type EnvLike } from "@pickforge/picklab-core"; +import { + agentsDir, + legacyAgentsDir, + resolveReadablePath, + writeFileAtomic, + type EnvLike, +} from "@pickforge/picklab-core"; export interface AgentStateEntry { registered: boolean; @@ -20,12 +26,27 @@ export function agentsStatePath(env: EnvLike = process.env): string { return path.join(agentsDir(env), "state.json"); } +function legacyAgentsStatePath(env: EnvLike): string | undefined { + const dir = legacyAgentsDir(env); + return dir === undefined ? undefined : path.join(dir, "state.json"); +} + +/** + * Read agent registration state. Falls back to a pre-#34 `~/.picklab/agents` + * record when the new home has none yet, so "is this agent registered?" + * keeps answering correctly across the default-root change without a + * migration step. Writes (`recordAgentState`) always target the new home. + */ export async function readAgentsState( env: EnvLike = process.env, ): Promise { + const statePath = await resolveReadablePath( + agentsStatePath(env), + legacyAgentsStatePath(env), + ); let raw: string; try { - raw = await fs.promises.readFile(agentsStatePath(env), "utf8"); + raw = await fs.promises.readFile(statePath, "utf8"); } catch (error) { const code = (error as NodeJS.ErrnoException).code; if (code === "ENOENT" || code === "ENOTDIR") { diff --git a/packages/agent-installers/test/state.test.ts b/packages/agent-installers/test/state.test.ts new file mode 100644 index 0000000..274c720 --- /dev/null +++ b/packages/agent-installers/test/state.test.ts @@ -0,0 +1,109 @@ +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 { readAgentsState, recordAgentState } from "../src/state.js"; + +let fakeHome: string; +let homedirSpy: ReturnType; + +beforeEach(async () => { + fakeHome = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-agentstate-fakehome-"), + ); + homedirSpy = vi.spyOn(os, "homedir").mockReturnValue(fakeHome); +}); + +afterEach(async () => { + homedirSpy.mockRestore(); + await fs.promises.rm(fakeHome, { recursive: true, force: true }); +}); + +describe("readAgentsState legacy home fallback", () => { + it("reads a legacy ~/.picklab/agents/state.json when PICKLAB_HOME is unset", async () => { + const legacyDir = path.join(fakeHome, ".picklab", "agents"); + await fs.promises.mkdir(legacyDir, { recursive: true }); + await fs.promises.writeFile( + path.join(legacyDir, "state.json"), + JSON.stringify({ + agents: { + "claude-code": { + registered: true, + configPath: "/legacy/.claude.json", + updatedAt: "2026-01-01T00:00:00.000Z", + }, + }, + }), + ); + + const state = await readAgentsState({}); + expect(state.agents["claude-code"]?.registered).toBe(true); + expect(state.agents["claude-code"]?.configPath).toBe("/legacy/.claude.json"); + }); + + it("prefers the new home once it has its own state", async () => { + await fs.promises.mkdir(path.join(fakeHome, ".picklab", "agents"), { + recursive: true, + }); + await fs.promises.writeFile( + path.join(fakeHome, ".picklab", "agents", "state.json"), + JSON.stringify({ + agents: { codex: { registered: true, configPath: "/legacy/codex" } }, + }), + ); + await recordAgentState( + "codex", + { registered: false, configPath: "/new/codex" }, + {}, + ); + + const state = await readAgentsState({}); + expect(state.agents.codex?.registered).toBe(false); + expect(state.agents.codex?.configPath).toBe("/new/codex"); + }); + + it("does not fall back once PICKLAB_HOME is set explicitly", async () => { + await fs.promises.mkdir(path.join(fakeHome, ".picklab", "agents"), { + recursive: true, + }); + await fs.promises.writeFile( + path.join(fakeHome, ".picklab", "agents", "state.json"), + JSON.stringify({ + agents: { codex: { registered: true, configPath: "/legacy/codex" } }, + }), + ); + + const state = await readAgentsState({ PICKLAB_HOME: "/other" }); + expect(state.agents).toEqual({}); + }); + + it("writes always target the new home, leaving legacy state untouched", async () => { + const legacyPath = path.join( + fakeHome, + ".picklab", + "agents", + "state.json", + ); + await fs.promises.mkdir(path.dirname(legacyPath), { recursive: true }); + await fs.promises.writeFile( + legacyPath, + JSON.stringify({ + agents: { codex: { registered: true, configPath: "/legacy/codex" } }, + }), + ); + + await recordAgentState( + "cursor", + { registered: true, configPath: "/new/cursor" }, + {}, + ); + + const legacyRaw = JSON.parse(await fs.promises.readFile(legacyPath, "utf8")); + expect(legacyRaw.agents.cursor).toBeUndefined(); + expect( + fs.existsSync( + path.join(fakeHome, ".pickforge", "picklab", "agents", "state.json"), + ), + ).toBe(true); + }); +}); From f2568a1a6ba02e2319d5ddac43c1639aee6ac94d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 01:29:51 -0300 Subject: [PATCH 3/5] fix(core): scope evidence retention pruning to the primary storage root pruneFinalizedEvidenceRuns computed retention over the full merged run catalog, which under home/custom mode includes the read-only legacy project-local fallback root added for discovery. Once more than `keep` finalized runs existed across both roots, it would rm -rf the oldest ones regardless of which root they lived in, deleting pre-existing legacy runs it never owned and never migrated. Reproduced live: 15 legacy + 10 new runs, keep=5 deleted 5 legacy dirs. Scope both the candidate set and the active-pointer bookkeeping to entries whose rootDir matches the resolved primary storage root; the legacy fallback root is read-only by contract. Also corrects the docstring, which still claimed no finalization producer called this function (finalizeActiveEvidenceRun has called it automatically since this feature shipped). --- packages/core/src/evidence.ts | 18 +++++++-- packages/core/test/evidence.test.ts | 59 +++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/packages/core/src/evidence.ts b/packages/core/src/evidence.ts index e4fcaa3..76a56d9 100644 --- a/packages/core/src/evidence.ts +++ b/packages/core/src/evidence.ts @@ -1672,10 +1672,16 @@ async function collectActiveRunIds(parent: string): Promise> { /** * Retain only the newest `keep` (default 20) finalized evidence runs per * project, deleting older finalized evidence run directories. Never prunes: - * running/active runs (status `running` or referenced by an active pointer) or - * legacy runs (no `evidenceVersion`). Returns the removed run ids. + * running/active runs (status `running` or referenced by an active pointer), + * legacy runs (no `evidenceVersion`), or anything outside the resolved + * storage mode's primary root. `openRunCatalog` layers a read-only legacy + * project-local root under home/custom mode purely for discovery + * (`artifact_list`/`report`/resources) — pruning that root would delete + * runs this process does not own and never migrated, so retention counting + * and removal are scoped to `entry.rootDir === resolved.runsDir` only. * - * This is a primitive: no finalization producer calls it yet. + * `finalizeActiveEvidenceRun` calls this automatically after every + * finalization. */ export async function pruneFinalizedEvidenceRuns( projectDir: string, @@ -1683,8 +1689,12 @@ export async function pruneFinalizedEvidenceRuns( env: EnvLike = process.env, ): Promise { const keep = opts.keep ?? EVIDENCE_RETENTION_KEEP; + const resolved = await resolveRunStorage(projectDir, env); + const primaryRootDir = path.resolve(resolved.runsDir); const catalog = await openRunCatalog(projectDir, env); - const entries = await catalog.list(); + const entries = (await catalog.list()).filter( + (entry) => entry.rootDir === primaryRootDir, + ); const activeByRoot = new Map>(); for (const entry of entries) { if (!activeByRoot.has(entry.rootDir)) { diff --git a/packages/core/test/evidence.test.ts b/packages/core/test/evidence.test.ts index 824a14f..53190fe 100644 --- a/packages/core/test/evidence.test.ts +++ b/packages/core/test/evidence.test.ts @@ -1328,6 +1328,65 @@ describe("finalized-run retention", () => { expect(fs.existsSync(symDir)).toBe(true); expect(fs.existsSync(badDir)).toBe(true); }); + + it("never prunes pre-existing legacy project-local runs through the read-only fallback root", async () => { + // Reproduces the P0: home mode layers the legacy project-local root + // under the catalog purely for read discovery. Retention must count and + // delete only within the resolved primary (home) root — never treat the + // legacy root's runs as removal candidates just because the merged + // cross-root list exceeds `keep`. + const home = await fs.promises.mkdtemp( + path.join(os.tmpdir(), "picklab-evi-prune-home-"), + ); + const homeEnv = { PICKLAB_HOME: home }; + try { + // 15 pre-existing legacy finalized runs (the file-level stub keeps + // `finalizedEvidenceRun`'s default env at project-local). + const legacyIds: string[] = []; + for (let i = 0; i < 15; i += 1) { + const minute = String(i).padStart(2, "0"); + legacyIds.push( + await finalizedEvidenceRun(`2026-05-01T09:${minute}:00Z`), + ); + } + // 10 new home-mode finalized runs. + const homeIds: string[] = []; + for (let i = 0; i < 10; i += 1) { + const minute = String(i).padStart(2, "0"); + const run = await createRun( + project, + "evi", + { evidence: true, now: new Date(`2026-06-09T10:${minute}:00Z`) }, + homeEnv, + ); + await run.finish("completed"); + homeIds.push(run.runId); + } + + // 25 total across both roots; `keep: 5` would prune 20 of them if + // retention were computed over the merged catalog instead of the + // primary root alone. + const removed = await pruneFinalizedEvidenceRuns( + project, + { keep: 5 }, + homeEnv, + ); + + expect(removed).toHaveLength(5); + expect(removed.sort()).toEqual(homeIds.slice(0, 5).sort()); + // All 15 legacy runs survive untouched. + for (const id of legacyIds) { + expect(fs.existsSync(path.join(runsRoot(), id))).toBe(true); + } + // The 5 newest home runs survive; only the 5 oldest home runs are gone. + const survivingHomeIds = (await listRuns(project, homeEnv)) + .map((r) => r.runId) + .filter((id) => homeIds.includes(id)); + expect(survivingHomeIds.sort()).toEqual(homeIds.slice(5).sort()); + } finally { + await fs.promises.rm(home, { recursive: true, force: true }); + } + }); }); describe("defensive filesystem errors propagate", () => { From bf0f775420ea3d5309508c4c9a9742d48523ef00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 01:30:02 -0300 Subject: [PATCH 4/5] fix(core): restrict custom storage to global config/env, reject in-project paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project-committed .picklab/config.json could select storage.mode "custom" with an arbitrary path, and that config travels with `git clone` — a hostile or careless repo could silently redirect artifact writes (screenshots, which may carry secrets) to any absolute path with zero prompt. Only the user-owned global config or the PICKLAB_STORAGE_MODE / PICKLAB_STORAGE_PATH env override may now select "custom" (and supply its path). Project config may still select "project-local" (blast radius already scoped to itself) or "home". A project config requesting "custom" is not an error: it falls back to global config's mode, then "home", so a cloned repo with a hostile or misconfigured project config never bricks `picklab`. The rejection is reported on the resolved result (`rejectedProjectCustom`) and surfaced by `picklab doctor` as a warning. Also reject a custom path that equals or is nested inside the project directory — otherwise it just reintroduces project pollution under a different name. Implemented by reading the global and project config layers separately (config.ts's new loadConfigLayers) instead of resolving storage from loadConfig's already-merged blob, since the merge alone can't tell which layer a value came from. --- README.md | 24 +++- docs/releases/UNRELEASED.md | 43 +++++-- packages/cli/src/provision/checks.ts | 19 +++ packages/cli/src/provision/detect.ts | 16 +++ packages/cli/test/checks.test.ts | 32 +++++ packages/core/src/config.ts | 35 +++++- packages/core/src/storage.ts | 130 ++++++++++++++++---- packages/core/test/storage.test.ts | 174 ++++++++++++++++++++++++++- 8 files changed, 427 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index f8830f7..6f35332 100644 --- a/README.md +++ b/README.md @@ -77,9 +77,10 @@ never collide. Use the platform home-directory equivalent on non-Linux systems. `PICKLAB_HOME` overrides the PickLab home root (default `~/.pickforge/picklab`); `picklab doctor` reports the resolved path. -Two other modes are available via `storage` in `.picklab/config.json` -(project-level) or the global config, or the `PICKLAB_STORAGE_MODE` / -`PICKLAB_STORAGE_PATH` environment overrides for automation and tests: +Two other modes are available via `storage` in the **global** config or the +`PICKLAB_STORAGE_MODE` / `PICKLAB_STORAGE_PATH` environment overrides for +automation and tests; `.picklab/config.json` (project-level) can select +`project-local`, but not `custom` — see below: ```json { @@ -91,9 +92,20 @@ Two other modes are available via `storage` in `.picklab/config.json` - `project-local` — restores the previous default: `.picklab/runs/` inside the project. Generated files then do appear in the project's source-control view; add `.picklab/runs/` to `.gitignore` if you opt into this mode. -- `custom` — an explicit absolute path: `{ "storage": { "mode": "custom", "path": "/abs/path" } }` - writes runs under `/runs/`. A relative path, or `custom` mode with no - path, is rejected. + Selectable from project or global config. +- `custom` — an explicit absolute path outside the project directory: + `{ "storage": { "mode": "custom", "path": "/abs/path" } }` writes runs + under `/runs/`. A relative path, a path equal to or nested inside the + project directory, or `custom` mode with no path, is rejected. + +**`custom` cannot be selected from project-level `.picklab/config.json`.** +That file is repo-committed and travels with `git clone`; honoring a +`custom` selection from it would let a cloned repository silently redirect +run artifacts (screenshots, which may carry secrets) to any absolute path +with no prompt. Only the user-owned global config or an env override may +select `custom`. A project config that requests `custom` is ignored — the +resolver falls back to global config's mode, then `home` — and `picklab +doctor` surfaces the rejected request as a warning. `.picklab/config.json` itself always stays project-local regardless of `storage` mode — only generated runtime artifacts move. diff --git a/docs/releases/UNRELEASED.md b/docs/releases/UNRELEASED.md index 3a45c06..39950a4 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -12,12 +12,17 @@ release description, then reset it after the release is published. `~/.pickforge/picklab` (was `~/.picklab`); the old default is still read non-destructively as a fallback for existing global config, agent state, and sessions — nothing is moved or deleted. `project-local` (restores the - previous layout) and `custom` (explicit absolute path) storage modes are - available via `.picklab/config.json`'s `storage` field or the - `PICKLAB_STORAGE_MODE` / `PICKLAB_STORAGE_PATH` env overrides. Existing - project-local runs remain discoverable by `artifact_list` / - `artifact_report` / MCP resources without migration. See the README's "Run - storage" section. + previous layout, selectable from project or global config) and `custom` + (explicit absolute path outside the project directory) storage modes are + available via the user-owned global config's `storage` field or the + `PICKLAB_STORAGE_MODE` / `PICKLAB_STORAGE_PATH` env overrides — a + project-committed `.picklab/config.json` cannot select `custom` (it + travels with `git clone`; a cloned repo requesting it falls back to home + with a `picklab doctor` warning, never a silent redirect of screenshots to + an attacker-chosen path). Existing project-local runs remain discoverable + by `artifact_list` / `artifact_report` / MCP resources without migration, + and retention pruning never touches them — only the resolved primary + storage root is ever pruned. See the README's "Run storage" section. - Added isolated headed Chrome/Chromium sessions, including CLI and MCP session lifecycle support and a static `picklab-browser` DevTools MCP entry. - Added `picklab watch` plus configurable manual/automatic VNC viewer attachment @@ -45,9 +50,19 @@ release description, then reset it after the release is published. catalog, and active-evidence-pointer resolution in core, CLI, and MCP through it. `openRunCatalog` now layers the resolved primary root with a read-only legacy project-local fallback root for non-destructive discovery. + The resolver applies a source allowlist so only the global config layer or + an env override may select `custom` storage (project config may still + select `project-local` or `home`), and rejects a custom path that equals + or is nested inside the project directory. `pruneFinalizedEvidenceRuns` + scopes its retention candidate set to the resolved primary root + (`entry.rootDir`) so the read-only legacy fallback root is never a removal + candidate. Added a legacy read-fallback (`resolveReadablePath`, per-entry) for global config, agent state, and sessions across the `~/.picklab` → - `~/.pickforge/picklab` default-root change. + `~/.pickforge/picklab` default-root change. `destroySessionRecord` now + removes both the new-home and legacy copies of a session record + unconditionally, so a record that exists at both locations cannot + resurrect via the legacy read fallback after being destroyed. - Added a private browser lifecycle package with confined ephemeral profiles, loopback-only CDP discovery, verified process-group teardown, concurrent session safety, and conservative dead-session reaping. @@ -86,10 +101,16 @@ release description, then reset it after the release is published. overrides) and `run-catalog.test.ts`'s "openRunCatalog storage modes" suite (home default, legacy project-local discovery, project isolation, custom mode), plus a `git status --porcelain` repo-cleanliness smoke test. -- `bun run test` — same pre-existing failure set as unmodified `main` on this - (macOS) sandbox (Darwin lacks `/proc`, so PID-identity/Xvfb/x11vnc-driven - tests fail there and on CI's Linux runners this class does not apply); - verified test-by-test against `main` on this branch's dev sandbox. +- `bun run test` — same pre-existing failure set as unmodified `main`, + verified test-by-test against `main` on this branch's (macOS) dev sandbox. + These are platform-environment failures, not specific to this change; on + CI's Linux runners this class does not apply. At least three distinct + causes, none `/proc`-only as previously stated here: + process-identity/Xvfb/x11vnc verification (Darwin has no `/proc`; most of + the count), a macOS tmpdir realpath mismatch (`/var` vs `/private/var`, + e.g. `run-catalog.test.ts`'s root-precedence test and + `devtools-mcp.test.ts`'s package-root assertion), and inline-screenshot/CDP + association mismatches in `devtools-evidence.test.ts`. - `bun run build` ### Not tested yet diff --git a/packages/cli/src/provision/checks.ts b/packages/cli/src/provision/checks.ts index b48b567..3aa6e32 100644 --- a/packages/cli/src/provision/checks.ts +++ b/packages/cli/src/provision/checks.ts @@ -111,6 +111,25 @@ export function evaluateChecks(s: DetectionSnapshot): DoctorCheck[] { }); } + if (s.storage.rejectedProjectCustom !== null) { + const requested = s.storage.rejectedProjectCustom.requestedPath; + checks.push({ + id: "storage-custom-rejected", + title: "Project config requested custom storage", + status: "warn", + detail: + requested === undefined + ? "the project's .picklab/config.json requested storage.mode " + + '"custom" with no path; it was ignored' + : `the project's .picklab/config.json requested storage.mode ` + + `"custom" (path: ${requested}); it was ignored`, + hint: + "project-committed config cannot select custom storage (it travels " + + "with git clone); set storage.mode in the global config instead, " + + "or PICKLAB_STORAGE_MODE/PICKLAB_STORAGE_PATH", + }); + } + if (s.config.ok) { checks.push({ id: "config", diff --git a/packages/cli/src/provision/detect.ts b/packages/cli/src/provision/detect.ts index 920fe83..5afd88b 100644 --- a/packages/cli/src/provision/detect.ts +++ b/packages/cli/src/provision/detect.ts @@ -12,6 +12,7 @@ import { loadConfig, picklabHome, resolvedDefaults, + resolveRunStorage, runCommand, type EnvLike, type PicklabProfile, @@ -28,6 +29,10 @@ export interface DetectionSnapshot { * explicitly — that is the user's own root, not a legacy one). */ legacyHome: { path: string } | null; config: { ok: boolean; error: string | null; profile: PicklabProfile | null }; + /** Present when the project-committed `.picklab/config.json` requested + * `storage.mode: "custom"` and the resolver rejected it (repo config + * cannot select custom storage) and fell back to the next layer. */ + storage: { rejectedProjectCustom: { requestedPath?: string } | null }; desktop: { xvfb: string | null; xdotool: string | null; @@ -132,6 +137,16 @@ export async function collectSnapshot( ? { path: legacyPath } : null; + let rejectedProjectCustom: { requestedPath?: string } | null = null; + try { + const resolvedStorage = await resolveRunStorage(projectDir, env); + rejectedProjectCustom = resolvedStorage.rejectedProjectCustom ?? null; + } catch { + // A resolver error (e.g. a broken global/env custom path) is not this + // check's concern; it surfaces wherever storage is actually resolved for + // a run. + } + const androidEnv = detectAndroidEnvironment({ env, homeDir: env.HOME !== undefined && env.HOME !== "" ? env.HOME : undefined, @@ -150,6 +165,7 @@ export async function collectSnapshot( }, legacyHome, config, + storage: { rejectedProjectCustom }, desktop: { xvfb: findOnPath("Xvfb", env), xdotool: findOnPath("xdotool", env), diff --git a/packages/cli/test/checks.test.ts b/packages/cli/test/checks.test.ts index 6642110..41312a0 100644 --- a/packages/cli/test/checks.test.ts +++ b/packages/cli/test/checks.test.ts @@ -10,6 +10,7 @@ function snapshot( overrides: { picklabHome?: Partial; legacyHome?: DetectionSnapshot["legacyHome"]; + storage?: DetectionSnapshot["storage"]; config?: Partial; desktop?: Partial; android?: Partial; @@ -25,6 +26,7 @@ function snapshot( ...overrides.picklabHome, }, legacyHome: overrides.legacyHome ?? null, + storage: overrides.storage ?? { rejectedProjectCustom: null }, config: { ok: true, error: null, profile: null, ...overrides.config }, desktop: { xvfb: "/usr/bin/Xvfb", @@ -111,6 +113,36 @@ describe("evaluateChecks", () => { expect(check.hint).toContain("non-destructively"); }); + it("omits the storage-custom-rejected check when nothing was rejected", () => { + const checks = evaluateChecks(snapshot()); + expect( + checks.some((check) => check.id === "storage-custom-rejected"), + ).toBe(false); + }); + + it("surfaces a rejected project-config custom storage request as a non-blocking warning", () => { + const check = checkById( + snapshot({ + storage: { + rejectedProjectCustom: { requestedPath: "/attacker/path" }, + }, + }), + "storage-custom-rejected", + ); + expect(check.status).toBe("warn"); + expect(check.detail).toContain("/attacker/path"); + expect(check.hint).toContain("global config"); + }); + + it("surfaces a rejected request even with no requested path", () => { + const check = checkById( + snapshot({ storage: { rejectedProjectCustom: {} } }), + "storage-custom-rejected", + ); + expect(check.status).toBe("warn"); + expect(check.detail).toContain("no path"); + }); + it("flags a broken config with its parse error", () => { const check = checkById( snapshot({ config: { ok: false, error: "Invalid PickLab config" } }), diff --git a/packages/core/src/config.ts b/packages/core/src/config.ts index 240213f..72ab74e 100644 --- a/packages/core/src/config.ts +++ b/packages/core/src/config.ts @@ -111,16 +111,47 @@ export async function readConfigFile(filePath: string): Promise { } } -export async function loadConfig( +export interface ConfigLayers { + /** The user-owned global config layer (never travels with `git clone`). */ + global: PicklabConfig; + /** The project-committed `.picklab/config.json` layer. */ + project: PicklabConfig; +} + +/** + * Read the global and project config layers separately, without merging + * them. Most callers want the merged view (`loadConfig`); this exists for + * callers — currently only `resolveRunStorage` — that must know which layer + * a value came from because the two layers carry different trust levels + * (project config is repo-committed and travels with `git clone`; global + * config is local and user-owned). + */ +export async function loadConfigLayers( projectDir: string, env: EnvLike = process.env, -): Promise { +): Promise { + // `resolveReadablePath` picks one whole file, never merges the new and + // legacy homes field-by-field: whichever one exists (new preferred) is + // read entirely on its own. That's safe here only because the caller + // (`loadConfig`, below) re-merges the result over `resolvedDefaults` and + // under the project layer anyway, so an all-or-nothing pick of the global + // layer still composes correctly with the rest of the precedence chain. + // A caller that needs partial-field legacy/new blending would need a + // different primitive. const globalPath = await resolveReadablePath( globalConfigPath(env), legacyGlobalConfigPath(env), ); const global = await readConfigFile(globalPath); const project = await readConfigFile(projectConfigPath(projectDir)); + return { global, project }; +} + +export async function loadConfig( + projectDir: string, + env: EnvLike = process.env, +): Promise { + const { global, project } = await loadConfigLayers(projectDir, env); return deepMerge( deepMerge(deepMerge({}, resolvedDefaults), global), project, diff --git a/packages/core/src/storage.ts b/packages/core/src/storage.ts index ce76d34..fc041ab 100644 --- a/packages/core/src/storage.ts +++ b/packages/core/src/storage.ts @@ -1,7 +1,12 @@ import crypto from "node:crypto"; import fs from "node:fs"; import path from "node:path"; -import { loadConfig, type StorageConfig, type StorageMode } from "./config.js"; +import { + loadConfigLayers, + resolvedDefaults, + type StorageConfig, + type StorageMode, +} from "./config.js"; import { picklabHome, runsDir, type EnvLike } from "./paths.js"; export type { StorageConfig, StorageMode } from "./config.js"; @@ -60,6 +65,17 @@ export interface ResolvedRunStorage { runsDir: string; /** Present only for `mode: "home"`. */ projectId?: string; + /** + * Present when the project-committed `.picklab/config.json` requested + * `storage.mode: "custom"` and it was rejected: a repo-committed config + * travels with `git clone` and screenshots may carry secrets, so only the + * user-owned global config or an env override may select `custom` (and + * supply its path). The resolver falls back to the next layer (global + * config, then `home`) rather than erroring, so a cloned repo with a + * hostile or misconfigured project config never bricks `picklab`; `picklab + * doctor` surfaces this field as a warning. + */ + rejectedProjectCustom?: { requestedPath?: string }; } function envStorageMode(env: EnvLike): StorageMode | undefined { @@ -70,18 +86,51 @@ function envStorageMode(env: EnvLike): StorageMode | undefined { return undefined; } +function validateCustomPath(customPath: string | undefined): string { + if (customPath === undefined || customPath === "") { + throw new StorageConfigError( + 'storage mode "custom" requires a storage path ' + + "(storage.path in config, or PICKLAB_STORAGE_PATH)", + ); + } + if (!path.isAbsolute(customPath)) { + throw new StorageConfigError( + `storage path must be an absolute path, got "${customPath}"`, + ); + } + return path.resolve(customPath); +} + +/** Whether `descendant` is `ancestor` itself or strictly nested under it. */ +function isSameOrDescendant(ancestor: string, descendant: string): boolean { + const relative = path.relative(ancestor, descendant); + return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); +} + /** - * Resolve where new run artifacts should be written for a project. Reads - * `storage` from project/global config (`.picklab/config.json` / - * `/config.json`), then an explicit `PICKLAB_STORAGE_MODE` / - * `PICKLAB_STORAGE_PATH` environment override for automation and tests. Env - * wins over config so a CI job or test harness can force a mode without - * touching project files. + * Resolve where new run artifacts should be written for a project. + * + * Reads `storage` from two config layers plus an environment override, in + * increasing precedence: global config (`/config.json`), + * project config (`.picklab/config.json`), then `PICKLAB_STORAGE_MODE` / + * `PICKLAB_STORAGE_PATH` for automation and tests. **`custom` mode is an + * exception to that precedence**: only the global-config layer or the env + * override may select it (and supply its path) — project config is + * repo-committed and travels with `git clone`, so honoring a `custom` + * selection from it would let a cloned repository silently redirect + * artifact writes (screenshots, which may carry secrets) to any absolute + * path with no prompt. A project config requesting `custom` is treated as + * absent and the resolver falls through to global config, then `home`; see + * `rejectedProjectCustom` on the result. Project config may still select + * `project-local` (blast radius already scoped to the project itself) or + * `home`. * * - `home` (default): `/projects//runs`, isolated per * project and outside every target repository. * - `project-local`: the pre-#34 `/.picklab/runs` layout. - * - `custom`: `/runs` under an explicit absolute path. + * - `custom`: `/runs` under an explicit absolute path outside + * the project directory (rejected if it equals or is nested inside it — + * that would just reintroduce project pollution, un-namespaced). * * This is the single resolver every run-creation and artifact-lookup path * (core, CLI, MCP) goes through, so they always agree on where runs live. @@ -90,34 +139,71 @@ export async function resolveRunStorage( projectDir: string, env: EnvLike = process.env, ): Promise { - const config = await loadConfig(projectDir, env); - const storage: StorageConfig = config.storage ?? {}; - const mode = envStorageMode(env) ?? storage.mode ?? "home"; - const customPath = env.PICKLAB_STORAGE_PATH ?? storage.path; + const { global, project } = await loadConfigLayers(projectDir, env); + const globalStorage: StorageConfig = global.storage ?? {}; + const projectStorage: StorageConfig = project.storage ?? {}; + + const envMode = envStorageMode(env); + let mode: StorageMode; + let customPath: string | undefined; + let rejectedProjectCustom: { requestedPath?: string } | undefined; + + if (envMode !== undefined) { + // The environment is always user/automation-controlled, never + // repo-committed: it may select any mode, including custom. Its custom + // path may come from the env itself or from global config, but never + // from project config — project config never supplies a custom path, + // regardless of which layer selected the mode. + mode = envMode; + customPath = env.PICKLAB_STORAGE_PATH ?? globalStorage.path; + } else if (projectStorage.mode === "custom") { + rejectedProjectCustom = { requestedPath: projectStorage.path }; + mode = globalStorage.mode ?? resolvedDefaults.storage.mode; + customPath = globalStorage.path; + } else if (projectStorage.mode !== undefined) { + // Guaranteed not "custom" (handled above), so no path is needed. + mode = projectStorage.mode; + customPath = undefined; + } else { + mode = globalStorage.mode ?? resolvedDefaults.storage.mode; + customPath = globalStorage.path; + } if (mode === "project-local") { - return { mode, runsDir: runsDir(projectDir) }; + const resolved: ResolvedRunStorage = { mode, runsDir: runsDir(projectDir) }; + if (rejectedProjectCustom !== undefined) { + resolved.rejectedProjectCustom = rejectedProjectCustom; + } + return resolved; } if (mode === "custom") { - if (customPath === undefined || customPath === "") { + const resolvedCustomRoot = validateCustomPath(customPath); + const resolvedProjectDir = path.resolve(projectDir); + if (isSameOrDescendant(resolvedProjectDir, resolvedCustomRoot)) { throw new StorageConfigError( - 'storage mode "custom" requires a storage path ' + - "(storage.path in config, or PICKLAB_STORAGE_PATH)", + `storage path must be outside the project directory, got "${customPath}" ` + + `under "${resolvedProjectDir}"`, ); } - if (!path.isAbsolute(customPath)) { - throw new StorageConfigError( - `storage path must be an absolute path, got "${customPath}"`, - ); + const resolved: ResolvedRunStorage = { + mode, + runsDir: path.join(resolvedCustomRoot, "runs"), + }; + if (rejectedProjectCustom !== undefined) { + resolved.rejectedProjectCustom = rejectedProjectCustom; } - return { mode, runsDir: path.join(path.resolve(customPath), "runs") }; + return resolved; } const id = await projectId(projectDir); - return { + const resolved: ResolvedRunStorage = { mode: "home", runsDir: path.join(picklabHome(env), "projects", id, "runs"), projectId: id, }; + if (rejectedProjectCustom !== undefined) { + resolved.rejectedProjectCustom = rejectedProjectCustom; + } + return resolved; } diff --git a/packages/core/test/storage.test.ts b/packages/core/test/storage.test.ts index 41714a1..465ac78 100644 --- a/packages/core/test/storage.test.ts +++ b/packages/core/test/storage.test.ts @@ -11,7 +11,7 @@ import { resolveRunStorage, StorageConfigError, } from "../src/storage.js"; -import { saveProjectConfig } from "../src/config.js"; +import { saveGlobalConfig, saveProjectConfig } from "../src/config.js"; import { createRun } from "../src/run.js"; const execFileAsync = promisify(execFile); @@ -128,15 +128,31 @@ describe("resolveRunStorage", () => { expect(resolved.mode).toBe("home"); }); - it("resolves custom mode from config with an absolute path", async () => { + it("resolves custom mode from GLOBAL config with an absolute path", async () => { + const home = path.join(root, "home"); const project = path.join(root, "project"); const custom = path.join(root, "custom-artifacts"); await fs.promises.mkdir(project, { recursive: true }); - await saveProjectConfig(project, { - storage: { mode: "custom", path: custom }, + await saveGlobalConfig({ storage: { mode: "custom", path: custom } }, { + PICKLAB_HOME: home, }); - const resolved = await resolveRunStorage(project, {}); + const resolved = await resolveRunStorage(project, { PICKLAB_HOME: home }); + + expect(resolved.mode).toBe("custom"); + expect(resolved.runsDir).toBe(path.join(custom, "runs")); + expect(resolved.rejectedProjectCustom).toBeUndefined(); + }); + + it("resolves custom mode from PICKLAB_STORAGE_MODE + PICKLAB_STORAGE_PATH", async () => { + const project = path.join(root, "project"); + const custom = path.join(root, "custom-artifacts"); + await fs.promises.mkdir(project, { recursive: true }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: custom, + }); expect(resolved.mode).toBe("custom"); expect(resolved.runsDir).toBe(path.join(custom, "runs")); @@ -162,6 +178,154 @@ describe("resolveRunStorage", () => { }), ).rejects.toThrow(/absolute/i); }); + + describe("project-committed config cannot select custom (P1)", () => { + it("ignores a project-config custom request and falls back to home, flagging the rejection", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + const hostilePath = path.join(root, "attacker-controlled"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { + storage: { mode: "custom", path: hostilePath }, + }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_HOME: home, + }); + + expect(resolved.mode).toBe("home"); + expect(resolved.runsDir.startsWith(home)).toBe(true); + expect(resolved.rejectedProjectCustom).toEqual({ + requestedPath: hostilePath, + }); + }); + + it("falls back to global config's mode (not the hostile path) when the project requests custom", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + const hostilePath = path.join(root, "attacker-controlled"); + await fs.promises.mkdir(project, { recursive: true }); + await saveGlobalConfig({ storage: { mode: "project-local" } }, { + PICKLAB_HOME: home, + }); + await saveProjectConfig(project, { + storage: { mode: "custom", path: hostilePath }, + }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_HOME: home, + }); + + expect(resolved.mode).toBe("project-local"); + expect(resolved.rejectedProjectCustom).toBeDefined(); + }); + + it("still allows project config to select project-local", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { storage: { mode: "project-local" } }); + + const resolved = await resolveRunStorage(project, {}); + + expect(resolved.mode).toBe("project-local"); + expect(resolved.rejectedProjectCustom).toBeUndefined(); + }); + + it("still allows project config to select home", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await saveGlobalConfig({ storage: { mode: "project-local" } }, { + PICKLAB_HOME: home, + }); + await saveProjectConfig(project, { storage: { mode: "home" } }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_HOME: home, + }); + + expect(resolved.mode).toBe("home"); + expect(resolved.rejectedProjectCustom).toBeUndefined(); + }); + + it("lets an env override select custom even when project config also requests it", async () => { + const project = path.join(root, "project"); + const custom = path.join(root, "custom-artifacts"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { + storage: { mode: "custom", path: "/should-be-ignored" }, + }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: custom, + }); + + expect(resolved.mode).toBe("custom"); + expect(resolved.runsDir).toBe(path.join(custom, "runs")); + // The env override made the mode selection moot, so no rejection is + // reported; the project's path was never consulted either way. + expect(resolved.rejectedProjectCustom).toBeUndefined(); + }); + + it("an env mode override never uses the project layer's custom path", async () => { + const home = path.join(root, "home"); + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + await saveProjectConfig(project, { + storage: { mode: "custom", path: "/should-never-be-used" }, + }); + + // env selects custom but supplies no path, and global config has none + // either: this must fail closed (missing path), never silently reuse + // the project layer's path. + await expect( + resolveRunStorage(project, { + PICKLAB_HOME: home, + PICKLAB_STORAGE_MODE: "custom", + }), + ).rejects.toThrow(StorageConfigError); + }); + }); + + describe("custom path containment (P2)", () => { + it("rejects a custom path equal to the project directory", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + + await expect( + resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: project, + }), + ).rejects.toThrow(/outside the project directory/i); + }); + + it("rejects a custom path nested inside the project directory", async () => { + const project = path.join(root, "project"); + await fs.promises.mkdir(project, { recursive: true }); + + await expect( + resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: path.join(project, "artifacts"), + }), + ).rejects.toThrow(/outside the project directory/i); + }); + + it("accepts a custom path that is a sibling of the project directory", async () => { + const project = path.join(root, "project"); + const sibling = path.join(root, "project-artifacts"); + await fs.promises.mkdir(project, { recursive: true }); + + const resolved = await resolveRunStorage(project, { + PICKLAB_STORAGE_MODE: "custom", + PICKLAB_STORAGE_PATH: sibling, + }); + + expect(resolved.runsDir).toBe(path.join(sibling, "runs")); + }); + }); }); describe("repo cleanliness smoke", () => { From 2063a84e59ee7d7905c8522da8be859cce75a0d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elberte=20Pl=C3=ADnio?= Date: Thu, 23 Jul 2026 01:30:13 -0300 Subject: [PATCH 5/5] fix(core): remove both session copies on destroy, not just the readable one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit destroySessionRecord removed only the path resolveReadablePath would pick for a read (new home if present, else legacy). A session created under the legacy home and later updated writes to the new home on every update but never touches the stale legacy copy, so once both exist, destroying it only deleted the new-home copy — resolveReadablePath then picked the surviving legacy file on the next read, resurrecting the "destroyed" session via getSession / listSessions. Remove both the new-home and legacy paths unconditionally (force); either or both may not exist. --- packages/core/src/session.ts | 16 +++++++++------ packages/core/test/session.test.ts | 32 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 3c4cf95..b3c5714 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -316,13 +316,17 @@ export async function destroySessionRecord( ).catch(() => {}); } } - // Remove from wherever the record actually lives (new home, or a legacy - // `~/.picklab` record read through the fallback) so an explicitly destroyed - // session never keeps reappearing in listSessions. + // Remove BOTH the new-home and legacy copies unconditionally, not just + // whichever `resolveReadablePath` would currently pick for a read. A + // session created under the legacy home and later updated (writes always + // target the new home) leaves stale copies at both locations; removing + // only the new-home one would let it resurrect via the legacy read + // fallback in getSession/listSessions. const legacyDir = legacySessionsDir(env); const legacyPath = legacyDir === undefined ? undefined : path.join(legacyDir, `${id}.json`); - await fs.promises.rm(await resolveReadablePath(sessionPath(id, env), legacyPath), { - force: true, - }); + await fs.promises.rm(sessionPath(id, env), { force: true }); + if (legacyPath !== undefined) { + await fs.promises.rm(legacyPath, { force: true }); + } } diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index f836742..3eca317 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -1202,4 +1202,36 @@ describe("legacy session home fallback", () => { ), ).toBe(false); }); + + it("destroying a session that has copies at BOTH the legacy and new home removes both, so it cannot resurrect", async () => { + // A session created under the legacy home, later updated: writes always + // target the new home, leaving a stale copy at the legacy path too. + writeLegacySession("desk-1eaac5"); + await updateSession("desk-1eaac5", { status: "running" }, {}); + + const legacyPath = path.join( + fakeHome, + ".picklab", + "sessions", + "desk-1eaac5.json", + ); + const newPath = path.join( + fakeHome, + ".pickforge", + "picklab", + "sessions", + "desk-1eaac5.json", + ); + expect(fs.existsSync(legacyPath)).toBe(true); + expect(fs.existsSync(newPath)).toBe(true); + + await destroySessionRecord("desk-1eaac5", {}); + + expect(fs.existsSync(legacyPath)).toBe(false); + expect(fs.existsSync(newPath)).toBe(false); + expect( + (await listSessions({})).some((record) => record.id === "desk-1eaac5"), + ).toBe(false); + expect(await getSession("desk-1eaac5", {})).toBeUndefined(); + }); });