diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index c9aa6a293..667e12a24 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -162,6 +162,12 @@ jobs: permissions: contents: read pull-requests: write + # The comment upsert below calls the REST `/issues/{n}/comments` endpoints + # (gh api ... issues/comments). With GITHUB_TOKEN those are gated by the + # `issues` permission, not `pull-requests` — without it the GET returns 401 + # on every PR that produces eval artifacts (PRs with no artifacts exit + # early and never hit it, which is why this stayed hidden). See #1802 CI fix. + issues: write steps: - uses: actions/checkout@v4 with: diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a2d5e3d4..121247026 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,74 @@ # Changelog +## [1.56.1.0] - 2026-06-03 + +## **`/sync-gbrain` can no longer delete your repo. Cleanup now refuses any directory it cannot prove it created.** + +A `/sync-gbrain` memory sync could recursively delete your entire working tree. A +crashed import left a checkpoint pointing at the repo root, the next sync +"resumed" into it, and the cleanup step `rm -rf`'d it, taking uncommitted and +untracked work with it. This release closes that path and fixes three more bugs +hiding in the same resume machinery: cleanup now deletes only directories it can +prove are gstack-minted staging dirs, the remote-http transcript dir is never +touched, an interrupted import actually keeps its checkpoint so the next run +resumes instead of restaging, and a resumed run no longer marks files that failed +to import as successfully ingested. + +### The numbers that matter + +Source: `bun test test/regression-1611-gbrain-sync-resume.test.ts` on this branch. + +| Metric | Before | After | Δ | +|--------|--------|-------|---| +| Repo-root `rm -rf` reachable | yes | no | closed | +| Proof required before delete | none | 5 checks | realpath + direct-child + name + .git tripwire + minted-marker | +| Resume after a timed-out import | broken (dir deleted) | works | fixed | +| Failed files mislabeled "ingested" on resume | yes | no | fixed | +| Resume regression-test assertions | 9 | 64 | +55 | + +The guard is fail-closed: anything it cannot prove it owns is left on disk (a few +seconds of re-staging next run) rather than deleted. That asymmetry is the design +- a missing marker can cost a little work, never your data. + +### What this means for you + +If you use `/sync-gbrain`, a crashed or timed-out import can no longer cost you +uncommitted work. Resume now does what it always claimed: a large sync that times +out picks up where it left off next run instead of starting over, and files that +failed to import get retried instead of silently skipped. Nothing to configure. +Upgrade and keep syncing. + +### Itemized changes + +#### Fixed +- **`/sync-gbrain` could `rm -rf` your repo root.** A poisoned resume checkpoint + (dir = the repo, written when an import was interrupted while the repo was the + working directory) was adopted as the staging dir and recursively deleted. A + single fail-closed ownership check now guards every staging delete and every + resume: a path must resolve cleanly, be a direct child of `~/.gstack` named + `.staging-ingest-*`, contain no `.git`, and carry a marker file gstack minted. + Anything else is refused. Contributed by @diazMelgarejo (cyre). +- **Remote-http syncs no longer churn (or scare you).** The persistent transcript + dir that the brain sync pushes is no longer routed through staging cleanup, so + it stops being deleted on every run and stops emitting a false "preventing data + loss" warning. +- **A timed-out import now actually resumes.** Previously the run said "checkpoint + preserved" but then deleted the staging dir, so the next run always restaged. + The staging dir is now kept when a checkpoint points at it, and the message is + honest when there is nothing to resume. +- **Resume no longer hides import failures.** A resumed run could mark files that + failed to import as ingested, so they were never retried. Failures now map back + to their source files on resume and get another pass. + +#### For contributors +- New `lib/staging-guard.ts` exports `checkOwnedStagingDir()`, the single + fail-closed predicate shared by the deletion chokepoint and the resume gate. It + returns the realpath-resolved canonical path so callers delete exactly what they + validated (closes a symlink TOCTOU). `makeStagingDir()` tears down and rethrows + if its marker write fails, so a marker-less dir can never leak. The + `#1611` resume regression suite grew to 64 assertions covering the poison + matrix, the remote-http gate, timeout-preserve, and resume failure-mapping. + ## [1.56.0.0] - 2026-06-03 ## **Five heavy skills now load their bulk on demand, the shared question preamble slimmed corpus-wide, and a paranoid test suite proves the questions never got worse.** diff --git a/VERSION b/VERSION index 1d97cf7aa..c14186016 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.56.0.0 +1.56.1.0 diff --git a/bin/gstack-gbrain-sync.ts b/bin/gstack-gbrain-sync.ts index d88fc51a4..10c1f215b 100644 --- a/bin/gstack-gbrain-sync.ts +++ b/bin/gstack-gbrain-sync.ts @@ -41,6 +41,7 @@ import { ensureSourceRegistered, sourcePageCount, parseSourcesList } from "../li import { detectAutopilot, decideSourceRemove, decideCodeSync } from "../lib/gbrain-guards"; import { localEngineStatus, type LocalEngineStatus } from "../lib/gbrain-local-status"; import { buildGbrainEnv, spawnGbrain, execGbrainJson, NEEDS_SHELL_ON_WINDOWS } from "../lib/gbrain-exec"; +import { checkOwnedStagingDir } from "../lib/staging-guard"; // ── Types ────────────────────────────────────────────────────────────────── @@ -160,7 +161,7 @@ export function readGbrainCheckpoint(): GbrainCheckpoint | null { export type ResumeVerdict = | { kind: "no-checkpoint" } | { kind: "resume"; stagingDir: string; processedIndex: number; totalFiles: number } - | { kind: "stale-staging-missing"; stagingDir: string }; + | { kind: "stale-staging-missing"; stagingDir: string; reason?: string }; /** * Decide whether the next memory-ingest run should resume from gbrain's @@ -169,20 +170,20 @@ export type ResumeVerdict = * - checkpoint + staging ok → resume (gbrain picks up at processedIndex+1) * - checkpoint + staging gone → warn, fall through to fresh restage */ -export function decideResume(): ResumeVerdict { +export function decideResume(gstackHome: string = GSTACK_HOME): ResumeVerdict { const cp = readGbrainCheckpoint(); if (!cp || !cp.dir) return { kind: "no-checkpoint" }; const stagingDir = cp.dir; - if (!existsSync(stagingDir)) { - return { kind: "stale-staging-missing", stagingDir }; - } - // Treat "non-empty" as the safe-to-resume signal. statSync on a missing - // file throws; we already handled missing above so this is dir-level shape. - try { - const st = statSync(stagingDir); - if (!st.isDirectory()) return { kind: "stale-staging-missing", stagingDir }; - } catch { - return { kind: "stale-staging-missing", stagingDir }; + // #1802: only resume into a path we can PROVE is a gstack-minted staging dir. + // A poisoned checkpoint (dir = repo root, written when an autopilot import was + // SIGTERM'd while CWD was the repo) would otherwise be adopted as the staging + // dir and later recursively deleted by cleanupStagingDir(). Fail-closed: any + // unprovable path restages from scratch (cost: one re-stage; never data loss). + // Pure decision: return the verdict (with reason) and let the caller log, + // so we don't double-log the same event from here and the call site. + const verdict = checkOwnedStagingDir(stagingDir, gstackHome); + if (!verdict.ok) { + return { kind: "stale-staging-missing", stagingDir, reason: verdict.reason }; } return { kind: "resume", @@ -953,8 +954,15 @@ function runMemoryIngest(args: CliArgs): StageResult { ); childEnv.GSTACK_INGEST_RESUME_DIR = resume.stagingDir; } else if (resume.kind === "stale-staging-missing") { + // The reason distinguishes "actually gone" (disk cleanup / reboot) from + // "refused as unowned" (#1802 poison: the path may still exist on disk). + // Logging "gone" for a refused poison path misdirects incident diagnosis. + const why = resume.reason + ? `staging dir not usable: ${resume.reason}` + : `staging dir ${resume.stagingDir} gone`; console.error( - `[sync:memory] previous checkpoint stale (staging dir ${resume.stagingDir} gone), restaging from scratch`, + `[sync:memory] previous checkpoint stale (${why}), restaging from scratch. ` + + `Remove ~/.gbrain/import-checkpoint.json to silence.`, ); } diff --git a/bin/gstack-memory-ingest.ts b/bin/gstack-memory-ingest.ts index 98907eeee..653d4069a 100644 --- a/bin/gstack-memory-ingest.ts +++ b/bin/gstack-memory-ingest.ts @@ -65,6 +65,7 @@ import { withErrorContext, } from "../lib/gstack-memory-helpers"; import { execGbrainText, spawnGbrainAsync } from "../lib/gbrain-exec"; +import { checkOwnedStagingDir, STAGING_MARKER } from "../lib/staging-guard"; // ── Types ────────────────────────────────────────────────────────────────── @@ -907,13 +908,23 @@ interface StagingResult { * Filename = `${slug}.md`. mkdir is recursive. Existing files overwrite. * Errors per-file are collected; the whole batch is best-effort. */ +/** + * Staging-relative path for a prepared page's slug. Single source of truth so + * writeStaged() (which mints the map) and the resume-path reconstruction (#1802 + * C4) compute identical keys — if they diverge, readNewFailures() silently stops + * mapping gbrain's failures back to sources and failed files get marked ingested. + */ +export function stagedRelPath(slug: string): string { + return `${slug}.md`; +} + function writeStaged(prepared: PreparedPage[], stagingDir: string): StagingResult { mkdirSync(stagingDir, { recursive: true }); const stagedPathToSource = new Map(); const errors: Array<{ slug: string; error: string }> = []; let written = 0; for (const p of prepared) { - const relPath = `${p.slug}.md`; + const relPath = stagedRelPath(p.slug); const absPath = join(stagingDir, relPath); try { mkdirSync(dirname(absPath), { recursive: true }); @@ -978,7 +989,7 @@ function parseImportJson(stdout: string): ImportJsonResult | null { * staging-dir-relative filename gbrain saw (e.g. "transcripts/foo.md"). * stagedPathToSource maps that back to the original source file. */ -function readNewFailures( +export function readNewFailures( syncFailuresPath: string, preImportOffset: number, stagedPathToSource: Map, @@ -1198,6 +1209,17 @@ function preparePages( function makeStagingDir(): string { const dir = join(GSTACK_HOME, `.staging-ingest-${process.pid}-${Date.now()}`); mkdirSync(dir, { recursive: true }); + // Mint the ownership marker (#1802) so cleanupStagingDir() and decideResume() + // can prove this dir was created by us before any recursive delete or resume. + // #1802 C5: fail hard if the marker can't be written — a marker-less dir would + // be refused by the guard forever (leaked, never cleaned). Tear down the + // partial dir and rethrow so the caller fails loudly instead of leaking. + try { + writeFileSync(join(dir, STAGING_MARKER), `${process.pid}\n${Date.now()}\n`, "utf-8"); + } catch (err) { + try { rmSync(dir, { recursive: true, force: true }); } catch { /* best-effort */ } + throw err; + } return dir; } @@ -1259,8 +1281,21 @@ function isRemoteHttpMcpMode(): boolean { * cleanup failure. */ function cleanupStagingDir(dir: string): void { + // #1802 deletion chokepoint: never recurse-delete a path we cannot PROVE we + // own. A poisoned resume could otherwise route the repo root here. + const verdict = checkOwnedStagingDir(dir, GSTACK_HOME); + if (!verdict.ok) { + console.error( + `[gbrain] staging cleanup REFUSED: "${dir}" is not an owned staging dir ` + + `(${verdict.reason}). Skipping rm -rf to prevent data loss (#1802).`, + ); + return; + } try { - rmSync(dir, { recursive: true, force: true }); + // #1802 C5: delete the realpath-resolved dir the guard validated, not the + // raw input — closes the TOCTOU gap where `dir` is a symlink swapped between + // the check above and this rmSync. canonicalPath is always set when ok. + rmSync(verdict.canonicalPath ?? dir, { recursive: true, force: true }); } catch { // best-effort } @@ -1515,10 +1550,20 @@ async function ingestPass(args: CliArgs): Promise { // tells it where to resume. const remoteHttpMode = isRemoteHttpMcpMode(); const resumeDir = process.env.GSTACK_INGEST_RESUME_DIR; + // #1802 second entry point: this binary is runnable directly, so it must not + // trust GSTACK_INGEST_RESUME_DIR just because it exists — a stale/poisoned env + // could make us `gbrain import` (and later clean up) an arbitrary directory. + // Prove ownership here too, independently of the orchestrator's decideResume. const resuming = !remoteHttpMode && typeof resumeDir === "string" && resumeDir.length > 0 - && existsSync(resumeDir); + && existsSync(resumeDir) + && checkOwnedStagingDir(resumeDir, GSTACK_HOME).ok; + if (!remoteHttpMode && resumeDir && resumeDir.length > 0 && !resuming) { + console.error( + `[memory-ingest] ignoring GSTACK_INGEST_RESUME_DIR="${resumeDir}" — not a proven staging dir (#1802); staging fresh.`, + ); + } const stagingDir = resuming ? resumeDir! : remoteHttpMode @@ -1531,6 +1576,11 @@ async function ingestPass(args: CliArgs): Promise { if (!remoteHttpMode) { _activeStagingDir = stagingDir; } + // #1802 C3: set when the import-timeout branch leaves a resumable checkpoint + // pointing at this staging dir, so the finally preserves it for the next run + // instead of deleting it (the SIGTERM forwarder's preserve branch only runs + // when the PARENT is signalled, which an internal timeout never does). + let preserveStaging = false; try { let staging: StagingResult; if (resuming) { @@ -1543,7 +1593,15 @@ async function ingestPass(args: CliArgs): Promise { `[memory-ingest] resuming previous staging dir ${stagingDir} (skipping prepare phase)`, ); } - staging = { staging_dir: stagingDir, written: prep.prepared.length, errors: [], stagedPathToSource: new Map() }; + // #1802 C4: reconstruct stagedPathToSource from the prepared pages so + // readNewFailures() can still map gbrain's per-file failures back to + // sources on resume. An empty map made every failed file fall through to + // state-recording — i.e. silently marked ingested despite failing. + const stagedPathToSource = new Map(); + for (const p of prep.prepared) { + stagedPathToSource.set(stagedRelPath(p.slug), p.source_path); + } + staging = { staging_dir: stagingDir, written: prep.prepared.length, errors: [], stagedPathToSource }; } else { staging = writeStaged(prep.prepared, stagingDir); } @@ -1639,14 +1697,23 @@ async function ingestPass(args: CliArgs): Promise { const importJson = parseImportJson(stdout); if (importResult.status !== 0) { - // #1611: on timeout, gbrain's import-checkpoint.json is preserved (the - // SIGTERM forwarder keeps the staging dir), so the next /sync-gbrain - // resumes rather than restarting. Tell the user instead of looking failed. + // #1611/#1802 C3: on timeout, gbrain may have written + // import-checkpoint.json so the next /sync-gbrain can resume. But an + // INTERNAL timeout (runGbrainImport kills the child and returns here) + // never signals the parent, so the SIGTERM forwarder's preserve branch + // doesn't run — and the finally would otherwise delete the staging dir + // despite a "checkpoint preserved" message. Mirror the forwarder: preserve + // only when gbrain actually checkpointed against this dir; otherwise let + // the finally clean up (nothing to resume) and say so honestly. if (importResult.timedOut) { const mins = Math.round(resolveImportTimeoutMs() / 60000); - const msg = - `gbrain import timed out after ${mins}min; checkpoint preserved — re-run ` + - `/sync-gbrain to resume (raise GSTACK_INGEST_TIMEOUT_MS for big brains)`; + const checkpointed = stagingDirIsCheckpointed(stagingDir); + const msg = checkpointed + ? `gbrain import timed out after ${mins}min; checkpoint preserved — re-run ` + + `/sync-gbrain to resume (raise GSTACK_INGEST_TIMEOUT_MS for big brains)` + : `gbrain import timed out after ${mins}min before writing a checkpoint; ` + + `re-run /sync-gbrain to restage (raise GSTACK_INGEST_TIMEOUT_MS for big brains)`; + if (checkpointed) preserveStaging = true; console.error(`[memory-ingest] ${msg}`); return { written: 0, @@ -1754,7 +1821,15 @@ async function ingestPass(args: CliArgs): Promise { ); } } finally { - cleanupStagingDir(stagingDir); + // #1802 D1: in remote-http mode `stagingDir` is the PERSISTENT transcript + // dir (makePersistentTranscriptDir, under ~/.gstack/transcripts/) that + // gstack-brain-sync push must pick up — it is NOT a `.staging-ingest-*` dir + // and must never be deleted here. The remote-http branch above already + // documents this intent ("Skip the ... cleanupStagingDir paths"), but a + // `finally` runs on its `return`, so the gate has to live here. Gating on + // mode (rather than widening the ownership guard) keeps checkOwnedStagingDir + // strict: it only ever sees `.staging-ingest-*` dirs. + if (!remoteHttpMode && !preserveStaging) cleanupStagingDir(stagingDir); _activeStagingDir = null; } diff --git a/lib/staging-guard.ts b/lib/staging-guard.ts new file mode 100644 index 000000000..998ae0f33 --- /dev/null +++ b/lib/staging-guard.ts @@ -0,0 +1,116 @@ +/** + * staging-guard — fail-closed ownership proof for gstack ingest staging dirs. + * + * Fixes #1802. The /sync-gbrain memory stage stages prepared pages to a + * throwaway dir under ~/.gstack and `rm -rf`s it when done. The resume path + * (#1611) reused gbrain's `import-checkpoint.json` `dir` field as that staging + * dir WITHOUT proving it was one. A poisoned checkpoint — `dir` = the repo + * root, written when an autopilot `gbrain import` was SIGTERM'd while CWD was + * the repo — was then adopted as the staging dir and recursively deleted, + * destroying the user's working tree. + * + * Root cause is a TRUST failure, not path math: code deleted a path it never + * proved it owned. This module is the single definition of "a path gstack is + * allowed to recurse-delete or resume into", shared by the resume gate + * (decideResume) and the deletion chokepoint (cleanupStagingDir). + * + * Ownership requires ALL of the following (fail-closed — any failure ⇒ refuse): + * 1. Resolvable — realpathSync succeeds (resolves symlinks and `..` to a + * real location before any structural reasoning). + * 2. Structural — canonical path is a DIRECT child of $GSTACK_HOME named + * `.staging-ingest-*` (makeStagingDir's contract). + * 3. Not a repo — no `.git` entry inside. A screaming last-line tripwire: + * even a logic error elsewhere can never recurse-delete a + * git working tree. + * 4. Minted by us — a `.gstack-staging` marker file (written by + * makeStagingDir) is present. Turns "looks like ours" + * into "was created by us this lineage". + * + * Design note (steelman, 2026-06-02): a 4-model review panel split 3-1 on the + * marker. The dissent argued the structural check alone is sufficient and the + * marker adds a missing-token failure mode. Adopted anyway because that failure + * mode is fail-SAFE: a missing marker only forces an unnecessary re-stage + * (seconds), never a wrong deletion. The asymmetry — the marker can cost work + * but never data — settles it. The structural check still runs first and cheap. + * + * The deeper, "inevitable" fix lives upstream in gbrain: checkpoint.dir should + * always be a gbrain-minted staging dir, never CWD. This guard is the + * mitigation at gstack's own rm -rf boundary; see the companion gbrain issue. + */ +import { realpathSync, existsSync, statSync, lstatSync } from "fs"; +import { join, dirname, basename } from "path"; + +/** Basename prefix every makeStagingDir() directory carries. */ +export const STAGING_PREFIX = ".staging-ingest-"; + +/** Marker file minted inside each staging dir at creation. */ +export const STAGING_MARKER = ".gstack-staging"; + +export interface StagingVerdict { + ok: boolean; + /** Precise rejection reason, for actionable logging. Undefined when ok. */ + reason?: string; + /** + * The realpath-resolved directory the verdict actually validated. Present only + * when ok. Callers that delete MUST `rmSync` this path, not the raw input — + * deleting the canonical path closes the TOCTOU gap where the input is a + * symlink swapped between this check and the delete (#1802 C5). + */ + canonicalPath?: string; +} + +/** + * Prove (fail-closed) that `dir` is a gstack-owned ingest staging directory + * that is safe to recurse-delete or resume into. Returns a structured verdict + * so callers can log exactly why a path was rejected. + * + * @param dir Candidate path (e.g. gbrain checkpoint.dir, or the active staging dir). + * @param gstackHome Resolved $GSTACK_HOME (injected for testability). + */ +export function checkOwnedStagingDir(dir: string, gstackHome: string): StagingVerdict { + if (!dir || typeof dir !== "string") { + return { ok: false, reason: "empty or non-string path" }; + } + let canon: string; + let home: string; + try { + canon = realpathSync(dir); + home = realpathSync(gstackHome); + } catch { + // Missing path or broken symlink ⇒ cannot prove ownership ⇒ refuse. + return { ok: false, reason: "unresolvable path (missing dir or broken symlink)" }; + } + // The target itself must be a directory (not a file/socket/etc named like one). + try { + if (!statSync(canon).isDirectory()) { + return { ok: false, reason: "not a directory" }; + } + } catch { + return { ok: false, reason: "unstattable target" }; + } + if (dirname(canon) !== home) { + return { ok: false, reason: `not a direct child of GSTACK_HOME (${home})` }; + } + if (!basename(canon).startsWith(STAGING_PREFIX)) { + return { ok: false, reason: `basename does not start with "${STAGING_PREFIX}"` }; + } + if (existsSync(join(canon, ".git"))) { + // Tripwire: never recurse-delete anything that looks like a git work tree. + return { ok: false, reason: "path contains .git — refusing to touch a git working tree" }; + } + // Marker must be a REGULAR FILE we minted — not a directory or symlink that + // merely shares the name (lstat, not stat, so a symlink can't impersonate it). + try { + if (!lstatSync(join(canon, STAGING_MARKER)).isFile()) { + return { ok: false, reason: `"${STAGING_MARKER}" exists but is not a regular file` }; + } + } catch { + return { ok: false, reason: `missing "${STAGING_MARKER}" marker — not minted by makeStagingDir` }; + } + return { ok: true, canonicalPath: canon }; +} + +/** Boolean convenience wrapper around {@link checkOwnedStagingDir}. */ +export function isOwnedStagingDir(dir: string, gstackHome: string): boolean { + return checkOwnedStagingDir(dir, gstackHome).ok; +} diff --git a/package.json b/package.json index ea039c88c..c4745de42 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.56.0.0", + "version": "1.56.1.0", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/test/regression-1611-gbrain-sync-resume.test.ts b/test/regression-1611-gbrain-sync-resume.test.ts index c1da8bb8c..1f4a06a89 100644 --- a/test/regression-1611-gbrain-sync-resume.test.ts +++ b/test/regression-1611-gbrain-sync-resume.test.ts @@ -35,6 +35,8 @@ import { readGbrainCheckpoint, decideResume, } from "../bin/gstack-gbrain-sync"; +import { checkOwnedStagingDir, STAGING_MARKER } from "../lib/staging-guard"; +import { stagedRelPath, readNewFailures } from "../bin/gstack-memory-ingest"; const ROOT = path.resolve(import.meta.dir, ".."); const DEFAULT_MS = 35 * 60 * 1000; @@ -132,9 +134,11 @@ describe("#1611 decideResume — checkpoint + staging detection", () => { expect(decideResume().kind).toBe("no-checkpoint"); }); - test("checkpoint + staging dir exists → resume verdict", () => { + test("checkpoint + minted staging dir exists → resume verdict", () => { fs.mkdirSync(stagingDir, { recursive: true }); fs.writeFileSync(stagingDir + "/page1.md", "content", "utf-8"); + // #1802: a real staging dir carries the ownership marker minted by makeStagingDir. + fs.writeFileSync(path.join(stagingDir, STAGING_MARKER), "99\n99\n", "utf-8"); fs.writeFileSync(cpPath, JSON.stringify({ dir: stagingDir, totalFiles: 1989, @@ -143,7 +147,8 @@ describe("#1611 decideResume — checkpoint + staging detection", () => { timestamp: "2026-05-19T19:30:05.008Z", }), "utf-8"); - const v = decideResume(); + // gstackHome is injected so the ownership check anchors on the test home. + const v = decideResume(tmpHome); expect(v.kind).toBe("resume"); if (v.kind === "resume") { expect(v.stagingDir).toBe(stagingDir); @@ -160,13 +165,41 @@ describe("#1611 decideResume — checkpoint + staging detection", () => { processedIndex: 1000, }), "utf-8"); - const v = decideResume(); + const v = decideResume(tmpHome); expect(v.kind).toBe("stale-staging-missing"); if (v.kind === "stale-staging-missing") { expect(v.stagingDir).toBe(stagingDir); } }); + // ── #1802 regression: poisoned checkpoint must never be adopted/deleted ──── + + test("#1802 checkpoint.dir = repo root with .git → stale-staging-missing (not resumed)", () => { + // Reproduces the exact poison: an interrupted import wrote checkpoint.dir = + // the repo working tree. It exists and is a directory, so the pre-#1802 + // code resumed (and cleanup later rm -rf'd it). It must now be refused. + const repoRoot = path.join(tmpHome, "my-repo"); + fs.mkdirSync(path.join(repoRoot, ".git"), { recursive: true }); + fs.writeFileSync(path.join(repoRoot, "important.py"), "# real work\n", "utf-8"); + fs.writeFileSync(cpPath, JSON.stringify({ dir: repoRoot, totalFiles: 10, processedIndex: 3 }), "utf-8"); + + const v = decideResume(tmpHome); + expect(v.kind).toBe("stale-staging-missing"); + // decideResume never deletes, but prove the repo is untouched by the verdict. + expect(fs.existsSync(path.join(repoRoot, "important.py"))).toBe(true); + }); + + test("#1802 staging-named dir WITHOUT marker → stale-staging-missing (not minted by us)", () => { + fs.mkdirSync(stagingDir, { recursive: true }); // .staging-ingest-99-99, but no marker + fs.writeFileSync(cpPath, JSON.stringify({ dir: stagingDir, totalFiles: 1, processedIndex: 0 }), "utf-8"); + expect(decideResume(tmpHome).kind).toBe("stale-staging-missing"); + }); + + test("#1802 checkpoint.dir = '/' → stale-staging-missing", () => { + fs.writeFileSync(cpPath, JSON.stringify({ dir: "/", totalFiles: 1, processedIndex: 0 }), "utf-8"); + expect(decideResume(tmpHome).kind).toBe("stale-staging-missing"); + }); + test("checkpoint with no dir field → no-checkpoint verdict", () => { fs.writeFileSync(cpPath, JSON.stringify({ totalFiles: 1989, @@ -222,6 +255,233 @@ describe("#1611 SIGTERM staging preservation — static invariants", () => { ); expect(body).toMatch(/GSTACK_INGEST_RESUME_DIR/); expect(body).toMatch(/resuming from gbrain checkpoint/); - expect(body).toMatch(/previous checkpoint stale.*staging dir.*gone.*restaging from scratch/); + expect(body).toMatch(/previous checkpoint stale/); + expect(body).toMatch(/restaging from scratch/); + // #1802: the caller distinguishes "refused as unowned" from "actually gone". + expect(body).toMatch(/staging dir not usable/); + }); +}); + +// ── #1802 checkOwnedStagingDir — fail-closed ownership matrix ─────────────── +// The single predicate guarding both the resume gate (decideResume) and the +// deletion chokepoint (cleanupStagingDir). Every branch is fail-closed: any +// case it cannot prove is owned must return ok:false. +describe("#1802 checkOwnedStagingDir — ownership matrix", () => { + let home: string; + + beforeEach(() => { + home = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-1802-")); + }); + afterEach(() => { + try { fs.rmSync(home, { recursive: true, force: true }); } catch { /* best-effort */ } + }); + + function mintStaging(name = ".staging-ingest-1-1"): string { + const d = path.join(home, name); + fs.mkdirSync(d, { recursive: true }); + fs.writeFileSync(path.join(d, STAGING_MARKER), "1\n1\n", "utf-8"); + return d; + } + + test("minted staging dir → ok", () => { + expect(checkOwnedStagingDir(mintStaging(), home).ok).toBe(true); + }); + + test("#1802 C5: ok verdict carries the realpath-resolved canonicalPath", () => { + const d = mintStaging(); + const v = checkOwnedStagingDir(d, home); + expect(v.ok).toBe(true); + // Callers must delete this (not the raw input) to close the symlink TOCTOU. + expect(v.canonicalPath).toBe(fs.realpathSync(d)); + }); + + test("repo root (direct child, has .git, no marker) → refused", () => { + const repo = path.join(home, "my-repo"); + fs.mkdirSync(path.join(repo, ".git"), { recursive: true }); + expect(checkOwnedStagingDir(repo, home).ok).toBe(false); + }); + + test("staging-named dir containing .git → refused by tripwire even with marker", () => { + const d = mintStaging(".staging-ingest-9-9"); + fs.mkdirSync(path.join(d, ".git"), { recursive: true }); + const v = checkOwnedStagingDir(d, home); + expect(v.ok).toBe(false); + expect(v.reason).toMatch(/\.git/); + }); + + test("staging-named dir without marker → refused (not minted)", () => { + const d = path.join(home, ".staging-ingest-2-2"); + fs.mkdirSync(d, { recursive: true }); + expect(checkOwnedStagingDir(d, home).ok).toBe(false); + }); + + test("right name but NOT a direct child of home → refused", () => { + const nested = path.join(home, "sub", ".staging-ingest-3-3"); + fs.mkdirSync(nested, { recursive: true }); + fs.writeFileSync(path.join(nested, STAGING_MARKER), "x", "utf-8"); + expect(checkOwnedStagingDir(nested, home).ok).toBe(false); + }); + + test("direct child of home but wrong name → refused", () => { + const d = path.join(home, "notstaging"); + fs.mkdirSync(d, { recursive: true }); + fs.writeFileSync(path.join(d, STAGING_MARKER), "x", "utf-8"); + expect(checkOwnedStagingDir(d, home).ok).toBe(false); + }); + + test("missing path → refused (unresolvable)", () => { + expect(checkOwnedStagingDir(path.join(home, ".staging-ingest-gone"), home).ok).toBe(false); + }); + + test("'/' and '' → refused", () => { + expect(checkOwnedStagingDir("/", home).ok).toBe(false); + expect(checkOwnedStagingDir("", home).ok).toBe(false); + }); + + test("symlink whose target escapes home → refused (realpath resolves first)", () => { + const outside = path.join(home, "..", path.basename(home) + "-outside"); + fs.mkdirSync(outside, { recursive: true }); + const link = path.join(home, ".staging-ingest-link"); + fs.symlinkSync(outside, link); + try { + // realpathSync resolves the link to `outside`, whose parent is not `home`. + expect(checkOwnedStagingDir(link, home).ok).toBe(false); + } finally { + try { fs.rmSync(outside, { recursive: true, force: true }); } catch { /* best-effort */ } + } + }); + + test("cleanupStagingDir + decideResume both call the guard (static invariant)", () => { + const ingest = fs.readFileSync(path.join(ROOT, "bin", "gstack-memory-ingest.ts"), "utf-8"); + const sync = fs.readFileSync(path.join(ROOT, "bin", "gstack-gbrain-sync.ts"), "utf-8"); + expect(ingest).toMatch(/checkOwnedStagingDir\(dir, GSTACK_HOME\)/); + expect(ingest).toMatch(/staging cleanup REFUSED/); + expect(sync).toMatch(/checkOwnedStagingDir\(stagingDir, gstackHome\)/); + }); +}); + +// ── #1802 D1: remote-http persistent dir must never hit cleanupStagingDir ─── +// In remote-http mode `stagingDir` is the PERSISTENT transcript dir +// (makePersistentTranscriptDir, under ~/.gstack/transcripts/) that +// gstack-brain-sync push consumes. The finally runs on the remote-http `return`, +// so the cleanup call there must be gated on `!remoteHttpMode` — otherwise the +// guard refuses it on every sync (false "prevent data loss" warning) and, pre- +// gate, the dir was deleted outright (broken artifacts handoff). +describe("#1802 D1 — remote-http finally gate (static invariant)", () => { + const ingest = fs.readFileSync( + path.join(ROOT, "bin", "gstack-memory-ingest.ts"), + "utf-8", + ); + + test("finally gates cleanupStagingDir on !remoteHttpMode", () => { + // Tolerates additional guards (e.g. C3's !preserveStaging) in the same + // condition — the load-bearing invariant is that remote-http never deletes. + expect(ingest).toMatch(/if \(!remoteHttpMode[^)]*\) cleanupStagingDir\(stagingDir\)/); + }); + + test("the only finally-scoped cleanup call is the gated one", () => { + // Locate the finally block and assert it does not contain a bare + // `cleanupStagingDir(stagingDir);` that would run regardless of mode. + const finallyAt = ingest.lastIndexOf("} finally {"); + expect(finallyAt).toBeGreaterThan(-1); + const finallySlice = ingest.slice(finallyAt, finallyAt + 800); + expect(finallySlice).not.toMatch(/^\s*cleanupStagingDir\(stagingDir\);/m); + }); +}); + +// ── #1802 C3: internal import-timeout must preserve a checkpointed staging dir ─ +// runGbrainImport kills only the child on an internal timeout; the parent +// returns normally, so the SIGTERM forwarder's preserve branch never runs. The +// timeout branch must mirror it (preserve when checkpointed) and the finally +// must honor that — otherwise "checkpoint preserved" is a lie and resume breaks. +describe("#1802 C3 — import-timeout preserve (static invariant)", () => { + const ingest = fs.readFileSync( + path.join(ROOT, "bin", "gstack-memory-ingest.ts"), + "utf-8", + ); + + test("timeout branch checks stagingDirIsCheckpointed and sets preserveStaging", () => { + const timeoutAt = ingest.indexOf("if (importResult.timedOut)"); + expect(timeoutAt).toBeGreaterThan(-1); + const slice = ingest.slice(timeoutAt, timeoutAt + 1200); + expect(slice).toMatch(/stagingDirIsCheckpointed\(stagingDir\)/); + expect(slice).toMatch(/preserveStaging = true/); + // The not-checkpointed path must say so honestly rather than promising resume. + expect(slice).toMatch(/before writing a checkpoint/); + }); + + test("finally honors preserveStaging", () => { + expect(ingest).toMatch( + /if \(!remoteHttpMode && !preserveStaging\) cleanupStagingDir\(stagingDir\)/, + ); + }); +}); + +// ── #1802 C5: hardening (static invariant) ───────────────────────────────── +describe("#1802 C5 — hardening (static invariant)", () => { + const ingest = fs.readFileSync( + path.join(ROOT, "bin", "gstack-memory-ingest.ts"), + "utf-8", + ); + + test("cleanupStagingDir deletes the canonical path, not the raw input", () => { + expect(ingest).toMatch(/rmSync\(verdict\.canonicalPath \?\? dir/); + }); + + test("makeStagingDir tears down + rethrows if the marker write fails", () => { + const at = ingest.indexOf("function makeStagingDir"); + expect(at).toBeGreaterThan(-1); + const slice = ingest.slice(at, at + 800); + expect(slice).toMatch(/catch \(err\)/); + expect(slice).toMatch(/rmSync\(dir, \{ recursive: true, force: true \}\)/); + expect(slice).toMatch(/throw err/); + }); +}); + +// ── #1802 C4: resume must not mark failed files as ingested ───────────────── +// readNewFailures() maps gbrain's per-file failures (keyed by staging-relative +// path) back to source paths so the caller can EXCLUDE them from state +// recording. On resume the map was rebuilt empty, so every failure was lost and +// the failed file was silently marked ingested. This proves the reconstructed +// map (built with stagedRelPath, the same key writeStaged uses) recovers it. +describe("#1802 C4 — resume failure mapping (behavioral)", () => { + let dir: string; + let cpHome: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-1802c4-")); + cpHome = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-1802c4-fail-")); + }); + afterEach(() => { + for (const d of [dir, cpHome]) { + try { fs.rmSync(d, { recursive: true, force: true }); } catch { /* best-effort */ } + } + }); + + test("stagedRelPath matches the writeStaged key format", () => { + expect(stagedRelPath("my-slug")).toBe("my-slug.md"); + expect(stagedRelPath("nested/slug")).toBe("nested/slug.md"); + }); + + test("reconstructed map maps the failure back to its source; empty map loses it", () => { + const failuresPath = path.join(cpHome, "sync-failures.jsonl"); + // gbrain records the failure keyed by the staging-relative path. + fs.writeFileSync( + failuresPath, + JSON.stringify({ path: stagedRelPath("doc-a"), error: "boom" }) + "\n", + "utf-8", + ); + + // The resume-path reconstruction: built from prepared pages via stagedRelPath. + const reconstructed = new Map([ + [stagedRelPath("doc-a"), "/src/doc-a.json"], + ]); + const recovered = readNewFailures(failuresPath, 0, reconstructed); + expect(recovered.has("/src/doc-a.json")).toBe(true); + + // The pre-fix bug: an empty map (what resume used) drops the failure, so the + // caller would state-record /src/doc-a.json as ingested. + const lost = readNewFailures(failuresPath, 0, new Map()); + expect(lost.size).toBe(0); }); });