diff --git a/codev/projects/bugfix-298-the-npm-pack-test-walks-the-li/status.yaml b/codev/projects/bugfix-298-the-npm-pack-test-walks-the-li/status.yaml new file mode 100644 index 000000000..afba6560b --- /dev/null +++ b/codev/projects/bugfix-298-the-npm-pack-test-walks-the-li/status.yaml @@ -0,0 +1,27 @@ +id: bugfix-298 +title: the-npm-pack-test-walks-the-li +protocol: bugfix +phase: verified +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-31T19:54:45.758Z' + approved_at: '2026-08-31T19:54:53.443Z' + approval: + authorization: flag-only + approved_at: '2026-08-31T19:54:53.443Z' + machine: chriss-MacBook-Pro.local + caller: CODEV_ARCHITECT_NAME=main (an architect session or a process it spawned) +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-31T19:25:27.056Z' +updated_at: '2026-08-31T19:55:09.071Z' +pr_history: + - phase: pr + pr_number: 300 + branch: builder/bugfix-298 + created_at: '2026-08-31T19:45:12.405Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-298_thread.md b/codev/state/bugfix-298_thread.md new file mode 100644 index 000000000..a93adf800 --- /dev/null +++ b/codev/state/bugfix-298_thread.md @@ -0,0 +1,178 @@ +# bugfix-298 — npm pack test walks the live repo + +## INVESTIGATE + +### Reproduced + +Not assumed. Ran `npm pack --dry-run --json --ignore-scripts` at the workspace root +while a background loop created and deleted `packages/codev/.test-race-298/state.db`. +Run 1 exited non-zero with exactly the issue's error: + +``` +npm error code ENOENT +npm error syscall lstat +npm error path .../packages/codev/.test-race-298 +``` + +### Root cause + +`packages/codev/src/__tests__/extension-retirement.test.ts:29-35` — `packedFiles()` +shells out to `npm pack --dry-run --json --ignore-scripts` with `cwd: workspaceRoot`. + +The root `package.json` declares `files: ["*", "!apps/streamdeck", "!apps/vscode"]`, +so npm-packlist walks the **entire live working tree**. Concurrent tests in the same +vitest run create and remove scratch directories under that root — the named example +is `packages/codev/src/agent-farm/__tests__/pir-832-migration.test.ts:18`, which +resolves `.test-pir-832` from `process.cwd()` (= `packages/codev`), `mkdirSync`s it in +`beforeEach` and `rmSync`s it in `afterEach`, once per test case. When the walk lstats +a path that vanished between readdir and lstat, npm exits non-zero, `execFileSync` +throws, and the test fails. + +Deterministic rather than flaky because the pack window is ~62s with `dist/` built and +the scratch directory churns many times inside it. + +### Measured: the walk observes the disk, not the repository + +Live pack list vs `git ls-files`: + +- 43 files packed that git does not track: `.builder-*`, `.claude/hooks/*`, + `*/node_modules/.bin/*`. Working-tree litter, exactly the class of thing that + disappears mid-walk. +- 55 tracked files not packed, all explained by real packaging rules: + `apps/web/.npmignore` (`node_modules`, `src`, `*.config.*`, `tsconfig*`) and npm's + always-excluded names (`.gitignore`, `.npmignore`, `pnpm-lock.yaml`). + +### Fix approach chosen + +File list from **git** (the repository), packaging rules from **npm** (not +reimplemented): + +1. `git ls-files` gives the tracked paths. +2. Materialise them as a placeholder skeleton in `mkdtemp` under the OS temp dir — + real content only for `package.json`, `.npmignore`, `.gitignore` (the only files + npm-packlist reads); every other path an empty file, since the walk only stats it. +3. Run `npm pack --dry-run --json` in that skeleton. + +Verified fidelity: the skeleton's pack list is **identical** to the real tree's pack +list minus the 43 untracked litter files — 3575 vs 3575 entries, zero divergence in +either direction. All four assertions hold on it. + +Cost: 0.3s to build the skeleton + 1.65s to pack ≈ 2s, against ~62s today. + +Rejected: hand-rolling the packaging rules on top of `git ls-files`. It would have to +reimplement `.npmignore` matching and npm's default excludes, and getting that wrong +silently changes 55 files' worth of answer while the test still reads green. Rejected +also: moving scratch directories out of the walk, which narrows the window rather than +removing it. + +### Scope + +Single test file, well under 300 LOC. Fits BUGFIX. + +### Not in scope + +The sibling `existsSync(apps/streamdeck)` assertion in the same file is #297. + +## FIX + +Single file changed: `packages/codev/src/__tests__/extension-retirement.test.ts`. + +`packedFiles()` no longer packs the live working tree. It now: + +1. `git ls-files -z` at the workspace root → tracked paths. +2. Materialises them under `mkdtemp` in the OS temp dir. Real content only for + `package.json`, `.npmignore`, `.gitignore` — the only files npm-packlist reads rather + than stats. Everything else is an empty placeholder. +3. `npm pack --dry-run --json --ignore-scripts` in that fixture, then removes it. + +The packaging rules stay npm's. Nothing about `.npmignore` or npm's default excludes is +reimplemented here. + +### Architect exchange on the comparison + +The architect asked for an in-test assertion that the skeleton and the **live** lists +match. I substituted skeleton vs a **real-content copy of the same git-derived set** and +said why: asserting against the live tree respawns the walk that failed 5 of 5, and the +live-vs-git difference is the 43 untracked litter files, i.e. the bug rather than drift. +The architect agreed and asked for that reasoning to live in the test comment, not only +in the PR body. It does. + +### Tests added + +- `derives the pack list from the repository, not from whatever is on disk` — creates + `packages/codev/.test-bugfix-298/state.db`, of exactly the shape other suites churn, + and asserts it never reaches the list. The regression test for #298. +- `packs the placeholder skeleton identically to the tracked tree with real contents` — + guards the one risk the placeholder trick introduces. + +### Revert verification + +Reverted `packedFiles()` to the live-tree walk and re-ran: + +- Regression test failed: `expected [ '.af-cron/ci-health.yaml', …(3620) ] to not include + 'packages/codev/.test-bugfix-298/state.db'`. +- Under 4 parallel churners creating and deleting 5 scratch dirs per iteration + (~455 iterations each), the pack test failed with the issue's exact error: + `npm error code ENOENT / npm error syscall lstat / lstat + '.../packages/codev/.test-churn-1-4'`. Bare `npm pack` under the same churn: 6 of 6 runs + ENOENT. +- Restored the fix and re-ran under identical churn 3 times: 7 of 7 tests passed each time. + +### Cost + +File went from ~62s to 10.9s for all 7 tests. + +## PR + +PR #300. CMAP: claude=APPROVE, opencode=APPROVE, codex=REQUEST_CHANGES (addressed). + +Gemini was not run. This workspace's `.codev/config.json` sets `porch.consultation.models` to +`["claude", "opencode"]`; Codex was added as the third lane to meet the protocol's three-verdict +requirement. + +### Codex, REQUEST_CHANGES — both points applied + +1. **Remove the review document.** Verified against the protocol rather than taken on trust: + `codev-skeleton/protocols/bugfix/protocol.md:3` reads "No spec, no plan, no artifact files: + the issue is the spec." `codev/reviews/bugfix-298-*.md` was removed. (Older `bugfix-214` and + `bugfix-274` review files exist in the repo, so the precedent is mixed, but the protocol text + is explicit.) Its substance moved into the PR body, which is where BUGFIX keeps the record. +2. **Restructure the PR body** to Summary / Root Cause / Fix / Test Plan. Done. + +### Claude, APPROVE — two of three observations applied + +1. `trackedPaths()` threw a raw `execFileSync` error where the npm call beside it framed its own. + Now framed, naming the step and why git is needed. +2. The old comment carried the #215/#216 timeout provenance and was deleted with it, leaving + `30_000` unexplained. Rationale restored, rewritten for the new cost model: the budget now + grows with the repo rather than with what happens to be on disk. +3. Not applied: a catch-and-stub around the full-content `copyFileSync` to close a residual + ENOENT window if a concurrent suite mutated a *tracked* file. The reviewer withdrew it after + checking that no suite does. Independently confirmed here — all 19 `.test-*` scratch + directories are untracked and resolved from `process.cwd()`. A catch that silently stubs would + also hide a genuinely missing tracked file. + +### Correction carried into the PR body + +The root cause named `pir-832-migration.test.ts` as the concurrent writer. It is one of **19** +distinct `.test-*` scratch directories, all created and removed under `packages/codev` from +`process.cwd()`. Verified by grep, not taken from the review. That is why the failure was reliable +rather than occasional. + +### Architect's two pre-merge asks + +1. **State the fixture's limit in `packedFiles`' doc comment.** Measured it rather than asserting + it: a live walk of a *built* tree returns 4,702 entries against the fixture's 3,575, and 950 of + that difference is build output. `dist/`, `dashboard-dist/`, `v2-dist/` and `client-dist/` are + gitignored, but a `files` array outranks `.gitignore` in npm-packlist, so the live walk ships + them and the tracked-only fixture never will. The comment says so, says it is deliberate, and + names where the built-tarball question is actually asked (`bugfix-214-publish-scrub.test.ts`, + per package). It also records why the four assertions still bite: all turn on tracked paths, and + 216 tracked `apps/vscode/` files would surface the moment the `!apps/vscode` negation stopped + excluding them. +2. **Measured rationale for the 30s timeout.** 2.0s measured, split 0.3s to materialise 3,846 + paths and 1.7s for npm's startup and walk. The multiple carries the point: the old budget had to + be 60s because its cost tracked whatever was on disk (#215 crossed the 10s default the first + time a runner had `dist/`; #216 raised it to 60s). This one tracks the tracked-file count. + +`.test-*` gitignoring is #302, not carried here. diff --git a/packages/codev/src/__tests__/extension-retirement.test.ts b/packages/codev/src/__tests__/extension-retirement.test.ts index 0f31989f8..b8f75c0e0 100644 --- a/packages/codev/src/__tests__/extension-retirement.test.ts +++ b/packages/codev/src/__tests__/extension-retirement.test.ts @@ -1,46 +1,117 @@ import { execFileSync } from 'node:child_process'; -import { existsSync, readFileSync } from 'node:fs'; -import { dirname, join, resolve } from 'node:path'; +import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { basename, dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; const workspaceRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../../../..'); /** - * The root pack's file list, resolved lazily and only for the one test that needs it (#216). - * - * This used to write a real tarball in a `beforeAll` and then shell out to `tar -tzf` purely to - * read a list of names. Three things were wrong with that, and #215 turned the combination red: - * - * 1. **The work far exceeded the assertion.** `npm pack --dry-run --json` returns the same - * list with no tarball written and no `tar` process. The root `package.json` is - * `files: ["*", …]`, so the pack sweeps the whole monorepo — 4,395 files and 27MB packed - * once `packages/codev` is built — and every byte of that was being compressed to disk and - * read back so four `startsWith` checks could run. - * 2. **It ran for every test in the file.** Four of the five tests read files or `pnpm list` - * and need nothing from the pack. In `beforeAll` a slow fixture takes down tests that never - * depended on it: when the hook died, all five were reported skipped. - * 3. **It had no explicit timeout,** so it ran on vitest's 10s default and crossed it by 544ms - * the first time CI had a built `dist/`, `dashboard-dist/` and `v2-dist/` present. + * The only files `npm pack` reads rather than merely stats: the manifest it takes `files` from, + * and the two ignore-rule files it honours at every level. Everything else contributes nothing + * but its path, which is why the skeleton below can stub it empty. + */ +const RULE_FILES = new Set(['package.json', '.npmignore', '.gitignore']); + +/** Repo-relative paths of every file git tracks. `-z` so odd filenames survive the split. */ +function trackedPaths(): string[] { + let raw: string; + try { + raw = execFileSync('git', ['ls-files', '-z'], { + cwd: workspaceRoot, + encoding: 'utf8', + maxBuffer: 1 << 28, + }); + } catch (err) { + // Framed rather than raw, so a non-git checkout says which step failed and why it is needed. + throw new Error( + `Could not list the tracked files under ${workspaceRoot}: \`git ls-files\` failed. ` + + `This test derives what the package ships from the repository, so it needs git. ` + + `${(err as Error).message}`, + ); + } + return raw.split('\0').filter(Boolean); +} + +/** + * Materialise the tracked file set as a standalone tree under the OS temp directory. * - * Note the path shape: `--dry-run --json` yields paths **without** the `package/` prefix that - * `tar -tzf` shows, so the assertion below matches on the bare repo-relative path. + * `withContent: false` writes every path as an empty file except the rule files above, which are + * copied verbatim — enough for npm to apply the real packaging rules, and 55MB cheaper. + * `withContent: true` copies everything, and exists only so the test below can prove the two + * produce the same list. */ -function packedFiles(): string[] { +function buildFixture(label: string, withContent: boolean): string { + const root = mkdtempSync(join(tmpdir(), `codev-pack-${label}-`)); + for (const rel of trackedPaths()) { + const dest = join(root, rel); + mkdirSync(dirname(dest), { recursive: true }); + if (withContent || RULE_FILES.has(basename(rel))) copyFileSync(join(workspaceRoot, rel), dest); + else writeFileSync(dest, ''); + } + return root; +} + +function packListIn(root: string): string[] { const raw = execFileSync('npm', ['pack', '--dry-run', '--json', '--ignore-scripts'], { - cwd: workspaceRoot, + cwd: root, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], maxBuffer: 1 << 28, }); const start = raw.indexOf('['); - if (start === -1) throw new Error(`npm pack produced no JSON for the workspace root: ${raw.slice(0, 200)}`); + if (start === -1) throw new Error(`npm pack produced no JSON for ${root}: ${raw.slice(0, 200)}`); const files: string[] = (JSON.parse(raw.slice(start))[0]?.files ?? []).map((f: { path: string }) => f.path); - // An empty list would satisfy both `not.toContain` assertions below without proving anything. - if (files.length === 0) throw new Error('npm reports an empty tarball for the workspace root'); + // An empty list would satisfy every `not.toContain` assertion below without proving anything. + if (files.length === 0) throw new Error(`npm reports an empty tarball for ${root}`); return files; } +/** + * What the root package ships, derived from the repository rather than from the disk (#298). + * + * This used to run `npm pack --dry-run --json` with `cwd: workspaceRoot`, which walks the entire + * live working tree. Two things were wrong with reading the tree instead of the repo: + * + * 1. **Concurrent tests perturbed it.** Other suites in the same vitest run create and delete + * scratch directories under the workspace root — `agent-farm/__tests__/pir-832-migration.test.ts` + * does it once per test case, in `beforeEach`/`afterEach`. When the walk lstat'd a path that + * had just been removed, npm exited non-zero and this test failed. Not flaky: five consecutive + * full-suite runs, five failures, because the walk took ~62s with `dist/` present and the + * scratch directory churned many times inside that window. + * 2. **It answered a question nobody asked.** The live list carried 43 files git does not + * track — `.builder-*`, files under `.claude/hooks/`, and every package's + * `node_modules/.bin/` shims — none of which say anything about what the package ships. + * + * Taking the paths from `git ls-files` fixes both. The packaging *rules* still come from npm, run + * against the fixture, rather than being reimplemented here — `apps/web/.npmignore` alone + * (`node_modules`, `src`, `*.config.*`, `tsconfig*`) plus npm's default excludes decide 55 tracked + * files, and a second copy of those rules is the copy that drifts. + * + * **The honest limit: this fixture holds only tracked files, so it cannot answer anything about + * build output.** `dist/`, `dashboard-dist/`, `v2-dist/` and `client-dist/` are gitignored, and a + * `files` array outranks `.gitignore` in npm-packlist — so a live walk of a *built* tree carries + * them (measured: 4,702 entries against the fixture's 3,575, 950 of the difference being build + * output) while the fixture never will. That is deliberate. This test asks what the repository + * ships, and the four assertions below all turn on tracked paths: `apps/web` and `apps/v2` package + * manifests are tracked, and 216 tracked `apps/vscode/` files would appear here the moment the + * `!apps/vscode` negation stopped excluding them. A question about the *built* tarball is a + * different question and needs a built tree — `bugfix-214-publish-scrub.test.ts` asks it, per + * package. + * + * Note the path shape: `--dry-run --json` yields paths **without** the `package/` prefix that + * `tar -tzf` shows, so the assertions below match on the bare repo-relative path. + */ +function packedFiles(withContent = false): string[] { + const root = buildFixture(withContent ? 'full' : 'skeleton', withContent); + try { + return packListIn(root); + } finally { + rmSync(root, { recursive: true, force: true }); + } +} + describe('extension retirement', () => { it('deletes the Stream Deck source tree rather than merely excluding its package', () => { expect(existsSync(join(workspaceRoot, 'apps/streamdeck'))).toBe(false); @@ -80,13 +151,11 @@ describe('extension retirement', () => { }); /** - * 60s, deliberately, and not the 10s default that failed. - * - * The budget is set by what the pack actually has to walk, which grows with the repo: 4,395 - * files today, ~4.7s warm on a developer machine, and it crossed 10s on a CI runner the first - * time the built output was present. 60s is roughly 12x the measured local cost — enough - * headroom that ordinary growth and a cold runner do not make this red again, and still short - * enough that a genuine hang fails rather than hanging the job. + * 30s against a measured 2.0s: 0.3s to materialise 3,846 paths, 1.7s for npm's own startup and + * walk of the fixture. 15x, and the multiple is the point — the old live-tree walk needed 60s + * because its cost tracked whatever happened to be on disk, crossing the 10s default in #215 + * the first time a runner had `dist/` present and forcing the 60s budget in #216. This one + * tracks the tracked-file count, which moves with the repo and slowly. */ it('packs neither retired extension while retaining supported apps', () => { const files = packedFiles(); @@ -94,7 +163,46 @@ describe('extension retirement', () => { expect(files).toContain('apps/v2/package.json'); expect(files.some((file) => file.startsWith('apps/vscode/'))).toBe(false); expect(files.some((file) => file.startsWith('apps/streamdeck/'))).toBe(false); - }, 60_000); + }, 30_000); + + /** + * The regression test for #298. + * + * A scratch directory under the workspace root, of exactly the shape other suites create, must + * not reach the pack list at all — if the list cannot see it while it exists, no concurrent + * create or delete of it can perturb the list either. Against the old live-tree walk this file + * was packed (the root `files` field is `["*", …]` and nothing gitignores it), so the assertion + * fails without the fix. + */ + it('derives the pack list from the repository, not from whatever is on disk', () => { + const scratch = join(workspaceRoot, 'packages/codev/.test-bugfix-298'); + mkdirSync(scratch, { recursive: true }); + writeFileSync(join(scratch, 'state.db'), ''); + try { + expect(packedFiles()).not.toContain('packages/codev/.test-bugfix-298/state.db'); + } finally { + rmSync(scratch, { recursive: true, force: true }); + } + }, 30_000); + + /** + * The skeleton stubs every non-rule file empty, which is only sound while npm decides inclusion + * from paths and rule files alone. Packing the same tracked set with its real contents must + * therefore yield an identical list; if a future ignore mechanism starts reading a file the + * skeleton blanks, the two diverge and this goes red rather than the skeleton silently + * answering a different question. + * + * The comparison is deliberately skeleton-against-git rather than skeleton-against-the-live-tree, + * and the next reader will ask why. Two reasons, both load-bearing. Packing the live tree is the + * walk that failed five consecutive full-suite runs, so asserting against it would reintroduce + * #298 inside the test written to close it. And the live-versus-git difference is not drift to + * guard against — it is the 43 untracked litter files, i.e. the bug. The one risk the placeholder + * trick actually adds is npm reading content from a file stubbed empty, and that is exactly what + * this compares. + */ + it('packs the placeholder skeleton identically to the tracked tree with real contents', () => { + expect(packedFiles()).toEqual(packedFiles(true)); + }, 120_000); it('marks the retained VS Code source unsupported', () => { const readme = readFileSync(join(workspaceRoot, 'apps/vscode/README.md'), 'utf8');