test: canonical removeTree helper and a lint gate on bare recursive rm - #823
Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ScriptedAlchemy
added
the
skip-changeset
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dc115ffc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const before = text[match.index - 1]; | ||
| if (before === '.' || before === '$') { | ||
| match = pattern.exec(text); |
There was a problem hiding this comment.
When a test uses the standard namespace form import * as fs from 'node:fs/promises' and calls fs.rm(path, { recursive: true }), this branch explicitly skips the call because rm is preceded by a dot. Consequently pnpm lint accepts exactly the retry-free recursive removal this gate is intended to prevent, allowing the teardown flake to regrow; inspect the resolved Node API call rather than discarding all member calls.
Useful? React with 👍 / 👎.
…eardown A late writer into a tree being removed makes rm reject with ENOTEMPTY. One helper retries the transient codes so each call site does not carry its own maxRetries.
…in lint
scripts/check-test-remove-tree.mjs fails pnpm lint on rm(..., { recursive: true }) without maxRetries under packages/*/tests. 1156 call sites across 204 files moved to removeTree.
After merging main, three install-surface teardowns still used bare recursive rm. Convert them to removeTree, and teach the lint gate to recognize namespace and aliased Node fs removal bindings so those forms cannot escape the gate.
Mask comments/strings before scanning, escape imported names literally, and read recursive/maxRetries from real options properties so GPT P2 cases pass.
Replace comment/string masking with an AST walk so options come only from the second argument (including quoted keys), and regex/template edge cases no longer hide or invent recursive rm hits.
Stop forging bare `rm` bindings from text regex so local identifiers and commented-out imports cannot bypass or falsely trip the removeTree lint gate.
Lock commented bare imports, non-fs rm imports, and aliased/namespace maxRetries pass paths so removalBindings stays ImportDeclaration-backed.
After rebasing onto main, the Codex add-only install tests still used bare recursive rm in finally blocks; the remove-tree lint gate correctly failed.
0699b4d to
cf340ce
Compare
1 participant
Why
Integration shards flaked in teardown with `ENOTEMPTY` when `rm(dir, { recursive: true })` raced a late write from a dev server or watcher. #802 patched one site and ten dev-server tests carried `maxRetries` by hand. Nothing stopped the next bare call. This adds one helper and a lint gate so the pattern cannot regrow.
Fixes #805.
Scope
Tradeoffs
The helper loops instead of passing `maxRetries` to `rm`. That lets the test inject an `rm` that rejects once, then succeeds, and assert on the directory being gone. Passing `maxRetries` through an injected `rm` would test nothing.
Blast Radius
Test files and the lint script. Runtime packages are untouched. Teardown of a tree that a late writer keeps busy now waits up to about 750 ms before surfacing the error.
Verification
In a worktree on `origin/main` at 57a9196: `pnpm build`, `pnpm typecheck`, `pnpm lint`, and `pnpm test:unit` passed. The unit pool ran 310 files, 4466 tests passed, 6 skipped. `remove-tree.test.ts` covers one `ENOTEMPTY` then success, and a persistent `ENOTEMPTY` surfacing as the original error with the directory still present.
Local gate on the merged head
82b16fc(branch containsorigin/mainat 57a9196):pnpm build,pnpm typecheck,pnpm lint(rslint +scripts/check-test-remove-tree.mjs): passpnpm test:unit: 0 failedpnpm test:integration:run(fresh dist): 1181 passed, 0 failed, 4 skippedCI on this head: Changeset, Docs site, and Package preview passed. The CI workflow was cancelled twice ~3 min in by an explicit cancel with no newer push (run 35179087170, attempts 1–2); every job that completed before the cancel passed. Merged on the local gate per AGENTS.md.