diff --git a/TODOS.md b/TODOS.md index cd5404754..7f2ee5450 100644 --- a/TODOS.md +++ b/TODOS.md @@ -96,6 +96,24 @@ invisible to Context Recovery until migrated. **Effort:** M → S with CC. **Priority:** P2. **Depends on:** the v1.68 wave (shipped the fix + parity tests). +### P3: gstack-slug degraded-heal probe cost on cache hits (v1.68 review-army finding) + +**What:** The v1.68 cache self-heal probes `_resolve_remote` (1-3 git forks) on +EVERY cache hit whenever the cached slug equals the marker-root basename — the +permanent steady state for remoteless and legit-sticky projects, on the +per-preamble hot path. Add a single-shot sentinel per cache entry so the heal +probe runs once, not forever. + +**Why:** "Cache hits stay git-spawn-free" only holds for owner-repo slugs +today. Cost is bounded (1-3 forks) but paid at every skill start on affected +projects. Also next-touch notes from the same review: extract a makeResult +helper for BulkResult's 11 hand-copied literals in bin/gstack-memory-ingest.ts; +dedup the brain-worktree default-path literal between bin/gstack-brain-sync and +bin/gstack-gbrain-source-wireup. + +**Effort:** S. **Priority:** P3. **Depends on:** cache-format compatibility +(sentinel must not break older readers). + ### P2: v1.67 coverage-audit test-gap backlog (5-agent sweep, ranked) The wave's Step-7 coverage audit (5 subsystem agents, ~700 changed paths, diff --git a/bin/gstack-brain-sync b/bin/gstack-brain-sync index 52b757877..2f81554b6 100755 --- a/bin/gstack-brain-sync +++ b/bin/gstack-brain-sync @@ -196,6 +196,11 @@ migrate_legacy_queue() { rm -f "$migrating" 2>/dev/null || true fi fi + # If the leftover STILL holds records, the conversion failed (e.g. python3 + # unavailable). The mv below would overwrite it and destroy those records — + # exactly the never-destroy invariant above. Defer this run's migration; + # the next run retries both files. + [ -s "$migrating" ] && return 0 if [ -s "$QUEUE" ]; then mkdir -p "$QUEUE_DIR" 2>/dev/null || return 0 mv -f "$QUEUE" "$migrating" 2>/dev/null || return 0 @@ -482,6 +487,13 @@ subcmd_once() { # drain reads the spool (transition window for pre-spool writers). migrate_legacy_queue + # Janitor: reap orphaned enqueue temp files. A writer killed between its + # tmp write and the atomic rename leaves `.tmp-*` behind forever — it never + # becomes a record and nothing else touches it. One hour is far beyond any + # live writer's write→rename window, so a fresh tmp (an in-flight enqueue) + # is never touched. Runs inside the run lock, so it can't race the drain. + find "$QUEUE_DIR" -maxdepth 1 -type f -name '.tmp-*' -mmin +60 -delete 2>/dev/null || true + local mode mode=$("$CONFIG_BIN" get artifacts_sync_mode 2>/dev/null || echo off) @@ -691,11 +703,13 @@ subcmd_status() { echo '{"status":"unknown","message":"no status file yet"}' fi # Supplemental info (not in status file). Depth = spool record files plus - # any not-yet-migrated legacy queue lines (transition window). + # any not-yet-migrated legacy queue lines (transition window), including a + # crash-leftover .migrating file — its records are still pending too. local queue_depth spool_depth legacy_depth spool_depth=$(ls "$QUEUE_DIR"/*.json 2>/dev/null | wc -l | tr -d ' ') legacy_depth=0 [ -f "$QUEUE" ] && legacy_depth=$(wc -l < "$QUEUE" | tr -d ' ') + [ -f "$QUEUE.migrating" ] && legacy_depth=$(( legacy_depth + $(wc -l < "$QUEUE.migrating" | tr -d ' ') )) queue_depth=$(( spool_depth + legacy_depth )) local last_push="never" [ -f "$LAST_PUSH_FILE" ] && last_push=$(cat "$LAST_PUSH_FILE" 2>/dev/null || echo never) @@ -727,7 +741,10 @@ subcmd_drop_queue() { echo "Refusing: --drop-queue discards pending syncs. Pass --yes to confirm." >&2 exit 1 fi - # Remove spool record files, then truncate any legacy queue remnant. + # Remove spool record files, then truncate any legacy queue remnant — + # including a crash-leftover .migrating file, whose records would otherwise + # resurrect on the next drain via migrate_legacy_queue after the user + # explicitly discarded the queue. local n=0 f for f in "$QUEUE_DIR"/*.json; do [ -e "$f" ] || continue @@ -739,6 +756,12 @@ subcmd_drop_queue() { n=$(( n + legacy_n )) : > "$QUEUE" fi + if [ -f "$QUEUE.migrating" ]; then + local mig_n + mig_n=$(wc -l < "$QUEUE.migrating" | tr -d ' ') + n=$(( n + mig_n )) + rm -f "$QUEUE.migrating" 2>/dev/null || true + fi if [ "$n" -eq 0 ]; then echo "queue already empty" exit 0 diff --git a/bin/gstack-brain-uninstall b/bin/gstack-brain-uninstall index 72841aca9..a240a855e 100755 --- a/bin/gstack-brain-uninstall +++ b/bin/gstack-brain-uninstall @@ -23,6 +23,7 @@ # .brain-queue.jsonl — legacy pending queue (pre-spool) # .brain-discover-cursor — discover-new cursor # .brain-last-push — timestamp marker +# .brain-worktree-last-advance — daily worktree-advance stamp (#2516) # .brain-skip.txt — user-maintained skip list # .brain-sync.lock.d/ — lock dir (if present) # .brain-sync-status.json — health status @@ -125,6 +126,7 @@ rm -f "$GSTACK_HOME/.brain-queue.jsonl.migrating" 2>/dev/null || true rm -f "$GSTACK_HOME/.brain-discover-cursor" 2>/dev/null || true rm -f "$GSTACK_HOME/.brain-last-push" 2>/dev/null || true rm -f "$GSTACK_HOME/.brain-last-pull" 2>/dev/null || true +rm -f "$GSTACK_HOME/.brain-worktree-last-advance" 2>/dev/null || true rm -f "$GSTACK_HOME/.brain-skip.txt" 2>/dev/null || true rm -f "$GSTACK_HOME/.brain-sync-status.json" 2>/dev/null || true rm -rf "$GSTACK_HOME/.brain-sync.lock.d" 2>/dev/null || true diff --git a/bin/gstack-gbrain-repo-policy b/bin/gstack-gbrain-repo-policy index 5494632de..f1204b19e 100755 --- a/bin/gstack-gbrain-repo-policy +++ b/bin/gstack-gbrain-repo-policy @@ -92,14 +92,23 @@ normalize() { case "$head" in *:*) url=$(printf '%s' "$url" | sed 's|:|/|') ;; esac + # Lowercase BEFORE the suffix strips so a `.GIT` suffix still strips — + # parity with lib/gstack-memory-helpers' canonicalizeRemote, which strips + # `.git` case-insensitively. GitHub and most hosts are case-insensitive on + # paths anyway; collapsing avoids duplicate entries for "Foo/Bar" vs + # "foo/bar". (Parity is pinned by test/gbrain-repo-policy-client.test.ts: + # a key set through THIS normalize must be found via the canonicalized form + # memory-ingest passes to `get --batch`.) + url=$(printf '%s' "$url" | tr '[:upper:]' '[:lower:]') + # Strip trailing slash(es) FIRST, so ".git/" still loses its suffix (same + # order as canonicalizeRemote — slash-first, then .git, then re-strip). + while [ "${url%/}" != "$url" ]; do url="${url%/}"; done # Strip trailing .git url="${url%.git}" - # Strip trailing / - url="${url%/}" - # Lowercase the whole thing. GitHub and most hosts are case-insensitive on - # paths anyway; collapsing avoids duplicate entries for "Foo/Bar" vs - # "foo/bar". - printf '%s\n' "$url" | tr '[:upper:]' '[:lower:]' + # Re-strip trailing slash(es): a path remote ending in a `.git` directory + # component ("/repo/.git") exposes a new trailing slash once .git is gone. + while [ "${url%/}" != "$url" ]; do url="${url%/}"; done + printf '%s\n' "$url" } # ensure_file — create the policy file if missing, migrate if legacy. diff --git a/bin/gstack-memory-ingest.ts b/bin/gstack-memory-ingest.ts index 860e6ae25..4fdb2cdfa 100644 --- a/bin/gstack-memory-ingest.ts +++ b/bin/gstack-memory-ingest.ts @@ -1077,17 +1077,6 @@ export function readNewFailures( // ── Main ingest passes ───────────────────────────────────────────────────── -/** - * Lightweight attribution check: does a transcript have a resolvable git - * remote for its cwd? Extracts the cwd from the first JSONL line that has - * one (mirroring the logic in parseTranscriptJsonl) and calls - * resolveGitRemote. Avoids the full parse (body rendering, message counting) - * because probe only needs the yes/no answer. - * - * Non-transcript types (artifacts) always pass — the attribution filter in - * preparePages only applies to transcripts (#2394). - */ - /** * The ONE attribution gate (#2394): a transcript is attributable iff its cwd * resolves to a git remote. Both probeMode (via transcriptIsAttributable) and @@ -1099,32 +1088,75 @@ function sessionIsAttributable(cwd: string | undefined | null): boolean { return resolveGitRemote(cwd) !== ""; } +/** + * Bounded prefix for the probe's cheap-parse (plan C7): transcripts run to + * tens of MB, and the probe only needs the cwd, which both agent formats put + * on the FIRST records. 256KB is orders of magnitude past any real header. + */ +const TRANSCRIPT_PROBE_MAX_BYTES = 256 * 1024; + +/** + * Lightweight attribution check: does a transcript have a resolvable git + * remote for its cwd? Reads a BOUNDED prefix (first 256KB, never the whole + * file — plan C7: the probe must stay a cheap parse on multi-MB transcripts), + * extracts the cwd with EXACTLY parseTranscriptJsonl's rules, and calls + * resolveGitRemote. Avoids the full parse (body rendering, message counting) + * because probe only needs the yes/no answer. + * + * Extraction MIRRORS parseTranscriptJsonl (the single source of truth for + * cwd semantics — keep the two in lockstep): + * - the first PARSEABLE line decides the format (Codex: type=session_meta + * or payload.id; else Claude Code); + * - Codex cwd comes from that FIRST record ONLY (payload.cwd || cwd) — + * a cwd appearing only on a later record is NOT used, exactly as + * parseTranscriptJsonl ignores it, so probe and prepare can never + * diverge on the same file; + * - Claude Code cwd comes from the first record that carries one; + * - unparseable lines are skipped (the truncated-tail case included). + * + * Non-transcript types (artifacts) always pass — the attribution filter in + * preparePages only applies to transcripts (#2394). + */ function transcriptIsAttributable(path: string): boolean { let raw: string; try { - raw = readFileSync(path, "utf-8"); + const fd = openSync(path, "r"); + try { + const buf = Buffer.alloc(TRANSCRIPT_PROBE_MAX_BYTES); + const n = readSync(fd, buf, 0, buf.length, 0); + raw = buf.toString("utf-8", 0, n); + } finally { + closeSync(fd); + } } catch { return false; } const lines = raw.split("\n").filter((l) => l.trim().length > 0); if (lines.length === 0) return false; - // Detect format: Codex first line has type=session_meta, Claude Code - // has cwd on a user/assistant record. let cwd = ""; + let sawFirstParseable = false; for (const line of lines) { + let rec: any; try { - const rec = JSON.parse(line); - if (rec?.type === "session_meta") { + rec = JSON.parse(line); + } catch { + continue; // mirrors parseTranscriptJsonl: unparseable lines are skipped + } + if (!sawFirstParseable) { + sawFirstParseable = true; + // Format detection mirrors parseTranscriptJsonl's `first` record check. + const isCodex = rec?.type === "session_meta" || rec?.payload?.id != null; + if (isCodex) { + // Codex: cwd comes from the session_meta FIRST record only. cwd = rec.payload?.cwd || rec.cwd || ""; break; } - if (rec?.cwd) { - cwd = rec.cwd; - break; - } - } catch { - continue; + } + // Claude Code: first record with a cwd wins (the first record included). + if (rec?.cwd) { + cwd = rec.cwd; + break; } } if (!cwd) return false; diff --git a/bin/gstack-session-update b/bin/gstack-session-update index 77eca42c5..692104b7c 100755 --- a/bin/gstack-session-update +++ b/bin/gstack-session-update @@ -73,16 +73,23 @@ fi [ -f "$_hb" ] || _hb="$LOCK_DIR" [ -n "$(find "$_hb" -maxdepth 0 -mmin +$LOCK_TTL_MINUTES 2>/dev/null)" ] } + # Reclaim is TOCTOU-safe via atomic mv-aside: `rm -rf` then `mkdir` lets TWO + # contenders both judge the lock stale, both remove it, and both win the + # mkdir (one rm can land between the other's rm and mkdir). `mv` of the lock + # dir is atomic — exactly one contender's mv succeeds; the loser's mv fails + # (ENOENT) and it backs off. The winner reaps the moved-aside dir at leisure. if ! mkdir "$LOCK_DIR" 2>/dev/null; then if lock_is_expired; then - rm -rf "$LOCK_DIR" 2>/dev/null + mv "$LOCK_DIR" "$LOCK_DIR.reap.$$" 2>/dev/null || { log_entry "SKIP lock_contested"; exit 0; } + rm -rf "$LOCK_DIR.reap.$$" 2>/dev/null mkdir "$LOCK_DIR" 2>/dev/null || { log_entry "SKIP lock_contested"; exit 0; } log_entry "RECLAIMED lock_ttl_expired" elif [ -f "$LOCK_DIR/pid" ]; then LOCK_PID=$(cat "$LOCK_DIR/pid" 2>/dev/null || echo 0) if [ "$LOCK_PID" -gt 0 ] 2>/dev/null && ! kill -0 "$LOCK_PID" 2>/dev/null; then - # Stale lock — remove and re-acquire - rm -rf "$LOCK_DIR" 2>/dev/null + # Stale lock — mv aside atomically (see reclaim note above), re-acquire + mv "$LOCK_DIR" "$LOCK_DIR.reap.$$" 2>/dev/null || { log_entry "SKIP lock_contested"; exit 0; } + rm -rf "$LOCK_DIR.reap.$$" 2>/dev/null mkdir "$LOCK_DIR" 2>/dev/null || { log_entry "SKIP lock_contested"; exit 0; } else # Live holder — or an empty/non-numeric pidfile inside the TTL diff --git a/bin/gstack-settings-hook b/bin/gstack-settings-hook index 46d22533f..7b6d58200 100755 --- a/bin/gstack-settings-hook +++ b/bin/gstack-settings-hook @@ -83,7 +83,17 @@ case "$ACTION" in const settingsPath = process.env.GSTACK_SETTINGS_PATH; const hookCmd = process.env.GSTACK_HOOK_CMD; let settings = {}; - try { settings = JSON.parse(fs.readFileSync(settingsPath, "utf8")); } catch {} + // An EXISTING file that does not parse must never be rewritten: the + // old catch{} folded it to {} and the atomic write below replaced the + // user permissions/env/other hooks with just ours. Refuse loudly. + if (fs.existsSync(settingsPath)) { + try { settings = JSON.parse(fs.readFileSync(settingsPath, "utf8")); } + catch (e) { + console.error("error: " + settingsPath + " exists but is not valid JSON (" + + (e && e.message ? e.message : e) + "); refusing to rewrite it. Fix or move the file, then re-run."); + process.exit(1); + } + } if (!settings.hooks) settings.hooks = {}; if (!settings.hooks.SessionStart) settings.hooks.SessionStart = []; const exists = settings.hooks.SessionStart.some(entry => @@ -97,7 +107,7 @@ case "$ACTION" in const tmp = settingsPath + ".tmp"; fs.writeFileSync(tmp, JSON.stringify(settings, null, 2) + "\n"); fs.renameSync(tmp, settingsPath); - ' 2>/dev/null + ' ;; remove) @@ -178,18 +188,34 @@ case "$ACTION" in const ensure = process.env.GSTACK_ENSURE === "1"; let settings = {}; - try { settings = JSON.parse(fs.readFileSync(settingsPath, "utf8")); } catch {} + // An EXISTING file that does not parse must never be rewritten: the + // old catch{} folded it to {} and the atomic write below replaced the + // user permissions/env/other hooks with just ours. Refuse loudly. + if (fs.existsSync(settingsPath)) { + try { settings = JSON.parse(fs.readFileSync(settingsPath, "utf8")); } + catch (e) { + console.error("error: " + settingsPath + " exists but is not valid JSON (" + + (e && e.message ? e.message : e) + "); refusing to rewrite it. Fix or move the file, then re-run."); + process.exit(1); + } + } const before = JSON.stringify(settings, null, 2); if (!settings.hooks) settings.hooks = {}; if (!settings.hooks[event]) settings.hooks[event] = []; + // Identity key is (event, source): any existing entry carrying OUR + // source tag for this event IS the entry to compare/update — a matcher + // change must update it in place, never push a SECOND gstack entry + // (the old key included the matcher, so a future matcher change would + // have duplicated the registration). Untagged legacy entries are still + // adopted when both matcher and command line up. const matchesEntry = (entry) => { + if (entry._gstack_source === source) return true; const sameMatcher = (entry.matcher || "") === matcher; const sameCommand = entry.hooks && entry.hooks[0] && entry.hooks[0].command === cmd; - const sameSource = entry._gstack_source === source; - return sameMatcher && (sameSource || sameCommand); + return sameMatcher && sameCommand; }; let existing = settings.hooks[event].find(matchesEntry); @@ -202,6 +228,10 @@ case "$ACTION" in if (existing) { existing.hooks = [hookEntry]; existing._gstack_source = source; + // Keep the matcher current too — under the (event, source) key the + // matched entry may carry a stale matcher. + if (matcher) existing.matcher = matcher; + else delete existing.matcher; } else { const newEntry = { _gstack_source: source, hooks: [hookEntry] }; if (matcher) newEntry.matcher = matcher; diff --git a/bin/gstack-slug b/bin/gstack-slug index fb164a69b..ede341e4c 100755 --- a/bin/gstack-slug +++ b/bin/gstack-slug @@ -224,6 +224,12 @@ if [[ -z "$SLUG" ]]; then if [[ -n "$REMOTE_URL" ]]; then RAW_SLUG=$(printf '%s' "${REMOTE_URL%.git}" | sed -E 's#.*[:/]([^/]+)/([^/]+)$#\1-\2#') SLUG=$(printf '%s' "$RAW_SLUG" | tr -cd 'a-zA-Z0-9._-') + # Dot-only / degenerate guard: a hostile origin like `url = ..` (git + # accepts it) passes sed unchanged and would become SLUG=".." — filing + # state one level ABOVE ~/.gstack/projects/. Reject empty/"."/".."/ + # slash-bearing slugs and fall through to the basename fallback below. + # (tr -cd already deletes "/", so */* is belt-and-braces.) + case "$SLUG" in ""|.|..|*/*) SLUG="" ;; esac fi fi diff --git a/browse/src/snapshot.ts b/browse/src/snapshot.ts index 3b4c610c7..22ea345f9 100644 --- a/browse/src/snapshot.ts +++ b/browse/src/snapshot.ts @@ -356,9 +356,14 @@ export async function handleSnapshot( // `-o` only means something to the two modes that PRODUCE an image. Passed // alone it used to be silently ignored: exit 0, no file, no explanation — // which reads as "the screenshot feature is broken" rather than "you forgot a - // flag", and cost a real debugging session before anyone noticed. - if (opts.outputPath && !opts.annotate && !opts.heatmap) { - output.push(`[warning] -o/--output was ignored: it names the file for an annotated screenshot, so it needs -a/--annotate (or -C/--cursor-interactive). For a plain screenshot use: browse screenshot ${opts.outputPath}`); + // flag", and cost a real debugging session before anyone noticed. Kept as a + // variable so the diff-mode returns below (which bypass `output`) can carry + // it too — diff mode must not regress to the silent-ignore behavior. + const outputIgnoredWarning = (opts.outputPath && !opts.annotate && !opts.heatmap) + ? `[warning] -o/--output was ignored: it names the output file for an annotated (-a/--annotate) or heatmap (-H/--heatmap) screenshot. For a plain screenshot use: browse screenshot ${opts.outputPath}` + : ''; + if (outputIgnoredWarning) { + output.push(outputIgnoredWarning); } // ─── Annotated screenshot (-a) ──────────────────────────── @@ -615,7 +620,8 @@ export async function handleSnapshot( const lastSnapshot = session.getLastSnapshot(); if (!lastSnapshot) { session.setLastSnapshot(snapshotText); - return snapshotText + '\n\n(no previous snapshot to diff against — this snapshot stored as baseline)'; + return snapshotText + '\n\n(no previous snapshot to diff against — this snapshot stored as baseline)' + + (outputIgnoredWarning ? '\n' + outputIgnoredWarning : ''); } const changes = Diff.diffLines(lastSnapshot, snapshotText); @@ -630,7 +636,8 @@ export async function handleSnapshot( } session.setLastSnapshot(snapshotText); - return stripLoneSurrogates(diffOutput.join('\n')); + return stripLoneSurrogates(diffOutput.join('\n') + + (outputIgnoredWarning ? '\n' + outputIgnoredWarning : '')); } // Store for future diffs diff --git a/lib/bin-context.ts b/lib/bin-context.ts index 3260951ac..204f34f7f 100644 --- a/lib/bin-context.ts +++ b/lib/bin-context.ts @@ -46,7 +46,7 @@ const STRONG_FILE_MARKERS = [".project.yaml", "package.json", "pyproject.toml", const WEAK_FILE_MARKERS = ["README.md", "README", "README.rst", "LICENSE", "LICENSE.md"]; /** - * Native port of bin/gstack-slug's `_outermost_project_root` (:77-113): walk UP + * Native port of bin/gstack-slug's `_outermost_project_root`: walk UP * from `startDir` tracking the OUTERMOST ancestor holding a strong marker and * the outermost holding a weak marker. Outermost STRONG wins; else outermost * WEAK; else "". Build/deploy artifacts (.vercel, node_modules, dist, ...) are @@ -77,6 +77,41 @@ export function outermostProjectRoot(startDir: string): string { return outermostStrong || outermostWeak; } +/** + * Native port of bin/gstack-slug's `_outermost_remote_repo` (step 1a): walk UP + * from `startDir` tracking the OUTERMOST ancestor that has a `.git` entry + * (directory for normal clones, FILE for git-worktrees/submodules — `git -C` + * resolves a worktree's remote through its main clone) AND whose `origin` + * remote resolves. This is the canonical-identity walk: a marker-only + * ancestor with no resolvable origin (stray empty ~/.git, stray package.json) + * cannot win here, so it cannot hijack remote-derived identity the way it can + * hijack the marker walk above. Nested-repo semantics preserved: an inner + * repo under an outer canonical-remote repo still resolves to the OUTER + * repo's remote (outermost wins). git spawns only at `.git`-bearing ancestors + * — typically one. Exported for the parity tests. + */ +export function outermostRemoteRepo(startDir: string): { root: string; url: string } { + let dir = startDir; + let root = ""; + let url = ""; + let depth = 0; + while (dir && dir !== "/" && depth < 64) { + if (existsSync(join(dir, ".git"))) { + const r = spawnSync("git", ["-C", dir, "remote", "get-url", "origin"], { encoding: "utf-8" }); + const u = r.status === 0 ? (r.stdout || "").trim() : ""; + if (u) { + root = dir; + url = u; + } + } + const parent = dirname(dir); + if (parent === dir) break; // dirname fixed point (C:\, ., //srv) + dir = parent; + depth += 1; + } + return { root, url }; +} + /** * Native port of bin/gstack-slug's resolution order, used when that script cannot be * spawned (see resolveSlug). Same steps, same alphabet, same cache file — so this and @@ -84,15 +119,28 @@ export function outermostProjectRoot(startDir: string): string { * Context Recovery preamble READS using the script. * * Resolution order (parity with the bash script, pinned by - * test/bin-context-windows-slug.test.ts against test/gstack-slug-cwd-walk-up.test.ts): + * test/bin-context-windows-slug.test.ts against test/gstack-slug-cwd-walk-up.test.ts + * and test/gstack-slug-parity.test.ts): * 0. $GSTACK_PROJECT_SLUG env override — wins over everything, never cached. * 1. Walk UP to the OUTERMOST project root (see outermostProjectRoot). Without * the walk, a nested/vendored repo derived its slug from the INNERMOST * `git remote get-url origin`, splitting the store the bash side keeps whole. - * 2. Cached slug is sticky — EXCEPT the provable old-bug shape (#1125): cached - * value equals basename(cwd) while the walk-up says cwd is NOT the project - * root; that cache came from the pre-walk-up resolver, so recompute and heal. - * 3. Git remote AT THE PROJECT ROOT: [:/]/[.git] → owner-repo. + * 2. Cached slug is sticky (#2212) — EXCEPT two provable bug shapes: + * - old-bug shape (#1125): cached value equals basename(cwd) while the + * walk-up says cwd is NOT the project root; that cache came from the + * pre-walk-up resolver, so recompute and heal. + * - degraded-ancestor shape (2026-08-17): cached equals the marker root's + * basename while a remote-bearing repo BELOW the marker root exists — + * the pre-remote-first resolver degraded to a stray ancestor's basename + * (stray empty ~/.git → SLUG=). Legit #2212 stickiness is + * safe: there the repo that adopted the remote IS the marker root + * (remote root == project root), so the heal never fires. + * 3. Canonical remote-derived slug from the OUTERMOST remote-bearing repo + * (see outermostRemoteRepo — never PROJECT_ROOT, which may be a + * marker-only ancestor with no remote): [:/]/[.git] → + * owner-repo, byte-parity with browse/bin/remote-slug. Degenerate slugs + * ("", ".", "..", anything with "/") are rejected — a hostile origin + * like `url = ..` must never escape ~/.gstack/projects/. * 4. Project root's basename; else basename(cwd) for plain non-project folders. */ export function slugFromEnvironment(gstackHome?: string, cwd: string = process.cwd()): string { @@ -108,24 +156,56 @@ export function slugFromEnvironment(gstackHome?: string, cwd: string = process.c // 1. outermost project root along the cwd ancestor chain (may be ""). const projectRoot = outermostProjectRoot(cwd); + // Lazy, memoized remote discovery (mirrors gstack-slug's _resolve_remote): + // needed on exactly two paths — fresh resolution and the degraded-ancestor + // heal check — so ordinary cache hits stay git-spawn-free. + let remote: { root: string; url: string } | null = null; + const resolveRemote = () => (remote ??= outermostRemoteRepo(cwd)); + let slug = ""; - // 2. cached slug is sticky (#2212), except the old-bug shape (#1125). + // 2. cached slug is sticky (#2212), except the two provable bug shapes + // (old-bug #1125 and degraded-ancestor 2026-08-17 — see the doc above). if (existsSync(cacheFile)) { try { const cached = sanitizeSlug(readFileSync(cacheFile, "utf-8").trim()); - const pwdBase = sanitizeSlug(basename(cwd)); - const oldBugShape = cached === pwdBase && projectRoot !== "" && projectRoot !== cwd; - if (cached && !oldBugShape) slug = cached; + if (cached) { + const pwdBase = sanitizeSlug(basename(cwd)); + const rootBase = projectRoot ? sanitizeSlug(basename(projectRoot)) : ""; + const oldBugShape = cached === pwdBase && projectRoot !== "" && projectRoot !== cwd; + const degradedAncestorShape = + !oldBugShape && + projectRoot !== "" && + cached === rootBase && + (() => { + const r = resolveRemote(); + return r.url !== "" && r.root !== projectRoot; + })(); + if (!oldBugShape && !degradedAncestorShape) slug = cached; + } } catch { slug = ""; } } - // 3. derive from the project root's git remote (a subdir without its own - // remote inherits the parent's — same as `git -C "$PROJECT_ROOT"`). - if (!slug && projectRoot) { - const r = spawnSync("git", ["-C", projectRoot, "remote", "get-url", "origin"], { encoding: "utf-8" }); - const m = (r.stdout || "").trim().match(/[:/]([^/]+\/[^/]+?)(?:\.git)?$/); - if (m) slug = sanitizeSlug(m[1].replace(/\//g, "-")); + // 3. canonical remote-derived slug from the outermost remote-bearing repo. + // Parse mirrors bin/gstack-slug step 2 exactly (byte-parity with + // browse/bin/remote-slug): `${REMOTE_URL%.git}` strips ONE trailing + // ".git" (case-sensitive), then sed extracts the LAST two path segments + // — and sed's no-match passthrough means the stripped URL itself is the + // raw slug when no [:/]owner/repo tail exists. + if (!slug) { + const { url } = resolveRemote(); + if (url) { + const stripped = url.endsWith(".git") ? url.slice(0, -4) : url; + const m = stripped.match(/[:/]([^/]+)\/([^/]+)$/); + const candidate = sanitizeSlug(m ? `${m[1]}-${m[2]}` : stripped); + // Dot-only / degenerate guard (mirrors bin/gstack-slug): a hostile + // origin like `url = ..` yields "." or ".." here, which would file + // state OUTSIDE ~/.gstack/projects/. Reject and let the basename + // fallback below anchor identity instead. + if (candidate && candidate !== "." && candidate !== ".." && !candidate.includes("/")) { + slug = candidate; + } + } } // 4. project root's basename, else pwd basename for plain folders. if (!slug && projectRoot) slug = sanitizeSlug(basename(projectRoot)); diff --git a/test/bin-context-windows-slug.test.ts b/test/bin-context-windows-slug.test.ts index be7291d8d..0597bfb3d 100644 --- a/test/bin-context-windows-slug.test.ts +++ b/test/bin-context-windows-slug.test.ts @@ -276,6 +276,85 @@ describe("walk-up parity with bin/gstack-slug (outermost project root)", () => { expectBoth(inner, "acme-outer"); }); + test("LIVE BUG SHAPE: a stray empty .git ancestor no longer degrades the slug (remote-first)", () => { + // The exact 2026-08-17 reproduction: an ancestor dir with an empty .git + // (not a valid repo, no origin) above a canonical-remote repo. The + // pre-remote-first native path resolved PROJECT_ROOT to the stray marker + // ancestor, found no origin THERE, and degraded to its basename — filing + // every repo under it into one shared ~/.gstack/projects//. + const strayHome = path.join(tmp, "strayhome"); + fs.mkdirSync(path.join(strayHome, ".git"), { recursive: true }); // empty — invalid repo + const repo = path.join(strayHome, "work", "repo"); + fs.mkdirSync(repo, { recursive: true }); + spawnSync("git", ["init", "-q", repo]); + spawnSync("git", ["-C", repo, "remote", "add", "origin", "https://github.com/garrytan/gstack"]); + expectBoth(repo, "garrytan-gstack"); + expect(slugFromEnvironment(nativeHome(), repo)).not.toBe("strayhome"); + }); + + test("nested repos under a stray marker: a no-remote outer cannot shadow an inner remote", () => { + // Outermost REMOTE-bearing repo wins — an outer repo whose origin does + // not resolve is skipped by the remote walk, so the inner remote-bearing + // repo carries identity (parity with bin/gstack-slug's _outermost_remote_repo). + const outer = path.join(tmp, "outer-plain"); + const inner = path.join(outer, "vendor", "inner-lib"); + fs.mkdirSync(inner, { recursive: true }); + spawnSync("git", ["init", "-q", outer]); // no origin — marker-only repo + spawnSync("git", ["init", "-q", inner]); + spawnSync("git", ["-C", inner, "remote", "add", "origin", "git@github.com:vendor/inner.git"]); + expectBoth(inner, "vendor-inner"); + }); + + test("degraded-ancestor cache self-heals: the pre-remote-first cached value is rewritten", () => { + // Pre-fix, the resolver cached basename(PROJECT_ROOT) for the stray + // marker ancestor. cached == the marker root's basename while a + // remote-bearing repo BELOW it exists → recompute + heal the cache. + const strayHome = path.join(tmp, "strayhome"); + fs.mkdirSync(path.join(strayHome, ".git"), { recursive: true }); + const repo = path.join(strayHome, "git", "proj"); + fs.mkdirSync(repo, { recursive: true }); + spawnSync("git", ["init", "-q", repo]); + spawnSync("git", ["-C", repo, "remote", "add", "origin", "https://github.com/garrytan/gstack"]); + + const cacheDir = path.join(nativeHome(), "slug-cache"); + fs.mkdirSync(cacheDir, { recursive: true }); + const cacheFile = path.join(cacheDir, toMsysPath(repo).replace(/\//g, "_")); + fs.writeFileSync(cacheFile, "strayhome"); // the degraded value the old resolver cached + + expect(slugFromEnvironment(nativeHome(), repo)).toBe("garrytan-gstack"); + // The cache file itself must have been overwritten (self-healing). + expect(fs.readFileSync(cacheFile, "utf-8")).toBe("garrytan-gstack"); + }); + + test("sticky identity preserved (#2212): a remote adopted AT the marker root is NOT healed", () => { + // Legit sticky shape: the repo that adopted the remote IS the marker root + // (remote root == project root), so the degraded-ancestor heal must not + // fire even though cached == basename(project root). + const repo = path.join(tmp, "stickyproj"); + fs.mkdirSync(repo, { recursive: true }); + spawnSync("git", ["init", "-q", repo]); + spawnSync("git", ["-C", repo, "remote", "add", "origin", "https://github.com/x/y.git"]); + + const cacheDir = path.join(nativeHome(), "slug-cache"); + fs.mkdirSync(cacheDir, { recursive: true }); + const cacheFile = path.join(cacheDir, toMsysPath(repo).replace(/\//g, "_")); + fs.writeFileSync(cacheFile, "stickyproj"); // pre-origin basename identity + + expect(slugFromEnvironment(nativeHome(), repo)).toBe("stickyproj"); + expect(fs.readFileSync(cacheFile, "utf-8")).toBe("stickyproj"); + }); + + test('hostile origin `url = ..` never becomes a dot slug — basename fallback (dot-only guard)', () => { + // git accepts `..` as a remote URL. Unchecked, the derived slug would be + // ".." — path traversal one level above ~/.gstack/projects/. Both + // implementations must reject it and fall through to the basename. + const repo = path.join(tmp, "dotty"); + fs.mkdirSync(repo, { recursive: true }); + spawnSync("git", ["init", "-q", repo]); + spawnSync("git", ["-C", repo, "remote", "add", "origin", ".."]); + expectBoth(repo, "dotty"); + }); + test("GSTACK_PROJECT_SLUG env override beats every other resolution path, never cached", () => { const projectRoot = path.join(tmp, "loadout"); const siteSubdir = path.join(projectRoot, "site"); diff --git a/test/brain-sync.test.ts b/test/brain-sync.test.ts index bc42bd6fc..7a0f1a70a 100644 --- a/test/brain-sync.test.ts +++ b/test/brain-sync.test.ts @@ -473,6 +473,32 @@ describe('gstack-brain-sync --discover-new', () => { }); }); +// --------------------------------------------------------------- +// Enqueue tmp janitor: a writer killed between its tmp write and the +// atomic rename orphans a .tmp-* file forever (it never becomes a +// record, nothing else touches it). The drain reaps ones older than +// 1 hour, inside its lock; fresh ones (in-flight enqueues) survive. +// --------------------------------------------------------------- +describe('enqueue tmp janitor', () => { + test('an orphaned .tmp-* older than 1h is reaped on --once; a fresh one survives', () => { + run(['gstack-artifacts-init', '--remote', bareRemote]); + run(['gstack-config', 'set', 'artifacts_sync_mode', 'full']); + fs.mkdirSync(spoolDir(), { recursive: true }); + + const oldTmp = path.join(spoolDir(), '.tmp-99999-x1'); + fs.writeFileSync(oldTmp, '{"file":"projects/p/learnings.jsonl"}\n'); + const past = new Date(Date.now() - 2 * 3600 * 1000); + fs.utimesSync(oldTmp, past, past); + + const freshTmp = path.join(spoolDir(), '.tmp-99999-x2'); + fs.writeFileSync(freshTmp, '{"file":"projects/p/learnings.jsonl"}\n'); + + expect(run(['gstack-brain-sync', '--once']).status).toBe(0); + expect(fs.existsSync(oldTmp)).toBe(false); // orphan reaped + expect(fs.existsSync(freshTmp)).toBe(true); // in-flight write untouched + }); +}); + // --------------------------------------------------------------- // #2549 queue integrity: classified drops, privacy retention, // surgical rewrite, unpushed-commit detector diff --git a/test/gbrain-repo-policy-client.test.ts b/test/gbrain-repo-policy-client.test.ts index bff6b682b..bd2e79e22 100644 --- a/test/gbrain-repo-policy-client.test.ts +++ b/test/gbrain-repo-policy-client.test.ts @@ -19,6 +19,7 @@ import * as os from "os"; import { spawnSync } from "child_process"; import { repoPolicyTierBatch } from "../lib/gbrain-repo-policy-client"; +import { canonicalizeRemote } from "../lib/gstack-memory-helpers"; const ROOT = path.resolve(import.meta.dir, ".."); const BIN = path.join(ROOT, "bin", "gstack-gbrain-repo-policy"); @@ -150,3 +151,60 @@ describe("repoPolicyTierBatch (TypeScript client)", () => { } }); }); + +// ── Normalize parity: bash normalize() ↔ lib canonicalizeRemote ───────────── +// +// bin/gstack-memory-ingest.ts produces page.git_remote via canonicalizeRemote +// (lib/gstack-memory-helpers) and then looks the policy up through +// repoPolicyTierBatch — whose bash side re-normalizes with normalize(). If +// the two functions disagree on ANY URL shape, a policy the user set via the +// script silently fails to apply to ingest (a deny that doesn't deny). The +// contract pinned here: for every shape X, `set X ` followed by a batch +// lookup of canonicalizeRemote(X) returns . Bash owns normalization — +// any divergence is fixed in the SCRIPT's normalize(), never by re-normalizing +// in TypeScript. + +describe("normalize parity: bash normalize() ↔ canonicalizeRemote (edge URL shapes)", () => { + // One distinct repo per shape so tiers don't overwrite each other. + const CORPUS: Array<{ shape: string; tier: "read-write" | "read-only" | "deny" }> = [ + { shape: "https://github.com/acme/plain", tier: "deny" }, + { shape: "https://github.com/acme/dotgit.git", tier: "read-only" }, + { shape: "https://github.com/acme/slash/", tier: "read-write" }, + // .git + trailing slash: bash must strip the slash BEFORE the .git suffix + // (slash-first order), as canonicalizeRemote does. + { shape: "https://github.com/acme/dotgitslash.git/", tier: "deny" }, + // Uppercase .GIT: canonicalizeRemote strips case-insensitively; bash must + // lowercase before the suffix strip or the key keeps a ".git" tail. + { shape: "https://github.com/ACME/UpperGit.GIT", tier: "read-only" }, + { shape: "git@github.com:acme/scp.git", tier: "deny" }, + { shape: "ssh://git@github.com/acme/sshurl.git", tier: "read-write" }, + ]; + + test("normalize prints exactly canonicalizeRemote(url) for every corpus shape", () => { + for (const { shape } of CORPUS) { + const r = run(["normalize", shape]); + expect(r.status).toBe(0); + expect(r.stdout.trim()).toBe(canonicalizeRemote(shape)); + } + }); + + test("a policy set via the script with shape X is found via canonicalizeRemote(X)", () => { + for (const { shape, tier } of CORPUS) { + expect(run(["set", shape, tier]).status).toBe(0); + } + const canon = CORPUS.map((c) => canonicalizeRemote(c.shape)); + const verdicts = repoPolicyTierBatch(canon, env()); + for (let i = 0; i < CORPUS.length; i++) { + expect(verdicts.get(canon[i])).toEqual({ tier: CORPUS[i].tier }); + } + }); + + test("cross-shape: set through one shape, looked up through another shape of the same repo", () => { + // The store keys on the normalized form, so every spelling of the same + // repo shares one entry — set through scp form, read through https form. + expect(run(["set", "git@github.com:acme/xshape.git", "deny"]).status).toBe(0); + const canon = canonicalizeRemote("https://github.com/ACME/XShape.GIT/"); + const verdicts = repoPolicyTierBatch([canon], env()); + expect(verdicts.get(canon)).toEqual({ tier: "deny" }); + }); +}); diff --git a/test/gstack-memory-ingest.test.ts b/test/gstack-memory-ingest.test.ts index cbbb03969..2d6c525a6 100644 --- a/test/gstack-memory-ingest.test.ts +++ b/test/gstack-memory-ingest.test.ts @@ -938,6 +938,82 @@ describe("#2394: probe applies the same attribution gate as prepare", () => { expect(reachedImport).toBe(probeNew); rmSync(home, { recursive: true, force: true }); }); + + it("a multi-MB transcript is still classified correctly (bounded probe read)", () => { + // The probe reads a BOUNDED 256KB prefix, never the whole file (plan C7). + // The cwd sits on the first line; >1MB of filler follows. Classification + // must come out attributable — and stay cheap on real multi-MB corpora. + const home = makeTestHome(); + const gstackHome = join(home, ".gstack"); + mkdirSync(gstackHome, { recursive: true }); + const attributableCwd = join(home, "work", "attributable-repo"); + mkdirSync(attributableCwd, { recursive: true }); + spawnSync("git", ["-C", attributableCwd, "init", "-q"], { encoding: "utf-8" }); + spawnSync("git", ["-C", attributableCwd, "remote", "add", "origin", "https://github.com/foo/bar.git"], { encoding: "utf-8" }); + + const ts = new Date().toISOString(); + const cwdLine = `{"type":"user","message":{"role":"user","content":"hello"},"timestamp":"${ts}","cwd":"${attributableCwd.replace(/\\/g, "\\\\")}"}\n`; + const filler = `{"type":"assistant","message":{"role":"assistant","content":"${"x".repeat(1000)}"}}\n`; + const body = cwdLine + filler.repeat(1100); // > 1MB after the cwd line + expect(body.length).toBeGreaterThan(1024 * 1024); + writeClaudeCodeSession(home, "work-attributable", "big1", body); + + const r = runScript(["--probe"], { HOME: home, GSTACK_HOME: gstackHome }); + expect(r.exitCode).toBe(0); + expect(r.stdout).toContain("Total files in window: 1"); + expect(r.stdout).not.toContain("Skipped (unattributed)"); + rmSync(home, { recursive: true, force: true }); + }); + + it("Codex format: session_meta cwd attributes the transcript in the probe", () => { + const home = makeTestHome(); + const gstackHome = join(home, ".gstack"); + mkdirSync(gstackHome, { recursive: true }); + const attributableCwd = makeAttributableCwd(home); + const today = new Date(); + const ymd = `${today.getFullYear()}-${String(today.getMonth() + 1).padStart(2, "0")}-${String(today.getDate()).padStart(2, "0")}`; + const session = `{"type":"session_meta","payload":{"id":"sess-meta-cwd","cwd":"${attributableCwd.replace(/\\/g, "\\\\")}"},"timestamp":"${today.toISOString()}"}\n`; + writeCodexSession(home, ymd, session); + + const r = runScript(["--probe"], { HOME: home, GSTACK_HOME: gstackHome }); + expect(r.exitCode).toBe(0); + expect(r.stdout).toContain("Total files in window: 1"); + expect(r.stdout).not.toContain("Skipped (unattributed)"); + rmSync(home, { recursive: true, force: true }); + }); + + it("parity: a Codex cwd appearing only on a LATER record is unattributed in probe AND prepare", () => { + // parseTranscriptJsonl reads Codex cwd from the session_meta FIRST record + // ONLY. The probe mirrors those exact rules — the pre-fix probe scanned + // every line for any cwd and DIVERGED on this shape (probe said + // attributable, prepare said not). + const home = makeTestHome(); + const gstackHome = join(home, ".gstack"); + mkdirSync(gstackHome, { recursive: true }); + const attributableCwd = makeAttributableCwd(home); + const today = new Date(); + const ymd = `${today.getFullYear()}-${String(today.getMonth() + 1).padStart(2, "0")}-${String(today.getDate()).padStart(2, "0")}`; + const session = + `{"type":"session_meta","payload":{"id":"sess-late-cwd"},"timestamp":"${today.toISOString()}"}\n` + + `{"type":"response_item","payload":{"type":"message","role":"user","content":[{"text":"hi"}]},"cwd":"${attributableCwd.replace(/\\/g, "\\\\")}"}\n`; + writeCodexSession(home, ymd, session); + + const probe = runScript(["--probe"], { HOME: home, GSTACK_HOME: gstackHome }); + expect(probe.exitCode).toBe(0); + expect(probe.stdout).toContain("Total files in window: 0"); + expect(probe.stdout).toContain("Skipped (unattributed): 1"); + + // Prepare agrees: nothing reaches the import stage (written + failed = 0) + // and the skip is attributed to the same gate. + const inc = runScript(["--incremental"], { HOME: home, GSTACK_HOME: gstackHome }); + expect(inc.exitCode).toBe(0); + const written = Number((inc.stdout.match(/written:\s+(\d+)/) || [])[1]); + const failed = Number((inc.stdout.match(/failed:\s+(\d+)/) || [])[1]); + const unattrib = Number((inc.stdout.match(/skipped \(unattrib\):\s+(\d+)/) || [])[1]); + expect(written + failed).toBe(0); + expect(unattrib).toBe(1); + rmSync(home, { recursive: true, force: true }); + }); }); // ── #2392: transcript ingest honors the per-remote trust policy ───────────── diff --git a/test/gstack-slug-parity.test.ts b/test/gstack-slug-parity.test.ts index 5e1ce5d36..7636dcb65 100644 --- a/test/gstack-slug-parity.test.ts +++ b/test/gstack-slug-parity.test.ts @@ -219,6 +219,20 @@ describe('gstack-slug ↔ remote-slug parity', () => { expect(fs.readFileSync(cacheFile, 'utf8').trim()).toBe('garrytan-gstack'); }); + test('hostile origin `url = ..` cannot become a ".." slug — basename fallback holds', () => { + // git accepts `..` as a remote URL; the sed parse passes it through + // unchanged, so unguarded it becomes SLUG=".." — filing state one level + // ABOVE ~/.gstack/projects/ (confined to ~/.gstack, but still traversal). + // The dot-only guard rejects it and the basename fallback anchors identity. + const repo = makeRepo(path.join(fixtures, 'dotty'), '..'); + const r = runSlug(repo, tmpHome); + expect(r.status).toBe(0); + expect(slugOf(r)).toBe('dotty'); + // The cache must hold the healed value, never the dot slug. + const cacheFile = path.join(tmpHome, '.gstack', 'slug-cache', encodedCacheKey(repo)); + expect(fs.readFileSync(cacheFile, 'utf8').trim()).toBe('dotty'); + }); + test('sticky identity preserved (#2212): repo that adopted a remote after first use is NOT healed', () => { // Legit sticky shape: the repo itself is the marker root (REMOTE_ROOT == // PROJECT_ROOT) and its cached identity is its pre-origin basename slug. diff --git a/test/gstack-version-bump.test.ts b/test/gstack-version-bump.test.ts index a35afd3c2..84e24e97c 100644 --- a/test/gstack-version-bump.test.ts +++ b/test/gstack-version-bump.test.ts @@ -579,10 +579,21 @@ describe('path containment: pins and flags cannot escape the repo', () => { }); describe('#2600: repair must not write fabricated 0.0.0.0 when VERSION is missing', () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-2600-')); - afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* noop */ } }); + // Per-test dirs: the tests assert both "VERSION absent" and "VERSION + // present" states, so a shared dir made them order-dependent (test 1's + // absence assertion only held because test 2 hadn't run yet). + const dirs: string[] = []; + const makeDir = (): string => { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-2600-')); + dirs.push(d); + return d; + }; + afterAll(() => { + for (const d of dirs) { try { fs.rmSync(d, { recursive: true, force: true }); } catch { /* noop */ } } + }); test('repair fails with exit 2 when VERSION file does not exist', () => { + const dir = makeDir(); // Set up: package.json exists with version 0.1.0.0, but no VERSION file fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: 'x', version: '0.1.0.0' }, null, 2) + '\n'); // VERSION file deliberately absent @@ -605,6 +616,7 @@ describe('#2600: repair must not write fabricated 0.0.0.0 when VERSION is missin }); test('repair works normally when VERSION file exists', () => { + const dir = makeDir(); // Set up: both VERSION and package.json exist, with drift fs.writeFileSync(path.join(dir, 'VERSION'), '2.0.0.0\n'); fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: 'x', version: '1.9.0' }, null, 2) + '\n'); @@ -618,6 +630,7 @@ describe('#2600: repair must not write fabricated 0.0.0.0 when VERSION is missin }); test('repair refuses to propagate a fabricated version when VERSION file is empty (#2600)', () => { + const dir = makeDir(); // VERSION exists but is empty — readVersionFile folds this into DEFAULT ("0.0.0.0"). // Without the `current === DEFAULT` guard, this would write 0.0.0 into package.json. fs.writeFileSync(path.join(dir, 'VERSION'), ''); @@ -641,8 +654,7 @@ describe('#2600: repair must not write fabricated 0.0.0.0 when VERSION is missin test('repair reproduces the exact issue scenario: VERSION in root, package.json in app/ (#2600)', () => { // The exact layout from the issue: VERSION at repo root, package.json in app/ // Running repair from app/ cwd with no VERSION there used to write 0.0.0.0 into app/package.json. - const rootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-2600-exact-')); - afterAll(() => { try { fs.rmSync(rootDir, { recursive: true, force: true }); } catch { /* noop */ } }); + const rootDir = makeDir(); fs.mkdirSync(path.join(rootDir, 'app'), { recursive: true }); fs.writeFileSync(path.join(rootDir, 'VERSION'), '0.2.0.0\n'); @@ -666,21 +678,31 @@ describe('#2600: repair must not write fabricated 0.0.0.0 when VERSION is missin }); describe('#2600: classify must surface versionFileExists=false when VERSION is missing', () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-2600-classify-')); - afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* noop */ } }); + // Per-test dirs: one test asserts VERSION absent, the other creates it — a + // shared dir made them order-dependent. Each test builds its own repo. + const dirs: string[] = []; + afterAll(() => { + for (const d of dirs) { try { fs.rmSync(d, { recursive: true, force: true }); } catch { /* noop */ } } + }); - // Set up a minimal git repo so classify can resolve base - const git = (...a: string[]) => execFileSync('git', a, { cwd: dir, stdio: 'pipe' }); - git('init', '-q', '-b', 'main'); - git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); - // Commit with no VERSION file - fs.writeFileSync(path.join(dir, 'README.md'), 'test\n'); - git('add', '-A'); git('commit', '-q', '-m', 'base'); - const head = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: dir }).toString().trim(); - fs.mkdirSync(path.join(dir, '.git', 'refs', 'remotes', 'origin'), { recursive: true }); - fs.writeFileSync(path.join(dir, '.git', 'refs', 'remotes', 'origin', 'main'), head + '\n'); + /** Minimal git repo (no VERSION committed) so classify can resolve base. */ + function makeRepoDir(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-2600-classify-')); + dirs.push(dir); + const git = (...a: string[]) => execFileSync('git', a, { cwd: dir, stdio: 'pipe' }); + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); + // Commit with no VERSION file + fs.writeFileSync(path.join(dir, 'README.md'), 'test\n'); + git('add', '-A'); git('commit', '-q', '-m', 'base'); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: dir }).toString().trim(); + fs.mkdirSync(path.join(dir, '.git', 'refs', 'remotes', 'origin'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.git', 'refs', 'remotes', 'origin', 'main'), head + '\n'); + return dir; + } test('classify reports versionFileExists=false when VERSION is absent', () => { + const dir = makeRepoDir(); // No package.json: pkgExists=false, pkgAgrees=true, current===base → FRESH. // (A package.json with a non-zero version would cause DRIFT_UNEXPECTED.) @@ -693,7 +715,8 @@ describe('#2600: classify must surface versionFileExists=false when VERSION is m }); test('classify reports versionFileExists=true when VERSION is present', () => { - // Now create VERSION AND sync package.json so pkgAgrees=true → ALREADY_BUMPED. + const dir = makeRepoDir(); + // Create VERSION AND sync package.json so pkgAgrees=true → ALREADY_BUMPED. fs.writeFileSync(path.join(dir, 'VERSION'), '0.2.0.0\n'); fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: 'x', version: '0.2.0.0' }, null, 2) + '\n'); diff --git a/test/session-update-autostash.test.ts b/test/session-update-autostash.test.ts index faf6ff3ca..90624b1e1 100644 --- a/test/session-update-autostash.test.ts +++ b/test/session-update-autostash.test.ts @@ -234,6 +234,23 @@ describe('gstack-session-update lock identity + TTL (#2613)', () => { } }, 30000); + test('reclaim is TOCTOU-safe: both reclaim branches mv the lock aside atomically (static pin)', () => { + // `rm -rf "$LOCK_DIR"` then `mkdir` lets TWO contenders both judge the + // lock stale and both win (one rm can land between the other's rm and + // mkdir). The atomic mv-aside makes exactly one contender own the reap: + // the loser's mv fails and it backs off with SKIP lock_contested. Pin + // that BOTH reclaim branches (TTL-expired and dead-PID) use it, and that + // no bare in-place `rm -rf "$LOCK_DIR"` survives outside the holder's + // own EXIT trap. + const src = fs.readFileSync(SCRIPT, 'utf8'); + const mvAside = src.match(/mv "\$LOCK_DIR" "\$LOCK_DIR\.reap\.\$\$" 2>\/dev\/null \|\| \{ log_entry "SKIP lock_contested"; exit 0; \}/g) || []; + expect(mvAside.length).toBe(2); // TTL branch + dead-PID branch + // The only rm -rf of the live lock dir is the holder's EXIT trap. + const bareRms = src.match(/rm -rf "\$LOCK_DIR"(?!\.)/g) || []; + expect(bareRms.length).toBe(1); + expect(src).toContain(`trap 'rm -rf "$LOCK_DIR" 2>/dev/null' EXIT`); + }); + test('an expired-TTL lock is reclaimed even when its pid is alive (PID reuse)', async () => { const { base, install, state } = makeFixture(); const holder = require('child_process').spawn('sleep', ['30'], { stdio: 'ignore' }); diff --git a/test/timeline-stop-hook.test.ts b/test/timeline-stop-hook.test.ts index c9c171ccf..24f39d4c0 100644 --- a/test/timeline-stop-hook.test.ts +++ b/test/timeline-stop-hook.test.ts @@ -343,6 +343,71 @@ describe('timeline-stop-hook wiring', () => { } }); + test('corrupt settings.json: ensure-event refuses (exit 1) and never rewrites the file', () => { + // The old catch{} folded an unparseable EXISTING settings.json into {} + // and the atomic write replaced the user's permissions/env/other hooks + // with just ours. Now: loud stderr error, exit 1, file byte-identical. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-ensure-corrupt-')); + try { + const settingsFile = path.join(dir, 'settings.json'); + const corrupt = '{ "permissions": { "allow": ["Bash(npm:*)"] }, INVALID'; + fs.writeFileSync(settingsFile, corrupt); + + const r = spawnSync('bash', [ + SETTINGS_HOOK, 'ensure-event', + '--event', 'Stop', + '--command', HOOK, + '--source', 'gstack-timeline-stop', + '--timeout', '5', + ], { env: { ...process.env, GSTACK_SETTINGS_FILE: settingsFile }, encoding: 'utf-8', timeout: 15_000 }); + + expect(r.status).toBe(1); + expect(r.stderr).toContain('not valid JSON'); + // Never rewritten — the corrupt bytes (and whatever the user can still + // salvage from them) survive verbatim. + expect(fs.readFileSync(settingsFile, 'utf-8')).toBe(corrupt); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test('a matcher change updates the tagged entry in place — still exactly one registration', () => { + // Identity key is (event, source): an existing gstack entry with a STALE + // matcher must be updated, never joined by a second entry (the old key + // included the matcher, so any future matcher change would duplicate). + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-ensure-matcher-')); + try { + const settingsFile = path.join(dir, 'settings.json'); + fs.writeFileSync(settingsFile, JSON.stringify({ + hooks: { + PreToolUse: [{ + _gstack_source: 'gstack-plan-tune', + matcher: 'OldMatcher', + hooks: [{ type: 'command', command: '/old/path/hook', timeout: 5 }], + }], + }, + }, null, 2) + '\n'); + + const r = spawnSync('bash', [ + SETTINGS_HOOK, 'ensure-event', + '--event', 'PreToolUse', + '--command', '/new/path/hook', + '--source', 'gstack-plan-tune', + '--matcher', 'NewMatcher', + '--timeout', '5', + ], { env: { ...process.env, GSTACK_SETTINGS_FILE: settingsFile }, encoding: 'utf-8', timeout: 15_000 }); + + expect(r.status).toBe(0); + const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8')); + expect(s.hooks.PreToolUse).toHaveLength(1); // updated in place — never two + expect(s.hooks.PreToolUse[0].matcher).toBe('NewMatcher'); + expect(s.hooks.PreToolUse[0].hooks[0].command).toBe('/new/path/hook'); + expect(s.hooks.PreToolUse[0]._gstack_source).toBe('gstack-plan-tune'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + test('a failed update leaves exactly one registration — never zero, never two', () => { // Root can write through 0o555 directories, so the failure injection // (read-only dir) does not bind there; the invariant is still covered by