fix: create temp dirs with mkdtemp, not a name built from Date.now()

[vanceingalls]

Nine js/insecure-temporary-file alerts, closed by fixing them rather than dismissing them.

The audit

All 29 open alerts for that rule, traced to how each path is actually built:

	Count	Verdict
Write lands inside a mkdtempSync directory	19	False positive — CodeQL's dataflow reaches tmpdir() without seeing the mkdtemp in between
fontCompression.ts	1	Mitigated — writes with flag: "wx" (exclusive create), and only takes the tmpdir branch under AWS_LAMBDA_FUNCTION_NAME, where /tmp is single-tenant
Predictable name directly under the shared temp dir	9	Real — fixed here

Why the nine are real

const dir = join(tmpdir(), `hf-patch-test-${Date.now()}`);
mkdirSync(dir, { recursive: true });

Two separate problems. The name is guessable to the millisecond, and there is a window between choosing it and creating it — so on a shared machine another user can pre-create the path or drop a symlink there first.

mkdtempSync closes both: it picks the random suffix and creates the directory 0700 in a single syscall.

const dir = mkdtempSync(join(tmpdir(), "hf-patch-test-"));

Same shape, one line shorter, and the alert disappears instead of being suppressed.

What changed

Alert-closing sites:

• normalize.test.ts — six sites; its now-unused mkdirSync import goes with them. The wav helper also pushes the directory into dirs, not the file path, so afterEach cleans it up — measured, the previous shape leaked four directories per suite run.
• generate-catalog-previews.ts — one construction, which accounted for three alerts on its own since the other two were writes into the directory it made.

Three further sites of the same shape that carried no alert — CodeQL's dataflow reaches the template-preview writes through a readdir walk and never connects them back to the tmpdir() root, so the alert list was narrower than the pattern:

• generate-template-previews.ts — the near-twin of the file above; generate-catalog-previews.ts:80 points straight at it.
• benchmark.ts and transparency-test.ts — producer dev entry points.

All three use the path only through their variable, so the random suffix is inert. Converting them is why the title can claim the class: closing only the alerts would have turned the rule green while the shape survived where nothing would re-flag it.

The shared helper already existed

scripts/catalog-preview-temp.ts exports exactly this primitive, so generate-catalog-previews.ts now calls createCatalogPreviewTempDir rather than repeating its body, and the rationale for mkdtemp lives on the helper as the only copy. That test was in no runner — not in test:scripts, and no vitest config globs scripts/*.test.ts — so it pinned uniqueness and mode 0700 on a function nothing called; adding it to test:scripts alongside a real caller makes it load-bearing for the first time.

Because that import puts the helper on the renderer's runtime path, it is also added to the Catalog Previews paths: trigger and renderer canary, per the rule the workflow already states for registry-target-paths.mjs.

Deliberately not touching the other 20

Excluding the rule repo-wide was the obvious shortcut and would have hidden this class of bug from future code. The false positives are better handled per-site.

Verification

• normalize.test.ts — 37 tests pass; bun run test:scripts — 177 pass, including the newly-wired helper test
• Leak measured directly by counting directories in os.tmpdir() across a run: previous shape +4 per run, current 0
• tsc --noEmit clean in producer and cli; oxlint and oxfmt clean
• Repo-wide sweep: no remaining site that builds a predictable name under tmpdir() and then creates it. What's left is deliberately stable (config.ts:659, fontCompression.ts:34, regression-harness.ts:975), unguessable (transcribe.ts:299 uses randomUUID), or a negative-test path nothing creates

🤖 Generated with Claude Code

[miguel-heygen]

Exact-head review at d82fa5f429c1baac55ad1a2cce16305aef77b531

The security mechanics are correct: every changed mkdtempSync(join(tmpdir(), prefix)) call atomically allocates a unique owner-only directory and removes the predictable-name/symlink race. Catalog-preview cleanup still runs in finally, Windows checks are green, and CodeQL is clear.

One introduced cleanup regression to fix:

• packages/cli/src/whisper/normalize.test.ts:570-572 creates <tmp>/hf-wav-test-XXXXXX/tone.wav, but pushes the file path into dirs. afterEach at :26-28 therefore deletes only tone.wav and leaves the newly created parent directory behind. Four tests call this helper, so each suite run leaks four empty temp directories. Keep const dir = mkdtempSync(...), push dir, then return join(dir, "tone.wav").

Non-blocking SSOT note: scripts/catalog-preview-temp.ts already exports this exact createCatalogPreviewTempDir() primitive and catalog-preview-temp.test.ts pins uniqueness + mode 0700. Importing that helper in generate-catalog-previews.ts would connect the production caller to the existing regression test instead of duplicating it inline.

Local targeted collection was unavailable because this clean worktree had no workspace dependency link for @hyperframes/parsers/ff-binaries; exact-head CI is fully green, including Test, Windows, CodeQL, lint, build, and preview jobs.

[jrusso1020]

Reviewed at d82fa5f4. Additive to @miguel-heygen's review at the same head — I'm not re-litigating the temp-dir mechanics, the cleanup regression, or CI, all of which it covers. My contribution is a sweep for whether this class of bug is actually gone, plus verification of the description's numbers.

On the cleanup regression: I reached normalize.test.ts:570-572 independently, before that review posted, and landed on the same fix — dirs gets the file path while the newly created hf-wav-test-XXXXXX/ directory goes untracked, so afterEach's rmSync removes tone.wav and leaves the directory, four per run. Recording the convergence rather than restating it: two reviewers arriving there separately is worth more than the finding twice. One thing to add on severity, since it argues for fixing it in this PR rather than after — sweeping a shared machine that runs these suites, I counted 3,015 leaked /tmp/hf-* directories, some weeks old. This shape is what produces that, so it isn't a theoretical tidiness point.

Strengths

• Every quantitative claim in the description verifies. I pulled the alerts rather than taking the table's word: 29 open for js/insecure-temporary-file, and the 9 called "real" are exactly normalize.test.ts × 6 and generate-catalog-previews.ts × 3. The remaining 20 partition 19 + 1 as claimed. The "one construction accounted for three alerts" detail holds too — the other two were writes into the directory it made. An audit table that survives being checked row by row is rare enough to say so.
• Fixing over dismissing, with the tradeoff written down. A repo-wide exclusion would have hidden this shape from everything written later. That's called correctly and explicitly, and it's the right instinct — which is why the sweep below matters.
• No assertions were lost in the rewrite, which -12/+7 could easily have hidden. It reconciles exactly: five sites collapse 2 lines → 1 (-10/+5), the wav site is 1 → 1, the import is 1 → 1. Every removed line is a join(tmpdir(), …), a mkdirSync, or the import. Import hygiene is right in both directions — mkdirSync is fully gone from normalize.test.ts, and correctly kept in generate-catalog-previews.ts, where it stays live at :151, :308, :340.

important — the class survives, in the near-twin of the file you fixed

The title is about the pattern, so I swept packages/ and scripts/ rather than trusting the diff's footprint. 476 tmpdir() references; after classifying all of them, three sites still build a predictable name and then create it:

// scripts/generate-template-previews.ts:129-130
const tmpDir = join(tmpdir(), `hf-preview-${templateId}-${Date.now()}`);
mkdirSync(tmpDir, { recursive: true });

// packages/producer/src/benchmark.ts:207-208
const tmpRoot = join(tmpdir(), `benchmark-${fixture.id}-${Date.now()}`);
mkdirSync(tmpRoot, { recursive: true });

// packages/producer/src/transparency-test.ts:312-313
const workRoot = join(tmpdir(), `hf-transparency-${process.pid}-${Date.now()}`);
mkdirSync(workRoot, { recursive: true });

(process.pid in the third adds entropy but stays enumerable, and the pre-create window is identical.)

The first is the one that matters. Put it beside the sample your own description uses to define the bug:

// the description's example of what's wrong
const dir = join(tmpdir(), `hf-patch-test-${Date.now()}`);
mkdirSync(dir, { recursive: true });

Same shape, same directory, same prepare*Dir role, followed by the same cpSync and then patchTemplateHtml writing into it (:65 reads, :73 writes). And generate-catalog-previews.ts:80 — a line inside the file this PR does fix — points straight at it: "examples use the existing generate-template-previews.ts."

The part I'd most want on the record: none of the three appears in the 29 alerts. CodeQL's dataflow reaches patchTemplateHtml's write through a readdir walk (join(entry.parentPath, entry.name)) and doesn't connect it back to the tmpdir()-derived root. So the audit was scoped to the alert list, the alert list is narrower than the pattern, and closing these nine turns the rule green while the class stays exactly where nothing will re-flag it. That cuts against the "don't hide this from future code" reasoning the description is otherwise right about.

Calibration, since it cuts against me: all three survivors are dev and build tooling — one preview generator, two producer dev entry points — the same category as what this PR fixed (a test file and a build script), not the render path. Nothing here regresses and nothing is worse than before. It's that the title claims the class while the diff reaches two of five sites. Follow-up, not a change request on this diff.

The inlined expression is byte-identical to an existing tested helper

Building on @miguel-heygen's SSOT note rather than repeating it — the duplication is exact, which makes it worse than stylistic:

// scripts/catalog-preview-temp.ts:7  (pre-existing)
return mkdtempSync(join(tmpdir(), `hf-catalog-${itemName}-`));

// scripts/generate-catalog-previews.ts:177  (this PR)
const tmpDir = mkdtempSync(join(tmpdir(), `hf-catalog-${item.name}-`));

Same call, same hf-catalog- prefix, same argument role. The addition I'd make: createCatalogPreviewTempDir's only importer today is its own test (catalog-preview-temp.test.ts:5), so that test pins uniqueness and mode 0700 on a function with zero production callers — it protects nothing that runs. Adopting the helper doesn't just deduplicate, it makes an existing test load-bearing for the first time. Left as-is, the prefix string lives in two places and whichever copy drifts is the one the test isn't watching. Worth reconciling with the description's "## No shared helper" section, which reads as though one would have to be created.

nits

• packages/cli/src/whisper/transcribe.ts:299 already solves this a third way — hyperframes-audio-${process.pid}-${randomUUID()}.wav. Unguessable, so out of the alert class, but the tree now carries three temp-naming conventions and only two are safe. If the two producer files get converted, worth landing them as mkdtemp rather than a third randomUUID variant.
• packages/engine/src/config.ts:659 builds a uid-keyed hyperframes-extract-cache- directory under tmpdir(). That one is deliberately stable and shared across runs, so mkdtemp is inapplicable by construction. Flagging only so whoever runs this sweep next doesn't "fix" it into a per-run directory and silently disable the cache.
• Three sweep hits are not hits, listed so the sweep reads as classified rather than filtered: frameDirCache.test.ts:101 and project.test.ts:22 are deliberately-nonexistent paths nothing ever creates, and probe-beginframe.ts:204 is prose — a comment explaining this very choice, with mkdtempSync on the line below it.

CI

All 8 required contexts on main are success at d82fa5f4, latest-run-per-name: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest. Gates come from rulesets here — classic protection 404s, and the empty legacy-status list is an artifact, not a pending gate. Ten contexts are skipped, none required. CLI smoke (required) is green but is not in the required set despite the name, so it shouldn't be read as a gate either way.

Verdict

Verdict: COMMENT
Reasoning: The diff is correct and minimal, mkdtempSync is the right primitive, and the nine alerts it closes are verifiably the nine real ones. I'm not stamping only because there's a live CHANGES_REQUESTED on this head for a defect I independently confirmed is real — that's the reviewer gate, not CI, which is fully green. Land the dirs.push(dir) fix and this is an approve from me. The unflagged twin at generate-template-previews.ts:129 is worth a follow-up while it's fresh: same bug, same directory, and CodeQL will never tell you about it.

— Rames Jusso

[vanceingalls]

Addressed at bdf9445b8.

The cleanup regression — fixed, and measured both ways

@miguel-heygen and @jrusso1020 both landed on normalize.test.ts:570-572 independently, and both were right:

const dir = mkdtempSync(join(tmpdir(), "hf-wav-test-"));
dirs.push(dir);
const path = join(dir, "tone.wav");
writeFileSync(path, buf);
return path;

Rather than assume the fix works, I counted the directories in os.tmpdir() across a suite run, on the old code and the new:

original code:  before=8  after=12  delta=+4
fixed code:     before=8  after=8   delta=0

So the "four per run" figure is exactly right, and it's now zero. (The before=8 is itself two leaked runs' worth that were already sitting there — corroborating @jrusso1020's 3,015-directory sweep. I removed the ones my own runs created.)

I also checked the five sibling call sites in the same file rather than just the one reported: all six mkdtempSync sites now push the directory, none push a file path.

SSOT — adopted, and the test was in worse shape than reported

prepareProjectDir now calls the existing helper:

const tmpDir = createCatalogPreviewTempDir(item.name);

mkdtempSync and tmpdir were used at exactly that one line in the file, so both imports are gone with it. The security rationale moved onto createCatalogPreviewTempDir, which is now the only place the primitive lives.

One correction to the premise, in the direction that makes the point stronger. @jrusso1020 noted the helper's only importer is its own test, so the test "protects nothing that runs." It's worse than that: scripts/catalog-preview-temp.test.ts is in no runner at all. It isn't in the explicit file list in test:scripts, and there is no root vitest config globbing scripts/*.test.ts — the only two references to the module anywhere in the tree were the test's own import and (now) the production caller. So it wasn't a test guarding an uncalled function; it was a test that never executed.

Adding it to test:scripts alongside the real caller is what actually makes it load-bearing:

bun run test:scripts  →  177 tests, 177 pass, 0 fail

The surviving three — converted here rather than deferred

@jrusso1020 scoped this as follow-up and I've gone against that; saying so explicitly so it's easy to push back on. The reasoning that moved me is the one in that review: closing the nine alerts turns the rule green while the class survives in the three places CodeQL structurally cannot reach, and the PR title claims the class. Deferring means the follow-up competes with a rule that now looks clean.

// scripts/generate-template-previews.ts
const tmpDir = mkdtempSync(join(tmpdir(), `hf-preview-${templateId}-`));

// packages/producer/src/benchmark.ts
const tmpRoot = mkdtempSync(join(tmpdir(), `benchmark-${fixture.id}-`));

// packages/producer/src/transparency-test.ts
const workRoot = mkdtempSync(join(tmpdir(), "hf-transparency-"));

Checked before converting: each path is consumed only through its variable — no re-derivation of the name, no external consumer — so the random suffix is inert. mkdirSync stays imported and live in all three (:182, :297, and three outDir calls respectively). Per the nit, these are mkdtemp rather than a third randomUUID convention. The process.pid in the transparency root is dropped since mkdtemp's suffix supersedes it.

A repo-wide sweep for join(tmpdir(), …) now shows no remaining site that builds a predictable name and creates it. Everything left is one of the categories already classified in review: the deliberately-stable shared caches (config.ts:659, fontCompression.ts:34, regression-harness.ts:975 — untouched, and config.ts deliberately so per the nit), the unguessable randomUUID path (transcribe.ts:299), and the negative-test paths nothing creates.

Verification

bun run test:scripts 177/177 · normalize.test.ts 37/37 · tsc --noEmit clean in producer and cli · oxfmt --check and oxlint clean. CI is running on this head.

[vanceingalls]

Fixed at aa3c8eb83, plus a PR-description rewrite. Taking the finding and two of the three description notes.

The change-detection gap — real, and self-inflicted

Confirmed and fixed. Routing the renderer through createCatalogPreviewTempDir made that module part of its runtime path, and the workflow already states the rule for the sibling case in its own comment — "the containment module the renderer imports. Without it a change to path-traversal defence alone never re-runs the job that exercises it." My SSOT change created a second such module and didn't register it, so the dedup quietly widened a blind spot the workflow was explicitly built to avoid.

Added to both places, and generalised the comment to cover both modules:

      # Modules the renderer imports. Without them a change to path-traversal
      # defence or temp-directory allocation alone never re-runs the job that
      # exercises it.
      - "scripts/registry-target-paths.mjs"
      - "scripts/catalog-preview-temp.ts"

RENDERER_CHANGED=$(git diff --name-only --diff-filter=ACMR "$BASE_SHA"...HEAD \
  -- scripts/generate-catalog-previews.ts scripts/registry-target-paths.mjs \
     scripts/catalog-preview-temp.ts)

Verified rather than assumed, both directions, against this branch's own range — the old argument list reports only generate-catalog-previews.ts while catalog-preview-temp.ts is genuinely modified in the diff, so a helper-only PR was invisible to the trigger and to the canary; the new list reports both. The YAML parses and the multi-line continuation executes as written.

Worth naming the general shape, since it will recur: extracting a shared module can silently remove a file from a path-based CI trigger. The trigger enumerates files, so moving code out of an enumerated file and into a new one drops it from the filter unless the filter moves too.

Description notes — two taken

Rewrote ## What changed. You're right that it listed two files while the diff touches five; the three unflagged conversions now have their own subsection explaining why they're in a PR whose alert table doesn't mention them (CodeQL's readdir-walk dataflow never connects those writes back to the tmpdir() root, so the alert list is narrower than the pattern). Also replaced ## No shared helper — that section argued a wrapper would need inventing, which was wrong: one already existed and is now used. Verification section now carries the measured leak numbers and the repo-wide sweep result.

The misclassified false-positive bucket — not taken here, but worth recording

The shared.ts:154/159/200 and planV2.ts:224 observations look right to me and I'm deliberately leaving them out of this PR: re-triaging entries in the "19 false positives" bucket means either changing those call sites or adjusting suppressions, which is a different change from the one under review and would need its own reasoning per site. The lgtm[]-is-dead detail is the useful part to have on the record — a suppression that modern CodeQL ignores reads as "handled" while doing nothing, which is strictly worse than no comment. Both belong in whatever drives that rule to zero.

Agreed, and useful to have measured

The 0700-vs-0755 analysis is the one I'd have most wanted checked and hadn't: same-user child processes only, no docker mount or other-UID reader in any touched script, and hostItemDirectory re-creating the destination means 0700 never propagates into published docs/public/catalog/items/. Same for confirming test:scripts is ubuntu-latest-only, so the POSIX mode assertion I newly wired in has no Windows exposure — that was a real risk of adding a previously-unrun test to CI, and I hadn't checked it.

[jrusso1020]

Re-review at aa3c8eb8. Two new commits since my pass at d82fa5f4: bdf9445b (cleanup fix + the sweep) and aa3c8eb8 (CI paths).

Everything I conditioned the approve on has landed, and I verified each at source rather than reading the commit subjects:

• The cleanup leak is fixed as specified — normalize.test.ts:570-573 now pushes the directory and writes tone.wav inside it. That closes @miguel-heygen's blocker and the thing I'd independently reached.
• All three predictable-name survivors from my sweep are converted — generate-template-previews.ts:130, benchmark.ts:208, transparency-test.ts:312, each now a single mkdtempSync. I'd explicitly filed that as a follow-up rather than a change request, so fixing it in-PR is scope I didn't ask for: the title claims the class, and now the diff's reach actually matches the claim.
• The SSOT point landed — generate-catalog-previews.ts imports createCatalogPreviewTempDir, and the rationale comment moved into the helper, which is the right place for it.

The half of that neither review caught, and it's the better half

I wrote last time that the helper's test "protects nothing that runs," because the helper had no production caller. That was understated. The test never executed at all.

On main, scripts/catalog-preview-temp.test.ts (883 bytes) imports describe/it from node:test, so only node --test can collect it. It is absent from test:scripts's explicit file list; there is no root vitest config (vitest.config.* / vitest.workspace.* all 404); and the vitest run scripts/catalog/ filter cannot match scripts/catalog-preview-temp.test.ts — hyphen, not slash. I also scanned all 14 workflows for an out-of-band node --test over scripts/: zero hits. So the file was dead in every lane, and its assertions on uniqueness and mode 0700 had never run.

bdf9445b registers it in test:scripts, which runs at ci.yml:302 inside the Test job — a required context — and Test is green at this head. So those two assertions execute for the first time here, against a helper that now also has a production caller. Adding the caller without registering the test would have left it half-done in a way that still reads as fixed; you found the other half.

Import hygiene, checked both directions again

This is where a five-site conversion regresses, and I praised it last time, so I re-ran it rather than assuming it held:

• mkdirSync retained and still live: benchmark.ts:297, transparency-test.ts:166,193,221, generate-template-previews.ts:182, generate-catalog-previews.ts:150,304,336.
• generate-catalog-previews.ts correctly drops mkdtempSync and tmpdir — both are now zero-use there.

Nothing unused, nothing missing.

The red regression at this head is a superseded run, not a failure

Flagging so nobody hunts a broken fixture. There are two waves of runs at aa3c8eb8. Wave 1 (31726141881) is run-level cancelled: its regression-shards reads cancelled and the aggregate regression consequently reads failure on its "Check results" step. Wave 2 (31726196015) is the live one — all nine shards success, aggregate success.

Latest-run-per-name, all 8 required contexts are green: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest. mergeable is MERGEABLE.

nit — the shared allocation now covers one twin, not both

generate-template-previews.ts:130 allocates inline while its sibling routes through the helper. Defensible exactly as written: the helper hardcodes hf-catalog-, so the template script would inherit the wrong prefix. But the rationale comment now lives in the helper, and the script that doesn't call it is the one that carried this bug last time. Parameterizing (createPreviewTempDir(prefix, name)) would put both twins behind the one tested allocation; leaving it means the SSOT comment sits in a file that one of the two callers never opens. Either answer is fine — worth choosing rather than defaulting.

What I verified vs. trusted

Verified: the three converted sites and the cleanup fix, read at this head; the helper's importer; that the helper test is node:test-flavoured, absent from main's runner list, unmatched by the vitest filter, and unreferenced by any of the 14 workflows; that test:scripts runs inside the required Test job; import usage counts per file; the two-wave run anatomy per shard and run id; required-context state at this head, latest-run-per-name.
Trusted: the temp-dir mechanics and symlink/race reasoning from @miguel-heygen's pass, which I did not re-derive; that the CodeQL alert set closes out as the description states, which I checked last time at d82fa5f4 and did not re-pull here.

Verdict: APPROVE
Reasoning: Both conditions I named at d82fa5f4 are met and verified at this head, the sweep I filed as a follow-up was fixed in-PR, and the test-registration half went beyond what either review asked for. Required CI is green once the superseded cancelled wave is set aside. One nit, non-gating. Note this approve does not unblock the merge: @miguel-heygen's CHANGES_REQUESTED (4922116923, submitted at d82fa5f4) is still recorded and this repo does not dismiss stale reviews on push, so reviewDecision stays CHANGES_REQUESTED until that review is updated. The defect it named is fixed, so what's left is a re-review, not another commit.

— Rames Jusso

[vanceingalls]

@miguel-heygen (Magi) — your CHANGES_REQUESTED review is at older head d82fa5f4. Fresh head is aa3c8eb8, and James Russo (Rames) has APPROVED at that head with explicit verification that everything conditioned on has landed (cleanup leak fixed at test:570-573, plus the sweep + CI paths). Please re-verify and convert if it holds.

— Via

[miguel-heygen]

Fresh-head re-review at aa3c8eb83175d9df15dc162598e0b573d77624b6.

The cleanup blocker is closed correctly at packages/cli/src/whisper/normalize.test.ts:567-576: the suite records the allocated directory and derives tone.wav from it, so afterEach removes the directory rather than only the file. The broader sweep also converts the three surviving predictable-name sites to mkdtempSync, and the catalog renderer now calls the tested shared allocator. Registering catalog-preview-temp.test.ts in test:scripts plus adding the helper to the preview workflow path/canary filters makes those assertions load-bearing.

All eight required contexts are green at this exact head, including both Windows lanes and regression. The older cancelled/failing superseded wave is not the current result.

Verdict: APPROVE
Reasoning: The introduced temp-directory leak is fixed, the same security pattern is closed at the remaining sites, and exact-head required CI is green.

— Magi