persist human review notes to a JSON sidecar via --store-notes

[muratovv]

What

Opt-in --store-notes <path> (cwd-relative) persists human c review notes to a JSON sidecar keyed by file id, readable directly off disk.

Why

Human notes currently die with the session, so an agent can't pick up your review after you quit — the reverse of --agent-context. Addresses #113 (persist notes past quit; suggests a .hunk/ sidecar).

Behavior

Opt-in; default behavior unchanged. Disk I/O is best-effort and never crashes the UI; startup warning if the path isn't writable. Includes tests, docs/agent-workflows.md, and a minor changeset.

[greptile-apps]

Greptile Summary

Adds an opt-in --store-notes <path> CLI flag that persists the human reviewer's c notes to a JSON sidecar on disk, letting an agent pick them up after the TUI session ends — the reverse of the existing --agent-context flow. The implementation is well-structured: I/O is entirely best-effort (read failures return empty, write failures are swallowed), a pre-TUI warning fires when the path isn't writable, and the feature is fully disabled when the flag is omitted.

• src/core/userNotesStore.ts — new module providing readUserNotes / writeUserNotes / userNotesWriteWarning; the write uses writeFileSync directly (non-atomic), which risks sidecar corruption if the process is killed mid-write.
• src/ui/hooks/useReviewController.ts — seeds state lazily from the sidecar on mount and writes through on every mutation via a new commitUserNotes helper; saveDraftNote was changed from the functional-updater form to a direct value, a minor consistency regression.
• Test coverage in both userNotesStore.test.ts and useReviewController.test.tsx is thorough, exercising round-trip, seed-on-mount, write-through on all mutations, and the no-sidecar fast path.

Confidence Score: 3/5

The change is opt-in and leaves the default path entirely unchanged, but the new sidecar write is non-atomic: a kill during the write truncates the file and the next startup silently discards all previously saved notes, which directly contradicts the feature's durability promise.

The implementation is clean and well-tested, but writeFileSync is not atomic. A SIGTERM or OOM kill mid-write leaves a partially written JSON file; readUserNotes then catches the parse error and returns an empty map, losing every note from prior sessions. Given that durability is the entire reason for the feature, this is worth fixing before merge.

src/core/userNotesStore.ts — the writeUserNotes function needs an atomic write (write to a temp file, then renameSync) to guard against corruption on abnormal exit.

Important Files Changed

Filename	Overview
src/core/userNotesStore.ts	New module: disk persistence for review notes. Read/write/warning helpers with best-effort error handling. The write path is non-atomic — a crash mid-write corrupts the sidecar and causes silent note loss on the next run.
src/ui/hooks/useReviewController.ts	Adds userNotesSidecarPath prop; seeds state from sidecar on mount and writes through on every mutation via commitUserNotes. Swapped functional updater to direct value in saveDraftNote, a minor regression that is safe under normal sequential usage.
src/core/startup.ts	Pre-TUI write-access warning added for --store-notes path; emits to stderr before the TUI takes the screen. Correct and minimal.
src/core/loaders.ts	Resolves storeNotes CLI option to an absolute userNotesSidecarPath in AppBootstrap; straightforward and correct.
src/core/cli.ts	Adds --store-notes <path> option to the shared CLI builder; wired correctly through buildCommonOptions.
src/core/types.ts	Adds storeNotes?: string to CommonOptions and userNotesSidecarPath?: string to AppBootstrap. Clean type additions.
src/core/userNotesStore.test.ts	New test file covering round-trip, missing/malformed sidecar, partial validation, and write-warning scenarios. Good coverage.
src/ui/hooks/useReviewController.test.tsx	Adds persistence integration tests: seed-on-mount, write-through on save/remove/clear, and no-disk-touch when no sidecar configured. Thorough.
src/ui/App.tsx	Passes userNotesSidecarPath from bootstrap into useReviewController. One-line change, correct.
src/core/config.ts	Propagates storeNotes through both resolution branches in resolveConfiguredCliInput. Clean, symmetric with agentContext.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant CLI
    participant loaders as loaders.ts
    participant startup as startup.ts
    participant App as App.tsx
    participant hook as useReviewController
    participant store as userNotesStore
    participant disk as JSON sidecar

    CLI->>loaders: --store-notes .hunk/notes.json
    loaders->>loaders: resolvePath(cwd, storeNotes) → absolute path
    loaders-->>startup: "AppBootstrap { userNotesSidecarPath }"

    startup->>store: userNotesWriteWarning(path)
    store->>disk: accessSync(target, W_OK)
    disk-->>store: ok / EACCES
    store-->>startup: undefined / warning string
    startup->>startup: write warning to stderr if present

    startup-->>App: bootstrap (with userNotesSidecarPath)
    App->>hook: "useReviewController({ userNotesSidecarPath })"

    hook->>store: readUserNotes(path)  [lazy useState init]
    store->>disk: readFileSync + JSON.parse
    disk-->>store: "UserNotesMap | error → {}"
    store-->>hook: seeded userNotesByFileId

    Note over hook,disk: Every mutation (save / remove / clear)
    hook->>store: writeUserNotes(path, next)
    store->>disk: mkdirSync + writeFileSync (best-effort)

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant CLI
    participant loaders as loaders.ts
    participant startup as startup.ts
    participant App as App.tsx
    participant hook as useReviewController
    participant store as userNotesStore
    participant disk as JSON sidecar

    CLI->>loaders: --store-notes .hunk/notes.json
    loaders->>loaders: resolvePath(cwd, storeNotes) → absolute path
    loaders-->>startup: "AppBootstrap { userNotesSidecarPath }"

    startup->>store: userNotesWriteWarning(path)
    store->>disk: accessSync(target, W_OK)
    disk-->>store: ok / EACCES
    store-->>startup: undefined / warning string
    startup->>startup: write warning to stderr if present

    startup-->>App: bootstrap (with userNotesSidecarPath)
    App->>hook: "useReviewController({ userNotesSidecarPath })"

    hook->>store: readUserNotes(path)  [lazy useState init]
    store->>disk: readFileSync + JSON.parse
    disk-->>store: "UserNotesMap | error → {}"
    store-->>hook: seeded userNotesByFileId

    Note over hook,disk: Every mutation (save / remove / clear)
    hook->>store: writeUserNotes(path, next)
    store->>disk: mkdirSync + writeFileSync (best-effort)

Loading

Comments Outside Diff (1)

1. src/ui/hooks/useReviewController.ts, line 869-902 (link)

   saveDraftNote regressed from functional updater to stale-closure form

   The previous code used setUserNotesByFileId((notesByFile) => ({ ...notesByFile, ... })) — the functional-updater pattern that reads the latest committed state regardless of when React schedules the update. The new code reads userNotesByFileId directly from the closure. Because userNotesByFileId is now in the dependency array the callback is recreated on every state change, so it is safe in normal sequential usage. However, two synchronous saves batched before the first render commit would each spread over the same stale snapshot and the first note would be silently dropped. The removeLiveComment / removeUserNote callables already read from the closure, so this is consistent — but saveDraftNote is the only mutation that appends, making it the one place where lost-write matters.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/ui/hooks/useReviewController.ts
   Line: 869-902
   
   Comment:
   **`saveDraftNote` regressed from functional updater to stale-closure form**
   
   The previous code used `setUserNotesByFileId((notesByFile) => ({ ...notesByFile, ... }))` — the functional-updater pattern that reads the *latest committed state* regardless of when React schedules the update. The new code reads `userNotesByFileId` directly from the closure. Because `userNotesByFileId` is now in the dependency array the callback is recreated on every state change, so it is safe in normal sequential usage. However, two synchronous saves batched before the first render commit would each spread over the same stale snapshot and the first note would be silently dropped. The `removeLiveComment` / `removeUserNote` callables already read from the closure, so this is consistent — but `saveDraftNote` is the only mutation that *appends*, making it the one place where lost-write matters.
   
   How can I resolve this? If you propose a fix, please make it concise.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/core/userNotesStore.ts:89-97
**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.

### Issue 2 of 2
src/ui/hooks/useReviewController.ts:869-902
**`saveDraftNote` regressed from functional updater to stale-closure form**

The previous code used `setUserNotesByFileId((notesByFile) => ({ ...notesByFile, ... }))` — the functional-updater pattern that reads the *latest committed state* regardless of when React schedules the update. The new code reads `userNotesByFileId` directly from the closure. Because `userNotesByFileId` is now in the dependency array the callback is recreated on every state change, so it is safe in normal sequential usage. However, two synchronous saves batched before the first render commit would each spread over the same stale snapshot and the first note would be silently dropped. The `removeLiveComment` / `removeUserNote` callables already read from the closure, so this is consistent — but `saveDraftNote` is the only mutation that *appends*, making it the one place where lost-write matters.

_{Reviews (1): Last reviewed commit: "feat: persist human review notes to a JS..." | Re-trigger Greptile}

[greptile-apps]

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.