fix(benchmark): create the benchmarks directory before writing into it (#117)

Author: addyCooks

src/commands/benchmark.tsx

@@ -1,11 +1,4 @@
-import {
-	existsSync,
-	mkdirSync,
-	readFileSync,
-	renameSync,
-	rmSync,
-	writeFileSync,
-} from 'node:fs';
+import {existsSync, mkdirSync, readFileSync, renameSync, rmSync} from 'node:fs';
 import {dirname, join} from 'node:path';
 import {Spinner, StatusMessage} from '@inkjs/ui';
 import {Box, Text, useApp} from 'ink';
@@ -28,10 +21,11 @@ import {
 } from '../lib/benchmark-utils.js';
 import {
 	configExists,
+	ensureBenchmarksDir,
 	findLatestGGUF,
-	getBenchmarksDir,
 	loadConfig,
 	resolveContextMessage,
+	writeFileAtomic,
 } from '../lib/config.js';
 import {
 	callJudge,
@@ -299,7 +293,7 @@ export function BenchmarkCommand({options}: Props) {
 			} catch {
 				// Minimal config (e.g., external benchmark runner) — no context message needed
 			}
-			const benchmarksDir = getBenchmarksDir();
+			const benchmarksDir = ensureBenchmarksDir();
 
 			if (options.model && options.base) {
 				setError(
@@ -461,7 +455,7 @@ export function BenchmarkCommand({options}: Props) {
 						match: 'startsWith',
 					},
 				];
-				writeFileSync(datasetPath, JSON.stringify(tests, null, 2));
+				writeFileAtomic(datasetPath, JSON.stringify(tests, null, 2));
 				setError(
 					`No benchmark dataset found. Created sample at ${datasetPath}`,
 				);
@@ -841,13 +835,13 @@ export function BenchmarkCommand({options}: Props) {
 				.toISOString()
 				.replace(/[:.]/g, '-')}.json`;
 			const resultPath = join(benchmarksDir, resultFilename);
-			writeFileSync(resultPath, JSON.stringify(finalResult, null, 2));
+			writeFileAtomic(resultPath, JSON.stringify(finalResult, null, 2));
 
 			// Save human-readable markdown report
 			const reportFilename = resultFilename.replace('.json', '.md');
 			const reportPath = join(benchmarksDir, reportFilename);
 			const report = generateMarkdownReport(finalResult, contextMsg);
-			writeFileSync(reportPath, report);
+			writeFileAtomic(reportPath, report);
 
 			setResults(finalResult);
 			setStatus('done');

src/commands/benchmark/compare.tsx

@@ -1,10 +1,4 @@
-import {
-	existsSync,
-	mkdirSync,
-	realpathSync,
-	statSync,
-	writeFileSync,
-} from 'node:fs';
+import {realpathSync, statSync, writeFileSync} from 'node:fs';
 import {join} from 'node:path';
 import {StatusMessage} from '@inkjs/ui';
 import {Box, Text, useApp} from 'ink';
@@ -25,8 +19,8 @@ import {
 	generateComparisonMarkdown,
 } from '../../lib/benchmark-compare.js';
 import {
+	ensureBenchmarksDir,
 	findLatestBenchmark,
-	getBenchmarksDir,
 	listBenchmarks,
 	loadBenchmark,
 	resolveBenchmarkPath,
@@ -135,10 +129,7 @@ export function BenchmarkCompareCommand({fileA, fileB}: Props) {
 			const result = compareBenchmarks(before, after);
 
 			const timestamp = new Date().toISOString().replace(/[:.]/g, '-');
-			const benchmarksDir = getBenchmarksDir();
-			if (!existsSync(benchmarksDir)) {
-				mkdirSync(benchmarksDir, {recursive: true});
-			}
+			const benchmarksDir = ensureBenchmarksDir();
 			const jsonPath = join(benchmarksDir, `compare-${timestamp}.json`);
 			const markdownPath = join(benchmarksDir, `compare-${timestamp}.md`);
 			writeFileSync(jsonPath, JSON.stringify(result, null, 2));

src/lib/config.spec.ts

@@ -1,5 +1,8 @@
 import {
+  existsSync,
   mkdirSync,
+  readdirSync,
+  readFileSync,
   rmSync,
   utimesSync,
   writeFileSync,
@@ -9,13 +12,15 @@ import test from "ava";
 import { ConfigSchema, TrainingConfigSchema } from "../types/index.js";
 import {
   createDefaultConfig,
+  ensureBenchmarksDir,
   findLatestGGUF,
   formatConfigIssues,
   listBenchmarks,
   resolveBenchmarkPath,
   findUnknownConfigKeys,
   loadConfig,
   resolveContextMessage,
+  writeFileAtomic,
 } from "./config.js";
 
 const TEST_DIR = join(process.cwd(), ".test-nanotune");
@@ -675,3 +680,118 @@ test.serial("loadConfig prints each unknown key once across loads", (t) => {
     rmSync(WARN_TEST_DIR, { recursive: true, force: true });
   }
 });
+
+
+// ── ensureBenchmarksDir ───────────────────────────────────────────────
+//
+// setupBenchTest leaves a project with no benchmarks directory — exactly the
+// state `git clone` produces, since .nanotune/.gitignore lists benchmarks/ and
+// only `nanotune init` ever creates it.
+
+test.serial("ensureBenchmarksDir creates the directory when it is missing", (t) => {
+  setupBenchTest();
+  try {
+    t.false(existsSync(BENCH_DIR));
+    const dir = ensureBenchmarksDir();
+    t.is(dir, BENCH_DIR);
+    t.true(existsSync(dir));
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+test.serial("ensureBenchmarksDir leaves an existing directory and its contents alone", (t) => {
+  setupBenchTest();
+  try {
+    mkdirSync(BENCH_DIR, { recursive: true });
+    writeFileSync(join(BENCH_DIR, "tests.json"), "[]");
+
+    t.is(ensureBenchmarksDir(), BENCH_DIR);
+    t.is(readFileSync(join(BENCH_DIR, "tests.json"), "utf-8"), "[]");
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+test.serial("results saved into a cloned project are discoverable afterwards", (t) => {
+  setupBenchTest();
+  try {
+    // The tail of a real run: both reports land under a benchmarks directory
+    // the clone never had, and `benchmark compare` still finds them there.
+    const dir = ensureBenchmarksDir();
+    const resultPath = join(dir, "benchmark-2026-01-01T00-00-00-000Z.json");
+    writeFileAtomic(resultPath, JSON.stringify({ summary: { total: 2 } }));
+    writeFileAtomic(resultPath.replace(".json", ".md"), "# Benchmark Report");
+
+    t.deepEqual(
+      listBenchmarks().map((b) => b.filename),
+      ["benchmark-2026-01-01T00-00-00-000Z.json"],
+    );
+    t.is(resolveBenchmarkPath("2026-01-01T00-00-00-000Z"), resultPath);
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+// ── writeFileAtomic ───────────────────────────────────────────────────
+
+test.serial("writeFileAtomic writes the contents and leaves no temp file behind", (t) => {
+  setupBenchTest();
+  try {
+    const dir = ensureBenchmarksDir();
+    const path = join(dir, "report.md");
+    writeFileAtomic(path, "# Benchmark Report");
+
+    t.is(readFileSync(path, "utf-8"), "# Benchmark Report");
+    t.deepEqual(readdirSync(dir), ["report.md"]);
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+test.serial("writeFileAtomic replaces an existing file", (t) => {
+  setupBenchTest();
+  try {
+    const dir = ensureBenchmarksDir();
+    const path = join(dir, "tests.json");
+    writeFileAtomic(path, "[1]");
+    writeFileAtomic(path, "[1,2]");
+
+    t.is(readFileSync(path, "utf-8"), "[1,2]");
+    t.deepEqual(readdirSync(dir), ["tests.json"]);
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+test.serial("writeFileAtomic cleans up its temp file when the rename fails", (t) => {
+  setupBenchTest();
+  try {
+    const dir = ensureBenchmarksDir();
+    // Renaming onto a non-empty directory fails, so the write never lands.
+    // The point is that it leaves no half-written sibling behind either — a
+    // stray temp is how a partial write gets mistaken for a finished run.
+    const blocked = join(dir, "blocked");
+    mkdirSync(blocked, { recursive: true });
+    writeFileSync(join(blocked, "keep.txt"), "keep");
+
+    t.throws(() => writeFileAtomic(blocked, "should not land"));
+    t.deepEqual(readdirSync(dir), ["blocked"]);
+    t.is(readFileSync(join(blocked, "keep.txt"), "utf-8"), "keep");
+  } finally {
+    teardownBenchTest();
+  }
+});
+
+test.serial("writeFileAtomic surfaces a missing parent rather than inventing one", (t) => {
+  setupBenchTest();
+  try {
+    // `--dataset` can point anywhere, so a typo'd path must still fail loudly
+    // instead of quietly creating directories outside the project.
+    const missing = join(BENCH_TEST_DIR, "nope", "tests.json");
+    t.throws(() => writeFileAtomic(missing, "[]"), { code: "ENOENT" });
+    t.false(existsSync(join(BENCH_TEST_DIR, "nope")));
+  } finally {
+    teardownBenchTest();
+  }
+});

src/lib/config.ts

@@ -3,6 +3,8 @@ import {
 	mkdirSync,
 	readdirSync,
 	readFileSync,
+	renameSync,
+	rmSync,
 	statSync,
 	writeFileSync,
 } from 'node:fs';
@@ -42,6 +44,20 @@ export function getBenchmarksDir(): string {
 	return join(getProjectDir(), 'benchmarks');
 }
 
+/**
+ * Return the project's benchmarks directory, creating it if it is missing.
+ * `.nanotune/.gitignore` lists `benchmarks/`, so `initializeProjectDirs` during
+ * `nanotune init` is the only thing that ever creates it — a project obtained
+ * by cloning has no benchmarks directory at all, and every write under it dies
+ * with a bare ENOENT. Use this rather than `getBenchmarksDir` anywhere you are
+ * about to write.
+ */
+export function ensureBenchmarksDir(): string {
+	const dir = getBenchmarksDir();
+	mkdirSync(dir, {recursive: true});
+	return dir;
+}
+
 export function getChatsDir(): string {
 	return join(getProjectDir(), 'chats');
 }
@@ -191,6 +207,24 @@ export function saveConfig(config: Config): void {
 	writeFileSync(path, JSON.stringify(config, null, 2));
 }
 
+/**
+ * Write `contents` to `path` via a sibling temp file renamed into place.
+ * rename(2) is atomic, so an interrupted or failed write leaves either the
+ * previous file or the complete new one — never a truncated file that a later
+ * read mistakes for a whole one. The temp carries the pid so concurrent runs
+ * cannot scribble over each other's, and the `finally` clears it on the paths
+ * where the rename never happened.
+ */
+export function writeFileAtomic(path: string, contents: string): void {
+	const tmp = `${path}.tmp-${process.pid}`;
+	try {
+		writeFileSync(tmp, contents);
+		renameSync(tmp, path);
+	} finally {
+		rmSync(tmp, {force: true});
+	}
+}
+
 const GITIGNORE_CONTENTS = `# Nanotune project artifacts
 adapters/
 models/