Codex review: needs maintainer review before merge. Reviewed May 27, 2026, 5:51 AM ET / 09:51 UTC.

Summary
Adds a standalone scripts/check-test-temp-cleanup.mjs mkdtemp cleanup scanner, fixture-backed tests, and a lint:tmp:test-temp-cleanup package script.

PR surface: Tests +156, Config +1, Other +187. Total +344 across 3 files.

Reproducibility: not applicable. this is internal test-tooling cleanup rather than a bug report. Source inspection and exact-head CI/check-run evidence are the relevant validation path.

Review metrics: 1 noteworthy metric.

• Package manifest dependency delta: 0 dependency specs changed; 1 lint script added. The dependency-changed label is caused by package.json, but the diff adds a script entry without changing package resolution.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• none

Risk before merge

• One exact-head broad check was still in progress when inspected; merge should wait for required checks to settle.
• The scanner intentionally remains heuristic and currently reports existing findings, so maintainers still need to accept the signal/noise tradeoff before broadening the gate.

Maintainer options:

1. Decide the mitigation before merge
   Keep the guard as a standalone lint script first, accept the heuristic only if the current signal is useful, and wire it into broader checks later after the current findings and noise profile are reviewed.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
Human review is the right lane because the PR is maintainer-labeled and the remaining decision is heuristic rollout acceptance, not a concrete code repair.

Security
Cleared: The diff adds a local repository scan script, fixture tests, and one package script; it does not add dependencies, lockfile changes, secrets handling, or new supply-chain execution.

Adjusted the scanner to reduce the false-negative case the bot pointed out, without moving to an AST-heavy approach.

What changed:

• the guard no longer suppresses a file just because it contains any afterEach / afterAll / finally
• it now only suppresses when a file has both a cleanup call (rm/rmSync) and a lifecycle/finally cleanup scope
• files that have hooks but no cleanup call remain flagged, which is the important case for the current rollout

Why this shape:

• the goal here is lightweight signal, not proof
• false negatives are acceptable to a point, but the earlier version was too easy to bypass accidentally
• this keeps the rule simple and cheap while making the most obvious false-negative category harder to miss

Re-ran validation:

• OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test test/scripts/check-test-temp-cleanup.test.ts
• node scripts/check-test-temp-cleanup.mjs --json

Current scan after the adjustment:

• 79 findings total
• 70 error
• 9 warning

Addressed the new bot review points.

Changes:

• made the scanner sparse-safe by skipping tracked test files that are absent on disk (ENOENT), so sparse worktrees do not fail the scan outright
• replaced the brittle current-main fixture expectations with hermetic temporary git repos inside the test, so the test no longer depends on whichever repo files happen to be clean or dirty today
• added focused coverage for:
  • obvious leak
  • hooks without cleanup call still being reported
  • lifecycle + cleanup call being suppressed
  • sparse-missing tracked file being skipped

Re-ran:

• OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test test/scripts/check-test-temp-cleanup.test.ts
• node scripts/check-test-temp-cleanup.mjs --json

Current scan after this update is unchanged in shape:

• 79 findings total
• 70 error
• 9 warning

Handled the last review point.

What changed:

• kept main() as the CLI entry path
• extracted renderTestTempCleanupReport(...) so tests can drive JSON rendering against a controlled fixture root instead of the live repo state
• updated the JSON-output test to use a hermetic temporary git repo fixture, so it no longer depends on whether the current repository happens to have findings

Also retained the earlier sparse-checkout fix:

• tracked-but-missing files now skip on ENOENT

Re-ran:

• OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test test/scripts/check-test-temp-cleanup.test.ts
• node scripts/check-test-temp-cleanup.mjs --json

Current scan remains:

• 79 findings total
• 70 error
• 9 warning

Review follow-up: here is the full path from the initial guard to the current heuristic, including what the bot surfaced and why the implementation changed the way it did.

Goal

This guard exists for one practical reason:

• tests create temp dirs under /tmp
• cleanup is sometimes missing
• those leftovers can persist long enough to trigger security scanners and operational noise

So the guard is trying to answer a narrow question:

Does this test file create temp dirs with mkdtemp
and fail to show an obvious cleanup path?

Not a full correctness proof. Just a lightweight repo-local detector that is cheap enough to run regularly.

Review path

1. Initial version

The first pass was intentionally simple:

mkdtemp found
  + no rm(...) in file        => error
  + rm(...) but no lifecycle  => warning
  + rm(...) + lifecycle hook  => suppress

That gave us a usable first signal, but the bot correctly pointed out that the suppression rule was too broad.

2. False negative: file-level suppression was too coarse

The first real issue was:

any rm(...) in file
+
any afterEach/afterAll/finally in file
=> suppress the finding

That can hide a real leak when the cleanup is for some unrelated path.

ASCII shape of the bug:

file
├─ const tempDir = mkdtemp(...)
├─ afterEach(() => rm(cacheDir))
└─ no cleanup for tempDir

old heuristic: "looks cleaned up"
actual state:  tempDir still leaks

That was the main behavioral flaw in the original rule.

3. Sparse checkout failure

The bot also found that git ls-files can list tracked test files that are absent in a sparse worktree.

git ls-files -> path exists in index
fs.readFile  -> ENOENT in sparse worktree

The fix there was straightforward: skip tracked-but-missing files on ENOENT instead of aborting the whole scan.

4. JSON test was coupled to live repo state

The earlier JSON-output test called the CLI path against the current repository and asserted that findings existed.

That meant the test would eventually fail for the wrong reason:

repo gets cleaner
=> fewer findings
=> JSON test fails
=> output code is fine, test is wrong

So the test was changed to run against a hermetic temporary git fixture instead of the live repository.

5. Remaining false negative: cleanup must match the same mkdtemp binding

The last bot point was the important one and is what the latest commit addresses.

The scanner now looks for variable bindings created from mkdtemp(...) / mkdtempSync(...), and only treats cleanup as relevant when the cleanup call references that same binding.

before
------
mkdtemp + any rm + any lifecycle = maybe suppressed

after
-----
mkdtemp bound to tempDir
cleanup must reference tempDir
and file must still show lifecycle/finally scope

ASCII view:

before

mkdtemp(tempDir)
     |
     +---- any rm(...) anywhere ------------------+
     +---- any afterEach/afterAll/finally -------+--> suppress


after

mkdtemp(tempDir)
     |
     +---- rm(... tempDir ...)? ---- yes ----+
     |                                       |
     +---- lifecycle/finally scope? -- yes --+--> suppress
     |
     +---- otherwise ----------------------------> report

That keeps the implementation lightweight, but removes the most obvious source of false negatives.

Important decisions

Decision 1: stay heuristic, do not jump to AST parsing yet

We explicitly kept this as a regex/string-based guard instead of moving straight to AST/dataflow analysis.

Reason:

• this is a repo hygiene detector, not a production parser
• we wanted to improve signal quickly without turning the script into a complex static-analysis subsystem
• false negatives are somewhat acceptable here
• high noise would be worse than imperfect coverage

So the design target is:

cheap to run
easy to understand
good enough to catch the common leaks

Decision 2: bias against noisy suppression

The review path made it clear that broad suppression is more dangerous than broad reporting.

So the current heuristic prefers this tradeoff:

if cleanup is not clearly tied to the mkdtemp binding
=> keep the finding

That may still leave some warnings/errors that a smarter analyzer could dismiss, but it avoids silently hiding obvious leaks.

Decision 3: keep fixture-backed tests for scanner behavior

The scanner tests now validate behavior through tiny temporary git repos.

That gives us deterministic coverage for:

• leaking temp dirs
• lifecycle hooks without cleanup
• unrelated cleanup not suppressing another temp dir
• sparse-missing tracked files
• JSON output shape

Current boundary / limitation

This is still not full semantic analysis. The scanner can miss or over-report more complex patterns.

Examples of remaining boundaries:

• aliasing or re-exported helper cleanup paths
• cleanup hidden behind helper functions the regex cannot connect back to the binding
• destructuring or more complex dataflow around temp-dir ownership
• non-standard lifecycle shapes that do not resemble afterEach, afterAll, or finally

So the rule should be read as:

obvious cleanup shown inline or near-inline => likely suppressed
non-obvious cleanup                         => likely still reported

That is deliberate for this rollout.

Why this matters beyond the script

The underlying operational issue is not just test neatness.

Residual temp directories can:

• create noisy vuln or secret-scan findings because extracted packages remain under /tmp
• make local and CI environments harder to reason about
• hide real cleanup regressions until they accumulate

So this guard is part of keeping test residue from becoming a security and operations distraction.

Validation rerun

Re-ran after the binding-aware fix:

• OPENCLAW_VITEST_MAX_WORKERS=1 pnpm test test/scripts/check-test-temp-cleanup.test.ts
• pnpm exec oxfmt --check --threads=1 scripts/check-test-temp-cleanup.mjs test/scripts/check-test-temp-cleanup.test.ts
• node scripts/check-test-temp-cleanup.mjs --json

In short:

v1: detect mkdtemp broadly
v2: sparse-safe
v3: hermetic JSON test
v4: suppression tied to the same temp-dir binding

That is a materially better floor without over-engineering the rule.

Dependency Changes Detected

This PR changes dependency-related files. Maintainers should confirm these changes are intentional.

Changed files:

• package.json

Maintainer follow-up:

• Review whether the dependency changes are intentional.
• Inspect resolved package deltas when lockfile, shrinkwrap, or workspace dependency policy changes are present.
• Treat package-lock.json and npm-shrinkwrap.json diffs as security-review surfaces.
• Run pnpm deps:changes:report -- --base-ref origin/main --markdown /tmp/dependency-changes.md --json /tmp/dependency-changes.json locally for detailed release-style evidence.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/26503657021
• Updated: 2026-05-27T09:51:51.602Z

ClawSweeper PR egg

✨ Hatched: 🥚 common Mossy Proofling

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

• Merged PRs are hatchable.
• Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
• Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: sleeps inside passing CI.
Image traits: location release reef; accessory CI status badge; palette violet, aqua, and starlight; mood mischievous; pose sitting proudly on a smooth stone; shell paper lantern shell; lighting cool dashboard glow; background small review tokens.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Mossy Proofling in ClawSweeper.

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/​@​create-markdown/​preview@​2.0.3
	npm/​@​a2ui/​lit@​0.10.0
	npm/​@​clawdbot/​lobster@​2026.5.22
	npm/​@​copilotkit/​aimock@​1.27.1
	npm/​@​azure/​identity@​4.13.1
	npm/​@​anthropic-ai/​vertex-sdk@​0.16.1
	npm/​@​discordjs/​voice@​0.19.2
	npm/​@​aws-sdk/​client-bedrock@​3.1053.0

View full report

Closing this PR in favor of a lower-noise helper-first design.

The cleanup scanner proved too strong for a CI gate: it has to infer whether arbitrary temp-dir cleanup patterns are sufficient, which turns into heuristic data-flow analysis and can force contributors to rewrite existing reasonable cleanup code just to match the scanner's recognized shape.

We'll replace this with a repo-local approach:

• extend the existing test temp helper pattern instead of adding a cleanup-sufficiency scanner;
• document the recommended helper usage for new tests;
• add, at most, a low-noise added-line report for new bare temp-dir creation;
• avoid blocking CI on existing cleanup style or requiring mechanical rewrites of touched files.

Thanks for iterating on this. The problem is real; the enforcement shape needs to be more conservative.