From 394f3fd233d1a66bf3edd1622eb8313dd44661d1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 01:41:59 +0000 Subject: [PATCH] fix: pre-landing review fixes, ship invocation 2 (GSTA-97) - Tests: contributor-branch failure row; bun.lock production closure never reaches the dev-toolchain marker; real-Bun rows isolate HOME, XDG_CONFIG_HOME and BUN_CONFIG_*; the import walker follows side-effect imports; file header matches the two install branches. - BROWSER.md quick start uses the frozen production install. - CHANGELOG: the reclaim recipe also drops the Agent SDK's cache entries (Bun hardlinks node_modules into its cache); Tests list is current. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- BROWSER.md | 2 +- CHANGELOG.md | 4 +- test/setup-production-deps.test.ts | 75 +++++++++++++++++++++++++++--- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/BROWSER.md b/BROWSER.md index 3047da8f5..071bd5ce3 100644 --- a/BROWSER.md +++ b/BROWSER.md @@ -275,7 +275,7 @@ reach for it. ```bash # One-time: build the binary (browse/dist/browse, ~58MB) -bun install && bun run build +bun install --production --frozen-lockfile && bun run build # Set $B once and forget about it B=./browse/dist/browse # or ~/.claude/skills/gstack/browse/dist/browse diff --git a/CHANGELOG.md b/CHANGELOG.md index 01a1c7576..7799a63ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,10 @@ - **Contributors and CI keep the full install.** `./setup` treats a checkout that already has the dev toolchain (`node_modules/typescript`) as a contributor's and runs the full `bun install --frozen-lockfile` there, so dev packages keep following `bun.lock` after a pull (a production install would report "no changes" and leave them at old versions). `bin/dev-setup` now always runs `bun install` (it skipped it when `node_modules` existed, which would have left a checkout first set up by `./setup` without the dev toolchain). CI workflows already run `bun install --frozen-lockfile` themselves. CONTRIBUTING.md says which command installs what. ### If you already have gstack installed -- An existing install keeps its dev dependencies and keeps getting the full install, because it already has the dev toolchain. To get the space back: `cd ~/.claude/skills/gstack && rm -rf node_modules && bun install --production --frozen-lockfile`. Don't do this in a checkout you develop gstack in. +- An existing install keeps its dev dependencies and keeps getting the full install, because it already has the dev toolchain. To get the space back: `cd ~/.claude/skills/gstack && rm -rf node_modules && bun install --production --frozen-lockfile && rm -rf "$(bun pm cache)"/@anthropic-ai/claude-agent-sdk*`. The last step matters: Bun hardlinks `node_modules` into its global cache, so the Agent SDK's binaries stay on disk until their cache entries go. Don't do this in a checkout you develop gstack in. ### Tests -- `test/setup-production-deps.test.ts`: `install_runtime_deps`, sliced out of `setup` and run against a recording `bun` that models Bun's frozen `--production`, passes `--production` on the frozen-lockfile call, refreshes a stale lockfile before retrying, and stops setup when the refresh fails. A checkout with the dev toolchain gets the full frozen install. Run with the real Bun on local `file:` packages: a fresh install leaves out devDependencies, a later production install keeps the ones a full install added, and a stale lockfile is refreshed while the install stays production-only. No other line in `setup` calls `bun install` (the pre-change line is the negative control). `bin/dev-setup`'s install block, run over an existing `node_modules`, still calls plain `bun install`. A scan of every shipped file (`bin`, `lib`, `browse/src`, `design/src`, `make-pdf/src`, `hosts`, `extension`, the resolvers, and the scripts `setup` and the build call by path) finds no import or `require` of a package outside `dependencies`. Modules the shipped code reaches by relative import (gen-skill-docs' `scripts/` helpers, and the `test/helpers/providers` modules `gstack-model-benchmark` loads) are scanned too. Negative controls: a planted Agent SDK import is caught, including one split over several lines and a re-export; a type-only import is not; and moving `@anthropic-ai/sdk` back to dev flags `gstack-distill-free-text`. The packages `bun run build` copies out of `node_modules` are production dependencies. +- `test/setup-production-deps.test.ts`: `install_runtime_deps`, sliced out of `setup` and run against a recording `bun` that models Bun's frozen `--production`, passes `--production` on the frozen-lockfile call, refreshes a stale lockfile before retrying, and stops setup when the refresh fails. A checkout with the dev toolchain gets the full frozen install, and a failed full install stops setup too. A walk of `bun.lock`'s production closure (with required-peer and optional-peer controls) proves no production dependency brings in `typescript`, the marker setup checks. The real-Bun rows run with `HOME`, `XDG_CONFIG_HOME` and `BUN_CONFIG_*` isolated, so a developer's global `.bunfig.toml` cannot change them. Run with the real Bun on local `file:` packages: a fresh install leaves out devDependencies, a later production install keeps the ones a full install added, and a stale lockfile is refreshed while the install stays production-only. No other line in `setup` calls `bun install` (the pre-change line is the negative control). `bin/dev-setup`'s install block, run over an existing `node_modules`, still calls plain `bun install`. A scan of every shipped file (`bin`, `lib`, `browse/src`, `design/src`, `make-pdf/src`, `hosts`, `extension`, the resolvers, and the scripts `setup` and the build call by path) finds no import or `require` of a package outside `dependencies`. Modules the shipped code reaches by relative import (gen-skill-docs' `scripts/` helpers, and the `test/helpers/providers` modules `gstack-model-benchmark` loads) are scanned too. Negative controls: a planted Agent SDK import is caught, including one split over several lines and a re-export; a type-only import is not; and moving `@anthropic-ai/sdk` back to dev flags `gstack-distill-free-text`. The packages `bun run build` copies out of `node_modules` are production dependencies. - `test/gstack-browser-ensure.test.ts`: with `node_modules` missing, the preflight runs the frozen production install, refreshes a stale lockfile before retrying, and ends a failed install in `BROWSER_UNAVAILABLE` with a production-install fix line. - The install fixtures' stub `bun` (`test/helpers/install-fixture.ts`, `test/helpers/claude-overlay-fixture.ts`, `test/team-mode.test.ts`) now matches `install --production --frozen-lockfile`, so a setup that dropped the flag would reach the real `bun` instead of the stub. diff --git a/test/setup-production-deps.test.ts b/test/setup-production-deps.test.ts index 81760c94c..9664d568f 100644 --- a/test/setup-production-deps.test.ts +++ b/test/setup-production-deps.test.ts @@ -9,9 +9,9 @@ import { runBashScript } from './helpers/bash-script'; // ./setup installs production dependencies only (GSTA-97). devDependencies are // the test and eval toolchain, chiefly the Claude Agent SDK's per-platform // native binaries (~440 MB on Linux). This file pins the two halves of that: -// every install setup runs passes --production (contributors and CI install -// the full set themselves), and nothing setup, the build or an installed skill -// loads is a devDependency. +// a user install (no dev toolchain in node_modules) is production-only, while a +// checkout that already has the dev toolchain gets the full frozen install; and +// nothing setup, the build or an installed skill loads is a devDependency. const ROOT = path.resolve(import.meta.dir, '..'); const read = (rel: string) => fs.readFileSync(path.join(ROOT, rel), 'utf-8'); @@ -19,6 +19,13 @@ const PKG = JSON.parse(read('package.json')); const bases: string[] = []; afterEach(() => { for (const b of bases.splice(0)) fs.rmSync(b, { recursive: true, force: true }); }); +/** process.env without the developer's global Bun config: HOME and XDG_CONFIG_HOME move into the temp base, BUN_CONFIG_* is dropped. */ +function isolatedBunEnv(base: string): NodeJS.ProcessEnv { + const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => !k.startsWith('BUN_CONFIG_'))); + for (const dir of ['home', 'xdg']) fs.mkdirSync(path.join(base, dir), { recursive: true }); + return { ...env, HOME: path.join(base, 'home'), XDG_CONFIG_HOME: path.join(base, 'xdg') }; +} + function tempBase(): string { const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-prod-deps-')); bases.push(base); @@ -89,6 +96,19 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende const stub2 = stubBun(base2); expect(runBashScript(script(stub2.bun, 'install_runtime_deps'), { cwd: base2, env: { ...process.env, FROZEN_RC: '1' } }).status).toBe(0); expect(stub2.calls()).toEqual(['install --frozen-lockfile', 'install']); + + // Generated by /ship Step 7 coverage audit (GSTA-97). + // Value: protects=a contributor checkout whose full install fails stops setup's install step instead of building on stale packages; + // fails_when=the dev branch swallows the plain install's failure (an added `|| true`, or a warn-and-continue fallback); + // why_new=both rows above succeed, and the failure row in the test above runs the production branch; seam=none; + // no_claim=setup's own subshell wiring around the call (stub bun) + const base3 = tempBase(); + fs.mkdirSync(path.join(base3, 'node_modules', 'typescript'), { recursive: true }); + const stub3 = stubBun(base3); + const failed = runBashScript(script(stub3.bun, 'install_runtime_deps\necho AFTER'), { cwd: base3, env: { ...process.env, FROZEN_RC: '1', FALLBACK_RC: '4' } }); + expect(failed.status).toBe(4); + expect(failed.stdout).not.toContain('AFTER'); + expect(stub3.calls()).toEqual(['install --frozen-lockfile', 'install']); }); test('every bun install in setup goes through install_runtime_deps', () => { @@ -151,7 +171,7 @@ describe.skipIf(process.platform === 'win32')('install_runtime_deps with the rea dependencies: { 'runtime-pkg': 'file:./pkgs/runtime-pkg' }, devDependencies: { 'dev-pkg': 'file:./pkgs/dev-pkg' }, })); - const env = { ...process.env, BUN_INSTALL_CACHE_DIR: path.join(base, 'cache') }; + const env = { ...isolatedBunEnv(base), BUN_INSTALL_CACHE_DIR: path.join(base, 'cache') }; const setup = read('setup'); const run = (call: string) => runBashScript( `set -e\nBUN_CMD=${JSON.stringify(process.execPath)}\n${sliceFunction(setup, 'bun_cmd')}\n${sliceFunction(setup, 'install_runtime_deps')}\n${call}\n`, @@ -191,7 +211,7 @@ describe.skipIf(process.platform === 'win32')('install_runtime_deps with the rea const manifest = (deps: Record) => fs.writeFileSync(path.join(project, 'package.json'), JSON.stringify({ name: 'prod-deps-fixture', version: '0.0.0', dependencies: deps, devDependencies: { 'dev-pkg': 'file:./pkgs/dev-pkg' }, })); - const env = { ...process.env, BUN_INSTALL_CACHE_DIR: path.join(base, 'cache') }; + const env = { ...isolatedBunEnv(base), BUN_INSTALL_CACHE_DIR: path.join(base, 'cache') }; const setup = read('setup'); const run = (call: string) => runBashScript( `set -e\nBUN_CMD=${JSON.stringify(process.execPath)}\n${sliceFunction(setup, 'bun_cmd')}\n${sliceFunction(setup, 'install_runtime_deps')}\n${call}\n`, @@ -276,12 +296,12 @@ describe('setup and runtime need only production dependencies', () => { const files = runtimeFiles(); const reached: Record = {}; const queue = Object.keys(files); - const relSpec = /^\s*(?:import|export)\s+(?!type\b)[^'";]*?from\s+['"](\.\.?\/[^'"]+)['"]|\b(?:import|require)\(\s*['"](\.\.?\/[^'"]+)['"]\s*\)/gm; + const relSpec = /^\s*(?:import|export)\s+(?!type\b)[^'";]*?from\s+['"](\.\.?\/[^'"]+)['"]|\b(?:import|require)\(\s*['"](\.\.?\/[^'"]+)['"]\s*\)|^\s*import\s+['"](\.\.?\/[^'"]+)['"]/gm; while (queue.length) { const rel = queue.pop()!; const text = files[rel] ?? reached[rel]; for (const m of text.matchAll(relSpec)) { - const spec = path.posix.normalize(path.posix.join(path.posix.dirname(rel), m[1] ?? m[2])); + const spec = path.posix.normalize(path.posix.join(path.posix.dirname(rel), m[1] ?? m[2] ?? m[3])); const target = [spec, `${spec}.ts`, `${spec}.js`, `${spec}/index.ts`].find((c) => fs.existsSync(path.join(ROOT, c)) && fs.statSync(path.join(ROOT, c)).isFile()); if (!target || target.startsWith('..') || target in files || target in reached || /(^|\/)node_modules\//.test(target) || (/(^|\/)test\//.test(target) && !target.startsWith('test/helpers/')) || !/\.(ts|js|mjs)$/.test(target)) continue; reached[target] = fs.readFileSync(path.join(ROOT, target), 'utf-8'); @@ -318,6 +338,47 @@ describe('setup and runtime need only production dependencies', () => { for (const name of copied) expect(PKG.dependencies).toHaveProperty([name]); }); + // Generated by /ship Step 7 coverage audit (GSTA-97). + // Value: protects=a production-only install never creates the dev-toolchain marker install_runtime_deps reads, so a user's next ./setup stays production-only; + // fails_when=a production dependency, or a package it pulls in (required peers included), starts depending on the marker package, so every repeat ./setup goes full; + // why_new=the real-Bun rows use local fixture packages, and nothing walks the repo's own bun.lock production closure; + // seam=none; no_claim=Bun's actual hoisting or platform filtering (a static, name-based walk of bun.lock that counts every candidate entry) + test('the production closure in bun.lock never installs the dev-toolchain marker setup checks', () => { + const marker = sliceFunction(read('setup'), 'install_runtime_deps').match(/\[ -d node_modules\/([^\s\]]+) \]/)?.[1]; + expect(marker).toBeDefined(); + expect(PKG.devDependencies).toHaveProperty([marker!]); + type Lock = { workspaces: Record }>; packages: Record }; + const closure = (lock: Lock): Set => { + const seen = new Set(); + const queue = Object.keys(lock.workspaces[''].dependencies ?? {}); + while (queue.length) { + const name = queue.pop()!; + if (seen.has(name)) continue; + seen.add(name); + for (const [key, entry] of Object.entries(lock.packages)) { + if (key !== name && !(key.endsWith(`/${name}`) && key.slice(0, -name.length - 1) in lock.packages)) continue; + const meta = (entry.find((e) => e && typeof e === 'object' && !Array.isArray(e)) ?? {}) as Record; + const optionalPeers = new Set(meta.optionalPeers ?? []); + for (const field of ['dependencies', 'optionalDependencies', 'peerDependencies']) { + for (const dep of Object.keys(meta[field] ?? {})) if (field !== 'peerDependencies' || !optionalPeers.has(dep)) queue.push(dep); + } + } + } + return seen; + }; + const lock: Lock = JSON.parse(read('bun.lock').replace(/,(\s*[}\]])/g, '$1')); + const production = closure(lock); + expect(production.has('playwright-core')).toBe(true); + expect(production.has(marker!)).toBe(false); + // Negative controls: a required peer on the marker is caught, through a nested entry too; an optional peer is not. + const synthetic = (meta: Record): Lock => ({ + workspaces: { '': { dependencies: { a: '1' } } }, + packages: { a: ['a@1', '', { dependencies: { b: '1' } }, 'sha'], b: ['b@1', '', {}, 'sha'], 'a/b': ['b@2', '', meta, 'sha'] }, + }); + expect(closure(synthetic({ peerDependencies: { [marker!]: '*' } })).has(marker!)).toBe(true); + expect(closure(synthetic({ peerDependencies: { [marker!]: '*' }, optionalPeers: [marker!] })).has(marker!)).toBe(false); + }); + test('the dev toolchain stays out of the user install', () => { for (const name of ['@anthropic-ai/claude-agent-sdk', 'typescript', 'prettier', '@types/bun']) { expect(PKG.devDependencies).toHaveProperty([name]);