From 6ac92d15ab5c43cba7afc084207f0c8b3ead5147 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:39:26 +0000 Subject: [PATCH 01/20] G1: ship without a version when no version source is configured Resolve the version source to four outcomes (valid, absent, ambiguous, broken) in lib/version-source.ts and read that one signal in gstack-version-bump (new NO_VERSION state; write refuses to invent a VERSION), gstack-next-version (no_version, no slot), /ship's bump, CHANGELOG and title steps, /land-and-deploy's drift check and /document-release. A configured version file that is missing, empty, unreadable or malformed stops with its path; 0.0.0.0 is never substituted. docs/skills.md documents .gstack/version-path. Migrates two tests that pinned the old fabricated-0.0.0.0 behavior. Co-authored-by: Koda Co-authored-by: Merit Blake --- bin/gstack-next-version | 51 ++++-- bin/gstack-version-bump | 97 ++++++++---- docs/skills.md | 16 ++ .../sections/release-body.md.tmpl | 19 ++- land-and-deploy/SKILL.md.tmpl | 8 +- .../sections/readiness-gate.md.tmpl | 6 +- lib/version-source.ts | 147 ++++++++++++++++++ ship/SKILL.md.tmpl | 15 +- ship/sections/changelog.md.tmpl | 4 + ship/sections/pr-body.md.tmpl | 2 +- test/gstack-next-version.test.ts | 112 ++++++++++--- test/gstack-version-bump.test.ts | 125 ++++++++++++++- test/ship-version-sync.test.ts | 54 +++++++ 13 files changed, 577 insertions(+), 79 deletions(-) diff --git a/bin/gstack-next-version b/bin/gstack-next-version index 6b480d87a..df5473b6b 100755 --- a/bin/gstack-next-version +++ b/bin/gstack-next-version @@ -27,8 +27,11 @@ // provide (#2501). // // Exit codes: -// 0 — emitted JSON successfully (may include "offline":true or "host":"unknown") -// 2 — invalid arguments +// 0 — emitted JSON successfully (may include "offline":true or "host":"unknown", +// or "no_version":true with "version":null when no version source is +// configured — see resolveVersionSource in lib/version-source.ts) +// 2 — invalid arguments, or a configured version source that is missing, +// empty, unreadable or malformed // 3 — util bug (unexpected exception) import { execFileSync, spawnSync } from "node:child_process"; @@ -43,6 +46,9 @@ import { cmpVersion, bumpWasCoerced, extractVersion, + noVersionNotice, + resolveVersionRel, + resolveVersionSource, type Bump, type Version, type VersionWidth, @@ -115,17 +121,11 @@ function runCommand(cmd: string, args: string[], timeoutMs = 15000): { ok: boole // at repo root > "VERSION". Pure function; takes the repo root as an argument so // tests can drive it with a fixture dir without mocking git. function resolveVersionPath(override: string | undefined, repoRoot: string): string { - if (override) return override.trim(); - const configFile = join(repoRoot, ".gstack", "version-path"); - if (existsSync(configFile)) { - try { - const firstLine = readFileSync(configFile, "utf8").split("\n")[0]?.trim() ?? ""; - if (firstLine) return firstLine; - } catch { - // fall through to default - } + try { + return resolveVersionRel(repoRoot, override).rel; + } catch { + return "VERSION"; } - return "VERSION"; } function repoToplevel(): string { @@ -683,7 +683,32 @@ async function main() { const warnings: string[] = []; const host = detectHost(); const repoRoot = repoToplevel(); - const versionPath = resolveVersionPath(args.versionPath, repoRoot); + // One no-version signal (#2334, #2343): the same four-outcome resolution + // gstack-version-bump uses. No configured source → report no_version and + // allocate nothing; a broken configured source → stop with the reason. + const src = resolveVersionSource(repoRoot, args.versionPath); + if (src.outcome === "broken") { + console.error(`Error: version source is broken: ${src.reason}. Fix that file or its pin (gstack never substitutes 0.0.0.0).`); + process.exit(2); + } + if (src.outcome !== "valid") { + process.stdout.write( + JSON.stringify( + { + version: null, + no_version: true, + version_source: { outcome: src.outcome, path: src.path, reason: src.reason }, + notice: noVersionNotice(src), + bump: args.bump, + warnings, + }, + null, + 2, + ) + "\n", + ); + return; + } + const versionPath = src.path!; const baseVersion = args.current || readBaseVersion(args.base, versionPath, repoRoot, warnings); const baseParsed = parseVersion(baseVersion); if (!baseParsed) { diff --git a/bin/gstack-version-bump b/bin/gstack-version-bump index a7de4e14c..7cb5d51e0 100755 --- a/bin/gstack-version-bump +++ b/bin/gstack-version-bump @@ -16,9 +16,14 @@ // classify --base [--version-path

] // Compares VERSION vs origin/:VERSION vs package.json.version. // Emits JSON: { state, baseVersion, currentVersion, pkgVersion, pkgExists } -// state ∈ FRESH | ALREADY_BUMPED | DRIFT_STALE_PKG | DRIFT_UNEXPECTED +// state ∈ FRESH | ALREADY_BUMPED | DRIFT_STALE_PKG | DRIFT_UNEXPECTED | NO_VERSION +// NO_VERSION: no version source is configured (absent) or the repo's +// version is owned elsewhere (ambiguous) — /ship ships without a +// version change and prints `notice`. A configured source that is +// missing, empty, unreadable or malformed exits 2 with the reason. // Exit 0 on a decidable state (incl. DRIFT_UNEXPECTED — it's a real state -// the caller must handle), exit 2 on bad args / unresolvable base. +// the caller must handle), exit 2 on bad args / unresolvable base / +// broken version source. // // write --version [--version-path

] [--regen-digest] // Validates the 4-digit pattern, writes VERSION + package.json.version. @@ -61,7 +66,16 @@ import { existsSync, readFileSync, realpathSync, writeFileSync } from "node:fs"; import { execFileSync } from "node:child_process"; import { dirname, isAbsolute, join, relative, resolve, sep } from "node:path"; -import { extractVersion, isJsonVersionPath, npmVersion, setVersionInJson } from "../lib/version-source"; +import { + extractVersion, + isJsonVersionPath, + noVersionNotice, + npmVersion, + resolveVersionRel as resolvePinnedRel, + resolveVersionSource, + setVersionInJson, + type VersionSource, +} from "../lib/version-source"; // 3- or 4-digit (#2501). gstack's own VERSION stays 4-digit MAJOR.MINOR.PATCH. // MICRO and stays the source of truth, but a repo whose pinned version source @@ -129,20 +143,16 @@ function assertRepoContained(cwd: string, rel: string, source: string): void { * Resolving once, here, keeps base and current reads in step. */ function resolveVersionRel(cwd: string, explicit?: string): string { - if (explicit) { - const rel = explicit.trim(); - assertRepoContained(cwd, rel, "--version-path"); - return rel; - } - const pin = join(cwd, ".gstack", "version-path"); - if (existsSync(pin)) { - const p = readFileSync(pin, "utf-8").split("\n")[0]?.trim() ?? ""; - if (p) { - assertRepoContained(cwd, p, ".gstack/version-path"); - return p; - } - } - return "VERSION"; + return resolvePinnedRel(cwd, explicit, (rel, source) => assertRepoContained(cwd, rel, source)).rel; +} + +/** The four-outcome resolution (lib/version-source.ts), containment-guarded. */ +function versionSource(cwd: string, explicit?: string): VersionSource { + return resolveVersionSource(cwd, explicit, (rel, source) => assertRepoContained(cwd, rel, source)); +} + +function failBroken(src: VersionSource, verb: string): never { + fail(`${verb}: version source is broken: ${src.reason}. Fix that file or its pin, then re-run /ship (gstack never substitutes 0.0.0.0).`, 2); } @@ -302,9 +312,31 @@ function classifyState( function cmdClassify(args: string[], cwd: string): void { const base = argVal(args, "--base"); if (!base) fail("classify requires --base ", 2); - const versionRel = resolveVersionRel(cwd, argVal(args, "--version-path")); + const src = versionSource(cwd, argVal(args, "--version-path")); + if (src.outcome === "broken") failBroken(src, "classify"); + if (src.outcome !== "valid") { + // No configured version source (#2334, #2343): report it as its own state + // instead of classifying an invented 0.0.0.0. /ship skips the bump, the + // CHANGELOG version header and the PR title prefix, and prints `notice`. + process.stdout.write( + JSON.stringify({ + state: "NO_VERSION", + versionSource: { outcome: src.outcome, path: src.path, reason: src.reason }, + notice: noVersionNotice(src), + baseVersion: null, + currentVersion: null, + pkgVersion: null, + pkgExists: false, + pkgPath: null, + expectedPkgVersion: null, + versionFileExists: false, + }) + "\n", + ); + return; + } + const versionRel = src.path!; const versionPath = join(cwd, versionRel); - const current = readVersionFile(versionPath, versionRel); + const current = src.version!; const baseV = baseVersion(cwd, base!, versionRel); // When the version-path IS a package.json (#2501), that file is the single // source of truth and the "VERSION vs package.json" drift states cannot @@ -313,35 +345,29 @@ function cmdClassify(args: string[], cwd: string): void { // file and itself. const jsonSource = isJsonVersionPath(versionRel); const pkgPath = jsonSource ? versionPath : resolvePkgPath(cwd, argVal(args, "--package-json-path")); - const pkg = jsonSource - ? { exists: existsSync(versionPath), version: current === DEFAULT ? "" : current } - : readPkgVersion(pkgPath); + const pkg = jsonSource ? { exists: true, version: current } : readPkgVersion(pkgPath); // Decision 11: the manifest carries the npm-valid 3-digit translation of // the 4-digit VERSION; drift is judged against the translated form. A // JSON version-path is its own source of truth, so its expected form is // the version itself. const expectedPkg = jsonSource ? current : npmVersion(current); const state = classifyState(current, baseV, pkg.exists, pkg.version, expectedPkg); - // Surface version-file absence so callers (and /ship) can tell "version is - // genuinely 0.0.0.0" from "we made up 0.0.0.0 because the file is missing" - // (#2600). Without this, the DRIFT_STALE_PKG dispatch on a missing VERSION - // would feed repair a fabricated version that passes the shape check. - const versionFileExists = existsSync(versionPath); process.stdout.write( JSON.stringify({ state, + versionSource: { outcome: src.outcome, path: versionRel, reason: null }, baseVersion: baseV, currentVersion: current, pkgVersion: pkg.version || null, pkgExists: pkg.exists, pkgPath: pkg.exists ? relative(cwd, pkgPath) : null, expectedPkgVersion: pkg.exists ? expectedPkg : null, - versionFileExists, + versionFileExists: true, }) + "\n", ); // DRIFT_UNEXPECTED is a real, decidable state — the caller stops on it, but the - // classification itself succeeded, so exit 0. (Bad args / unresolvable base are - // the only exit-2 cases.) + // classification itself succeeded, so exit 0. (Bad args / unresolvable base / + // a broken version source are the only exit-2 cases.) } /** @@ -383,7 +409,13 @@ function cmdWrite(args: string[], cwd: string): void { if (!VERSION_RE.test(version!)) { fail(`NEW_VERSION (${version}) does not match MAJOR.MINOR.PATCH[.MICRO]. Aborting.`, 2); } - const versionRel = resolveVersionRel(cwd, argVal(args, "--version-path")); + const src = versionSource(cwd, argVal(args, "--version-path")); + if (src.outcome === "broken") failBroken(src, "write"); + if (src.outcome !== "valid") { + // Never create a version file the project did not configure (#2334). + fail(`write: refusing to invent a version. ${noVersionNotice(src)}`, 2); + } + const versionRel = src.path!; const versionPath = join(cwd, versionRel); // A package.json version-path (#2501) is written in place, keeping the rest @@ -392,9 +424,6 @@ function cmdWrite(args: string[], cwd: string): void { // actually publishes from; in a monorepo whose truth is frontend/package.json // the root one either doesn't exist or isn't the version users see. if (isJsonVersionPath(versionRel)) { - if (!existsSync(versionPath)) { - fail(`write: ${versionRel} does not exist. Check --version-path / .gstack/version-path.`, 2); - } // Decision 11: a JSON manifest can only carry npm-valid semver. A repo // whose package.json still mirrors the legacy 4-digit form and pins it as // the version-path would otherwise get "1.67.0.1" written into a manifest diff --git a/docs/skills.md b/docs/skills.md index 5f41a888e..2e07952ce 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -694,6 +694,22 @@ Every `/ship` run builds a code path map from your diff, searches for correspond A lot of branches die when the interesting work is done and only the boring release work is left. Humans procrastinate that part. AI should not. +### Versions: where `/ship` looks, and when it ships without one + +`/ship` bumps a version only when the project says where the version lives. It checks, in order: + +1. `--version-path ` passed to `gstack-version-bump` / `gstack-next-version`. +2. `.gstack/version-path`: a committed one-line file holding the version file's repo-relative path, for example `package.json` or `apps/web/package.json`. A `.json` path is read and written as its `"version"` field; 3-digit semver stays 3-digit. +3. A root `VERSION` file (gstack's own 4-digit `MAJOR.MINOR.PATCH.MICRO` format). + +A root `package.json` on its own does not count, because many apps carry a placeholder there. To have `/ship` version it, run `echo package.json > .gstack/version-path` and commit the file. + +When none of these exist, `/ship` ships without a version change: no bump, no CHANGELOG version header, no `v1.2.3` title prefix, no tag. It prints: + +> Shipped without a version change: no version source is configured (no VERSION file, no .gstack/version-path). To version releases, create VERSION or write the version file's path (for example package.json) to .gstack/version-path. + +It does the same, with the reason, when release-please, Changesets or semantic-release is configured, when the repo is a workspace monorepo, or when `package.json` holds a placeholder such as `0.0.0-development`; those tools own the version. `/land-and-deploy` and `/document-release` read the same signal. A configured version file that is missing, empty, unreadable or malformed stops `/ship` with the file's path and the problem; gstack never substitutes `0.0.0.0`. + ### Third-party web actions (v1.72.0.0+) Sometimes the release work leaves the terminal: registering an API key, creating a vendor account, wiring a webhook or OAuth app. Instead of handing you a manual step list, `/ship` (and `/spec`, `/office-hours`, `/land-and-deploy`, `/setup-deploy`) offers to drive the browser for you. Aside first — it acts across your real logged-in sessions, which is exactly what vendor dashboards need. No Aside? gstack's own visible browser (headed `$B` with handoff for sign-in) is the fallback on every platform, with one pointer to aside.com (macOS 15+) per task. diff --git a/document-release/sections/release-body.md.tmpl b/document-release/sections/release-body.md.tmpl index e2c85ae83..72f138bd8 100644 --- a/document-release/sections/release-body.md.tmpl +++ b/document-release/sections/release-body.md.tmpl @@ -156,7 +156,15 @@ If TODOS.md does not exist, skip this step. **Ask before changing VERSION** — the version number is the user's release decision. -1. **If VERSION does not exist:** Skip silently. +1. **Read the version source** (the same signal `/ship` uses): + ```bash + bun run ~/.claude/skills/gstack/bin/gstack-version-bump classify --base + ``` + `state: NO_VERSION` means no version source is configured or release automation owns + it: print `VERSION: not applicable ()`, skip this step, and never + create VERSION. Exit 2 means a configured version file is broken: show stderr and + skip this step without guessing a version. Otherwise `versionSource.path` is the + version file (VERSION unless pinned); read it wherever this step says VERSION. 2. Check if VERSION was already modified on this branch: @@ -356,13 +364,14 @@ rmdir "" **PR/MR title sync (idempotent, always-on):** -PR titles must always start with `v` — same rule as `/ship`. If Step 8 bumped VERSION after `/ship` had already created the PR, the title is now stale. This sub-step fixes it. +PR titles must start with `v` whenever the project has a version source — same rule as `/ship`. If Step 8 bumped VERSION after `/ship` had already created the PR, the title is now stale. This sub-step fixes it. -Run this entire block in one shell, substituting `github` or `gitlab` for `` from Step 0. No variables cross tool calls. Missing VERSION or PR/MR skips title sync; an update failure warns and continues. +Run this entire block in one shell, substituting `github` or `gitlab` for `` from Step 0 and the base branch for ``. No variables cross tool calls. No version source (NO_VERSION) or no PR/MR skips title sync; an update failure warns and continues. ```bash -V=$(cat VERSION 2>/dev/null | tr -d '[:space:]') -[ -n "$V" ] || exit 0 +VB=$(bun run ~/.claude/skills/gstack/bin/gstack-version-bump classify --base ) || { echo "Title sync: not run (the version source is broken; see the error above)."; exit 0; } +V=$(echo "$VB" | jq -r '.currentVersion // empty') +[ -n "$V" ] || { echo "Title sync: not applicable (no version source is configured)."; exit 0; } case "" in github) CURRENT_TITLE=$(gh pr view --json title -q .title 2>/dev/null || true) ;; gitlab) CURRENT_TITLE=$(glab mr view -F json 2>/dev/null | jq -r '.title // empty') ;; diff --git a/land-and-deploy/SKILL.md.tmpl b/land-and-deploy/SKILL.md.tmpl index 0bdf56285..18b9a37c3 100644 --- a/land-and-deploy/SKILL.md.tmpl +++ b/land-and-deploy/SKILL.md.tmpl @@ -203,12 +203,16 @@ QUEUE_JSON=$(bun run ~/.claude/skills/gstack/bin/gstack-next-version \ --current-version "$BASE_VERSION" 2>/dev/null || echo '{"offline":true}') NEXT_SLOT=$(echo "$QUEUE_JSON" | jq -r '.version // empty') OFFLINE=$(echo "$QUEUE_JSON" | jq -r '.offline // false') +NO_VERSION=$(echo "$QUEUE_JSON" | jq -r '.no_version // false') ``` Use the existing conservative patch-level allocation; compare numeric version -components, not lexical strings. If this project has no VERSION, report this check -not applicable. A missing/unparseable version on only one side is unavailable, not green. +components, not lexical strings. A missing/unparseable version on only one side is +unavailable, not green. +0. `NO_VERSION=true` (no configured version source, the same signal `/ship` reads): + print `VERSION check: not applicable ()` and continue to + Step 3.5. Never expect a version-prefixed title, CHANGELOG version header or tag. 1. `OFFLINE=true`, helper failure or invalid result: report VERSION check unavailable with the reason; continue to Step 3.5. CI's version gate is the backstop. 2. `BRANCH_VERSION >= NEXT_SLOT`: no drift; continue. diff --git a/land-and-deploy/sections/readiness-gate.md.tmpl b/land-and-deploy/sections/readiness-gate.md.tmpl index e3ab67846..df742d514 100644 --- a/land-and-deploy/sections/readiness-gate.md.tmpl +++ b/land-and-deploy/sections/readiness-gate.md.tmpl @@ -185,7 +185,7 @@ git log --oneline "$BASE_SHA..$PR_HEAD" | head -20 Compare the PR body against the actual commits. Check for: 1. **Missing features** — commits that add significant functionality not mentioned in the PR 2. **Stale descriptions** — PR body mentions things that were later changed or reverted -3. **Wrong version** — PR title or body references a version that doesn't match VERSION file +3. **Wrong version** — PR title or body references a version that doesn't match the version file (under NO_VERSION, any version prefix is wrong) If the PR body looks stale or incomplete: **WARNING — PR body may not reflect current changes.** List what's missing or stale. @@ -205,7 +205,9 @@ git diff --name-only "$BASE_SHA...$PR_HEAD" -- README.md CHANGELOG.md ARCHITECTU If CHANGELOG.md and VERSION were NOT modified on this branch and the diff includes new features (new files, new commands, new skills): **WARNING — /document-release -likely not run. CHANGELOG and VERSION not updated despite new features.** +likely not run. CHANGELOG and VERSION not updated despite new features.** Skip the +VERSION half when Step 3.4 reported NO_VERSION: an unversioned project ships with no +version change by design. If only docs changed (no code): skip this check. diff --git a/lib/version-source.ts b/lib/version-source.ts index 17a745131..23ee6aa6a 100644 --- a/lib/version-source.ts +++ b/lib/version-source.ts @@ -32,6 +32,9 @@ // // Re-derived from PR #2501 by @YiftahR. +import { existsSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + export type Version = [number, number, number, number]; export type VersionWidth = 3 | 4; export type Bump = "major" | "minor" | "patch" | "micro"; @@ -128,3 +131,147 @@ export function setVersionInJson(raw: string, version: string): string { parsed.version = version; return JSON.stringify(parsed, null, 2) + "\n"; } + +// --------------------------------------------------------------------------- +// Where the version lives, as one of four outcomes (#2334, #2343). +// +// Most repos /ship runs in have no VERSION file. Folding "no file" into +// 0.0.0.0 made /ship invent a version (or stop on DRIFT_UNEXPECTED when a +// package.json disagreed with the invented zero). Every reader now asks this +// resolver instead and reads the same signal: +// +// valid a configured source holds a parsable version — use it. +// absent nothing configured — ship without a version change. +// ambiguous nothing configured, and release automation, a workspace +// monorepo or a placeholder version says the version is owned +// elsewhere — ship without a version change and say why. +// broken a configured source is missing, empty, unreadable or +// malformed — stop with the reason. Never substitute 0.0.0.0. +// +// "Configured" means --version-path, the .gstack/version-path pin, or a root +// VERSION file. A root package.json alone is not a configured source: pin it +// in .gstack/version-path to have /ship version it. + +export type VersionSourceOutcome = "valid" | "absent" | "ambiguous" | "broken"; + +export interface VersionSource { + outcome: VersionSourceOutcome; + /** Repo-relative path of the version file, or null when none is configured. */ + path: string | null; + pinnedBy: "--version-path" | ".gstack/version-path" | null; + /** The parsed version string (outcome "valid" only). */ + version: string | null; + /** Why the outcome is not "valid"; null when it is. */ + reason: string | null; +} + +export const NO_VERSION_NOTICE = + "Shipped without a version change: no version source is configured (no VERSION file, no .gstack/version-path). " + + "To version releases, create VERSION or write the version file's path (for example package.json) to .gstack/version-path."; + +/** The one line every no-version reader prints. Null when a version exists or the source is broken. */ +export function noVersionNotice(src: VersionSource): string | null { + if (src.outcome === "absent") return NO_VERSION_NOTICE; + if (src.outcome === "ambiguous") { + return ( + `Shipped without a version change: the version source is ambiguous (${src.reason}). ` + + "To have /ship version releases, write the version file's path (for example package.json) to .gstack/version-path." + ); + } + return null; +} + +/** + * The version file's repo-relative path and who chose it: + * --version-path, else the first line of .gstack/version-path, else VERSION. + * A blank pin file is no pin. `guard` runs on every configured path before it + * is read (gstack-version-bump's repo-containment check). + */ +export function resolveVersionRel( + repoRoot: string, + explicit?: string, + guard?: (rel: string, source: string) => void, +): { rel: string; pinnedBy: VersionSource["pinnedBy"] } { + if (explicit && explicit.trim()) { + const rel = explicit.trim(); + guard?.(rel, "--version-path"); + return { rel, pinnedBy: "--version-path" }; + } + const pin = join(repoRoot, ".gstack", "version-path"); + if (existsSync(pin)) { + const rel = readFileSync(pin, "utf-8").split("\n")[0]?.trim() ?? ""; + if (rel) { + guard?.(rel, ".gstack/version-path"); + return { rel, pinnedBy: ".gstack/version-path" }; + } + } + return { rel: "VERSION", pinnedBy: null }; +} + +function readJson(path: string): Record | null { + try { + const parsed = JSON.parse(readFileSync(path, "utf-8")); + return parsed && typeof parsed === "object" ? (parsed as Record) : null; + } catch { + return null; + } +} + +/** Why an unconfigured repo's version is owned by something other than /ship, or null. */ +function ambiguityReason(repoRoot: string): string | null { + const has = (rel: string) => existsSync(join(repoRoot, rel)); + const pkg = readJson(join(repoRoot, "package.json")); + const found = (names: string[]) => names.find(has); + const rp = found(["release-please-config.json", ".release-please-manifest.json"]); + if (rp) return `release-please manages versions here (${rp})`; + if (has(".changeset/config.json")) return "Changesets manages versions here (.changeset/config.json)"; + const sr = found([ + ".releaserc", ".releaserc.json", ".releaserc.yaml", ".releaserc.yml", ".releaserc.js", ".releaserc.cjs", + "release.config.js", "release.config.cjs", "release.config.mjs", + ]); + if (sr || (pkg && "release" in pkg)) return `semantic-release manages versions here (${sr ?? "package.json release"})`; + const ws = found(["pnpm-workspace.yaml", "lerna.json"]) ?? (pkg && "workspaces" in pkg ? "package.json workspaces" : undefined); + if (ws) return `this is a monorepo with no single version file (${ws})`; + const v = pkg?.version; + if (typeof v === "string" && /^0\.0\.0(?:-|$)/.test(v.trim())) { + return `package.json holds the placeholder version ${v.trim()}`; + } + return null; +} + +/** Resolve the version source to one of the four outcomes. Never throws for file problems. */ +export function resolveVersionSource( + repoRoot: string, + explicit?: string, + guard?: (rel: string, source: string) => void, +): VersionSource { + let resolved: ReturnType; + try { + resolved = resolveVersionRel(repoRoot, explicit, guard); + } catch (e) { + const code = (e as NodeJS.ErrnoException)?.code ?? "read failed"; + return { outcome: "broken", path: null, pinnedBy: ".gstack/version-path", version: null, reason: `.gstack/version-path is unreadable (${code})` }; + } + const { rel, pinnedBy } = resolved; + const abs = join(repoRoot, rel); + const by = pinnedBy ? ` (set by ${pinnedBy})` : ""; + const broken = (why: string): VersionSource => ({ outcome: "broken", path: rel, pinnedBy, version: null, reason: why + by }); + if (!existsSync(abs)) { + if (pinnedBy) return broken(`${rel} does not exist`); + const why = ambiguityReason(repoRoot); + return { outcome: why ? "ambiguous" : "absent", path: null, pinnedBy: null, version: null, reason: why ?? "no VERSION file and no .gstack/version-path" }; + } + let raw: string; + try { + raw = readFileSync(abs, "utf-8"); + } catch (e) { + return broken(`${rel} is unreadable (${(e as NodeJS.ErrnoException)?.code ?? "read failed"})`); + } + if (!raw.trim()) return broken(`${rel} is empty or contains no parsable version`); + const v = extractVersion(raw, rel); + if (!v || !parseVersion(v)) { + const shown = isJsonVersionPath(rel) ? (v ? `"version": "${v}"` : "no \"version\" string") : `"${raw.trim().slice(0, 40)}"`; + return broken(`${rel} contains no parsable version (found ${shown})`); + } + return { outcome: "valid", path: rel, pinnedBy, version: v, reason: null }; +} diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index a8987b5c6..776f69586 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -242,6 +242,14 @@ chooses it in item 2 and ALREADY_BUMPED derives it in item 1. Repair alone never re-bumps. - **DRIFT_UNEXPECTED** → STOP: package.json disagrees with VERSION while VERSION matches base. Reconcile the manual edit, then reclassify. + - **NO_VERSION** → the repo has no configured version source, or release automation, + a monorepo or a placeholder owns it (`versionSource.reason`). Print `notice` + verbatim and ship without a version change: skip the rest of Step 12 and Step 13's + CHANGELOG entry, never create VERSION, and use an unprefixed title in Step 18. + `NEW_VERSION` stays unset; Step 20 logs `"version":null`. + - **Exit 2** → STOP and show stderr. A configured version file (`--version-path`, + `.gstack/version-path` or VERSION) that is missing, empty, unreadable or malformed + names its path; fix it or the pin, then reclassify. Never substitute `0.0.0.0`. 2. **Decide the bump level** from the diff (agent judgment): - **MICRO**: <50 lines, trivial tweaks/config. **PATCH**: 50+ lines, no feature signals. @@ -593,6 +601,8 @@ Prepare the title from that result; Step 19 scans and publishes it: 2. For a new PR/MR, compose `v :

`. 3. Save the result as `NEW_TITLE` for Step 19. Every created or updated title MUST start with `v$NEW_VERSION `; never publish an unprefixed title. +4. **NO_VERSION (Step 12):** replaces items 1-3. Keep an existing title as it is, or + compose `: ` for a new PR/MR, with no version prefix. {{SECTION:pr-body}} @@ -613,7 +623,7 @@ Substitute from earlier steps: - **PLAN_TOTAL**: total plan items extracted in Step 8 (0 if no plan file) - **PLAN_DONE**: count of DONE + CHANGED items from Step 8 (0 if no plan file) - **VERIFY_RESULT**: "pass", "fail", or "skipped", set after Step 9 executes Step 8.1's verification list -- **VERSION**: from the VERSION file +- **VERSION**: `NEW_VERSION` from Step 12, written as `null` (unquoted) under NO_VERSION The shell supplies the branch. Run this automatically, without confirmation. @@ -654,5 +664,6 @@ hand-roll VERSION/package.json writes. Follow the numbered gates and their explicit exceptions. - **Never force push.** Use regular `git push` only. -- **Always use the 4-digit version format** from the VERSION file. +- **Use the version format of the configured version file** (4-digit for VERSION). Under + NO_VERSION, never invent one. - **Step 7 generates coverage tests.** They must pass before committing. Never commit failing tests. diff --git a/ship/sections/changelog.md.tmpl b/ship/sections/changelog.md.tmpl index 066c1d1b3..884d202a0 100644 --- a/ship/sections/changelog.md.tmpl +++ b/ship/sections/changelog.md.tmpl @@ -1,3 +1,7 @@ {{CHANGELOG_WORKFLOW}} +**NO_VERSION (Step 12):** skip this step. Write no CHANGELOG entry or version header; +the project's release automation, or its maintainers, own release notes. Step 12's +notice already told the user. + --- diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index b779640c2..259b3fdf4 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -154,7 +154,7 @@ Update the title with the same scanned `NEW_TITLE`: `gh pr edit --title "$NEW_TI **REST fallback:** if `gh pr edit` fails with the `repository.pullRequest.projectCards` GraphQL deprecation, do not re-ask for auth. Use the SAME scanned file: `PR_NUMBER=$(gh pr view --json number -q .number)`, then `gh api "repos/{owner}/{repo}/pulls/$PR_NUMBER" -X PATCH -F body=@"$PR_BODY_FILE"`; for the title use `gh api "repos/{owner}/{repo}/pulls/$PR_NUMBER" -X PATCH -f title="$NEW_TITLE"`. -**Self-check:** re-fetch the title and assert it starts with `v$NEW_VERSION `. Retry once if wrong, then surface any failure. Print the existing URL and continue to Step 20; do not run the create commands below. +**Self-check:** re-fetch the title and assert it equals `NEW_TITLE` (it starts with `v$NEW_VERSION ` unless Step 12 reported NO_VERSION). Retry once if wrong, then surface any failure. Print the existing URL and continue to Step 20; do not run the create commands below. **No open PR/MR, GitHub:** diff --git a/test/gstack-next-version.test.ts b/test/gstack-next-version.test.ts index 0eea3a538..9c8901318 100644 --- a/test/gstack-next-version.test.ts +++ b/test/gstack-next-version.test.ts @@ -931,6 +931,7 @@ describe("width pinned on failed base read (3-digit repos)", () => { }); describe("integration (smoke)", () => { + const SMOKE_SCRIPT = join(import.meta.dir, "..", "bin", "gstack-next-version"); // Bumps timeout to 30s — the test spawns a real `bun run` subprocess that // does a `gh pr list` against the live GitHub API to inspect claimed slots. // Network latency makes 5s tight on developer machines. @@ -963,24 +964,39 @@ describe("integration (smoke)", () => { }, 30_000); // Headroom over the 4-5s wall time of the spawned process under load test("CLI runs with --version-path and surfaces it in JSON output", async () => { - const proc = Bun.spawnSync([ - "bun", - "run", - "./bin/gstack-next-version", - "--base", - "main", - "--bump", - "patch", - "--current-version", - "1.6.3.0", - "--workspace-root", - "null", - "--version-path", - "Tinas Second Brain/health-tracker/VERSION", - ], { timeout: 30_000 }); - const out = new TextDecoder().decode(proc.stdout); - const parsed = JSON.parse(out); - expect(parsed).toHaveProperty("version_path", "Tinas Second Brain/health-tracker/VERSION"); + // G1 (#2334): an explicit --version-path must now exist (a missing pinned + // file stops instead of reading as 0.0.0.0), so the pinned path is created + // in a scratch repo instead of pointing at nothing inside this checkout. + const dir = mkdtempSync(join(tmpdir(), "nextver-vpath-")); + const stubDir = mkdtempSync(join(tmpdir(), "nextver-vpath-stub-")); + try { + writeFileSync(join(stubDir, "gh"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); + writeFileSync(join(stubDir, "glab"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); + Bun.spawnSync(["git", "init", "-q", "-b", "main"], { cwd: dir, timeout: 30_000 }); + mkdirSync(join(dir, "Tinas Second Brain", "health-tracker"), { recursive: true }); + writeFileSync(join(dir, "Tinas Second Brain", "health-tracker", "VERSION"), "1.6.3.0\n"); + const proc = Bun.spawnSync([ + "bun", + "run", + SMOKE_SCRIPT, + "--base", + "main", + "--bump", + "patch", + "--current-version", + "1.6.3.0", + "--workspace-root", + "null", + "--version-path", + "Tinas Second Brain/health-tracker/VERSION", + ], { cwd: dir, env: { ...process.env, PATH: `${stubDir}:${process.env.PATH}` }, timeout: 30_000 }); + const out = new TextDecoder().decode(proc.stdout); + const parsed = JSON.parse(out); + expect(parsed).toHaveProperty("version_path", "Tinas Second Brain/health-tracker/VERSION"); + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(stubDir, { recursive: true, force: true }); + } }, 30_000); }); @@ -1086,3 +1102,63 @@ describe("fetchGitClaimed — laundered ls-remote (exit 0, empty output) is neve } }); }); + +// G1 (#2334, #2343): gstack-next-version reads the same no-version signal as +// gstack-version-bump. Before, a repo with no VERSION file got a version +// allocated from an assumed 0.0.0.0 base, and a broken pin did the same. +describe("no version source: one signal, never an invented slot (G1)", () => { + const SCRIPT = join(import.meta.dir, "..", "bin", "gstack-next-version"); + + function runIn(files: Record, extra: string[] = []) { + const dir = mkdtempSync(join(tmpdir(), "nextver-nover-")); + const stubDir = mkdtempSync(join(tmpdir(), "nextver-nover-stub-")); + try { + writeFileSync(join(stubDir, "gh"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); + writeFileSync(join(stubDir, "glab"), "#!/bin/sh\nexit 1\n", { mode: 0o755 }); + Bun.spawnSync(["git", "init", "-q", "-b", "main"], { cwd: dir, timeout: 30_000 }); + for (const [rel, body] of Object.entries(files)) { + mkdirSync(join(dir, rel, ".."), { recursive: true }); + writeFileSync(join(dir, rel), body); + } + const proc = Bun.spawnSync( + ["bun", "run", SCRIPT, "--base", "main", "--bump", "patch", "--workspace-root", "null", ...extra], + { cwd: dir, env: { ...process.env, PATH: `${stubDir}:${process.env.PATH}` }, timeout: 30_000 }, + ); + return { + code: proc.exitCode, + stdout: new TextDecoder().decode(proc.stdout), + stderr: new TextDecoder().decode(proc.stderr), + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(stubDir, { recursive: true, force: true }); + } + } + + test("a versionless app reports no_version and allocates nothing", () => { + const r = runIn({ "package.json": JSON.stringify({ name: "app", version: "2.3.4" }) }, ["--current-version", "0.0.0.0"]); + expect(r.code).toBe(0); + const out = JSON.parse(r.stdout); + expect(out.no_version).toBe(true); + expect(out.version).toBeNull(); + expect(out.version_source.outcome).toBe("absent"); + expect(out.notice).toContain("Shipped without a version change"); + }, 30_000); + + test("a release-please repo is ambiguous: no slot, reason named", () => { + const r = runIn({ "release-please-config.json": "{}", "package.json": JSON.stringify({ name: "a", version: "1.0.0" }) }); + expect(r.code).toBe(0); + const out = JSON.parse(r.stdout); + expect(out.version).toBeNull(); + expect(out.version_source.outcome).toBe("ambiguous"); + expect(out.version_source.reason).toContain("release-please"); + }, 30_000); + + test("a broken pin stops with the path instead of assuming 0.0.0.0", () => { + const r = runIn({ ".gstack/version-path": "apps/web/package.json\n" }); + expect(r.code).toBe(2); + expect(r.stdout).toBe(""); + expect(r.stderr).toContain("apps/web/package.json does not exist"); + expect(r.stderr).not.toContain("assuming"); + }, 30_000); +}); diff --git a/test/gstack-version-bump.test.ts b/test/gstack-version-bump.test.ts index 2c8a55835..74fcbdc31 100644 --- a/test/gstack-version-bump.test.ts +++ b/test/gstack-version-bump.test.ts @@ -737,9 +737,11 @@ describe('#2600: classify must surface versionFileExists=false when VERSION is m const out = execFileSync('bun', [BIN, 'classify', '--base', 'main'], { cwd: dir, timeout: 30_000 }).toString(); const result = JSON.parse(out); + // G1 (#2334/#2343) deliberately replaced the fabricated 0.0.0.0 / FRESH + // classification: no configured version source is its own state. expect(result.versionFileExists).toBe(false); - expect(result.currentVersion).toBe('0.0.0.0'); // fabricated default - expect(result.state).toBe('FRESH'); // base also reads 0.0.0.0, no pkg drift + expect(result.currentVersion).toBeNull(); + expect(result.state).toBe('NO_VERSION'); }); test('classify reports versionFileExists=true when VERSION is present', () => { @@ -841,3 +843,122 @@ describe('write --regen-digest regenerates the gstack agents digest (explicit op fs.rmSync(dir, { recursive: true, force: true }); }); }); + +/** + * G1 (#2334, #2343): four resolution outcomes. Most repos /ship runs in have + * no VERSION file. Before, classify folded "no file" into 0.0.0.0 — a repo + * with a real package.json version then hit DRIFT_UNEXPECTED (VERSION "equals" + * base at 0.0.0.0 while package.json disagrees) and /ship stopped, and a repo + * without one classified FRESH and had a VERSION file invented. A pinned file + * that was missing or malformed read as 0.0.0.0 too. + */ +describe('G1: version source outcomes (absent / valid / ambiguous / broken)', () => { + const dirs: string[] = []; + afterAll(() => { + for (const d of dirs) { try { fs.chmodSync(d, 0o755); fs.rmSync(d, { recursive: true, force: true }); } catch { /* noop */ } } + }); + + function repo(files: Record): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-g1-')); + dirs.push(dir); + const git = (...a: string[]) => execFileSync('git', a, { cwd: dir, stdio: 'pipe', timeout: 30_000 }); + git('init', '-q', '-b', 'main'); + git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); + fs.writeFileSync(path.join(dir, 'README.md'), 'x\n'); + for (const [rel, body] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(dir, rel)), { recursive: true }); + fs.writeFileSync(path.join(dir, rel), body); + } + git('add', '-A'); git('commit', '-q', '-m', 'base'); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: dir, timeout: 30_000 }).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; + } + + function run(dir: string, args: string[]): { code: number; stdout: string; stderr: string } { + try { + const stdout = execFileSync('bun', [BIN, ...args], { cwd: dir, stdio: 'pipe', timeout: 30_000 }).toString(); + return { code: 0, stdout, stderr: '' }; + } catch (e: any) { + return { code: e.status, stdout: String(e.stdout ?? ''), stderr: String(e.stderr ?? '') }; + } + } + + const NOTICE = + 'Shipped without a version change: no version source is configured (no VERSION file, no .gstack/version-path). ' + + "To version releases, create VERSION or write the version file's path (for example package.json) to .gstack/version-path."; + + test('versionless app with a real package.json version: NO_VERSION + exact notice, not DRIFT_UNEXPECTED', () => { + const dir = repo({ 'package.json': JSON.stringify({ name: 'app', version: '2.3.4' }, null, 2) + '\n' }); + const r = run(dir, ['classify', '--base', 'main']); + expect(r.code).toBe(0); + const out = JSON.parse(r.stdout); + expect(out.state).toBe('NO_VERSION'); + expect(out.versionSource.outcome).toBe('absent'); + expect(out.notice).toBe(NOTICE); + expect(out.currentVersion).toBeNull(); + expect(out.baseVersion).toBeNull(); + }); + + test('write in a versionless repo refuses and never creates VERSION', () => { + const dir = repo({ 'package.json': JSON.stringify({ name: 'app', version: '2.3.4' }, null, 2) + '\n' }); + const r = run(dir, ['write', '--version', '0.0.1.0']); + expect(r.code).toBe(2); + expect(r.stderr).toContain('Shipped without a version change'); + expect(fs.existsSync(path.join(dir, 'VERSION'))).toBe(false); + expect(JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf-8')).version).toBe('2.3.4'); + }); + + const ambiguous: Array<[string, Record, string]> = [ + ['monorepo (pnpm workspace)', { 'pnpm-workspace.yaml': 'packages:\n - apps/*\n', 'apps/web/package.json': '{"name":"web","version":"1.0.0"}' }, 'monorepo'], + ['monorepo (package.json workspaces)', { 'package.json': JSON.stringify({ name: 'root', private: true, workspaces: ['packages/*'] }) }, 'monorepo'], + ['release-please', { 'release-please-config.json': '{}', '.release-please-manifest.json': '{".":"1.4.0"}', 'package.json': '{"name":"a","version":"1.4.0"}' }, 'release-please'], + ['semantic-release placeholder', { 'package.json': JSON.stringify({ name: 'a', version: '0.0.0-development' }) }, 'placeholder'], + ]; + for (const [name, files, reason] of ambiguous) { + test(`ambiguous: ${name} ships without a version and says why`, () => { + const dir = repo(files); + const r = run(dir, ['classify', '--base', 'main']); + expect(r.code).toBe(0); + const out = JSON.parse(r.stdout); + expect(out.state).toBe('NO_VERSION'); + expect(out.versionSource.outcome).toBe('ambiguous'); + expect(out.versionSource.reason).toContain(reason); + expect(out.notice).toContain('Shipped without a version change'); + expect(out.notice).toContain(out.versionSource.reason); + }); + } + + test('valid: a package.json pinned in .gstack/version-path is used', () => { + const dir = repo({ + '.gstack/version-path': 'package.json\n', + 'package.json': JSON.stringify({ name: 'app', version: '2.3.4' }, null, 2) + '\n', + }); + const out = JSON.parse(run(dir, ['classify', '--base', 'main']).stdout); + expect(out.state).toBe('FRESH'); + expect(out.versionSource.outcome).toBe('valid'); + expect(out.currentVersion).toBe('2.3.4'); + }); + + const brokenPins: Array<[string, Record, string]> = [ + ['missing', { '.gstack/version-path': 'apps/web/package.json\n' }, 'apps/web/package.json does not exist'], + ['empty', { '.gstack/version-path': 'VERSION.txt\n', 'VERSION.txt': '\n' }, 'VERSION.txt is empty'], + ['malformed JSON', { '.gstack/version-path': 'package.json\n', 'package.json': '{ not json' }, 'package.json contains no parsable version'], + ['malformed text', { 'VERSION': 'release-candidate\n' }, 'VERSION contains no parsable version'], + ['unreadable (a directory)', { '.gstack/version-path': 'ver\n', 'ver/keep': '' }, 'ver is unreadable'], + ]; + for (const [name, files, reason] of brokenPins) { + test(`broken pin (${name}) stops with the path and never reads as 0.0.0.0`, () => { + const dir = repo(files); + const r = run(dir, ['classify', '--base', 'main']); + expect(r.code).toBe(2); + expect(r.stdout).toBe(''); + expect(r.stderr).toContain(reason); + expect(r.stderr).toContain('never substitutes 0.0.0.0'); + const w = run(dir, ['write', '--version', '1.0.0.0']); + expect(w.code).toBe(2); + expect(w.stderr).toContain(reason); + }); + } +}); diff --git a/test/ship-version-sync.test.ts b/test/ship-version-sync.test.ts index 223b46454..00356a670 100644 --- a/test/ship-version-sync.test.ts +++ b/test/ship-version-sync.test.ts @@ -253,3 +253,57 @@ test("DRIFT REPAIR: sync path syncs pkg to VERSION without re-bumping", () => { expect(readFileSync(join(dir, "VERSION"), "utf8").trim()).toBe("0.1.0.0"); expect(pkgVersion()).toBe("0.1.0.0"); }); + +// G1 (#2334, #2343): the generated /ship flow, driven against the real +// classifier on a versionless app, a monorepo, a release-please repo and a +// broken pin. Every state the CLI emits must have a dispatch arm, and the +// NO_VERSION arm must carry through to the CHANGELOG and title steps. +test("rendered /ship handles every version-source outcome the classifier emits", () => { + const ship = readFileSync(join(import.meta.dir, "../ship/SKILL.md"), "utf8"); + const changelog = readFileSync(join(import.meta.dir, "../ship/sections/changelog.md"), "utf8"); + const step12 = ship.slice(ship.indexOf("## Step 12:"), ship.indexOf("## Step 14:")).replace(/\s+/g, " "); + const step18 = ship.slice(ship.indexOf("Prepare the title from that result"), ship.indexOf("## Step 20:")).replace(/\s+/g, " "); + const classifyCmd = step12.match(/bun run \S+gstack-version-bump classify --base /); + expect(classifyCmd).not.toBeNull(); + const bin = join(import.meta.dir, "../bin/gstack-version-bump"); + const fixtures: Record> = { + versionless: { "package.json": pkgJson("2.3.4") }, + monorepo: { "pnpm-workspace.yaml": "packages: []\n" }, + releasePlease: { "release-please-config.json": "{}" }, + brokenPin: { ".gstack-pin": "" }, + }; + for (const [name, files] of Object.entries(fixtures)) { + const repo = mkdtempSync(join(tmpdir(), `ship-g1-${name}-`)); + try { + for (const [rel, body] of Object.entries(files)) writeFileSync(join(repo, rel), body); + if (name === "brokenPin") { + execSync("mkdir -p .gstack && echo missing/VERSION > .gstack/version-path", { cwd: repo, shell: "/bin/bash", timeout: 10_000 }); + } + execSync("git init -q -b main && git -c user.email=t@t -c user.name=t add -A && git -c user.email=t@t -c user.name=t commit -qm base && git update-ref refs/remotes/origin/main HEAD", { cwd: repo, shell: "/bin/bash", timeout: 30_000 }); + let state = ""; + let code = 0; + try { + state = JSON.parse(execSync(`bun "${bin}" classify --base main`, { cwd: repo, stdio: "pipe", timeout: 30_000 }).toString()).state; + } catch (e: any) { + code = e.status; + } + if (name === "brokenPin") { + expect(code).toBe(2); + expect(step12).toMatch(/\*\*Exit 2\*\* → STOP[^.]*stderr/); + expect(step12).toContain("Never substitute `0.0.0.0`"); + } else { + expect(state).toBe("NO_VERSION"); + expect(step12).toContain(`**${state}** →`); + } + } finally { + rmSync(repo, { recursive: true, force: true }); + } + } + const arm = step12.slice(step12.indexOf("**NO_VERSION** →"), step12.indexOf("**Exit 2** →")); + expect(arm).toContain("`notice`"); + expect(arm).toMatch(/skip the rest of Step 12 and Step 13/); + expect(arm).toContain("never create VERSION"); + expect(changelog).toMatch(/NO_VERSION[^\n]*skip this step/); + expect(step18).toMatch(/NO_VERSION[^.]*replaces items 1-3/); + expect(step18).toContain("no version prefix"); +}); From bd1d7d0d345e7f789aaf4f151610e2ceb61ee9c7 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:45:40 +0000 Subject: [PATCH 02/20] F4: keep every paid design variant across regenerate rounds $D variants now reserves a collision-safe round-/ directory per run (atomic mkdir) and writes a manifest.json of what each round saved. The paid image request and the local write retry separately: a write failure retries the write (temp + rename) and, if the path stays unwritable, keeps the bytes in the temp dir instead of buying the image again. Responsive variants land in their round directory too. Readers updated: design-shotgun reserves a round dir and a private temp dir for its parallel agents and records approved_path/round in approved.json; design-html resolves approved_path (legacy letters still work) and finds round-dir variants; design-consultation checks the printed paths; the gallery shows round-dir variants. Variants that were already overwritten before this change cannot be recovered. Migrates the consultation contract test that pinned the flat path. Co-authored-by: Matt Van Horn --- .../sections/proposal-and-preview.md.tmpl | 6 +- design-html/SKILL.md.tmpl | 7 +- design-shotgun/SKILL.md.tmpl | 48 +++-- design/src/gallery.ts | 15 +- design/src/variants.ts | 172 ++++++++++++++---- design/test/gallery.test.ts | 9 + design/test/variant-rounds.test.ts | 108 +++++++++++ ...sign-consultation-command-contract.test.ts | 3 +- 8 files changed, 310 insertions(+), 58 deletions(-) create mode 100644 design/test/variant-rounds.test.ts diff --git a/design-consultation/sections/proposal-and-preview.md.tmpl b/design-consultation/sections/proposal-and-preview.md.tmpl index f76f9fcb0..dd5a683cd 100644 --- a/design-consultation/sections/proposal-and-preview.md.tmpl +++ b/design-consultation/sections/proposal-and-preview.md.tmpl @@ -114,10 +114,12 @@ Brief: Phase 3 aesthetic/colors/type/spacing/layout plus Phase 1 product context $D variants --brief "" --count 3 --output-dir "$_DESIGN_DIR/" ``` -Run quality check on each successful path returned by `variants`; never include failed variants: +`variants` writes into a new `round-/` under the output dir and prints `roundDir`, +`manifest` and `paths` (saved images only). Run the quality check on each of those +`paths`; never include failed variants: ```bash -$D check --image "$_DESIGN_DIR/variant-A.png" --brief "" +$D check --image "" --brief "" ``` Read JSON, not exit code: `pass: false` means regenerate addressing `issues`, then recheck. `pass: true` with an unavailable/skipped warning is missing automated coverage; disclose it and inspect visually. diff --git a/design-html/SKILL.md.tmpl b/design-html/SKILL.md.tmpl index 6d97772c6..cc486a896 100644 --- a/design-html/SKILL.md.tmpl +++ b/design-html/SKILL.md.tmpl @@ -71,7 +71,7 @@ GSTACK_STATE_ROOT=$(~/.claude/skills/gstack/bin/gstack-paths --get GSTACK_STATE_ setopt +o nomatch 2>/dev/null || true _APPROVED=$(ls -t "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/approved.json 2>/dev/null | head -1) [ -n "$_APPROVED" ] && echo "APPROVED: $_APPROVED" || echo "NO_APPROVED" -_VARIANTS=$(ls -t "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/variant-*.png 2>/dev/null | head -1) +_VARIANTS=$(ls -t "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/variant-*.png "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/round-*/variant-*.png 2>/dev/null | head -1) [ -n "$_VARIANTS" ] && echo "VARIANTS: $_VARIANTS" || echo "NO_VARIANTS" _FINALIZED=$(ls -t "$GSTACK_STATE_ROOT/projects/$SLUG/designs/"*/finalized.html 2>/dev/null | head -1) [ -n "$_FINALIZED" ] && echo "FINALIZED: $_FINALIZED" || echo "NO_FINALIZED" @@ -83,7 +83,10 @@ Now route based on what was found. Check these cases in order: ### Case A: approved.json exists (design-shotgun ran) If `APPROVED` was found, read it. Extract: approved variant PNG path, user feedback, -screen name. Also read the CEO plan if one exists (it adds strategic context). +screen name. The PNG path is `approved_path` when present (it may sit in a `round-/` +directory); older files name only a letter in `approved_variant`, meaning +`variant-.png` beside approved.json. If that file is missing, say so and ask +which variant to use instead of picking the newest PNG. Also read the CEO plan if one exists (it adds strategic context). Read `DESIGN.md` if it exists in the repo root. These tokens take priority for system-level values (fonts, brand colors, spacing scale). diff --git a/design-shotgun/SKILL.md.tmpl b/design-shotgun/SKILL.md.tmpl index ac3d1ecdd..70c26e583 100644 --- a/design-shotgun/SKILL.md.tmpl +++ b/design-shotgun/SKILL.md.tmpl @@ -264,6 +264,24 @@ variant (parallel foreground calls in one message still run concurrently, and th comparison board needs every variant's result). Each agent is independent and handles its own generation, quality check, verification, and retry. +**Reserve this round's directory first.** Every generation round, including each +regenerate round, writes to its own `round-/` so earlier paid variants are never +overwritten. Substitute Step 3's `DESIGN_DIR`: + +```bash +_DESIGN_DIR="" +mkdir -p "$_DESIGN_DIR" || exit 1 +_N=1 +until mkdir "$_DESIGN_DIR/round-$_N" 2>/dev/null; do + _N=$((_N + 1)); [ "$_N" -le 999 ] || { echo "ROUND_DIR_FAILED: $_DESIGN_DIR"; exit 1; } +done +_ROUND_TMP=$(mktemp -d "${TMPDIR:-/tmp}/gstack-variants.XXXXXX") || exit 1 +echo "ROUND_DIR: $_DESIGN_DIR/round-$_N" +echo "ROUND_TMP: $_ROUND_TMP" +``` + +Substitute the printed `ROUND_DIR` and `ROUND_TMP` into every agent prompt below. + **Important: $D path propagation.** The `$D` variable from DESIGN SETUP is a shell variable that agents do NOT inherit. Substitute the resolved absolute path (from the `DESIGN_READY: /path/to/design` output in Step 0) into each agent prompt. @@ -275,18 +293,18 @@ Generate a design variant and save it. Design binary: {absolute path to $D binary} Brief: {the full variant-specific brief for this direction} -Output: /tmp/variant-{letter}.png -Final location: {_DESIGN_DIR absolute path}/variant-{letter}.png +Output: {ROUND_TMP}/variant-{letter}.png +Final location: {ROUND_DIR}/variant-{letter}.png Steps: -1. Run: {$D path} generate --brief "{brief}" --output /tmp/variant-{letter}.png +1. Run: {$D path} generate --brief "{brief}" --output {ROUND_TMP}/variant-{letter}.png 2. If the command fails with a rate limit error (429 or "rate limit"), wait 5 seconds and retry. Up to 3 retries. 3. If the output file is missing or empty after the command succeeds, retry once. -4. Copy: cp /tmp/variant-{letter}.png {_DESIGN_DIR}/variant-{letter}.png -5. Quality check: {$D path} check --image {_DESIGN_DIR}/variant-{letter}.png --brief "{brief}" +4. Copy: cp {ROUND_TMP}/variant-{letter}.png {ROUND_DIR}/variant-{letter}.png +5. Quality check: {$D path} check --image {ROUND_DIR}/variant-{letter}.png --brief "{brief}" If quality check fails, retry generation once. -6. Verify: ls -lh {_DESIGN_DIR}/variant-{letter}.png +6. Verify: ls -lh {ROUND_DIR}/variant-{letter}.png 7. Report exactly one of: VARIANT_{letter}_DONE: {file size} VARIANT_{letter}_FAILED: {error description} @@ -295,11 +313,12 @@ Steps: For the evolve path, replace step 1 with: ``` -{$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output /tmp/variant-{letter}.png +{$D path} evolve --screenshot {_DESIGN_DIR}/current.png --brief "{brief}" --output {ROUND_TMP}/variant-{letter}.png ``` -**Generate to `/tmp/`, then `cp`:** in sandboxed sessions `$D generate --output` under -`~/.gstack/` can abort ("The operation was aborted"), while `/tmp/` works. +**Generate to `ROUND_TMP`, then `cp`:** in sandboxed sessions `$D generate --output` under +`~/.gstack/` can abort ("The operation was aborted"), while the temp dir works. `ROUND_TMP` +is private to this round, so concurrent sessions never share a file. ### Step 3d: Results @@ -319,10 +338,12 @@ image list from whatever variant files actually exist, not a hardcoded A/B/C lis ```bash setopt +o nomatch 2>/dev/null || true # zsh compat -_IMAGES=$(ls "$_DESIGN_DIR"/variant-*.png 2>/dev/null | tr '\n' ',' | sed 's/,$//') +_IMAGES=$(ls ""/variant-*.png 2>/dev/null | tr '\n' ',' | sed 's/,$//') ``` -Use `$_IMAGES` in the `$D compare --images` command. +Use `$_IMAGES` in the `$D compare --images` command. A regenerate round run through +`$D variants --output-dir "$_DESIGN_DIR/"` reserves the next `round-/` itself and +prints `roundDir`, `manifest` and the saved `paths`; build that round's board from `paths`. ## Step 4: Comparison Board + Feedback Loop @@ -346,7 +367,10 @@ Use AskUserQuestion to confirm before saving. ## Step 6: Save & Next Steps -Write `approved.json` to `$_DESIGN_DIR/` (handled by the loop above). +Write `approved.json` to `$_DESIGN_DIR/` (handled by the loop above). Also record +`approved_path`, the absolute path of the chosen PNG, and its `round`. The choice may +come from an earlier round (for example round 1's B after round 2); earlier rounds stay +on disk, and `round-/manifest.json` (from `$D variants`) lists what each round saved. If invoked from another skill: return the structured feedback for that skill to consume. The calling skill reads `approved.json` and the approved variant PNG. diff --git a/design/src/gallery.ts b/design/src/gallery.ts index 95675559d..e6defaf96 100644 --- a/design/src/gallery.ts +++ b/design/src/gallery.ts @@ -44,13 +44,18 @@ export function generateGalleryHtml(designsDir: string): string { } } - // Find variant PNGs + // Find variant PNGs: legacy flat files plus every round-/ directory + // (`$D variants` writes each regenerate round to its own directory, #1529). const variants: string[] = []; try { - const files = fs.readdirSync(sessionDir); - for (const f of files) { - if (f.match(/variant-[A-Z]\.png$/i) || f.match(/variant-\d+\.png$/i)) { - variants.push(path.join(sessionDir, f)); + const dirs = [sessionDir, ...fs.readdirSync(sessionDir) + .filter(f => /^round-\d+$/.test(f)) + .map(f => path.join(sessionDir, f))]; + for (const dir of dirs) { + for (const f of fs.readdirSync(dir)) { + if (f.match(/variant-[A-Z]\.png$/i) || f.match(/variant-\d+\.png$/i)) { + variants.push(path.join(dir, f)); + } } } variants.sort(); diff --git a/design/src/variants.ts b/design/src/variants.ts index 5042da98c..4fe2fda14 100644 --- a/design/src/variants.ts +++ b/design/src/variants.ts @@ -2,9 +2,16 @@ * Generate N design variants from a brief. * Uses staggered parallel: 1s delay between API calls to avoid rate limits. * Falls back to exponential backoff on 429s. + * + * Every run writes into its own round directory (/round-/, + * reserved atomically) with a manifest.json of what was saved, so a + * regenerate round never overwrites the images an earlier round paid for + * (#1529). The paid request and the local write retry separately: a disk + * error retries the write, never the purchase. */ import fs from "fs"; +import os from "os"; import path from "path"; import { requireApiKey } from "./auth"; import { receiptedFetch } from "./receipted-fetch"; @@ -50,7 +57,7 @@ export async function generateVariant( size: string, quality: string, fetchFn: typeof globalThis.fetch = globalThis.fetch, -): Promise<{ path: string; success: boolean; error?: string }> { +): Promise { const maxRetries = 3; const MAX_RETRY_AFTER_MS = 60_000; // cap honored Retry-After to bound stalls let lastError = ""; @@ -130,8 +137,7 @@ export async function generateVariant( return { path: outputPath, success: false, error: "No image data in response" }; } - fs.writeFileSync(outputPath, Buffer.from(imageItem.result, "base64")); - return { path: outputPath, success: true }; + return saveVariantImage(outputPath, Buffer.from(imageItem.result, "base64")); } catch (err: any) { clearTimeout(timeout); if (err.name === "AbortError") { @@ -144,6 +150,94 @@ export async function generateVariant( return { path: outputPath, success: false, error: lastError }; } +export interface VariantResult { + path: string; + success: boolean; + error?: string; + /** Where a paid image was kept when the output path could not be written. */ + rescuedPath?: string; +} + +/** + * Write an image that was already paid for. Retries the local write (temp + * file + rename) and, if the output path stays unwritable, keeps the bytes in + * the system temp dir instead of dropping them. Never re-requests the image. + */ +export function saveVariantImage(outputPath: string, bytes: Buffer, attempts = 3): VariantResult { + let lastError = ""; + for (let i = 0; i < attempts; i++) { + const tmp = `${outputPath}.${process.pid}.${i}.tmp`; + try { + fs.writeFileSync(tmp, bytes); + fs.renameSync(tmp, outputPath); + return { path: outputPath, success: true }; + } catch (err: any) { + lastError = err.message; + try { fs.rmSync(tmp, { force: true }); } catch { /* best effort */ } + } + } + const error = `image was generated but could not be saved to ${outputPath} (${lastError})`; + try { + const rescueDir = fs.mkdtempSync(path.join(os.tmpdir(), "gstack-variant-rescue-")); + const rescuedPath = path.join(rescueDir, path.basename(outputPath)); + fs.writeFileSync(rescuedPath, bytes); + return { path: outputPath, success: false, error: `${error}; kept at ${rescuedPath}`, rescuedPath }; + } catch { + return { path: outputPath, success: false, error }; + } +} + +export interface RoundManifestEntry { + label: string; + file: string; + status: "saved" | "failed"; + error?: string; + rescuedPath?: string; +} + +/** + * Reserve the next free round- directory under outputDir. mkdir without + * `recursive` fails with EEXIST when the name is taken, so two runs (or two + * sessions) can never be handed the same directory. + */ +export function reserveRoundDir(outputDir: string): { round: number; dir: string } { + fs.mkdirSync(outputDir, { recursive: true }); + const taken = fs.readdirSync(outputDir) + .map(name => /^round-(\d+)$/.exec(name)) + .filter((m): m is RegExpExecArray => m !== null) + .map(m => Number(m[1])); + for (let round = taken.length ? Math.max(...taken) + 1 : 1; ; round++) { + const dir = path.join(outputDir, `round-${round}`); + try { + fs.mkdirSync(dir); + return { round, dir }; + } catch (err: any) { + if (err.code !== "EEXIST") throw err; + } + } +} + +/** Write the round's manifest last, atomically, from the results actually on disk. */ +function writeRoundManifest(dir: string, round: number, kind: "style" | "responsive", entries: RoundManifestEntry[]): string { + const manifestPath = path.join(dir, "manifest.json"); + const tmp = `${manifestPath}.${process.pid}.tmp`; + fs.writeFileSync(tmp, JSON.stringify({ schema: 1, round, kind, created_at: new Date().toISOString(), variants: entries }, null, 2) + "\n"); + fs.renameSync(tmp, manifestPath); + return manifestPath; +} + +function manifestEntries(labels: string[], results: PromiseSettledResult[]): RoundManifestEntry[] { + return results.map((result, i) => { + const file = result.status === "fulfilled" ? path.basename(result.value.path) : `${labels[i]}.png`; + if (result.status === "fulfilled" && result.value.success && fs.existsSync(result.value.path)) { + return { label: labels[i], file, status: "saved" }; + } + const error = result.status === "fulfilled" ? result.value.error : (result.reason as Error).message; + const rescuedPath = result.status === "fulfilled" ? result.value.rescuedPath : undefined; + return { label: labels[i], file, status: "failed", error, ...(rescuedPath ? { rescuedPath } : {}) }; + }); +} + /** * Generate N variants with staggered parallel execution. */ @@ -155,11 +249,11 @@ export async function variants(options: VariantsOptions): Promise { const quality = options.quality || "high"; - fs.mkdirSync(options.outputDir, { recursive: true }); + const { round, dir: roundDir } = reserveRoundDir(options.outputDir); // If viewports specified, generate responsive variants instead of style variants if (options.viewports) { - await generateResponsiveVariants(apiKey, baseBrief, options.outputDir, options.viewports, quality); + await generateResponsiveVariants(apiKey, baseBrief, roundDir, round, options.viewports, quality); return; } @@ -174,11 +268,12 @@ export async function variants(options: VariantsOptions): Promise { }); const size = options.size || "1536x1024"; - console.error(`Generating ${count} variants...`); + console.error(`Generating ${count} variants into ${roundDir}...`); const startTime = Date.now(); // Staggered parallel: start each call 1.5s apart - const promises: Promise<{ path: string; success: boolean; error?: string }>[] = []; + const promises: Promise[] = []; + const labels: string[] = []; for (let i = 0; i < count; i++) { const variation = STYLE_VARIATIONS[i] || ""; @@ -186,7 +281,8 @@ export async function variants(options: VariantsOptions): Promise { ? `${baseBrief}\n\nStyle direction: ${variation}` : baseBrief; - const outputPath = path.join(options.outputDir, `variant-${String.fromCharCode(65 + i)}.png`); + labels.push(String.fromCharCode(65 + i)); + const outputPath = path.join(roundDir, `variant-${labels[i]}.png`); // Stagger: wait 1.5s between launches const delay = i * 1500; @@ -201,33 +297,32 @@ export async function variants(options: VariantsOptions): Promise { const results = await Promise.allSettled(promises); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); + const entries = manifestEntries(labels, results); + const manifest = writeRoundManifest(roundDir, round, "style", entries); + const succeeded = entries.filter(e => e.status === "saved").map(e => path.join(roundDir, e.file)); - const succeeded: string[] = []; - const failed: string[] = []; - - for (const result of results) { - if (result.status === "fulfilled" && result.value.success) { - const size = fs.statSync(result.value.path).size; - console.error(` ✓ ${path.basename(result.value.path)} (${(size / 1024).toFixed(0)}KB)`); - succeeded.push(result.value.path); + for (const e of entries) { + if (e.status === "saved") { + const size = fs.statSync(path.join(roundDir, e.file)).size; + console.error(` ✓ ${e.file} (${(size / 1024).toFixed(0)}KB)`); } else { - const error = result.status === "fulfilled" ? result.value.error : (result.reason as Error).message; - const filePath = result.status === "fulfilled" ? result.value.path : "unknown"; - console.error(` ✗ ${path.basename(filePath)}: ${error}`); - failed.push(path.basename(filePath)); + console.error(` ✗ ${e.file}: ${e.error}`); } } - console.error(`\n${succeeded.length}/${count} variants generated (${elapsed}s)`); + console.error(`\n${succeeded.length}/${count} variants generated in round ${round} (${elapsed}s)`); // Output structured result to stdout console.log(JSON.stringify({ outputDir: options.outputDir, + round, + roundDir, + manifest, count, succeeded: succeeded.length, - failed: failed.length, + failed: entries.length - succeeded.length, paths: succeeded, - errors: failed, + errors: entries.filter(e => e.status === "failed").map(e => e.file), }, null, 2)); } @@ -240,7 +335,8 @@ const VIEWPORT_CONFIGS: Record { @@ -249,6 +345,7 @@ async function generateResponsiveVariants( if (configs.length === 0) { console.error(`No valid viewports. Use: desktop, tablet, mobile`); + fs.rmdirSync(roundDir); process.exit(1); } @@ -261,10 +358,10 @@ async function generateResponsiveVariants( config.suffix === "tablet" ? "Use a responsive layout that works for medium screens." : "" }`; - const outputPath = path.join(outputDir, `responsive-${config.suffix}.png`); + const outputPath = path.join(roundDir, `responsive-${config.suffix}.png`); const delay = i * 1500; - return new Promise<{ path: string; success: boolean; error?: string }>(resolve => + return new Promise(resolve => setTimeout(resolve, delay) ).then(() => { console.error(` Starting ${config.desc}...`); @@ -274,22 +371,25 @@ async function generateResponsiveVariants( const results = await Promise.allSettled(promises); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); + const entries = manifestEntries(configs.map(c => c.suffix), results); + const manifest = writeRoundManifest(roundDir, round, "responsive", entries); + const succeeded = entries.filter(e => e.status === "saved").map(e => path.join(roundDir, e.file)); - const succeeded: string[] = []; - for (const result of results) { - if (result.status === "fulfilled" && result.value.success) { - const sz = fs.statSync(result.value.path).size; - console.error(` ✓ ${path.basename(result.value.path)} (${(sz / 1024).toFixed(0)}KB)`); - succeeded.push(result.value.path); + for (const e of entries) { + if (e.status === "saved") { + const sz = fs.statSync(path.join(roundDir, e.file)).size; + console.error(` ✓ ${e.file} (${(sz / 1024).toFixed(0)}KB)`); } else { - const error = result.status === "fulfilled" ? result.value.error : (result.reason as Error).message; - console.error(` ✗ ${error}`); + console.error(` ✗ ${e.file}: ${e.error}`); } } - console.error(`\n${succeeded.length}/${configs.length} responsive variants generated (${elapsed}s)`); + console.error(`\n${succeeded.length}/${configs.length} responsive variants generated in round ${round} (${elapsed}s)`); console.log(JSON.stringify({ - outputDir, + outputDir: path.dirname(roundDir), + round, + roundDir, + manifest, viewports: viewportList, succeeded: succeeded.length, paths: succeeded, diff --git a/design/test/gallery.test.ts b/design/test/gallery.test.ts index 7eaebc618..342de41da 100644 --- a/design/test/gallery.test.ts +++ b/design/test/gallery.test.ts @@ -136,4 +136,13 @@ describe('Gallery generation', () => { // All images are base64 expect(html).toContain('data:image/png;base64,'); }); + test('F4: variants in round-/ directories are shown, not only flat files (#1529)', () => { + const sessionDir = path.join(tmpDir, 'rounds', 'pricing-20261003'); + fs.mkdirSync(path.join(sessionDir, 'round-1'), { recursive: true }); + fs.mkdirSync(path.join(sessionDir, 'round-2'), { recursive: true }); + createTestPng(path.join(sessionDir, 'round-1', 'variant-A.png')); + createTestPng(path.join(sessionDir, 'round-2', 'variant-A.png')); + const html = generateGalleryHtml(path.join(tmpDir, 'rounds')); + expect(html.match(/data:image\/png;base64,/g)?.length).toBe(2); + }); }); diff --git a/design/test/variant-rounds.test.ts b/design/test/variant-rounds.test.ts new file mode 100644 index 000000000..8e65b6e17 --- /dev/null +++ b/design/test/variant-rounds.test.ts @@ -0,0 +1,108 @@ +/** + * F4 (#1529): paid design variants survive regenerate rounds. + * + * Before: every `$D variants` run wrote /variant-A.png ..., so + * round 2 overwrote round 1's images, and a local write failure was caught + * by the request retry loop, which bought the same image again. + */ +import { describe, test, expect, beforeEach, afterEach } from "bun:test"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { generateVariant, variants } from "../src/variants"; + +let calls = 0; +let tmpDir: string; +const saved = { fetch: globalThis.fetch, key: process.env.OPENAI_API_KEY, home: process.env.GSTACK_HOME, log: console.log, err: console.error }; +let stdout: string[] = []; + +/** Each paid call returns distinct bytes, so a later round overwriting an earlier file is visible. */ +function stubFetch(): typeof globalThis.fetch { + return (async () => { + calls++; + const bytes = Buffer.from(`image-${calls}`).toString("base64"); + return new Response(JSON.stringify({ output: [{ type: "image_generation_call", result: bytes }] }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + }) as typeof globalThis.fetch; +} + +beforeEach(() => { + calls = 0; + stdout = []; + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "variant-rounds-")); + process.env.GSTACK_HOME = tmpDir; + process.env.OPENAI_API_KEY = "sk-test-not-real"; + globalThis.fetch = stubFetch(); + console.log = (s: string) => { stdout.push(String(s)); }; + console.error = () => {}; +}); + +afterEach(() => { + globalThis.fetch = saved.fetch; + if (saved.key === undefined) delete process.env.OPENAI_API_KEY; else process.env.OPENAI_API_KEY = saved.key; + if (saved.home === undefined) delete process.env.GSTACK_HOME; else process.env.GSTACK_HOME = saved.home; + console.log = saved.log; + console.error = saved.err; + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +function lastResult(): any { + return JSON.parse(stdout[stdout.length - 1]); +} + +describe("F4: collision-safe variant rounds", () => { + test("round 2 writes a new directory; round 1's paid files and manifest stay intact", async () => { + const outputDir = path.join(tmpDir, "designs", "mockup"); + await variants({ brief: "a pricing page", count: 2, outputDir }); + const r1 = lastResult(); + const round1B = fs.readFileSync(path.join(r1.roundDir, "variant-B.png")); + + await variants({ brief: "a pricing page, warmer", count: 2, outputDir }); + const r2 = lastResult(); + + expect(r1.round).toBe(1); + expect(r2.round).toBe(2); + expect(r2.roundDir).not.toBe(r1.roundDir); + expect(r2.paths.every((p: string) => p.startsWith(r2.roundDir + path.sep))).toBe(true); + expect(fs.readFileSync(path.join(r1.roundDir, "variant-B.png"))).toEqual(round1B); + + // Choosing a round-1 variant after round 2: the round-1 manifest still + // resolves label B to a saved file holding round 1's image. + const m1 = JSON.parse(fs.readFileSync(r1.manifest, "utf-8")); + const chosen = m1.variants.find((v: any) => v.label === "B"); + expect(chosen.status).toBe("saved"); + expect(fs.readFileSync(path.join(r1.roundDir, chosen.file))).toEqual(round1B); + expect(fs.readFileSync(path.join(r2.roundDir, "variant-B.png"))).not.toEqual(round1B); + }, 30_000); + + test("responsive rounds get their own names per round", async () => { + const outputDir = path.join(tmpDir, "designs", "responsive"); + await variants({ brief: "a dashboard", outputDir, viewports: "desktop" }); + const first = lastResult().paths[0]; + const firstBytes = fs.readFileSync(first); + await variants({ brief: "a dashboard", outputDir, viewports: "desktop" }); + const second = lastResult().paths[0]; + expect(second).not.toBe(first); + expect(fs.readFileSync(first)).toEqual(firstBytes); + }, 30_000); + + test("a reserved round directory is never handed out twice", async () => { + const outputDir = path.join(tmpDir, "designs", "race"); + fs.mkdirSync(path.join(outputDir, "round-1"), { recursive: true }); + await variants({ brief: "x", count: 1, outputDir }); + expect(lastResult().round).toBe(2); + }, 30_000); + + test("a local write failure retries the write, never the paid request, and keeps the bytes", async () => { + const outputPath = path.join(tmpDir, "missing-dir", "variant-A.png"); + const r = await generateVariant("sk-test-not-real", "p", outputPath, "1536x1024", "high", stubFetch()); + expect(calls).toBe(1); + expect(r.success).toBe(false); + expect(r.error).toContain("could not be saved"); + expect(r.rescuedPath).toBeDefined(); + expect(fs.readFileSync(r.rescuedPath!, "utf-8")).toBe("image-1"); + fs.rmSync(path.dirname(r.rescuedPath!), { recursive: true, force: true }); + }, 30_000); +}); diff --git a/test/design-consultation-command-contract.test.ts b/test/design-consultation-command-contract.test.ts index 0fb50e558..d89633890 100644 --- a/test/design-consultation-command-contract.test.ts +++ b/test/design-consultation-command-contract.test.ts @@ -57,7 +57,8 @@ test('actual CLI variants have no session; generation supplies the session requi const variants = f.run(['variants', '--brief', 'Readable civic dashboard', '--count', '1', '--output-dir', f.dir]); expect(variants.status, variants.stderr).toBe(0); const variantResult = JSON.parse(variants.stdout); - expect(variantResult.paths).toEqual([path.join(f.dir, 'variant-A.png')]); + // F4 (#1529): each variants run writes its own round directory. + expect(variantResult.paths).toEqual([path.join(f.dir, 'round-1', 'variant-A.png')]); expect(variantResult).not.toHaveProperty('sessionFile'); const callCount = readFileSync(f.calls, 'utf8').split('\n').length; const missing = f.run(['iterate', '--feedback', 'Larger title', '--output', path.join(f.dir, 'missing.png')]); From 214cf0b364786fb440fcd9106a7d5536d56fa703 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:48:26 +0000 Subject: [PATCH 03/20] G1: quote the base placeholder in document-release title sync The title-sync block is checked with bash -n; an unquoted parses as a redirection. Migrates the test that pinned the old root VERSION read to the shared classifier call. --- document-release/sections/release-body.md.tmpl | 2 +- test/document-skills-redaction.test.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/document-release/sections/release-body.md.tmpl b/document-release/sections/release-body.md.tmpl index 72f138bd8..41d257d2a 100644 --- a/document-release/sections/release-body.md.tmpl +++ b/document-release/sections/release-body.md.tmpl @@ -369,7 +369,7 @@ PR titles must start with `v` whenever the project has a version source Run this entire block in one shell, substituting `github` or `gitlab` for `` from Step 0 and the base branch for ``. No variables cross tool calls. No version source (NO_VERSION) or no PR/MR skips title sync; an update failure warns and continues. ```bash -VB=$(bun run ~/.claude/skills/gstack/bin/gstack-version-bump classify --base ) || { echo "Title sync: not run (the version source is broken; see the error above)."; exit 0; } +VB=$(bun run ~/.claude/skills/gstack/bin/gstack-version-bump classify --base "") || { echo "Title sync: not run (the version source is broken; see the error above)."; exit 0; } V=$(echo "$VB" | jq -r '.currentVersion // empty') [ -n "$V" ] || { echo "Title sync: not applicable (no version source is configured)."; exit 0; } case "" in diff --git a/test/document-skills-redaction.test.ts b/test/document-skills-redaction.test.ts index 95fb96151..4c856e2b3 100644 --- a/test/document-skills-redaction.test.ts +++ b/test/document-skills-redaction.test.ts @@ -43,7 +43,8 @@ describe("/document-release redaction", () => { test("title synchronization keeps every variable in one valid shell block", () => { const section = RELEASE.slice(RELEASE.indexOf('**PR/MR title sync')); const script = section.match(/```bash\n([\s\S]*?)\n```/)![1]; - for (const command of ['V=$(cat VERSION', 'CURRENT_TITLE=$(gh pr view', 'NEW_TITLE=$(', 'gh pr edit --title "$NEW_TITLE"', 'glab mr update -t "$NEW_TITLE"']) { + // G1 (#2334): the version comes from the shared classifier, not a root VERSION read. + for (const command of ['VB=$(bun run ~/.claude/skills/gstack/bin/gstack-version-bump classify', 'CURRENT_TITLE=$(gh pr view', 'NEW_TITLE=$(', 'gh pr edit --title "$NEW_TITLE"', 'glab mr update -t "$NEW_TITLE"']) { expect(script).toContain(command); } const result = spawnSync('bash', ['-n'], { input: script, encoding: 'utf8', timeout: 5000 }); From 3e55fb8d8ae733c3623ae88ebb82255f92d053ce Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:48:58 +0000 Subject: [PATCH 04/20] G3: give every logging-bin mktemp a ${TMPDIR:-/tmp} template Bare mktemp, mktemp -t and literal /tmp templates in the learnings, question and preference logs (plus jsonl-merge, distill, the two dashboards, /ship's PR body and /document-release's run dir) now use "${TMPDIR:-/tmp}/.XXXXXX", the form the INV-3 lint accepts. When the temp file cannot be created, the learnings, question and preference logs now say the entry was not recorded and how to fix TMPDIR instead of exiting with only mktemp's own error. Three tests that pinned the bare PR_BODY_FILE=$(mktemp) and one that pinned the /tmp run dir migrate. GNU and BSD mktemp already honor TMPDIR without a template, so the macOS-sandbox drop in #2952 is unverified here; the template makes the temp location explicit on every mktemp. Co-authored-by: Davon Johnson --- bin/gstack-community-dashboard | 2 +- bin/gstack-distill-free-text | 2 +- bin/gstack-jsonl-merge | 2 +- bin/gstack-learnings-log | 2 +- bin/gstack-question-log | 2 +- bin/gstack-question-preference | 2 +- bin/gstack-security-dashboard | 2 +- .../sections/release-body.md.tmpl | 2 +- ship/sections/pr-body.md.tmpl | 2 +- test/document-skills-redaction.test.ts | 2 +- test/learnings.test.ts | 33 ++++++++++++++++++ ...regression-pr1169-mktemp-fallbacks.test.ts | 34 +++++++++++++++++-- test/ship-publication-gates.test.ts | 2 +- test/workflow-excerpt.test.ts | 2 +- 14 files changed, 77 insertions(+), 14 deletions(-) diff --git a/bin/gstack-community-dashboard b/bin/gstack-community-dashboard index 2b40b25b7..1a336c96a 100755 --- a/bin/gstack-community-dashboard +++ b/bin/gstack-community-dashboard @@ -36,7 +36,7 @@ fi # ─── Fetch aggregated stats from edge function ──────────────── # HTTP status captured (#1947): a backend failure must read as "unknown", # never as a healthy "Weekly active installs: 0". -TMPBODY="$(mktemp)" +TMPBODY="$(mktemp "${TMPDIR:-/tmp}/gstack-community.XXXXXX")" trap 'rm -f "$TMPBODY"' EXIT SUPA_HOST="${SUPABASE_URL#*://}"; SUPA_HOST="${SUPA_HOST%%/*}" HTTP_CODE="$(_receipted_curl open community-dashboard "$SUPA_HOST" community-pulse-fetch "user-invoked dashboard" --no-payload \ diff --git a/bin/gstack-distill-free-text b/bin/gstack-distill-free-text index 86cbd8918..0a2fdddec 100755 --- a/bin/gstack-distill-free-text +++ b/bin/gstack-distill-free-text @@ -129,7 +129,7 @@ fi # Heredoc into temp file (avoids $(cat <<'PROMPT'...) which choked the # bash parser on apostrophes elsewhere in the script). -DISTILL_PROMPT_FILE=$(mktemp) +DISTILL_PROMPT_FILE=$(mktemp "${TMPDIR:-/tmp}/gstack-distill.XXXXXX") trap 'rm -f "$DISTILL_PROMPT_FILE"' EXIT cat > "$DISTILL_PROMPT_FILE" <<'PROMPT' You are gstack dream-cycle distiller. Below are free-text responses the diff --git a/bin/gstack-jsonl-merge b/bin/gstack-jsonl-merge index 9c4e22688..ce439d2ac 100755 --- a/bin/gstack-jsonl-merge +++ b/bin/gstack-jsonl-merge @@ -45,7 +45,7 @@ BASE="$1" OURS="$2" THEIRS="$3" -TMP=$(mktemp /tmp/gstack-jsonl-merge.XXXXXX) || exit 1 +TMP=$(mktemp "${TMPDIR:-/tmp}/gstack-jsonl-merge.XXXXXX") || exit 1 trap 'rm -f "$TMP" 2>/dev/null || true' EXIT python3 - "$BASE" "$OURS" "$THEIRS" > "$TMP" <<'PYEOF' diff --git a/bin/gstack-learnings-log b/bin/gstack-learnings-log index fd0efaa56..4c4a2b6e6 100755 --- a/bin/gstack-learnings-log +++ b/bin/gstack-learnings-log @@ -24,7 +24,7 @@ INPUT="$1" # Validate and sanitize input. Errors surface (#1950): stderr is captured and # printed on failure instead of swallowed — a silent exit 1 here cost Windows # users every AI-logged learning. -TMPERR=$(mktemp) +TMPERR=$(mktemp "${TMPDIR:-/tmp}/gstack-learnings-log.XXXXXX") || { echo "gstack-learnings-log: could not create a temp file in ${TMPDIR:-/tmp}, so the learning was not recorded. Fix: point TMPDIR at a writable directory." >&2; exit 1; } trap 'rm -f "$TMPERR"' EXIT set +e VALIDATED=$(printf '%s' "$INPUT" | GSTACK_LIB_DIR="$SCRIPT_DIR/../lib" bun -e " diff --git a/bin/gstack-question-log b/bin/gstack-question-log index d769bf371..d0d729ca5 100755 --- a/bin/gstack-question-log +++ b/bin/gstack-question-log @@ -43,7 +43,7 @@ mkdir -p "$GSTACK_HOME/projects/$SLUG" INPUT="$1" # Validate and enrich from registry. -TMPERR=$(mktemp) +TMPERR=$(mktemp "${TMPDIR:-/tmp}/gstack-question-log.XXXXXX") || { echo "gstack-question-log: could not create a temp file in ${TMPDIR:-/tmp}, so the question was not logged. Fix: point TMPDIR at a writable directory." >&2; exit 1; } trap 'rm -f "$TMPERR"' EXIT set +e VALIDATED=$(printf '%s' "$INPUT" | GSTACK_LIB_DIR="$SCRIPT_DIR/../lib" bun -e " diff --git a/bin/gstack-question-preference b/bin/gstack-question-preference index 51de1347e..d4452d04e 100755 --- a/bin/gstack-question-preference +++ b/bin/gstack-question-preference @@ -143,7 +143,7 @@ do_write() { fi ensure_file local TMPERR - TMPERR=$(mktemp) + TMPERR=$(mktemp "${TMPDIR:-/tmp}/gstack-question-preference.XXXXXX") || { echo "gstack-question-preference: could not create a temp file in ${TMPDIR:-/tmp}, so the preference was not saved. Fix: point TMPDIR at a writable directory." >&2; return 1; } # Use function-local cleanup via RETURN trap so variable lookup only happens # while the function is on the stack (avoids EXIT-trap unbound-var race). trap "rm -f '$TMPERR'" RETURN diff --git a/bin/gstack-security-dashboard b/bin/gstack-security-dashboard index 864fd8f94..5d2d355a0 100755 --- a/bin/gstack-security-dashboard +++ b/bin/gstack-security-dashboard @@ -47,7 +47,7 @@ fi # Fetch with the HTTP status captured (#1947). A backend failure must read # as "unknown", never as a healthy "0 attacks" — fake zeros on a security # surface are indistinguishable from good news. -TMPBODY="$(mktemp)" +TMPBODY="$(mktemp "${TMPDIR:-/tmp}/gstack-security.XXXXXX")" trap 'rm -f "$TMPBODY"' EXIT SUPA_HOST="${SUPABASE_URL#*://}"; SUPA_HOST="${SUPA_HOST%%/*}" HTTP_CODE="$(_receipted_curl open security-dashboard "$SUPA_HOST" community-pulse-fetch "user-invoked dashboard" --no-payload \ diff --git a/document-release/sections/release-body.md.tmpl b/document-release/sections/release-body.md.tmpl index 41d257d2a..3e14f84f1 100644 --- a/document-release/sections/release-body.md.tmpl +++ b/document-release/sections/release-body.md.tmpl @@ -245,7 +245,7 @@ near the write-back. 1. Create a private run directory, then replace **every** `` below with its printed absolute path. This literal path survives separate shell calls; do not substitute `$$`. ```bash -mktemp -d /tmp/gstack-doc-release-XXXXXXXX +mktemp -d "${TMPDIR:-/tmp}/gstack-doc-release-XXXXXXXX" ``` Fetch the existing PR/MR body using the platform from the shared Step 0. If no PR/MR exists, skip body/title updates and continue to the summary. diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index 259b3fdf4..962587df4 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -119,7 +119,7 @@ saved section file path as `DOCS_SECTION_FILE`. REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibility 2>/dev/null) [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -PR_BODY_FILE=$(mktemp) || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } +PR_BODY_FILE=$(mktemp "${TMPDIR:-/tmp}/gstack-pr-body.XXXXXX") || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } { cat <<'PR_BODY_EOF' PR_BODY_EOF diff --git a/test/document-skills-redaction.test.ts b/test/document-skills-redaction.test.ts index 4c856e2b3..1d09079b1 100644 --- a/test/document-skills-redaction.test.ts +++ b/test/document-skills-redaction.test.ts @@ -35,7 +35,7 @@ describe("/document-release redaction", () => { expect(RELEASE).toMatch(/exit 3 \(HIGH\).*do NOT edit/i); }); test("separate shell calls share an explicit run directory and never re-read raw tracker text", () => { - expect(RELEASE).toContain('mktemp -d /tmp/gstack-doc-release-XXXXXXXX'); + expect(RELEASE).toContain('mktemp -d "${TMPDIR:-/tmp}/gstack-doc-release-XXXXXXXX"'); expect(RELEASE).not.toContain('/tmp/gstack-pr-body-$$'); expect(RELEASE).not.toContain(''); expect(RELEASE).toContain('pathlib.Path(sys.argv[1]).read_text()'); diff --git a/test/learnings.test.ts b/test/learnings.test.ts index e43fe7c04..15f38984f 100644 --- a/test/learnings.test.ts +++ b/test/learnings.test.ts @@ -321,3 +321,36 @@ describe('gstack-learnings-search edge cases', () => { expect(output).toContain('confidence: 0/10'); }); }); + +// G3 (#2952): the temp file honors TMPDIR, and a temp-file failure says the +// learning was not recorded instead of exiting with only mktemp's own error. +describe('G3: learnings-log temp file and TMPDIR', () => { + test('logs through a private TMPDIR', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-learn-tmpdir-')); + try { + execSync(`${BIN}/gstack-learnings-log '{"skill":"review","type":"pitfall","key":"tmpdir-ok","insight":"x","confidence":5,"source":"observed"}'`, { + cwd: ROOT, env: { ...process.env, GSTACK_HOME: tmpDir, TMPDIR: tmp }, encoding: 'utf-8', timeout: 15000, + }); + expect(fs.readFileSync(findLearningsFile()!, 'utf-8')).toContain('tmpdir-ok'); + expect(fs.readdirSync(tmp)).toEqual([]); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('an unusable TMPDIR fails loudly and names the dropped learning', () => { + let status = 0; + let stderr = ''; + try { + execSync(`${BIN}/gstack-learnings-log '{"skill":"review","type":"pitfall","key":"tmpdir-bad","insight":"x","confidence":5,"source":"observed"}'`, { + cwd: ROOT, env: { ...process.env, GSTACK_HOME: tmpDir, TMPDIR: path.join(tmpDir, 'does-not-exist') }, encoding: 'utf-8', stdio: 'pipe', timeout: 15000, + }); + } catch (e: any) { + status = e.status; + stderr = String(e.stderr); + } + expect(status).not.toBe(0); + expect(stderr).toContain('the learning was not recorded'); + expect(stderr).toContain('TMPDIR'); + }); +}); diff --git a/test/regression-pr1169-mktemp-fallbacks.test.ts b/test/regression-pr1169-mktemp-fallbacks.test.ts index 02cfd78f0..4e13403cd 100644 --- a/test/regression-pr1169-mktemp-fallbacks.test.ts +++ b/test/regression-pr1169-mktemp-fallbacks.test.ts @@ -75,9 +75,10 @@ describe("#2679: skill-content mktemp guards", () => { expect(rendered).toMatch(/REDACT_FILE=\$\(mktemp\)\s*\|\|\s*\{[^}]*exit 1/); }); - test("ship pr-body template guards PR_BODY_FILE=$(mktemp) with a loud exit", () => { + test("ship pr-body template guards PR_BODY_FILE=$(mktemp ...) with a loud exit", () => { const body = readScript("ship/sections/pr-body.md.tmpl"); - expect(body).toMatch(/PR_BODY_FILE=\$\(mktemp\)\s*\|\|\s*\{[^}]*exit 1/); + // G3 (#2952): the mktemp now carries a ${TMPDIR:-/tmp} template. + expect(body).toMatch(/PR_BODY_FILE=\$\(mktemp "\$\{TMPDIR:-\/tmp\}\/[^"]+"\)\s*\|\|\s*\{[^}]*exit 1/); }); test("ship pr-body GitLab path sends the SCANNED file, never a re-rendered heredoc", () => { @@ -151,3 +152,32 @@ describe("PR #1169 bug #5: supabase/verify-rls.sh mktemp fallback", () => { expect(guard).not.toBeNull(); }); }); + +// G3 (#2952): bare `mktemp`, `mktemp -t` and literal `/tmp/` templates ignore +// the TMPDIR a sandboxed agent shell sets, and a failed mktemp under `set -e` +// silently dropped the learning or question being logged. +describe("G3: mktemp honors TMPDIR in logging bins and lane-owned skill bash", () => { + const FILES = [ + "bin/gstack-learnings-log", + "bin/gstack-question-log", + "bin/gstack-question-preference", + "bin/gstack-jsonl-merge", + "bin/gstack-distill-free-text", + "bin/gstack-community-dashboard", + "bin/gstack-security-dashboard", + "ship/sections/pr-body.md.tmpl", + "document-release/sections/release-body.md.tmpl", + ]; + for (const rel of FILES) { + test(`${rel}: every temp-dir mktemp uses a \${TMPDIR:-/tmp} template`, () => { + const calls = [...readScript(rel).matchAll(/(?:\$\(|^[ \t]*)(mktemp\b[^\n)]*)/gm)].map((m) => m[1]); + expect(calls.length).toBeGreaterThan(0); + for (const call of calls) { + expect(call).not.toMatch(/^mktemp\s*$/); + expect(call).not.toMatch(/^mktemp\s+(-d\s+)?-t\b/); + expect(call).not.toMatch(/^mktemp\s+(-d\s+)?["']?\/tmp\//); + expect(call).toMatch(/\$\{TMPDIR:-\/tmp\}\//); + } + }); + } +}); diff --git a/test/ship-publication-gates.test.ts b/test/ship-publication-gates.test.ts index e305ec529..19cf71955 100644 --- a/test/ship-publication-gates.test.ts +++ b/test/ship-publication-gates.test.ts @@ -52,7 +52,7 @@ exec ${JSON.stringify(process.execPath)} ${JSON.stringify(scanner)} "$@" test('publication composes the saved documentation section unchanged and refuses to scan without it', () => { const block = template.match(/```bash\n(: "\$\{NEW_TITLE:[\s\S]*?)\n```/)?.[1]!; - const compose = block.slice(block.indexOf('PR_BODY_FILE=$(mktemp)'), block.indexOf('~/.claude/skills/gstack/bin/gstack-redact --from-file')); + const compose = block.slice(block.indexOf('PR_BODY_FILE=$(mktemp'), block.indexOf('~/.claude/skills/gstack/bin/gstack-redact --from-file')); const root = mkdtempSync(join(tmpdir(), 'ship-compose-')); try { const section = '**Status:** current — no edits.\n\n- Diagram drift: none (no diagrams in any doc).'; diff --git a/test/workflow-excerpt.test.ts b/test/workflow-excerpt.test.ts index 9744e52df..4b82715e4 100644 --- a/test/workflow-excerpt.test.ts +++ b/test/workflow-excerpt.test.ts @@ -108,7 +108,7 @@ describe('workflow judge excerpts', () => { test('ship publishes existing PRs only after shared body composition and scan', () => { const text = readWorkflowExcerpt('ship/SKILL.md', '# Ship:', '## Important Rules'); const publish = text.slice(text.indexOf('## Step 19:'), text.indexOf('## Step 20:')); - const compose = publish.indexOf('PR_BODY_FILE=$(mktemp)'); + const compose = publish.indexOf('PR_BODY_FILE=$(mktemp'); const scan = publish.indexOf('gstack-redact --from-file "$PR_BODY_FILE"'); const edit = publish.indexOf('gh pr edit --body-file'); expect(compose).toBeGreaterThan(0); From e99c1fc7a0bc51a8e94f9906b414811ca473159d Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:55:06 +0000 Subject: [PATCH 05/20] F1: bind the plan audit to this branch's plan, never the newest file Plan File Discovery now prefers an explicit binding (the plan-mode file or /autoplan ACTIVE_PLAN from the conversation, then a "Plan: " line in the PR body), then lists candidates: repo-committed docs/designs/ files this branch changed or that name it, and personal plan files that name the branch. Candidates are offered, never picked silently. The repo-name match and the newest-file-of-the-day fallback, which audited PRs against unrelated plans, are gone. With nothing bound or chosen it prints the exact line 'Plan completion audit: not run (...). Fix: add "Plan: " to the PR body, or run /autoplan.' /ship's parent now binds the plan before dispatching the audit subagent ({{PLAN_COMPLETION_GATE_SHIP:discovery}}), so the child never searches. Migrates three tests that pinned the old markers and notice. Co-authored-by: loulanyue <260355617@qq.com> --- scripts/resolvers/plan-gates.ts | 52 ++++++---- ship/sections/plan-completion.md.tmpl | 11 +- test/remote-identity-vectors.test.ts | 3 +- test/review-workflow-clarity.test.ts | 13 ++- test/ship-plan-completion-invariants.test.ts | 102 +++++++++++++++++++ 5 files changed, 154 insertions(+), 27 deletions(-) diff --git a/scripts/resolvers/plan-gates.ts b/scripts/resolvers/plan-gates.ts index 930329a64..ccb215995 100644 --- a/scripts/resolvers/plan-gates.ts +++ b/scripts/resolvers/plan-gates.ts @@ -186,36 +186,52 @@ table-bearing section that must be the file's terminal heading.`; // ─── Plan File Discovery (shared helper) ────────────────────────────── +export const PLAN_AUDIT_NOT_RUN = + 'Plan completion audit: not run (no plan is bound to this branch and no docs/designs/ file matches). Fix: add "Plan: " to the PR body, or run /autoplan.'; + function generatePlanFileDiscovery(ship = false): string { return `### Plan File Discovery -1. **Conversation context (primary):** Use the active plan file from this conversation or its plan-mode system context. +Audit the plan this branch was built from, never a plan that is merely the newest file. -2. **Content-based search (fallback):** Without a conversation-supplied path, search by content: +1. **Conversation context (primary):** the plan-mode file in this conversation's system context, or the \`ACTIVE_PLAN\` of a \`/autoplan\` run in this conversation. Either is a binding. +2. **PR body binding:** a \`Plan: \` line in this branch's open PR body, printed below as \`PLAN_BINDING:\`. A relative path resolves against the repository root. +3. **Content-based search (fallback):** without a binding, list candidates; never pick one silently. \`\`\`bash setopt +o nomatch 2>/dev/null || true # zsh compat BRANCH=$(git branch --show-current 2>/dev/null | tr '/' '-' | tr -cd 'a-zA-Z0-9._-') -REPO=$(basename "$(git rev-parse --show-toplevel 2>/dev/null)") +_REPOTOP=$(git rev-parse --show-toplevel 2>/dev/null) +_BOUND=$(gh pr view --json body -q .body 2>/dev/null | tr -d '\\r\`' | sed -n 's/^[[:space:]]*Plan:[[:space:]]*\\([^[:space:]]*\\).*/\\1/p' | head -1) +[ -n "$_BOUND" ] && echo "PLAN_BINDING: $_BOUND" +if [ -n "$_REPOTOP" ]; then + _BASE=$(git merge-base "origin/" HEAD 2>/dev/null) + { [ -n "$_BASE" ] && git -C "$_REPOTOP" diff --name-only --diff-filter=AM "$_BASE" -- 'docs/designs/*.md' + [ -n "$BRANCH" ] && git -C "$_REPOTOP" grep -l -F -e "$BRANCH" -- 'docs/designs/*.md' + } 2>/dev/null | sort -u | sed "s|^|PLAN_CANDIDATE: $_REPOTOP/|" +fi GSTACK_STATE_ROOT=$(~/.claude/skills/gstack/bin/gstack-paths --get GSTACK_STATE_ROOT); : "\${GSTACK_STATE_ROOT:?gstack-paths failed; reinstall with ./setup or /gstack-upgrade}" _PLAN_SLUG=$(~/.claude/skills/gstack/bin/gstack-slug 2>/dev/null | sed -n 's/^SLUG=//p') || true _PLAN_SLUG="\${_PLAN_SLUG:-$(basename "$PWD" | tr -cd 'a-zA-Z0-9._-')}" for PLAN_DIR in "$GSTACK_STATE_ROOT/projects/$_PLAN_SLUG" "$HOME/.claude/plans" "$HOME/.codex/plans" ".gstack/plans"; do - [ -d "$PLAN_DIR" ] || continue - PLAN=$(ls -t "$PLAN_DIR"/*.md 2>/dev/null | xargs grep -l "$BRANCH" 2>/dev/null | head -1) - [ -z "$PLAN" ] && PLAN=$(ls -t "$PLAN_DIR"/*.md 2>/dev/null | xargs grep -l "$REPO" 2>/dev/null | head -1) - [ -z "$PLAN" ] && PLAN=$(find "$PLAN_DIR" -name '*.md' -mmin -1440 -maxdepth 1 2>/dev/null | xargs -r ls -t 2>/dev/null | head -1) - [ -n "$PLAN" ] && break + [ -d "$PLAN_DIR" ] && [ -n "$BRANCH" ] || continue + grep -l -F -e "$BRANCH" "$PLAN_DIR"/*.md 2>/dev/null | sed 's|^|PLAN_CANDIDATE: |' done -[ -n "$PLAN" ] && echo "PLAN_FILE: $PLAN" || echo "NO_PLAN_FILE" \`\`\` -3. **Validation:** For search results, read the first 20 lines and verify the project, feature and current branch. A mismatch means "no plan file found." Conversation-supplied paths bypass this search-result check. +\`PLAN_CANDIDATE:\` lines are repo-committed \`docs/designs/\` files this branch changed or that name the branch, then personal plan files that name it. Offer them with AskUserQuestion: one option per candidate (at most four), plus "No plan: skip the audit". Recommend the candidate only when exactly one \`docs/designs/\` file changed on this branch; otherwise recommend skipping. Spawned or non-interactive runs take the recommendation. Read the chosen file's first 20 lines to confirm the project and feature. -**Error handling:** -${ship ? `- No plan file found → skip with "No plan file detected — skipping." -- Plan file found but unreadable (permissions, encoding) → return an audit error to the parent. Do not report no plan or successful zero counts; the parent applies its audit-failure recovery and skip/stop decision.` : `- No plan file found → say "No plan file detected." and use the Fallback Intent Sources below. -- Plan file found but unreadable (permissions, encoding) → say "Plan file found but unreadable." and use the Fallback Intent Sources below; never report plan items as verified.`}`; +4. **No binding and no chosen candidate:** print exactly this line, then ${ship ? 'skip dispatch and record zero counts with this line as the summary' : 'use the Fallback Intent Sources below'}: + \`${PLAN_AUDIT_NOT_RUN}\` + +**Error handling:** a bound or chosen plan file that is unreadable (permissions, encoding) ${ship ? 'is an audit error, not "no plan": the parent applies its audit-failure recovery and skip/stop decision.' : '→ say "Plan file found but unreadable." and use the Fallback Intent Sources below; never report plan items as verified.'}`; +} + +/** Ship's child audit: the parent already bound the plan, so the child never searches. */ +function planInputShip(): string { + return `### Plan input + +Audit only the plan the parent supplied (path or full text). Do not search for another plan. If the supplied file is unreadable (permissions, encoding), return an audit error: do not report no plan or successful zero counts.`; } // ─── Plan Completion Audit ──────────────────────────────────────────── @@ -483,8 +499,8 @@ function generatePlanCompletionAuditInner(mode: PlanCompletionMode, part: 'audit const sections: string[] = []; let gate = ''; - // ── Plan file discovery (shared) ── - sections.push(generatePlanFileDiscovery(mode === 'ship')); + // ── Plan input: ship's parent binds the plan before dispatch (F1) ── + sections.push(mode === 'ship' ? planInputShip() : generatePlanFileDiscovery(false)); // ── Item extraction ── sections.push(planItemExtraction(mode)); @@ -513,7 +529,9 @@ export function generatePlanCompletionAuditShip(_ctx: TemplateContext): string { return generatePlanCompletionAuditInner('ship'); } -export function generatePlanCompletionGateShip(_ctx: TemplateContext): string { +/** `{{PLAN_COMPLETION_GATE_SHIP:discovery}}` renders the parent-side plan binding that runs before dispatch. */ +export function generatePlanCompletionGateShip(_ctx: TemplateContext, args?: string[]): string { + if (args?.includes('discovery')) return generatePlanFileDiscovery(true); return generatePlanCompletionAuditInner('ship', 'gate'); } diff --git a/ship/sections/plan-completion.md.tmpl b/ship/sections/plan-completion.md.tmpl index cada4329e..ce3640b1b 100644 --- a/ship/sections/plan-completion.md.tmpl +++ b/ship/sections/plan-completion.md.tmpl @@ -11,10 +11,13 @@ and `run_in_background: false`. Use Step 7's shared foreground-dispatch rule. The child reads the plan and every referenced code file; the parent validates its report and applies the gates below. -**Subagent prompt:** Substitute `` and supply the active plan's absolute path -or complete text, including user-approved scope changes. If none is known, say -so; the child runs the fallback search below. If discovery found no plan, skip -dispatch. The child does not inherit the parent's conversation. +**Before dispatch, the parent binds the plan:** + +{{PLAN_COMPLETION_GATE_SHIP:discovery}} + +**Subagent prompt:** Substitute `` and supply the bound plan's absolute path +or complete text, including user-approved scope changes. When discovery printed the +not-run line, skip dispatch. The child does not inherit the parent's conversation. ````text You are running a ship-workflow plan completion audit. The base branch is ``. Use `git diff origin/` and inspect untracked files from `git status` to see the full proposed change. Do not commit or push. Report only: classify every item, but do not execute Gate Logic, ask the user, or advance the workflow. The parent applies those gates to your report. diff --git a/test/remote-identity-vectors.test.ts b/test/remote-identity-vectors.test.ts index 2932dd821..04ffb71c8 100644 --- a/test/remote-identity-vectors.test.ts +++ b/test/remote-identity-vectors.test.ts @@ -138,7 +138,8 @@ describe('every resolver files a nested-group repo under the same slug', () => { encoding: 'utf-8', timeout: 30_000, }); - expect(r.stdout).toContain(`PLAN_FILE: ${path.join(planDir, 'plan.md')}`); + // F1 (#2768): search results are offered as candidates, never auto-bound. + expect(r.stdout).toContain(`PLAN_CANDIDATE: ${path.join(planDir, 'plan.md')}`); } finally { fs.rmSync(tmp, { recursive: true, force: true }); } diff --git a/test/review-workflow-clarity.test.ts b/test/review-workflow-clarity.test.ts index e3a63df17..e42948bb4 100644 --- a/test/review-workflow-clarity.test.ts +++ b/test/review-workflow-clarity.test.ts @@ -3,7 +3,7 @@ import { readFileSync } from 'node:fs'; import { join } from 'node:path'; import { generateReviewArmy } from '../scripts/resolvers/review-army'; import { generateCrossReviewDedup, generateSharedCodeReuse, generateScopeDrift } from '../scripts/resolvers/review-scope'; -import { generatePlanCompletionAuditReview, generatePlanCompletionAuditShip } from '../scripts/resolvers/plan-gates'; +import { generatePlanCompletionAuditReview, generatePlanCompletionAuditShip, generatePlanCompletionGateShip, PLAN_AUDIT_NOT_RUN } from '../scripts/resolvers/plan-gates'; import { generateQAReview } from '../scripts/resolvers/qa'; import { generateConfidenceCalibration } from '../scripts/resolvers/confidence'; import { HOST_PATHS, type TemplateContext } from '../scripts/resolvers/types'; @@ -468,12 +468,15 @@ test('review keeps its smoke-clock, setup-authority, plan-gate and findings-sour expect(qa).not.toContain('Ask for setup/permission'); expect(generateQAReview({ ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' })).not.toContain('/review sets none'); const audit = generatePlanCompletionAuditReview(ctx).replace(/\s+/g, ' '); - expect(audit).toContain('"No plan file detected."'); - expect(audit).toContain('Fallback Intent Sources'); - expect(audit).not.toContain('skip with "No plan file detected — skipping."'); + // F1 (#2768): no bound plan prints the exact not-run line instead of the old notice. + expect(audit).toContain(PLAN_AUDIT_NOT_RUN); + expect(audit).toMatch(/use the Fallback Intent Sources below/); + expect(audit).not.toMatch(/skip dispatch/); expect(audit).toContain('**HIGH-impact plan-file discrepancies** trigger AskUserQuestion'); expect(audit).toMatch(/derived only from fallback sources[\s\S]{0,80}never trigger this question/i); - expect(generatePlanCompletionAuditShip({ ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' })).toContain('skip with "No plan file detected — skipping."'); + const shipCtx = { ...ctx, skillName: 'ship', tmplPath: 'ship/SKILL.md.tmpl' }; + expect(generatePlanCompletionGateShip(shipCtx, ['discovery'])).toMatch(/skip dispatch and record zero counts/); + expect(generatePlanCompletionAuditShip(shipCtx)).toContain('Do not search for another plan'); const persist = skill.slice(skill.indexOf('### 2. Fill the record')).replace(/\s+/g, ' '); expect(persist).toMatch(/combined final-pass findings \(core, specialist, adversarial, actionable Greptile, verified exploratory QA findings\)/i); }); diff --git a/test/ship-plan-completion-invariants.test.ts b/test/ship-plan-completion-invariants.test.ts index e7a30d546..1aa7e3e24 100644 --- a/test/ship-plan-completion-invariants.test.ts +++ b/test/ship-plan-completion-invariants.test.ts @@ -238,3 +238,105 @@ test('push idempotency requires the live remote SHA and fails closed on transpor expect(unavailable.stdout).toContain('BLOCKED'); } finally { fs.rmSync(cwd, { recursive: true, force: true }); } }, 120_000); + +// F1 (#2768, #2800): the Plan Completion Audit used to fall back to "the newest +// plan file from the last day" and to any plan naming the repo, so a branch +// with no plan was audited against an unrelated one, and repo-committed +// docs/designs/ was never searched. The rendered discovery block now binds +// explicitly, lists docs/designs/ candidates, and never auto-picks. +describe('F1: plan binding, docs/designs candidates, exact not-run line', () => { + const ROOT = path.join(__dirname, '..'); + const NOT_RUN = 'Plan completion audit: not run (no plan is bound to this branch and no docs/designs/ file matches). Fix: add "Plan: " to the PR body, or run /autoplan.'; + const sections = { + ship: path.join(ROOT, 'ship', 'sections', 'plan-completion.md'), + review: path.join(ROOT, 'review', 'sections', 'plan-completion.md'), + }; + + function discovery(file: string): string { + const md = fs.readFileSync(file, 'utf8'); + return md.slice(md.indexOf('### Plan File Discovery')).match(/```bash\n([\s\S]*?)```/)![1] + .replaceAll('~/.claude/skills/gstack', ROOT).replaceAll('', 'main'); + } + + function fixture(opts: { design?: boolean; prBody?: string }) { + const tmp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'plan-bind-'))); + const repo = path.join(tmp, 'repo'); + fs.mkdirSync(repo); + const env = { ...process.env, HOME: tmp, GSTACK_HOME: path.join(tmp, '.gstack'), GIT_CONFIG_GLOBAL: '/dev/null', GIT_AUTHOR_NAME: 't', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t' } as Record; + delete env.GSTACK_STATE_ROOT; delete env.GSTACK_PROJECT_SLUG; + const git = (...a: string[]) => spawnSync('git', a, { cwd: repo, env, encoding: 'utf8', timeout: 30_000 }); + git('init', '-q', '-b', 'main'); + fs.mkdirSync(path.join(repo, 'docs', 'designs'), { recursive: true }); + fs.writeFileSync(path.join(repo, 'docs', 'designs', 'OLD_FEATURE.md'), '# an older, unrelated design\n'); + git('add', '-A'); git('commit', '-qm', 'base'); + git('update-ref', 'refs/remotes/origin/main', 'HEAD'); + git('checkout', '-qb', 'feat-pricing'); + fs.writeFileSync(path.join(repo, 'src.ts'), 'x\n'); + if (opts.design) fs.writeFileSync(path.join(repo, 'docs', 'designs', 'PRICING.md'), '# pricing plan\n'); + git('add', '-A'); git('commit', '-qm', 'work'); + // An unrelated plan-mode file from today, naming neither branch nor feature. + fs.mkdirSync(path.join(tmp, '.claude', 'plans'), { recursive: true }); + fs.writeFileSync(path.join(tmp, '.claude', 'plans', 'recent-unrelated.md'), '# refactor the billing page for repo\n'); + const bin = path.join(tmp, 'bin'); + fs.mkdirSync(bin); + if (opts.prBody !== undefined) fs.writeFileSync(path.join(tmp, 'pr-body.txt'), opts.prBody); + fs.writeFileSync(path.join(bin, 'gh'), opts.prBody === undefined ? '#!/bin/sh\nexit 1\n' : `#!/bin/sh\ncat '${path.join(tmp, 'pr-body.txt')}'\n`, { mode: 0o755 }); + env.PATH = `${bin}:${process.env.PATH}`; + return { tmp, repo, env }; + } + + function run(which: 'ship' | 'review', f: ReturnType) { + return spawnSync('bash', ['-c', discovery(sections[which])], { cwd: f.repo, env: f.env, encoding: 'utf8', timeout: 30_000 }); + } + + for (const which of ['ship', 'review'] as const) { + test(`${which}: a docs/designs file changed on this branch is a candidate; an unrelated recent plan is not`, () => { + const f = fixture({ design: true }); + try { + const out = run(which, f).stdout; + expect(out).toContain(`PLAN_CANDIDATE: ${path.join(f.repo, 'docs', 'designs', 'PRICING.md')}`); + expect(out).not.toContain('OLD_FEATURE.md'); + expect(out).not.toContain('recent-unrelated.md'); + expect(out).not.toContain('PLAN_FILE:'); + } finally { fs.rmSync(f.tmp, { recursive: true, force: true }); } + }); + + test(`${which}: nothing bound and nothing matching yields no plan at all`, () => { + const f = fixture({}); + try { + const out = run(which, f).stdout; + expect(out).not.toContain('PLAN_CANDIDATE:'); + expect(out).not.toContain('PLAN_FILE:'); + expect(out).not.toContain('recent-unrelated.md'); + } finally { fs.rmSync(f.tmp, { recursive: true, force: true }); } + }); + + test(`${which}: a "Plan:" line in the PR body is printed as the binding`, () => { + const f = fixture({ prBody: 'Summary\n\nPlan: `docs/designs/PRICING.md`\n' }); + try { + expect(run(which, f).stdout).toContain('PLAN_BINDING: docs/designs/PRICING.md'); + } finally { fs.rmSync(f.tmp, { recursive: true, force: true }); } + }); + + test(`${which}: binding precedence and the exact not-run line`, () => { + const md = fs.readFileSync(sections[which], 'utf8'); + const text = md.slice(md.indexOf('### Plan File Discovery')).replace(/\s+/g, ' '); + expect(text).toContain(NOT_RUN); + const order = ['Conversation context (primary)', 'PR body binding', 'Content-based search (fallback)', 'No binding and no chosen candidate']; + const at = order.map(o => text.indexOf(o)); + expect(at.every(i => i >= 0)).toBe(true); + expect([...at].sort((a, b) => a - b)).toEqual(at); + expect(text).toMatch(/never pick one silently/i); + expect(text).toContain('No plan: skip the audit'); + }); + } + + test('ship binds the plan in the parent before dispatch; the child never searches', () => { + const md = fs.readFileSync(sections.ship, 'utf8'); + expect(md.indexOf('### Plan File Discovery')).toBeLessThan(md.indexOf('````text')); + const child = md.slice(md.indexOf('````text'), md.lastIndexOf('````')); + expect(child).not.toContain('### Plan File Discovery'); + expect(child).toMatch(/Do not search for another plan/); + expect(md).toMatch(/not-run line, skip dispatch/); + }); +}); From 998f116b6de6ff1754ee205a73f8679d954c065b Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:56:17 +0000 Subject: [PATCH 06/20] F2: design-doc discovery reads docs/designs, never a root DESIGN.md A root DESIGN.md is the design system /design-consultation writes, so preferring it unconditionally reviewed plans against a token file while printing "Design doc found" and never looked at docs/designs/ feature docs. Discovery now considers only docs/designs/*.md as the repo-local design doc (still freshness-compared with the private ~/.gstack copy). The CEO finding fixture seeds its supplied design as a docs/designs doc and its test follows. Co-authored-by: Bit0ps --- scripts/resolvers/design-doc-discovery.ts | 12 ++++---- test/ceo-finding-fixture.test.ts | 34 +++++++++++++++++++++-- test/helpers/ceo-finding-fixture.ts | 8 ++++-- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/scripts/resolvers/design-doc-discovery.ts b/scripts/resolvers/design-doc-discovery.ts index 757d85efe..37b4455cd 100644 --- a/scripts/resolvers/design-doc-discovery.ts +++ b/scripts/resolvers/design-doc-discovery.ts @@ -3,8 +3,10 @@ * * Finds the design doc a plan review should read: newest branch-scoped doc * under ~/.gstack/projects//, falling back to newest project-scoped - * doc, then lets a repo-local doc (DESIGN.md or docs/designs/*.md) win when - * it is at least as fresh. office-hours dual-writes docs/designs/ alongside + * doc, then lets a repo-local feature doc (docs/designs/*.md) win when + * it is at least as fresh. A root DESIGN.md is never a design doc: it is the + * design system /design-consultation writes, and preferring it reviewed + * plans against a token file while printing "Design doc found" (#2839). office-hours dual-writes docs/designs/ alongside * ~/.gstack, and the committed copy is what teammates see — but a stale old * repo doc must never shadow a newer private session. * @@ -34,10 +36,8 @@ export const DESIGN_DOC_DISCOVERY_BLOCK = `_LOCALDOC=$(ls -t ~/.gstack/projects/ # see. A stale old repo doc never shadows a newer private session. _REPOTOP=$(git rev-parse --show-toplevel 2>/dev/null || echo "") _REPODOC="" -if [ -n "$_REPOTOP" ]; then - [ -f "$_REPOTOP/DESIGN.md" ] && _REPODOC="$_REPOTOP/DESIGN.md" - [ -z "$_REPODOC" ] && _REPODOC=$(ls -t "$_REPOTOP"/docs/designs/*.md 2>/dev/null | head -1) -fi +# Only docs/designs/ holds feature design docs; a root DESIGN.md is the design system (#2839). +[ -n "$_REPOTOP" ] && _REPODOC=$(ls -t "$_REPOTOP"/docs/designs/*.md 2>/dev/null | head -1) DESIGN="$_LOCALDOC" if [ -n "$_REPODOC" ] && { [ -z "$_LOCALDOC" ] || [ "$_REPODOC" -nt "$_LOCALDOC" ]; }; then DESIGN="$_REPODOC" diff --git a/test/ceo-finding-fixture.test.ts b/test/ceo-finding-fixture.test.ts index cc6fc8832..1c0fe62c5 100644 --- a/test/ceo-finding-fixture.test.ts +++ b/test/ceo-finding-fixture.test.ts @@ -291,6 +291,36 @@ test.each([ expect(pickSuppliedCeoPlanStart({ options })).toBe(expected); }); +describe('F2: design-doc discovery prefers docs/designs over a root DESIGN.md (#2839)', () => { + function discover(files: Record): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'design-doc-f2-')); + try { + const cwd = path.join(root, 'project'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd); fs.mkdirSync(home); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd, timeout: 30_000 }); + for (const [rel, body] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(cwd, rel)), { recursive: true }); + fs.writeFileSync(path.join(cwd, rel), body); + } + // The design system is the newest file, so mtime alone would pick it. + if (files['DESIGN.md']) fs.utimesSync(path.join(cwd, 'DESIGN.md'), new Date(), new Date(Date.now() + 60_000)); + return execFileSync('bash', ['-c', `SLUG=fixture; BRANCH=main; ${DESIGN_DOC_DISCOVERY_BLOCK}`], { + cwd, env: { PATH: process.env.PATH!, HOME: home }, encoding: 'utf8', timeout: 10_000, + }).replace(fs.realpathSync(cwd), '').replace(cwd, ''); + } finally { fs.rmSync(root, { recursive: true, force: true }); } + } + + test('a feature doc in docs/designs/ wins over a newer root DESIGN.md', () => { + expect(discover({ 'DESIGN.md': '# Design system\ncolors: {}\n', 'docs/designs/pricing.md': '# Pricing design\n' })) + .toBe('Design doc found: /docs/designs/pricing.md\n'); + }); + + test('a root DESIGN.md alone is a design system, not a design doc', () => { + expect(discover({ 'DESIGN.md': '# Design system\n' })).toBe('No design doc found\n'); + }); +}); + describe('CEO finding fixture establishes scope before launch', () => { test('a supplied design satisfies actual prerequisite discovery without becoming a branch change', () => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ceo-design-seed-')); @@ -304,8 +334,8 @@ describe('CEO finding fixture establishes scope before launch', () => { const output = execFileSync('bash', ['-c', `SLUG=fixture; BRANCH=main; ${DESIGN_DOC_DISCOVERY_BLOCK}`], { cwd, env: { PATH: process.env.PATH!, HOME: home }, encoding: 'utf8', timeout: 10_000, }); - expect(output).toBe(`Design doc found: ${path.join(cwd, 'DESIGN.md')}\n`); - expect(execFileSync('git', ['show', 'HEAD:DESIGN.md'], { cwd, encoding: 'utf8', timeout: 30_000 })).toBe(design); + expect(output).toBe(`Design doc found: ${path.join(cwd, 'docs', 'designs', 'feature-design.md')}\n`); + expect(execFileSync('git', ['show', 'HEAD:docs/designs/feature-design.md'], { cwd, encoding: 'utf8', timeout: 30_000 })).toBe(design); expect(execFileSync('git', ['show', 'HEAD:review-input.md'], { cwd, encoding: 'utf8', timeout: 30_000 })).toBe(plan); expect(execFileSync('git', ['status', '--porcelain'], { cwd, encoding: 'utf8', timeout: 30_000 })).toBe(''); expect(execFileSync('git', ['diff', 'origin/main...HEAD'], { cwd, encoding: 'utf8', timeout: 30_000 })).toBe(''); diff --git a/test/helpers/ceo-finding-fixture.ts b/test/helpers/ceo-finding-fixture.ts index a5ebce8fe..22d67dc11 100644 --- a/test/helpers/ceo-finding-fixture.ts +++ b/test/helpers/ceo-finding-fixture.ts @@ -59,7 +59,11 @@ export function seedPlanReviewProject(projectDir: string, plan: string, skill: ' throw new Error('Plan review fixture requires a fresh private directory'); } fs.writeFileSync(path.join(projectDir, 'review-input.md'), plan, { flag: 'wx' }); - if (design !== undefined) fs.writeFileSync(path.join(projectDir, 'DESIGN.md'), design, { flag: 'wx' }); + // A feature design doc lives in docs/designs/; a root DESIGN.md is a design system (F2, #2839). + if (design !== undefined) { + fs.mkdirSync(path.join(projectDir, 'docs', 'designs'), { recursive: true }); + fs.writeFileSync(path.join(projectDir, 'docs', 'designs', 'feature-design.md'), design, { flag: 'wx' }); + } fs.writeFileSync(path.join(projectDir, 'README.md'), `# ${skill} fixture\n`, { flag: 'wx' }); fs.writeFileSync(path.join(projectDir, 'CLAUDE.md'), [ `# ${skill}`, '', @@ -73,7 +77,7 @@ export function seedPlanReviewProject(projectDir: string, plan: string, skill: ' ].join('\n'), { flag: 'wx' }); const git = (args: string[]) => execFileSync('git', args, { cwd: projectDir, stdio: 'pipe', timeout: 10_000 }); git(['init', '-b', 'main']); - git(['add', 'README.md', 'CLAUDE.md', 'review-input.md', ...(design === undefined ? [] : ['DESIGN.md'])]); + git(['add', 'README.md', 'CLAUDE.md', 'review-input.md', ...(design === undefined ? [] : ['docs/designs/feature-design.md'])]); git(['-c', 'user.name=Finding fixture', '-c', 'user.email=fixture@gstack.test', 'commit', '-m', 'Seed review input']); git(['update-ref', 'refs/remotes/origin/main', 'HEAD']); } From 373118784acb53fbe6701774068c29b2b544728f Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 21:02:26 +0000 Subject: [PATCH 07/20] G2: key the sticky slug cache on the git common dir A linked worktree used to resolve its slug fresh from the remote while the main checkout kept its sticky pre-remote identity, so learnings, checkpoints and timelines split into two buckets. gstack-slug and its TS twin in lib/bin-context.ts now key the cache on the git common dir (read from .git files, no git spawn, so cache hits stay git-free) and read entries in one deterministic order: the repository entry, then the main checkout's pre-upgrade entry, then this directory's own. Both the repository and per-path entries are written. When a worktree's own old entry named a different bucket that still holds data, that slug is recorded in projects//.former-slugs and reported by --get FORMER_SLUGS. New --adopt-legacy --from merges it: JSONL logs append missing lines with de-duplication, missing files copy, differing files are listed as conflicts and never overwritten, the source bucket is kept, and it prints migrated/pending/conflicting counts; reruns are idempotent. --adopt-legacy --dismiss drops the reminder. Co-authored-by: loulanyue <260355617@qq.com> --- bin/gstack-slug | 164 +++++++++++++++++++++++++++++--- lib/bin-context.ts | 93 ++++++++++++++---- test/gstack-slug-parity.test.ts | 86 +++++++++++++++++ 3 files changed, 311 insertions(+), 32 deletions(-) diff --git a/bin/gstack-slug b/bin/gstack-slug index ccacc4682..d7b1b815d 100755 --- a/bin/gstack-slug +++ b/bin/gstack-slug @@ -11,6 +11,8 @@ # --stamp-checkpoint write remote:/project_root: into a checkpoint's frontmatter # --classify-checkpoints read checkpoint paths on stdin, print "\t" # --adopt-legacy [--copy ...] review/copy files from the pre-#3003 bucket +# --adopt-legacy --from merge a former bucket (e.g. a worktree's) into this one +# --adopt-legacy --dismiss stop naming a former bucket at skill start # # Nested-group remotes (#3003): a hosted remote with 3+ path segments # (gitlab.com/a/product/repo) files under "-<16 hex>" instead of the @@ -22,6 +24,17 @@ # (git@work-gitlab:…) are a different canonical host than the https spelling; # pin those with GSTACK_PROJECT_SLUG. # +# Cache key (#2767): inside a git repository the entry is keyed on the git +# common dir, which the main checkout, its subdirectories and every linked +# worktree share, so a worktree can no longer resolve a different slug than +# the main checkout. It is read from .git files without spawning git. Entries +# written before this keyed on pwd; on a miss the main checkout's old entry +# wins over the current directory's own old entry (deterministic: the main +# checkout holds the project's original, possibly pre-remote, identity). When +# a worktree's own old entry named a different, non-empty bucket, that slug is +# recorded in projects//.former-slugs until --adopt-legacy --from merges +# it or --dismiss drops it. Both keys are written, so pwd-keyed readers agree. +# # Cache format: entries are versioned ("v2:"). An unversioned entry from # an older gstack is recomputed once (one git spawn) and rewritten as v2; it # keeps its value unless it is exactly the pre-#3003 last-two slug of a @@ -91,8 +104,8 @@ case "${1:-}" in "") ;; --identity) MODE=identity ;; --get) MODE=get; MODE_ARG="${2:-}" - case "$MODE_ARG" in SLUG|BRANCH|PROJECT_REMOTE|PROJECT_ROOT|LEGACY_SLUG) ;; - *) echo "usage: gstack-slug --get SLUG|BRANCH|PROJECT_REMOTE|PROJECT_ROOT|LEGACY_SLUG" >&2; exit 2 ;; + case "$MODE_ARG" in SLUG|BRANCH|PROJECT_REMOTE|PROJECT_ROOT|LEGACY_SLUG|FORMER_SLUGS) ;; + *) echo "usage: gstack-slug --get SLUG|BRANCH|PROJECT_REMOTE|PROJECT_ROOT|LEGACY_SLUG|FORMER_SLUGS" >&2; exit 2 ;; esac ;; --stamp-checkpoint) MODE=stamp; MODE_ARG="${2:-}" [[ -n "$MODE_ARG" ]] || { echo "usage: gstack-slug --stamp-checkpoint " >&2; exit 2; } ;; @@ -102,8 +115,12 @@ case "${1:-}" in shift [[ $# -gt 0 ]] || { echo "usage: gstack-slug --adopt-legacy --copy ..." >&2; exit 2; } COPY_PATHS=("$@") + elif [[ "${1:-}" == "--from" || "${1:-}" == "--dismiss" ]]; then + MODE=$([[ "$1" == "--from" ]] && echo adopt-from || echo adopt-dismiss) + MODE_ARG=$(printf '%s' "${2:-}" | tr -cd 'a-zA-Z0-9._-') + case "$MODE_ARG" in ""|.|..) echo "usage: gstack-slug --adopt-legacy $1 " >&2; exit 2 ;; esac elif [[ -n "${1:-}" ]]; then - echo "usage: gstack-slug --adopt-legacy [--copy ...]" >&2; exit 2 + echo "usage: gstack-slug --adopt-legacy [--copy ... | --from | --dismiss ]" >&2; exit 2 fi ;; -h|--help) cat <<'USAGE' @@ -119,6 +136,10 @@ gstack-slug — print SLUG= and BRANCH= for the current di gstack-slug --adopt-legacy list the old bucket's files (checkpoints marked matching/foreign/unknown); asks per file on a terminal gstack-slug --adopt-legacy --copy P… copy the named files (paths relative to the old bucket) + gstack-slug --adopt-legacy --from S merge bucket S into this project's: JSONL logs merge with + de-duplication, missing files copy, differing files are + listed as conflicts and never overwritten; S is kept + gstack-slug --adopt-legacy --dismiss S stop naming former bucket S at skill start Env: GSTACK_PROJECT_SLUG pins the slug (ssh host aliases, vendored sub-repos); never cached. USAGE exit 0 ;; @@ -130,6 +151,41 @@ PROJECT_DIR="$(pwd)" CACHE_KEY=$(printf '%s' "$PROJECT_DIR" | tr '/' '_') CACHE_FILE="${CACHE_DIR}/${CACHE_KEY}" +# Physical git common dir of the repository holding $1 (nearest .git entry), +# or nothing outside git. A worktree's .git file names its gitdir, whose +# `commondir` file points at the shared dir. No git spawn. +_git_common_dir() { + local dir="$1" parent="" gitdir="" common="" depth=0 + while [[ -n "$dir" && $depth -lt 64 ]]; do + if [[ -d "$dir/.git" ]]; then (cd "$dir/.git" 2>/dev/null && pwd -P); return 0; fi + if [[ -f "$dir/.git" ]]; then + gitdir=$(sed -n 's/^gitdir: *//p' "$dir/.git" 2>/dev/null | head -1) + [[ -n "$gitdir" ]] || return 0 + [[ "$gitdir" == /* || "$gitdir" =~ ^[A-Za-z]: ]] || gitdir="$dir/$gitdir" + common="$gitdir" + if [[ -f "$gitdir/commondir" ]]; then + common=$(head -1 "$gitdir/commondir" 2>/dev/null) + [[ "$common" == /* || "$common" =~ ^[A-Za-z]: ]] || common="$gitdir/$common" + fi + (cd "$common" 2>/dev/null && pwd -P); return 0 + fi + parent=$(dirname "$dir") + [[ "$parent" == "$dir" ]] && break + dir="$parent" + depth=$((depth + 1)) + done + return 0 +} +LEGACY_CACHE_FILE="$CACHE_FILE" +MAIN_CACHE_FILE="" +GIT_COMMON_DIR=$(_git_common_dir "$PROJECT_DIR") +if [[ -n "$GIT_COMMON_DIR" ]]; then + CACHE_FILE="${CACHE_DIR}/$(printf '%s' "$GIT_COMMON_DIR" | tr '/' '_')" + if [[ "$(basename "$GIT_COMMON_DIR")" == .git ]]; then + MAIN_CACHE_FILE="${CACHE_DIR}/$(dirname "$GIT_COMMON_DIR" | tr '/' '_')" + fi +fi + SLUG="" # 0. Explicit env override — wins over everything. Escape hatch for vendored @@ -276,8 +332,12 @@ _resolve_remote() { # so the heal never fires. _CACHE_V2=0 LEGACY_NOTICE_SLUG="" -if [[ -z "$SLUG" && -f "$CACHE_FILE" ]]; then - _RAW_CACHE=$(cat "$CACHE_FILE" 2>/dev/null || true) +_ENTRY_FILE="" +for _f in "$CACHE_FILE" "$MAIN_CACHE_FILE" "$LEGACY_CACHE_FILE"; do + if [[ -n "$_f" && -f "$_f" ]]; then _ENTRY_FILE="$_f"; break; fi +done +if [[ -z "$SLUG" && -n "$_ENTRY_FILE" ]]; then + _RAW_CACHE=$(cat "$_ENTRY_FILE" 2>/dev/null || true) if [[ "$_RAW_CACHE" == v2:* ]]; then _CACHE_V2=1 _RAW_CACHE="${_RAW_CACHE#v2:}" @@ -356,18 +416,33 @@ SLUG="${SLUG:-$(basename "$PROJECT_DIR" | tr -cd 'a-zA-Z0-9._-')}" # injection, but the invariant should not depend on that reasoning). SLUG=$(printf '%s' "$SLUG" | tr -cd 'a-zA-Z0-9._-') +# A worktree whose own pre-#2767 entry named a different bucket that still +# holds data: remember it so skill start can name the merge command. +if [[ -n "$SLUG" && "$SLUG_FROM_ENV" -eq 0 && "$_ENTRY_FILE" != "$LEGACY_CACHE_FILE" && -f "$LEGACY_CACHE_FILE" ]]; then + _OWN=$(cat "$LEGACY_CACHE_FILE" 2>/dev/null || true) + _OWN=$(printf '%s' "${_OWN#v2:}" | tr -cd 'a-zA-Z0-9._-') + _FORMER_FILE="$GSTACK_STATE_ROOT/projects/$SLUG/.former-slugs" + if [[ -n "$_OWN" && "$_OWN" != "$SLUG" && "$_OWN" != . && "$_OWN" != .. && -n "$(ls -A "$GSTACK_STATE_ROOT/projects/$_OWN" 2>/dev/null)" ]] \ + && ! grep -qxF -- "$_OWN" "$_FORMER_FILE" 2>/dev/null; then + mkdir -p "$(dirname "$_FORMER_FILE")" 2>/dev/null && printf '%s\n' "$_OWN" >> "$_FORMER_FILE" 2>/dev/null || true + fi +fi + if [[ -n "$SLUG" && "$SLUG_FROM_ENV" -eq 0 ]]; then - CURRENT_CACHE="" - if [[ -f "$CACHE_FILE" ]]; then - CURRENT_CACHE=$(cat "$CACHE_FILE" 2>/dev/null || true) - fi - if [[ "$CURRENT_CACHE" != "v2:$SLUG" ]]; then - mkdir -p "$CACHE_DIR" 2>/dev/null || true - CACHE_TMP=$(mktemp "$CACHE_DIR/.slug-XXXXXX" 2>/dev/null) || CACHE_TMP="" - if [[ -n "$CACHE_TMP" ]]; then - printf 'v2:%s' "$SLUG" > "$CACHE_TMP" && mv "$CACHE_TMP" "$CACHE_FILE" 2>/dev/null || rm -f "$CACHE_TMP" 2>/dev/null + for _f in "$CACHE_FILE" "$LEGACY_CACHE_FILE"; do + CURRENT_CACHE="" + if [[ -f "$_f" ]]; then + CURRENT_CACHE=$(cat "$_f" 2>/dev/null || true) fi - fi + if [[ "$CURRENT_CACHE" != "v2:$SLUG" ]]; then + mkdir -p "$CACHE_DIR" 2>/dev/null || true + CACHE_TMP=$(mktemp "$CACHE_DIR/.slug-XXXXXX" 2>/dev/null) || CACHE_TMP="" + if [[ -n "$CACHE_TMP" ]]; then + printf 'v2:%s' "$SLUG" > "$CACHE_TMP" && mv "$CACHE_TMP" "$_f" 2>/dev/null || rm -f "$CACHE_TMP" 2>/dev/null + fi + fi + [[ "$CACHE_FILE" == "$LEGACY_CACHE_FILE" ]] && break + done fi # One notice on the resolve that first files this project under its new @@ -385,6 +460,65 @@ if [[ "$MODE" == "get" && ( "$MODE_ARG" == SLUG || "$MODE_ARG" == BRANCH ) ]]; t if [[ "$MODE_ARG" == SLUG ]]; then printf '%s\n' "$SLUG"; else printf '%s\n' "$BRANCH"; fi exit 0 fi +FORMER_FILE="$GSTACK_STATE_ROOT/projects/$SLUG/.former-slugs" +if [[ "$MODE" == "get" && "$MODE_ARG" == FORMER_SLUGS ]]; then + # Former buckets still holding data and not yet adopted or dismissed. + while IFS= read -r _former || [[ -n "$_former" ]]; do + _former=$(printf '%s' "$_former" | tr -cd 'a-zA-Z0-9._-') + case "$_former" in ""|.|..) continue ;; esac + [[ -n "$(ls -A "$GSTACK_STATE_ROOT/projects/$_former" 2>/dev/null)" ]] && printf '%s\n' "$_former" + done < <(cat "$FORMER_FILE" 2>/dev/null || true) + exit 0 +fi +_forget_former() { + [[ -f "$FORMER_FILE" ]] || return 0 + local tmp + tmp=$(mktemp "$FORMER_FILE.XXXXXX") || return 1 + grep -vxF -- "$1" "$FORMER_FILE" > "$tmp" || true + mv "$tmp" "$FORMER_FILE" +} +if [[ "$MODE" == "adopt-dismiss" ]]; then + _forget_former "$MODE_ARG" && echo "Dismissed projects/$MODE_ARG; its files are kept and skill start no longer names it." + exit $? +fi +if [[ "$MODE" == "adopt-from" ]]; then + FROM_BUCKET="$GSTACK_STATE_ROOT/projects/$MODE_ARG" + TO_BUCKET="$GSTACK_STATE_ROOT/projects/$SLUG" + [[ "$MODE_ARG" != "$SLUG" ]] || { echo "gstack-slug: projects/$MODE_ARG is this project's own bucket." >&2; exit 2; } + [[ -d "$FROM_BUCKET" && ! -L "$FROM_BUCKET" ]] || { echo "gstack-slug: no bucket at $FROM_BUCKET." >&2; exit 1; } + mkdir -p "$TO_BUCKET" || exit 1 + MIGRATED=0; PENDING=0; CONFLICTING=0; UNCHANGED=0; CONFLICTS=""; PENDINGS="" + while IFS= read -r -d '' rel; do + rel="${rel#./}" + src="$FROM_BUCKET/$rel"; dst="$TO_BUCKET/$rel" + if [[ -L "$src" || ! -f "$src" ]]; then + PENDING=$((PENDING + 1)); PENDINGS="$PENDINGS $rel (not a regular file; review by hand)"$'\n'; continue + fi + if [[ ! -e "$dst" ]]; then + mkdir -p "$(dirname "$dst")" && cp -p "$src" "$dst" && MIGRATED=$((MIGRATED + 1)) && continue + PENDING=$((PENDING + 1)); PENDINGS="$PENDINGS $rel (copy failed)"$'\n'; continue + fi + if cmp -s "$src" "$dst"; then UNCHANGED=$((UNCHANGED + 1)); continue; fi + if [[ "$rel" == *.jsonl && -f "$dst" && ! -L "$dst" ]]; then + # Append-only logs: add the lines this bucket lacks, keeping its order. + tmp=$(mktemp "$dst.XXXXXX") || { PENDING=$((PENDING + 1)); continue; } + if { cat "$dst"; awk 'FNR == NR { seen[$0] = 1; next } !($0 in seen) { seen[$0] = 1; print }' "$dst" "$src"; } > "$tmp"; then + if cmp -s "$tmp" "$dst"; then rm -f "$tmp"; UNCHANGED=$((UNCHANGED + 1)); else mv "$tmp" "$dst"; MIGRATED=$((MIGRATED + 1)); fi + else + rm -f "$tmp"; PENDING=$((PENDING + 1)); PENDINGS="$PENDINGS $rel (merge failed)"$'\n' + fi + continue + fi + CONFLICTING=$((CONFLICTING + 1)); CONFLICTS="$CONFLICTS $rel"$'\n' + done < <(cd "$FROM_BUCKET" && find . \( -type f -o -type l \) ! -name .former-slugs -print0 2>/dev/null) + echo "adopt-legacy --from $MODE_ARG into projects/$SLUG: migrated $MIGRATED, pending $PENDING, conflicting $CONFLICTING, unchanged $UNCHANGED" + [[ -z "$PENDINGS" ]] || printf 'Pending (left in projects/%s):\n%s' "$MODE_ARG" "$PENDINGS" + [[ -z "$CONFLICTS" ]] || printf 'Conflicting (both differ; nothing overwritten, compare by hand):\n%s' "$CONFLICTS" + echo "projects/$MODE_ARG is kept; delete it yourself once you are satisfied." + _forget_former "$MODE_ARG" || true + [[ "$PENDING" -eq 0 ]] + exit $? +fi if [[ "$MODE" == "default" ]]; then echo "SLUG=$SLUG" echo "BRANCH=$BRANCH" diff --git a/lib/bin-context.ts b/lib/bin-context.ts index 87f0bda3d..aa72eceb9 100644 --- a/lib/bin-context.ts +++ b/lib/bin-context.ts @@ -6,8 +6,8 @@ */ import { spawnSync } from "child_process"; -import { existsSync, mkdirSync, readFileSync, renameSync, statSync, writeFileSync } from "fs"; -import { basename, dirname, join } from "path"; +import { existsSync, mkdirSync, readFileSync, realpathSync, renameSync, statSync, writeFileSync } from "fs"; +import { basename, dirname, isAbsolute, join, resolve } from "path"; import { resolveStateRoot } from "./state-root"; import { legacyRemoteSlug, remoteSlug } from "./remote-identity"; @@ -158,6 +158,58 @@ export function slugCacheFile(stateRoot: string, cwd: string): string { return join(stateRoot, "slug-cache", toMsysPath(cwd).replace(/\//g, "_")); } +/** + * Twin of gstack-slug's `_git_common_dir` (#2767): the physical git common dir + * of the repository holding `cwd` (nearest `.git` entry), read from `.git` + * files without spawning git; "" outside git. A linked worktree's `.git` file + * names its gitdir, whose `commondir` file points at the shared dir. + */ +export function gitCommonDir(cwd: string): string { + let dir = cwd; + for (let depth = 0; dir && depth < 64; depth++) { + const dotGit = join(dir, ".git"); + try { + const st = statSync(dotGit); + if (st.isDirectory()) return realpathSync(dotGit); + if (st.isFile()) { + const m = readFileSync(dotGit, "utf-8").match(/^gitdir: *(.+)$/m); + if (!m) return ""; + const gitdir = isAbsolute(m[1].trim()) ? m[1].trim() : resolve(dir, m[1].trim()); + let common = gitdir; + try { + const rel = readFileSync(join(gitdir, "commondir"), "utf-8").split("\n")[0].trim(); + if (rel) common = isAbsolute(rel) ? rel : resolve(gitdir, rel); + } catch { + // no commondir file: a submodule or plain gitdir is its own common dir + } + return realpathSync(common); + } + } catch { + // no .git here (or unreadable) — keep walking + } + const parent = dirname(dir); + if (parent === dir) break; + dir = parent; + } + return ""; +} + +/** + * The cache entries gstack-slug consults, in its deterministic read order: + * the repository-scoped entry (keyed on the git common dir), then the main + * checkout's pre-#2767 per-path entry, then this directory's own per-path + * entry. Outside git all three collapse to the per-path entry. + */ +export function slugCacheReadOrder(stateRoot: string, cwd: string): string[] { + const legacy = slugCacheFile(stateRoot, cwd); + const common = gitCommonDir(cwd); + if (!common) return [legacy]; + const files = [slugCacheFile(stateRoot, common)]; + if (basename(common) === ".git") files.push(slugCacheFile(stateRoot, dirname(common))); + files.push(legacy); + return [...new Set(files)]; +} + /** Cache entries written since #3003 carry this version prefix ("v2:"). */ export const SLUG_CACHE_VERSION_PREFIX = "v2:"; @@ -169,9 +221,11 @@ export const SLUG_CACHE_VERSION_PREFIX = "v2:"; * preamble runs, having already written the entry. */ export function readVersionedSlugCache(stateRoot: string, cwd: string): string | undefined { + const file = slugCacheReadOrder(stateRoot, cwd).find((f) => existsSync(f)); + if (!file) return undefined; let raw: string; try { - raw = readFileSync(slugCacheFile(stateRoot, cwd), "utf-8"); + raw = readFileSync(file, "utf-8"); } catch { return undefined; } @@ -183,7 +237,9 @@ export function readVersionedSlugCache(stateRoot: string, cwd: string): string | export function slugFromEnvironment(gstackHome?: string, cwd: string = process.cwd()): string { const home = gstackHome || resolveStateRoot(); const cacheDir = join(home, "slug-cache"); - const cacheFile = slugCacheFile(home, cwd); + const readOrder = slugCacheReadOrder(home, cwd); + const cacheFile = readOrder.find((f) => existsSync(f)); + const writeTargets = [readOrder[0], slugCacheFile(home, cwd)]; // 0. explicit env override — per-invocation escape hatch, never persisted // (caching it would rebind THIS cwd's slug for every later env-less run). @@ -202,7 +258,7 @@ export function slugFromEnvironment(gstackHome?: string, cwd: string = process.c let slug = ""; // 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)) { + if (cacheFile) { try { const raw = readFileSync(cacheFile, "utf-8"); const versioned = raw.startsWith(SLUG_CACHE_VERSION_PREFIX); @@ -271,21 +327,24 @@ export function slugFromEnvironment(gstackHome?: string, cwd: string = process.c // 5. cache it, as gstack-slug does — atomic, self-healing (only rewrites when // the value changed — single-shot, key-local), and failures stay silent. - try { - let current = ""; + // Both the repository-scoped entry and this path's entry (#2767). + for (const target of new Set(writeTargets)) { try { - current = readFileSync(cacheFile, "utf-8"); + let current = ""; + try { + current = readFileSync(target, "utf-8"); + } catch { + // no cache yet — write below + } + if (current !== `${SLUG_CACHE_VERSION_PREFIX}${slug}`) { + mkdirSync(cacheDir, { recursive: true }); + const tmp = `${target}.tmp.${process.pid}`; + writeFileSync(tmp, `${SLUG_CACHE_VERSION_PREFIX}${slug}`, "utf-8"); + renameSync(tmp, target); + } } catch { - // no cache yet — write below + // best-effort cache; a miss only costs a re-derive on the next call } - if (current !== `${SLUG_CACHE_VERSION_PREFIX}${slug}`) { - mkdirSync(cacheDir, { recursive: true }); - const tmp = `${cacheFile}.tmp.${process.pid}`; - writeFileSync(tmp, `${SLUG_CACHE_VERSION_PREFIX}${slug}`, "utf-8"); - renameSync(tmp, cacheFile); - } - } catch { - // best-effort cache; a miss only costs a re-derive on the next call } return slug; } diff --git a/test/gstack-slug-parity.test.ts b/test/gstack-slug-parity.test.ts index 307ef0fe5..21ff6dba8 100644 --- a/test/gstack-slug-parity.test.ts +++ b/test/gstack-slug-parity.test.ts @@ -374,3 +374,89 @@ describe('gstack-slug ↔ remote-slug parity', () => { } }); }); + +// G2 (#2767): the sticky slug cache was keyed on pwd, so a linked worktree +// resolved its slug fresh from the remote while the main checkout kept its +// sticky pre-remote identity, splitting learnings and checkpoints into two +// buckets. The cache is now keyed on the git common dir in both gstack-slug +// and lib/bin-context.ts, and --adopt-legacy --from merges a former bucket. +describe('G2: linked worktrees share the main checkout identity', () => { + let tmpHome: string; + let fixtures: string; + beforeEach(() => { + tmpHome = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'slug-wt-home-'))); + fixtures = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'slug-wt-fix-'))); + }); + afterEach(() => { + try { fs.rmSync(tmpHome, { recursive: true, force: true }); } catch {} + try { fs.rmSync(fixtures, { recursive: true, force: true }); } catch {} + }); + + const state = () => path.join(tmpHome, '.gstack'); + const runIn = (cwd: string, args: string[]) => spawnSync('bash', [SLUG_SCRIPT, ...args], { + cwd, env: baseEnv(tmpHome), encoding: 'utf8', timeout: 10_000, + }); + + /** Main checkout that used gstack before adopting a remote, plus a linked worktree. */ + function stickyRepoWithWorktree(): { main: string; wt: string } { + const main = makeRepo(path.join(fixtures, 'myproj')); + git(['-C', main, '-c', 'user.email=t@t', '-c', 'user.name=t', 'commit', '-q', '--allow-empty', '-m', 'init']); + expect(slugOf(runSlug(main, tmpHome))).toBe('myproj'); + git(['-C', main, 'remote', 'add', 'origin', 'git@github.com:someowner/myproj.git']); + const wt = path.join(fixtures, 'myproj-wt'); + git(['-C', main, 'worktree', 'add', '-q', '-b', 'feature', wt]); + return { main, wt }; + } + + test('a linked worktree resolves the main checkout sticky slug (bash and TS agree)', async () => { + const { main, wt } = stickyRepoWithWorktree(); + expect(slugOf(runSlug(wt, tmpHome))).toBe('myproj'); + expect(slugOf(runSlug(path.join(main), tmpHome))).toBe('myproj'); + const { slugFromEnvironment, readVersionedSlugCache } = await import('../lib/bin-context'); + expect(readVersionedSlugCache(state(), wt)).toBe('myproj'); + expect(slugFromEnvironment(state(), wt)).toBe('myproj'); + }); + + test('disagreeing pre-upgrade caches: the main checkout wins and the former bucket is named', () => { + const { main, wt } = stickyRepoWithWorktree(); + const cacheDir = path.join(state(), 'slug-cache'); + fs.writeFileSync(path.join(cacheDir, encodedCacheKey(wt)), 'v2:someowner-myproj'); + fs.rmSync(path.join(cacheDir, encodedCacheKey(path.join(main, '.git'))), { force: true }); + fs.mkdirSync(path.join(state(), 'projects', 'someowner-myproj'), { recursive: true }); + fs.writeFileSync(path.join(state(), 'projects', 'someowner-myproj', 'learnings.jsonl'), '{"key":"a"}\n'); + expect(slugOf(runSlug(wt, tmpHome))).toBe('myproj'); + expect(runIn(wt, ['--get', 'FORMER_SLUGS']).stdout).toBe('someowner-myproj\n'); + expect(runIn(main, ['--get', 'FORMER_SLUGS']).stdout).toBe('someowner-myproj\n'); + expect(runIn(wt, ['--adopt-legacy', '--dismiss', 'someowner-myproj']).status).toBe(0); + expect(runIn(wt, ['--get', 'FORMER_SLUGS']).stdout).toBe(''); + expect(fs.existsSync(path.join(state(), 'projects', 'someowner-myproj', 'learnings.jsonl'))).toBe(true); + }); + + test('--adopt-legacy --from merges JSONL with de-duplication, copies missing files, lists conflicts, and reruns idempotently', () => { + const { wt } = stickyRepoWithWorktree(); + const from = path.join(state(), 'projects', 'someowner-myproj'); + const to = path.join(state(), 'projects', 'myproj'); + fs.mkdirSync(path.join(from, 'checkpoints'), { recursive: true }); + fs.mkdirSync(path.join(to, 'checkpoints'), { recursive: true }); + fs.writeFileSync(path.join(to, 'learnings.jsonl'), '{"key":"a"}\n{"key":"b"}\n'); + fs.writeFileSync(path.join(from, 'learnings.jsonl'), '{"key":"b"}\n{"key":"c"}\n'); + fs.writeFileSync(path.join(from, 'timeline.jsonl'), '{"event":"x"}\n'); + fs.writeFileSync(path.join(to, 'checkpoints', 'cp.md'), 'main version\n'); + fs.writeFileSync(path.join(from, 'checkpoints', 'cp.md'), 'worktree version\n'); + fs.writeFileSync(path.join(state(), 'projects', 'myproj', '.former-slugs'), 'someowner-myproj\n'); + + const first = runIn(wt, ['--adopt-legacy', '--from', 'someowner-myproj']); + expect(first.status).toBe(0); + expect(first.stdout).toContain('migrated 2, pending 0, conflicting 1, unchanged 0'); + expect(first.stdout).toContain('checkpoints/cp.md'); + expect(fs.readFileSync(path.join(to, 'learnings.jsonl'), 'utf8')).toBe('{"key":"a"}\n{"key":"b"}\n{"key":"c"}\n'); + expect(fs.readFileSync(path.join(to, 'timeline.jsonl'), 'utf8')).toBe('{"event":"x"}\n'); + expect(fs.readFileSync(path.join(to, 'checkpoints', 'cp.md'), 'utf8')).toBe('main version\n'); + expect(fs.readFileSync(path.join(from, 'learnings.jsonl'), 'utf8')).toBe('{"key":"b"}\n{"key":"c"}\n'); + expect(runIn(wt, ['--get', 'FORMER_SLUGS']).stdout).toBe(''); + + const again = runIn(wt, ['--adopt-legacy', '--from', 'someowner-myproj']); + expect(again.stdout).toContain('migrated 0, pending 0, conflicting 1, unchanged 2'); + expect(fs.readFileSync(path.join(to, 'learnings.jsonl'), 'utf8')).toBe('{"key":"a"}\n{"key":"b"}\n{"key":"c"}\n'); + }); +}); From 85c9d4cebdc71120389d7c0a5761948fecf64bbb Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 21:04:53 +0000 Subject: [PATCH 08/20] B5: learnings and timeline reads fail loudly when bun is missing gstack-learnings-search and gstack-timeline-read ended in '2>/dev/null || exit 0', so a missing bun or a crashed embedded script printed nothing and exited 0, identical to 'nothing recorded'. Both now exit 127 with '