Replace the agent review protocol with one instruction and a killable wait

.gitignore

@@ -1,3 +1,8 @@
 dist/
 *.log
 .DS_Store
+
+# crq report output. Anything left in the working tree reads as unlanded work,
+# which would make `crq next` answer "push" forever instead of "done".
+crq-next.json
+crq-feedback.json

cmd/crq/main_test.go

@@ -0,0 +1,114 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// The thread commands take IDs bare so a caller can clear a whole round in one
+// process. The transcripts that motivated this were full of shell loops running
+// one `crq resolve` subprocess per thread, every round.
+func TestParseThreadCommand(t *testing.T) {
+	const (
+		t1 = "PRRT_kwDOAAAAAA1"
+		t2 = "PRRT_kwDOAAAAAA2"
+	)
+	cases := []struct {
+		name    string
+		args    []string
+		reason  bool
+		want    []string
+		wantRsn string
+		wantRes bool
+		wantErr bool
+	}{
+		{name: "bare ids", args: []string{t1, t2}, want: []string{t1, t2}},
+		{name: "flag form still works", args: []string{"--thread", t1, "--thread", t2}, want: []string{t1, t2}},
+		{name: "mixed forms", args: []string{"--thread", t1, t2}, want: []string{t1, t2}},
+		{
+			// The old signature demanded a target these commands never used:
+			// thread node IDs are globally unique.
+			name: "legacy repo and pr are dropped",
+			args: []string{"owner/repo", "123", t1},
+			want: []string{t1},
+		},
+		{
+			name: "legacy target with flag threads",
+			args: []string{"owner/repo", "123", "--thread", t1},
+			want: []string{t1},
+		},
+		{
+			// A repo-shaped first arg is only a target when a PR number follows,
+			// so an ID that happens to contain "/" is not eaten.
+			name: "slashed id without a pr number is a thread",
+			args: []string{"weird/id", t1},
+			want: []string{"weird/id", t1},
+		},
+		{name: "dangling --thread is an error", args: []string{"--thread"}, wantErr: true},
+		{
+			name: "unknown flag is an error, not a thread id",
+			args: []string{"--treahd", t1}, wantErr: true,
+		},
+		{
+			// Declining resolves by default: a thread left open keeps its finding
+			// actionable, so the loop would repeat `fix` forever.
+			name: "decline resolves by default",
+			args: []string{t1, "--reason", "not a real issue"}, reason: true,
+			want: []string{t1}, wantRsn: "not a real issue", wantRes: true,
+		},
+		{
+			name: "decline --keep-open leaves the disagreement open",
+			args: []string{t1, "--reason", "still discussing", "--keep-open"}, reason: true,
+			want: []string{t1}, wantRsn: "still discussing", wantRes: false,
+		},
+		{
+			// --resolve used to be required; accepting it as a no-op keeps existing
+			// callers working now that it is the default.
+			name: "legacy --resolve still accepted",
+			args: []string{t1, "--reason", "no", "--resolve"}, reason: true,
+			want: []string{t1}, wantRsn: "no", wantRes: true,
+		},
+		{
+			// resolve has no --reason, so passing one must fail rather than be
+			// swallowed as a positional.
+			name: "reason flag is rejected by resolve",
+			args: []string{t1, "--reason", "x"}, wantErr: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			threads, reason, resolve, ok := parseThreadCommand(tc.args, tc.reason)
+			if tc.wantErr {
+				if ok {
+					t.Fatalf("parseThreadCommand(%v) = %v, want an error", tc.args, threads)
+				}
+				return
+			}
+			if !ok {
+				t.Fatalf("parseThreadCommand(%v) failed, want %v", tc.args, tc.want)
+			}
+			if strings.Join(threads, ",") != strings.Join(tc.want, ",") {
+				t.Errorf("threads = %v, want %v", threads, tc.want)
+			}
+			if reason != tc.wantRsn {
+				t.Errorf("reason = %q, want %q", reason, tc.wantRsn)
+			}
+			if resolve != tc.wantRes {
+				t.Errorf("resolve = %v, want %v", resolve, tc.wantRes)
+			}
+		})
+	}
+}
+
+// A mistyped flag used to be dropped as a non-positional, so `--wiat` ran the
+// non-blocking form and looked like success.
+func TestUnknownFlag(t *testing.T) {
+	if _, found := unknownFlag([]string{"owner/repo", "1", "--wait"}, "--wait"); found {
+		t.Error("a known flag must be accepted")
+	}
+	bad, found := unknownFlag([]string{"owner/repo", "1", "--wiat"}, "--wait")
+	if !found || bad != "--wiat" {
+		t.Errorf("unknownFlag = (%q, %v), want (--wiat, true)", bad, found)
+	}
+}

examples/review-loop.sh

@@ -1,36 +1,54 @@
 #!/usr/bin/env bash
-# Minimal agent wrapper around crq's JSON/exit-code contract.
+# Minimal agent wrapper around crq's next-action contract.
+#
+# One step of the loop: ask crq what to do about this PR and print it. There is
+# no exit code to interpret and no delay to invent — `crq next` answers both.
+# Drop this inside your own while-loop, or let your agent drive it.
 set -euo pipefail
 
 REPO="${REPO:?set REPO=owner/name}"
 PR="${PR:?set PR=<number>}"
-OUT="${OUT:-crq-feedback.json}"
+# Default OUT lives OUTSIDE the checkout on purpose. crq decides push-vs-done by
+# asking whether the working tree holds changes the PR head lacks, so a report
+# written into the repository is itself uncommitted work: the loop would then
+# read as "push" forever and could never reach "done".
+if [ -n "${OUT:-}" ]; then
+  # A caller-provided path is theirs to keep; only clean up what we created.
+  :
+else
+  OUT="$(mktemp -t crq-next.XXXXXX.json)"
+  trap 'rm -f "$OUT"' EXIT
+fi
 
-set +e
-crq loop "$REPO" "$PR" > "$OUT"
-rc=$?
-set -e
+crq next "$REPO" "$PR" > "$OUT"
+action=$(jq -r .action "$OUT")
+reason=$(jq -r '.reason // ""' "$OUT")
+recheck=$(jq -r '.recheck_after // ""' "$OUT")
 
-case "$rc" in
-  0)
-    echo "converged or no actionable findings; see $OUT"
-    ;;
-  10)
-    echo "actionable findings written to $OUT"
-    echo "fix valid findings and validate locally"
-    echo "resolve each addressed thread immediately after its local fix:"
+echo "action: $action${reason:+ — $reason}"
+
+case "$action" in
+  fix)
+    echo "fix the findings in $OUT and validate locally, then resolve each addressed thread:"
     echo "  jq -r '.findings[] | select(.thread_id != null) | .thread_id' '$OUT'"
-    echo "  crq resolve '$REPO' '$PR' --thread THREAD_ID"
-    echo "if any .reviewed_by value is false: HOLD THE HEAD; do not commit or push"
-    echo "after every required bot is true: fix/resolve the rest, then commit/push once"
+    echo "  crq resolve THREAD_ID [THREAD_ID...]"
+    echo "  crq decline THREAD_ID --reason 'why not addressed'"
+    ;;
+  hold)
+    echo "DO NOT commit or push — moving the head restarts the pending review"
+    echo "pending reviewers: $(jq -r '(.pending // []) | join(", ")' "$OUT")"
+    echo "call crq next again at $recheck"
     ;;
-  2)
-    echo "timed out waiting for feedback; do not push a stale-feedback round; see $OUT"
+  push)
+    echo "the head is released; commit and push your accumulated fixes once, then call crq next again"
     ;;
-  *)
-    echo "crq loop failed with exit $rc"
+  wait)
+    echo "nothing to do; call crq next again at $recheck"
+    ;;
+  done)
+    echo "converged"
+    ;;
+  blocked)
+    echo "needs a human"
     ;;
 esac
-
-# Propagate crq's exit code so automation can branch on findings/timeout/convergence.
-exit "$rc"

internal/crq/feedback.go

@@ -26,6 +26,21 @@ type FeedbackReport struct {
 	ReviewedBy map[string]bool   `json:"reviewed_by"`
 	Findings   []dialect.Finding `json:"findings"`
 	CheckedAt  time.Time         `json:"checked_at"`
+	// Open is whether the PR was open in the same observation Head and
+	// ReviewedBy came from. It is deliberately not serialized — the feedback
+	// JSON contract is frozen — and exists so a caller deciding an action reads
+	// head, open and evidence from ONE snapshot rather than re-reading the pull.
+	Open bool `json:"-"`
+	// HeadRef and HeadRepo describe where the PR's branch actually lives. On a
+	// fork PR that repository is not the one the PR is filed against, and a
+	// contributor's checkout only has a remote for it. Not serialized: the
+	// feedback JSON contract is frozen.
+	HeadRef  string `json:"-"`
+	HeadRepo string `json:"-"`
+	// LastEvidenceAt is when the newest review from a feedback bot landed. It
+	// anchors the settle window for a caller that holds no state between calls:
+	// "quiet since" is derivable, where the loop's in-process settledAt is not.
+	LastEvidenceAt time.Time `json:"-"`
 	// CodeRabbitDeferred marks a round degraded to Codex-only while the
 	// CodeRabbit account is rate-limited: Codex feedback is authoritative for
 	// this round, the CodeRabbit review stays queued and fires after
@@ -87,6 +102,9 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 		Repo:       repo,
 		PR:         pr,
 		Head:       head,
+		Open:       obs.eng.Open,
+		HeadRef:    pull.Head.Ref,
+		HeadRepo:   NormalizeRepo(pull.Head.Repo.FullName),
 		ReviewedBy: map[string]bool{},
 		Findings:   []dialect.Finding{},
 		CheckedAt:  now,
@@ -107,6 +125,29 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe
 	}
 	completion := engine.Completion(completionRound, obs.eng, s.policy())
 	report.ReviewedBy = completion.ReviewedBy
+	// Completion does not always arrive as a review: a clean-summary comment, a
+	// paired completion reply and a co-reviewer check run all satisfy it. Anchor
+	// the settle window on the newest of ANY of them, or a round completed by a
+	// comment would settle against a stale timestamp and converge instantly.
+	evidenceBots := dialect.BotSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots))
+	noteEvidence := func(at time.Time) {
+		if at.After(report.LastEvidenceAt) {
+			report.LastEvidenceAt = at
+		}
+	}
+	for _, review := range obs.reviews {
+		if dialect.InBots(evidenceBots, review.User.Login) {
+			noteEvidence(review.SubmittedAt)
+		}
+	}
+	for _, comment := range obs.comments {
+		if dialect.InBots(evidenceBots, comment.User.Login) {
+			noteEvidence(comment.UpdatedAt)
+		}
+	}
+	for _, check := range obs.eng.Checks {
+		noteEvidence(check.CompletedAt)
+	}
 	verdictCutoff := anchorCutoff
 	if verdictCutoff.IsZero() {
 		// Not fired yet (queued behind another PR): without a fire anchor the