Fix #298: derive the pack list from git, not from a live-tree walk - #300
Conversation
`packedFiles()` ran `npm pack --dry-run --json` with `cwd: workspaceRoot`, which walks the entire live working tree. Other suites in the same vitest run create and delete scratch directories under that root — pir-832-migration.test.ts does it once per test case — and when the walk lstat'd a path that had just been removed, npm exited non-zero and the test failed. Five consecutive full-suite runs, five failures, because the walk took ~62s with `dist/` present. The list now comes from `git ls-files`, materialised as a placeholder skeleton under `mkdtemp` that nothing else can touch, with real content only for the files npm-packlist reads rather than stats: `package.json`, `.npmignore`, `.gitignore`. The packaging rules stay npm's; nothing is reimplemented here. The skeleton's pack list is identical to the real tree's minus untracked working-tree litter — 3575 entries against 3575, zero divergence — and a second test now asserts that rather than leaving it as a one-off measurement. Regression test: a scratch file of exactly the shape other suites churn must never reach the list. Reverting the fix fails it, and fails the pack test with the issue's ENOENT under concurrent churn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex (REQUEST_CHANGES): - Remove codev/reviews/bugfix-298-*.md. The BUGFIX protocol is explicit — "No spec, no plan, no artifact files: the issue is the spec." Its substance moves into the PR body. - The PR body now carries the Summary / Root Cause / Fix / Test Plan structure the protocol specifies. Claude (APPROVE, minor): - `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. - Restore the timeout rationale the #215/#216 comment carried, rewritten for the new cost model: the budget grows with the repo rather than with what happens to be on disk. Also correct the root cause in the thread and PR body: pir-832-migration.test.ts is one of 19 distinct .test-* scratch directories churning under packages/codev, not the only one. That is why the failure was reliable rather than occasional. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Integration review, one reviewer (claude). Verdict APPROVE, nothing blocking. Full output in Two small things worth taking before merge
The 30s timeout lost its rationale. The old 60s carried a paragraph tying the budget to a measurement, and this deletes it without replacing it. This file's own convention is strong on that: Confirmed rather than assumedThe fidelity test is the right shape. It does not require The revert verification is real: regression test red on revert, the exact ENOENT reproduced under churn, 7/7 green with the fix. This is now the only test deriving the root pack list.
Not this PR
#297, the Merge on green. Four projects are waiting behind this. |
Two pre-merge asks from the architect. The fixture holds only tracked files, so it cannot answer anything about build output. Measured rather than asserted: a live walk of a built tree returns 4,702 entries against the fixture's 3,575, 950 of the difference being build output — `files` outranks `.gitignore` in npm-packlist, so the gitignored `dist/`, `dashboard-dist/`, `v2-dist/` and `client-dist/` ship in a live walk and never in the fixture. The comment records that this is deliberate, that all four assertions turn on tracked paths, and that the built-tarball question is asked by bugfix-214-publish-scrub.test.ts instead. The 30s budget now carries its measurement: 2.0s, split 0.3s to materialise 3,846 paths and 1.7s for npm's startup and walk. The old 60s existed because its cost tracked whatever was on disk; this one tracks the tracked-file count. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chore(porch): bugfix-298 state commits stranded after #300 merged
Summary
The extension-retirement pack test derived what the root package ships by packing the live
working tree, which other suites churn scratch directories under.
npm packstat'd a path thathad just been removed and the test failed — five consecutive full-suite runs, five failures,
blocking
porch approvefor every project in the workspace. The list now comes from therepository (
git ls-files), packed in an isolated fixture.Fixes #298
Root Cause
packedFiles()rannpm pack --dry-run --json --ignore-scriptswithcwd: workspaceRoot. Theroot
package.jsondeclaresfiles: ["*", "!apps/streamdeck", "!apps/vscode"], so npm-packlistwalked the entire tree.
Other suites in the same vitest run create and delete scratch directories under that tree —
19 distinct
.test-*directories, every one resolved fromprocess.cwd()(=packages/codev)and created and removed per test or per test case.
packages/codev/src/agent-farm/__tests__/pir-832-migration.test.ts:18is the one that produced the reported error, but it is one of 19, which is why the failure is
reliable rather than occasional. When the walk lstat'd a path that had vanished between readdir
and lstat, npm exited non-zero,
execFileSyncthrew:Deterministic rather than flaky: the walk took ~62s with
dist/present, and those directorieschurned many times inside that window.
What the live walk was actually answering
Comparing the live pack list against
git ls-files:.builder-*, files under.claude/hooks/, andeach package's
node_modules/.bin/shims. Working-tree litter, and exactly the class of thingthat disappears mid-walk.
apps/web/.npmignore(node_modules,src,*.config.*,tsconfig*) and npm'salways-excluded names (
.gitignore,.npmignore,pnpm-lock.yaml).The first group is the bug. The second is why the packaging rules were not reimplemented.
Fix
packedFiles()now:git ls-files -zat the workspace root.mkdtempin the OS temp directory, where no other test can reach them.Real content only for
package.json,.npmignoreand.gitignore— the only filesnpm-packlist reads rather than stats — and an empty placeholder for every other path.
npm pack --dry-run --json --ignore-scriptsin that fixture, then removes it.The file set comes from git; the packaging rules stay npm's. Neither is reimplemented in the test,
so there is no second copy of the rules to drift.
Why the placeholder skeleton is sound
Its pack list is identical to the real tree's, minus the untracked litter: 3575 entries against
3575, zero divergence in either direction. All four original assertions hold on it.
Those numbers were measured on an unbuilt tree. The fixture holds only tracked files, so it
cannot answer anything about build output —
dist/,dashboard-dist/,v2-dist/andclient-dist/are gitignored, and afilesarray outranks.gitignorein npm-packlist, so alive walk of a built tree carries them: 4,702 entries against the fixture's 3,575, 950 of the
difference being build output. That is deliberate, and it is stated in the test's doc comment.
All four assertions turn on tracked paths — the
apps/webandapps/v2manifests are tracked,and 216 tracked
apps/vscode/files would appear the moment the!apps/vscodenegation stoppedexcluding them. A question about the built tarball needs a built tree, which is what
bugfix-214-publish-scrub.test.tsasks, per package.The skeleton-versus-real-content identity is now an assertion rather than a one-off measurement. A
second test packs the same git-derived set with its real contents and requires an identical list,
so a future ignore mechanism that reads a file the skeleton blanks turns the suite red instead of
silently changing the answer.
It deliberately does not compare against the live tree, and the test comment says so: packing the
live tree is the walk that failed 5 of 5, so asserting against it would reintroduce #298 inside
the test written to close it — and the live-versus-git difference is the 43 litter files, i.e. the
bug rather than drift to guard.
Test Plan
derives the pack list from the repository, not from whatever is on diskcreatespackages/codev/.test-bugfix-298/state.db, of exactly the shape the 19churners create, and asserts it never reaches the list. Nothing in the repo gitignores
.test-*and the rootfilesis["*", …], so the old walk packed it.packs the placeholder skeleton identically to the tracked tree with real contents.porch check bugfix-298: build ✓ 15.2s.porch check bugfix-298: tests ✓ 213.9s.Revert verification
Reverting
packedFiles()to the live-tree walk:expected [ '.af-cron/ci-health.yaml', …(3620) ] to not include 'packages/codev/.test-bugfix-298/state.db'.iterations each), the pack test fails with the issue's exact error:
npm error code ENOENT / npm error syscall lstat / lstat '.../packages/codev/.test-churn-1-4'.Bare
npm packunder the same churn: 6 of 6 runs ENOENT.Cost
The file goes from ~62s for one test to 10.4s for all seven.
Adjacent, not fixed
existsSync(join(workspaceRoot, 'apps/streamdeck'))assertion in thesame file reads a live working tree for the same reason.
bugfix-214-publish-scrub.test.tsandv2-packaging.e2e.test.tsalso shell out tonpm pack --dry-run, but per package and against explicitfilesallowlists (["dist"],["src","dist"], andpackages/codev'sdist/bin/skeleton/templates/*-dist) ratherthan
["*"]. No.test-*directory sits inside any listed entry, so npm never walks one.Noted rather than changed.
CMAP
gitfailure, restored timeout rationale). The third — a residual ENOENT window in the full-content copy — was withdrawn by the reviewer after verifying no suite mutates a tracked file.xai/grok-4.6)gpt-5.6-sol)codev/reviews/artifact is removed (BUGFIX ships no artifact files; the issue and this PR body are the record), and this body now carries the Summary / Root Cause / Fix / Test Plan structure the protocol specifies.Gemini was not run: this workspace's
.codev/config.jsonsetsporch.consultation.modelsto["claude", "opencode"], and Codex was added as the third lane.