diff --git a/README.md b/README.md index 58d919d..6f35332 100644 --- a/README.md +++ b/README.md @@ -59,7 +59,64 @@ 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 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 +{ + "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. + 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. + +**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 +125,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 +202,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..39950a4 100644 --- a/docs/releases/UNRELEASED.md +++ b/docs/releases/UNRELEASED.md @@ -5,6 +5,24 @@ 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, 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 @@ -26,6 +44,25 @@ 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. + 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. `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. @@ -60,12 +97,28 @@ 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`, + 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 +- `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/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); + }); +}); 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..3aa6e32 100644 --- a/packages/cli/src/provision/checks.ts +++ b/packages/cli/src/provision/checks.ts @@ -99,6 +99,37 @@ 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.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 e510c75..5afd88b 100644 --- a/packages/cli/src/provision/detect.ts +++ b/packages/cli/src/provision/detect.ts @@ -8,9 +8,11 @@ import { type SystemImage, } from "@pickforge/picklab-android"; import { + legacyPicklabHome, loadConfig, picklabHome, resolvedDefaults, + resolveRunStorage, runCommand, type EnvLike, type PicklabProfile, @@ -22,7 +24,15 @@ 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 }; + /** 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; @@ -121,6 +131,21 @@ 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; + + 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, @@ -138,7 +163,9 @@ export async function collectSnapshot( exists: homeExists, writable: homeExists && isWritable(homePath), }, + 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 02333ca..41312a0 100644 --- a/packages/cli/test/checks.test.ts +++ b/packages/cli/test/checks.test.ts @@ -9,6 +9,8 @@ import type { DetectionSnapshot } from "../src/provision/detect.js"; function snapshot( overrides: { picklabHome?: Partial; + legacyHome?: DetectionSnapshot["legacyHome"]; + storage?: DetectionSnapshot["storage"]; config?: Partial; desktop?: Partial; android?: Partial; @@ -18,11 +20,13 @@ function snapshot( ): DetectionSnapshot { return { picklabHome: { - path: "/home/u/.picklab", + path: "/home/u/.pickforge/picklab", exists: true, writable: true, ...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", @@ -94,6 +98,51 @@ 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("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/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..72ab74e 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; /** @@ -91,12 +111,47 @@ export async function readConfigFile(filePath: string): Promise { } } +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 { + // `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 = await readConfigFile(globalConfigPath(env)); - const project = await readConfigFile(projectConfigPath(projectDir)); + const { global, project } = await loadConfigLayers(projectDir, env); return deepMerge( deepMerge(deepMerge({}, resolvedDefaults), global), project, diff --git a/packages/core/src/evidence.ts b/packages/core/src/evidence.ts index e6b9b15..76a56d9 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( @@ -1635,18 +1672,29 @@ 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, opts: PruneEvidenceOptions = {}, + env: EnvLike = process.env, ): Promise { const keep = opts.keep ?? EVIDENCE_RETENTION_KEEP; - const catalog = await openRunCatalog(projectDir); - const entries = await catalog.list(); + const resolved = await resolveRunStorage(projectDir, env); + const primaryRootDir = path.resolve(resolved.runsDir); + const catalog = await openRunCatalog(projectDir, env); + 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/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..b3c5714 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,27 @@ 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(() => {}); } } + // 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(sessionPath(id, env), { force: true }); + if (legacyPath !== undefined) { + await fs.promises.rm(legacyPath, { force: true }); + } } diff --git a/packages/core/src/storage.ts b/packages/core/src/storage.ts new file mode 100644 index 0000000..fc041ab --- /dev/null +++ b/packages/core/src/storage.ts @@ -0,0 +1,209 @@ +import crypto from "node:crypto"; +import fs from "node:fs"; +import path from "node:path"; +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"; + +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; + /** + * 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 { + const value = env.PICKLAB_STORAGE_MODE; + if (value === "home" || value === "project-local" || value === "custom") { + return value; + } + 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 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 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. + */ +export async function resolveRunStorage( + projectDir: string, + env: EnvLike = process.env, +): Promise { + 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") { + const resolved: ResolvedRunStorage = { mode, runsDir: runsDir(projectDir) }; + if (rejectedProjectCustom !== undefined) { + resolved.rejectedProjectCustom = rejectedProjectCustom; + } + return resolved; + } + + if (mode === "custom") { + const resolvedCustomRoot = validateCustomPath(customPath); + const resolvedProjectDir = path.resolve(projectDir); + if (isSameOrDescendant(resolvedProjectDir, resolvedCustomRoot)) { + throw new StorageConfigError( + `storage path must be outside the project directory, got "${customPath}" ` + + `under "${resolvedProjectDir}"`, + ); + } + const resolved: ResolvedRunStorage = { + mode, + runsDir: path.join(resolvedCustomRoot, "runs"), + }; + if (rejectedProjectCustom !== undefined) { + resolved.rejectedProjectCustom = rejectedProjectCustom; + } + return resolved; + } + + const id = await projectId(projectDir); + 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/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..53190fe 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"); @@ -1321,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", () => { @@ -1407,7 +1473,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..3eca317 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,103 @@ 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); + }); + + 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(); + }); +}); diff --git a/packages/core/test/storage.test.ts b/packages/core/test/storage.test.ts new file mode 100644 index 0000000..465ac78 --- /dev/null +++ b/packages/core/test/storage.test.ts @@ -0,0 +1,370 @@ +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 { saveGlobalConfig, 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 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 saveGlobalConfig({ storage: { mode: "custom", path: custom } }, { + PICKLAB_HOME: home, + }); + + 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")); + }); + + 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("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", () => { + 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();