diff --git a/CHANGELOG.md b/CHANGELOG.md index 8765244..bdb3517 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and ## [Unreleased] +### Changed + +- `.dunk/comments.json` is now deleted when the last comment is resolved instead of leaving an empty `{ "comments": [] }` file. Reads already treat a missing file as "no comments", so an empty review leaves no artifact for a coding agent or human reviewer to reason about. + ## [0.15.0] - 2026-05-19 ### Changed diff --git a/src/core/cliComments.test.ts b/src/core/cliComments.test.ts index 35058b5..aa0c5c7 100644 --- a/src/core/cliComments.test.ts +++ b/src/core/cliComments.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { computeAnchor, readCommentsFile, splitLines, writeCommentsFile } from "./comments"; @@ -329,6 +329,24 @@ describe("dunk comments CLI", () => { expect(remaining.comments.map((c) => c.id)).toEqual([2]); }); + test("resolving the last comment deletes the comments file", () => { + const repoRoot = createTempRepo(); + writeCommentsFile(repoRoot, { + schema: 1, + comments: [ + { id: 1, file: "a.ts", line: 1, range: [1, 1], anchor: "aaaaaaaaaaaaaaaa", body: "only" }, + ], + }); + + const output = runCommentsResolve([1], { cwd: repoRoot }); + expect(output).toBe("Resolved 1 comment: #1.\n"); + + // CLI-visible: the review reports clean. + expect(runCommentsList("text", { cwd: repoRoot })).toBe("No pending comments.\n"); + // And the literal contract of this change: no empty artifact left behind. + expect(existsSync(join(repoRoot, ".dunk", "comments.json"))).toBe(false); + }); + test("resolve refuses partial success when one id is missing", () => { const repoRoot = createTempRepo(); writeCommentsFile(repoRoot, { diff --git a/src/core/comments.test.ts b/src/core/comments.test.ts index f8a8f7f..0584136 100644 --- a/src/core/comments.test.ts +++ b/src/core/comments.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { computeAnchor, + mutateCommentsFile, nextCommentId, readCommentsFile, resolveComments, @@ -68,6 +69,84 @@ describe("dunk comments", () => { }); }); + test("writeCommentsFile deletes the file when the review has no comments", () => { + withTempRepo((repoRoot) => { + const path = join(repoRoot, ".dunk", "comments.json"); + writeCommentsFile(repoRoot, { + schema: 1, + comments: [ + { id: 1, file: "a.ts", line: 1, range: [1, 1], anchor: "aaaaaaaaaaaaaaaa", body: "x" }, + ], + }); + expect(existsSync(path)).toBe(true); + + writeCommentsFile(repoRoot, { schema: 1, comments: [] }); + + // No empty `{ "comments": [] }` artifact is left behind, and reads still + // round-trip to "no comments". + expect(existsSync(path)).toBe(false); + expect(readCommentsFile(repoRoot).comments).toEqual([]); + }); + }); + + test("writeCommentsFile with no comments is a no-op when the file is absent", () => { + withTempRepo((repoRoot) => { + expect(() => writeCommentsFile(repoRoot, { schema: 1, comments: [] })).not.toThrow(); + expect(existsSync(join(repoRoot, ".dunk", "comments.json"))).toBe(false); + }); + }); + + test("mutateCommentsFile deletes the file when the last comment is removed", () => { + withTempRepo((repoRoot) => { + const path = join(repoRoot, ".dunk", "comments.json"); + writeCommentsFile(repoRoot, { + schema: 1, + comments: [ + { id: 7, file: "a.ts", line: 1, range: [1, 1], anchor: "aaaaaaaaaaaaaaaa", body: "x" }, + ], + }); + + const next = mutateCommentsFile(repoRoot, (current) => withRemovedComment(current, 7)); + + expect(next.comments).toEqual([]); + expect(existsSync(path)).toBe(false); + }); + }); + + test("a concurrent deletion mid-mutation is treated as an empty review, not an error", () => { + withTempRepo((repoRoot) => { + const path = join(repoRoot, ".dunk", "comments.json"); + writeCommentsFile(repoRoot, { + schema: 1, + comments: [ + { id: 1, file: "a.ts", line: 1, range: [1, 1], anchor: "aaaaaaaaaaaaaaaa", body: "old" }, + ], + }); + + // The first attempt deletes the file after the read but before the + // fingerprint re-check, so currentFingerprint hits ENOENT and the + // optimistic loop retries against the now-absent (empty) file. + let deletedOnce = false; + const result = mutateCommentsFile(repoRoot, (current) => { + if (!deletedOnce) { + deletedOnce = true; + rmSync(path, { force: true }); + } + return withAddedComment(current, { + file: "b.ts", + line: 2, + range: [2, 2], + anchor: "bbbbbbbbbbbbbbbb", + body: "fresh", + }).file; + }); + + expect(deletedOnce).toBe(true); + expect(result.comments.map((c) => c.body)).toEqual(["fresh"]); + expect(readCommentsFile(repoRoot).comments.map((c) => c.body)).toEqual(["fresh"]); + }); + }); + test("nextCommentId never reuses an id, even after deletions", () => { let file: import("./comments").CommentsFile = { schema: 1, comments: [] }; file = withAddedComment(file, { diff --git a/src/core/comments.ts b/src/core/comments.ts index 03ec196..854ad8a 100644 --- a/src/core/comments.ts +++ b/src/core/comments.ts @@ -1,9 +1,9 @@ /** `.dunk/comments.json` reader/writer and drift detection. */ import { createHash } from "node:crypto"; -import { existsSync, mkdirSync, readFileSync, renameSync, statSync, writeFileSync } from "node:fs"; +import { mkdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; import { isAbsolute, join, relative, resolve as resolvePath } from "node:path"; import { z } from "zod"; -import { DUNK_COMMENTS_FILENAME, DUNK_DIR } from "./dunkPaths"; +import { DUNK_COMMENTS_FILENAME, DUNK_COMMENTS_RELATIVE_PATH, DUNK_DIR } from "./dunkPaths"; import { DunkUserError } from "./errors"; import { hunkLineRange } from "./hunkRange"; import { LARGE_FILE_MAX_BYTES } from "./limits"; @@ -217,7 +217,7 @@ function parseCommentsFile(raw: string, path: string): CommentsFile { } catch (error) { const detail = error instanceof Error ? error.message : String(error); throw new DunkUserError(`Malformed JSON in ${path}: ${detail}`, [ - "Repair the file by hand, or delete it to start fresh — dunk recreates an empty one on the next write.", + "Repair the file by hand, or delete it to start fresh — dunk treats a missing file as no comments and recreates it on the next comment.", ]); } @@ -242,20 +242,55 @@ function parseCommentsFile(raw: string, path: string): CommentsFile { return result.data; } +/** The absolute path to `.dunk/comments.json` for one repo root. */ +function commentsFilePath(repoRoot: string): string { + return join(repoRoot, DUNK_COMMENTS_RELATIVE_PATH); +} + +/** + * Read the comments file's raw bytes, or `null` when it does not exist. + * + * Resolving the last comment now deletes the file, and the TUI and agent CLI + * ping-pong on the same review, so a concurrent deletion can land between an + * `existsSync` check and the read. Catching `ENOENT` from a single read closes + * that race instead of throwing for what is just an empty review. + */ +function readCommentsRawOrNull(path: string): string | null { + try { + return readFileSync(path, "utf8"); + } catch (error) { + if (error instanceof Error && (error as NodeJS.ErrnoException).code === "ENOENT") { + return null; + } + throw error; + } +} + /** Read `.dunk/comments.json` from the repo root, returning [] if missing. */ export function readCommentsFile(repoRoot: string): CommentsFile { - const path = join(repoRoot, DUNK_DIR, DUNK_COMMENTS_FILENAME); - if (!existsSync(path)) { + const path = commentsFilePath(repoRoot); + const raw = readCommentsRawOrNull(path); + if (raw === null) { return { schema: SCHEMA_VERSION, comments: [] }; } - return parseCommentsFile(readFileSync(path, "utf8"), path); + return parseCommentsFile(raw, path); } /** Atomically write the comments file using a unique temp + rename. */ export function writeCommentsFile(repoRoot: string, file: CommentsFile): void { const dir = join(repoRoot, DUNK_DIR); - mkdirSync(dir, { recursive: true }); const finalPath = join(dir, DUNK_COMMENTS_FILENAME); + + // An empty review has no file. A `{ "schema": 1, "comments": [] }` artifact + // is just noise an agent or human reviewer would have to reason about, and + // reads already treat a missing file as "no comments", so deleting on empty + // round-trips cleanly. `force` makes this a no-op when the file is absent. + if (file.comments.length === 0) { + rmSync(finalPath, { force: true }); + return; + } + + mkdirSync(dir, { recursive: true }); // Unique per-write temp filename so two writers (TUI + agent CLI) never // collide on the same `.tmp` and double-rename through it. Same-filesystem // rename remains atomic, so concurrent readers still see one whole file. @@ -513,12 +548,12 @@ function readCommentsFileWithFingerprint(repoRoot: string): { file: CommentsFile; fingerprint: string | null; } { - const path = join(repoRoot, DUNK_DIR, DUNK_COMMENTS_FILENAME); - if (!existsSync(path)) { + const path = commentsFilePath(repoRoot); + const raw = readCommentsRawOrNull(path); + if (raw === null) { return { file: { schema: SCHEMA_VERSION, comments: [] }, fingerprint: null }; } - const raw = readFileSync(path, "utf8"); return { file: parseCommentsFile(raw, path), fingerprint: createHash("sha256").update(raw).digest("hex"), @@ -527,11 +562,12 @@ function readCommentsFileWithFingerprint(repoRoot: string): { /** Compute the on-disk fingerprint for one comments file, or null if absent. */ function currentFingerprint(repoRoot: string): string | null { - const path = join(repoRoot, DUNK_DIR, DUNK_COMMENTS_FILENAME); - if (!existsSync(path)) { + const path = commentsFilePath(repoRoot); + const raw = readCommentsRawOrNull(path); + if (raw === null) { return null; } - return createHash("sha256").update(readFileSync(path, "utf8")).digest("hex"); + return createHash("sha256").update(raw).digest("hex"); } /** @@ -560,6 +596,6 @@ export function mutateCommentsFile( } throw new Error( - `dunk comments: gave up after ${MUTATE_MAX_ATTEMPTS} optimistic retries on ${join(repoRoot, DUNK_DIR, DUNK_COMMENTS_FILENAME)}.`, + `dunk comments: gave up after ${MUTATE_MAX_ATTEMPTS} optimistic retries on ${commentsFilePath(repoRoot)}.`, ); }