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)