Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
92 changes: 92 additions & 0 deletions src/core/userNotesStore.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { afterEach, describe, expect, test } from "bun:test";
import { chmodSync, mkdtempSync, 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("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");
});
});
});
97 changes: 97 additions & 0 deletions src/core/userNotesStore.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* 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. Set `HUNK_DEBUG=1` to surface swallowed errors on stderr.
*/
import { accessSync, constants, existsSync, mkdirSync, readFileSync, 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. */
export function writeUserNotes(path: string, map: UserNotesMap): void {
try {
mkdirSync(dirname(path), { recursive: true });
writeFileSync(path, JSON.stringify(map, null, 2), { encoding: "utf8" });
} catch (error) {
// Best-effort: a write failure must never crash the review UI.
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