mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 16:34:00 +01:00
fix: pre-landing review fixes, pass 2 (GSTA-97)
- gstack-browser-ensure's failure line and the README tip suggest the frozen production install (./setup skips the install when the build is current, and a bare --production install is frozen anyway). - setup install guard catches quoted and unquoted $BUN_CMD spellings. - Replace the exact-text gstack-browser-ensure grep with an invariant: no full bun install; behavior is covered by gstack-browser-ensure.test.ts. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
a3d402d07e
commit
b5b8820df8
5 files changed
+25
-12
No files matched your search
+2
-2
@@ -5,7 +5,7 @@
|
||||
**A fresh install is about 520 MB smaller: `./setup` installs production dependencies only.** Setup ran `bun install` without `--production`, so every user also got the test and eval toolchain from `devDependencies`, mostly the Claude Agent SDK's native binaries (both the glibc and musl builds on Linux, ~440 MB). Measured on a clean Ubuntu 24.04 container with a cold Bun cache (4 vCPU, two runs each): `node_modules` goes from 1,094 MB to 572 MB, the checkout from 1.74 GB to 1.22 GB, and the whole install with Chromium from 2.42 GB to 1.90 GB. `./setup` took 24.6 s and 20.0 s before, 21.1 s and 18.9 s after.
|
||||
|
||||
### Changed
|
||||
- **`setup`**: every dependency install (before the build, the Codex render and each host render) goes through one `install_runtime_deps` function: `bun install --production --frozen-lockfile`. Bun treats `--production` as frozen, so when `bun.lock` is behind `package.json` (a fork or a local edit) it refreshes the lockfile with `bun install --lockfile-only` and installs production-only again. **`bin/gstack-browser-ensure`** installs the same way when `node_modules` is missing, and its failure line now points to `./setup`.
|
||||
- **`setup`**: every dependency install (before the build, the Codex render and each host render) goes through one `install_runtime_deps` function: `bun install --production --frozen-lockfile`. Bun treats `--production` as frozen, so when `bun.lock` is behind `package.json` (a fork or a local edit) it refreshes the lockfile with `bun install --lockfile-only` and installs production-only again. **`bin/gstack-browser-ensure`** installs the same way when `node_modules` is missing, and its failure line suggests the production install rather than a full `bun install`.
|
||||
- **`package.json`**: `@anthropic-ai/sdk` (loaded by `gstack-distill-free-text`, /plan-tune's dream cycle) and `xterm` and `xterm-addon-fit` (copied into the browser extension by `bun run build`) move to `dependencies`, because a user install runs them. The Claude Agent SDK, TypeScript, Prettier and `@types/bun` stay dev-only.
|
||||
- **Contributors and CI keep the full install.** Bun never removes packages that are already installed, so a full `bun install` survives any later `./setup`. `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.
|
||||
|
||||
@@ -14,7 +14,7 @@
|
||||
|
||||
### 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. 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) 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 the `./setup` fix line.
|
||||
- `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.
|
||||
|
||||
### Gate integrity
|
||||
|
||||
@@ -790,7 +790,7 @@ A project install lives in the project's `.claude/skills/gstack` or
|
||||
|
||||
**`/browse` (or `/qa`, `/design-review`) says `NEEDS_ASIDE` or `ASIDE_NOT_RUNNING`?** That's the probe telling you it's about to use the fallback browser. Want Aside? Open the app and sign in — `aside --version` should print a version and `aside repl 'console.log("ok")'` should print `ok` — then re-run. gstack never installs it for you. Want the fallback on purpose while Aside is open? `GSTACK_SKIP_ASIDE=1` makes every skill, the renderer, and `./setup` treat Aside as absent. When Aside is absent the probe prints `NEEDS_ASIDE: <OS>` and skills trust that line for the macOS-only download pitch; `GSTACK_PLATFORM` overrides the OS it names, for tests and unusual hosts (set it in your shell — gstack never reads it from a project `.env`).
|
||||
|
||||
**`/browse` fails on the fallback browser?** `cd ~/.claude/skills/gstack && bun install --production && bun run build`
|
||||
**`/browse` fails on the fallback browser?** `cd ~/.claude/skills/gstack && bun install --production --frozen-lockfile && bun run build`
|
||||
|
||||
**`/make-pdf` or `/diagram` can't render?** Same two paths: with Aside open they print through Aside (`bun run ~/.claude/skills/gstack/bin/gstack-render.ts some.html --screenshot /tmp/out.png` tests it directly, and its first line, `ENGINE=aside` or `ENGINE=browse`, names the browser that actually rendered); without it they use the bundled browser, so `bun run build` is the fix.
|
||||
|
||||
|
||||
@@ -73,7 +73,7 @@ if [ ! -d "$ROOT/node_modules/playwright" ]; then
|
||||
[ "$CHECK" -eq 1 ] && fail "playwright is not installed under $ROOT/node_modules" "cd $ROOT && ./setup"
|
||||
command -v bun >/dev/null 2>&1 || fail "playwright is not installed under $ROOT/node_modules and bun is not on PATH" "install Bun, then cd $ROOT && ./setup"
|
||||
log "gstack-browser-ensure: installing node modules (bun install)..."
|
||||
(cd "$ROOT" && { bun install --production --frozen-lockfile >/dev/null 2>&1 || { bun install --lockfile-only >/dev/null && bun install --production --frozen-lockfile >/dev/null; }; }) || fail "bun install failed under $ROOT" "cd $ROOT && ./setup, then re-run $0"
|
||||
(cd "$ROOT" && { bun install --production --frozen-lockfile >/dev/null 2>&1 || { bun install --lockfile-only >/dev/null && bun install --production --frozen-lockfile >/dev/null; }; }) || fail "bun install failed under $ROOT" "cd $ROOT && bun install --production --frozen-lockfile, then re-run $0"
|
||||
fi
|
||||
|
||||
probe
|
||||
|
||||
@@ -179,7 +179,7 @@ describe('gstack-browser-ensure installs a missing node_modules production-only
|
||||
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`);
|
||||
expect(r.stderr).toContain(`fix: cd ${f.root} && bun install --production --frozen-lockfile, then re-run`);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -76,18 +76,31 @@ describe.skipIf(process.platform === 'win32')('setup installs production depende
|
||||
|
||||
test('every bun install in setup goes through install_runtime_deps', () => {
|
||||
const body = setup.replace(sliceFunction(setup, 'install_runtime_deps'), '');
|
||||
const direct = body.split('\n').filter((l) => /\b(bun_cmd|"\$BUN_CMD"|bun)\s+install\b/.test(l) && !l.trim().startsWith('#'));
|
||||
const directInstall = /(?:^|[\s;&|(])(?:bun_cmd|"?\$\{?BUN_CMD\}?"?|bun)\s+install\b/;
|
||||
const direct = body.split('\n').filter((l) => directInstall.test(l) && !l.trim().startsWith('#'));
|
||||
expect(direct).toEqual([]);
|
||||
// The build, the Codex render and each host render install first.
|
||||
expect(setup.match(/^\s+install_runtime_deps$/gm)?.length).toBeGreaterThanOrEqual(7);
|
||||
// Negative control: the pre-GSTA-97 line is caught by the same filter.
|
||||
expect(/\b(bun_cmd|"\$BUN_CMD"|bun)\s+install\b/.test(' bun_cmd install --frozen-lockfile 2>/dev/null || bun_cmd install')).toBe(true);
|
||||
// Negative controls: the pre-GSTA-97 line and every other spelling of a direct install are caught.
|
||||
for (const line of [
|
||||
' bun_cmd install --frozen-lockfile 2>/dev/null || bun_cmd install',
|
||||
' "$BUN_CMD" install --frozen-lockfile',
|
||||
' $BUN_CMD install',
|
||||
' "${BUN_CMD}" install',
|
||||
' (cd x && bun install)',
|
||||
]) expect(directInstall.test(line), line).toBe(true);
|
||||
expect(directInstall.test(' bun_cmd x playwright install chromium')).toBe(false);
|
||||
});
|
||||
|
||||
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 --lockfile-only >/dev/null && bun install --production --frozen-lockfile >/dev/null; }');
|
||||
test('gstack-browser-ensure never runs a full bun install', () => {
|
||||
const fullInstalls = (text: string) => text.split('\n')
|
||||
.filter((l) => !l.trim().startsWith('#') && !/^\s*log /.test(l))
|
||||
.flatMap((l) => [...l.matchAll(/\bbun install\b(?! --production| --lockfile-only| failed)/g)].map(() => l.trim()));
|
||||
const ensure = read('bin/gstack-browser-ensure');
|
||||
expect(ensure).toContain('bun install --production --frozen-lockfile');
|
||||
expect(fullInstalls(ensure)).toEqual([]);
|
||||
// Negative control: the pre-GSTA-97 install line is caught.
|
||||
expect(fullInstalls(' (cd "$ROOT" && { bun install --frozen-lockfile >/dev/null 2>&1 || bun install >/dev/null; })')).toHaveLength(2);
|
||||
});
|
||||
|
||||
test('bin/dev-setup installs the full set even over a production-only node_modules', () => {
|
||||
|
||||
Reference in new issue
Block a user