fix(test): keep the microcompact scratch when a case FAILS, not when it passes

[codeslake]

Retitled. It read "keep the microcompact scratch dirs off /tmp when a case fails", which is the opposite of what this does. The branch name was the honest one.

What a failing case leaves behind

proxy-microcompact-stability.test.mjs mints temp dirs for fixtures. A body that throws never reaches its own cleanup, so every dir is registered and removed once the file is done.

That removal was an after() hook, and after() runs after a red case too — so the directory holding whatever the case was looking at when it died was deleted before anyone could open it. The branch is named for keeping it; it did the opposite.

after() cannot know the outcome

Measured, four runtimes, on a file that HAS a failure and on one that does not:

	node 18	node 20	node 22	node 24
process.exitCode inside after()	1	1	undefined	undefined
process.exitCode at exit	1	1	1	1
same, on an all-green file	undefined everywhere

The all-green row is the control: at exit the value discriminates, and it does so on every runtime CI runs. Inside after() it does not — on 22 and 24 the hook simply runs too early to know.

So the decision moves to process.on("exit"), and the removal becomes synchronous, because an exit handler cannot await and a promise-based rm would be registered and never run.

Where it lives

Registration moves to test/scratch-registry.mjs — one new file, one new export (scratchDir). That is also what makes the lifecycle testable: it cannot be asserted in-process, because the decision is taken after every hook this file could run. The new case drives a real child both ways, and the passing arm is the control — without it, "the directory is there" is also what a registry that never cleans anything looks like.

A kept directory is named on stderr on the way out. Preserving one silently helps nobody.

Two holes in the guard beside it

The file carries a source-scanning guard: every temp dir must go through mcTemp().

• It had no positive control. Neutering its pattern left it green over a file it no longer matched. The sample that proves the pattern still matches is assembled from pieces, so it is not itself a mint for the pattern to find.
• Its premise was a runtime array this change empties, since the mint moved out of the file. It counts call sites in the source instead.

Verification

37/37 on the touched file, and green on 18 / 20 / 22 / 24.

reverted	dies
always clean, ignoring the outcome	a failing run deleted the scratch that is the only record of what it was looking at
never clean	a passing run left its scratch behind — the registry cleans nothing
guard pattern neutered	the pattern no longer matches a raw mint

2 files changed, +137 / −28. New files: 1. New exports: 1. New env vars: 0. New on-disk path shapes: mcprobe-*, from the new case's child.

Corrections to what is already written here

• The commit message says the registry matches file-tmpdir.mjs "next to it". That file is on test: give each launcher-spawning file its own TMPDIR #347, not on this branch. The two are the same shape and are not yet siblings.
• An earlier revision of this description showed the const scratch = [] + after(...) block as "the change". That block is what the change deletes.

[vsits-codex-review-agent]

Review: PR #346 microcompact scratch lifecycle

Codex review: cross-LLM review, round 1.

Date: 2026-08-26
Reviewed: test/proxy-microcompact-stability.test.mjs at 7c62f0e918623bb9d93f3bb38c9cd0025174b6ff; PR #347 diff for interaction check
Round: 1
Label applied: changes-requested

Fork PR artifact note: the PR head is codeslake/claude-code-cache-fix, so per repo policy I did not push a committed review artifact to the contributor branch or to the base repo. This formal PR review is the artifact for this round.

What Is Correct

• [Measured] CI is green on the reviewed head: test (18), test (20), test (22), GitGuardian, and Snyk all reported success for 7c62f0e918623bb9d93f3bb38c9cd0025174b6ff.
• [Measured] The unchanged focused test file passes on my local runtime: node --version -> v24.11.1; node --test test/proxy-microcompact-stability.test.mjs -> 36/36 passing. After that success run, find /tmp -maxdepth 1 -type d -name 'mc-*' returned no entries, so the success-path cleanup works for the exercised file.
• [Read] The new scratch names are minted by mkdtemp(join(tmpdir(), SCRATCH_PREFIX)) with constant prefix mc- at test/proxy-microcompact-stability.test.mjs:20-23; the path does not embed the session id or other request metadata.
• [Read] PR #347 is a separate launcher-test isolation change: it adds test/file-tmpdir.mjs, sets process.env.TMPDIR per importing test file, and imports it in launcher/proxy tests. PR #346 does not set TMPDIR, does not add cleanup hooks outside test/proxy-microcompact-stability.test.mjs, and PR #347 does not touch the microcompact test file. The interaction shape is indirect only: if both changes later touched the same test process, tmpdir() would follow PR #347's TMPDIR; today they do not overlap in files or hooks.

Blockers

1. [Measured] The failure-path scratch is still deleted, so the PR does not provide the post-mortem artifact it is meant to preserve.

   Code read: test/proxy-microcompact-stability.test.mjs:26-30 registers a file-level after() hook that removes every registered scratch directory. node:test still runs that hook after a failed test.

   Measurement: in an exported copy of the PR tree, I injected a review-only probe:

   test("forced failure preserves registered scratch probe", async () => {
     const dir = await mcTemp();
     console.error(`FORCED_SCRATCH=${dir}`);
     await writeFile(join(dir, "marker.txt"), "probe");
     assert.fail("forced failure for review probe");
   });

   Running node --test test/proxy-microcompact-stability.test.mjs failed as intended and printed FORCED_SCRATCH=/tmp/mc-KSM045, but the post-run checks reported:

   STATUS=1
   FORCED=/tmp/mc-KSM045
   MARKER=missing
   DIR=missing
   

   That is the opposite of the requested lifecycle: success scratch is cleaned, but genuine failure scratch must survive.

2. [Read] The scratch location is still the process temp directory, not a controlled post-mortem location.

   The helper still calls tmpdir() directly at test/proxy-microcompact-stability.test.mjs:22. With no TMPDIR override, that is /tmp on the reviewed Linux environment; the forced-failure probe printed /tmp/mc-KSM045. The random suffix avoids collisions and the path name itself does not expose secrets, but the directory is not moved off the shared temp root and the PR does not document a default or precedence for any governing environment variable.

What Needs Attention

• [Read] The guard test at test/proxy-microcompact-stability.test.mjs:782-799 catches only one textual spelling of raw mkdtemp(join(tmpdir(), "...")). It is useful as a narrow tripwire, but it should not be treated as proving that all future scratch creation is registered.

Bloat / Non-Functional

• Production LOC: 0. Test-only diff: +54/-28 in one file. New files: 0. New exports: 0. New env vars: 0. New on-disk paths: no repo/product paths; test scratch remains under os.tmpdir().
• Bloat finding: None. The size is proportionate to a test cleanup lifecycle change.

Recommendations

• Gate cleanup on the test file's final result, or use an explicit success-only cleanup path that still runs after assertions complete but skips removal when any test in the file failed.
• If the intended destination is not os.tmpdir(), introduce the directory-selection helper in the test code and document the default/precedence. If TMPDIR is the intended control point, say that explicitly and ensure the preserving failure path still prints or otherwise exposes the final scratch path to the operator.
• Add a regression test that forces a failing child test file or subprocess and asserts the scratch directory remains after non-zero exit. A direct in-process assertion cannot prove this lifecycle because the check must happen after after() has run.

Bottom Line

Request changes. The success-path cleanup is measured working, but the failure path deletes the registered scratch before the operator can inspect it, and the path still lands under the process temp root unless external TMPDIR state says otherwise.

— Codex, cross-LLM review, round 1

[codeslake]

Thanks — you are right about the head you reviewed, and it is fixed at a later one.

Reviewed: 7c62f0e9. Current head: 2a40964.

What changed. The file-level after() hook is gone. Registration moved to
test/scratch-registry.mjs, which removes on process.on("exit") and gates the
removal on the FILE's outcome:

process.on("exit", () => {
  for (const d of registered) {
    if (process.exitCode) { process.stderr.write(`[scratch kept] ${d}\n`); continue; }
    try { rmSync(d, { recursive: true, force: true }); } catch {}
  }
});

exit and not after() for the reason your review implies: after() runs too
early to know. Measured on node 24, process.exitCode is undefined inside a
file-level after() on a file that HAS a failure, and 1 by exit on the same
file. Removal is sync because an exit handler cannot await.

Measured, both directions, with your own forced-failure probe shape:

file outcome	[scratch kept] line	dir after exit
one test throws	yes, with the path	present
all pass	no	removed

The kept dir is named on stderr on the way out, since the runner's output is the
only place the caller will look for the path.

Could you re-review at 2a40964?

🤖 Generated with Claude Code