mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-11 03:00:26 +01:00
fix: pre-landing review fixes (GSTA-97)
- setup/gstack-browser-ensure: Bun treats --production as frozen, so the old fallback could never recover a stale lockfile; refresh it with --lockfile-only, then install production-only again. - gstack-browser-ensure failure line and the README tip no longer suggest a full bun install to users. - Tests: real-Bun stale-lockfile and keep-devDeps contracts, behavioral browser-ensure install tests, relative-import closure scan, multi-line and re-export negative controls; CHANGELOG matches. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed
1 parent
748c45a4a4
commit
a3d402d07e
6 files changed
+189
-14
No files matched your search
@@ -136,6 +136,53 @@ describe('gstack-browser-ensure with a winget-shaped Bun (#3131)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// Generated by /ship Step 7 coverage audit (GSTA-97).
|
||||
// Value: protects=a missing node_modules is installed production-only, a stale lockfile is refreshed before the retry, and a failed install ends in BROWSER_UNAVAILABLE;
|
||||
// fails_when=--production leaves either install, the lockfile refresh is lost, or a failed install no longer reports the fix line;
|
||||
// why_new=setup-production-deps.test.ts matches the line's text only, and the --check case never installs;
|
||||
// seam=none; no_claim=a real bun install (a stub bun records argv, models Bun's frozen --production rule and restores a stub playwright)
|
||||
describe('gstack-browser-ensure installs a missing node_modules production-only (GSTA-97)', () => {
|
||||
function installing(frozenRc: number, fallbackRc: number) {
|
||||
const f = fixture();
|
||||
fs.writeFileSync(f.marker, '');
|
||||
fs.renameSync(path.join(f.root, 'node_modules/playwright'), path.join(f.base, 'playwright-stub'));
|
||||
fs.writeFileSync(path.join(f.base, 'stub/bun'), [
|
||||
'#!/usr/bin/env bash',
|
||||
'[ "$1" = install ] || exit 9',
|
||||
`echo "$*" >> ${JSON.stringify(path.join(f.base, 'install-calls'))}`,
|
||||
`refreshed=${JSON.stringify(path.join(f.base, 'lockfile-refreshed'))}`,
|
||||
`case "$*" in\n *--lockfile-only*) rc=${fallbackRc}; [ "$rc" -eq 0 ] && : > "$refreshed"; exit "$rc" ;;\n *--frozen-lockfile*) rc=${frozenRc}; [ -f "$refreshed" ] && rc=0 ;;\n *) exit 9 ;;\nesac`,
|
||||
`[ "$rc" -eq 0 ] && mkdir -p node_modules && cp -R ${JSON.stringify(path.join(f.base, 'playwright-stub'))} node_modules/playwright`,
|
||||
'exit "$rc"',
|
||||
].join('\n') + '\n', { mode: 0o755 });
|
||||
const r = f.run();
|
||||
const calls = fs.existsSync(path.join(f.base, 'install-calls')) ? fs.readFileSync(path.join(f.base, 'install-calls'), 'utf8').trim().split('\n') : [];
|
||||
return { f, r, calls };
|
||||
}
|
||||
|
||||
test('the frozen production install, then BROWSER_OK', () => {
|
||||
const { r, calls } = installing(0, 0);
|
||||
expect(r.status, r.stdout + r.stderr).toBe(0);
|
||||
expect(calls).toEqual(['install --production --frozen-lockfile']);
|
||||
expect(lastLine(r.stdout)).toBe('BROWSER_OK node 141.0.7390.37');
|
||||
});
|
||||
|
||||
test('a stale lockfile is refreshed, then installed production-only again', () => {
|
||||
const { r, calls } = installing(1, 0);
|
||||
expect(r.status, r.stdout + r.stderr).toBe(0);
|
||||
expect(calls).toEqual(['install --production --frozen-lockfile', 'install --lockfile-only', 'install --production --frozen-lockfile']);
|
||||
expect(lastLine(r.stdout)).toBe('BROWSER_OK node 141.0.7390.37');
|
||||
});
|
||||
|
||||
test('a failed install is BROWSER_UNAVAILABLE with the fix line', () => {
|
||||
const { f, r, calls } = installing(1, 1);
|
||||
expect(r.status).toBe(1);
|
||||
expect(calls).toEqual(['install --production --frozen-lockfile', 'install --lockfile-only']);
|
||||
expect(lastLine(r.stdout)).toBe(`BROWSER_UNAVAILABLE bun install failed under ${f.root}`);
|
||||
expect(r.stderr).toContain(`fix: cd ${f.root} && ./setup, then re-run`);
|
||||
});
|
||||
});
|
||||
|
||||
describe('browse preflight wiring (scripts/resolvers/browse.ts)', () => {
|
||||
test('every browser skill\'s $B detection runs gstack-browser-ensure from the install root before READY', () => {
|
||||
for (const host of ['claude', 'codex'] as const) {
|
||||
|
||||
@@ -33,11 +33,16 @@ function sliceFunction(source: string, name: string): string {
|
||||
return source.slice(start + 1, end + 3);
|
||||
}
|
||||
|
||||
/** A bun that records each argv line; `install` with --frozen-lockfile exits $FROZEN_RC. */
|
||||
/**
|
||||
* A bun that records each argv line. Like Bun, a --frozen-lockfile install fails
|
||||
* ($FROZEN_RC) until `install --lockfile-only` has refreshed the lockfile; any other
|
||||
* call exits $FALLBACK_RC.
|
||||
*/
|
||||
function stubBun(base: string): { bun: string; calls: () => string[] } {
|
||||
const log = path.join(base, 'calls');
|
||||
const bun = path.join(base, 'bun');
|
||||
fs.writeFileSync(bun, `#!/usr/bin/env bash\necho "$*" >> ${JSON.stringify(log)}\ncase "$*" in *--frozen-lockfile*) exit "\${FROZEN_RC:-0}" ;; esac\nexit 0\n`, { mode: 0o755 });
|
||||
const fresh = path.join(base, 'lockfile-refreshed');
|
||||
fs.writeFileSync(bun, `#!/usr/bin/env bash\necho "$*" >> ${JSON.stringify(log)}\ncase "$*" in\n *--frozen-lockfile*) [ -f ${JSON.stringify(fresh)} ] && exit 0; exit "\${FROZEN_RC:-0}" ;;\n *--lockfile-only*) rc="\${FALLBACK_RC:-0}"; [ "$rc" -eq 0 ] && : > ${JSON.stringify(fresh)}; exit "$rc" ;;\nesac\nexit "\${FALLBACK_RC:-0}"\n`, { mode: 0o755 });
|
||||
return { bun, calls: () => (fs.existsSync(log) ? fs.readFileSync(log, 'utf-8').trim().split('\n') : []) };
|
||||
}
|
||||
|
||||
@@ -46,7 +51,7 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende
|
||||
const script = (bun: string, call: string) =>
|
||||
`set -e\nBUN_CMD=${JSON.stringify(bun)}\n${sliceFunction(setup, 'bun_cmd')}\n${sliceFunction(setup, 'install_runtime_deps')}\n${call}\n`;
|
||||
|
||||
test('install_runtime_deps passes --production with the frozen lockfile, and keeps it on the fallback', () => {
|
||||
test('install_runtime_deps passes --production with the frozen lockfile, and refreshes a stale lockfile before installing production-only again', () => {
|
||||
const base = tempBase();
|
||||
const { bun, calls } = stubBun(base);
|
||||
expect(runBashScript(script(bun, 'install_runtime_deps'), { env: { ...process.env, FROZEN_RC: '0' } }).status).toBe(0);
|
||||
@@ -55,7 +60,18 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende
|
||||
const base2 = tempBase();
|
||||
const stub2 = stubBun(base2);
|
||||
expect(runBashScript(script(stub2.bun, 'install_runtime_deps'), { env: { ...process.env, FROZEN_RC: '1' } }).status).toBe(0);
|
||||
expect(stub2.calls()).toEqual(['install --production --frozen-lockfile', 'install --production']);
|
||||
expect(stub2.calls()).toEqual(['install --production --frozen-lockfile', 'install --lockfile-only', 'install --production --frozen-lockfile']);
|
||||
|
||||
// Generated by /ship Step 7 coverage audit (GSTA-97).
|
||||
// Value: protects=a failed fallback install stops setup's install step instead of building on a missing node_modules;
|
||||
// fails_when=install_runtime_deps swallows the fallback's failure (an added `|| true` or a trailing command);
|
||||
// why_new=both rows above end in a successful install; seam=none; no_claim=setup's own subshell wiring around the call
|
||||
const base3 = tempBase();
|
||||
const stub3 = stubBun(base3);
|
||||
const failed = runBashScript(script(stub3.bun, 'install_runtime_deps\necho AFTER'), { env: { ...process.env, FROZEN_RC: '1', FALLBACK_RC: '3' } });
|
||||
expect(failed.status).toBe(3);
|
||||
expect(failed.stdout).not.toContain('AFTER');
|
||||
expect(stub3.calls()).toEqual(['install --production --frozen-lockfile', 'install --lockfile-only']);
|
||||
});
|
||||
|
||||
test('every bun install in setup goes through install_runtime_deps', () => {
|
||||
@@ -71,7 +87,7 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende
|
||||
test('gstack-browser-ensure installs production dependencies too', () => {
|
||||
const line = read('bin/gstack-browser-ensure').split('\n').find((l) => /bun install/.test(l) && !l.trim().startsWith('#') && !/log /.test(l))!;
|
||||
expect(line).toContain('bun install --production --frozen-lockfile');
|
||||
expect(line).toContain('|| bun install --production >/dev/null');
|
||||
expect(line).toContain('|| { bun install --lockfile-only >/dev/null && bun install --production --frozen-lockfile >/dev/null; }');
|
||||
});
|
||||
|
||||
test('bin/dev-setup installs the full set even over a production-only node_modules', () => {
|
||||
@@ -87,11 +103,87 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende
|
||||
});
|
||||
});
|
||||
|
||||
// Generated by /ship Step 7 coverage audit (GSTA-97).
|
||||
// Value: protects=with the real Bun, install_runtime_deps omits devDependencies on a fresh install and keeps the ones a full install already put there;
|
||||
// fails_when=Bun starts pruning devDependencies on --production (a later ./setup strips a contributor's toolchain) or --production stops excluding them;
|
||||
// why_new=every other test here stubs bun, so the CHANGELOG and CONTRIBUTING claim that ./setup never removes packages was untested;
|
||||
// seam=none; no_claim=registry packages or the repo's own bun.lock (two local file: packages stand in; offline)
|
||||
describe.skipIf(process.platform === 'win32')('install_runtime_deps with the real Bun', () => {
|
||||
test('a fresh install leaves out devDependencies; a later production install keeps the ones a full install added', () => {
|
||||
const base = tempBase();
|
||||
const project = path.join(base, 'project');
|
||||
for (const name of ['runtime-pkg', 'dev-pkg']) {
|
||||
fs.mkdirSync(path.join(project, 'pkgs', name), { recursive: true });
|
||||
fs.writeFileSync(path.join(project, 'pkgs', name, 'package.json'), JSON.stringify({ name, version: '1.0.0' }));
|
||||
}
|
||||
fs.writeFileSync(path.join(project, 'package.json'), JSON.stringify({
|
||||
name: 'prod-deps-fixture', version: '0.0.0',
|
||||
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 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`,
|
||||
{ cwd: project, env, timeout: 90_000 });
|
||||
const has = (name: string) => fs.existsSync(path.join(project, 'node_modules', name, 'package.json'));
|
||||
|
||||
const lockfile = run('bun_cmd install');
|
||||
expect(lockfile.status, lockfile.stderr).toBe(0);
|
||||
const lock = fs.readFileSync(path.join(project, 'bun.lock'), 'utf-8');
|
||||
fs.rmSync(path.join(project, 'node_modules'), { recursive: true, force: true });
|
||||
|
||||
const fresh = run('install_runtime_deps');
|
||||
expect(fresh.status, fresh.stderr).toBe(0);
|
||||
expect([has('runtime-pkg'), has('dev-pkg')]).toEqual([true, false]);
|
||||
|
||||
const full = run('bun_cmd install');
|
||||
expect(full.status, full.stderr).toBe(0);
|
||||
expect(has('dev-pkg')).toBe(true);
|
||||
|
||||
const later = run('install_runtime_deps');
|
||||
expect(later.status, later.stderr).toBe(0);
|
||||
expect([has('runtime-pkg'), has('dev-pkg')]).toEqual([true, true]);
|
||||
expect(fs.readFileSync(path.join(project, 'bun.lock'), 'utf-8')).toBe(lock);
|
||||
});
|
||||
|
||||
// Value: protects=a checkout whose bun.lock is behind package.json (a fork, a local edit) still installs, production-only;
|
||||
// fails_when=the fallback reruns a --production install, which Bun treats as frozen, so setup aborts on the stale lockfile;
|
||||
// why_new=the stub-bun rows cannot model Bun's rule that --production implies a frozen lockfile;
|
||||
// seam=none; no_claim=registry resolution (local file: packages; offline)
|
||||
test('a stale lockfile is refreshed, then the install stays production-only', () => {
|
||||
const base = tempBase();
|
||||
const project = path.join(base, 'project');
|
||||
for (const name of ['runtime-pkg', 'late-pkg', 'dev-pkg']) {
|
||||
fs.mkdirSync(path.join(project, 'pkgs', name), { recursive: true });
|
||||
fs.writeFileSync(path.join(project, 'pkgs', name, 'package.json'), JSON.stringify({ name, version: '1.0.0' }));
|
||||
}
|
||||
const manifest = (deps: Record<string, string>) => 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 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`,
|
||||
{ cwd: project, env, timeout: 90_000 });
|
||||
const has = (name: string) => fs.existsSync(path.join(project, 'node_modules', name, 'package.json'));
|
||||
|
||||
manifest({ 'runtime-pkg': 'file:./pkgs/runtime-pkg' });
|
||||
expect(run('bun_cmd install --lockfile-only').status).toBe(0);
|
||||
manifest({ 'runtime-pkg': 'file:./pkgs/runtime-pkg', 'late-pkg': 'file:./pkgs/late-pkg' });
|
||||
|
||||
const stale = run('install_runtime_deps');
|
||||
expect(stale.status, stale.stderr).toBe(0);
|
||||
expect([has('runtime-pkg'), has('late-pkg'), has('dev-pkg')]).toEqual([true, true, false]);
|
||||
expect(fs.readFileSync(path.join(project, 'bun.lock'), 'utf-8')).toContain('late-pkg');
|
||||
});
|
||||
});
|
||||
|
||||
/** Bare package names a source file loads (import/require/dynamic import), type-only imports excluded. */
|
||||
export function importedPackages(text: string): string[] {
|
||||
function importedPackages(text: string): string[] {
|
||||
const out = new Set<string>();
|
||||
const patterns = [
|
||||
/^\s*import\s+(?!type\b)[^'"\n]*?from\s+['"]([^'"]+)['"]/gm,
|
||||
/^\s*(?:import|export)\s+(?!type\b)[^'";]*?from\s+['"]([^'"]+)['"]/gm,
|
||||
/^\s*import\s+['"]([^'"]+)['"]/gm,
|
||||
/\brequire\(\s*['"]([^'"]+)['"]\s*\)/g,
|
||||
/\bimport\(\s*['"]([^'"]+)['"]\s*\)/g,
|
||||
@@ -109,7 +201,7 @@ export function importedPackages(text: string): string[] {
|
||||
}
|
||||
|
||||
/** Every shipped-runtime file that loads a package which is not a production dependency. */
|
||||
export function nonProductionImports(pkg: { dependencies?: Record<string, string> }, files: Record<string, string>): string[] {
|
||||
function nonProductionImports(pkg: { dependencies?: Record<string, string> }, files: Record<string, string>): string[] {
|
||||
const deps = pkg.dependencies ?? {};
|
||||
const bad: string[] = [];
|
||||
for (const [file, text] of Object.entries(files)) {
|
||||
@@ -145,9 +237,42 @@ describe('setup and runtime need only production dependencies', () => {
|
||||
expect(nonProductionImports(PKG, files)).toEqual([]);
|
||||
});
|
||||
|
||||
// Generated by /ship Step 7 coverage audit (GSTA-97).
|
||||
// Value: protects=modules the shipped entry points load by relative import (gen-skill-docs' scripts/ helpers) use production dependencies only;
|
||||
// fails_when=a scripts/ module gen-skill-docs imports adds a devDependency import, breaking every user render;
|
||||
// why_new=runtimeFiles() scans fixed roots, and scripts/discover-skills.ts, gen-agents-digest.ts and the rest are outside them;
|
||||
// seam=none; no_claim=dynamic import paths built at runtime, or packages loaded by a spawned subprocess
|
||||
test('every module the shipped code reaches by relative import loads only production dependencies', () => {
|
||||
const files = runtimeFiles();
|
||||
const reached: Record<string, string> = {};
|
||||
const queue = Object.keys(files);
|
||||
const relSpec = /^\s*(?:import|export)\s+(?!type\b)[^'";]*?from\s+['"](\.\.?\/[^'"]+)['"]|\b(?:import|require)\(\s*['"](\.\.?\/[^'"]+)['"]\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 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 || /(^|\/)(test|node_modules)\//.test(target) || !/\.(ts|js|mjs)$/.test(target)) continue;
|
||||
reached[target] = fs.readFileSync(path.join(ROOT, target), 'utf-8');
|
||||
queue.push(target);
|
||||
}
|
||||
}
|
||||
expect(Object.keys(reached)).toContain('scripts/discover-skills.ts');
|
||||
expect(nonProductionImports(PKG, reached)).toEqual([]);
|
||||
});
|
||||
|
||||
test('negative controls: a devDependency import and a moved-back runtime package are caught', () => {
|
||||
expect(nonProductionImports(PKG, { 'bin/x': "import { query } from '@anthropic-ai/claude-agent-sdk';" })).toEqual(['bin/x: @anthropic-ai/claude-agent-sdk']);
|
||||
expect(nonProductionImports(PKG, { 'bin/x': "import type { Q } from '@anthropic-ai/claude-agent-sdk';\nimport fs from 'node:fs';" })).toEqual([]);
|
||||
expect(nonProductionImports(PKG, { 'lib/x.ts': "export { query } from '@anthropic-ai/claude-agent-sdk';" })).toEqual(['lib/x.ts: @anthropic-ai/claude-agent-sdk']);
|
||||
expect(nonProductionImports(PKG, { 'lib/x.ts': "export * from 'typescript';" })).toEqual(['lib/x.ts: typescript']);
|
||||
expect(nonProductionImports(PKG, { 'lib/x.ts': "export type { Q } from '@anthropic-ai/claude-agent-sdk';" })).toEqual([]);
|
||||
// Generated by /ship Step 7 coverage audit (GSTA-97).
|
||||
// Value: protects=the scan sees an import statement split over several lines;
|
||||
// fails_when=importedPackages only matches one-line imports, so a multi-line devDependency import ships unflagged;
|
||||
// why_new=both existing controls are one-line imports; seam=none; no_claim=none beyond protects
|
||||
expect(nonProductionImports(PKG, { 'bin/x': "import {\n query,\n type SDKMessage,\n} from '@anthropic-ai/claude-agent-sdk';" })).toEqual(['bin/x: @anthropic-ai/claude-agent-sdk']);
|
||||
const { ['@anthropic-ai/sdk']: _sdk, ...rest } = PKG.dependencies;
|
||||
const files = runtimeFiles();
|
||||
expect(nonProductionImports({ dependencies: rest }, files)).toContain('bin/gstack-distill-free-text: @anthropic-ai/sdk');
|
||||
|
||||
Reference in new issue
Block a user