diff --git a/TODOS.md b/TODOS.md index abc4c831c..12d02f317 100644 --- a/TODOS.md +++ b/TODOS.md @@ -425,10 +425,17 @@ From the approved 2026-10 test/eval/CI audit plan; the audit PR files these. - **`scripts/ship-measure.ts` on env-var hosts** — /ship's measure loop runs `$GSTACK_ROOT/scripts/ship-measure.ts`, which imports gstack's test helpers and eval harness, so a Codex/Factory/... runtime root would need most of the - development tree. It is the one `$GSTACK_ROOT` path - `test/runtime-root-assets.test.ts` defers. Either make ship-measure - self-contained or have the skill call it from the source checkout - (`.source-path`). **Effort:** M. **Priority:** P3. + development tree. `bin/gstack-model-benchmark` (/benchmark-models imports + the test tree's providers) and `bin/gstack-ship-queue` (the merge queue + imports the eval gate) have the same problem and fail to start from a + copied Windows root. `test/runtime-root-assets.test.ts` defers all three. + For each, either make it self-contained or have the skill call it from the + source checkout (`.source-path`). That test also follows static imports + only: lazy `import()` and `require()` calls (`bin/gstack-evidence` loads + `scripts/lib/shard-engine` with a fallback, and + `lib/pregate/flaky-exception.ts` requires it and a test helper), modules a + shell script launches, and bare package imports go unchecked. + **Effort:** M. **Priority:** P3. - **Rendered `SKILL.md` copies inside runtime roots are discoverable** — `office-hours/SKILL.md` (#2449) and now `plan-design-review/SKILL.md` sit in `~/.codex/skills/gstack//` because skills read them as diff --git a/changelog.d/keep-review-working-in-copied-windows-installs.md b/changelog.d/keep-review-working-in-copied-windows-installs.md new file mode 100644 index 000000000..adfb5ae4a --- /dev/null +++ b/changelog.d/keep-review-working-in-copied-windows-installs.md @@ -0,0 +1,4 @@ +--- +important: blocker +--- +- **/review and /ship start again on Windows under Codex, Cursor and the other non-Claude hosts.** On Windows, setup copies gstack's `bin/` and `lib/` into each host's runtime folder instead of linking them, and it left out three `scripts/` files those commands import. The review planner that /review and /ship run first stopped with `Cannot find module '../scripts/resolvers/fix-check'`, and the commands behind /retro and /write-skill had the same gap. Setup now copies `scripts/resolvers/fix-check.ts`, `scripts/discover-skills.ts` and `scripts/lib/ci-history.ts` too, and a test follows the static imports of the commands each host's skills run, so a new static import outside the runtime folder fails CI. Thanks to @SatriaSuriaAriffin, who found and reported the fix-check gap (#4275). diff --git a/setup b/setup index 56fded0f8..c969f3d7d 100755 --- a/setup +++ b/setup @@ -790,9 +790,11 @@ _link_or_copy() { # residue): the preamble's jargon list, question registry and AskUserQuestion # docs, the review checklists and specialists, /plan-devex-review's Hall of # Fame, the test-value bar, design-html's vendored pretext.js and VERSION. -# And crew/lib, which bin and lib import (the crew/ move, GST-1833): a root -# whose bin and lib are copies (Windows) needs it beside them to start. -# Pinned by test/runtime-root-assets.test.ts. A destination that already exists +# And crew/lib, which bin and lib import (the crew/ move, GST-1833), plus the +# scripts/ modules they import: a root whose bin and lib are copies (Windows) +# needs them beside it to start (/review and /ship's review planner imports +# fix-check.ts). +# Pinned by test/runtime-root-assets.test.ts, which also follows those imports. A destination that already exists # as a real entry is left alone: in the repo-local sidecar review/ is a link to # the source, so its files already resolve and must never be linked over. _link_runtime_dists() { @@ -813,6 +815,7 @@ _link_runtime_dists() { fi done for f in VERSION scripts/jargon-list.json scripts/question-registry.ts \ + scripts/resolvers/fix-check.ts scripts/discover-skills.ts scripts/lib/ci-history.ts \ docs/askuserquestion-split.md docs/askuserquestion-cjk.md docs/test-value-bar.md \ review/checklist.md review/design-checklist.md review/greptile-triage.md review/TODOS-format.md \ plan-devex-review/dx-hall-of-fame.md; do diff --git a/test/runtime-root-assets.test.ts b/test/runtime-root-assets.test.ts index d18eed0be..5e5cb3635 100644 --- a/test/runtime-root-assets.test.ts +++ b/test/runtime-root-assets.test.ts @@ -20,6 +20,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { HOST_CONFIG_MAP } from '../hosts'; +import { bunIsolationArgs } from '../lib/bun-isolation'; import { runGeneration } from '../scripts/gen-skill-docs'; const ROOT = path.resolve(import.meta.dir, '..'); @@ -43,7 +44,9 @@ const ROOT_BUILDERS: Record = { // Deferred, with the reason recorded in TODOS.md: /ship's measure loop runs // scripts/ship-measure.ts, which imports gstack's test helpers and eval // harness; a runtime root would need the whole development tree. -const DEFERRED = new Set(['scripts/ship-measure.ts']); +// gstack-model-benchmark (/benchmark-models) and the merge queue's +// gstack-ship-queue are deferred the same way: both import that tree. +const DEFERRED = new Set(['scripts/ship-measure.ts', 'bin/gstack-model-benchmark', 'bin/gstack-ship-queue']); // Compiled outputs: the stub source carries placeholder binaries, so a fresh // checkout without a build still exercises the links to them. @@ -97,17 +100,52 @@ function makeSource(): void { } } -function buildRoot(host: string): { root: string; stderr: string; status: number | null } { - const root = path.join(tmp, 'home', host, 'skills', 'gstack'); +// windows: setup's IS_WINDOWS=1 arm, where bin/, lib/ and the runtime dists +// are real copies instead of links, so a module they import resolves only if +// setup copied it beside them. It copies from the checkout itself, because +// cp -R copies the stub source's top-level links as links. +function buildRoot(host: string, windows = false): { root: string; stderr: string; status: number | null } { + const root = path.join(tmp, windows ? 'home-windows' : 'home', host, 'skills', 'gstack'); fs.mkdirSync(path.dirname(root), { recursive: true }); const functions = ['_link_or_copy', '_link_runtime_dists', '_copy_runtime_skill_refs', '_copy_skill_md', '_sidecar_root_user_owned', '_gstack_generated_header', ROOT_BUILDERS[host]!] .filter(name => SETUP_SRC.includes(`\n${name}() {`)).map(extractFunction).join('\n'); - const r = spawnSync('bash', ['-c', `set -e\nIS_WINDOWS=0\n${functions}\n${ROOT_BUILDERS[host]} "${src}" "${root}"`], + const r = spawnSync('bash', ['-c', `set -e\nIS_WINDOWS=${windows ? 1 : 0}\n${functions}\n${ROOT_BUILDERS[host]} "${windows ? ROOT : src}" "${root}"`], { encoding: 'utf8', timeout: 30_000, env: { PATH: process.env.PATH ?? '/usr/bin:/bin', HOME: path.join(tmp, 'home') } }); return { root, stderr: r.stderr, status: r.status }; } +const transpiler = new Bun.Transpiler({ loader: 'ts' }); +function isBunSource(file: string): boolean { + if (!fs.existsSync(file) || !fs.statSync(file).isFile()) return false; + return /\.(ts|js|mjs)$/.test(file) || (!path.extname(file) && /^#!.*\bbun\b/.test(fs.readFileSync(file, 'utf8'))); +} + +/** + * Static imports reachable from the code a host's skills run that do not + * resolve inside its runtime root. Paths resolve from where the root holds + * each file, never through its links, which is how a copied root loads them. + */ +function importsOutsideRoot(host: string, root: string): string[] { + const queue = [...referencedPaths(host)].filter(rel => !DEFERRED.has(rel)).map(rel => path.join(root, rel)).filter(isBunSource); + const seen = new Set(); + const outside = new Set(); + while (queue.length) { + const file = queue.pop()!; + if (seen.has(file)) continue; + seen.add(file); + for (const { path: spec, kind } of transpiler.scanImports(fs.readFileSync(file, 'utf8').replace(/^#!.*/, ''))) { + if (kind !== 'import-statement' || !spec.startsWith('.')) continue; + const target = path.resolve(path.dirname(file), spec); + const found = [target, target.replace(/\.js$/, '.ts'), `${target}.ts`, `${target}.js`, path.join(target, 'index.ts')] + .find(c => c.startsWith(root + path.sep) && fs.existsSync(c) && fs.statSync(c).isFile()); + if (!found) outside.add(`${path.relative(root, file)} -> ${path.relative(root, target)}`); + else if (isBunSource(found)) queue.push(found); + } + } + return [...outside].sort(); +} + beforeAll(async () => { const result = await runGeneration({ host: 'all', outputRoot: renderDir }); if (result.exitCode !== 0) throw new Error(result.diagnostics.filter(d => d.kind === 'error').map(d => d.message).join('\n')); @@ -139,6 +177,26 @@ describe.skipIf(process.platform === 'win32')('runtime roots carry every $GSTACK }); } + for (const host of Object.keys(ROOT_BUILDERS)) { + test(`${host}: every module the skills' commands import is inside the runtime root`, () => { + const { root, status } = buildRoot(host); + expect(status).toBe(0); + expect(importsOutsideRoot(host, root)).toEqual([]); + }); + } + + test('the review planner starts from a copied (Windows) runtime root', () => { + const { root, stderr, status } = buildRoot('codex', true); + expect(stderr).toBe(''); + expect(status).toBe(0); + expect(fs.lstatSync(path.join(root, 'bin')).isSymbolicLink()).toBe(false); + const r = spawnSync(process.execPath, [...bunIsolationArgs(), path.join(root, 'bin', 'gstack-review-plan'), '--help'], + { cwd: tmp, encoding: 'utf8', timeout: 30_000, env: { PATH: process.env.PATH ?? '/usr/bin:/bin', HOME: path.join(tmp, 'home-windows') } }); + expect(r.stderr).not.toContain('Cannot find module'); + expect(r.status).toBe(0); + expect(r.stdout).toContain('--fix-check-prompt'); + }); + test('a runtime root never gains a whole skill directory beside the rendered copies', () => { const { root } = buildRoot('codex'); for (const dir of ['plan-design-review', 'office-hours']) { diff --git a/test/setup-production-deps.test.ts b/test/setup-production-deps.test.ts index 5289b4e13..38b3e4d10 100644 --- a/test/setup-production-deps.test.ts +++ b/test/setup-production-deps.test.ts @@ -572,7 +572,8 @@ describe('setup and runtime need only production dependencies', () => { queue.push(target); } } - expect(Object.keys(reached)).toContain('scripts/discover-skills.ts'); + expect(Object.keys(reached)).toContain('scripts/gen-agents-digest.ts'); + expect(Object.keys({ ...files, ...reached })).toContain('scripts/discover-skills.ts'); expect(Object.keys(reached)).toContain('test/helpers/providers/claude.ts'); expect(nonProductionImports(PKG, reached)).toEqual([]); });