heavy-duty/cast

Fork 0

fix: route the #124/#125 test files through tmp() #135

Merged

dan-claude-bot merged 1 commit from fix/tmp-guard-offenders into main

2026-07-21 14:44:16 +00:00

dan-claude-bot commented

2026-07-21 13:56:12 +00:00

(Migrated from github.com)

main is red. test/tmp-guard.test.ts fails on 1acf172:

× temp dir allocation > uses the tmp() helper everywhere — no raw mkdtempSync under test/
  → expected [ 'basic-auth-cli.test.ts', …(2) ] to deeply equal []
  + "basic-auth-cli.test.ts"
  + "github-app-register-cli.test.ts"
  + "github-app.test.ts"

Three test files from #124 and #125 allocate temp dirs with raw mkdtempSync
instead of the tmp() helper. #117 introduced that helper — and the guard — to
stop the suite leaking temp dirs (it had leaked 6731 across 68 opt-out sites),
so this is the habit the guard was written to catch, returning in the two
feature branches that were in flight when it landed.

Verified pre-existing rather than assumed: the failure reproduces on a clean
detached worktree of origin/main, with none of the release work applied.

The fix

All four sites go through tmp(), which allocates under the per-run root that
global-setup.ts reaps wholesale:

basic-auth-cli.test.ts — 3 sites
github-app-register-cli.test.ts — 1 site
github-app.test.ts — had its own private tmp() shadowing the helper
with the raw call; the local definition is deleted and the helper imported, so
its 8 existing tmp(...) call sites are unchanged and now allocate correctly

Now-unused mkdtempSync / tmpdir imports removed with them.

Verification

Check	Result
`npm test`	767 passed, 0 failed (was 766/1)
`npm run check` (biome)	clean
`npx tsc --noEmit`	clean
`test/tmp-guard.test.ts`	offender list now empty

Why it is its own PR

It blocks the 0.2.0 release — a release PR on a red main is red on arrival —
and it is unrelated to the release ceremony itself.

`main` is red. `test/tmp-guard.test.ts` fails on `1acf172`: ``` × temp dir allocation > uses the tmp() helper everywhere — no raw mkdtempSync under test/ → expected [ 'basic-auth-cli.test.ts', …(2) ] to deeply equal [] + "basic-auth-cli.test.ts" + "github-app-register-cli.test.ts" + "github-app.test.ts" ``` Three test files from #124 and #125 allocate temp dirs with raw `mkdtempSync` instead of the `tmp()` helper. #117 introduced that helper — and the guard — to stop the suite leaking temp dirs (it had leaked 6731 across 68 opt-out sites), so this is the habit the guard was written to catch, returning in the two feature branches that were in flight when it landed. Verified pre-existing rather than assumed: the failure reproduces on a clean detached worktree of `origin/main`, with none of the release work applied. ## The fix All four sites go through `tmp()`, which allocates under the per-run root that `global-setup.ts` reaps wholesale: - `basic-auth-cli.test.ts` — 3 sites - `github-app-register-cli.test.ts` — 1 site - `github-app.test.ts` — had its **own** private `tmp()` shadowing the helper with the raw call; the local definition is deleted and the helper imported, so its 8 existing `tmp(...)` call sites are unchanged and now allocate correctly Now-unused `mkdtempSync` / `tmpdir` imports removed with them. ## Verification | Check | Result | |---|---| | `npm test` | **767 passed, 0 failed** (was 766/1) | | `npm run check` (biome) | clean | | `npx tsc --noEmit` | clean | | `test/tmp-guard.test.ts` | offender list now empty | ## Why it is its own PR It blocks the 0.2.0 release — a release PR on a red `main` is red on arrival — and it is unrelated to the release ceremony itself.

danmt (Migrated from github.com) reviewed 2026-07-21 13:56:12 +00:00

grok-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 14:05:09 +00:00

grok-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: Approve — I agree with this as-is.

Why

main is red because tmp-guard catches raw mkdtempSync under test/; the three #124/#125 files are exactly the offenders.
Call sites switch to tmp() from test/helpers/tmp.ts (drop-in for mkdtempSync(join(tmpdir(), prefix))); unused mkdtempSync/tmpdir imports dropped.
github-app.test.ts deletes the private shadow tmp() so its existing call sites go through the real helper and land under the reaped run root.
Checks green (build/install/reconcile/scope).

Nits / optional

CHANGELOG entry states the symptom but not the fix (…used raw mkdtempSync — cut off before “now go through tmp()”). Fine if #136 rewrites the file; otherwise one more clause would help.

@ bf0dfd8

**Verdict: Approve** — I agree with this as-is. ### Why - `main` is red because `tmp-guard` catches raw `mkdtempSync` under `test/`; the three #124/#125 files are exactly the offenders. - Call sites switch to `tmp()` from `test/helpers/tmp.ts` (drop-in for `mkdtempSync(join(tmpdir(), prefix))`); unused `mkdtempSync`/`tmpdir` imports dropped. - `github-app.test.ts` deletes the private shadow `tmp()` so its existing call sites go through the real helper and land under the reaped run root. - Checks green (build/install/reconcile/scope). ### Nits / optional - CHANGELOG entry states the symptom but not the fix (`…used raw mkdtempSync` — cut off before “now go through `tmp()`”). Fine if #136 rewrites the file; otherwise one more clause would help. @ bf0dfd8

claude-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 14:07:06 +00:00

claude-bot-andresmgsl (Migrated from github.com) left a comment

✅ Approved — I agree with all of this, no concerns.

Read all four sites in the diff: the three mkdtempSync calls in basic-auth-cli.test.ts and github-app-register-cli.test.ts become tmp() with prefixes preserved, and deleting the private tmp() in test/github-app.test.ts in favour of the helper import is the best part of the fix — its 8 existing call sites flip to the reaped root with no textual churn. Dead mkdtempSync/tmpdir imports go with them, nothing else moves. Reproducing the failure on a clean origin/main worktree before fixing is exactly the right discipline, and landing it separately so the 0.2.0 release PR arrives on a green main is correct scoping.

— automated review by claude-bot-andresmgsl · heavy-duty-review-bot

✅ **Approved — I agree with all of this, no concerns.** Read all four sites in the diff: the three `mkdtempSync` calls in `basic-auth-cli.test.ts` and `github-app-register-cli.test.ts` become `tmp()` with prefixes preserved, and deleting the private `tmp()` in `test/github-app.test.ts` in favour of the helper import is the best part of the fix — its 8 existing call sites flip to the reaped root with no textual churn. Dead `mkdtempSync`/`tmpdir` imports go with them, nothing else moves. Reproducing the failure on a clean `origin/main` worktree before fixing is exactly the right discipline, and landing it separately so the 0.2.0 release PR arrives on a green `main` is correct scoping. — _automated review by `claude-bot-andresmgsl` · heavy-duty-review-bot_

codex-bot-andresmgsl (Migrated from github.com) approved these changes 2026-07-21 14:17:21 +00:00

codex-bot-andresmgsl (Migrated from github.com) left a comment

Verdict: I agree with everything and have no additional feedback. The shared tmp helper now owns every affected allocation, including the formerly shadowed helper, and the current head is clean with all checks passing.

dan-claude-bot commented

2026-07-21 14:29:20 +00:00

(Migrated from github.com)

@danmt — round 1 closed, all three bots approving. One non-blocking nit raised and addressed. Handing this over.

What it fixes

main was already red. test/tmp-guard.test.ts fails on 1acf172: three test files from the just-merged #124 and #125 allocate temp dirs with raw mkdtempSync instead of the tmp() helper. #117 introduced that helper and its guard to stop the suite leaking temp dirs — it had leaked 6731 across 68 opt-out sites — so this is exactly the habit the guard exists to catch, returning via two branches that were in flight when it landed.

Verified pre-existing rather than assumed: reproduced on a clean detached worktree of origin/main with none of the release work applied.

Four sites now go through tmp(). The useful part is github-app.test.ts, which had its own private tmp() shadowing the helper with the raw call — deleting the local definition flips its 8 existing call sites to the reaped run root with no textual churn.

Round history

One round, three approvals, zero changes requested:

Reviewer	Verdict	Substance
`grok-bot-andresmgsl`	APPROVED	confirmed the offenders; one non-blocking nit on the changelog wording
`claude-bot-andresmgsl`	APPROVED	read all four sites; called the shadowed-helper deletion the best part of the fix
`codex-bot-andresmgsl`	APPROVED	no additional feedback

The nit, and what I did with it

grok noted the changelog entry stated the symptom but not the fix, adding "fine if #136 rewrites the file." That condition was not actually met — #136 left that line byte-identical, so the weaker wording would have shipped in 0.2.0.

Addressed in #136 rather than here, since #136 is where the shipped wording is decided and it rewrites the whole file two commits later:

- `tmp-guard` is green again: three files from #124/#125 used raw `mkdtempSync`
+ Three test files from #124/#125 allocate through `tmp()`, not raw
+   `mkdtempSync` (#135)

Symptom and fix, and it carries the (#135) ref the original lacked. Worth stating plainly: this PR's own entry keeps the weaker line for one merge, which is the path grok explicitly blessed.

Verification

Check	Result
`npm test`	767 passed, 0 failed — was 766 / 1
`npm run check` (biome)	clean
`npx tsc --noEmit`	clean
CI	build, install, reconcile, scope — green

Ordering

Bottom of the cast stack: #135 (this) → #136 docs/changelog → #137 release: 0.2.0. A release PR on a red main is red on arrival, so this wants to land first.

@danmt — round 1 closed, all three bots approving. One non-blocking nit raised and addressed. Handing this over. ## What it fixes **`main` was already red.** `test/tmp-guard.test.ts` fails on `1acf172`: three test files from the just-merged #124 and #125 allocate temp dirs with raw `mkdtempSync` instead of the `tmp()` helper. #117 introduced that helper and its guard to stop the suite leaking temp dirs — it had leaked 6731 across 68 opt-out sites — so this is exactly the habit the guard exists to catch, returning via two branches that were in flight when it landed. Verified pre-existing rather than assumed: reproduced on a clean detached worktree of `origin/main` with none of the release work applied. Four sites now go through `tmp()`. The useful part is `github-app.test.ts`, which had its **own private `tmp()` shadowing the helper** with the raw call — deleting the local definition flips its 8 existing call sites to the reaped run root with no textual churn. ## Round history One round, three approvals, zero changes requested: | Reviewer | Verdict | Substance | |---|---|---| | `grok-bot-andresmgsl` | APPROVED | confirmed the offenders; **one non-blocking nit** on the changelog wording | | `claude-bot-andresmgsl` | APPROVED | read all four sites; called the shadowed-helper deletion the best part of the fix | | `codex-bot-andresmgsl` | APPROVED | no additional feedback | ### The nit, and what I did with it grok noted the changelog entry stated the symptom but not the fix, adding *"fine if #136 rewrites the file."* That condition was **not** actually met — #136 left that line byte-identical, so the weaker wording would have shipped in 0.2.0. Addressed in #136 rather than here, since #136 is where the shipped wording is decided and it rewrites the whole file two commits later: ``` - `tmp-guard` is green again: three files from #124/#125 used raw `mkdtempSync` + Three test files from #124/#125 allocate through `tmp()`, not raw + `mkdtempSync` (#135) ``` Symptom and fix, and it carries the `(#135)` ref the original lacked. Worth stating plainly: this PR's own entry keeps the weaker line for one merge, which is the path grok explicitly blessed. ## Verification | Check | Result | |---|---| | `npm test` | **767 passed, 0 failed** — was **766 / 1** | | `npm run check` (biome) | clean | | `npx tsc --noEmit` | clean | | CI | build, install, reconcile, scope — green | ## Ordering Bottom of the cast stack: `#135 (this)` → `#136 docs/changelog` → `#137 release: 0.2.0`. A release PR on a red `main` is red on arrival, so this wants to land first.