Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/persist-review-notes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"hunkdiff": minor
---

Add an opt-in `--store-notes <path>` flag that persists human review notes to a JSON sidecar (cwd-relative). Notes survive closing the TUI and can be read back off disk by an agent. Omitting the flag keeps notes in-memory only, preserving current behavior.
13 changes: 13 additions & 0 deletions docs/agent-workflows.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,19 @@ hunk patch change.patch --agent-context notes.json

For a compact real example, see [`examples/3-agent-review-demo/agent-context.json`](../examples/3-agent-review-demo/agent-context.json).

## Reverse workflow: persist human review notes for an agent

`--agent-context` loads agent → human notes into the diff. `--store-notes` does the reverse: it persists the **human** notes you write in the TUI (the `c` notes) to a JSON sidecar so an agent can pick them up after you close the review.

```bash
hunk diff <merge-base> --store-notes .hunk/notes.json
```

- The path is resolved relative to the current working directory. Omit the flag to keep notes in-memory only (the default).
- Notes are written through on every change and seeded back on the next run with the same path, so a review resumes where you left off.
- The sidecar is a JSON object keyed by file id, each value an array of notes (`id`, `filePath`, `side`, `line`, `summary`, …) that an agent can read directly off disk — no daemon or live session required.
- The file is **not** auto-ignored by git; point `--store-notes` at an already-ignored location (for example a `.hunk/` directory you gitignore) if you don't want the notes tracked.

## Practical defaults

- start with `hunk session review --repo . --json`
Expand Down
6 changes: 6 additions & 0 deletions src/core/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function buildCommonOptions(
mode?: LayoutMode;
theme?: string;
agentContext?: string;
storeNotes?: string;
pager?: boolean;
watch?: boolean;
transparentBackground?: boolean;
Expand All @@ -68,6 +69,7 @@ function buildCommonOptions(
mode: options.mode,
theme: options.theme,
agentContext: options.agentContext,
storeNotes: options.storeNotes,
pager: options.pager ? true : undefined,
watch: options.watch ? true : undefined,
excludeUntracked: resolveBooleanFlag(argv, "--exclude-untracked", "--no-exclude-untracked"),
Expand All @@ -85,6 +87,10 @@ function applyCommonOptions(command: Command) {
.option("--mode <mode>", "layout mode: auto, split, stack", parseLayoutMode)
.option("--theme <theme>", "named theme override")
.option("--agent-context <path>", "JSON sidecar with agent rationale")
.option(
"--store-notes <path>",
"persist review notes to a JSON sidecar at <path> (cwd-relative)",
)
.option("--pager", "use pager-style chrome and controls")
.option("--line-numbers", "show line numbers")
.option("--no-line-numbers", "hide line numbers")
Expand Down
28 changes: 28 additions & 0 deletions src/core/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,34 @@ describe("config resolution", () => {
expect(bootstrap.initialCopyDecorations).toBe(false);
});

test("--store-notes resolves the sidecar path against cwd; omitting it disables persistence", async () => {
const repo = createTempDir("hunk-store-notes-");
createRepo(repo);

const before = join(repo, "before.ts");
const after = join(repo, "after.ts");
writeFileSync(before, "export const alpha = 1;\n");
writeFileSync(after, "export const alpha = 2;\n");

const withFlag = await loadAppBootstrap(
resolveConfiguredCliInput(
{ kind: "diff", left: before, right: after, options: { storeNotes: ".hunk/notes.json" } },
{ cwd: repo, env: { HOME: repo } },
).input,
{ cwd: repo },
);
expect(withFlag.userNotesSidecarPath).toBe(join(repo, ".hunk", "notes.json"));

const withoutFlag = await loadAppBootstrap(
resolveConfiguredCliInput(
{ kind: "diff", left: before, right: after, options: {} },
{ cwd: repo, env: { HOME: repo } },
).input,
{ cwd: repo },
);
expect(withoutFlag.userNotesSidecarPath).toBeUndefined();
});

test("loadAppBootstrap carries the configured custom theme into the UI bootstrap", async () => {
const home = createTempDir("hunk-config-home-");
const repo = createTempDir("hunk-config-repo-");
Expand Down
2 changes: 2 additions & 0 deletions src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ export function resolveConfiguredCliInput(
// renderer theme-mode detection for their initial palette.
theme: "github-dark-default",
agentContext: input.options.agentContext,
storeNotes: input.options.storeNotes,
pager: input.options.pager ?? false,
watch: input.options.watch ?? false,
excludeUntracked: false,
Expand Down Expand Up @@ -346,6 +347,7 @@ export function resolveConfiguredCliInput(
resolvedOptions = {
...resolvedOptions,
agentContext: input.options.agentContext,
storeNotes: input.options.storeNotes,
pager: input.options.pager ?? false,
watch: input.options.watch ?? resolvedOptions.watch ?? false,
excludeUntracked: resolvedOptions.excludeUntracked ?? false,
Expand Down
3 changes: 3 additions & 0 deletions src/core/loaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,9 @@ export async function loadAppBootstrap(
return {
input,
changeset,
userNotesSidecarPath: input.options.storeNotes
? resolvePath(cwd, input.options.storeNotes)
: undefined,
initialMode: input.options.mode ?? "auto",
initialTheme: input.options.theme,
customTheme,
Expand Down
10 changes: 10 additions & 0 deletions src/core/startup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
import type { AppBootstrap, CliInput, ParsedCliInput, SessionCommandInput } from "./types";
import { canReloadInput } from "./watch";
import { parseCli } from "./cli";
import { userNotesWriteWarning } from "./userNotesStore";

export type StartupPlan =
| {
Expand Down Expand Up @@ -233,6 +234,15 @@ export async function prepareStartupPlan(

bootstrap.initialThemeMode = initialThemeMode ?? bootstrap.initialThemeMode;

// Warn before the TUI takes the screen if --store-notes points somewhere we
// cannot write, so review notes are not silently lost on save.
if (bootstrap.userNotesSidecarPath) {
const warning = userNotesWriteWarning(bootstrap.userNotesSidecarPath);
if (warning) {
process.stderr.write(`${warning}\n`);
}
}

controllingTerminal ??= usesPipedPatchInputImpl(cliInput) ? openControllingTerminalImpl() : null;

return {
Expand Down
4 changes: 4 additions & 0 deletions src/core/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ export interface CommonOptions {
wrapLines?: boolean;
hunkHeaders?: boolean;
agentNotes?: boolean;
/** Path for the human-notes JSON sidecar; when set, notes persist there. */
storeNotes?: string;
copyDecorations?: boolean;
transparentBackground?: boolean;
colorMoved?: boolean;
Expand Down Expand Up @@ -357,6 +359,8 @@ export type ParsedCliInput =
export interface AppBootstrap {
input: CliInput;
changeset: Changeset;
/** Absolute path to the human-notes JSON sidecar, when --store-notes was passed. */
userNotesSidecarPath?: string;
initialMode: LayoutMode;
initialTheme?: string;
initialThemeMode?: TerminalThemeMode;
Expand Down
104 changes: 104 additions & 0 deletions src/core/userNotesStore.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { afterEach, describe, expect, test } from "bun:test";
import { chmodSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import type { UserReviewNote } from "../ui/hooks/useReviewController";
import { readUserNotes, userNotesWriteWarning, writeUserNotes } from "./userNotesStore";

const tempDirs: string[] = [];

afterEach(() => {
while (tempDirs.length > 0) {
const dir = tempDirs.pop();
if (dir) {
rmSync(dir, { recursive: true, force: true });
}
}
});

function createTempDir(prefix: string) {
const dir = mkdtempSync(join(tmpdir(), prefix));
tempDirs.push(dir);
return dir;
}

function note(id: string, summary: string): UserReviewNote {
return {
id,
source: "user",
filePath: "src/foo.ts",
hunkIndex: 0,
side: "new",
line: 1,
summary,
author: "user",
editable: true,
} as UserReviewNote;
}

describe("userNotesStore", () => {
test("writeUserNotes round-trips through readUserNotes, creating missing dirs", () => {
const root = createTempDir("hunk-notes-");
const path = join(root, ".hunk", "notes.json");
const map = { "repo:0:src/foo.ts": [note("user:1", "first"), note("user:2", "second")] };

writeUserNotes(path, map);

expect(readUserNotes(path)).toEqual(map);
});

test("writeUserNotes replaces existing notes atomically without leaving temp litter", () => {
const root = createTempDir("hunk-notes-");
const path = join(root, "notes.json");

writeUserNotes(path, { "repo:0:src/foo.ts": [note("user:1", "first")] });
writeUserNotes(path, { "repo:0:src/bar.ts": [note("user:2", "second")] });

// Full replacement, and the temp sibling used for the atomic rename is gone.
expect(readUserNotes(path)).toEqual({ "repo:0:src/bar.ts": [note("user:2", "second")] });
expect(readdirSync(root).filter((name) => name.endsWith(".tmp"))).toEqual([]);
});

test("readUserNotes tolerates a missing or malformed sidecar", () => {
const root = createTempDir("hunk-notes-");
expect(readUserNotes(join(root, "absent.json"))).toEqual({});

const malformed = join(root, "bad.json");
writeFileSync(malformed, "{ not json");
expect(readUserNotes(malformed)).toEqual({});
});

test("readUserNotes drops entries that are not plausible note arrays", () => {
const root = createTempDir("hunk-notes-");
const path = join(root, "notes.json");
writeFileSync(path, JSON.stringify({ good: [note("user:1", "ok")], bad: [{ nope: true }] }));

expect(Object.keys(readUserNotes(path))).toEqual(["good"]);
});

describe("userNotesWriteWarning", () => {
test("no warning when the sidecar is missing but an ancestor is writable", () => {
const root = createTempDir("hunk-notes-");
expect(userNotesWriteWarning(join(root, ".hunk", "notes.json"))).toBeUndefined();
});

test("no warning when the sidecar already exists and is writable (prior review)", () => {
const root = createTempDir("hunk-notes-");
const path = join(root, "notes.json");
writeUserNotes(path, { "repo:0:src/foo.ts": [note("user:1", "prior")] });

expect(userNotesWriteWarning(path)).toBeUndefined();
});

test("warns when an existing sidecar is read-only", () => {
const root = createTempDir("hunk-notes-");
const path = join(root, "notes.json");
writeFileSync(path, "{}");
chmodSync(path, 0o444);

const warning = userNotesWriteWarning(path);
expect(warning).toContain(path);
expect(warning).toContain("not be saved");
});
});
});
123 changes: 123 additions & 0 deletions src/core/userNotesStore.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
/**
* Disk persistence for human-authored review notes ("c" notes).
*
* Notes are mirrored to the JSON sidecar at the caller-supplied `--store-notes`
* path so they survive closing the TUI and can be read back by an AI agent
* directly off disk. Every disk operation is best-effort: a read failure yields
* an empty map and a write failure is swallowed, so persistence never crashes
* the review UI. Writes go through a temp sibling + atomic rename so an
* interrupted write (SIGTERM, OOM, power loss) can never truncate an existing
* sidecar and lose prior notes. Set `HUNK_DEBUG=1` to surface swallowed errors.
*/
import {
accessSync,
constants,
existsSync,
mkdirSync,
readFileSync,
renameSync,
rmSync,
writeFileSync,
} from "node:fs";
import { dirname } from "node:path";
import type { UserReviewNote } from "../ui/hooks/useReviewController";

export type UserNotesMap = Record<string, UserReviewNote[]>;

/** Walk up from a path to the closest ancestor directory that already exists. */
function nearestExistingDir(path: string): string {
let dir = dirname(path);
while (!existsSync(dir) && dirname(dir) !== dir) {
dir = dirname(dir);
}
return dir;
}

/**
* Best-effort startup heads-up: return a warning when the sidecar at `path`
* cannot be written, else undefined. Existence is fine — only un-writability
* warns. An existing sidecar must itself be writable; a missing one needs its
* nearest existing ancestor writable so `mkdir -p` can create the rest. This is
* a UX signal only; the write path stays fault-tolerant regardless.
*/
export function userNotesWriteWarning(path: string): string | undefined {
const target = existsSync(path) ? path : nearestExistingDir(path);
try {
accessSync(target, constants.W_OK);
return undefined;
} catch {
return `hunk: cannot write review notes to ${path}; notes from this session will not be saved.`;
}
}

/** Emit a swallowed-error diagnostic only when the user opted into debug output. */
function debugUserNotesError(action: string, path: string, error: unknown): void {
if (process.env.HUNK_DEBUG === "1") {
process.stderr.write(`hunk: failed to ${action} user notes at ${path}: ${String(error)}\n`);
}
}

/** Return whether one parsed value is an array of plausible user notes. */
function isUserNoteArray(value: unknown): value is UserReviewNote[] {
return (
Array.isArray(value) &&
value.every(
(entry) =>
typeof entry === "object" &&
entry !== null &&
typeof (entry as { id?: unknown }).id === "string" &&
typeof (entry as { summary?: unknown }).summary === "string",
)
);
}

/** Read persisted human notes, tolerating a missing or malformed sidecar file. */
export function readUserNotes(path: string): UserNotesMap {
let parsed: unknown;
try {
parsed = JSON.parse(readFileSync(path, "utf8"));
} catch (error) {
if ((error as NodeJS.ErrnoException)?.code !== "ENOENT") {
debugUserNotesError("read", path, error);
}
return {};
}

if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
return {};
}

const map: UserNotesMap = {};
for (const [fileId, notes] of Object.entries(parsed as Record<string, unknown>)) {
if (isUserNoteArray(notes)) {
map[fileId] = notes;
}
}
return map;
}

/**
* Persist human notes to the caller-supplied sidecar path, creating parent dirs.
*
* The payload is written to a temp sibling and then `renameSync`d into place.
* `rename(2)` is atomic on POSIX, so a crash mid-write leaves the previous
* sidecar intact rather than a truncated file that `readUserNotes` would discard
* — preserving the durability the feature promises.
*/
export function writeUserNotes(path: string, map: UserNotesMap): void {
const tempPath = `${path}.${process.pid}.tmp`;
try {
mkdirSync(dirname(path), { recursive: true });
writeFileSync(tempPath, JSON.stringify(map, null, 2), { encoding: "utf8" });
renameSync(tempPath, path);
} catch (error) {
// Best-effort: a write failure must never crash the review UI. Drop any
// partial temp file so a failed write can't litter the sidecar directory.
try {
rmSync(tempPath, { force: true });
} catch {
// ignore cleanup failure
}
debugUserNotesError("write", path, error);
}
}
Comment on lines +107 to +123

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Non-atomic write can silently corrupt the sidecar

writeFileSync truncates the file before writing, so a SIGTERM, OOM kill, or power-loss mid-write leaves a partial JSON blob. On the next startup, readUserNotes catches the parse error and returns {}, silently discarding every note the user wrote in previous sessions — exactly the failure mode the feature promises to prevent.

The safe pattern is: write to a sibling temp file (e.g. path + ".tmp"), then renameSync into place. rename(2) is atomic on POSIX, so a kill between write and rename leaves the old sidecar intact rather than an empty one.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/userNotesStore.ts
Line: 89-97

Comment:
**Non-atomic write can silently corrupt the sidecar**

`writeFileSync` truncates the file before writing, so a SIGTERM, OOM kill, or power-loss mid-write leaves a partial JSON blob. On the next startup, `readUserNotes` catches the parse error and returns `{}`, silently discarding every note the user wrote in previous sessions — exactly the failure mode the feature promises to prevent.

The safe pattern is: write to a sibling temp file (e.g. `path + ".tmp"`), then `renameSync` into place. `rename(2)` is atomic on POSIX, so a kill between write and rename leaves the old sidecar intact rather than an empty one.

How can I resolve this? If you propose a fix, please make it concise.

5 changes: 4 additions & 1 deletion src/ui/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,10 @@ export function App({
})),
[activeTheme.id, themeOptions],
);
const review = useReviewController({ files: bootstrap.changeset.files });
const review = useReviewController({
files: bootstrap.changeset.files,
userNotesSidecarPath: bootstrap.userNotesSidecarPath,
});
const filteredFiles = review.visibleFiles;
const selectedFile = review.selectedFile;
const selectedHunkIndex = review.selectedHunkIndex;
Expand Down
Loading