From 3dff8495ddd4a8c3fb83a45e5e2c28dc006e8964 Mon Sep 17 00:00:00 2001 From: memtomem <118508877+memtomem@users.noreply.github.com> Date: Tue, 10 Nov 2026 17:21:35 +0900 Subject: [PATCH] Write review feedback on a room candidate and repair from it (#173 PR 2) `room feedback add CAND --kind change|question [--file PATH]` records a request on one completed candidate, optionally tied to a file it changed. `room repair CAND` now sends that candidate's open requests by default (`--feedback ID` narrows them), and `--prompt` becomes optional. `feedback resolve` records, by a human only, whether a later candidate of the same member met a request, and `feedback carry` writes it again on that later candidate. Records are append-only rows in a new `room_feedback` table, and status is derived on every read, never stored: open, sent, addressed, not_addressed, withdrawn or unknown. A request reads `sent` only once a repair of its own candidate completed, so a failed repair sends it again. The repair plan binds each request's record digest and status, and the claim re-derives them, so a resolve between confirm and claim refuses before a provider starts. Feedback text reaches only the repaired member's prompt. `rounds.prompt` holds the free text or a fixed line, so it stays out of what apply shares to memory, and room events carry ids only. Status hangs on how rows point at each other, and those links live in columns. So the room's rows are checked as a set first: canonical unique ids, sequences exactly 1..N, each resolution naming an earlier request on its own candidate in column and body, no other room's row naming one. Any failure makes every request in the room unknown and stops the writers, the repair batch and the claim. A damaged body on an intact set stays the concern of its own request. Deleting the newest row leaves no gap and is not seen, as with judgments. Refs #173 Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 1 + docs/room-targeted-repair-plan.md | 2 +- docs/user-guide.ko.md | 37 +- docs/user-guide.md | 39 +- src/syncmill/cli.py | 245 +++++- src/syncmill/rooms.py | 1154 ++++++++++++++++++++++++++++- tests/test_room_feedback.py | 892 ++++++++++++++++++++++ tests/test_room_repair.py | 2 +- 8 files changed, 2351 insertions(+), 21 deletions(-) create mode 100644 tests/test_room_feedback.py diff --git a/CLAUDE.md b/CLAUDE.md index b0a63cf..80bc486 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,7 @@ uv run syncmill room export --include patch: [--include output:| # room rounds also record each member's conversation (session cold/resume/fork, checkpoint candidate ids + omitted, memory delivery; v2 adds lessons.selected ids + per-member lessons delivery, readers accept v1+v2) in rounds.conversation_json, written once under the lease before the fan-out (rooms.round_conversation_status binds it to SQLite rows); compare shows per-candidate facts and round counts only (plus the round's selected lesson ids) # 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 repair --prompt TEXT (--preview | --confirm TOKEN) [--cold] # #173 PR 1: one member runs from its newest completed candidate; the token is `RepairPlan.token` (rooms.py) over a plan rebuilt at confirm; `start_round(repair_guard=)` re-derives revision, parent rules and session in the claim's transaction; provenance v3 (repair rounds only, all-member rounds keep v2; `round_expected_members` narrows conversation records and `_missing_members`); the checkout is rebuilt only when it differs from the parent's patch; foreground only; design: docs/room-targeted-repair-plan.md +uv run syncmill room feedback add --kind change|question [--file PATH] --text TEXT # #173 PR 2: append-only room_feedback rows; `list ` / `resolve addressed|not-addressed|withdrawn [--on NEW]` / `carry --to NEW`; status derived by rooms.feedback_status (open/sent/addressed/not_addressed/withdrawn/unknown), never stored; `room repair` sends the open requests (or `--feedback ID`), `--prompt` optional; feedback text stays out of rounds.prompt, room_events and memory 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; a room created with `room create --goal G` (rooms.goal, verbatim, fixed) also gets scope==G lessons via project_lessons(goal=), marked `(goal G)`; 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 diff --git a/docs/room-targeted-repair-plan.md b/docs/room-targeted-repair-plan.md index 3077b56..c38d9e5 100644 --- a/docs/room-targeted-repair-plan.md +++ b/docs/room-targeted-repair-plan.md @@ -1,6 +1,6 @@ # Targeted room repair: design for #173 -Status: **APPROVED** 2026-11-09. PR 1 (section 9) is implemented; the rest is not. Baseline: +Status: **APPROVED** 2026-11-09. PR 1 and PR 2 (section 9) are implemented; the rest is not. Baseline: main @ `132e6c4`, before #273 changed what a retained worktree holds. Where that moved a decision, the section says so. The proposed answers in section 14 are the approved ones, except where decision 5 says otherwise. diff --git a/docs/user-guide.ko.md b/docs/user-guide.ko.md index e6e44a9..03230a4 100644 --- a/docs/user-guide.ko.md +++ b/docs/user-guide.ko.md @@ -749,7 +749,11 @@ SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room cand SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room compare [--round N | --all-rounds] [--json] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room export [--round N | --all-rounds] [--format markdown|json] [--include KIND:TARGET ... (--preview | --confirm TOKEN)] [--output FILE] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room diff <후보-id> -SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room repair <후보-id> --prompt "고칠 내용" (--preview | --confirm TOKEN) [--cold] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback add <후보-id> --kind change|question [--file PATH] (--text TEXT | --text-file PATH) +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback list [--candidate <후보-id>] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room repair <후보-id> [--prompt "고칠 내용"] [--feedback ID ...] (--preview | --confirm TOKEN) [--cold] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback resolve <피드백-id> addressed|not-addressed|withdrawn [--on <후보-id>] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback carry <피드백-id> --to <후보-id> SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room select <후보-id> [--expected-revision N] [--expected-digest SHA256] [--rationale TEXT | --rationale-file PATH] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room apply --verify [--expected-revision N] # 적용한 변경을 써 본 뒤: 받아들이는지 기록 @@ -1012,6 +1016,37 @@ syncmill room repair <후보-id> --prompt "고칠 내용" --confirm <토큰> 에이전트가 다시 설치할 때까지 실패할 수 있습니다. - `--detach`와 TUI에서는 아직 수리를 쓸 수 없어요. +프롬프트 하나 대신 후보에 리뷰 피드백을 먼저 남길 수도 있어요. 고칠 요청(change)과 +답을 듣고 싶은 질문(question)이고, 각각 후보가 바꾼 파일 하나에 묶을 수 있습니다. +그러면 수리는 그 후보에 열려 있는 요청을 모두 보내고, `--prompt`를 주면 그것도 함께 +보내요. `--feedback ID`를 주면 지정한 요청만 보냅니다. + +```bash +syncmill room feedback add <후보-id> --kind change --file calc.py --text "예전 이름 유지" +syncmill room feedback add <후보-id> --kind question --text "캐시를 하나 더 둔 이유는?" +syncmill room feedback list +syncmill room repair <후보-id> --preview +``` + +피드백을 쓰는 일은 provider를 부르지 않고, 방의 상태도 revision도 바꾸지 않아요. 그 +텍스트는 수리되는 멤버의 프롬프트에만 들어가고, 라운드에 저장되는 프롬프트, 방의 이벤트 +로그, 공유 메모리에는 들어가지 않습니다(에이전트가 자기 출력에서 인용할 수는 있어요). +`feedback list`는 요청마다 상태를 보여 주는데, 매번 기록에서 다시 계산합니다. + +- `open`: 아직 보내지 않았거나, 실패한 수리로만 보냈어요. 다음 수리가 다시 보냅니다. +- `sent`: 이 후보의 수리가 완료되면서 받았어요. +- `addressed`, `not-addressed`, `withdrawn`: `feedback resolve`로 직접 기록한 결과예요. + 요청을 해결 처리하는 건 사람뿐이고, 에이전트 출력에서 해결했다는 주장을 읽지 + 않습니다. `addressed`와 `not-addressed`에는 확인한, 같은 멤버의 이후 후보를 + `--on`으로 지정해요. +- `unknown`: 믿을 수 없는 기록이에요. 예전 상태로 대신 읽지 않습니다. 방의 피드백 + 기록끼리의 연결이 깨지면(중간 기록이 빠졌거나, resolution이 엉뚱한 요청을 가리키면) + 그 방의 요청이 모두 `unknown`이 되고, 더는 피드백을 쓸 수 없어요. + +요청은 처음 쓴 후보에 머물러요. 수리로 더 새로운 후보가 생기면 예전 후보는 다시 고칠 +수 없으니, `feedback carry --to <새-id>`로 새 후보에 열린 요청으로 다시 씁니다. +파일 지정은 새 후보도 그 파일을 바꿨을 때만 유지돼요. + `room candidates`와 TUI 후보 화면에 `cache 85% (34.2k/40.1k)`처럼 캐시 적중률이 표시됩니다. diff --git a/docs/user-guide.md b/docs/user-guide.md index de90f2d..8199e43 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -792,7 +792,11 @@ SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room cand SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room compare [--round N | --all-rounds] [--json] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room export [--round N | --all-rounds] [--format markdown|json] [--include KIND:TARGET ... (--preview | --confirm TOKEN)] [--output FILE] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room diff -SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room repair --prompt "what to change" (--preview | --confirm TOKEN) [--cold] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback add --kind change|question [--file PATH] (--text TEXT | --text-file PATH) +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback list [--candidate ] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room repair [--prompt "what to change"] [--feedback ID ...] (--preview | --confirm TOKEN) [--cold] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback resolve addressed|not-addressed|withdrawn [--on ] +SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room feedback carry --to SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room select [--expected-revision N] [--expected-digest SHA256] [--rationale TEXT | --rationale-file PATH] SYNCMILL_ROOMS__ENABLED=true uv run --project "$SYNCMILL_SRC" syncmill room apply --verify [--expected-revision N] # after you have lived with the applied change: record whether you accept it @@ -1110,6 +1114,39 @@ their candidates stay as they were, and your source is not touched. dependencies. A gate that relies on them can fail until the agent reinstalls them. - `--detach` and the TUI do not offer repair yet. +Instead of one prompt, you can write review feedback on the candidate first: change +requests, and questions you want answered, each optionally tied to a file the candidate +changed. A repair then sends every open request on that candidate, plus `--prompt` if you +give one; `--feedback ID` sends only the requests you name. + +```bash +syncmill room feedback add --kind change --file calc.py --text "keep the old name" +syncmill room feedback add --kind question --text "why a second cache?" +syncmill room feedback list +syncmill room repair --preview +``` + +Writing feedback calls no provider and changes neither the room's state nor its revision. +Its text goes only to the repaired member's prompt: not to the round's stored prompt, the +room's event log or shared memory (an agent may still quote it in its own output). +`feedback list` shows each request's status, worked out from the records each time: + +- `open`: not sent yet, or only sent by repairs that failed. The next repair sends it again. +- `sent`: a completed repair of this candidate received it. +- `addressed`, `not-addressed`, `withdrawn`: what you recorded with `feedback resolve`. + Only you resolve a request; nothing reads an agent's output for a claim that it did. + `addressed` and `not-addressed` name the later candidate of the same member you + checked (`--on`). +- `unknown`: a record that cannot be trusted. It is never read as an older status. If + the links between the room's feedback records are damaged (a record missing from the + middle, a resolution pointing at the wrong request), every request in the room reads + `unknown` and no more feedback can be written there. + +A request stays on the candidate it was written for. Once a repair produced a newer +candidate, the old one cannot be repaired again: `feedback carry --to ` +writes the request again on the new candidate as an open request, and keeps the file only +if the new candidate still changes it. + When a provider reports token usage, `room candidates` and the TUI candidate views show it — e.g. `cache 85% (34.2k/40.1k)` — so you can see the cache hit-rate per member per round. diff --git a/src/syncmill/cli.py b/src/syncmill/cli.py index fb97eec..3057bfd 100644 --- a/src/syncmill/cli.py +++ b/src/syncmill/cli.py @@ -128,6 +128,7 @@ ) from syncmill.rooms import ( _ROOM_PROMPT_MAX_BYTES, + FEEDBACK_KINDS, JUDGMENT_VERDICTS, RepairPlan, RoomCoordinator, @@ -3146,6 +3147,14 @@ def room_round( help="Run this repair without any approved project lessons. A resumed " "conversation keeps what earlier rounds sent it.", ) +@click.option( + "--feedback", + "feedback_ids", + multiple=True, + metavar="ID", + help="Send only this open feedback request (repeatable). Default: every open " + "request on the candidate.", +) def room_repair( candidate_id: str, prompt: str | None, @@ -3156,14 +3165,17 @@ def room_repair( repo: Path, skip_lesson: tuple[str, ...] = (), no_lessons: bool = False, + feedback_ids: tuple[str, ...] = (), ) -> None: """Run only the member that produced CANDIDATE_ID, from that candidate. - The result is a new candidate, verified again, that you compare and select - yourself. Other members, their candidates and the source are not touched.""" + It sends the candidate's open feedback requests (`room feedback add`) and + the optional --prompt. The result is a new candidate, verified again, that + you compare and select yourself. Other members, their candidates and the + source are not touched.""" if preview == (confirm is not None): raise click.UsageError("room repair needs exactly one of --preview and --confirm") - prompt = _resolve_prompt(prompt, prompt_file, required=True) + prompt = _resolve_prompt(prompt, prompt_file, required=False) _, config, coordinator = _room_coordinator(repo) try: parent = coordinator.store.get_candidate(candidate_id) @@ -3178,7 +3190,11 @@ def room_repair( cold=cold, ) plan = coordinator.build_repair_plan( - candidate_id, str(prompt), cold=cold, lessons=lessons + candidate_id, + prompt, + feedback_ids=list(feedback_ids) or None, + cold=cold, + lessons=lessons, ) if preview: _print_repair_plan(plan) @@ -3223,6 +3239,17 @@ def _print_repair_plan(plan: RepairPlan) -> None: else: for command in plan.gate["commands"]: echo(f"Verification: {_shown(command)}") + if plan.feedback: + changes = sum(1 for item in plan.feedback if item["kind"] == "change") + questions = len(plan.feedback) - changes + echo( + f"Feedback: {changes} change request(s), {questions} question(s); " + "the text is in `room feedback list`" + ) + for item in plan.feedback: + echo(f" {_shown(item['id'])} {_shown(item['kind'])}") + else: + echo("Feedback: none; the prompt alone says what to change") echo(f"Prompt: {len(plan.agent_task.encode('utf-8', 'surrogateescape'))} bytes") echo("Starts from: this candidate's patch. The checkout is rebuilt only if it differs,") echo(" which loses its ignored files, such as installed dependencies.") @@ -3678,6 +3705,216 @@ def room_judge( echo("Rationale: " + ("recorded" if record["rationale"] else "none")) +@room_group.group("feedback") +def room_feedback_group() -> None: + """Review feedback on one candidate: change requests and questions that + `room repair` sends to the member that produced it. No provider call.""" + + +def _feedback_text(text: str | None, text_file: Path | None) -> str: + if (text is None) == (text_file is None): + raise click.UsageError("give exactly one of --text and --text-file") + if text_file is None: + return str(text) + try: + # Decoded from bytes so that \r reaches the control-character check. + return _read_bounded_utf8(text_file) + except (OSError, UnicodeDecodeError) as exc: + raise click.BadParameter( + f"cannot read {printable_name(str(text_file))}: {exc}", + param_hint="--text-file", + ) from exc + + +def _feedback_status_label(status: str) -> str: + return status.replace("_", "-") + + +@room_feedback_group.command("add") +@click.argument("candidate_id") +@click.option("--kind", required=True, type=click.Choice(sorted(FEEDBACK_KINDS))) +@click.option( + "--file", "path", default=None, metavar="PATH", + help="Anchor the request to a file this candidate changed, as `room compare` lists it.", +) +@click.option("--text", default=None, help="The request or question (your own words).") +@click.option( + "--text-file", type=click.Path(path_type=Path, dir_okay=False), default=None, + help="Read the text from a UTF-8 file.", +) +@click.option( + "--repo", default=".", type=click.Path(path_type=Path), help=_PROJECT_REPO_HELP +) +def room_feedback_add( + candidate_id: str, + kind: str, + path: str | None, + text: str | None, + text_file: Path | None, + repo: Path, +) -> None: + """Write a change request or a question on CANDIDATE_ID. + + Changes neither the room's state nor its revision, and is never shared to + memory or written to the room's event log.""" + body = _feedback_text(text, text_file) + _, _, coordinator = _room_coordinator(repo) + try: + record = coordinator.add_feedback( + candidate_id, kind, body, path=path, surface="cli" + ) + except RoomError as exc: + raise click.ClickException(printable_name(str(exc))) from exc + finally: + coordinator.close() + label = "change request" if record["kind"] == "change" else "question" + anchor = record["anchor"] + echo( + f"Recorded {label} {_shown(record['feedback_id'])} on candidate " + f"{_shown(record['candidate_id'])}" + + (f" (file: {_shown(anchor['path'])})" if anchor else "") + ) + echo( + "Send it with: " + + printable_command( + "syncmill", "room", "repair", args=[record["candidate_id"], "--preview"] + ) + ) + + +@room_feedback_group.command("list") +@click.argument("room_id") +@click.option("--candidate", "candidate_id", default=None, metavar="CAND", + help="Only requests written on this candidate.") +@click.option( + "--repo", default=".", type=click.Path(path_type=Path), help=_PROJECT_REPO_HELP +) +def room_feedback_list(room_id: str, candidate_id: str | None, repo: Path) -> None: + """List a room's feedback requests with their derived status.""" + _, _, coordinator = _room_coordinator(repo) + try: + view = coordinator.store.feedback(room_id) + except RoomError as exc: + raise click.ClickException(printable_name(str(exc))) from exc + finally: + coordinator.close() + # Keyed as the checks are: a damaged store can hold a NULL id. + rows = {str(row.get("id")): row for row in view.rows} + shown = 0 + for feedback_id, check in view.checks.items(): + row_candidate = rows[feedback_id].get("candidate_id") + if candidate_id is not None and row_candidate != candidate_id: + continue + shown += 1 + request = check.request + if request is None: + echo( + f"{_shown(feedback_id)} {_shown(row_candidate)} unknown: " + f"{_shown(check.reason)}" + ) + continue + anchor = request["anchor"] + line = ( + f"{_shown(feedback_id)} {_shown(request['candidate_id'])} " + f"{_shown(request['kind']):8} {_shown(_feedback_status_label(check.status)):13} " + + (f"file: {_shown(anchor['path'])}" if anchor else "file: none") + ) + if request["carried_from"]: + line += f" carried from {_shown(request['carried_from'])}" + echo(line) + if check.status == "unknown": + # A record that cannot be trusted shows why, never its text. + echo(f" {_shown(check.reason)}") + continue + if check.resolution is not None and check.resolution["evaluated_candidate_id"]: + echo(f" on candidate {_shown(check.resolution['evaluated_candidate_id'])}") + for text_line in printable_text(request["text"]).split("\n"): + echo(f" | {text_line}") + if not shown: + echo(f"No feedback in room {_shown(room_id)}" + ( + f" on candidate {_shown(candidate_id)}" if candidate_id else "" + )) + for warning in view.warnings: + echo(f"Warning: {_shown(warning)}") + + +@room_feedback_group.command("resolve") +@click.argument("feedback_id") +@click.argument( + "status", type=click.Choice(["addressed", "not-addressed", "withdrawn"]) +) +@click.option( + "--on", "on", default=None, metavar="CAND", + help="The later candidate of the same member you evaluated " + "(needed for addressed and not-addressed).", +) +@click.option( + "--repo", default=".", type=click.Path(path_type=Path), help=_PROJECT_REPO_HELP +) +def room_feedback_resolve(feedback_id: str, status: str, on: str | None, repo: Path) -> None: + """Record whether a later candidate met FEEDBACK_ID, or withdraw it. + + Only you resolve a request: nothing reads an agent's output for a claim + of having addressed it.""" + _, _, coordinator = _room_coordinator(repo) + try: + record, created = coordinator.resolve_feedback( + feedback_id, status.replace("-", "_"), on=on, surface="cli" + ) + except RoomError as exc: + raise click.ClickException(printable_name(str(exc))) from exc + finally: + coordinator.close() + label = _feedback_status_label(record["status"]) + where = ( + f" on candidate {_shown(record['evaluated_candidate_id'])}" + if record["evaluated_candidate_id"] + else "" + ) + if not created: + echo(f"Already recorded: {_shown(feedback_id)} {label}{where}; nothing changed") + return + echo(f"Recorded {_shown(feedback_id)} {label}{where}") + + +@room_feedback_group.command("carry") +@click.argument("feedback_id") +@click.option("--to", "to", required=True, metavar="CAND", + help="A later completed candidate of the same member.") +@click.option( + "--repo", default=".", type=click.Path(path_type=Path), help=_PROJECT_REPO_HELP +) +def room_feedback_carry(feedback_id: str, to: str, repo: Path) -> None: + """Write FEEDBACK_ID again on a later candidate, as a new open request. + + Requests stay on the candidate they were written against; this is how one + moves on. The file anchor is kept only when the later candidate still + changes that file.""" + _, _, coordinator = _room_coordinator(repo) + try: + carried = coordinator.carry_feedback(feedback_id, to, surface="cli") + except RoomError as exc: + raise click.ClickException(printable_name(str(exc))) from exc + finally: + coordinator.close() + record = carried.record + prefix = "Already carried" if not carried.created else "Carried" + echo( + f"{prefix} {_shown(feedback_id)} to candidate {_shown(record['candidate_id'])} " + f"as {_shown(record['feedback_id'])}" + ) + source = carried.source_anchor + if source is None: + echo("File: none") + elif record["anchor"] is not None: + echo(f"File: {_shown(record['anchor']['path'])}, kept") + else: + echo( + f"File: none; {_shown(source['path'])} is not among the files candidate " + f"{_shown(record['candidate_id'])} changed" + ) + + @room_group.command("promote") @click.argument("room_id") @click.option("--lesson", default=None, help="The short lesson to reuse (your own words).") diff --git a/src/syncmill/rooms.py b/src/syncmill/rooms.py index ae6303a..95d5fd1 100644 --- a/src/syncmill/rooms.py +++ b/src/syncmill/rooms.py @@ -330,8 +330,8 @@ class RepairPlan: agent_task_sha256: str lesson_ids: tuple[str, ...] gate: dict[str, Any] - #: Each entry binds a request's id, record digest and status. Requests - #: arrive with the feedback store, so a plan holds none yet. + #: Each entry binds a request's id, record digest and status, in + #: sequence order; the round's provenance names the same ids. feedback: tuple[dict[str, str], ...] = () provider_calls: int = 1 @@ -370,13 +370,22 @@ def _text_sha256(value: str) -> str: return sha256(value.encode("utf-8", errors="surrogateescape")).hexdigest() -def render_repair_task(candidate_id: str, prompt: str) -> str: - """What a repair asks of its member. Deterministic, because the plan binds - its digest and a preview and its confirmation must agree on the bytes.""" +def render_repair_task( + candidate_id: str, + prompt: str | None, + requests: list[dict[str, Any]] | tuple[dict[str, Any], ...] = (), +) -> str: + """What a repair asks of its member: the feedback requests, then the + optional free text. Deterministic, because the plan binds its digest and a + preview and its confirmation must agree on the bytes. Without requests it + is the opening lines and the prompt alone.""" + parts = [render_feedback_batch(requests)] if requests else [] + if prompt: + parts.append(prompt) return ( f"Repair your candidate {candidate_id}. Your working tree holds exactly " "that candidate's patch. Ignored files, such as installed dependencies, " - f"may be missing.\n\n{prompt}" + "may be missing.\n\n" + "\n\n".join(parts) ) @@ -765,6 +774,531 @@ def outcome(status: str, reason: str | None = None, **extra: Any) -> OutcomeChec +# ------------------------------------------------------------ feedback (#173) +# +# Review feedback a human writes on one candidate: a change request or a +# question, optionally anchored to a file the candidate changed. Append-only, +# like judgments: a request is never edited, and a later resolution supersedes +# an earlier one. Status is derived from these rows and the room's rounds, +# never stored, by the one validator below. + +FEEDBACK_KINDS = frozenset({"change", "question"}) +FEEDBACK_RESOLUTIONS = frozenset({"addressed", "not_addressed", "withdrawn"}) +#: Every status :func:`feedback_status` reports. ``unknown`` refuses to guess. +FEEDBACK_STATUSES = ( + "open", + "sent", + "addressed", + "not_addressed", + "withdrawn", + "unknown", +) +FEEDBACK_TEXT_MAX_BYTES = 8 * 1024 +_FEEDBACK_SCHEMA_VERSION = 1 +#: Listed, not derived from _TERMINAL_ROOM_STATES: a room that is running, +#: applying or awaiting recovery takes no feedback until it settles. +_FEEDBACK_WRITABLE_STATES = frozenset({"awaiting_decision", "selected"}) +_FEEDBACK_REQUEST_KEYS = frozenset( + { + "schema_version", + "feedback_id", + "room_id", + "candidate_id", + "patch_sha256", + "kind", + "text", + "anchor", + "carried_from", + "surface", + "sequence", + "created_at", + } +) +_FEEDBACK_RESOLUTION_KEYS = frozenset( + { + "schema_version", + "feedback_id", + "room_id", + "request_id", + "status", + "evaluated_candidate_id", + "evaluated_patch_sha256", + "surface", + "sequence", + "created_at", + } +) + + +def normalize_feedback_text(value: object) -> str: + """Feedback text in stored form: control characters other than newline + and tab refused, ends stripped, at most 8 KiB. Empty is refused.""" + text = _normalize_human_text(value, "feedback text", FEEDBACK_TEXT_MAX_BYTES) + if text is None: + raise RoomError("feedback text must not be empty") + return text + + +@dataclasses.dataclass(frozen=True) +class FeedbackCheck: + #: one of FEEDBACK_STATUSES + status: str + reason: str | None = None + #: The request's record, when it validates (also for ``unknown`` caused + #: by a damaged resolution, so a listing can still name its candidate). + request: dict[str, Any] | None = None + #: The resolution that decides the status, when one does. + resolution: dict[str, Any] | None = None + + +@dataclasses.dataclass(frozen=True) +class FeedbackView: + """A room's feedback as read in one snapshot.""" + + room: dict[str, Any] + candidates: dict[str, dict[str, Any]] + #: Every room_feedback row, oldest first. + rows: list[dict[str, Any]] + #: Each request row's status, by id, in sequence order. + checks: dict[str, FeedbackCheck] + #: Repair rounds whose feedback ids do not name a request they repaired. + warnings: list[str] + #: Why the room's records cannot be believed as a set, or None. + problem: str | None = None + + +@dataclasses.dataclass(frozen=True) +class CarriedFeedback: + record: dict[str, Any] + created: bool + #: The source request's anchor, so a caller can say whether it was kept. + source_anchor: dict[str, str] | None + + +def _feedback_record(raw: object) -> dict[str, Any] | str: + """A stored record decoded, or why it cannot be.""" + if not isinstance(raw, str): + return "feedback record is not text" + try: + record = json.loads(raw) + except (ValueError, RecursionError): + return "feedback record is not valid JSON" + if not isinstance(record, dict): + return "feedback record is not an object" + version = record.get("schema_version") + if not _plain_int(version) or version != _FEEDBACK_SCHEMA_VERSION: + return f"unsupported feedback record version {version!r}" + return record + + +def _feedback_common_problem( + row: dict[str, Any], record: dict[str, Any], room_id: object +) -> str | None: + if record["feedback_id"] != row.get("id"): + return "feedback record id differs from its row" + if record["room_id"] != room_id or row.get("room_id") != room_id: + return "feedback record names a different room" + sequence = record["sequence"] + if not _plain_int(sequence) or sequence < 1 or sequence != row.get("sequence"): + return "feedback record sequence differs from its row" + surface = record["surface"] + if not isinstance(surface, str) or surface not in SELECTION_SURFACES: + return f"feedback record surface {surface!r} is unknown" + if not isinstance(record["created_at"], str) or not record["created_at"]: + return "feedback record created_at is malformed" + return None + + +def _feedback_request_problem( + row: dict[str, Any], + record: dict[str, Any], + *, + room_id: object, + candidates: dict[str, dict[str, Any]], +) -> str | None: + """Why a stored request cannot be believed, or ``None``. Anchor membership + in the candidate's files was checked when it was written; candidate + artifacts are immutable, so only its shape is checked here.""" + if row.get("record_type") != "request" or row.get("request_id") is not None: + return "feedback row is not a request" + if set(record) != _FEEDBACK_REQUEST_KEYS: + return "feedback request keys are not the v1 set" + problem = _feedback_common_problem(row, record, room_id) + if problem is not None: + return problem + candidate_id = record["candidate_id"] + if candidate_id != row.get("candidate_id"): + return "feedback request names a different candidate than its row" + candidate = candidates.get(candidate_id) if isinstance(candidate_id, str) else None + if candidate is None: + return "feedback request's candidate is not in this room" + if candidate.get("state") != "completed": + return "feedback request's candidate is not a completed candidate" + if record["patch_sha256"] != candidate.get("patch_sha256"): + return "feedback request's patch digest differs from the candidate's" + kind = record["kind"] + if not isinstance(kind, str) or kind not in FEEDBACK_KINDS: + return f"feedback request kind {kind!r} is unknown" + try: + if normalize_feedback_text(record["text"]) != record["text"]: + return "feedback request text is not in stored form" + except RoomError as exc: + return f"feedback request text is invalid: {exc}" + anchor = record["anchor"] + if anchor is not None and not ( + isinstance(anchor, dict) + and set(anchor) == {"path"} + and isinstance(anchor["path"], str) + and anchor["path"] + ): + return "feedback request anchor is malformed" + carried = record["carried_from"] + if carried is not None and not _canonical_uuid(carried): + return "feedback request carried_from is malformed" + return None + + +def _feedback_resolution_problem( + row: dict[str, Any], + record: dict[str, Any], + *, + request_row: dict[str, Any], + request: dict[str, Any], + room_id: object, + candidates: dict[str, dict[str, Any]], +) -> str | None: + """Why a stored resolution cannot decide its request's status, or ``None``. + The writer runs the same check on the record it is about to insert.""" + if row.get("record_type") != "resolution": + return "feedback row is not a resolution" + if set(record) != _FEEDBACK_RESOLUTION_KEYS: + return "feedback resolution keys are not the v1 set" + problem = _feedback_common_problem(row, record, room_id) + if problem is not None: + return problem + if record["request_id"] != request["feedback_id"] or row.get( + "request_id" + ) != request["feedback_id"]: + return "feedback resolution names a different request" + if row.get("candidate_id") != request["candidate_id"]: + return "feedback resolution row names a different candidate than its request" + if record["sequence"] <= request_row.get("sequence", 0): + return "feedback resolution is not after its request" + status = record["status"] + if not isinstance(status, str) or status not in FEEDBACK_RESOLUTIONS: + return f"feedback resolution status {status!r} is unknown" + evaluated_id = record["evaluated_candidate_id"] + digest = record["evaluated_patch_sha256"] + if status == "withdrawn": + if evaluated_id is not None or digest is not None: + return "a withdrawal names an evaluated candidate" + return None + origin = candidates[request["candidate_id"]] + evaluated = candidates.get(evaluated_id) if isinstance(evaluated_id, str) else None + if evaluated is None: + return "feedback resolution's evaluated candidate is not in this room" + if evaluated.get("member_id") != origin.get("member_id"): + return "feedback resolution's evaluated candidate is another member's" + if not ( + _plain_int(evaluated.get("round_number")) + and _plain_int(origin.get("round_number")) + and evaluated["round_number"] > origin["round_number"] + ): + return "feedback resolution's evaluated candidate is not from a later round" + if evaluated.get("state") != "completed": + return "feedback resolution's evaluated candidate is not a completed candidate" + if digest != evaluated.get("patch_sha256"): + return "feedback resolution's patch digest differs from the evaluated candidate's" + return None + + +def _repair_rounds( + rounds: list[dict[str, Any]], + *, + member_ids: list[str], + candidates: dict[str, dict[str, Any]], +) -> tuple[list[tuple[int, dict[str, Any]]], list[tuple[object, str]]]: + """Each round whose provenance is a recorded repair, as its number and + tree; and each round whose provenance is stored but cannot be believed, + as its number and raw text. A damaged repair record cannot say which + requests it sent, so a caller treats every id its text holds as named.""" + found: list[tuple[int, dict[str, Any]]] = [] + untrusted: list[tuple[object, str]] = [] + for row in rounds: + number = row.get("number") + raw = row.get("provenance_json") + if not _plain_int(number): + if isinstance(raw, str): + untrusted.append((number, raw)) + continue + check = round_provenance_status( + raw, + round_number=number, + member_ids=member_ids, + candidates=candidates, + ) + if check.status == "recorded" and check.record is not None: + tree = check.record["tree"] + if tree.get("kind") == "repair": + found.append((number, tree)) + elif check.status == "malformed" and isinstance(raw, str): + untrusted.append((number, raw)) + return found, untrusted + + +def _repair_round_completed( + number: int, tree: dict[str, Any], candidates: dict[str, dict[str, Any]] +) -> bool: + return any( + row.get("round_number") == number + and row.get("member_id") == tree["member_id"] + and row.get("state") == "completed" + for row in candidates.values() + ) + + +def feedback_status( + request_row: dict[str, Any], + resolution_rows: list[dict[str, Any]], + *, + room_id: object, + candidates: dict[str, dict[str, Any]], + rounds: list[dict[str, Any]], + member_ids: list[str], +) -> FeedbackCheck: + """One request's status, from durable records only. + + ``resolution_rows`` are the rows whose ``request_id`` column names this + request, in any order. ``candidates`` and ``rounds`` are the room's, read + in the same snapshot. The latest resolution decides, and only when it + binds: a damaged latest record is ``unknown``, never an older status. + Without one, the request is ``sent`` once a repair round of its own + candidate names it and produced a completed candidate, else ``open``. + """ + parsed = _feedback_record(request_row.get("record_json")) + if isinstance(parsed, str): + return FeedbackCheck("unknown", parsed) + problem = _feedback_request_problem( + request_row, parsed, room_id=room_id, candidates=candidates + ) + if problem is None and candidates[parsed["candidate_id"]].get("member_id") not in member_ids: + problem = "feedback request's candidate belongs to no member of this room" + if problem is not None: + return FeedbackCheck("unknown", problem) + request = parsed + if resolution_rows: + # The room's record set was checked first (feedback_inventory_problem), + # so every sequence here is a distinct plain int. + latest_row = max(resolution_rows, key=lambda row: row["sequence"]) + resolution = _feedback_record(latest_row.get("record_json")) + if isinstance(resolution, str): + return FeedbackCheck("unknown", f"latest resolution: {resolution}", request) + problem = _feedback_resolution_problem( + latest_row, + resolution, + request_row=request_row, + request=request, + room_id=room_id, + candidates=candidates, + ) + if problem is not None: + return FeedbackCheck("unknown", f"latest resolution: {problem}", request) + return FeedbackCheck(resolution["status"], None, request, resolution) + repairs, untrusted = _repair_rounds( + rounds, member_ids=member_ids, candidates=candidates + ) + for number, raw in untrusted: + if request["feedback_id"] in raw: + return FeedbackCheck( + "unknown", + f"round {number}'s record names this request but cannot be believed", + request, + ) + sent = False + for number, tree in repairs: + if request["feedback_id"] not in tree["feedback_ids"]: + continue + if tree["parent_candidate_id"] != request["candidate_id"]: + return FeedbackCheck( + "unknown", + f"repair round {number} names this request but repaired another candidate", + request, + ) + if _repair_round_completed(number, tree, candidates): + sent = True + return FeedbackCheck("sent" if sent else "open", None, request) + + +def feedback_inventory_problem( + feedback_rows: list[dict[str, Any]], *, foreign_references: int = 0 +) -> str | None: + """Why a room's feedback records cannot be believed as a set, or ``None``. + + Status hangs on how rows point at each other, and those links live in + columns a damaged store can change. So before any one request is read, + the set must be whole: canonical unique ids, sequences exactly 1..N + (the writer allocates them so; a gap is a deleted or moved row), each + row a request or a resolution, every resolution naming an earlier + request on its own candidate, in its column and its body alike, and no + row of another room naming one of these. Any failure makes every request + ``unknown``. A damaged body on an intact set stays the business of the + one request it belongs to. Removing the newest row leaves no gap and is + not seen, the limit judgments share. + + ``feedback_rows`` are the room's rows in sequence order; + ``foreign_references`` counts other rooms' rows whose ``request_id`` + names one of them.""" + if foreign_references: + return "a feedback row of another room names a request of this room" + seen: dict[str, dict[str, Any]] = {} + for position, row in enumerate(feedback_rows, 1): + feedback_id = row.get("id") + if not _canonical_uuid(feedback_id): + return f"feedback row {position} has a malformed id" + if feedback_id in seen: + return f"feedback id {feedback_id} is stored twice" + if not _plain_int(row.get("sequence")) or row["sequence"] != position: + return "feedback sequences are not 1..N; a record is missing, moved or renumbered" + if not isinstance(row.get("candidate_id"), str): + return f"feedback {feedback_id} names no candidate" + record_type = row.get("record_type") + request_id = row.get("request_id") + if record_type == "request": + if request_id is not None: + return f"feedback request {feedback_id} names a request of its own" + elif record_type == "resolution": + target = seen.get(request_id) if isinstance(request_id, str) else None + if target is None or target.get("record_type") != "request": + return f"feedback resolution {feedback_id} names no earlier request" + if row["candidate_id"] != target["candidate_id"]: + return ( + f"feedback resolution {feedback_id} is filed under another " + "candidate than its request" + ) + body = _feedback_record(row.get("record_json")) + if ( + isinstance(body, dict) + and "request_id" in body + and body["request_id"] != request_id + ): + return ( + f"feedback resolution {feedback_id} names one request in its row " + "and another in its record" + ) + else: + return f"feedback {feedback_id} is neither a request nor a resolution" + seen[feedback_id] = row + return None + + +def feedback_statuses( + feedback_rows: list[dict[str, Any]], + *, + room_id: object, + candidates: dict[str, dict[str, Any]], + rounds: list[dict[str, Any]], + member_ids: list[str], + foreign_references: int = 0, +) -> dict[str, FeedbackCheck]: + """:func:`feedback_status` for every request row of a room, by id. + + The room's records are checked as a set first + (:func:`feedback_inventory_problem`). If the set fails, every row that + could be a request reads ``unknown``, so none is dropped from a listing + and none lets an older or default status through.""" + problem = feedback_inventory_problem( + feedback_rows, foreign_references=foreign_references + ) + if problem is not None: + reason = f"the room's feedback records cannot be trusted: {problem}" + return { + str(row.get("id")): FeedbackCheck("unknown", reason) + for row in feedback_rows + if row.get("record_type") != "resolution" or row.get("request_id") is None + } + resolutions: dict[object, list[dict[str, Any]]] = {} + for row in feedback_rows: + if row.get("request_id") is not None: + resolutions.setdefault(row.get("request_id"), []).append(row) + return { + str(row["id"]): feedback_status( + row, + resolutions.get(row["id"], []), + room_id=room_id, + candidates=candidates, + rounds=rounds, + member_ids=member_ids, + ) + for row in feedback_rows + if row.get("request_id") is None + } + + +def repair_feedback_problems( + rounds: list[dict[str, Any]], + feedback_rows: list[dict[str, Any]], + *, + candidates: dict[str, dict[str, Any]], + member_ids: list[str], +) -> list[str]: + """Body-free warnings for repair rounds whose recorded feedback ids do not + name a request on the candidate they repaired. A request-side check never + sees an id that names no request, so this reads from the round's side.""" + by_id = {row.get("id"): row for row in feedback_rows} + problems: list[str] = [] + repairs, _ = _repair_rounds(rounds, member_ids=member_ids, candidates=candidates) + for number, tree in repairs: + for feedback_id in tree["feedback_ids"]: + row = by_id.get(feedback_id) + if row is None: + why = "which is not a request in this room" + elif row.get("record_type") != "request": + why = "which is a resolution, not a request" + elif row.get("candidate_id") != tree["parent_candidate_id"]: + why = "which is a request on another candidate" + else: + continue + problems.append(f"repair round {number} names feedback {feedback_id}, {why}") + return problems + + +def _render_feedback_item(index: int, request: dict[str, Any]) -> str: + anchor = request.get("anchor") + prefix = "" + if anchor is not None: + path = anchor["path"] + # json.dumps escapes every non-ASCII character, so a quoted path + # passes _validate_prompt_text whatever a captured path holds. + shown = ( + json.dumps(path) + if "]" in path or has_control_characters(path) + else path + ) + prefix = f"[{shown}] " + lines = request["text"].split("\n") + head = f"{index}. {prefix}{lines[0]}" + return "\n".join([head, *(" " + line if line else "" for line in lines[1:])]) + + +def render_feedback_batch(requests: list[dict[str, Any]] | tuple[dict[str, Any], ...]) -> str: + """The request part of a repair's task, change requests first, each kind + in the order given. Deterministic: the plan binds its digest.""" + sections: list[str] = [] + for kind, heading in ( + ("change", "Change requests:"), + ("question", "Questions (answer these in your response):"), + ): + items = [request for request in requests if request["kind"] == kind] + if items: + sections.append( + "\n".join( + [heading] + + [_render_feedback_item(i, item) for i, item in enumerate(items, 1)] + ) + ) + return "\n\n".join(sections) + + _PROVENANCE_SCHEMA_VERSION = 2 #: Written only by a repair round, which names one member and one parent. #: All-member rounds keep writing v2, whatever came before them in the room. @@ -1656,6 +2190,24 @@ def _create_schema(self) -> None: created_at TEXT NOT NULL, UNIQUE(room_id, sequence) ); + -- Review feedback on one candidate (#173). Append-only: a request + -- is never edited and a later resolution supersedes an earlier + -- one. request_id sits outside the JSON so that a resolution with + -- a damaged body can still be attributed. Status is derived on + -- read (feedback_status), never stored. + CREATE TABLE IF NOT EXISTS room_feedback ( + id TEXT PRIMARY KEY, + room_id TEXT NOT NULL REFERENCES rooms(id), + candidate_id TEXT NOT NULL REFERENCES candidates(id), + record_type TEXT NOT NULL, + request_id TEXT REFERENCES room_feedback(id), + sequence INTEGER NOT NULL, + record_json TEXT NOT NULL, + created_at TEXT NOT NULL, + UNIQUE(room_id, sequence) + ); + CREATE INDEX IF NOT EXISTS idx_room_feedback_candidate + ON room_feedback(room_id, candidate_id); CREATE INDEX IF NOT EXISTS idx_rooms_state ON rooms(state, created_at); CREATE INDEX IF NOT EXISTS idx_candidates_room ON candidates(room_id, created_at); CREATE INDEX IF NOT EXISTS idx_round_members ON round_members(round_id); @@ -1942,6 +2494,10 @@ def prune_rooms(self, room_ids: list[str]) -> list[str]: else: eligible = True if eligible: + # Before candidates, which it references. One statement + # covers its own request_id references: SQLite checks + # immediate foreign keys at the end of the statement. + conn.execute("DELETE FROM room_feedback WHERE room_id=?", (room_id,)) conn.execute("DELETE FROM candidates WHERE room_id=?", (room_id,)) conn.execute( "DELETE FROM room_workspaces WHERE room_id=?", (room_id,) @@ -2157,6 +2713,22 @@ def _repair_guard_problem( stored = cls._stored_session(conn, room, plan.member_id) if (None if stored is None else _text_sha256(stored)) != plan.session_sha256: return f"the member's provider session changed; {again}" + if plan.feedback: + view = cls._feedback_view(conn, room["id"]) + raw = {row["id"]: str(row["record_json"]) for row in view.rows} + for item in plan.feedback: + check = view.checks.get(item["id"]) + if ( + check is None + or check.status != item["status"] + or check.request is None + or check.request["candidate_id"] != plan.candidate_id + or _text_sha256(raw[item["id"]]) != item["record_sha256"] + ): + return ( + f"feedback {item['id']} changed since this repair was " + f"confirmed; {again}" + ) return None @staticmethod @@ -3333,6 +3905,471 @@ def applied(self, room_id: str, run_id: str) -> None: raise RoomError("room apply state changed before finalization") self._event(conn, room_id, "apply.completed", {"run_id": run_id}) + # ------------------------------------------------------------ feedback + + @classmethod + def _feedback_view(cls, conn: sqlite3.Connection, room_id: str) -> FeedbackView: + row = conn.execute("SELECT * FROM rooms WHERE id=?", (room_id,)).fetchone() + if row is None: + raise RoomError(f"unknown room: {room_id}") + room = cls._room(row) + member_ids = [ + member["member_id"] + for member in room["members"] + if isinstance(member, dict) and "member_id" in member + ] + candidates = cls._candidate_rows(conn, room_id) + rounds = [ + dict(r) + for r in conn.execute( + "SELECT * FROM rounds WHERE room_id=? ORDER BY number", (room_id,) + ).fetchall() + ] + rows = [ + dict(r) + for r in conn.execute( + "SELECT * FROM room_feedback WHERE room_id=? ORDER BY sequence", + (room_id,), + ).fetchall() + ] + foreign = int( + conn.execute( + """SELECT COUNT(*) FROM room_feedback WHERE room_id IS NOT ? + AND request_id IN (SELECT id FROM room_feedback WHERE room_id=?)""", + (room_id, room_id), + ).fetchone()[0] + ) + problem = feedback_inventory_problem(rows, foreign_references=foreign) + return FeedbackView( + room=room, + candidates=candidates, + rows=rows, + checks=feedback_statuses( + rows, + room_id=room_id, + candidates=candidates, + rounds=rounds, + member_ids=member_ids, + foreign_references=foreign, + ), + warnings=([problem] if problem else []) + + repair_feedback_problems( + rounds, rows, candidates=candidates, member_ids=member_ids + ), + problem=problem, + ) + + def feedback(self, room_id: str) -> FeedbackView: + """The room's feedback and each request's status, read in one snapshot.""" + with self._read_snapshot() as conn: + return self._feedback_view(conn, room_id) + + def feedback_room(self, feedback_id: str) -> str: + """The room a feedback request belongs to.""" + return self._feedback_room_of(self._conn, feedback_id) + + def feedback_count(self, room_id: str) -> int: + row = self._conn.execute( + "SELECT COUNT(*) FROM room_feedback WHERE room_id=?", (room_id,) + ).fetchone() + return int(row[0]) + + @staticmethod + def _feedback_room_of(conn: sqlite3.Connection, feedback_id: str) -> str: + row = conn.execute( + "SELECT room_id, record_type FROM room_feedback WHERE id=?", (feedback_id,) + ).fetchone() + if row is None: + raise RoomError(f"unknown feedback: {feedback_id}") + if row["record_type"] != "request": + raise RoomError(f"feedback {feedback_id} is a resolution, not a request") + return str(row["room_id"]) + + @staticmethod + def _feedback_writable(view: FeedbackView) -> None: + state = view.room["state"] + if state not in _FEEDBACK_WRITABLE_STATES: + raise RoomError( + f"room is {state}; feedback is written only while a room awaits a " + "decision or has a selected candidate" + ) + if view.problem is not None: + raise RoomError( + f"the room's feedback records cannot be trusted ({view.problem}); " + "nothing more is written to them" + ) + + @staticmethod + def _trusted_request(view: FeedbackView, request_id: str) -> FeedbackCheck: + check = view.checks.get(request_id) + if check is None: + raise RoomError(f"unknown feedback: {request_id}") + if check.status == "unknown" or check.request is None: + raise RoomError( + f"feedback {request_id} cannot be trusted ({check.reason}); " + "inspect it with `syncmill room feedback list`" + ) + return check + + def _insert_feedback( + self, + conn: sqlite3.Connection, + view: FeedbackView, + *, + record_type: str, + candidate_id: str, + body: dict[str, Any], + request: tuple[dict[str, Any], dict[str, Any]] | None = None, + ) -> dict[str, Any]: + """Validate a new record with the reader's own check, then insert it. + ``request`` is the (row, record) a resolution resolves.""" + room_id = view.room["id"] + sequence = int( + conn.execute( + "SELECT COALESCE(MAX(sequence), 0) + 1 FROM room_feedback WHERE room_id=?", + (room_id,), + ).fetchone()[0] + ) + now = _now() + feedback_id = str(uuid4()) + record = { + "schema_version": _FEEDBACK_SCHEMA_VERSION, + "feedback_id": feedback_id, + "room_id": room_id, + **body, + "sequence": sequence, + "created_at": now, + } + row = { + "id": feedback_id, + "room_id": room_id, + "candidate_id": candidate_id, + "record_type": record_type, + "request_id": None if request is None else request[1]["feedback_id"], + "sequence": sequence, + } + if request is None: + problem = _feedback_request_problem( + row, record, room_id=room_id, candidates=view.candidates + ) + else: + problem = _feedback_resolution_problem( + row, + record, + request_row=request[0], + request=request[1], + room_id=room_id, + candidates=view.candidates, + ) + if problem is not None: + raise RoomError(problem) + conn.execute( + """INSERT INTO room_feedback + (id,room_id,candidate_id,record_type,request_id,sequence,record_json,created_at) + VALUES (?,?,?,?,?,?,?,?)""", + ( + feedback_id, + room_id, + candidate_id, + record_type, + row["request_id"], + sequence, + json.dumps(record, ensure_ascii=False, sort_keys=True, separators=(",", ":")), + now, + ), + ) + # Body-free: feedback text never enters room_events. + if request is None: + self._event( + conn, + room_id, + "feedback.requested", + { + "feedback_id": feedback_id, + "candidate_id": candidate_id, + "kind": record["kind"], + "anchored": record["anchor"] is not None, + "carried_from": record["carried_from"], + "sequence": sequence, + "surface": record["surface"], + }, + ) + else: + self._event( + conn, + room_id, + "feedback.resolved", + { + "feedback_id": feedback_id, + "request_id": record["request_id"], + "status": record["status"], + "evaluated_candidate_id": record["evaluated_candidate_id"], + "sequence": sequence, + "surface": record["surface"], + }, + ) + return record + + def add_feedback( + self, + candidate_id: str, + kind: str, + text: str, + *, + path: str | None, + files_changed: list[str], + verified_patch_sha256: str, + surface: str = "api", + ) -> dict[str, Any]: + """Append a request on one completed candidate. ``files_changed`` and + ``verified_patch_sha256`` come from the artifact the caller verified; + the candidate row must still carry that digest. Changes neither the + room's state nor its revision.""" + if kind not in FEEDBACK_KINDS: + raise RoomError( + f"unknown feedback kind {kind!r}; use one of " + + ", ".join(sorted(FEEDBACK_KINDS)) + ) + if surface not in SELECTION_SURFACES: + raise RoomError(f"unknown feedback surface: {surface}") + text = normalize_feedback_text(text) + anchor: dict[str, str] | None = None + if path is not None: + if path not in files_changed: + raise RoomError( + f"{path} is not a file this candidate changed; --file takes a path " + "exactly as `syncmill room compare` lists it" + ) + anchor = {"path": path} + with self._tx() as conn: + found = conn.execute( + "SELECT room_id FROM candidates WHERE id=?", (candidate_id,) + ).fetchone() + if found is None: + raise RoomError(f"unknown candidate: {candidate_id}") + view = self._feedback_view(conn, str(found["room_id"])) + self._feedback_writable(view) + candidate = view.candidates[candidate_id] + if candidate["state"] != "completed": + raise RoomError( + f"candidate ended {candidate['state']}, so it holds no finished work " + "to review; feedback is written on completed candidates" + ) + if candidate["patch_sha256"] != verified_patch_sha256: + raise RoomError("candidate changed since its artifact was verified") + return self._insert_feedback( + conn, + view, + record_type="request", + candidate_id=candidate_id, + body={ + "candidate_id": candidate_id, + "patch_sha256": candidate["patch_sha256"], + "kind": kind, + "text": text, + "anchor": anchor, + "carried_from": None, + "surface": surface, + }, + ) + + def resolve_feedback( + self, + request_id: str, + status: str, + *, + evaluated_candidate_id: str | None = None, + verified_patch_sha256: str | None = None, + surface: str = "api", + ) -> tuple[dict[str, Any], bool]: + """Append a human resolution of a request. Returns ``(record, created)``; + repeating the current resolution is a no-op returning it.""" + if status not in FEEDBACK_RESOLUTIONS: + raise RoomError( + f"unknown resolution {status!r}; use one of " + + ", ".join(sorted(FEEDBACK_RESOLUTIONS)) + ) + if surface not in SELECTION_SURFACES: + raise RoomError(f"unknown feedback surface: {surface}") + if status == "withdrawn" and evaluated_candidate_id is not None: + raise RoomError("a withdrawal names no candidate; drop --on") + if status != "withdrawn" and evaluated_candidate_id is None: + raise RoomError( + "say which candidate you evaluated with --on CANDIDATE: a later " + "completed candidate of the same member" + ) + with self._tx() as conn: + view = self._feedback_view(conn, self._feedback_room_of(conn, request_id)) + self._feedback_writable(view) + check = self._trusted_request(view, request_id) + evaluated_digest: str | None = None + if evaluated_candidate_id is not None: + evaluated = view.candidates.get(evaluated_candidate_id) + if evaluated is None: + raise RoomError( + f"candidate {evaluated_candidate_id} is not a candidate of this room" + ) + if evaluated["patch_sha256"] != verified_patch_sha256: + raise RoomError("candidate changed since its artifact was verified") + evaluated_digest = evaluated["patch_sha256"] + current = check.resolution + if ( + current is not None + and current["status"] == status + and current["evaluated_candidate_id"] == evaluated_candidate_id + ): + return current, False + request_row = next(row for row in view.rows if row["id"] == request_id) + try: + record = self._insert_feedback( + conn, + view, + record_type="resolution", + candidate_id=check.request["candidate_id"], + body={ + "request_id": request_id, + "status": status, + "evaluated_candidate_id": evaluated_candidate_id, + "evaluated_patch_sha256": evaluated_digest, + "surface": surface, + }, + request=(request_row, check.request), + ) + except RoomError as exc: + raise RoomError(f"cannot resolve on that candidate: {exc}") from exc + return record, True + + def carry_feedback( + self, + request_id: str, + to_candidate_id: str, + *, + files_changed: list[str], + verified_patch_sha256: str, + surface: str = "api", + ) -> CarriedFeedback: + """Write a copy of a request on a later completed candidate of the same + member. The file anchor is kept only when ``files_changed`` (the + target's verified list) still holds its path. Carrying the same + request to the same candidate again returns the earlier copy.""" + if surface not in SELECTION_SURFACES: + raise RoomError(f"unknown feedback surface: {surface}") + with self._tx() as conn: + view = self._feedback_view(conn, self._feedback_room_of(conn, request_id)) + self._feedback_writable(view) + check = self._trusted_request(view, request_id) + source = check.request + if check.status == "withdrawn": + raise RoomError(f"feedback {request_id} was withdrawn; nothing to carry") + target = view.candidates.get(to_candidate_id) + if target is None: + raise RoomError(f"candidate {to_candidate_id} is not a candidate of this room") + if target["patch_sha256"] != verified_patch_sha256: + raise RoomError("candidate changed since its artifact was verified") + origin = view.candidates[source["candidate_id"]] + if target["member_id"] != origin["member_id"]: + raise RoomError( + "feedback is carried to a later candidate of the member it was " + "written for; write a new request with `room feedback add` instead" + ) + if not ( + _plain_int(target.get("round_number")) + and _plain_int(origin.get("round_number")) + and target["round_number"] > origin["round_number"] + ): + raise RoomError( + f"candidate {to_candidate_id} is not from a later round than the " + "request's candidate" + ) + if target["state"] != "completed": + raise RoomError( + f"candidate ended {target['state']}; feedback is carried to a " + "completed candidate" + ) + for other in view.checks.values(): + if ( + other.request is not None + and other.request["carried_from"] == request_id + and other.request["candidate_id"] == to_candidate_id + ): + if other.status == "unknown": + raise RoomError( + f"feedback {request_id} was already carried to that " + f"candidate, and the copy cannot be trusted ({other.reason})" + ) + return CarriedFeedback(other.request, False, source["anchor"]) + anchor = source["anchor"] + if anchor is not None and anchor["path"] not in files_changed: + anchor = None + record = self._insert_feedback( + conn, + view, + record_type="request", + candidate_id=to_candidate_id, + body={ + "candidate_id": to_candidate_id, + "patch_sha256": target["patch_sha256"], + "kind": source["kind"], + "text": source["text"], + "anchor": anchor, + "carried_from": request_id, + "surface": surface, + }, + ) + return CarriedFeedback(record, True, source["anchor"]) + + def repair_batch( + self, + room_id: str, + candidate_id: str, + feedback_ids: list[str] | tuple[str, ...] | None, + ) -> list[tuple[dict[str, Any], str]]: + """The requests a repair of this candidate sends, as (record, + record_json) in sequence order: every ``open`` request on it, or + exactly ``feedback_ids``, each of which must be one of those.""" + view = self.feedback(room_id) + if view.problem is not None: + raise RoomError( + f"the room's feedback records cannot be trusted ({view.problem}); " + "repair with --prompt only after they are fixed, or start a new room" + ) + raw = {row["id"]: str(row["record_json"]) for row in view.rows} + if feedback_ids is None: + chosen = { + feedback_id + for feedback_id, check in view.checks.items() + if check.status == "open" + and check.request is not None + and check.request["candidate_id"] == candidate_id + } + else: + if len(set(feedback_ids)) != len(feedback_ids): + raise RoomError("a feedback id is listed twice") + for feedback_id in feedback_ids: + check = view.checks.get(feedback_id) + if check is None: + raise RoomError(f"{feedback_id} is not a feedback request in this room") + if check.request is None or check.status == "unknown": + raise RoomError( + f"feedback {feedback_id} cannot be trusted ({check.reason})" + ) + if check.request["candidate_id"] != candidate_id: + raise RoomError( + f"feedback {feedback_id} is a request on candidate " + f"{check.request['candidate_id']}, not on {candidate_id}" + ) + if check.status != "open": + raise RoomError( + f"feedback {feedback_id} is {check.status}; a repair sends open " + "requests only" + ) + chosen = set(feedback_ids) + return [ + (check.request, raw[feedback_id]) + for feedback_id, check in view.checks.items() + if feedback_id in chosen and check.request is not None + ] + def list_judgments(self, room_id: str) -> list[dict[str, Any]]: """The room's judgment rows, oldest first (raw; validate before use).""" rows = self._conn.execute( @@ -3898,21 +4935,33 @@ def is_repair(row: dict[str, Any]) -> bool: def build_repair_plan( self, candidate_id: str, - prompt: str, + prompt: str | None, *, + feedback_ids: list[str] | tuple[str, ...] | None = None, cold: bool = False, lessons: list[RoundLesson] | tuple[RoundLesson, ...] | None = None, ) -> RepairPlan: """The plan for repairing one candidate as of now. Read-only: it starts nothing, and a repair that could only fail after the round's lease is - refused here instead, so such a repair cannot freeze the room.""" + refused here instead, so such a repair cannot freeze the room. + + It sends the candidate's ``open`` feedback requests, or exactly + ``feedback_ids``, and the optional free-text ``prompt``. The round + stores only the free text (or a fixed line naming the candidate): + feedback text stays out of ``rounds.prompt``, which apply shares.""" room_id = str(self.store.get_candidate(candidate_id)["room_id"]) problem = self.store.repair_parent_problem(room_id, candidate_id) if problem is not None: raise RoomError(problem) - if not prompt.strip(): - raise RoomError("a repair needs a prompt saying what to change") - room = self.preflight_round(room_id, prompt) + free_text = prompt if prompt is not None and prompt.strip() else None + batch = self.store.repair_batch(room_id, candidate_id, feedback_ids) + if not batch and free_text is None: + raise RoomError( + f"nothing to repair: candidate {candidate_id} has no open feedback " + "and no prompt was given" + ) + round_prompt = free_text if free_text is not None else f"Repair candidate {candidate_id}" + room = self.preflight_round(room_id, round_prompt) parent = self.store.candidate_rows(room_id)[candidate_id] member_id = parent["member_id"] try: @@ -3929,7 +4978,9 @@ def build_repair_plan( "candidate's patch records an embedded repository, which a rebuild " "cannot restore, so it cannot be repaired" ) - agent_task = render_repair_task(candidate_id, prompt) + agent_task = render_repair_task( + candidate_id, free_text, [request for request, _ in batch] + ) # round_lessons_fit answers True when there are no lessons, and an # oversized task would otherwise first fail after the lease is held. _validate_prompt_text(agent_task) @@ -3958,11 +5009,20 @@ def build_repair_plan( cold=cold, session_mode="resume" if conversations and stored and not cold else "cold", session_sha256=None if stored is None else _text_sha256(stored), - prompt=prompt, + prompt=round_prompt, agent_task=agent_task, agent_task_sha256=_text_sha256(agent_task), lesson_ids=tuple(lesson.id for lesson in selected), gate=_gate_settings_record(self.config.gate), + feedback=tuple( + { + "id": request["feedback_id"], + "kind": request["kind"], + "record_sha256": _text_sha256(raw), + "status": "open", + } + for request, raw in batch + ), ) def _continued_parents(self, room: dict[str, Any]) -> dict[str, dict[str, Any] | None]: @@ -4942,6 +6002,74 @@ def judge( surface=surface, ) + def _own_room(self, room_id: str) -> None: + recorded_room = self.store.get_room(room_id) + if Path(recorded_room["repo_root"]).resolve() != self.repo_root: + raise RoomError("room belongs to a different repository") + + def _verified_files(self, candidate_id: str) -> tuple[str, list[str]]: + """A candidate's verified patch digest and its manifest's changed files.""" + self._own_room(str(self.store.get_candidate(candidate_id)["room_id"])) + digests = self.store.verify_candidate_artifact(candidate_id) + files = self.store.load_candidate_manifest(candidate_id).get("files_changed") + if not isinstance(files, list) or not all(isinstance(f, str) for f in files): + raise RoomError(f"candidate's changed-file list is malformed: {candidate_id}") + return digests["patch_sha256"], files + + def add_feedback( + self, + candidate_id: str, + kind: str, + text: str, + *, + path: str | None = None, + surface: str = "api", + ) -> dict[str, Any]: + """Write a change request or a question on one candidate (#173).""" + digest, files = self._verified_files(candidate_id) + return self.store.add_feedback( + candidate_id, + kind, + text, + path=path, + files_changed=files, + verified_patch_sha256=digest, + surface=surface, + ) + + def resolve_feedback( + self, + request_id: str, + status: str, + *, + on: str | None = None, + surface: str = "api", + ) -> tuple[dict[str, Any], bool]: + self._own_room(self.store.feedback_room(request_id)) + digest = None + if on is not None: + digest, _ = self._verified_files(on) + return self.store.resolve_feedback( + request_id, + status, + evaluated_candidate_id=on, + verified_patch_sha256=digest, + surface=surface, + ) + + def carry_feedback( + self, request_id: str, to_candidate_id: str, *, surface: str = "api" + ) -> CarriedFeedback: + self._own_room(self.store.feedback_room(request_id)) + digest, files = self._verified_files(to_candidate_id) + return self.store.carry_feedback( + request_id, + to_candidate_id, + files_changed=files, + verified_patch_sha256=digest, + surface=surface, + ) + def select( self, room_id: str, diff --git a/tests/test_room_feedback.py b/tests/test_room_feedback.py new file mode 100644 index 0000000..fa9a4eb --- /dev/null +++ b/tests/test_room_feedback.py @@ -0,0 +1,892 @@ +"""Review feedback on one candidate, and repairs that send it (#173 PR 2).""" + +from __future__ import annotations + +import json +import sqlite3 +from types import SimpleNamespace +from uuid import uuid4 + +import pytest +from click.testing import CliRunner + +from conftest import FakeMemory +from syncmill.cli import cli +from syncmill.rooms import ( + FEEDBACK_STATUSES, + RoomCoordinator, + RoomError, + render_feedback_batch, + render_repair_task, +) + +from test_room_repair import Harness, Member, _config + + +SENTINEL = "SENTINEL-FEEDBACK-7f3a" + + +@pytest.fixture +def make(git_repo, tmp_path): + made: list[Harness] = [] + + def build(*agents: Member, **config) -> Harness: + harness = Harness(git_repo, tmp_path, list(agents), **config) + made.append(harness) + return harness + + yield build + for harness in made: + harness.close() + + +def _both(): + return Member("claude"), Member("codex") + + +def _status(room: Harness, feedback_id: str) -> str: + return room.store.feedback(room.room_id).checks[feedback_id].status + + +def _add(room: Harness, candidate_id: str, text: str = "rename it", **kwargs) -> dict: + return room.coordinator.add_feedback( + candidate_id, kwargs.pop("kind", "change"), text, **kwargs + ) + + +def _raw(room: Harness) -> sqlite3.Connection: + return room.store._conn + + +# ------------------------------------------------------------------ writing + + +def test_a_request_is_recorded_without_touching_the_room(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + before = room.store.get_room(room.room_id) + + record = _add(room, parent["id"], f"rename it {SENTINEL}", path="claude.txt") + + assert record["candidate_id"] == parent["id"] + assert record["patch_sha256"] == parent["patch_sha256"] + assert record["anchor"] == {"path": "claude.txt"} + assert record["carried_from"] is None + after = room.store.get_room(room.room_id) + assert (after["state"], after["revision"]) == (before["state"], before["revision"]) + assert _status(room, record["feedback_id"]) == "open" + [event] = room.events("feedback.requested") + assert event["payload"]["feedback_id"] == record["feedback_id"] + assert event["payload"]["anchored"] is True + + +def test_feedback_text_never_enters_the_event_log(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"], SENTINEL) + room.repair(parent["id"], None) + new = room.candidate("claude") + room.coordinator.resolve_feedback(request["feedback_id"], "not_addressed", on=new["id"]) + room.coordinator.carry_feedback(request["feedback_id"], new["id"]) + rows = _raw(room).execute("SELECT payload_json FROM room_events").fetchall() + assert rows and all(SENTINEL not in row[0] for row in rows) + + +@pytest.mark.parametrize( + "path, match", + [("codex.txt", "not a file this candidate changed"), ("./claude.txt", "not a file")], +) +def test_an_anchor_must_name_a_file_the_candidate_changed(make, path, match): + room = make(*_both()) + room.round() + with pytest.raises(RoomError, match=match): + _add(room, room.candidate("claude")["id"], path=path) + assert room.store.feedback_count(room.room_id) == 0 + + +@pytest.mark.parametrize( + "text, match", + [ + ("", "must not be empty"), + (" \n ", "must not be empty"), + ("bad\x1bescape", "control characters"), + ("x" * (8 * 1024 + 1), "the limit is 8192"), + ], +) +def test_feedback_text_is_checked(make, text, match): + room = make(Member("claude")) + room.round() + with pytest.raises(RoomError, match=match): + _add(room, room.candidate("claude")["id"], text) + + +def test_a_request_needs_a_completed_candidate(make): + room = make(Member("claude", fail_calls={1})) + room.round() + with pytest.raises(RoomError, match="ended failed"): + _add(room, room.candidate("claude")["id"]) + + +def test_feedback_is_writable_after_a_selection_but_not_after_close(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + room.coordinator.select(room.room_id, parent["id"]) + revision = room.store.get_room(room.room_id)["revision"] + _add(room, parent["id"]) + assert room.store.get_room(room.room_id)["revision"] == revision + room.coordinator.close_room(room.room_id) + with pytest.raises(RoomError, match="room is closed"): + _add(room, parent["id"]) + + +# ---------------------------------------------------------------- resolving + + +def test_resolution_binds_to_a_later_completed_candidate_of_the_same_member(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + peer = room.candidate("codex") + request = _add(room, parent["id"])["feedback_id"] + + with pytest.raises(RoomError, match="--on"): + room.coordinator.resolve_feedback(request, "addressed") + with pytest.raises(RoomError, match="drop --on"): + room.coordinator.resolve_feedback(request, "withdrawn", on=parent["id"]) + with pytest.raises(RoomError, match="another member's"): + room.coordinator.resolve_feedback(request, "addressed", on=peer["id"]) + with pytest.raises(RoomError, match="later round"): + room.coordinator.resolve_feedback(request, "addressed", on=parent["id"]) + + room.round() + later = room.candidate("claude") + record, created = room.coordinator.resolve_feedback(request, "addressed", on=later["id"]) + assert created + assert record["evaluated_patch_sha256"] == later["patch_sha256"] + assert _status(room, request) == "addressed" + + again, created = room.coordinator.resolve_feedback(request, "addressed", on=later["id"]) + assert not created and again == record + room.coordinator.resolve_feedback(request, "withdrawn") + assert _status(room, request) == "withdrawn" + + +def test_a_failed_later_attempt_is_no_evidence(make): + room = make(Member("claude", fail_calls={2})) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + room.round() + failed = room.candidate("claude", 2) + with pytest.raises(RoomError, match="not a completed candidate"): + room.coordinator.resolve_feedback(request, "addressed", on=failed["id"]) + + +def test_evidence_from_another_room_is_refused(make, git_repo, tmp_path): + room = make(Member("claude")) + room.round() + request = _add(room, room.candidate("claude")["id"])["feedback_id"] + room.coordinator.close_room(room.room_id) + other_id = room.coordinator.create("another room", ["claude"])["id"] + room.coordinator.run_round(other_id, "go") + foreign = room.store.list_candidates(other_id)[0] + room.store._conn.execute( + "UPDATE rooms SET state='awaiting_decision' WHERE id=?", (room.room_id,) + ) + with pytest.raises(RoomError, match="not a candidate of this room"): + room.coordinator.resolve_feedback(request, "addressed", on=foreign["id"]) + + +def _insert_resolution(room: Harness, request: dict, **overrides) -> str: + """A resolution row written directly, bypassing the writer.""" + conn = _raw(room) + sequence = conn.execute( + "SELECT MAX(sequence)+1 FROM room_feedback WHERE room_id=?", (room.room_id,) + ).fetchone()[0] + record = { + "schema_version": 1, + "feedback_id": str(uuid4()), + "room_id": room.room_id, + "request_id": request["feedback_id"], + "status": "withdrawn", + "evaluated_candidate_id": None, + "evaluated_patch_sha256": None, + "surface": "api", + "sequence": sequence, + "created_at": "2026-11-10T00:00:00+00:00", + } + row = { + "candidate_id": request["candidate_id"], + "request_id": request["feedback_id"], + "sequence": sequence, + } + row.update({k: v for k, v in overrides.items() if k in row}) + record.update({k: v for k, v in overrides.items() if k in record and k not in row}) + if "record_sequence" in overrides: + record["sequence"] = overrides["record_sequence"] + body = overrides.get("record_json", json.dumps(record, sort_keys=True)) + conn.execute( + """INSERT INTO room_feedback + (id,room_id,candidate_id,record_type,request_id,sequence,record_json,created_at) + VALUES (?,?,?,'resolution',?,?,?,'now')""", + (record["feedback_id"], room.room_id, row["candidate_id"], row["request_id"], + row["sequence"], body), + ) + return record["feedback_id"] + + +def test_a_well_formed_direct_resolution_is_believed(make): + # The control for the damaged cases below: the helper writes a valid row. + room = make(*_both()) + room.round() + request = _add(room, room.candidate("claude")["id"]) + _insert_resolution(room, request) + assert _status(room, request["feedback_id"]) == "withdrawn" + + +@pytest.mark.parametrize( + "damage", + [ + {"record_json": "{not json"}, + {"status": "fixed"}, + {"status": "addressed"}, + {"record_sequence": 99}, + ], + ids=["json", "status", "no-evidence", "body-sequence"], +) +def test_a_damaged_latest_resolution_body_reads_unknown_for_its_request_only(make, damage): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"]) + other = _add(room, parent["id"], "another one") + room.coordinator.resolve_feedback(request["feedback_id"], "withdrawn") + _insert_resolution(room, request, **damage) + view = room.store.feedback(room.room_id) + assert view.problem is None + assert view.checks[request["feedback_id"]].status == "unknown" + assert "latest resolution" in view.checks[request["feedback_id"]].reason + assert view.checks[other["feedback_id"]].status == "open" + with pytest.raises(RoomError, match="cannot be trusted"): + room.coordinator.resolve_feedback(request["feedback_id"], "withdrawn") + + +@pytest.mark.parametrize("field, value", [("kind", []), ("kind", {}), ("text", 5)]) +def test_a_request_with_a_malformed_value_reads_unknown_alone(make, field, value): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"]) + other = _add(room, parent["id"], "another") + _raw(room).execute( + "UPDATE room_feedback SET record_json=? WHERE id=?", + (json.dumps(dict(request, **{field: value})), request["feedback_id"]), + ) + checks = room.store.feedback(room.room_id).checks + assert checks[request["feedback_id"]].status == "unknown" + assert checks[other["feedback_id"]].status == "open" + + +@pytest.mark.parametrize("value", [[], {}, 3]) +def test_a_resolution_with_a_malformed_status_reads_unknown_alone(make, value): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"]) + other = _add(room, parent["id"], "another") + _insert_resolution(room, request, status=value) + checks = room.store.feedback(room.room_id).checks + assert checks[request["feedback_id"]].status == "unknown" + assert checks[other["feedback_id"]].status == "open" + + +def test_another_repository_cannot_write_this_rooms_feedback(make, tmp_path): + import subprocess + + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + other_repo = tmp_path / "other" + other_repo.mkdir() + subprocess.run(["git", "init", "-q", str(other_repo)], check=True) + other = RoomCoordinator(room.config, other_repo, agents=[Member("claude")]) + try: + for attempt in ( + lambda: other.add_feedback(parent["id"], "change", "x"), + lambda: other.resolve_feedback(request, "withdrawn"), + lambda: other.carry_feedback(request, parent["id"]), + ): + with pytest.raises(RoomError, match="different repository"): + attempt() + finally: + other.close() + assert room.store.feedback_count(room.room_id) == 1 + + +def _damaged_set(room: Harness): + """A request with a withdrawal, then a second request, all on claude's + candidate: the withdrawal is not the last row, so removing it leaves a gap.""" + parent = room.candidate("claude") + first = _add(room, parent["id"], "first") + room.coordinator.resolve_feedback(first["feedback_id"], "withdrawn") + second = _add(room, parent["id"], "second") + withdrawal = room.store.feedback(room.room_id).checks[first["feedback_id"]].resolution + return parent, first, second, withdrawal["feedback_id"] + + +_SET_DAMAGE = { + "resolution-request-null": "UPDATE room_feedback SET request_id=NULL WHERE id=:w", + "resolution-retargeted": "UPDATE room_feedback SET request_id=:s WHERE id=:w", + "resolution-other-candidate": "UPDATE room_feedback SET candidate_id=:peer WHERE id=:w", + "resolution-type": "UPDATE room_feedback SET record_type='garbage' WHERE id=:w", + "request-names-request": "UPDATE room_feedback SET request_id=:f WHERE id=:s", + "request-id-null": "UPDATE room_feedback SET id=NULL WHERE id=:s", + "deleted": "DELETE FROM room_feedback WHERE id=:w", + "sequence-text": "UPDATE room_feedback SET sequence='broken' WHERE id=:w", + "moved-to-another-room": "UPDATE room_feedback SET room_id='elsewhere' WHERE id=:w", +} + + +@pytest.mark.parametrize("damage", sorted(_SET_DAMAGE)) +def test_a_damaged_record_set_makes_every_request_unknown_and_stops_writers(make, damage): + room = make(*_both()) + room.round() + parent, first, second, withdrawal = _damaged_set(room) + conn = _raw(room) + conn.execute("PRAGMA foreign_keys=OFF") + conn.execute( + _SET_DAMAGE[damage], + { + "w": withdrawal, + "f": first["feedback_id"], + "s": second["feedback_id"], + "peer": room.candidate("codex")["id"], + }, + ) + conn.execute("PRAGMA foreign_keys=ON") + + view = room.store.feedback(room.room_id) + assert view.problem is not None + assert view.problem in view.warnings[0] + assert view.checks and all(c.status == "unknown" for c in view.checks.values()) + assert all(c.request is None for c in view.checks.values()) + with pytest.raises(RoomError, match="cannot be trusted"): + _add(room, parent["id"]) + with pytest.raises(RoomError, match="cannot be trusted|unknown feedback"): + room.coordinator.resolve_feedback(first["feedback_id"], "withdrawn") + with pytest.raises(RoomError, match="cannot be trusted"): + room.coordinator.build_repair_plan(parent["id"], None) + + +@pytest.mark.parametrize("damage", ["moved-last", "retargeted-earlier"]) +def test_damage_only_the_set_check_can_see(make, damage): + # The damaged row is the room's last, so no gap shows; and the request + # it is pointed at is an earlier one on the same candidate. + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + first = _add(room, parent["id"], "first")["feedback_id"] + second = _add(room, parent["id"], "second")["feedback_id"] + room.coordinator.resolve_feedback(second, "withdrawn") + withdrawal = room.store.feedback(room.room_id).checks[second].resolution["feedback_id"] + conn = _raw(room) + if damage == "moved-last": + conn.execute("PRAGMA foreign_keys=OFF") + conn.execute( + "UPDATE room_feedback SET room_id='elsewhere' WHERE id=?", (withdrawal,) + ) + conn.execute("PRAGMA foreign_keys=ON") + else: + conn.execute( + "UPDATE room_feedback SET request_id=? WHERE id=?", (first, withdrawal) + ) + view = room.store.feedback(room.room_id) + assert view.problem is not None + assert {c.status for c in view.checks.values()} == {"unknown"} + + +def test_a_record_set_damaged_after_confirm_refuses_the_round(make): + claude = Member("claude") + room = make(claude) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + plan = room.coordinator.build_repair_plan(parent["id"], None) + _raw(room).execute("UPDATE room_feedback SET sequence=7 WHERE id=?", (request,)) + with pytest.raises(RoomError, match=f"feedback {request} changed"): + room.coordinator.run_round(room.room_id, plan.prompt, target=plan) + assert len(claude.scopes) == 1 + + +def test_a_repair_record_that_cannot_be_believed_makes_its_requests_unknown(make): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + room.repair(parent["id"], None) + assert _status(room, request) == "sent" + row = room.rounds()[-1] + record = json.loads(row["provenance_json"]) + record["tree"]["kind"] = [] + _raw(room).execute( + "UPDATE rounds SET provenance_json=? WHERE id=?", (json.dumps(record), row["id"]) + ) + check = room.store.feedback(room.room_id).checks[request] + assert check.status == "unknown" + assert "cannot be believed" in check.reason + + +def test_a_request_on_a_candidate_of_no_member_reads_unknown(make): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + _raw(room).execute("UPDATE candidates SET member_id='ghost' WHERE id=?", (parent["id"],)) + assert _status(room, request) == "unknown" + + +def test_carry_refuses_to_return_an_untrusted_earlier_copy(make): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + room.repair(parent["id"], None) + new = room.candidate("claude") + copy = room.coordinator.carry_feedback(request, new["id"]).record["feedback_id"] + room.coordinator.resolve_feedback(copy, "withdrawn") + _raw(room).execute( + "UPDATE room_feedback SET record_json='{damaged' WHERE request_id=?", (copy,) + ) + assert _status(room, copy) == "unknown" + with pytest.raises(RoomError, match="cannot be trusted"): + room.coordinator.carry_feedback(request, new["id"]) + + +def test_a_damaged_request_reads_unknown(make): + room = make(Member("claude")) + room.round() + request = _add(room, room.candidate("claude")["id"]) + _raw(room).execute( + "UPDATE room_feedback SET record_json=? WHERE id=?", + (json.dumps(dict(request, patch_sha256="0" * 64)), request["feedback_id"]), + ) + check = room.store.feedback(room.room_id).checks[request["feedback_id"]] + assert check.status == "unknown" and "patch digest" in check.reason + + +# ------------------------------------------------------------------- repair + + +def test_render_pins_the_task_bytes(): + head = ( + "Repair your candidate C. Your working tree holds exactly that candidate's " + "patch. Ignored files, such as installed dependencies, may be missing.\n\n" + ) + assert render_repair_task("C", "free") == head + "free" + requests = [ + {"kind": "question", "text": "why?", "anchor": {"path": "a.py"}}, + {"kind": "change", "text": "one\ntwo\n\nthree", "anchor": None}, + {"kind": "change", "text": "x", "anchor": {"path": "we]ird\n.py"}}, + ] + assert render_repair_task("C", None, requests) == head + ( + "Change requests:\n" + "1. one\n two\n\n three\n" + '2. ["we]ird\\n.py"] x\n' + "\n" + "Questions (answer these in your response):\n" + "1. [a.py] why?" + ) + assert render_repair_task("C", "free", requests[:1]) == head + ( + "Questions (answer these in your response):\n1. [a.py] why?\n\nfree" + ) + assert render_feedback_batch([]) == "" + nel = render_feedback_batch([{"kind": "change", "text": "x", "anchor": {"path": "a\x85b.py"}}]) + assert nel == 'Change requests:\n1. ["a\\u0085b.py"] x' + assert "\x85" not in render_repair_task("C", None, [{"kind": "change", "text": "x", "anchor": {"path": "a\x85b.py"}}]) + + +def test_a_repair_sends_the_candidates_open_requests(make): + claude, codex = _both() + room = make(claude, codex) + room.round() + parent = room.candidate("claude") + change = _add(room, parent["id"], f"rename {SENTINEL}", path="claude.txt") + question = _add(room, parent["id"], "why this way?", kind="question") + withdrawn = _add(room, parent["id"], "never mind") + room.coordinator.resolve_feedback(withdrawn["feedback_id"], "withdrawn") + _add(room, room.candidate("codex")["id"], "not for claude") + + plan, _ = room.repair(parent["id"], None) + + assert [item["id"] for item in plan.feedback] == [ + change["feedback_id"], + question["feedback_id"], + ] + assert plan.agent_task == render_repair_task(parent["id"], None, [change, question]) + assert plan.agent_task in claude.scopes[-1]["prompt"] + row = room.rounds()[-1] + assert row["prompt"] == f"Repair candidate {parent['id']}" + assert SENTINEL not in row["prompt"] + tree = room.provenance(row).record["tree"] + assert tree["feedback_ids"] == [change["feedback_id"], question["feedback_id"]] + assert _status(room, change["feedback_id"]) == "sent" + assert _status(room, question["feedback_id"]) == "sent" + assert _status(room, withdrawn["feedback_id"]) == "withdrawn" + assert room.store.feedback(room.room_id).warnings == [] + + +def test_a_repair_with_neither_feedback_nor_prompt_is_refused(make): + room = make(Member("claude")) + room.round() + with pytest.raises(RoomError, match="nothing to repair"): + room.coordinator.build_repair_plan(room.candidate("claude")["id"], None) + + +def test_feedback_ids_narrow_the_batch_and_must_be_open_here(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + first = _add(room, parent["id"], "first")["feedback_id"] + second = _add(room, parent["id"], "second")["feedback_id"] + gone = _add(room, parent["id"], "gone")["feedback_id"] + room.coordinator.resolve_feedback(gone, "withdrawn") + peer = _add(room, room.candidate("codex")["id"])["feedback_id"] + build = room.coordinator.build_repair_plan + + plan = build(parent["id"], None, feedback_ids=[second]) + assert [item["id"] for item in plan.feedback] == [second] + assert build(parent["id"], None, feedback_ids=[second, first]).token == build( + parent["id"], None, feedback_ids=[first, second] + ).token + for ids, match in ( + ([gone], "is withdrawn"), + ([peer], "not on"), + ([str(uuid4())], "not a feedback request"), + ([first, first], "listed twice"), + ): + with pytest.raises(RoomError, match=match): + build(parent["id"], None, feedback_ids=ids) + + +def test_a_failed_repair_leaves_its_requests_open(make): + room = make(Member("claude", fail_calls={2})) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + room.repair(parent["id"], None) + assert _status(room, request) == "open" + plan = room.coordinator.build_repair_plan(parent["id"], None) + assert [item["id"] for item in plan.feedback] == [request] + + +@pytest.mark.parametrize("change", ["withdraw", "resolve"]) +def test_feedback_changed_after_confirm_refuses_the_round(make, change): + claude = Member("claude") + room = make(claude) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + plan = room.coordinator.build_repair_plan(parent["id"], None) + if change == "withdraw": + room.coordinator.resolve_feedback(request, "withdrawn") + else: + # Make the request no longer open without a resolution: rewrite it. + _raw(room).execute( + "UPDATE room_feedback SET record_json=? WHERE id=?", + ( + json.dumps( + dict(room.store.feedback(room.room_id).checks[request].request, text="changed"), + sort_keys=True, + ), + request, + ), + ) + calls = len(claude.scopes) + with pytest.raises(RoomError, match=f"feedback {request} changed"): + room.coordinator.run_round(room.room_id, plan.prompt, target=plan) + assert len(claude.scopes) == calls + assert len(room.rounds()) == 1 + assert room.store.get_room(room.room_id)["state"] == "awaiting_decision" + + +def test_a_request_added_after_confirm_stays_open(make): + room = make(Member("claude")) + room.round() + parent = room.candidate("claude") + sent = _add(room, parent["id"], "first")["feedback_id"] + plan = room.coordinator.build_repair_plan(parent["id"], None) + late = _add(room, parent["id"], "second")["feedback_id"] + room.coordinator.run_round(room.room_id, plan.prompt, target=plan) + assert _status(room, sent) == "sent" + assert _status(room, late) == "open" + + +def test_after_a_repair_the_request_is_carried_to_the_new_candidate(make): + room = make(Member("claude"), gate={"commands": ["false"]}) + room.round() + parent = room.candidate("claude") + anchored = _add(room, parent["id"], "keep the file", path="claude.txt")["feedback_id"] + plain = _add(room, parent["id"], "no file")["feedback_id"] + room.repair(parent["id"], None) + new = room.candidate("claude") + assert new["state"] == "completed" and not new["eligible"] # the gate failed + + with pytest.raises(RoomError, match="newer completed candidate"): + room.coordinator.build_repair_plan(parent["id"], None) + carried = room.coordinator.carry_feedback(anchored, new["id"]) + assert carried.created + assert carried.record["anchor"] == {"path": "claude.txt"} + assert carried.record["carried_from"] == anchored + assert _status(room, carried.record["feedback_id"]) == "open" + assert _status(room, anchored) == "sent" + assert room.coordinator.carry_feedback(anchored, new["id"]).created is False + room.coordinator.carry_feedback(plain, new["id"]) + + plan = room.coordinator.build_repair_plan(new["id"], None) + assert len(plan.feedback) == 2 + + +def test_carry_drops_an_anchor_the_new_candidate_does_not_change(make): + class Mover(Member): + async def aexecute(self, prompt, workdir, timeout): + result = await super().aexecute(prompt, workdir, timeout) + if len(self.scopes) == 2: + (workdir / "claude.txt").rename(workdir / "moved.txt") + return result + + room = make(Mover("claude")) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"], path="claude.txt")["feedback_id"] + room.repair(parent["id"], None) + new = room.candidate("claude") + assert "claude.txt" not in room.store.load_candidate(new["id"])["files_changed"] + carried = room.coordinator.carry_feedback(request, new["id"]) + assert carried.record["anchor"] is None + assert carried.source_anchor == {"path": "claude.txt"} + + +def test_carry_refuses_another_member_an_earlier_round_and_a_withdrawal(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + request = _add(room, parent["id"])["feedback_id"] + with pytest.raises(RoomError, match="member it was written for"): + room.coordinator.carry_feedback(request, room.candidate("codex")["id"]) + with pytest.raises(RoomError, match="not from a later round"): + room.coordinator.carry_feedback(request, parent["id"]) + room.coordinator.resolve_feedback(request, "withdrawn") + room.round() + with pytest.raises(RoomError, match="withdrawn"): + room.coordinator.carry_feedback(request, room.candidate("claude")["id"]) + + +# ------------------------------------------------------ the round's side + + +def _rewrite_feedback_ids(room: Harness, row: dict, ids: list[str]) -> None: + record = json.loads(row["provenance_json"]) + record["tree"]["feedback_ids"] = ids + _raw(room).execute( + "UPDATE rounds SET provenance_json=? WHERE id=?", (json.dumps(record), row["id"]) + ) + + +def test_a_repair_round_naming_what_is_not_its_request_is_reported(make): + room = make(*_both()) + room.round() + parent = room.candidate("claude") + peer_request = _add(room, room.candidate("codex")["id"])["feedback_id"] + own = _add(room, parent["id"])["feedback_id"] + room.coordinator.resolve_feedback(own, "withdrawn") + resolution = room.store.feedback(room.room_id).checks[own].resolution["feedback_id"] + room.repair(parent["id"], "free text only") + missing = str(uuid4()) + _rewrite_feedback_ids(room, room.rounds()[-1], [missing, resolution, peer_request]) + + view = room.store.feedback(room.room_id) + assert view.warnings == [ + f"repair round 2 names feedback {missing}, which is not a request in this room", + f"repair round 2 names feedback {resolution}, which is a resolution, not a request", + f"repair round 2 names feedback {peer_request}, which is a request on another candidate", + ] + # Attributed to the wrong candidate, the request itself is not believed. + assert view.checks[peer_request].status == "unknown" + assert "repaired another candidate" in view.checks[peer_request].reason + + +# ------------------------------------------------------- memory and apply + + +class _Recording(FakeMemory): + def __init__(self): + super().__init__() + self.queries: list[str] = [] + self.shared: list[str] = [] + + async def search_context(self, query, *args, **kwargs): + self.queries.append(query) + return await super().search_context(query, *args, **kwargs) + + async def share_result(self, task, result, *, kind="task-result"): + self.shared.append(task.prompt) + return await super().share_result(task, result, kind=kind) + + +def test_no_feedback_text_reaches_memory_through_search_or_apply(git_repo, tmp_path): + memory = _Recording() + agent = Member("claude") + coordinator = RoomCoordinator(_config(tmp_path), git_repo, agents=[agent], memory=memory) + try: + room_id = coordinator.create("the room prompt", ["claude"])["id"] + coordinator.run_round(room_id, "a round") + parent = coordinator.store.list_candidates(room_id)[0] + coordinator.add_feedback(parent["id"], "change", f"please {SENTINEL}") + plan = coordinator.build_repair_plan(parent["id"], None) + coordinator.run_round(room_id, plan.prompt, target=plan) + assert SENTINEL in agent.scopes[-1]["prompt"] + new = max(coordinator.store.list_candidates(room_id), key=lambda r: r["round_number"]) + assert SENTINEL not in coordinator.store.load_candidate(new["id"])["output"] + coordinator.select(room_id, new["id"]) + coordinator.apply(room_id) + assert memory.queries and all(SENTINEL not in q for q in memory.queries) + assert memory.shared and all(SENTINEL not in p for p in memory.shared) + finally: + coordinator.close() + + +def test_prune_removes_a_rooms_feedback(make): + room = make(Member("claude")) + room.round() + request = _add(room, room.candidate("claude")["id"])["feedback_id"] + room.coordinator.resolve_feedback(request, "withdrawn") + room.coordinator.close_room(room.room_id) + assert room.coordinator.prune_terminal(keep=0, retention_days=0) == [room.room_id] + assert _raw(room).execute("SELECT COUNT(*) FROM room_feedback").fetchone()[0] == 0 + + +def test_every_status_is_listed(): + assert set(FEEDBACK_STATUSES) == { + "open", "sent", "addressed", "not_addressed", "withdrawn", "unknown", + } + + +# ------------------------------------------------------------------------ CLI + + +@pytest.fixture +def cli_room(monkeypatch, git_repo, tmp_path): + claude, codex = _both() + config = _config(tmp_path) + agents = [claude, codex] + + def build() -> RoomCoordinator: + return RoomCoordinator(config, git_repo, agents=agents) + + monkeypatch.setattr("syncmill.cli._room_coordinator", lambda repo: (git_repo, config, build())) + coordinator = build() + room_id = coordinator.create("the room prompt", ["claude", "codex"])["id"] + coordinator.run_round(room_id, "a round") + parent = next( + c for c in coordinator.store.list_candidates(room_id) if c["member_id"] == "claude" + ) + yield SimpleNamespace(coordinator=coordinator, room_id=room_id, parent=parent, claude=claude) + coordinator.close() + + +def _run(*args: str): + return CliRunner().invoke(cli, ["room", *args]) + + +def _id_from(output: str, word: str) -> str: + tokens = output.split() + return tokens[tokens.index(word) + 1] + + +def test_cli_add_list_resolve_and_repair(cli_room, tmp_path): + parent = cli_room.parent["id"] + res = _run("feedback", "add", parent, "--kind", "change", "--file", "claude.txt", + "--text", "line one\nline two") + assert res.exit_code == 0, res.output + assert "(file: claude.txt)" in res.output + request = _id_from(res.output, "request") + text_file = tmp_path / "q.txt" + text_file.write_bytes(b"why?\r\n") + refused = _run("feedback", "add", parent, "--kind", "question", "--text-file", str(text_file)) + assert refused.exit_code == 1 and "control characters" in refused.output + + listed = _run("feedback", "list", cli_room.room_id) + assert listed.exit_code == 0, listed.output + assert f"{request} {parent} change open" in listed.output + assert " | line one\n | line two" in listed.output + + preview = _run("repair", parent, "--preview") + assert preview.exit_code == 0, preview.output + assert "Feedback: 1 change request(s), 0 question(s)" in preview.output + assert f"{request} change" in preview.output + assert "line one" not in preview.output + token = next( + line.split()[1] for line in preview.output.splitlines() if line.startswith("Token:") + ) + ran = _run("repair", parent, "--confirm", token) + assert ran.exit_code == 0, ran.output + assert len(cli_room.claude.scopes) == 2 + assert "sent" in _run("feedback", "list", cli_room.room_id, "--candidate", parent).output + + new = max( + (c for c in cli_room.coordinator.store.list_candidates(cli_room.room_id) + if c["member_id"] == "claude"), + key=lambda c: c["round_number"], + )["id"] + resolved = _run("feedback", "resolve", request, "not-addressed", "--on", new) + assert resolved.exit_code == 0, resolved.output + assert f"Recorded {request} not-addressed on candidate {new}" in resolved.output + again = _run("feedback", "resolve", request, "not-addressed", "--on", new) + assert "Already recorded" in again.output + + carried = _run("feedback", "carry", request, "--to", new) + assert carried.exit_code == 0, carried.output + assert "File: claude.txt, kept" in carried.output + + +def test_cli_list_never_prints_the_text_of_an_unknown_entry(cli_room): + coordinator = cli_room.coordinator + request = coordinator.add_feedback(cli_room.parent["id"], "change", SENTINEL) + coordinator.resolve_feedback(request["feedback_id"], "withdrawn") + coordinator.store._conn.execute( + "UPDATE room_feedback SET record_json='{damaged' WHERE record_type='resolution'" + ) + listed = _run("feedback", "list", cli_room.room_id) + assert listed.exit_code == 0, listed.output + assert "latest resolution" in listed.output + assert SENTINEL not in listed.output + + +def test_cli_list_survives_a_null_feedback_id(cli_room): + coordinator = cli_room.coordinator + request = coordinator.add_feedback(cli_room.parent["id"], "change", SENTINEL) + coordinator.store._conn.execute( + "UPDATE room_feedback SET id=NULL WHERE id=?", (request["feedback_id"],) + ) + listed = _run("feedback", "list", cli_room.room_id) + assert listed.exit_code == 0, listed.output + assert "cannot be trusted" in listed.output + assert SENTINEL not in listed.output + + +def test_cli_refusals_are_one_line_errors(cli_room): + res = _run("feedback", "add", cli_room.parent["id"], "--kind", "change", + "--file", "nope.txt", "--text", "x") + assert res.exit_code == 1 and "not a file this candidate changed" in res.output + res = _run("feedback", "add", cli_room.parent["id"], "--kind", "change") + assert res.exit_code == 2 and "exactly one of --text and --text-file" in res.output + res = _run("feedback", "resolve", str(uuid4()), "withdrawn") + assert res.exit_code == 1 and "unknown feedback" in res.output + res = _run("repair", cli_room.parent["id"], "--preview") + assert res.exit_code == 1 and "nothing to repair" in res.output + res = _run("feedback", "list", cli_room.room_id) + assert res.exit_code == 0 and "No feedback in room" in res.output diff --git a/tests/test_room_repair.py b/tests/test_room_repair.py index d2c0eaa..82e8262 100644 --- a/tests/test_room_repair.py +++ b/tests/test_room_repair.py @@ -436,7 +436,7 @@ def _no_change(room: Harness, revision: int, rounds: int) -> None: def test_a_repair_with_nothing_to_do_is_refused(make, prompt): room = make(Member("claude")) room.round() - with pytest.raises(RoomError, match="needs a prompt"): + with pytest.raises(RoomError, match="nothing to repair"): room.coordinator.build_repair_plan(room.candidate("claude")["id"], prompt)