[allenhutchison]

Human overview

Phase P3 of planning/tui-room-todos-notes.md: in the TUI's Notes pane, c writes a new note, e edits one in $EDITOR, D deletes after a confirmation, and Space ticks a checklist line, for your own notes and a room's. An edit is saved against the version you read, and a 412 follows the same three-way resolution as pepper notes edit. Closes #179 (tracking: #157).

Human required (optional)

• Manual Windows Terminal check of the suspended $EDITOR flow

AI reviewer

What the diff does

• note_edit.py (new): open_in_editor(initial, *, prefix) returns (text, error) instead of printing; texts_match, save_conflict_copy (with its dir helpers) and note_diff (the CLI's _print_note_diff body, returned as text) moved out of cli.py. cli.py keeps _open_in_editor as a thin wrapper (prefix pepper-skill-, errors to render.error) and _print_note_diff (writes the diff to stderr), so the CLI's output is unchanged. Tests that patched cli._conflict_fallback_dir now patch note_edit.conflict_fallback_dir.
• api_client.toggle_checklist (POST /api/v1/artifacts/{id}/toggle, principal= for a room), # MIRROR: src/pepper/api/artifacts.py::ArtifactToggleResponse (exempted in test_api_contract.py like the other method-level mirrors). A 409 surfaces as ApiError with the server sentence in detail.
• state.py: ChecklistItem/checklist_items (index, text, occurrence, matches, as the server's checklist_items) and split_new_note.
• apps_pane.py: bindings c, e, D, space, gated by check_action (Notes only; space only with a note open).
• app.py: a notewrite on-demand loop (one write at a time) using _start -> worker -> _post. Edit reads the note fresh, runs $EDITOR under suspend(), sends nothing when texts_match, otherwise edit_artifact(if_match=etag). 412 saves the conflict copy, shows the diff in an InfoScreen, then a PickerScreen (re-open / overwrite / quit). Every write applies its response and then _want("apps") re-reads the list; a 409 on a tick shows the server's sentence and re-reads the note. SuspendNotSupported gives a notice pointing at pepper notes edit/create. Server text goes through sanitize_line, notices use markup=False.

Choices where the doc is silent

• Space with several checklist lines opens a picker of the lines (a single line is ticked directly); the viewer has no line cursor.
• A new note with a title but no body is not sent (the server refuses a blank body; the CLI aborts the same way).
• The server's own sentence is shown verbatim for 400/403/409 on a write, which covers the imported-note refusal (§3). That refusal comes from the server at save time, so the user finds out after editing, as with the CLI.
• The footer in a Notes pane now lists the new keys, so the your own notes with one note open snapshot is updated.

Where to look hardest: _edit_in_editor runs the blocking editor on the UI thread inside a worker delivery callback (needed for suspend); _edit_conflict's chained InfoScreen -> PickerScreen.

Verification: uv run ruff format --check, ruff check, ty check src/pepper_cli/ clean; uv run pytest --cov: 2695 passed, 2 skipped, coverage 95.2% (floor 88, not touched). Pilot tests patch App.suspend and the editor; one test uses the real suspend to cover the no-terminal notice.

Review passes: /simplify was done as a manual read, not the four-agent run. /code-review medium ran against the umbrella checkout rather than this worktree and reviewed an unrelated file, so it says nothing about this diff; no code-review result is claimed.

Overlap: #173 (P2) and #178 are open and also touch tui/app.py, apps_pane.py and state.py. Both bind space on PaneScroll; here the action is tick, gated to Notes, so the bindings coexist but the BINDINGS/check_action hunks will conflict textually. Whichever lands second rebases.

Checklist (repo-specific)

• uv run ruff format --check and uv run ruff check clean (no new lint ignores)
• uv run ty check src/pepper_cli/ clean
• uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered
• Server-contract changes: /api/v1 usage stays additive-only, # MIRROR: comments updated, unknown fields and frames still tolerated
• No token or credential is logged, printed, or put in a fixture
• README updated (new keys row). Bundled SKILL.md not changed: it does not document the TUI panes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QNDeh8M8jKfHhtWGJwkxTm

Confidence Score: 5/5

The PR appears safe to merge; the reported loading issue is fixed.

Summary

Adds note creation, editing, deletion, and checklist ticks to personal and room Notes panes.

• Notes can be created and edited from the Notes pane.
• The Notes pane asks before deleting a note.
• Checklist lines in an open note can be ticked or unticked.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Write reply arrives] --> B{Written note still open?}
    B -->|Yes| C[Drop older reads and show saved text]
    B -->|No| D[Keep the current note's read]
    C --> E[Refresh the notes list]
    D --> E

Loading

_{Reviews (3) · Last reviewed commit: "fix(tui): a write's answer for a note yo..." · Reviewed by Greptile}

[vycari-review]

Vycari Review (shadow)

Shadow round 3 on ea49189. This comment is non-gating and needs no response: it is collected to measure agreement with the current reviewer. The round's verdict is the Vycari Review (shadow) check run.

Open: P0 0 · P1 4 · P2 5 · confirmed fixed 0 · withdrawn 0

Judge's notes

Three P1 findings from the last round remain open: edits are lost when a conflict is cancelled or Re-open fails, failed saves drop the edit, and a slow read can open the editor on a note the user has left. A new P1 adds that the conflict dialog ignores the user's choice when the save fails after the user has left the pane.

P1 · VR-478a9257 · A 412 that lands after the user left the pane still opens the conflict dialog, and every choice then reports 'Not saved'

src/pepper_cli/tui/app.py lines 2546–2571 · symbol PepperApp._edit_conflict · lens concurrency-time · status new

_edit_conflict never checks that its pane is still the one on screen before it saves the conflict copy and pushes InfoScreen and PickerScreen. The only guard is in chosen(): if pane is not self.pane, every choice, including 'overwrite' and 'reopen', falls through to notify 'Not saved.'. So the user is shown a conflict dialog over whatever pane or thread they have moved to, and the choice they make is discarded. If no conflict copy was written (saved is None), the text the user picked 'overwrite' for is dropped with only a generic notice.

Evidence: app.py:2525-2571: _edit_conflict has no pane-current check before save_conflict_copy and push_screen; chosen() at 2547-2550 refuses every choice when pane is not self.pane. app.py:1915-1948: _show_pane/_hide_pane bump only the 'apps' and 'note' generations, so notewrite results are still delivered after the pane is hidden (_delivered -> _finish returns True for notewrite, since its generation is never bumped). _edit_rejected at 2517-2523 routes ArtifactConflictError to _edit_conflict unconditionally. The existing prior finding VR-7aa6259b covers only the no-copy Esc path on the current pane; this is the different mechanism of the pane changing while the write is in flight.

Failure scenario

1) On a room's note, the user presses e, closes the editor, and the save (notewrite loop, if_match=etag) is in flight. 2) The user presses Esc, which runs _close_pane/_hide_pane. _hide_pane bumps the apps and note generations but not notewrite, so the write is still delivered. 3) The server answers 412. _edit_rejected calls _edit_conflict, which writes the conflict copy, then pushes the diff and the 'Save your edit?' picker over the thread. 4) The user picks 'Overwrite the new version with your text'. chosen() sees pane is not self.pane and shows 'Not saved.' The edit is not sent, and if the conflict copy also failed the text exists nowhere.

How this was verified (traced-callers)

Read _finish and _hide_pane. _hide_pane bumps only the apps and note generations, so notewrite results are still delivered. _edit_rejected routes the 412 to _edit_conflict, and chosen() refuses on pane mismatch. Reachable from: e key in the Notes pane, then Esc while the save is in flight.

Proving test

Using the fake server with a held edit response: open a note, press e, and inside the fake editor's answer, switch pane (Esc) before the write returns. Make the server answer 412 with a current etag. Assert that no PickerScreen is pushed once the pane is no longer self.pane, or, if it is pushed, that choosing 'overwrite' results in a PUT with the user's text and the 'Saved' notice. On head, the choice produces only 'Not saved.' and no edit request.

P1 · VR-7aa6259b · Choosing re-open after a 412 drops the user's text with no picker when the editor cannot run again

src/pepper_cli/tui/app.py lines 2546–2560 · symbol PepperApp._edit_conflict · lens fail-open · status still-open

In chosen(), the 'reopen' branch calls _run_editor(text, 'edit') and returns silently when it yields None (line 2555-2556). No conflict picker is pushed again, so the user is left with the note still stale, the copy-save may have failed (save_conflict_copy returned None, so nothing is on disk), and the in-memory text is discarded when the callback ends. The only signal is the transient notice from _run_editor.

Evidence: Read app.py 2525-2560: chosen() returns at 2555-2556 with no re-prompt and no 'Not saved' message on that branch. save_conflict_copy result is only used for the picker label (2548, 2562-2567). _run_editor (2408-2432) only notifies. Only a second editor failure triggers this, so the harm needs an unusual precondition, hence P2.

Failure scenario

A 412 on an edit. The working directory and the temp fallback cannot take the conflict copy, so the diff is shown with 'Your text is in the diff below'. The user picks 're-open'. The editor cannot be re-launched (for example the terminal no longer supports suspend, or the editor binary errors), so _run_editor returns None. The notice flashes and no save happens. The user's typed text no longer exists anywhere, and the screen offers no way back to the choice that would have kept it.

How this was verified (read-only-reasoning)

Read the chosen() reopen branch. It returns after _run_editor returns None, with only the notice that _run_editor itself raises. The copy is normally on disk, which limits the loss. Reachable from: 412 on an edit, then choosing reopen in the PickerScreen.

Proving test

Pilot test: fake_editor answers the first edit with 'mine', forces a 412 with the conflict copy directory unwritable (PEPPER_CONFLICT_DIR pointing at a file), then makes the second editor call return (None, 'editor exited with status 1'). Press Enter through InfoScreen, choose 'reopen' in the PickerScreen. Assert that a picker is shown again or that the text is still available; on the PR head no picker is shown and the text is gone.

P1 · VR-cabbf528 · A failed save discards the completed note edit

src/pepper_cli/tui/app.py lines 2313–2316 · symbol PepperApp._edit_rejected · lens cross-file-contract · status still-open

Only a 412 preserves the edited text. Other save failures display a notice and discard it, although the editor helper has already deleted its temporary file. No note draft or retry operation retains the completed edit.

Evidence: note_edit.open_in_editor reads the completed edit and unconditionally unlinks its file at src/pepper_cli/note_edit.py:52-54. _send_edit captures the text only in request callbacks at src/pepper_cli/tui/app.py:2296-2300. _edit_rejected preserves it exclusively for ArtifactConflictError at :2313-2315; the remaining branch calls the notice-only failure handler at :2316. _action_failed merely notifies at :1518-1521, and _run has no notewrite retry branch at :568-601. A subsequent e fetches the server body afresh at :2271-2282. The analogous create failure callback at :2256 also retains no recovery state.

Failure scenario

Open a note with e, write substantial changes, and close the editor successfully. The save encounters a connection failure before reaching the server. The TUI reports the network error, but the edited text is now unrecoverable. Pressing e again opens the original server body. Creating a note has the same failure mechanism.

How this was verified (read-only-reasoning)

From the diff, _edit_rejected sends non-conflict errors to _note_write_failed, which only notifies. open_in_editor unlinks its temp file. The new-note on_error likewise only notifies. Reachable from: e or c key followed by a failed save.

Proving test

Have the editor return a distinctive changed body, make edit_artifact raise httpx.ConnectError, and wait for the failure notice. Restore connectivity and press e again; assert that the editor receives the unsaved body for recovery. The head instead supplies the original server body, and no recovery copy exists.

P1 · VR-f1a6abe4 · Pending edit opens after navigation to another note

src/pepper_cli/tui/app.py lines 2273–2277 · symbol PepperApp._edit_in_editor · lens concurrency-time · status still-open

The fresh-read callback checks only pane identity. Leaving a note or opening another note reuses that pane, so the callback can launch an editor for a note the user has already left.

Evidence: app.py:2266-2269 runs the fresh read on notewrite and captures only the pane for delivery. app.py:2274 checks pane identity without checking the note or navigation generation. _end_note at app.py:2062-2068 and _open_note at app.py:2070-2075 invalidate only the note loop, leaving this callback current. app.py:2285 saves using the returned detail's id. planning/tui-room-todos-notes.md §10 specifies dropping results for notes no longer open.

Failure scenario

While viewing note A, press e and delay its fresh detail response. Press Escape, select note B, and open it. When A's response arrives, the terminal unexpectedly opens A in the editor while B is the current note. Saving edits modifies A.

How this was verified (read-only-reasoning)

app.py:2274 checks only `pane is not self.pane`; _end_note and _open_note bump only the note loop, not notewrite. Reachable from: e key then navigating to another note.

Proving test

Delay the fresh read initiated by e on note A. Navigate back and open note B within the same pane, then release A's response. Assert that the editor is not launched and no edit request for A is sent.

P2 · VR-0fdc1242 · An editor save whose temp file cannot be unlinked drops the user's edit in the TUI with a 'couldn't use the editor's file' notice

src/pepper_cli/note_edit.py lines 37–53 · symbol open_in_editor · lens fail-open · status still-open

open_in_editor reads the edited file with Path(path).read_text(...) and then removes it in a finally: os.unlink(path). If unlink raises OSError (on Windows the file can still be held open or locked by the editor or a scanner, which the repo's Windows guidance calls out), the exception propagates and the already-read text is discarded. In the TUI, _run_editor catches OSError around the call and sets text=None, so _edit_in_editor/_send_edit never runs and the edited text is gone. Before this PR the CLI path had the same shape, but the TUI now catches it and drops the edit while reporting only a file error.

Evidence: note_edit.py open_in_editor: return Path(path).read_text(...) inside try with finally os.unlink(path) (no suppression). app.py _run_editor (around 2211-2214) catches (OSError, UnicodeDecodeError) and returns None. tests exercise only read and launch failures, not unlink failure. The temp file is created with prefix pepper-note-, so a stale copy also remains if the unlink fails silently in other paths.

Failure scenario

On Windows Terminal a user presses e on a note, edits it in an editor that keeps the temp file open briefly after exit, and saves. read_text succeeds, unlink raises PermissionError, open_in_editor raises, _run_editor catches OSError and returns None. The notice says couldn't use the editor's file: [WinError 32] ... and no edit is sent. The user's changes exist only in the deleted temp file if the unlink raised before removal.

How this was verified (read-only-reasoning)

From the diff, note_edit.open_in_editor does read_text then an unsuppressed os.unlink in finally, and _run_editor catches OSError and returns None. The base cli._open_in_editor has the same finally unlink. Reachable from: e key in the TUI, then an unlink failure.

Proving test

In tests/test_note_edit.py (posix is fine): monkeypatch os.unlink in note_edit to raise PermissionError after the fake editor appends text, then call open_in_editor and assert the edited text is returned or preserved. In tests/test_tui_note_writes.py, patch note_edit.open_in_editor's unlink to raise and assert that 'e' sends an edit with the edited text; the head sends nothing.

P2 · VR-2231c3dc · A delete that finds the note already gone reports failure and leaves the deleted note in the list

src/pepper_cli/tui/app.py lines 2434–2441 · symbol PepperApp._note_write_failed · lens fail-open · status new

_note_write_failed only special-cases 400/403/409 and otherwise calls _action_failed, which says 'Couldn't delete the note (404…)'. _delete_note's on_error (2592) returns False, so _failed (app.py 462-474) never calls _want('apps') or removes the row. The list keeps the note until some later read.

Evidence: app.py 2434-2441 (status check), 2584-2593 (_delete_note on_error), 447-474 (_failed: on_error False -> failures+=1, _schedule only, no _want). Checked for a re-read after a failed write: _tick_failed (2653-2657) re-reads the note but no equivalent exists for the delete path. Harm is a wrong failure message and a stale row, not persisted wrong state, hence P2.

Failure scenario

Another client deletes the note N. The user, still seeing N in the Notes list, presses D and confirms. The server answers 404. The user sees 'Couldn't delete the note' while N stays selectable and openable in the pane. Pressing D again repeats the 404. Nothing re-reads the list after the failure, so the stale row remains until a later apps read.

How this was verified (traced-callers)

Read _failed. When on_error returns False it only increments failures and schedules. _note_write_failed handles only 400/403/409 specially, so a 404 falls to _action_failed. Unlike _tick_failed, the delete path has no re-read. Reachable from: D key and confirm in the Notes pane, after another client deleted the note.

Proving test

Pilot test: server_with_apps; delete the note's row and detail on the fake server (or set note_write_error to (404, 'not found')) before pressing D and confirming. Assert the pane's notes no longer include the note, or that a '_want(apps)' read occurred. On the PR head the row stays and the notice says the delete failed.

P2 · VR-2c0b607a · Empty-editor notice in the TUI tells a note writer to 'edit skills'

src/pepper_cli/note_edit.py lines 40–40 · symbol open_in_editor · lens cross-file-contract · status still-open

open_in_editor was moved out of cli.py into a shared helper that the TUI now calls for notes (app.py _run_editor, via note_edit.open_in_editor). Its empty-editor error string is still the skill-specific wording '$VISUAL/$EDITOR is empty — set one to edit skills'. The TUI surfaces this string verbatim as an error notice for `e` (edit note) and `c` (new note), so a user editing a note is told the editor is for skills.

Evidence: note_edit.py:40-41 returns the literal string 'set one to edit skills' from the shared helper. app.py:_run_editor (around lines 2201-2216) passes the error through self.notify(sanitize_line(error)) with no context substitution. tests/test_note_edit.py asserts the same string. Consumers checked: cli.py:_open_in_editor (skills, where the wording is correct) and app.py _run_editor (notes, where it is not). Protection looked for: no branch in open_in_editor or _run_editor that rewrites the message by caller; prefix is the only per-caller parameter. Only user-visible text is affected; no data is written wrong.

Failure scenario

A user with VISUAL=' ' (or an empty value) presses `c` or `e` in the TUI Notes pane. _run_editor gets (None, '$VISUAL/$EDITOR is empty — set one to edit skills') from note_edit.open_in_editor and shows it as an error notice. The message is wrong for the context: it names skills, not notes.

How this was verified (read-only-reasoning)

The string is at note_edit.py:40 and _run_editor passes it straight to notify. Reachable from: e or c key in the Notes pane with a blank VISUAL.

Proving test

Set VISUAL to '  ' in a Pilot test of PepperApp with a Notes pane open, press 'e' with suspend patched to a no-op, and assert the notice text does not contain 'skills' (it currently does). The CLI variant in test_note_edit.py asserts the same string, so the shared helper is the source of the wrong text.

P2 · VR-341abaa7 · Space sends a tick computed from a possibly stale checkbox state, so the press can do nothing

src/pepper_cli/tui/app.py lines 2415–2433 · symbol PepperApp._tick · lens concurrency-time · status still-open

_tick sends checked=not item.checked, where item.checked comes from pane.note.body as it was when space was pressed. The toggle endpoint sets the state it is given rather than flipping it (api_client.toggle_checklist docstring: 'checked is the state to set, not the state it was in'). If another writer ticked the same line after this view was last read, the view shows the old box and the send asks for the state it already has. The server accepts it and nothing changes. The user sees the same state after pressing space, and the displayed state is only corrected when the response lands.

Evidence: app.py:2404-2408 (items built from pane.note.body at press time); app.py:2415-2433 (checked=not item.checked); api_client.py toggle_checklist docstring (checked is the target state); the note loop only re-reads when the list etag changes, so the body can be stale for up to one poll. Harm is one wasted keypress with the state corrected by the response, so P2.

Failure scenario

1) The Notes pane shows an unchecked box for 'Sleeping bags' from a read taken before a second client ticked that line. 2) The user presses space. _tick sends checked=True. 3) The server already has the line ticked, so it returns the note unchanged with the line ticked. 4) The user sees no change, and presses space again to untick, which they did not intend.

How this was verified (read-only-reasoning)

From the diff, _tick sends checked=not item.checked, where item comes from pane.note.body at press time. The toggle sets a state rather than flipping, so a stale view gives a harmless idempotent send. Reachable from: space key on an open note.

Proving test

Fake server: the stored line is already ticked while the pane was read as unticked. Press space on that single-line note. Assert the toggle request sends checked=True and that the response leaves the line ticked. The user's intent (a flip) is not met on the first press.

P2 · VR-4d83cebd · A list read that began before a delete can briefly bring the deleted note back into the open pane

src/pepper_cli/tui/app.py lines 2388–2398 · symbol PepperApp._note_deleted · lens concurrency-time · status still-open

_note_deleted removes the note from pane.notes and then calls _want('apps'). If an apps-list read was already in flight when the delete landed, _want only sets the dirty flag. That read was issued before the delete, so its result still contains the note. When it lands, _got_notes (app.py:2037-2042) assigns pane.notes = found.items with no generation check, and the deleted note is rendered again. The corrective re-read runs only afterwards, from the dirty flag in _delivered. The apps loop's generation is not bumped by the delete, so the stale result is not dropped.

Evidence: app.py:2037-2042 (_got_notes assigns without a generation check); app.py:2388-2398 (_note_deleted, _want after the removal); app.py:660-673 (_want sets dirty when busy); app.py:441-443 (_delivered re-runs dirty kinds after deliver). The window lasts one apps round-trip and self-heals, so the harm is transient and the severity is P2.

Failure scenario

1) The apps poll R_a is in flight with the Notes list still showing note N. 2) The user presses D on N and confirms. The delete response arrives: _note_deleted removes N from pane.notes, renders, then _want('apps') sees the loop busy and marks it dirty. 3) R_a lands with N in its items. _got_notes puts N back in pane.notes and renders it. 4) Until the dirty re-read returns, N is visible in the list and Enter opens it. The read then fails with 404 and the pane shows 'Couldn't open the note'.

How this was verified (traced-callers)

Read _got_notes (app.py:2037-2042), which overwrites pane.notes unconditionally, and _delivered (app.py:426-443), which delivers first and re-runs the dirty kind afterwards. A list read issued before the delete can therefore redraw the deleted note until the re-read lands. The effect is transient. Reachable from: D key then confirm, with an apps poll in flight.

Proving test

Gate the GET apps list route on the fake server. Open Notes, start the apps read, press D on a note and confirm. Release the gate with the old list. Assert that the rendered pane does not contain the deleted note after the apps read is delivered, before the dirty re-read is answered.

[allenhutchison]

Review round 1: addressed (head bc4d1ad)

Greptile scored 4/5 on e5c17fe (read from the PR description block). This push fixes all three findings, plus two found in self-review and the Windows failure.

Greptile findings

• P1 "editor errors end the session" (app.py): fixed in 89e581f. A temp-file OSError or UnicodeDecodeError is now a notice, not a crash.
• P2 "saved changes appear to revert" (app.py): fixed in 3dcc0f6. A note read that started before a write is dropped when the write's answer arrives.
• P2 "new notes lose indentation" (state.py): fixed in bc4d1ad. split_new_note keeps the first body line's indent.

Self-review

• 035099a: when the conflict copy can't be saved, the 412 picker no longer says "your text stays saved". It says "Quit (your text is lost)".
• 0166cac (windows check): the fake server minted quoted etags, and the " in them was illegal in the Windows conflict-copy filename. The fake now mints bare etags, as the server's JSON does. Only the ETag header is quoted.

Accepted choices, unchanged. These were decided before review: a note with only a title is not sent, an imported note is refused, space opens a picker when there are several checklist lines, and the server's 400/403/409 sentences are shown verbatim.

Pre-flight before the push: ruff format, ruff check, ty, pytest (2702 passed, 2 skipped), coverage 95.2% against a floor of 88.

review.responderTier is capable. This run is inheriting the session model.

[allenhutchison]

Review round 2: addressed (head fc598de)

Greptile scored 4/5 on bc4d1ad (read from the PR description block). It had one new finding, and this push fixes it.

• P1 "next note stays loading" (app.py): fixed in fc598de. _apply_written_note now acts only while the written note is still open, so a tick that answers after you've opened another note no longer drops that note's read. A new test covers the race, and it fails without the fix.

Thread 4415168043 (editor errors, round 1) is listed again against bc4d1ad. It is the same thread, re-anchored after the push, not a new raise. 89e581f addressed it.

Checks on bc4d1ad were green, windows included. That confirms the round 1 etag fixture fix.

Pre-flight before the push: ruff format, ruff check, ty, pytest (2703 passed, 2 skipped), coverage 95.2% against a floor of 88.

review.responderTier is capable. This run is inheriting the session model.