From 69c76f1eb4919dddfeeecfda7bf52bcb26d1a160 Mon Sep 17 00:00:00 2001 From: memtomem <118508877+memtomem@users.noreply.github.com> Date: Thu, 5 Nov 2026 10:02:33 +0900 Subject: [PATCH 1/4] Record verdicts, promote lessons and choose round lessons in the TUI (#174) The Rooms tab gains the human review actions the CLI already has: - j records accepted/rejected on an applied room (RoomCoordinator.judge, surface="tui"), fenced on the patch digest the dialog showed. - p promotes a lesson from a judged room; promote_lesson gains expected_judgment_id, checked on the same outcome read as the provenance, so a verdict changed while the dialog was open is never promoted. - The round dialog lists the project's approved lessons with checkboxes, recomputes select_lessons(skip=unchecked) live, refuses (never drops) when round_lessons_fit fails under round_prompt's fallback rule, and the worker gets exactly the shown pairs. The history note moves into the dialog, per lineage, before confirmation. Every Rooms dialog callback acts on the room id and store root captured when it opened (the round dialog used the ambient selection). The room pane shows the outcome; the evidence snapshot also compares the judgment count, degrading locally. _refresh_board keeps every read inside its guard. Checkbox labels from stored text go through plain(), and markup_sinks flags any other non-literal label. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 21 +- CLAUDE.md | 2 +- docs/architecture/tui.md | 20 + docs/assets/tui/09-rooms-diff.svg | 20 +- docs/assets/tui/10-rooms-output.svg | 20 +- docs/assets/tui/11-rooms-details.svg | 24 +- docs/assets/tui/12-rooms-compare.svg | 24 +- docs/user-guide.ko.md | 15 +- docs/user-guide.md | 17 +- src/syncmill/rooms.py | 24 +- src/syncmill/tui.py | 386 +++++++++++-- src/syncmill/tui_collaboration.py | 325 ++++++++++- tests/test_tui.py | 105 ++-- tests/test_tui_render.py | 14 +- tests/test_tui_room_review.py | 799 +++++++++++++++++++++++++++ 15 files changed, 1637 insertions(+), 179 deletions(-) create mode 100644 tests/test_tui_room_review.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c0ff6a5..1472de9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,16 @@ All notable changes to SyncMill are documented here. checkpoint and before the task, with the excerpts and memory context shrinking to make room. A resumed conversation keeps what earlier rounds sent it, so a round that would resume one in a room with earlier lessons adds a note about - `--fresh-session`. TUI-launched rounds do not send lessons yet. + `--fresh-session`. +- The TUI Rooms tab records verdicts and promotes lessons (#174): `j` records + accepted/rejected on an applied room, bound to the patch digest the dialog + showed; `p` promotes a lesson from a judged room and refuses if the verdict + changed while the dialog was open (`RoomCoordinator.promote_lesson` gains + `expected_judgment_id`). The room pane shows the outcome. TUI rounds now send + the project's approved lessons: the round dialog lists them with checkboxes, + applies the CLI's skip and budget rules live, and refuses to start when the + prompt leaves no room for the checked lessons; the history note shows there, + before you confirm, for the lineage you pick. The round conversation record is now `schema_version` 2: it adds the selected lesson ids and each member's delivery, shown by `room compare`, `room export` and the TUI (`rounds[].conversation.lessons`, @@ -70,6 +79,16 @@ All notable changes to SyncMill are documented here. ### Fixed +- The TUI no longer exits when the board's decisions or the selected item + cannot be read during a Board-tab refresh; it shows `DEGRADED` and keeps + the last view, as it already did for the item list. +- A TUI round, verdict or promotion acts on the room its dialog was opened + for. A round dialog used to launch on whichever room was selected when you + confirmed, which a refresh could have changed meanwhile. +- A verdict recorded from any surface now marks an open TUI Compare or + Details snapshot as changed; a verdict changes neither the room's revision + nor its candidates, so the snapshot used to keep showing the old outcome + without saying so. - `syncmill start` suggests verification commands only from files whose name matches exactly, so each suggestion names a file that is on disk. On a case-insensitive filesystem (the macOS default) a repository with only a diff --git a/CLAUDE.md b/CLAUDE.md index d1eea0a..867d896 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,7 @@ uv run syncmill room export --include patch: [--include output:| # room rounds record their effective starting tree + gate settings (v2 adds the gate's env_allowlist: null = inherited, [] distinct) in rounds.provenance_json (rooms.round_provenance_status, writer and readers share it, readers accept v1+v2; NULL = legacy); compare/export/TUI show them as starting tree / gate settings beside the report-derived gate profile, whose JSON fields are unchanged; a v1 round's environment reads not recorded and never groups as same settings; `room apply --verify` still runs the current `[gate]` and prints a `verify:` notice (`rooms.verify_settings_notice`) unless the round's v2 record equals it uv run syncmill room select [--expected-revision N --expected-digest SHA256 --rationale TEXT] # verifies the artifact, records a bound selection (rooms.selection_json); apply refuses an inconsistent record uv run syncmill room judge accepted|rejected [--expected-digest SHA256 --rationale TEXT] # #174: append-only human verdict on the APPLIED candidate (room_judgments, bound to candidate + patch digest + applied run; never inferred from select/apply, never touches memory, no state/revision change); rooms.room_outcome_status is the one read model (not_selected / selected_not_applied / applied_unreviewed / accepted / rejected / unknown — a damaged latest record is unknown, never an older verdict) shown by compare/export as room.outcome -uv run syncmill room promote --lesson TEXT [--scope GOAL --supersedes ID] # #174 PR 2: needs the board; only a judged room (accepted/rejected); a board decision bound to project_id with provenance_json (room/judgment/verdict/candidate/digest/run) and a unique source_key (retry returns the existing decision, never reactivates a superseded one); `run` shows then injects Board.project_lessons (project-wide scope only; board.select_lessons budget; --skip-lesson/--no-lessons, forwarded in _inner_argv) via Supervisor.run(lessons=) AFTER routing classified the raw prompt; board items get unbound + own-project decisions; room rounds: `room round`/`start` show them (cli._show_round_lessons, rooms.round_lessons_fit) and pass RoundLesson pairs to run_round(lessons=) (detached: a second anonymous fd, --lessons-fd); the coordinator never opens the board; lessons sit after the checkpoint, before the task, excerpts budgeted around them; TUI rounds send none (note only) +uv run syncmill room promote --lesson TEXT [--scope GOAL --supersedes ID] # #174 PR 2: needs the board; only a judged room (accepted/rejected); a board decision bound to project_id with provenance_json (room/judgment/verdict/candidate/digest/run) and a unique source_key (retry returns the existing decision, never reactivates a superseded one); `run` shows then injects Board.project_lessons (project-wide scope only; board.select_lessons budget; --skip-lesson/--no-lessons, forwarded in _inner_argv) via Supervisor.run(lessons=) AFTER routing classified the raw prompt; board items get unbound + own-project decisions; room rounds: `room round`/`start` show them (cli._show_round_lessons, rooms.round_lessons_fit) and pass RoundLesson pairs to run_round(lessons=) (detached: a second anonymous fd, --lessons-fd); the coordinator never opens the board; lessons sit after the checkpoint, before the task, excerpts budgeted around them; TUI (Rooms `j` verdict / `p` promote, `tui.py` `_confirm_room_judge`/`_confirm_lesson_promote`): the round dialog reads `project_lessons` once on the Textual thread (`RoundLessonsView`), recomputes `select_lessons(skip=unchecked)` live, refuses (never drops) when `round_lessons_fit` fails on `round_prompt`'s fallback rule, and launches with exactly those pairs; promote passes `expected_judgment_id` (checked on the same outcome read as the provenance); every Rooms dialog callback acts on the captured room id + store root uv run syncmill conversations list uv run syncmill conversations show uv run syncmill conversations close diff --git a/docs/architecture/tui.md b/docs/architecture/tui.md index bc1ef40..162ee0d 100644 --- a/docs/architecture/tui.md +++ b/docs/architecture/tui.md @@ -86,6 +86,26 @@ the store root changes or the store becomes unavailable. Both views read rows from the snapshot (no artifact read on the event loop), so their gate/eligibility/usage/blocker wording is the CLI's; the Output view (`o`) interprets none of it. +Rooms review actions (#174): `j` records a verdict (`RoomCoordinator.judge`, +`surface="tui"`, fenced on the patch digest the dialog showed), `p` promotes a +lesson (`promote_lesson(expected_judgment_id=)`, refused when the latest +judgment is no longer the one the dialog showed), and the `x` round dialog +shows the project's approved lessons (`RoundLessonsView`, read once when it +opens) and recomputes the CLI's `select_lessons(skip=)` on every checkbox, +prompt or lineage change. It refuses instead of dropping lessons when the +prompt leaves no room (`round_prompt` gives the fit check and the launch one +fallback rule), and the worker gets exactly the shown pairs. Every Rooms dialog +captures `(room id, store root)` when it opens; its callback acts on those and +refuses when the store was rebound, never reading the ambient selection. Judge +and promote run on the Textual thread on the existing connections (SQLite is +thread-affine; the board connection must not enter a worker), accepting up to +the stores' 5 s busy timeout of blocking under write contention. The evidence +snapshot also records the room's judgment count, read before the build: a +verdict changes neither revision nor candidates, so `changed` compares the +count too, and a failed count read degrades to a "cannot tell" line instead +of aborting the staged room update. Checkbox labels built from stored text go +through `plain(...)` (Textual parses a `str` label as markup); the +`markup_sinks` guard flags any other non-literal label. EVERY poll path is degrade-guarded (`_ROOM_POLL_ERRORS`/`_SESSION_POLL_ERRORS`/ `_JOB_POLL_ERRORS`) — an unguarded read escapes the interval callback and kills the app, so a store failure must show DEGRADED and keep the last good view. diff --git a/docs/assets/tui/09-rooms-diff.svg b/docs/assets/tui/09-rooms-diff.svg index 2b42eb0..3f0e39d 100644 --- a/docs/assets/tui/09-rooms-diff.svg +++ b/docs/assets/tui/09-rooms-diff.svg @@ -172,7 +172,7 @@ - + ⭘syncmill tui BoardRunsReportsTeamsStatusSessionsRooms @@ -186,10 +186,10 @@ ▊▎│ ▊▎│Recent events ▊▎│round 1 completed -▊▎│candidate selected -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│Candidate: -┌──────────────────────────────────────────────────────────────────────┐│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d | Diff -▊a1b2c3d4 codex completed ✓▎│sha256: fedcba9876543210 +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│candidate selected +┌──────────────────────────────────────────────────────────────────────┐│Candidate: +▊a1b2c3d4 codex completed ✓▎│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d | Diff +▊▎│sha256: fedcba9876543210 ▊▎│ ▊▎│diff --git a/room-smoke.txt b/room-smoke.txt ▊▎│new file mode 100644 @@ -208,11 +208,11 @@ ▊▎│ ▊▎│ ▊▎│ -▊▎│ -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│ -rooms:1 | candidates:1 | view:diff - -a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | R recover | k close +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│ +rooms:1 | candidates:1 | view:diff + +a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | j verdict | p promote | R +recover | k close 1-7 tabs | r refresh | ? help | q quit diff --git a/docs/assets/tui/10-rooms-output.svg b/docs/assets/tui/10-rooms-output.svg index a22c202..61513bd 100644 --- a/docs/assets/tui/10-rooms-output.svg +++ b/docs/assets/tui/10-rooms-output.svg @@ -172,7 +172,7 @@ - + ⭘syncmill tui BoardRunsReportsTeamsStatusSessionsRooms @@ -186,10 +186,10 @@ ▊▎│ ▊▎│Recent events ▊▎│round 1 completed -▊▎│candidate selected -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│Candidate: -┌──────────────────────────────────────────────────────────────────────┐│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d -▊a1b2c3d4 codex completed ✓▎│[Output] +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│candidate selected +┌──────────────────────────────────────────────────────────────────────┐│Candidate: +▊a1b2c3d4 codex completed ✓▎│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d +▊▎│[Output] ▊▎│State: completed | exit: 0 | 18.2s ▊▎│Gate, usage, eligibility: see Details (m) or ▊▎│Compare (c) @@ -208,11 +208,11 @@ ▊▎│ ▊▎│ ▊▎│ -▊▎│ -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│ -rooms:1 | candidates:1 | view:output - -a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | R recover | k close +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│ +rooms:1 | candidates:1 | view:output + +a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | j verdict | p promote | R +recover | k close 1-7 tabs | r refresh | ? help | q quit diff --git a/docs/assets/tui/11-rooms-details.svg b/docs/assets/tui/11-rooms-details.svg index 5627db9..3faaaa9 100644 --- a/docs/assets/tui/11-rooms-details.svg +++ b/docs/assets/tui/11-rooms-details.svg @@ -173,7 +173,7 @@ - + ⭘syncmill tui BoardRunsReportsTeamsStatusSessionsRooms @@ -187,10 +187,10 @@ ▊▎│ ▊▎│Recent events ▊▎│round 1 completed -▊▎│candidate selected -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│Candidate: -┌──────────────────────────────────────────────────────────────────────┐│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d -▊a1b2c3d4 codex completed ✓▎│[Details] +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│candidate selected +┌──────────────────────────────────────────────────────────────────────┐│Candidate: +▊a1b2c3d4 codex completed ✓▎│a1b2c3d4-5e6f-4a7b-8c9d-0e1f2a3b4c5d +▊▎│[Details] ▊▎│evidence as of 12:00:00 ▊▎│Member: codex (codex) ▊▎│Round: 1 | attempt: 1 @@ -200,20 +200,20 @@ ▊▎│Gate settings: commands, timeout 300s, ▊▎│environment inherited (no allowlist): ▊▎│pytest -q -▊▎│Conversations: each member has its own; +▊▎│Conversations: each member has its own;▁▁ ▊▎│sessions: cold 3; memory context: none -▊▎│found; approved lessons: not recorded (an▆▆ +▊▎│found; approved lessons: not recorded (an ▊▎│older round); checkpoint excerpts are ▊▎│capped at 4,000 bytes each ▊▎│Conversation: cold session; no earlier ▊▎│candidates; no memory context found ▊▎│Checkpoint candidates: none ▊▎│State: completed | eligible: yes -▊▎│Exit: 0 | duration: 18.2s -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│Gate: passed -rooms:1 | candidates:1 | view:details - -a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | R recover | k close +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│Exit: 0 | duration: 18.2s +rooms:1 | candidates:1 | view:details + +a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | j verdict | p promote | R +recover | k close 1-7 tabs | r refresh | ? help | q quit diff --git a/docs/assets/tui/12-rooms-compare.svg b/docs/assets/tui/12-rooms-compare.svg index 8791979..5fe9302 100644 --- a/docs/assets/tui/12-rooms-compare.svg +++ b/docs/assets/tui/12-rooms-compare.svg @@ -173,7 +173,7 @@ - + ⭘syncmill tui BoardRunsReportsTeamsStatusSessionsRooms @@ -187,10 +187,10 @@ ▊▎│ ▊▎│Recent events ▊▎│round 1 completed -▊▎│candidate selected -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│[Compare]  room room-000 -┌──────────────────────────────────────────────────────────────────────┐│(awaiting_decision) -▊a1b2c3d4 codex completed ✓▎│Round 1 of 1 (complete) +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│candidate selected +┌──────────────────────────────────────────────────────────────────────┐│[Compare]  room room-000 +▊a1b2c3d4 codex completed ✓▎│(awaiting_decision) +▊▎│Round 1 of 1 (complete) ▊▎│starting tree: snapshot (requested lineage: ▊▎│continue-own) ▊▎│gate settings: commands, timeout 300s, @@ -200,8 +200,8 @@ ▊▎│conversations: each member has its own; ▊▎│sessions: cold 3; memory context: none ▊▎│found; approved lessons: not recorded (an -▊▎│older round); checkpoint excerpts are -▊▎│capped at 4,000 bytes each▂▂ +▊▎│older round); checkpoint excerpts are▅▅ +▊▎│capped at 4,000 bytes each ▊▎│source: unchanged since the room started ▊▎│Selected: none ▊▎│evidence as of 12:00:00 @@ -209,11 +209,11 @@ ▊▎│> a1b2c3d4 codex completed ▊▎│  conv: cold, full, 0 cand, mem none ▊▎│  gate passed · 18.2s · in 40.1k out 2.3k -▊▎│  146 B · 1 files · eligible · blockers 0 -▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│  e5f6a7b8 claude completed -rooms:1 | candidates:2 | view:compare - -a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | R recover | k close +▊▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▎│  146 B · 1 files · eligible · blockers 0 +rooms:1 | candidates:2 | view:compare + +a create | x round | t terminal | d/o/m/c diff/output/details/compare | e select | g apply | j verdict | p promote | R +recover | k close 1-7 tabs | r refresh | ? help | q quit diff --git a/docs/user-guide.ko.md b/docs/user-guide.ko.md index 0f22cd0..50cb6e0 100644 --- a/docs/user-guide.ko.md +++ b/docs/user-guide.ko.md @@ -888,8 +888,11 @@ Room 라운드에도 같은 프로젝트 전체 교훈이 들어갑니다. `room 내용이 그대로 남습니다. 교훈을 빼면 다시 보내지 않을 뿐, 이미 보낸 것을 거둘 수는 없습니다. 이전 라운드에 교훈이 있던 방에서 대화를 이어 가는 라운드라면 목록에 `--fresh-session`을 안내하는 문장이 붙습니다. 이 안내는 목록을 보여 준 시점의 방 상태를 기준으로 하므로, -detached worker가 시작하기 전에 끝난 라운드는 반영하지 않습니다. TUI에서 시작한 라운드는 아직 교훈을 보내지 않고 같은 안내를 -보여 줍니다. `room compare`는 라운드가 고른 교훈과 후보마다 교훈이 들어갔는지를 보여 줍니다. +detached worker가 시작하기 전에 끝난 라운드는 반영하지 않습니다. TUI에서 시작한 라운드도 +교훈을 보냅니다. 라운드 대화상자가 교훈마다 체크박스를 보여 주고(체크 해제 = `--skip-lesson`), +예산 때문에 빠진 교훈을 표시하며, 프롬프트가 체크한 교훈을 담을 자리를 남기지 않으면 교훈을 +빼고 진행하는 대신 시작을 거부합니다. 이전 교훈 안내는 고른 lineage에 맞춰 대화상자 안에 +나옵니다. `room compare`는 라운드가 고른 교훈과 후보마다 교훈이 들어갔는지를 보여 줍니다. `room apply --verify`는 다른 `--verify`와 마찬가지로 라운드에 기록된 설정이 아니라 현재 `[gate]`로 검증합니다. 기록을 다시 쓰면 지금 설정한 것보다 넓은 환경이 verify에 넘어갈 수 @@ -1065,7 +1068,13 @@ Rooms의 `a`는 prompt와 member만 기록하며 provider를 호출하지 않습 잘림 상태). 완료된 후보는 Diff로, 실패한 후보는 Output으로 열려요. `R`은 `room recover`로 **프로젝트 전체**의 죽은 room 워커/중단된 apply를 복구하고, `k`는 선택한 room을 -닫습니다(후보와 영수증은 보존). +닫습니다(후보와 영수증은 보존). room 화면에는 결과(outcome)가 나옵니다. `j`는 적용된 room에 +대한 판정을 기록합니다(`room judge`: accepted 또는 rejected를 고르고 이유를 덧붙일 수 있으며, +판정은 대화상자에 보인 patch digest에 묶입니다). `p`는 판정된 room에서 교훈을 승격합니다 +(`room promote`, board 필요). 대화상자는 교훈에 기록될 출처를 보여 주고, 열려 있는 동안 판정이 +바뀌었으면 거부합니다. `x`의 라운드 대화상자는 프로젝트의 승인된 교훈을 보여 줍니다(Rooms 참고). +이 대화상자들은 그사이 선택이 바뀌어도 처음 연 room에만 적용됩니다. 어디서든 판정이 기록되면 +열려 있는 Compare/Details 스냅샷에 바뀌었다는 표시가 붙습니다. 실행은 Textual이 아니라 분리된 백그라운드 매니저가 맡아요. `q`는 화면만 닫고 실행은 계속합니다. 다시 `syncmill tui`를 열면 실행 상태와 로컬 rolling 로그에 재연결돼요. diff --git a/docs/user-guide.md b/docs/user-guide.md index 28f3691..808ded7 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -964,8 +964,11 @@ conversation keeps whatever earlier rounds sent it: leaving a lesson out stops i being sent again, it cannot take it back. When a round would resume a conversation in a room whose earlier rounds had lessons, the list adds a note pointing to `--fresh-session`. The note reflects the room as of the listing: a round that -finishes before a detached worker starts is not taken into account. Rounds started from the TUI do not send lessons yet and show -the same note. `room compare` shows which lessons a round selected and, per +finishes before a detached worker starts is not taken into account. Rounds started from +the TUI send lessons too: the round dialog lists them with a checkbox each (unchecking is +`--skip-lesson`), names any left out by the budget, and refuses to start, instead of +dropping them, when the prompt leaves no room for the checked ones; the history note +appears in the dialog for the lineage you pick. `room compare` shows which lessons a round selected and, per candidate, whether they were included. `room apply --verify` runs the current `[gate]`, like every `--verify`, not the @@ -1175,7 +1178,15 @@ code, per-check gate results, files, digests, and truncation state). A completed candidate opens on Diff; a failed one opens on Output. `R` recovers dead room workers/interrupted applies for the whole project via `room recover`, and `k` closes the -selected room (candidates and receipts are preserved). +selected room (candidates and receipts are preserved). The room pane shows its +outcome. `j` records your verdict on an applied room (`room judge`: you pick +accepted or rejected, add an optional rationale, and the verdict is bound to the +patch digest the dialog showed). `p` promotes a lesson from a judged room +(`room promote`, needs the board); the dialog shows the source the lesson will +record and refuses if the verdict changed while it was open. `x` lists the +project's approved lessons in the round dialog (see Rooms). These dialogs act on +the room they were opened for, even if the selection moves meanwhile. A verdict +recorded anywhere marks an open Compare or Details snapshot as changed. Runs are owned by a detached manager, not Textual. Only one TUI-managed worker per board is allowed, but ordinary external `board run` processes remain legal. Pressing diff --git a/src/syncmill/rooms.py b/src/syncmill/rooms.py index 9f1f533..65c520b 100644 --- a/src/syncmill/rooms.py +++ b/src/syncmill/rooms.py @@ -2959,6 +2959,15 @@ def list_judgments(self, room_id: str) -> list[dict[str, Any]]: ).fetchall() return [dict(row) for row in rows] + def judgment_count(self, room_id: str) -> int: + """How many judgment rows the room holds: a cheap freshness signal for + a view built from an earlier read (a verdict changes neither the + room's revision nor its candidates).""" + row = self._conn.execute( + "SELECT COUNT(*) FROM room_judgments WHERE room_id=?", (room_id,) + ).fetchone() + return int(row[0]) + def outcome(self, room_id: str) -> OutcomeCheck: """The room's human outcome, read in one snapshot.""" with self._read_snapshot() as conn: @@ -4242,13 +4251,18 @@ def promote_lesson( *, scope: str = "", supersedes: str | None = None, + expected_judgment_id: str | None = None, ) -> tuple[Any, bool]: """Promote a short, human-written lesson from a judged room into the board's decisions, bound to this project (#174). Only a room whose latest judgment binds (accepted or rejected) can be promoted: the lesson's provenance names that judgment, and a retry of - the same promotion returns the decision it already created.""" + the same promotion returns the decision it already created. + ``expected_judgment_id`` refuses the promotion unless that judgment is + still the latest, checked on the same read the provenance comes from, + so a verdict recorded after the person reviewed it is never promoted + in its name.""" text = normalize_lesson(lesson) if not isinstance(scope, str): raise RoomError("lesson scope must be text") @@ -4263,6 +4277,14 @@ def promote_lesson( f"room outcome is {outcome.status}{detail}; record a verdict with " "`syncmill room judge` before promoting a lesson" ) + if ( + expected_judgment_id is not None + and judgment["judgment_id"] != expected_judgment_id + ): + raise RoomError( + "the room's verdict changed since you reviewed it; review the " + "promotion again" + ) provenance = { "kind": "room-judgment", "room_id": room_id, diff --git a/src/syncmill/tui.py b/src/syncmill/tui.py index b8260a9..5b9e976 100644 --- a/src/syncmill/tui.py +++ b/src/syncmill/tui.py @@ -75,9 +75,18 @@ build_room_comparison, find_round, find_row, + outcome_lines, render_round_table, ) -from syncmill.rooms import RoomCoordinator, RoomError, RoomStore, launch_room_worker +from syncmill.rooms import ( + JUDGMENT_VERDICTS, + RoomCoordinator, + RoomError, + RoomStore, + RoundLesson, + launch_room_worker, + round_lessons_fit, +) from syncmill.session import SessionLaunchError from syncmill.review_candidates import ( DEFAULT_GOAL as REVIEW_DEFAULT_GOAL, @@ -119,8 +128,11 @@ from syncmill.tui_collaboration import ( CandidateSelectModal, CollaborationConfirmModal, + LessonPromoteModal, RoomCreateModal, + RoomJudgeModal, RoomRoundModal, + RoundLessonsView, SessionOpenModal, _patch_is_binary, render_candidate_details, @@ -128,6 +140,7 @@ render_candidate_output, render_room, render_session, + round_prompt, ) _CLI_SHIM = "from syncmill.cli import cli\ncli(prog_name='syncmill')" @@ -368,7 +381,7 @@ def render_artifact(manifest: dict, result: dict | None) -> str: "sessions-tab": "a open | x attach | c checkpoint | k close", "rooms-tab": ( "a create | x round | t terminal | d/o/m/c diff/output/details/compare | " - "e select | g apply | R recover | k close" + "e select | g apply | j verdict | p promote | R recover | k close" ), } @@ -662,15 +675,22 @@ def on_button_pressed(self, event: Button.Pressed) -> None: def build_room_evidence( store_root: Path, output_max_bytes: int, repo_root: Path, room_id: str, now: float -) -> tuple[RoomComparison, int, frozenset]: +) -> tuple[RoomComparison, int, frozenset, int | None]: """Build one room's evidence snapshot. Runs in a thread worker: SQLite connections are thread-affine, so it opens its own store and closes it. - Returns the comparison plus the room revision and candidate ids read - BEFORE the build, so anything that changes during it counts as a change.""" + Returns the comparison plus the room revision, candidate ids and judgment + count read BEFORE the build, so anything that changes during it counts as + a change. A verdict changes neither revision nor candidates, hence the + count; it is None when it could not be read (freshness then rests on the + other two).""" store = RoomStore(store_root, output_max_bytes) try: room = store.get_room(room_id) candidate_ids = frozenset(c["id"] for c in store.list_candidates(room_id)) + try: + judgments: int | None = store.judgment_count(room_id) + except sqlite3.Error: + judgments = None comparison = build_room_comparison( store, room_id, @@ -680,7 +700,7 @@ def build_room_evidence( ) finally: store.close() - return comparison, int(room["revision"]), candidate_ids + return comparison, int(room["revision"]), candidate_ids, judgments def select_room_candidate( @@ -738,6 +758,8 @@ class SyncmillTuiApp(App[None]): ("c", "checkpoint", "Checkpoint"), ("K", "force_stop", "Force stop"), ("R", "recover_runs", "Recover"), + ("j", "judge", "Verdict"), + ("p", "promote", "Promote"), ("?", "help", "Help"), ("q", "quit", "Quit UI"), ] @@ -1236,7 +1258,7 @@ def action_quit(self) -> None: def action_help(self) -> None: self.notify( - "1-7 tabs; Sessions a/x/c/k; Rooms a/x/t/d/o/m/c/e/g/R/k; " + "1-7 tabs; Sessions a/x/c/k; Rooms a/x/t/d/o/m/c/e/g/j/p/R/k; " "Board a/i/d/e/u/h/s/x/g; Runs v/f/k/K/R (Artifacts: d/g/k); " "q detaches" ) @@ -1332,7 +1354,12 @@ def _refresh_board(self) -> None: try: items = self._board.list(state=self._filter) stats = self._board.stats() + decisions = len(self._board.active_decisions()) + selected_exists = bool( + self._selected_item_id and self._board.get(self._selected_item_id) + ) except BoardError as exc: + # Every read is in here: one escaping a poll tick kills the app. self._board_error = str(exc) self._update_static("#board-status", f"DEGRADED: {exc}") return @@ -1371,12 +1398,12 @@ def _refresh_board(self) -> None: ) table.move_cursor(row=index) table.scroll_to(y=min(scroll_y, max(0, len(rows) - 1)), animate=False) - if self._selected_item_id and self._board.get(self._selected_item_id): + if selected_exists and self._selected_item_id: self._show_item(self._selected_item_id) self._update_static( "#board-status", f"items:{stats.total} | filter:{self._filter or 'all'} | " - f"decisions:{len(self._board.active_decisions())}", + f"decisions:{decisions}", ) def on_data_table_row_highlighted(self, event: DataTable.RowHighlighted) -> None: @@ -2439,7 +2466,12 @@ def _stage_room(self, room_id: str, *, reset_candidate: bool = False) -> tuple: room = self._rooms.store.get_room(room_id) candidates = self._rooms.store.list_candidates(room_id) events = self._rooms.store.events(room_id) - detail = render_room(room, candidates, events) + try: + outcome = outcome_lines(self._rooms.store.outcome(room_id)) + except _ROOM_POLL_ERRORS as exc: + # Local: a damaged judgment table must not hide the room. + outcome = [f"Outcome: unavailable ({printable_name(str(exc))})"] + detail = render_room(room, candidates, events, outcome) ids = [candidate["id"] for candidate in candidates] entries = [ ( @@ -2674,7 +2706,7 @@ def _apply_evidence( generation: int, room_id: str, root: str, - result: tuple[RoomComparison, int, frozenset] | None, + result: tuple[RoomComparison, int, frozenset, int | None] | None, error: str | None, built_at: float, ) -> None: @@ -2692,13 +2724,14 @@ def _apply_evidence( and (room_id, root) == current ): if result is not None: - comparison, revision, candidate_ids = result + comparison, revision, candidate_ids, judgments = result self._evidence = { "root": root, "room_id": room_id, "comparison": comparison, "revision": revision, "candidate_ids": candidate_ids, + "judgment_count": judgments, "as_of": time.strftime("%H:%M:%S", time.localtime(built_at)), } self._evidence_error = None @@ -2745,6 +2778,7 @@ def _stage_evidence_view( ] target_round = max(numbers) if numbers else None changed = False + freshness_error = None if evidence is not None and room_id: room = self._rooms.store.get_room(room_id) current_ids = frozenset( @@ -2754,6 +2788,17 @@ def _stage_evidence_view( int(room["revision"]) != evidence["revision"] or current_ids != evidence["candidate_ids"] ) + if not changed and evidence.get("judgment_count") is not None: + # A verdict (from any surface) changes neither of the above. + # Degraded locally: an unreadable judgments table must not take + # the whole staged room update down with it. + try: + changed = ( + self._rooms.store.judgment_count(room_id) + != evidence["judgment_count"] + ) + except _ROOM_POLL_ERRORS as exc: + freshness_error = str(exc) or type(exc).__name__ key = ( "evidence", room_id, @@ -2762,6 +2807,7 @@ def _stage_evidence_view( self._evidence_build_id, error, changed, + freshness_error, building, target_round, stored_rounds, @@ -2779,6 +2825,11 @@ def _stage_evidence_view( return key, "\n".join(lines), view, pages, False if changed: lines.append("room changed since this snapshot — press r") + if freshness_error is not None: + lines.append( + f"cannot tell whether a verdict changed since this snapshot " + f"({printable_name(freshness_error)}) — press r" + ) comparison: RoomComparison = evidence["comparison"] as_of = evidence["as_of"] found = find_row(comparison, candidate_id) if candidate_id else None @@ -2910,39 +2961,109 @@ def _apply_room_create(self, fields: dict | bool | None) -> None: self._selected_room_id = room["id"] self._refresh_rooms() - def _confirm_room_round(self) -> None: + def _room_target(self) -> tuple[str, str] | None: + """The selected room and the store it lives in, captured when a dialog + opens: its callback acts on these, never on whatever is selected when + the person confirms (a poll can move the selection meanwhile).""" if self._rooms is None or not self._selected_room_id: self.notify("select a room", severity="warning") + return None + return self._selected_room_id, str(self._rooms.store.root) + + def _room_store_is(self, store_root: str) -> bool: + """Whether the Rooms store shown now is the one a dialog was opened on; + warns when it is not (the TUI rebound to another project).""" + if self._rooms is not None and str(self._rooms.store.root) == store_root: + return True + self.notify( + "the Rooms store changed while the dialog was open; nothing was done", + severity="warning", + ) + return False + + def _round_lessons(self, room_id: str, room: dict) -> RoundLessonsView: + """The approved lessons a TUI round can send (#174), read once on the + Textual thread when the dialog opens (the board connection is + thread-affine). Advisory like the CLI's: an unreadable board means the + round runs without lessons, said in the dialog.""" + history: dict[str, bool] = {} + for lineage in ("continue-own", "branch-from"): + # Best-effort: a lineage that cannot be checked gets no note. + with contextlib.suppress(*_ROOM_POLL_ERRORS, OSError): + history[lineage] = self._rooms.resumes_with_lesson_history( + room_id, lineage=lineage + ) + creation_prompt = str(room["prompt"]) + + def fits(prompt: str | None, texts: list[str]) -> bool: + return round_lessons_fit( + prompt if prompt is not None else creation_prompt, texts + ) + + if not self._config.board.enabled: + return RoundLessonsView(history=history, fits=fits) + if self._board is None: + return RoundLessonsView( + unavailable=self._board_error or "the board did not open", + history=history, + fits=fits, + ) + try: + lessons = tuple(self._board.project_lessons(self._rooms.project_id)) + except (BoardError, sqlite3.Error) as exc: + return RoundLessonsView(unavailable=str(exc), history=history, fits=fits) + return RoundLessonsView(lessons=lessons, history=history, fits=fits) + + def _confirm_room_round(self) -> None: + target = self._room_target() + if target is None: return + room_id, store_root = target try: - room = self._rooms.store.get_room(self._selected_room_id) + room = self._rooms.store.get_room(room_id) # Prefill with the PREVIOUS round's prompt (creation prompt on a # fresh room) — the modal is where per-round instructions land now. - rounds = self._rooms.store.list_rounds(self._selected_room_id) + rounds = self._rooms.store.list_rounds(room_id) default_prompt = rounds[-1]["prompt"] if rounds else room["prompt"] eligible = [ row - for row in self._rooms.store.list_candidates(self._selected_room_id) + for row in self._rooms.store.list_candidates(room_id) if row.get("eligible") ] - except _ROOM_POLL_ERRORS as exc: + lessons = self._round_lessons(room_id, room) + except (*_ROOM_POLL_ERRORS, OSError) as exc: self.notify(str(exc), severity="error") return self.push_screen( - RoomRoundModal(room, default_prompt, eligible), - self._apply_room_round, + RoomRoundModal(room, default_prompt, eligible, lessons), + lambda fields, room_id=room_id, store_root=store_root, shown=lessons.lessons: ( + self._apply_room_round(fields, room_id, store_root, shown) + ), ) - def _apply_room_round(self, fields: dict | bool | None) -> None: - if not isinstance(fields, dict) or self._rooms is None or not self._selected_room_id: + def _apply_room_round( + self, + fields: dict | bool | None, + room_id: str, + store_root: str, + shown: tuple = (), + ) -> None: + if not isinstance(fields, dict) or not self._room_store_is(store_root): return - # Preserve the prompt VERBATIM (the raw-user-text contract the stored - # rounds.prompt keeps, and what --prompt-file does): strip only to - # decide whether an all-whitespace entry means "no prompt". - raw_prompt = str(fields.get("prompt") or "") - prompt = raw_prompt if raw_prompt.strip() else None + assert self._rooms is not None + # The same rule the dialog's fit check used (round_prompt): the text + # verbatim, or None to reuse the room creation prompt. + prompt = round_prompt(fields.get("prompt")) lineage = str(fields.get("lineage") or "continue-own") base_candidate = fields.get("base_candidate") + # Exactly the lessons the dialog showed as going in, from the snapshot + # it was built from — never a second board read. + by_id = {lesson.id: lesson for lesson in shown} + chosen = [str(lesson_id) for lesson_id in fields.get("lessons") or ()] + if any(lesson_id not in by_id for lesson_id in chosen): + self.notify("a chosen lesson was not in the dialog; nothing was run", severity="error") + return + lessons = tuple(RoundLesson(lesson_id, by_id[lesson_id].text) for lesson_id in chosen) if prompt is None: self.notify( "no round prompt — reusing the room creation prompt", @@ -2950,49 +3071,218 @@ def _apply_room_round(self, fields: dict | bool | None) -> None: ) try: self._rooms.preflight_round( - self._selected_room_id, + room_id, prompt, lineage=lineage, base_candidate_id=base_candidate, ) pid = launch_room_worker( self._project_root(), - self._selected_room_id, + room_id, prompt, lineage=lineage, base_candidate_id=base_candidate, resolved_config=self._rooms.config, config_base=self._toml_path.parent, + lessons=lessons, ) - self._rooms.store.record_event( - self._selected_room_id, "worker.launched", {"pid": pid} - ) + self._rooms.store.record_event(room_id, "worker.launched", {"pid": pid}) # ProjectError covers a repository that disappeared under us: git_root # reports an unusable path that way rather than letting subprocess's # OSError through, and an unguarded raise here kills the whole app. - except (RoomError, ArtifactError, WorkspaceError, ProjectError, OSError) as exc: + except ( + RoomError, ArtifactError, WorkspaceError, ProjectError, OSError, sqlite3.Error + ) as exc: + self.notify(str(exc), severity="error") + return + sent = f", {len(lessons)} approved lesson(s)" if lessons else "" + self.notify(f"room worker started (pid {pid}{sent})") + self._refresh_rooms() + + def action_judge(self) -> None: + if self._active() == "rooms-tab": + self._confirm_room_judge() + + def action_promote(self) -> None: + if self._active() == "rooms-tab": + self._confirm_lesson_promote() + + def _confirm_room_judge(self) -> None: + target = self._room_target() + if target is None: + return + room_id, store_root = target + try: + outcome = self._rooms.store.outcome(room_id) + if outcome.status not in ("applied_unreviewed", *JUDGMENT_VERDICTS): + reason = f" ({outcome.reason})" if outcome.reason else "" + self.notify( + f"room outcome is {outcome.status}{reason}; only an applied " + "change can be accepted or rejected", + severity="warning", + ) + return + row = self._rooms.store.get_candidate(str(outcome.candidate_id)) + except (*_ROOM_POLL_ERRORS, OSError) as exc: + self.notify(str(exc), severity="error") + return + p = printable_name + current = ( + "not reviewed yet" + if outcome.judgment is None + else f"{outcome.status} (judgment {outcome.judgment['sequence']} of " + f"{outcome.judgment_count}; a new verdict supersedes it, history is kept)" + ) + detail = ( + f"Room: {p(room_id)}\n" + f"Applied candidate: {p(str(outcome.candidate_id))}\n" + f"Applied as run: {p(str(outcome.applied_run_id))}\n" + f"Patch sha256: {p(str(row['patch_sha256']))}\n" + f"Current verdict: {current}\n\n" + "The verdict is bound to this patch digest. Nothing is shared to memory." + ) + self.push_screen( + RoomJudgeModal(detail), + lambda fields, room_id=room_id, store_root=store_root, digest=row[ + "patch_sha256" + ]: self._apply_room_judge(fields, room_id, store_root, digest), + ) + + def _apply_room_judge( + self, fields: dict | bool | None, room_id: str, store_root: str, digest: str + ) -> None: + if not isinstance(fields, dict) or not self._room_store_is(store_root): + return + assert self._rooms is not None + try: + # The rationale goes as typed: the store decides what an empty or + # padded one means, exactly as for `room judge --rationale`. + record, created = self._rooms.judge( + room_id, + str(fields["verdict"]), + expected_patch_sha256=digest, + rationale=fields.get("rationale"), + surface="tui", + ) + except (*_ROOM_POLL_ERRORS, OSError) as exc: self.notify(str(exc), severity="error") return - self.notify(f"room worker started (pid {pid})") - # The TUI does not show or inject approved lessons yet (#174), but a - # resumed conversation may still hold ones earlier rounds sent. Read - # after the launch and best-effort: the worker is already running, so - # a failed read must not look like a failed launch. - lesson_history = False - with contextlib.suppress(*_ROOM_POLL_ERRORS): - lesson_history = self._rooms.resumes_with_lesson_history( - self._selected_room_id, lineage=lineage - ) - if lesson_history: + if created: self.notify( - "approved lessons are not sent from the TUI; resumed conversations " - "may still hold lessons sent in earlier rounds. The fresh lineage, " - "or `syncmill room round --fresh-session`, starts them without " - "that history", - severity="warning", + f"recorded {record['verdict']} for candidate " + f"{printable_name(str(record['candidate_id'])[:8])}" + ) + else: + self.notify( + f"already recorded: {record['verdict']} (judgment " + f"{record['sequence']}); nothing changed" ) self._refresh_rooms() + def _confirm_lesson_promote(self) -> None: + target = self._room_target() + if target is None: + return + room_id, store_root = target + if self._board is None: + self.notify( + ( + f"promoting a lesson needs the board, which did not open: " + f"{self._board_error or 'unknown error'}" + ) + if self._config.board.enabled + else "promoting a lesson needs the board, where lessons are kept " + "as decisions: set SYNCMILL_BOARD__ENABLED=true", + severity="error", + ) + return + try: + project = self._rooms.project_id + outcome = self._rooms.store.outcome(room_id) + judgment = outcome.judgment + if outcome.status not in JUDGMENT_VERDICTS or judgment is None: + reason = f" ({outcome.reason})" if outcome.reason else "" + self.notify( + f"room outcome is {outcome.status}{reason}; record a verdict " + "with j before promoting a lesson", + severity="warning", + ) + return + # active_decisions(project_id=) also returns unbound decisions, + # which the board refuses to let a project lesson replace. + supersedable = [ + decision + for decision in self._board.active_decisions(project_id=project) + if decision.project_id == project + ] + except (*_ROOM_POLL_ERRORS, BoardError, OSError) as exc: + self.notify(str(exc), severity="error") + return + p = printable_name + detail = ( + "The lesson is recorded with where it came from:\n" + f"Room: {p(room_id)}\n" + f"Candidate: {p(str(judgment['candidate_id']))}\n" + f"Verdict: {p(str(judgment['verdict']))} by a human (judgment " + f"{p(str(judgment['judgment_id']))})\n" + f"Patch sha256: {p(str(judgment['patch_sha256']))}\n" + f"Applied as run: {p(str(judgment['applied_run_id']))}\n" + f"Project: {p(project)}\n\n" + "Runs of this project show the lessons they use before starting, and " + "each can be left out per run." + ) + choices = [ + ( + f"{d.id[:8]} [{d.scope or 'whole project'}] {d.text}", + d.id, + d.scope, + ) + for d in supersedable + ] + self.push_screen( + LessonPromoteModal(detail, choices), + lambda fields, room_id=room_id, store_root=store_root, judgment_id=judgment[ + "judgment_id" + ]: self._apply_lesson_promote(fields, room_id, store_root, judgment_id), + ) + + def _apply_lesson_promote( + self, fields: dict | bool | None, room_id: str, store_root: str, judgment_id: str + ) -> None: + if not isinstance(fields, dict) or not self._room_store_is(store_root): + return + assert self._rooms is not None + if self._board is None: + self.notify("the board is no longer open; nothing was promoted", severity="error") + return + try: + # Text and scope go as typed, as for `room promote`; the + # coordinator and the board validate both. + decision, created = self._rooms.promote_lesson( + room_id, + self._board, + str(fields.get("lesson") or ""), + scope=str(fields.get("scope") or ""), + supersedes=fields.get("supersedes"), + expected_judgment_id=judgment_id, + ) + except (*_ROOM_POLL_ERRORS, BoardError, OSError) as exc: + self.notify(str(exc), severity="error") + return + where = ( + f"goal {printable_name(decision.scope)}" if decision.scope else "the whole project" + ) + if created: + self.notify(f"promoted lesson {decision.id[:8]} for {where}") + else: + state = ( + f"superseded by {decision.superseded_by[:8]}" + if decision.superseded_by + else "active" + ) + self.notify(f"already promoted as lesson {decision.id[:8]} ({state}); nothing changed") + self._refresh_board() + def _confirm_candidate_selection(self) -> None: if ( self._rooms is None diff --git a/src/syncmill/tui_collaboration.py b/src/syncmill/tui_collaboration.py index 88f5b85..c2e5a81 100644 --- a/src/syncmill/tui_collaboration.py +++ b/src/syncmill/tui_collaboration.py @@ -2,7 +2,9 @@ from __future__ import annotations +import dataclasses import re +from collections.abc import Callable from textual.app import ComposeResult from textual.containers import Horizontal, Vertical, VerticalScroll @@ -10,6 +12,7 @@ from textual.widgets import Button, Checkbox, Input, Select, TextArea from syncmill.artifacts import printable_name, printable_text +from syncmill.board import Decision, lesson_source, select_lessons from syncmill.config import SyncmillConfig from syncmill.room_compare import ( CandidateRow, @@ -26,7 +29,7 @@ usage_label, ) from syncmill.session import interactive_agents -from syncmill.tui_render import PlainLabel, PlainStatic, plain_select +from syncmill.tui_render import PlainLabel, PlainStatic, plain, plain_select class _CollabModal(ModalScreen[dict | bool | None]): @@ -108,6 +111,43 @@ def on_button_pressed(self, event: Button.Pressed) -> None: ) +def round_prompt(raw: object) -> str | None: + """The round prompt a submitted TextArea value means: the text VERBATIM + (the raw-user-text contract the stored rounds.prompt keeps), or None — + reuse the room creation prompt — when it is empty or all whitespace. The + lesson fit check and the launch both use this, so they cannot disagree.""" + text = str(raw or "") + return text if text.strip() else None + + +@dataclasses.dataclass(frozen=True) +class RoundLessonsView: + """What the round dialog shows about approved lessons (#174), read once + when the dialog opens. + + ``lessons`` are the project's active project-wide lessons, oldest first; + the dialog re-runs the CLI's ``select_lessons`` on every change, so a + skipped lesson frees budget exactly as ``--skip-lesson`` does. + ``unavailable`` is why lessons could not be read (the round runs without + them). ``history`` maps a lineage to whether a round launched with it now + would resume a conversation that earlier rounds sent lessons to (a lineage + missing from it could not be checked). ``fits(prompt, texts)`` is the + coordinator's own fit check, with a None prompt meaning the creation + prompt.""" + + lessons: tuple[Decision, ...] = () + unavailable: str | None = None + history: dict[str, bool] = dataclasses.field(default_factory=dict) + fits: Callable[[str | None, list[str]], bool] = lambda prompt, texts: True + + +_HISTORY_NOTE = ( + "Resumed conversations may still hold lessons sent in earlier rounds: " + "unchecking a lesson stops sending it, it does not take back what a " + "conversation was already sent. The fresh lineage starts without that history." +) + + class RoomRoundModal(_CollabModal): """Per-round prompt + lineage input for the Rooms tab. @@ -124,11 +164,18 @@ class RoomRoundModal(_CollabModal): ("branch-from (seed all from one candidate)", "branch-from"), ] - def __init__(self, room: dict, default_prompt: str, eligible: list[dict]): + def __init__( + self, + room: dict, + default_prompt: str, + eligible: list[dict], + lessons: RoundLessonsView | None = None, + ): super().__init__() self._room = room self._default_prompt = default_prompt self._eligible = eligible + self._lessons = lessons if lessons is not None else RoundLessonsView() def compose(self) -> ComposeResult: members = self._room.get("members", []) @@ -137,38 +184,121 @@ def compose(self) -> ComposeResult: f"Run round {int(self._room.get('round_count') or 0) + 1} — " f"{len(members)} provider call(s)" ) - area = TextArea(id="round-prompt", language=None) - # Escaped prefill: new prompts cannot contain controls (the - # coordinator rejects them), but a row stored before that - # validation could — never hand raw sequences to the terminal. - area.text = escape_controls(self._default_prompt) - yield area - yield plain_select( - self._LINEAGES, - value="continue-own", - allow_blank=False, - id="round-lineage", - ) - if self._eligible: + # Everything but the confirmation and the buttons scrolls, so they + # stay reachable at 80x24 with a full lesson list. + with VerticalScroll(id="round-body"): + area = TextArea(id="round-prompt", language=None) + # Escaped prefill: new prompts cannot contain controls (the + # coordinator rejects them), but a row stored before that + # validation could — never hand raw sequences to the terminal. + area.text = escape_controls(self._default_prompt) + yield area yield plain_select( - [ - (f"{row['id'][:8]} {row['member_id']}", row["id"]) - for row in self._eligible - ], - allow_blank=True, - prompt="base candidate (branch-from only)", - id="round-base", + self._LINEAGES, + value="continue-own", + allow_blank=False, + id="round-lineage", + ) + if self._eligible: + yield plain_select( + [ + (f"{row['id'][:8]} {row['member_id']}", row["id"]) + for row in self._eligible + ], + allow_blank=True, + prompt="base candidate (branch-from only)", + id="round-base", + ) + if self._lessons.unavailable is not None: + yield PlainStatic( + f"Lessons unavailable ({printable_name(self._lessons.unavailable)}); " + "this round runs without them.", + classes="warning", + ) + if self._lessons.lessons: + yield PlainLabel( + "Approved project lessons (sent to every member's prompt " + "this round; uncheck to leave one out of this round):" + ) + for index, lesson in enumerate(self._lessons.lessons): + # plain(): a str label is parsed as markup by Checkbox. + yield Checkbox( + plain(f"{lesson.id[:8]} {lesson.text}"), + value=True, + id=f"round-lesson-{index}", + ) + source = lesson_source(lesson) + if source: + yield PlainStatic(f" {source}") + yield PlainStatic( + "Replace a lesson with p on a judged room, or " + '`syncmill board decide --supersedes ID ""`.' + ) + yield PlainStatic("", id="round-lessons-status") + yield PlainStatic( + "The detached worker keeps running if the TUI closes. An empty " + "prompt reuses the room creation prompt.", + classes="warning", ) - yield PlainStatic( - "The detached worker keeps running if the TUI closes. An empty " - "prompt reuses the room creation prompt.", - classes="warning", - ) yield Checkbox("I reviewed this exact action", id="confirmed") with Horizontal(classes="form-buttons"): yield Button("Run round", variant="warning", id="apply") yield Button("Cancel", id="cancel") + def on_mount(self) -> None: + self._update_lessons_status() + + def _lesson_choice(self) -> tuple[list[Decision], list[Decision], list[Decision]]: + """``(used, skipped, over_budget)`` for the boxes as they stand: the + CLI's selection, with an unchecked box as ``--skip-lesson``.""" + skip = frozenset( + lesson.id + for index, lesson in enumerate(self._lessons.lessons) + if not self.query_one(f"#round-lesson-{index}", Checkbox).value + ) + return select_lessons(list(self._lessons.lessons), skip=skip) + + def _lineage(self) -> object: + return self.query_one("#round-lineage", Select).value + + def _status_text(self) -> tuple[str, bool]: + """The lessons status block, and whether the chosen lessons fit.""" + lines: list[str] = [] + fits = True + if self._lessons.lessons: + used, skipped, over = self._lesson_choice() + prompt = round_prompt(self.query_one("#round-prompt", TextArea).text) + fits = not used or self._lessons.fits(prompt, [d.text for d in used]) + lines.append(f"Lessons this round: {len(used)} of {len(self._lessons.lessons)}") + lines.extend(f" left out: {d.id[:8]} (unchecked)" for d in skipped) + lines.extend( + f" left out: {d.id[:8]} (over the per-run lesson budget)" for d in over + ) + if not fits: + lines.append( + " this round's prompt leaves no room for the checked lessons: " + "uncheck some or shorten the prompt" + ) + if self._lessons.history.get(str(self._lineage())): + lines.append(_HISTORY_NOTE) + return "\n".join(lines), fits + + def _update_lessons_status(self) -> None: + text, _ = self._status_text() + self.query_one("#round-lessons-status", PlainStatic).update(text) + + def on_checkbox_changed(self, event: Checkbox.Changed) -> None: + if (event.checkbox.id or "").startswith("round-lesson-"): + self._update_lessons_status() + + def on_select_changed(self, event: Select.Changed) -> None: + if event.select.id == "round-lineage": + self._update_lessons_status() + + def on_text_area_changed(self, event: TextArea.Changed) -> None: + if event.text_area.id == "round-prompt" and self._lessons.lessons: + self._update_lessons_status() + def on_button_pressed(self, event: Button.Pressed) -> None: if event.button.id != "apply": self.dismiss(None) @@ -176,7 +306,20 @@ def on_button_pressed(self, event: Button.Pressed) -> None: if not self.query_one("#confirmed", Checkbox).value: self.app.notify("confirm the action", severity="warning") return - lineage = self.query_one("#round-lineage", Select).value + used: list[Decision] = [] + if self._lessons.lessons: + used, _, _ = self._lesson_choice() + _, fits = self._status_text() + if not fits: + # Refused, not dropped: the dialog showed these lessons as going + # in, so running without them would not be what was confirmed. + self.app.notify( + "this round's prompt leaves no room for the checked lessons; " + "uncheck some or shorten the prompt", + severity="warning", + ) + return + lineage = self._lineage() base = None if self._eligible: value = self.query_one("#round-base", Select).value @@ -195,6 +338,7 @@ def on_button_pressed(self, event: Button.Pressed) -> None: "prompt": self.query_one("#round-prompt", TextArea).text, "lineage": lineage, "base_candidate": base, + "lessons": [lesson.id for lesson in used], } ) @@ -262,6 +406,121 @@ def on_button_pressed(self, event: Button.Pressed) -> None: ) +class RoomJudgeModal(_CollabModal): + """Record a human verdict on a room's applied change (#174), with an + optional rationale. The verdict has no default, so a stray keypress cannot + record one. Dumb like the other modals: the store validates the text.""" + + _VERDICTS = [("accepted", "accepted"), ("rejected", "rejected")] + + def __init__(self, detail: str): + super().__init__() + self._detail = detail + + def compose(self) -> ComposeResult: + with Vertical(classes="wide-dialog"): + yield PlainLabel("Record your verdict on the applied change") + with VerticalScroll(id="judge-body"): + yield PlainStatic(self._detail) + yield plain_select( + self._VERDICTS, + allow_blank=True, + prompt="verdict", + id="judge-verdict", + ) + yield PlainLabel("Rationale (optional) — why you accept or reject it:") + yield TextArea(id="judge-rationale", language=None) + yield Checkbox("I reviewed this exact change", id="confirmed") + with Horizontal(classes="form-buttons"): + yield Button("Record", variant="warning", id="apply") + yield Button("Cancel", id="cancel") + + def on_button_pressed(self, event: Button.Pressed) -> None: + if event.button.id != "apply": + self.dismiss(None) + return + verdict = self.query_one("#judge-verdict", Select).value + if verdict not in ("accepted", "rejected"): + self.app.notify("choose accepted or rejected", severity="warning") + return + if not self.query_one("#confirmed", Checkbox).value: + self.app.notify("confirm the action", severity="warning") + return + self.dismiss( + { + "verdict": verdict, + "rationale": self.query_one("#judge-rationale", TextArea).text, + } + ) + + +class LessonPromoteModal(_CollabModal): + """Promote a lesson the person writes from a judged room (#174). + + ``supersedable`` are ``(label, decision id, scope)`` for this project's + active lessons. A replacement must have the same scope (the board refuses + otherwise); the dialog checks that before closing so the error sits next + to the fields.""" + + def __init__(self, detail: str, supersedable: list[tuple[str, str, str]]): + super().__init__() + self._detail = detail + self._supersedable = supersedable + + def compose(self) -> ComposeResult: + with Vertical(classes="wide-dialog"): + yield PlainLabel("Promote a lesson for reuse in this project's runs") + with VerticalScroll(id="promote-body"): + yield PlainStatic(self._detail) + yield PlainLabel("Lesson (your own words, at most 1000 bytes):") + yield TextArea(id="promote-lesson", language=None) + yield PlainLabel("Scope: a board goal, or blank for the whole project") + yield Input(placeholder="blank = whole project", id="promote-scope") + yield plain_select( + [ + ("(replaces nothing)", ""), + *((label, decision_id) for label, decision_id, _ in self._supersedable), + ], + value="", + allow_blank=False, + id="promote-supersedes", + ) + yield Checkbox("I reviewed this lesson and its source", id="confirmed") + with Horizontal(classes="form-buttons"): + yield Button("Promote", variant="warning", id="apply") + yield Button("Cancel", id="cancel") + + def on_button_pressed(self, event: Button.Pressed) -> None: + if event.button.id != "apply": + self.dismiss(None) + return + if not self.query_one("#confirmed", Checkbox).value: + self.app.notify("confirm the action", severity="warning") + return + scope = self.query_one("#promote-scope", Input).value + value = self.query_one("#promote-supersedes", Select).value + supersedes = value if isinstance(value, str) and value else None + if supersedes is not None: + target_scope = next( + (scope_ for _, decision_id, scope_ in self._supersedable + if decision_id == supersedes), + None, + ) + if target_scope != scope: + self.app.notify( + "a lesson can only replace one of the same scope", + severity="warning", + ) + return + self.dismiss( + { + "lesson": self.query_one("#promote-lesson", TextArea).text, + "scope": scope, + "supersedes": supersedes, + } + ) + + def _patch_is_binary(head: bytes) -> bool: """Whether a patch head carries binary content. Derived — no stored flag: patches are captured with `git diff --binary`, whose binary hunks are @@ -295,7 +554,12 @@ def render_session(value: dict) -> str: ) -def render_room(room: dict, candidates: list[dict], events: list[dict]) -> str: +def render_room( + room: dict, + candidates: list[dict], + events: list[dict], + outcome: list[str] | tuple[str, ...] = (), +) -> str: members = ", ".join(_p(member["member_id"]) for member in room.get("members", [])) tail = events[-5:] lines = [ @@ -306,11 +570,12 @@ def render_room(room: dict, candidates: list[dict], events: list[dict]) -> str: f"Selected: {_p(room.get('selected_candidate_id') or '-')}", f"Apply run: {_p(room.get('apply_run_id') or '-')}", f"Applied run: {_p(room.get('applied_run_id') or '-')}", + *outcome, "", "Recent events:", *(f"{_p(event['sequence'])} {_p(event['kind'])}" for event in tail), "", - "x run round | e select candidate | g apply selected", + "x run round | e select candidate | g apply selected | j verdict | p promote", ] return "\n".join(lines) diff --git a/tests/test_tui.py b/tests/test_tui.py index 4ba921d..ad3af23 100644 --- a/tests/test_tui.py +++ b/tests/test_tui.py @@ -1197,7 +1197,7 @@ def build(store_root, output_max, repo_root, room_id, now): calls.append(room_id) comparison = _comparison(rows, room_id=room_id, **kwargs) ids = frozenset(r.candidate_id for r in rows) - return comparison, comparison.revision, ids + return comparison, comparison.revision, ids, None monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -3019,11 +3019,16 @@ class _FakeRoundRooms: def __init__(self, *, rounds=None, candidates=None): self.config = object() + self.project_id = "proj-1" self.preflights: list[tuple] = [] self.events: list[tuple] = [] + self.history_asked: list[tuple] = [] + self.history: dict = {} outer = self class _Store: + root = "/fake-room-store" + @staticmethod def get_room(room_id): return { @@ -3051,10 +3056,20 @@ def preflight_round(self, room_id, prompt, *, lineage, base_candidate_id): self.preflights.append((room_id, prompt, lineage, base_candidate_id)) return {} + def resumes_with_lesson_history(self, room_id, *, lineage): + self.history_asked.append((room_id, lineage)) + value = self.history.get(lineage, False) + if isinstance(value, Exception): + raise value + return value + def close(self): return None +_ROUND_STORE = "/fake-room-store" + + def test_room_round_modal_prefills_previous_round_prompt(tmp_path, git_repo, monkeypatch): async def scenario(): app = _room_apply_app(tmp_path, git_repo) @@ -3101,7 +3116,8 @@ def fake_launch(root, room_id, prompt=None, **kwargs): app._apply_room_round( {"prompt": "edited round prompt", "lineage": "fresh", - "base_candidate": None} + "base_candidate": None}, + "room-a", _ROUND_STORE, ) await pilot.pause() assert rooms.preflights == [ @@ -3114,66 +3130,61 @@ def fake_launch(root, room_id, prompt=None, **kwargs): # An EMPTY prompt falls back (None) with a visible warning. app._apply_room_round( - {"prompt": " ", "lineage": "continue-own", "base_candidate": None} + {"prompt": " ", "lineage": "continue-own", "base_candidate": None}, + "room-a", _ROUND_STORE, ) await pilot.pause() assert launched["prompt"] is None + assert launched["lessons"] == () # A bool (the legacy confirm shape) or None is ignored outright. before = len(rooms.preflights) - app._apply_room_round(True) - app._apply_room_round(None) + app._apply_room_round(True, "room-a", _ROUND_STORE) + app._apply_room_round(None, "room-a", _ROUND_STORE) assert len(rooms.preflights) == before drive(scenario()) -@pytest.mark.parametrize("history", [True, False, "raises"]) -def test_room_round_launch_notes_lessons_a_resumed_conversation_may_hold( - tmp_path, git_repo, monkeypatch, history +def test_room_round_reads_lesson_history_per_lineage_before_confirmation( + tmp_path, git_repo, monkeypatch ): - """The TUI sends no approved lessons yet (#174), but warns when a resumed - conversation may hold ones earlier rounds sent. The read comes after the - launch, so a failed read never turns a started worker into an error.""" + """The history note (#174) is decided when the dialog opens, per lineage + that can resume (fresh never does), so the person can still pick fresh; + a lineage whose check fails gets no note. Nothing is said after launch.""" + from syncmill.rooms import RoomError async def scenario(): app = _room_apply_app(tmp_path, git_repo) async with app.run_test() as pilot: await pilot.pause() rooms = _FakeRoundRooms() - asked: list = [] - - def resumes(room_id, **kwargs): - asked.append((room_id, kwargs)) - if history == "raises": - from syncmill.rooms import RoomError - - raise RoomError("store hiccup") - return history - - rooms.resumes_with_lesson_history = resumes + rooms.history = {"continue-own": True, "branch-from": RoomError("hiccup")} app._rooms = rooms app._selected_room_id = "room-a" - launched: dict = {} + staged: dict = {} monkeypatch.setattr( - "syncmill.tui.launch_room_worker", - lambda root, room_id, prompt=None, **kwargs: launched.update(kwargs) or 4242, + app, "push_screen", + lambda screen, callback: staged.update(screen=screen, cb=callback), ) - monkeypatch.setattr(app, "_refresh_rooms", lambda: None) + app._confirm_room_round() + view = staged["screen"]._lessons + assert view.history == {"continue-own": True} + assert rooms.history_asked == [ + ("room-a", "continue-own"), ("room-a", "branch-from") + ] + notes: list = [] monkeypatch.setattr( app, "notify", lambda message, **kwargs: notes.append((message, kwargs)) ) - app._apply_room_round( - {"prompt": "go", "lineage": "continue-own", "base_candidate": None} + monkeypatch.setattr( + "syncmill.tui.launch_room_worker", lambda *a, **k: 4242 ) + monkeypatch.setattr(app, "_refresh_rooms", lambda: None) + staged["cb"]({"prompt": "go", "lineage": "continue-own", + "base_candidate": None, "lessons": []}) await pilot.pause() - assert "lessons" not in launched - assert asked == [("room-a", {"lineage": "continue-own"})] - assert notes[0][0] == "room worker started (pid 4242)" - warned = [m for m, kw in notes if "may still hold lessons" in m] - assert len(warned) == (1 if history is True else 0) - assert all("--fresh-session" in m and "fresh lineage" in m for m in warned) - assert not any(kw.get("severity") == "error" for _, kw in notes) + assert [m for m, _ in notes] == ["room worker started (pid 4242)"] drive(scenario()) @@ -3209,7 +3220,7 @@ async def scenario(): await pilot.pause() assert results == [{ "prompt": "prefill", "lineage": "branch-from", - "base_candidate": "cand-12345678", + "base_candidate": "cand-12345678", "lessons": [], }] drive(scenario()) @@ -3274,7 +3285,7 @@ async def scenario(): app._apply_room_round({ "prompt": " keep\n my indentation \n", "lineage": "continue-own", "base_candidate": None, - }) + }, "room-a", _ROUND_STORE) await pilot.pause() assert launched["prompt"] == " keep\n my indentation \n" @@ -3837,7 +3848,7 @@ def unavailable(repo_root): monkeypatch.setattr("syncmill.tui.repository_fingerprint", unavailable) room, *_ = _seeded_room(tmp_path) try: - comparison, _, _ = build_room_evidence( + comparison, *_ = build_room_evidence( room.store.root, 10**6, tmp_path, room.id, 0.0 ) finally: @@ -3910,7 +3921,7 @@ def build(store_root, output_max, repo_root, room_id, now): gate.wait(5) running["now"] -= 1 rows = [_candidate_row(candidate_id="cand-ok", member_id=f"build{running['builds']}")] - return _comparison(rows), 1, frozenset({"cand-ok"}) + return _comparison(rows), 1, frozenset({"cand-ok"}), None monkeypatch.setattr("syncmill.tui.build_room_evidence", build) rendered: list[str] = [] @@ -3960,7 +3971,7 @@ def test_a_store_root_change_retires_an_in_flight_build(tmp_path, git_repo, monk def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) - return _comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}) + return _comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), None monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4036,7 +4047,7 @@ async def scenario(): _fake_evidence(monkeypatch, [_candidate_row(candidate_id="cand-ok")]) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (_comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"})), + lambda *a: (_comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), None), ) await pilot.press("7") await pilot.pause() @@ -4126,7 +4137,7 @@ async def scenario(): ) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (comparison, 1, frozenset()), + lambda *a: (comparison, 1, frozenset(), None), ) await pilot.press("7") await pilot.pause() @@ -4352,7 +4363,7 @@ def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) rows = [_candidate_row(candidate_id="cand-ok", member_id=f"built-for-{room_id}")] - return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}) + return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), None monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4448,7 +4459,7 @@ def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) rows = [_candidate_row(candidate_id="cand-ok", member_id=f"built-for-{room_id}")] - return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}) + return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), None monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4609,7 +4620,7 @@ async def scenario(): monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (_comparison([]), 1, frozenset()), + lambda *a: (_comparison([]), 1, frozenset(), None), ) await pilot.press("7") await pilot.pause() @@ -4650,7 +4661,7 @@ async def scenario(): app._rooms = _FakeRooms(store=_Damaged()) monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( - "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset()) + "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), None) ) await pilot.press("7") await pilot.pause() @@ -4766,7 +4777,7 @@ async def scenario(): app._rooms = _FakeRooms(store=_OnlyDamaged()) monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( - "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset()) + "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), None) ) await pilot.press("7") await pilot.pause() diff --git a/tests/test_tui_render.py b/tests/test_tui_render.py index 7cd7333..ec9c440 100644 --- a/tests/test_tui_render.py +++ b/tests/test_tui_render.py @@ -281,7 +281,9 @@ def markup_sinks(source: str, path: str) -> list[str]: ``plain_select`` exist for that); an ``OptionList`` without ``markup=False``; a DataTable row or column not wrapped in ``plain``; ``notify`` with ``markup=True`` or called on something other than the app - (``Widget.notify`` passes ``markup=True`` itself).""" + (``Widget.notify`` passes ``markup=True`` itself); a ``Checkbox`` whose + label is neither a string literal nor ``plain(...)`` (Textual parses a + ``str`` label as markup, so stored text must arrive as ``Text``).""" tree = ast.parse(source) found = [] @@ -307,6 +309,13 @@ def is_plain(node: ast.AST) -> bool: if cls is None and isinstance(func, ast.Name): if func.id in ("Static", "Label", "Select", "Option"): found.append(f"{where} {func.id}(") + if func.id == "Checkbox" and node.args: + label = node.args[0] + literal = isinstance(label, ast.Constant) and isinstance( + label.value, str + ) + if not (literal or is_plain(label)): + found.append(f"{where} Checkbox label without plain") if func.id == "OptionList": value = keyword(node, "markup") if not (isinstance(value, ast.Constant) and value.value is False): @@ -348,6 +357,8 @@ def test_textual_modules_render_no_text_as_markup(): ("OptionList(id='a')\n", "m.py:1 OptionList without markup=False"), ("t.add_row(name)\n", "m.py:1 add_row without plain"), ("t.add_columns('a', 'b')\n", "m.py:1 add_columns without plain"), + ("Checkbox(text, id='a')\n", "m.py:1 Checkbox label without plain"), + ("Checkbox(f'{lesson.text}')\n", "m.py:1 Checkbox label without plain"), ("class S:\n def f(self):\n self.notify('x')\n", "m.py:3 self.notify"), ("class SyncmillTuiApp:\n def f(self):\n self.notify('x', markup=True)\n", "m.py:3 notify markup"), @@ -361,6 +372,7 @@ def test_the_markup_guard_allows_the_plain_forms(): source = ( "PlainStatic('x')\nplain_select([], id='s')\nOptionList(id='a', markup=False)\n" "PlainOption('x', id='a')\n" + "Checkbox('I reviewed this', id='c')\nCheckbox(plain(text), value=True)\n" "t.add_row(*map(plain, cells), key=k)\nt.add_columns(plain('a'))\n" "class DashboardApp:\n def f(self):\n self.notify('x')\n" " super().notify('x', markup=markup)\n" diff --git a/tests/test_tui_room_review.py b/tests/test_tui_room_review.py new file mode 100644 index 0000000..aa8fbd0 --- /dev/null +++ b/tests/test_tui_room_review.py @@ -0,0 +1,799 @@ +"""The TUI Rooms tab's human review actions (#174): recording a verdict (j), +promoting a lesson (p), and choosing approved lessons for a round. Driven +against a real RoomCoordinator and a real Board.""" + +from __future__ import annotations + +import asyncio + +import pytest +from textual.widgets import Button, Checkbox, Input, Select, TextArea + +from test_room_judgments import _applied_room +from test_rooms import WritingAgent, _config +from syncmill.board import Board, BoardError +from syncmill.config import SyncmillConfig +from syncmill.rooms import _ROOM_PROMPT_MAX_BYTES, RoomCoordinator, RoomError +from syncmill.tui import SyncmillTuiApp +from syncmill.tui_collaboration import ( + LessonPromoteModal, + RoomJudgeModal, + RoomRoundModal, + round_prompt, +) + + +def drive(coro): + return asyncio.run(coro) + + +def _app(tmp_path, git_repo, *, board: Board | None, enabled: bool = True, + board_error: str | None = None) -> SyncmillTuiApp: + return SyncmillTuiApp( + SyncmillConfig( + repo_root=str(git_repo), + runs={"root": str(tmp_path / "runs")}, + board={"enabled": enabled, "db_path": str(tmp_path / "board.db")}, + ), + board=board, + board_error=board_error, + toml_path=git_repo / "syncmill.toml", + refresh_seconds=3600, + initial_tab="board", + ) + + +def _coordinator(git_repo, tmp_path) -> RoomCoordinator: + return RoomCoordinator( + _config(tmp_path), git_repo, agents=[WritingAgent("claude"), WritingAgent("codex")] + ) + + +def _notes(app, monkeypatch) -> list[tuple[str, dict]]: + notes: list[tuple[str, dict]] = [] + real = app.notify + + def notify(message, **kwargs): + notes.append((message, kwargs)) + real(message, **kwargs) + + monkeypatch.setattr(app, "notify", notify) + return notes + + +def _captured(app, monkeypatch) -> dict: + staged: dict = {} + monkeypatch.setattr( + app, "push_screen", + lambda screen, callback: staged.update(screen=screen, cb=callback), + ) + return staged + + +async def _press_apply(pilot, modal) -> None: + modal.on_button_pressed(Button.Pressed(modal.query_one("#apply", Button))) + await pilot.pause() + + +# ------------------------------------------------------------------ verdict + + +def test_a_verdict_recorded_in_the_tui_is_bound_and_marked_tui( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, candidate, run_id = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + app._confirm_room_judge() + await pilot.pause() + modal = app.screen + assert isinstance(modal, RoomJudgeModal) + assert candidate["patch_sha256"] in modal._detail + assert run_id in modal._detail and "not reviewed yet" in modal._detail + + # No default verdict: confirming without one records nothing. + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + assert app.screen is modal + assert coordinator.store.outcome(room_id).status == "applied_unreviewed" + + modal.query_one("#judge-verdict", Select).value = "accepted" + modal.query_one("#judge-rationale", TextArea).text = " matches the spec " + await _press_apply(pilot, modal) + outcome = coordinator.store.outcome(room_id) + assert outcome.status == "accepted" + assert outcome.judgment["surface"] == "tui" + assert outcome.judgment["patch_sha256"] == candidate["patch_sha256"] + # Normalized by the store, as for `room judge --rationale`. + assert outcome.judgment["rationale"] == "matches the spec" + assert notes[-1][0] == f"recorded accepted for candidate {candidate['id'][:8]}" + assert "Outcome: ACCEPTED (human)" in app.room_detail_text + + drive(scenario()) + + +@pytest.mark.parametrize("stage", ["created", "ran", "selected"]) +def test_a_verdict_needs_an_applied_change(git_repo, tmp_path, monkeypatch, stage): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + if stage != "created": + coordinator.run_round(room["id"]) + if stage == "selected": + first = coordinator.store.list_candidates(room["id"])[0] + coordinator.select(room["id"], first["id"]) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + notes = _notes(app, monkeypatch) + staged = _captured(app, monkeypatch) + app._confirm_room_judge() + assert staged == {} + assert "only an applied change can be accepted or rejected" in notes[-1][0] + + drive(scenario()) + + +def test_a_repeated_verdict_says_nothing_changed(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room_id, candidate, _ = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + root = str(coordinator.store.root) + fields = {"verdict": "rejected", "rationale": ""} + app._apply_room_judge(fields, room_id, root, candidate["patch_sha256"]) + app._apply_room_judge(fields, room_id, root, candidate["patch_sha256"]) + assert notes[-1][0] == "already recorded: rejected (judgment 1); nothing changed" + assert coordinator.store.outcome(room_id).judgment_count == 1 + + drive(scenario()) + + +def test_a_verdict_is_fenced_on_the_digest_and_store_it_was_reviewed_on( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, candidate, _ = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + fields = {"verdict": "accepted", "rationale": ""} + app._apply_room_judge(fields, room_id, str(coordinator.store.root), "0" * 64) + assert notes[-1][1].get("severity") == "error" + assert "differs from the one you reviewed" in notes[-1][0] + app._apply_room_judge(fields, room_id, "/another/store", candidate["patch_sha256"]) + assert "Rooms store changed while the dialog was open" in notes[-1][0] + assert coordinator.store.outcome(room_id).judgment_count == 0 + + drive(scenario()) + + +def test_a_verdict_dialog_targets_the_room_it_was_opened_on( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, candidate, _ = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + staged = _captured(app, monkeypatch) + app._confirm_room_judge() + app._selected_room_id = "some-other-room" # a poll moved the selection + staged["cb"]({"verdict": "accepted", "rationale": ""}) + assert coordinator.store.outcome(room_id).status == "accepted" + + drive(scenario()) + + +def test_a_broken_judgment_table_never_kills_the_app(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room_id, candidate, _ = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + staged = _captured(app, monkeypatch) + app._confirm_room_judge() # opens on a healthy store + coordinator.store._conn.execute("DROP TABLE room_judgments") + staged["cb"]({"verdict": "accepted", "rationale": ""}) + assert notes[-1][1].get("severity") == "error" + app._confirm_room_judge() + assert notes[-1][1].get("severity") == "error" + assert "no such table" in notes[-1][0] + # The room pane stays up and says why the outcome is missing. + app._refresh_rooms() + assert "Outcome: unavailable (" in app.room_detail_text + assert app.is_running + + drive(scenario()) + + +# ------------------------------------------------------------------ promote + + +def _judged(coordinator, verdict="accepted"): + room_id, candidate, run_id = _applied_room(coordinator) + record, _ = coordinator.judge(room_id, verdict) + return room_id, record + + +def test_promoting_needs_the_board(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _ = _judged(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + app._confirm_lesson_promote() + assert "set SYNCMILL_BOARD__ENABLED=true" in notes[-1][0] + app._config.board.enabled = True + app._board_error = "board.db is not a database" + app._confirm_lesson_promote() + assert notes[-1][0] == ( + "promoting a lesson needs the board, which did not open: " + "board.db is not a database" + ) + + drive(scenario()) + + +def test_promoting_needs_a_verdict(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _, _ = _applied_room(coordinator) + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + staged = _captured(app, monkeypatch) + app._confirm_lesson_promote() + assert staged == {} + assert "room outcome is applied_unreviewed" in notes[-1][0] + assert "record a verdict with j" in notes[-1][0] + + drive(scenario()) + + +def test_a_lesson_promoted_in_the_tui_carries_the_judgment_it_was_reviewed_with( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, record = _judged(coordinator) + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + app._confirm_lesson_promote() + await pilot.pause() + modal = app.screen + assert isinstance(modal, LessonPromoteModal) + assert record["judgment_id"] in modal._detail + modal.query_one("#promote-lesson", TextArea).text = "prefer small diffs" + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + [lesson] = board.project_lessons(coordinator.project_id) + assert lesson.text == "prefer small diffs" + assert lesson.provenance["judgment_id"] == record["judgment_id"] + assert notes[-1][0] == f"promoted lesson {lesson.id[:8]} for the whole project" + + # The same promotion again changes nothing. + app._apply_lesson_promote( + {"lesson": "prefer small diffs", "scope": "", "supersedes": None}, + room_id, str(coordinator.store.root), record["judgment_id"], + ) + assert notes[-1][0] == ( + f"already promoted as lesson {lesson.id[:8]} (active); nothing changed" + ) + assert len(board.project_lessons(coordinator.project_id)) == 1 + + drive(scenario()) + + +def test_a_verdict_changed_during_the_dialog_is_not_promoted( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _ = _judged(coordinator, "accepted") + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + notes = _notes(app, monkeypatch) + staged = _captured(app, monkeypatch) + app._confirm_lesson_promote() + coordinator.judge(room_id, "rejected") # another terminal + staged["cb"]({"lesson": "keep it", "scope": "", "supersedes": None}) + assert "verdict changed since you reviewed it" in notes[-1][0] + assert board.project_lessons(coordinator.project_id) == [] + + drive(scenario()) + + +def test_only_this_projects_lessons_can_be_replaced(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _ = _judged(coordinator) + board = Board(str(tmp_path / "board.db")) + project = coordinator.project_id + prov = {"kind": "room-judgment", "room_id": "r", "judgment_id": "j", + "verdict": "accepted", "candidate_id": "c", "patch_sha256": "a" * 64, + "applied_run_id": "run"} + own, _ = board.promote_lesson("own", project_id=project, provenance=prov, + source_key="k1") + goal, _ = board.promote_lesson("goal", project_id=project, provenance=prov, + source_key="k2", scope="backend") + board.promote_lesson("foreign", project_id="other", provenance=prov, source_key="k3") + board.add_decision("unbound") + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + app._confirm_lesson_promote() + await pilot.pause() + modal = app.screen + assert [(i, s) for _, i, s in modal._supersedable] == [ + (own.id, ""), (goal.id, "backend") + ] + # A replacement must share the scope: refused in the dialog. + modal.query_one("#promote-lesson", TextArea).text = "better" + modal.query_one("#promote-supersedes", Select).value = goal.id + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + assert app.screen is modal + modal.query_one("#promote-scope", Input).value = "backend" + await _press_apply(pilot, modal) + [replacement] = [ + d for d in board.active_decisions(project_id=project) if d.text == "better" + ] + assert replacement.scope == "backend" + assert board.active_decisions(project_id=project).count(goal) == 0 + + drive(scenario()) + + +def test_a_board_that_fails_after_promotion_degrades_instead_of_crashing( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, record = _judged(coordinator) + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + + def broken(*args, **kwargs): + raise BoardError("active_decisions failed: disk I/O error") + + monkeypatch.setattr(board, "active_decisions", broken) + app._apply_lesson_promote( + {"lesson": "prefer small diffs", "scope": "", "supersedes": None}, + room_id, str(coordinator.store.root), record["judgment_id"], + ) + assert len(board.project_lessons(coordinator.project_id)) == 1 + assert "DEGRADED: active_decisions failed" in str( + app.query_one("#board-status").render() + ) + app.action_tab("board-tab") + await pilot.pause() + app._poll() + assert app.is_running + + drive(scenario()) + + +def test_the_board_tab_survives_an_unreadable_selected_item( + git_repo, tmp_path, monkeypatch +): + board = Board(str(tmp_path / "board.db")) + item = board.add("a task") + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._selected_item_id = item.id + + def broken(*args, **kwargs): + raise BoardError("get failed: database is locked") + + monkeypatch.setattr(board, "get", broken) + app._refresh_board() + assert "DEGRADED: get failed" in str(app.query_one("#board-status").render()) + assert app.is_running + + drive(scenario()) + + +# ------------------------------------------------------------ round lessons + + +def _lessons(board, project, count, *, text="lesson {i}"): + prov = {"kind": "room-judgment", "room_id": "r", "judgment_id": "j", + "verdict": "accepted", "candidate_id": "c", "patch_sha256": "a" * 64, + "applied_run_id": "run"} + return [ + board.promote_lesson(text.format(i=i), project_id=project, provenance=prov, + source_key=f"k{i}")[0] + for i in range(count) + ] + + +def _launches(monkeypatch) -> list[dict]: + launched: list[dict] = [] + + def fake_launch(root, room_id, prompt=None, **kwargs): + launched.append({"room_id": room_id, "prompt": prompt, **kwargs}) + return 4242 + + monkeypatch.setattr("syncmill.tui.launch_room_worker", fake_launch) + return launched + + +def test_a_round_sends_exactly_the_checked_lessons_with_the_clis_budget( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + lessons = _lessons(board, coordinator.project_id, 21) # the budget is 20 + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + launched = _launches(monkeypatch) + app._confirm_room_round() + await pilot.pause() + modal = app.screen + assert isinstance(modal, RoomRoundModal) + status = str(modal.query_one("#round-lessons-status").render()) + assert "Lessons this round: 20 of 21" in status + assert f"left out: {lessons[0].id[:8]} (over the per-run lesson budget)" in status + + # Unchecking one of the newest frees budget for the oldest, as + # --skip-lesson does. + modal.query_one("#round-lesson-20", Checkbox).value = False + await pilot.pause() + status = str(modal.query_one("#round-lessons-status").render()) + assert f"left out: {lessons[20].id[:8]} (unchecked)" in status + assert "over the per-run lesson budget" not in status + + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + [launch] = launched + assert [(x.id, x.text) for x in launch["lessons"]] == [ + (x.id, x.text) for x in lessons[:20] + ] + assert launch["room_id"] == room["id"] + + drive(scenario()) + + +def test_a_round_that_cannot_fit_its_lessons_is_refused_until_it_does( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + _lessons(board, coordinator.project_id, 1, text="x" * 900 + "{i}") + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + notes = _notes(app, monkeypatch) + launched = _launches(monkeypatch) + app._confirm_room_round() + await pilot.pause() + modal = app.screen + modal.query_one("#round-prompt", TextArea).text = "p" * (_ROOM_PROMPT_MAX_BYTES - 200) + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + assert app.screen is modal and launched == [] + assert "leaves no room for the checked lessons" in notes[-1][0] + + # Shortening the prompt lets it through with the lesson. + modal.query_one("#round-prompt", TextArea).text = "short" + await _press_apply(pilot, modal) + assert len(launched) == 1 and len(launched[0]["lessons"]) == 1 + + drive(scenario()) + + +def test_the_fit_check_and_the_launch_read_a_blank_prompt_the_same_way( + git_repo, tmp_path +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + huge = {"prompt": "p" * (_ROOM_PROMPT_MAX_BYTES - 200)} + view = app._round_lessons(room["id"], huge) + assert round_prompt(" \n\t") is None + assert round_prompt(" keep ") == " keep " + # None = the creation prompt, which here leaves no room. + assert not view.fits(round_prompt(" "), ["x" * 900]) + assert view.fits(round_prompt("short"), ["x" * 900]) + + drive(scenario()) + + +def test_a_round_without_usable_lessons_says_why_and_sends_none( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + r = coordinator.store.get_room(room["id"]) + assert app._round_lessons(room["id"], r).lessons == () + + def broken(project): + raise BoardError("project_lessons failed: disk I/O error") + + monkeypatch.setattr(board, "project_lessons", broken) + view = app._round_lessons(room["id"], r) + assert view.unavailable == "project_lessons failed: disk I/O error" + app._board = None + app._board_error = "board.db is locked" + assert app._round_lessons(room["id"], r).unavailable == "board.db is locked" + app._config.board.enabled = False + view = app._round_lessons(room["id"], r) + assert view.unavailable is None and view.lessons == () + + launched = _launches(monkeypatch) + app._apply_room_round( + {"prompt": "go", "lineage": "fresh", "base_candidate": None, "lessons": []}, + room["id"], str(coordinator.store.root), + ) + assert launched[0]["lessons"] == () + + drive(scenario()) + + +def test_a_lesson_id_the_dialog_did_not_show_is_refused(git_repo, tmp_path, monkeypatch): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + launched = _launches(monkeypatch) + notes = _notes(app, monkeypatch) + app._apply_room_round( + {"prompt": "go", "lineage": "fresh", "base_candidate": None, + "lessons": ["00000000-0000-4000-8000-000000000000"]}, + room["id"], str(coordinator.store.root), + ) + assert launched == [] and notes[-1][1].get("severity") == "error" + + drive(scenario()) + + +def test_lesson_text_in_the_round_dialog_is_never_markup(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + _lessons(board, coordinator.project_id, 1, text="[b]bold[/b] [oops \u202eRTL{i}") + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + app._confirm_room_round() + await pilot.pause() + label = app.screen.query_one("#round-lesson-0", Checkbox).label + assert "[b]bold[/b] [oops \\u202eRTL0" in label.plain + assert not label.spans + + drive(scenario()) + + +def test_the_round_dialog_keeps_its_buttons_reachable_at_80x24(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + _lessons(board, coordinator.project_id, 21) + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test(size=(80, 24)) as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + app._confirm_room_round() + await pilot.pause() + for selector in ("#confirmed", "#apply", "#cancel"): + region = app.screen.query_one(selector).region + assert region.height > 0 and region.bottom <= 24, selector + + drive(scenario()) + + +def test_the_history_note_follows_the_chosen_lineage(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + from syncmill.tui_collaboration import RoundLessonsView + + app.push_screen( + RoomRoundModal( + coordinator.store.get_room(room["id"]), "p", [], + RoundLessonsView(history={"continue-own": True, "branch-from": False}), + ) + ) + await pilot.pause() + modal = app.screen + status = lambda: str(modal.query_one("#round-lessons-status").render()) # noqa: E731 + assert "may still hold lessons sent in earlier rounds" in status() + modal.query_one("#round-lineage", Select).value = "fresh" + await pilot.pause() + assert status() == "" + + drive(scenario()) + + +# ---------------------------------------------------------------- freshness + + +def _snapshot(app, coordinator, room_id): + from syncmill.tui import build_room_evidence + + result = build_room_evidence( + coordinator.store.root, coordinator.store.candidates.output_max_bytes, + coordinator.repo_root, room_id, 0.0, + ) + app._apply_evidence( + app._evidence_generation, room_id, str(coordinator.store.root), result, None, 0.0 + ) + + +def test_a_verdict_from_anywhere_marks_the_compare_snapshot_stale( + git_repo, tmp_path +): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _, _ = _applied_room(coordinator) + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + _snapshot(app, coordinator, room_id) + + def text(): + return app._stage_evidence_view(room_id, None, "compare", 1)[1] + + assert "room changed since this snapshot" not in text() + coordinator.judge(room_id, "accepted") # e.g. `room judge` elsewhere + assert "room changed since this snapshot" in text() + _snapshot(app, coordinator, room_id) + coordinator.judge(room_id, "accepted") # idempotent: no new row + assert "room changed since this snapshot" not in text() + + coordinator.store._conn.execute("DROP TABLE room_judgments") + shown = text() + assert "cannot tell whether a verdict changed since this snapshot" in shown + assert "Outcome:" in shown # the snapshot still renders + + drive(scenario()) + + +# ------------------------------------------------------------- coordinator + + +def test_promote_lesson_refuses_a_verdict_other_than_the_one_reviewed( + git_repo, tmp_path +): + coordinator = _coordinator(git_repo, tmp_path) + board = Board(str(tmp_path / "board.db")) + try: + room_id, first = _judged(coordinator, "accepted") + coordinator.judge(room_id, "rejected") + with pytest.raises(RoomError, match="verdict changed since you reviewed it"): + coordinator.promote_lesson( + room_id, board, "a lesson", expected_judgment_id=first["judgment_id"] + ) + assert board.project_lessons(coordinator.project_id) == [] + latest = coordinator.store.outcome(room_id).judgment + lesson, created = coordinator.promote_lesson( + room_id, board, "a lesson", expected_judgment_id=latest["judgment_id"] + ) + assert created and lesson.provenance["judgment_id"] == latest["judgment_id"] + finally: + board.close() + coordinator.close() + + +def test_judgment_count_counts_rows(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + try: + room_id, _, _ = _applied_room(coordinator) + assert coordinator.store.judgment_count(room_id) == 0 + coordinator.judge(room_id, "accepted") + coordinator.judge(room_id, "rejected") + assert coordinator.store.judgment_count(room_id) == 2 + assert coordinator.store.judgment_count("no-such-room") == 0 + finally: + coordinator.close() + + +def test_a_blank_round_prompt_is_checked_against_the_creation_prompt( + git_repo, tmp_path, monkeypatch +): + """A whitespace-only prompt reuses the creation prompt, so the dialog's + fit check must measure that one, not the whitespace.""" + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("c" * (_ROOM_PROMPT_MAX_BYTES - 200), ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + _lessons(board, coordinator.project_id, 1, text="x" * 900 + "{i}") + + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + launched = _launches(monkeypatch) + app._confirm_room_round() + await pilot.pause() + modal = app.screen + modal.query_one("#round-prompt", TextArea).text = " \n" + modal.query_one("#confirmed", Checkbox).value = True + await _press_apply(pilot, modal) + assert app.screen is modal and launched == [] + + drive(scenario()) From 8f183af91bf9df6f9409d292720eaf4aa6403a78 Mon Sep 17 00:00:00 2001 From: memtomem <118508877+memtomem@users.noreply.github.com> Date: Thu, 5 Nov 2026 10:30:46 +0900 Subject: [PATCH 2/4] Address TUI review round 1: submit boundary, uncounted snapshots - A round submission catches the room store's error boundary, so a room row that turned unreadable after the dialog opened is reported, not raised out of the callback. - An evidence snapshot whose build could not count verdicts says it cannot tell whether a verdict changed, instead of silently skipping the check. - The round dialog says when the board is off; the verdict dialog shows the current outcome block (who, when, why) it would supersede. - An 80x24 test toggles a lesson and submits with the mouse. - Docs: only the round, verdict and promote dialogs capture the room and store. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- docs/architecture/tui.md | 8 +- src/syncmill/tui.py | 41 +++++---- src/syncmill/tui_collaboration.py | 7 ++ tests/test_tui.py | 23 ++--- tests/test_tui_room_review.py | 138 +++++++++++++++++++++++++++++- 6 files changed, 186 insertions(+), 33 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 867d896..a2496ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,7 @@ uv run syncmill room export --include patch: [--include output:| # room rounds record their effective starting tree + gate settings (v2 adds the gate's env_allowlist: null = inherited, [] distinct) in rounds.provenance_json (rooms.round_provenance_status, writer and readers share it, readers accept v1+v2; NULL = legacy); compare/export/TUI show them as starting tree / gate settings beside the report-derived gate profile, whose JSON fields are unchanged; a v1 round's environment reads not recorded and never groups as same settings; `room apply --verify` still runs the current `[gate]` and prints a `verify:` notice (`rooms.verify_settings_notice`) unless the round's v2 record equals it uv run syncmill room select [--expected-revision N --expected-digest SHA256 --rationale TEXT] # verifies the artifact, records a bound selection (rooms.selection_json); apply refuses an inconsistent record uv run syncmill room judge accepted|rejected [--expected-digest SHA256 --rationale TEXT] # #174: append-only human verdict on the APPLIED candidate (room_judgments, bound to candidate + patch digest + applied run; never inferred from select/apply, never touches memory, no state/revision change); rooms.room_outcome_status is the one read model (not_selected / selected_not_applied / applied_unreviewed / accepted / rejected / unknown — a damaged latest record is unknown, never an older verdict) shown by compare/export as room.outcome -uv run syncmill room promote --lesson TEXT [--scope GOAL --supersedes ID] # #174 PR 2: needs the board; only a judged room (accepted/rejected); a board decision bound to project_id with provenance_json (room/judgment/verdict/candidate/digest/run) and a unique source_key (retry returns the existing decision, never reactivates a superseded one); `run` shows then injects Board.project_lessons (project-wide scope only; board.select_lessons budget; --skip-lesson/--no-lessons, forwarded in _inner_argv) via Supervisor.run(lessons=) AFTER routing classified the raw prompt; board items get unbound + own-project decisions; room rounds: `room round`/`start` show them (cli._show_round_lessons, rooms.round_lessons_fit) and pass RoundLesson pairs to run_round(lessons=) (detached: a second anonymous fd, --lessons-fd); the coordinator never opens the board; lessons sit after the checkpoint, before the task, excerpts budgeted around them; TUI (Rooms `j` verdict / `p` promote, `tui.py` `_confirm_room_judge`/`_confirm_lesson_promote`): the round dialog reads `project_lessons` once on the Textual thread (`RoundLessonsView`), recomputes `select_lessons(skip=unchecked)` live, refuses (never drops) when `round_lessons_fit` fails on `round_prompt`'s fallback rule, and launches with exactly those pairs; promote passes `expected_judgment_id` (checked on the same outcome read as the provenance); every Rooms dialog callback acts on the captured room id + store root +uv run syncmill room promote --lesson TEXT [--scope GOAL --supersedes ID] # #174 PR 2: needs the board; only a judged room (accepted/rejected); a board decision bound to project_id with provenance_json (room/judgment/verdict/candidate/digest/run) and a unique source_key (retry returns the existing decision, never reactivates a superseded one); `run` shows then injects Board.project_lessons (project-wide scope only; board.select_lessons budget; --skip-lesson/--no-lessons, forwarded in _inner_argv) via Supervisor.run(lessons=) AFTER routing classified the raw prompt; board items get unbound + own-project decisions; room rounds: `room round`/`start` show them (cli._show_round_lessons, rooms.round_lessons_fit) and pass RoundLesson pairs to run_round(lessons=) (detached: a second anonymous fd, --lessons-fd); the coordinator never opens the board; lessons sit after the checkpoint, before the task, excerpts budgeted around them; TUI (Rooms `j` verdict / `p` promote, `tui.py` `_confirm_room_judge`/`_confirm_lesson_promote`): the round dialog reads `project_lessons` once on the Textual thread (`RoundLessonsView`), recomputes `select_lessons(skip=unchecked)` live, refuses (never drops) when `round_lessons_fit` fails on `round_prompt`'s fallback rule, and launches with exactly those pairs; promote passes `expected_judgment_id` (checked on the same outcome read as the provenance); the round/verdict/promote dialog callbacks act on the captured room id + store root uv run syncmill conversations list uv run syncmill conversations show uv run syncmill conversations close diff --git a/docs/architecture/tui.md b/docs/architecture/tui.md index 162ee0d..608778b 100644 --- a/docs/architecture/tui.md +++ b/docs/architecture/tui.md @@ -94,16 +94,16 @@ shows the project's approved lessons (`RoundLessonsView`, read once when it opens) and recomputes the CLI's `select_lessons(skip=)` on every checkbox, prompt or lineage change. It refuses instead of dropping lessons when the prompt leaves no room (`round_prompt` gives the fit check and the launch one -fallback rule), and the worker gets exactly the shown pairs. Every Rooms dialog -captures `(room id, store root)` when it opens; its callback acts on those and +fallback rule), and the worker gets exactly the shown pairs. The round, verdict +and promote dialogs capture `(room id, store root)` when they open; its callback acts on those and refuses when the store was rebound, never reading the ambient selection. Judge and promote run on the Textual thread on the existing connections (SQLite is thread-affine; the board connection must not enter a worker), accepting up to the stores' 5 s busy timeout of blocking under write contention. The evidence snapshot also records the room's judgment count, read before the build: a verdict changes neither revision nor candidates, so `changed` compares the -count too, and a failed count read degrades to a "cannot tell" line instead -of aborting the staged room update. Checkbox labels built from stored text go +count too; a failed count read, at the build or now, degrades to a "cannot +tell" line instead of aborting the staged room update. Checkbox labels built from stored text go through `plain(...)` (Textual parses a `str` label as markup); the `markup_sinks` guard flags any other non-literal label. EVERY poll path is degrade-guarded (`_ROOM_POLL_ERRORS`/`_SESSION_POLL_ERRORS`/ diff --git a/src/syncmill/tui.py b/src/syncmill/tui.py index 5b9e976..94f1cd1 100644 --- a/src/syncmill/tui.py +++ b/src/syncmill/tui.py @@ -2788,7 +2788,11 @@ def _stage_evidence_view( int(room["revision"]) != evidence["revision"] or current_ids != evidence["candidate_ids"] ) - if not changed and evidence.get("judgment_count") is not None: + if not changed and evidence.get("judgment_count") is None: + # The build could not count verdicts, so a later one cannot be + # detected against it: say so instead of implying "unchanged". + freshness_error = "this snapshot could not count verdicts" + elif not changed: # A verdict (from any surface) changes neither of the above. # Degraded locally: an unreadable judgments table must not take # the whole staged room update down with it. @@ -3001,7 +3005,7 @@ def fits(prompt: str | None, texts: list[str]) -> bool: ) if not self._config.board.enabled: - return RoundLessonsView(history=history, fits=fits) + return RoundLessonsView(board_off=True, history=history, fits=fits) if self._board is None: return RoundLessonsView( unavailable=self._board_error or "the board did not open", @@ -3091,9 +3095,11 @@ def _apply_room_round( # reports an unusable path that way rather than letting subprocess's # OSError through, and an unguarded raise here kills the whole app. except ( - RoomError, ArtifactError, WorkspaceError, ProjectError, OSError, sqlite3.Error + *_ROOM_POLL_ERRORS, ArtifactError, WorkspaceError, ProjectError, OSError ) as exc: - self.notify(str(exc), severity="error") + # The room store's error boundary too: a row that turned unreadable + # after the dialog opened must not escape the callback. + self.notify(str(exc) or type(exc).__name__, severity="error") return sent = f", {len(lessons)} approved lesson(s)" if lessons else "" self.notify(f"room worker started (pid {pid}{sent})") @@ -3127,19 +3133,20 @@ def _confirm_room_judge(self) -> None: self.notify(str(exc), severity="error") return p = printable_name - current = ( - "not reviewed yet" - if outcome.judgment is None - else f"{outcome.status} (judgment {outcome.judgment['sequence']} of " - f"{outcome.judgment_count}; a new verdict supersedes it, history is kept)" - ) - detail = ( - f"Room: {p(room_id)}\n" - f"Applied candidate: {p(str(outcome.candidate_id))}\n" - f"Applied as run: {p(str(outcome.applied_run_id))}\n" - f"Patch sha256: {p(str(row['patch_sha256']))}\n" - f"Current verdict: {current}\n\n" - "The verdict is bound to this patch digest. Nothing is shared to memory." + detail = "\n".join( + [ + f"Room: {p(room_id)}", + f"Applied candidate: {p(str(outcome.candidate_id))}", + f"Patch sha256: {p(str(row['patch_sha256']))}", + "", + # The current verdict as compare shows it (who, when, why), so + # the person sees what a new verdict would supersede. + *outcome_lines(outcome), + "", + "The verdict is bound to this patch digest; a new one supersedes " + "the current one and both stay in the history. Nothing is shared " + "to memory.", + ] ) self.push_screen( RoomJudgeModal(detail), diff --git a/src/syncmill/tui_collaboration.py b/src/syncmill/tui_collaboration.py index c2e5a81..0ca4372 100644 --- a/src/syncmill/tui_collaboration.py +++ b/src/syncmill/tui_collaboration.py @@ -125,6 +125,7 @@ class RoundLessonsView: """What the round dialog shows about approved lessons (#174), read once when the dialog opens. + ``board_off`` says the board is disabled, so no lessons can be sent. ``lessons`` are the project's active project-wide lessons, oldest first; the dialog re-runs the CLI's ``select_lessons`` on every change, so a skipped lesson frees budget exactly as ``--skip-lesson`` does. @@ -136,6 +137,7 @@ class RoundLessonsView: prompt.""" lessons: tuple[Decision, ...] = () + board_off: bool = False unavailable: str | None = None history: dict[str, bool] = dataclasses.field(default_factory=dict) fits: Callable[[str | None, list[str]], bool] = lambda prompt, texts: True @@ -209,6 +211,11 @@ def compose(self) -> ComposeResult: prompt="base candidate (branch-from only)", id="round-base", ) + if self._lessons.board_off: + yield PlainStatic( + "Lessons: the board is off, so this round sends no approved " + "lessons." + ) if self._lessons.unavailable is not None: yield PlainStatic( f"Lessons unavailable ({printable_name(self._lessons.unavailable)}); " diff --git a/tests/test_tui.py b/tests/test_tui.py index ad3af23..040df98 100644 --- a/tests/test_tui.py +++ b/tests/test_tui.py @@ -1197,7 +1197,7 @@ def build(store_root, output_max, repo_root, room_id, now): calls.append(room_id) comparison = _comparison(rows, room_id=room_id, **kwargs) ids = frozenset(r.candidate_id for r in rows) - return comparison, comparison.revision, ids, None + return comparison, comparison.revision, ids, 0 monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -1210,6 +1210,9 @@ class _FakeCandidateRoomStore: root = "/nonexistent-room-store" candidates = type("_Candidates", (), {"output_max_bytes": 1})() + def judgment_count(self, room_id): + return 0 + def __init__(self, patch: bytes = b"diff --git a/x b/x\n+line\n" * 20_000): self.patch = patch self.rows = { @@ -3921,7 +3924,7 @@ def build(store_root, output_max, repo_root, room_id, now): gate.wait(5) running["now"] -= 1 rows = [_candidate_row(candidate_id="cand-ok", member_id=f"build{running['builds']}")] - return _comparison(rows), 1, frozenset({"cand-ok"}), None + return _comparison(rows), 1, frozenset({"cand-ok"}), 0 monkeypatch.setattr("syncmill.tui.build_room_evidence", build) rendered: list[str] = [] @@ -3971,7 +3974,7 @@ def test_a_store_root_change_retires_an_in_flight_build(tmp_path, git_repo, monk def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) - return _comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), None + return _comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), 0 monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4047,7 +4050,7 @@ async def scenario(): _fake_evidence(monkeypatch, [_candidate_row(candidate_id="cand-ok")]) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (_comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), None), + lambda *a: (_comparison([_candidate_row(candidate_id="cand-ok")]), 1, frozenset({"cand-ok"}), 0), ) await pilot.press("7") await pilot.pause() @@ -4137,7 +4140,7 @@ async def scenario(): ) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (comparison, 1, frozenset(), None), + lambda *a: (comparison, 1, frozenset(), 0), ) await pilot.press("7") await pilot.pause() @@ -4363,7 +4366,7 @@ def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) rows = [_candidate_row(candidate_id="cand-ok", member_id=f"built-for-{room_id}")] - return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), None + return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), 0 monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4459,7 +4462,7 @@ def build(store_root, output_max, repo_root, room_id, now): started.set() gate.wait(5) rows = [_candidate_row(candidate_id="cand-ok", member_id=f"built-for-{room_id}")] - return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), None + return _comparison(rows, room_id=room_id), 1, frozenset({"cand-ok", "cand-bad"}), 0 monkeypatch.setattr("syncmill.tui.build_room_evidence", build) @@ -4620,7 +4623,7 @@ async def scenario(): monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( "syncmill.tui.build_room_evidence", - lambda *a: (_comparison([]), 1, frozenset(), None), + lambda *a: (_comparison([]), 1, frozenset(), 0), ) await pilot.press("7") await pilot.pause() @@ -4661,7 +4664,7 @@ async def scenario(): app._rooms = _FakeRooms(store=_Damaged()) monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( - "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), None) + "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), 0) ) await pilot.press("7") await pilot.pause() @@ -4777,7 +4780,7 @@ async def scenario(): app._rooms = _FakeRooms(store=_OnlyDamaged()) monkeypatch.setattr(app, "_reload_config", lambda: True) monkeypatch.setattr( - "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), None) + "syncmill.tui.build_room_evidence", lambda *a: (damaged, 1, frozenset(), 0) ) await pilot.press("7") await pilot.pause() diff --git a/tests/test_tui_room_review.py b/tests/test_tui_room_review.py index aa8fbd0..b8a9a53 100644 --- a/tests/test_tui_room_review.py +++ b/tests/test_tui_room_review.py @@ -5,6 +5,7 @@ from __future__ import annotations import asyncio +import sqlite3 import pytest from textual.widgets import Button, Checkbox, Input, Select, TextArea @@ -95,7 +96,7 @@ async def scenario(): modal = app.screen assert isinstance(modal, RoomJudgeModal) assert candidate["patch_sha256"] in modal._detail - assert run_id in modal._detail and "not reviewed yet" in modal._detail + assert f"Outcome: applied as run {run_id}, not reviewed" in modal._detail # No default verdict: confirming without one records nothing. modal.query_one("#confirmed", Checkbox).value = True @@ -797,3 +798,138 @@ async def scenario(): assert app.screen is modal and launched == [] drive(scenario()) + + +def test_a_room_that_turns_unreadable_after_the_round_dialog_opened_is_reported( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + notes = _notes(app, monkeypatch) + staged = _captured(app, monkeypatch) + launched = _launches(monkeypatch) + app._confirm_room_round() + with coordinator.store._tx() as conn: + conn.execute( + "UPDATE rooms SET members_json='{not json' WHERE id=?", (room["id"],) + ) + staged["cb"]({"prompt": "go", "lineage": "fresh", "base_candidate": None, + "lessons": []}) + await pilot.pause() + assert launched == [] + assert notes[-1][1].get("severity") == "error" + assert app.is_running + + drive(scenario()) + + +def test_a_snapshot_that_could_not_count_verdicts_never_claims_it_is_current( + git_repo, tmp_path, monkeypatch +): + from syncmill.rooms import RoomStore + + coordinator = _coordinator(git_repo, tmp_path) + room_id, _, _ = _applied_room(coordinator) + real = RoomStore.judgment_count + + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + + def failing(self, room_id): + raise sqlite3.OperationalError("database is locked") + + monkeypatch.setattr(RoomStore, "judgment_count", failing) + _snapshot(app, coordinator, room_id) # the build cannot count + monkeypatch.setattr(RoomStore, "judgment_count", real) + coordinator.judge(room_id, "accepted") + text = app._stage_evidence_view(room_id, None, "compare", 1)[1] + assert "cannot tell whether a verdict changed since this snapshot" in text + assert "this snapshot could not count verdicts" in text + + drive(scenario()) + + +def test_the_round_dialog_says_when_the_board_is_off(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + app._confirm_room_round() + await pilot.pause() + shown = " ".join( + str(widget.render()) for widget in app.screen.query("PlainStatic") + ) + assert "the board is off, so this round sends no approved lessons" in shown + + drive(scenario()) + + +def test_the_verdict_dialog_shows_the_verdict_it_would_supersede(git_repo, tmp_path): + coordinator = _coordinator(git_repo, tmp_path) + room_id, _, _ = _applied_room(coordinator) + coordinator.judge(room_id, "rejected", rationale="broke the parser") + + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test() as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room_id + app._confirm_room_judge() + await pilot.pause() + detail = app.screen._detail + assert "Outcome: REJECTED (human)" in detail + assert "judged via API" in detail # coordinator.judge default surface + assert "broke the parser" in detail + + drive(scenario()) + + +def test_lessons_can_be_toggled_and_submitted_with_the_mouse_at_80x24( + git_repo, tmp_path, monkeypatch +): + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + lessons = _lessons(board, coordinator.project_id, 21) + + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test(size=(80, 24)) as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + launched = _launches(monkeypatch) + app._confirm_room_round() + await pilot.pause() + modal = app.screen + box = modal.query_one("#round-lesson-20", Checkbox) + box.scroll_visible(animate=False) + await pilot.pause() + await pilot.click("#round-lesson-20") + await pilot.pause() + assert box.value is False + await pilot.click("#confirmed") + await pilot.pause() + await pilot.click("#apply") + await pilot.pause() + [launch] = launched + assert [x.id for x in launch["lessons"]] == [x.id for x in lessons[:20]] + + drive(scenario()) From 35d704d02317c8d766f268ca71d2230c4ca46703 Mon Sep 17 00:00:00 2001 From: memtomem <118508877+memtomem@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:07:11 +0900 Subject: [PATCH 3/4] Show a long lesson whole in the TUI round dialog A Checkbox label never wraps, so a lesson longer than the dialog was cut off at its edge while every word of it was sent to every member (PR review: a 401-character lesson made a 421-column checkbox in a 66-column body). The checkbox label is now just the lesson id; the full text is a wrapping PlainStatic under it. The lessons heading was a 108-column PlainLabel that widened the body too; it is a PlainStatic now. At 80x24 the body no longer scrolls sideways and a 410-character lesson wraps to seven lines. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/tui.md | 5 +++- src/syncmill/tui.py | 1 + src/syncmill/tui_collaboration.py | 15 ++++++++--- tests/test_tui_room_review.py | 43 ++++++++++++++++++++++++++++--- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/docs/architecture/tui.md b/docs/architecture/tui.md index 608778b..3cfc3cb 100644 --- a/docs/architecture/tui.md +++ b/docs/architecture/tui.md @@ -105,7 +105,10 @@ verdict changes neither revision nor candidates, so `changed` compares the count too; a failed count read, at the build or now, degrades to a "cannot tell" line instead of aborting the staged room update. Checkbox labels built from stored text go through `plain(...)` (Textual parses a `str` label as markup); the -`markup_sinks` guard flags any other non-literal label. +`markup_sinks` guard flags any other non-literal label. A lesson's checkbox +label is only its id: a `Checkbox` label never wraps, so a long lesson would be +cut off at the dialog edge while every word of it is sent. The full text is a +wrapping `PlainStatic` under it. EVERY poll path is degrade-guarded (`_ROOM_POLL_ERRORS`/`_SESSION_POLL_ERRORS`/ `_JOB_POLL_ERRORS`) — an unguarded read escapes the interval callback and kills the app, so a store failure must show DEGRADED and keep the last good view. diff --git a/src/syncmill/tui.py b/src/syncmill/tui.py index 94f1cd1..86fd9f2 100644 --- a/src/syncmill/tui.py +++ b/src/syncmill/tui.py @@ -783,6 +783,7 @@ class SyncmillTuiApp(App[None]): .form-buttons { height: auto; align-horizontal: right; } #selection-body { height: 1fr; } #selection-rationale { height: 6; } + .lesson-text { width: 1fr; padding: 0 0 0 4; } .form-buttons Button { margin-left: 2; } _FormModal { align: center middle; } .warning { color: $warning; margin: 1 0; } diff --git a/src/syncmill/tui_collaboration.py b/src/syncmill/tui_collaboration.py index 0ca4372..2228621 100644 --- a/src/syncmill/tui_collaboration.py +++ b/src/syncmill/tui_collaboration.py @@ -223,20 +223,27 @@ def compose(self) -> ComposeResult: classes="warning", ) if self._lessons.lessons: - yield PlainLabel( + yield PlainStatic( "Approved project lessons (sent to every member's prompt " "this round; uncheck to leave one out of this round):" ) for index, lesson in enumerate(self._lessons.lessons): - # plain(): a str label is parsed as markup by Checkbox. + # The label is only the id: a Checkbox label does not + # wrap, so a long lesson would be cut off at the dialog + # edge while every word of it is sent. The full text is + # a wrapping PlainStatic below it. plain(): a str label + # is parsed as markup by Checkbox. yield Checkbox( - plain(f"{lesson.id[:8]} {lesson.text}"), + plain(f"lesson {lesson.id[:8]}"), value=True, id=f"round-lesson-{index}", ) + yield PlainStatic( + lesson.text, classes="lesson-text", id=f"round-lesson-text-{index}" + ) source = lesson_source(lesson) if source: - yield PlainStatic(f" {source}") + yield PlainStatic(source, classes="lesson-text") yield PlainStatic( "Replace a lesson with p on a judged room, or " '`syncmill board decide --supersedes ID ""`.' diff --git a/tests/test_tui_room_review.py b/tests/test_tui_room_review.py index b8a9a53..7443e50 100644 --- a/tests/test_tui_room_review.py +++ b/tests/test_tui_room_review.py @@ -625,7 +625,10 @@ def test_lesson_text_in_the_round_dialog_is_never_markup(git_repo, tmp_path): coordinator = _coordinator(git_repo, tmp_path) room = coordinator.create("compare", ["claude", "codex"]) board = Board(str(tmp_path / "board.db")) - _lessons(board, coordinator.project_id, 1, text="[b]bold[/b] [oops \u202eRTL{i}") + [lesson] = _lessons( + board, coordinator.project_id, 1, text="[b]bold[/b] [oops \u202eRTL{i}" + ) + async def scenario(): app = _app(tmp_path, git_repo, board=board) async with app.run_test() as pilot: @@ -635,8 +638,42 @@ async def scenario(): app._confirm_room_round() await pilot.pause() label = app.screen.query_one("#round-lesson-0", Checkbox).label - assert "[b]bold[/b] [oops \\u202eRTL0" in label.plain - assert not label.spans + assert label.plain == f"lesson {lesson.id[:8]}" and not label.spans + shown = app.screen.query_one("#round-lesson-text-0").render() + assert "[b]bold[/b] [oops \\u202eRTL0" in str(shown) + assert not getattr(shown, "spans", None) + + drive(scenario()) + + +def test_a_long_lesson_is_shown_whole_in_the_round_dialog_at_80x24(git_repo, tmp_path): + """Every word of a lesson is sent, so every word must be readable: a long + lesson wraps inside the dialog instead of running past its edge.""" + coordinator = _coordinator(git_repo, tmp_path) + room = coordinator.create("compare", ["claude", "codex"]) + board = Board(str(tmp_path / "board.db")) + _lessons(board, coordinator.project_id, 1, text="word " * 80 + "FINALWORDS{i}") + + async def scenario(): + app = _app(tmp_path, git_repo, board=board) + async with app.run_test(size=(80, 24)) as pilot: + await pilot.pause() + app._rooms = coordinator + app._selected_room_id = room["id"] + app._confirm_room_round() + await pilot.pause() + body = app.screen.query_one("#round-body") + text = app.screen.query_one("#round-lesson-text-0") + assert body.max_scroll_x == 0 + assert text.region.right <= body.region.right + assert text.region.height > 1 # wrapped, not cut + text.scroll_visible(animate=False) + await pilot.pause() + lines = [ + "".join(segment.text for segment in strip) + for strip in text.render_lines(text.region.reset_offset) + ] + assert "FINALWORDS0" in "".join(lines) drive(scenario()) From 7a76fed805d24ba2ca9ecfde6dc35287e9ee636d Mon Sep 17 00:00:00 2001 From: memtomem <118508877+memtomem@users.noreply.github.com> Date: Thu, 5 Nov 2026 14:27:35 +0900 Subject: [PATCH 4/4] Keep typed text visible in the TUI dialogs' editors The round prompt, verdict rationale and lesson editors kept TextArea's default 1fr height inside a scrolling dialog body, where it collapses to its border: at 80x24 all three had a content height of 0, so typing changed the value without showing it (PR review). They now have fixed heights (8 rows for the prompt, 6 for the other two), and a test types into each at 80x24 and 120x40 and checks the words are drawn. Co-Authored-By: Claude Opus 5.5 --- src/syncmill/tui.py | 4 ++++ tests/test_tui_room_review.py | 45 +++++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/syncmill/tui.py b/src/syncmill/tui.py index 86fd9f2..c7170ae 100644 --- a/src/syncmill/tui.py +++ b/src/syncmill/tui.py @@ -784,6 +784,10 @@ class SyncmillTuiApp(App[None]): #selection-body { height: 1fr; } #selection-rationale { height: 6; } .lesson-text { width: 1fr; padding: 0 0 0 4; } + /* Editors inside a scrolling dialog body get a fixed height: TextArea's + default 1fr collapses to its border there, hiding what is typed. */ + #round-prompt { height: 8; } + #judge-rationale, #promote-lesson { height: 6; } .form-buttons Button { margin-left: 2; } _FormModal { align: center middle; } .warning { color: $warning; margin: 1 0; } diff --git a/tests/test_tui_room_review.py b/tests/test_tui_room_review.py index 7443e50..66e6a00 100644 --- a/tests/test_tui_room_review.py +++ b/tests/test_tui_room_review.py @@ -20,6 +20,7 @@ LessonPromoteModal, RoomJudgeModal, RoomRoundModal, + RoundLessonsView, round_prompt, ) @@ -705,8 +706,6 @@ async def scenario(): app = _app(tmp_path, git_repo, board=None, enabled=False) async with app.run_test() as pilot: await pilot.pause() - from syncmill.tui_collaboration import RoundLessonsView - app.push_screen( RoomRoundModal( coordinator.store.get_room(room["id"]), "p", [], @@ -970,3 +969,45 @@ async def scenario(): assert [x.id for x in launch["lessons"]] == [x.id for x in lessons[:20]] drive(scenario()) + + +@pytest.mark.parametrize("size", [(80, 24), (120, 40)]) +@pytest.mark.parametrize("dialog", ["round", "judge", "promote"]) +def test_text_typed_into_a_dialog_editor_stays_visible(tmp_path, git_repo, size, dialog): + """A TextArea's default 1fr height collapses to its border inside a + scrolling dialog body: typing changed the value but showed nothing.""" + board = Board(str(tmp_path / "board.db")) + lessons = tuple(_lessons(board, "proj", 3)) + + async def scenario(): + app = _app(tmp_path, git_repo, board=None, enabled=False) + async with app.run_test(size=size) as pilot: + await pilot.pause() + modal, selector = { + "round": ( + RoomRoundModal( + {"id": "r", "members": [], "round_count": 0}, "", [], + RoundLessonsView(lessons=lessons), + ), + "#round-prompt", + ), + "judge": (RoomJudgeModal("Room: r\n" * 8), "#judge-rationale"), + "promote": (LessonPromoteModal("source\n" * 8, []), "#promote-lesson"), + }[dialog] + app.push_screen(modal) + await pilot.pause() + editor = app.screen.query_one(selector, TextArea) + editor.focus() + editor.scroll_visible(animate=False) + await pilot.pause() + await pilot.press(*"visible words") + await pilot.pause() + assert editor.text == "visible words" + assert editor.content_region.height >= 3 + shown = "".join( + "".join(segment.text for segment in editor.render_line(y)) + for y in range(editor.region.height) + ) + assert "visible words" in shown + + drive(scenario())