Split the watch-loop predicate from the merge gate (additively)

config/dev-model.yaml

@@ -118,11 +118,11 @@ review:
   # against the check name.
   informational_checks: [coderabbit]
   # Whether a PR must have at least one real (non-informational) CI check before
-  # pr_watch will report `done`. True is the safe default: it stops an autonomous
+  # pr_watch will report green. True is the safe default: it stops an autonomous
   # merge on a PR whose CI never ran. Set false for a repo with no CI at all —
   # otherwise pr-watch can never converge and `dev_session.sh merge` always
-  # refuses. With false, `done` additionally requires current-head review evidence
-  # (it already does) — that receipt becomes the only quality gate, so set this
+  # refuses. With false, `mergeable` still requires a current-head review receipt
+  # (it always does) — that receipt becomes the only quality gate, so set this
   # deliberately.
   require_ci: true
 

docs/agentic-dev-kit/workflows/pr-watch.md

@@ -27,16 +27,31 @@ that `dev_session.sh merge <scope>` re-checks.
 
 ## Loop
 
-Repeat until the report says **done**:
+Repeat until the report says **converged**:
 
 1. **Poll.** `uv run <engine-dir>/pr_watch.py <PR#> --json` (omit `<PR#>` for the current
-   branch). Read `done`, `checks` (`all_green`, `failing[]`, `pending`),
+   branch). Read `converged`, `mergeable`, `checks` (`all_green`, `failing[]`, `pending`),
    `merge_blockers[]`, `review_evidence`, and `new_comments[]`.
 
-1. **If `done` (checks all green + nothing new + PR open/ready/mergeable with no
-   requested changes + independent review evidence bound to the current head):**
-   stop the loop and report — PR #, the green check count, review source, and "no
-   outstanding review findings." You're finished.
+   The two predicates answer different questions and you need both:
+
+   - **`converged`** — "is there more for me to fix?" Green, nothing new, not
+     settling. This is what ends *this* loop.
+   - **`mergeable`** — "is this authorized to merge?" `converged` **plus** no
+     `merge_blockers[]` **plus** an independent-review receipt bound to the current
+     head. This is what `dev_session.sh merge` re-checks.
+
+   A PR can be `converged` and not `mergeable` — most commonly because no review
+   receipt has been recorded yet. That is the normal, expected state at the end of
+   the loop, not a failure.
+
+   `done` also appears in the report. It is a **legacy alias for `mergeable`**, kept
+   so that an older `dev_session.sh` still gates on merge authorization. Prefer
+   `converged` / `mergeable`; never assume `done` means "the loop finished."
+
+1. **If `converged`:** stop the loop and report — PR #, the green check count, and
+   "no outstanding review findings." Then record the independent review (see below)
+   so the PR becomes `mergeable`; if `mergeable` is already true, say so.
 
 1. **If checks are still `pending` and there are no new comments:** nothing to do yet
    — wait and re-poll (see Pacing). CI can take 20–30 min; that's expected, keep
@@ -109,8 +124,8 @@ Self-pace on a bounded cadence — don't busy-wait:
 
 ## Stop conditions
 
-- **Done** — `done: true` (green + clean + current-head independent review evidence).
-  Report and finish. This is the goal.
+- **Converged** — `converged: true` (green + clean). Report and finish. This is the
+  goal of the loop. State `mergeable` too: convergence is not merge clearance.
 - **Stuck / needs a decision** — a check fails for a reason you can't resolve (a
   flaky-infra failure that won't clear on re-run; an external dependency; a finding
   that needs an operator product/design call). Stop, report the specific blocker, and
@@ -124,9 +139,10 @@ Self-pace on a bounded cadence — don't busy-wait:
   re-running on a different PR starts fresh.
 - Known auto-noise from your review bots (walkthrough / "no actionable comments"
   summaries) is filtered out by the engine. Reviewer-unavailable notices are
-  deliberately *not* noise: they surface and block `done`; acknowledging one still
-  leaves the current-head review-evidence blocker until the configured fallback runs
-  and records its receipt.
+  deliberately *not* noise: they surface as new comments and so block `converged`;
+  acknowledging one clears `converged` but still leaves the current-head
+  review-evidence blocker on `mergeable` until the configured fallback runs and
+  records its receipt.
 - **Tune this for your own bot mix in `config/dev-model.yaml`, never in the engine.**
   `review.noise_markers`, `review.unavailable_markers` and
   `review.informational_checks` are read from config; the engine only carries them
@@ -136,10 +152,11 @@ Self-pace on a bounded cadence — don't busy-wait:
   (`noise_markers: []`) means "filter nothing".
 - `review.require_ci` (default `true`) is whether a PR must have at least one real,
   non-informational check before it can report green. Leave it `true` unless the repo
-  genuinely has no CI — with no checks and `require_ci: true`, `done` can never flip
+  genuinely has no CI — with no checks and `require_ci: true`, `converged` can never flip
   and `dev_session.sh merge` will always refuse. Setting it `false` does **not**
-  weaken the review gate: `done` still requires a current-head independent-review
-  receipt, which then becomes the only quality gate — so set it deliberately.
+  weaken the review gate: `mergeable` still requires a current-head
+  independent-review receipt, which then becomes the only quality gate — so set it
+  deliberately.
 - This is interactive-only. A scheduled job that opens its own PRs should be excluded
   from this loop by your cron/CI runner's env signal (any of `DEVKIT_CI_ENV_VARS`,
   default `JOB_NAME,CI,GITHUB_ACTIONS,GITLAB_CI,BUILDKITE`), so an automated open

kit-manifest.json

@@ -23,7 +23,7 @@
     },
     "docs/agentic-dev-kit/workflows/pr-watch.md": {
       "role": "workflow",
-      "sha256": "169d3e27e149a49f736931b838d117a2fe6ad3ac6ddcaefccb99a51b97b505c8"
+      "sha256": "0b187c8ea7d725872215b4d4a5e56929814011228f7979adf1aa8423652048c8"
     },
     "docs/agentic-dev-kit/workflows/session-start.md": {
       "role": "workflow",
@@ -59,7 +59,7 @@
     },
     "scripts/dev_session.sh": {
       "role": "engine",
-      "sha256": "51626c216d361a3dd5e117eab0fdf443c8d3a0438fbd4bc3a8aec63c34d6dd50"
+      "sha256": "52d962af8088e70fd7b4682d67ec7d6d4354c70f3d8db49b724d4c42c04d98f6"
     },
     "scripts/hooks/pre-push": {
       "role": "hook",
@@ -99,7 +99,7 @@
     },
     "scripts/pr_watch.py": {
       "role": "engine",
-      "sha256": "8e1fa0e8e14cc4e26172a91ea675194f7f985592a139ac5a09dd9def97deecd1"
+      "sha256": "735428aa7a06141da7b3d385324a350e7a722fd2c2c47d0e5f97047fda41df37"
     },
     "scripts/reconcile_sessions.sh": {
       "role": "engine",

scripts/dev_session.sh

@@ -728,7 +728,7 @@ cmd_merge() {
 
     local merge_class="operator" branch="" base="$DEFAULT_BASE"
     local resolved repo_nwo pr
-    local report done validated_pr validated_base validated_head
+    local report mergeable validated_pr validated_base validated_head
     [[ -s "$session_dir/merge_class" ]] && merge_class="$(cat "$session_dir/merge_class")"
     [[ "$merge_class" == "self" ]] \
         || _die "lane '$scope' is operator-merge (or missing metadata); autonomous merge refused"
@@ -746,12 +746,18 @@ cmd_merge() {
     report="$(GH_REPO="$repo_nwo" DEVKIT_STATE_ROOT="$session_dir/state" \
         uv run "$SCRIPT_DIR/pr_watch.py" "$pr" --json)" \
         || _die "pr-watch failed for PR #$pr"
-    IFS=$'\t' read -r done validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c '
+    # Gate on `mergeable`, NOT `done`. `done` is the watch-loop predicate
+    # ("anything left to fix?") and is deliberately true on a green, comment-clean
+    # PR that carries no review receipt. `mergeable` is the merge-gate predicate:
+    # done AND no deterministic merge blocker AND a receipt bound to this head.
+    # Read the flag pr_watch computes — never re-derive it here, or this gate
+    # becomes a second copy of the contract that can drift from the engine's.
+    IFS=$'\t' read -r mergeable validated_pr validated_base validated_head <<< "$(printf '%s' "$report" | python3 -c '
 import json, sys
 d = json.load(sys.stdin)
-print("\t".join(("true" if d.get("done") is True else "false", str(d.get("pr") or ""), str(d.get("base") or ""), str(d.get("head") or ""))))
+print("\t".join(("true" if d.get("mergeable") is True else "false", str(d.get("pr") or ""), str(d.get("base") or ""), str(d.get("head") or ""))))
 ')"
-    [[ "$done" == "true" ]] \
+    [[ "$mergeable" == "true" ]] \
         || _die "PR #$pr is not green, review-clean, and merge-ready; run pr-watch to convergence first"
     [[ "$validated_pr" == "$pr" ]] \
         || _die "pr-watch validated PR #$validated_pr, not resolved PR #$pr"

scripts/pr_watch.py

@@ -11,13 +11,27 @@
 comment surface (issue comments, review submissions, inline review comments),
 filters out known auto-noise (while surfacing reviewer-unavailability notices),
 diffs against a per-PR seen-set so only *new actionable* comments surface, and
-reports whether the PR is `done` (checks green, nothing new, merge-ready, and
-independently reviewed at the current head).
+reports two distinct predicates:
 
-The caller loops: run this -> if not done, fix the failures / address or reply
-to the new comments -> `--mark-seen` -> wait -> run again. `done` flips true
-once CI is green, every finding has been handled, and an explicitly recorded
-independent-review receipt covers the current head.
+- `converged` — checks green, nothing new to act on, not mid-settle. The
+  WATCH-LOOP predicate: "is there more for me to fix?"
+- `mergeable` — `converged` AND no deterministic merge blocker AND an
+  independent-review receipt bound to the current head. The MERGE-GATE
+  predicate, re-checked by `dev_session.sh merge` at act time.
+
+A `converged` PR is not necessarily `mergeable`. Keeping them separate is what
+lets a caller watch to convergence without being forced to record a review
+receipt just to terminate the loop (see `decide_converged`).
+
+`done` is a LEGACY alias, always equal to `mergeable`. Its meaning is unchanged
+and must stay that way: engine upgrades are per-file, so a new `pr_watch.py` can
+run against an older `dev_session.sh` that gates merges on `done` — repurposing
+the key would silently authorize merges on unreviewed PRs.
+
+The caller loops: run this -> if not converged, fix the failures / address or
+reply to the new comments -> `--mark-seen` -> wait -> run again. `converged`
+flips true once CI is green and every finding has been handled; `mergeable`
+additionally waits on `--record-review`.
 
 `--mark-seen` NEVER re-polls `gh`. Every plain poll (any invocation without
 `--mark-seen`) persists the exact ``all_seen_keys`` it just reported into a
@@ -52,8 +66,8 @@
 right before `gh pr merge`.
 
 Exit codes:
-    0 — reported (regardless of done/not-done; check `done` in the output),
-        or the draft-bit assertion held/was corrected successfully
+    0 — reported (regardless of the verdict; check `converged` / `mergeable` in
+        the output), or the draft-bit assertion held/was corrected successfully
     2 — usage error (no PR found, gh failure), or a draft-bit assertion that
         failed to correct (`ok: false`)
 """
@@ -413,14 +427,14 @@ def summarize_checks(rollup: list[dict], *, require_ci: bool | None = None) -> d
     - ``True`` — a PR with zero blocking checks is **not** green. This is the
       safe default: it stops an autonomous merge on a PR whose CI never ran.
     - ``False`` — a zero-check PR can be green. Needed for a repo with no CI at
-      all, where the ``blocking_total > 0`` clause otherwise makes ``done``
+      all, where the ``blocking_total > 0`` clause otherwise makes ``converged``
       unreachable forever, so the watch loop never terminates and
       ``dev_session.sh merge`` always refuses.
 
-    ``False`` does not remove the quality gate: :func:`decide_done` separately
-    requires an independent-review receipt bound to the *current* head, so on a
-    CI-less repo that receipt becomes the only gate — which is why the flag is
-    opt-in per repo rather than inferred from an empty rollup.
+    ``False`` does not remove the quality gate: :func:`decide_mergeable`
+    separately requires an independent-review receipt bound to the *current*
+    head, so on a CI-less repo that receipt becomes the only gate — which is why
+    the flag is opt-in per repo rather than inferred from an empty rollup.
     """
     if require_ci is None:
         require_ci = _REQUIRE_CI
@@ -588,27 +602,86 @@ def new_actionable(comments: list[dict], seen: set[str]) -> list[dict]:
     ]
 
 
-def decide_done(
+def decide_converged(
     checks: dict,
     new_items: list[dict],
     *,
-    merge_blockers: list[str] | None = None,
-    review_evidence: bool = False,
     settling: bool = False,
 ) -> bool:
-    """Done = green, independently reviewed, merge-ready, and not mid-settle.
+    """Converged = green, nothing left to act on, and not mid-settle.
+
+    The **watch-loop** predicate: it answers "is there more for me to fix?",
+    which is the only question the poll/fix/mark-seen loop needs. It deliberately
+    does NOT mean "safe to merge" — see :func:`decide_mergeable`.
+
+    This exists because merge authorization used to be the *only* thing a caller
+    could ask for. A loop that has genuinely finished still reported not-done
+    until someone recorded a review receipt, which (a) wedges any caller that
+    watches to convergence without recording one, and (b) pressures the operator
+    into recording a receipt early just to terminate the loop — exactly the
+    premature-receipt failure tracked in issue #19.
 
     ``settling`` is set right after a push (the PR head SHA moved, or the rollup
     is smaller than the largest seen for this head — new checks not yet
     registered), so a poll can't false-settle on the *stale pre-push* rollup
     (an all-green old commit) before the new commit's CI even starts.
     """
-    return (
-        checks["all_green"]
-        and not new_items
-        and not merge_blockers
-        and review_evidence
-        and not settling
+    return checks["all_green"] and not new_items and not settling
+
+
+def decide_mergeable(
+    converged: bool,
+    *,
+    merge_blockers: list[str] | None = None,
+    review_evidence: bool = False,
+) -> bool:
+    """Mergeable = the watch loop converged AND the merge is authorized.
+
+    Strictly stronger than :func:`decide_converged`: a PR must first have nothing
+    left to act on, and additionally carry no deterministic merge blocker (draft,
+    non-open, blocked merge state, changes requested) and an independent-review
+    receipt bound to the *current* head.
+
+    This is what an autonomous self-merge gates on (``dev_session.sh merge``).
+
+    The result is coerced to ``bool`` deliberately: a bare ``and`` chain returns
+    its last operand, so a truthy non-bool ``review_evidence`` would propagate
+    into the report — and ``dev_session.sh merge`` tests the JSON value with an
+    identity check (``is True``), which such a value fails *closed* but
+    confusingly. A safety gate should not depend on every caller passing a real
+    bool.
+    """
+    return bool(converged and not merge_blockers and review_evidence)
+
+
+def decide_done(
+    checks: dict,
+    new_items: list[dict],
+    *,
+    merge_blockers: list[str] | None = None,
+    review_evidence: bool = False,
+    settling: bool = False,
+) -> bool:
+    """Legacy name for :func:`decide_mergeable`. Semantics UNCHANGED.
+
+    ``done`` predates the split of the watch-loop predicate from the merge-gate
+    predicate, and has always meant "green, independently reviewed, merge-ready,
+    and not mid-settle". This function keeps that meaning exactly, for any Python
+    caller that imported it.
+
+    **Do not read a compatibility guarantee into this function.** The thing that
+    protects an older ``dev_session.sh`` is the report's ``done`` **key** (see
+    the assignment in :func:`build_report`), because that gate shells out to
+    ``pr_watch.py --json`` and never imports this module. This function has no
+    in-engine caller. Deleting the key while keeping this function would remove
+    the protection entirely.
+
+    Prefer :func:`decide_converged` / :func:`decide_mergeable` in new code.
+    """
+    return decide_mergeable(
+        decide_converged(checks, new_items, settling=settling),
+        merge_blockers=merge_blockers,
+        review_evidence=review_evidence,
     )
 
 
@@ -752,7 +825,7 @@ def build_report(
       moved since ``prior_head``; ``max_total`` — the largest check count seen for
       this head (persisted across runs); ``settling`` — true while a just-pushed
       commit's checks are still registering (the false-settle guard; forces
-      ``done`` false). See :func:`decide_done`.
+      ``converged`` false). See :func:`decide_converged`.
     - ``checks`` — the :func:`summarize_checks` rollup (``total`` / ``success`` /
       ``pending`` / ``informational`` / ``failing`` / ``all_green``).
     - ``new_comments`` — only the *fresh, actionable* comments (not in ``seen``,
@@ -769,8 +842,15 @@ def build_report(
     - ``merge_blockers`` — deterministic reasons the PR is not currently safe to
       merge (draft, blocked/unknown merge state, requested changes, non-open PR,
       or missing current-head review evidence).
-    - ``done`` — :func:`decide_done`: all checks green, current-head review
-      evidence, no fresh comments, no merge blockers, and not ``settling``.
+    - ``converged`` — :func:`decide_converged`: all checks green, no fresh
+      comments, and not ``settling``. The **watch-loop** predicate: "is there
+      more to fix?" A converged PR is NOT necessarily safe to merge.
+    - ``mergeable`` — :func:`decide_mergeable`: ``converged`` AND no
+      ``merge_blockers`` AND current-head review evidence. The **merge-gate**
+      predicate, and what ``dev_session.sh merge`` re-checks at act time.
+    - ``done`` — legacy alias, always equal to ``mergeable``. Kept so an older
+      ``dev_session.sh`` reading ``done`` still gates on merge authorization
+      rather than falling open; see :func:`decide_done`.
     """
     checks = summarize_checks(view.get("statusCheckRollup") or [])
     comments = collect_comments(view, inline)
@@ -860,20 +940,31 @@ def build_report(
             {k for c in comments for k in (c["key"], c["content_key"])}
         ),
     }
-    report["done"] = decide_done(
-        checks,
-        fresh,
+    report["converged"] = decide_converged(checks, fresh, settling=settling)
+    report["mergeable"] = decide_mergeable(
+        report["converged"],
         merge_blockers=merge_blockers,
         review_evidence=review_evidence["valid"],
-        settling=settling,
     )
+    # Legacy alias, identical to `mergeable` — see :func:`decide_done` for why
+    # this key must never be repurposed to mean watch-convergence.
+    report["done"] = report["mergeable"]
     return report
 
 
 def render(report: dict) -> str:
     ck = report["checks"]
     lines = [f"PR #{report['pr']} — {report['url']}"]
-    state = "✅ DONE — green + clean" if report["done"] else "⏳ not done"
+    # `converged` answers "anything left to fix?"; `mergeable` answers "safe to
+    # merge?". Naming the converged-but-unauthorized state explicitly is the
+    # point: it is the normal end of a watch loop, not a failure, and it must not
+    # read as merge clearance.
+    if not report.get("converged"):
+        state = "⏳ not converged"
+    elif report.get("mergeable"):
+        state = "✅ DONE — green, reviewed, merge-ready"
+    else:
+        state = "✅ converged — green + clean · NOT mergeable (see merge blockers below)"
     if report.get("settling"):
         state += " (settling — new commit pushed; waiting for its checks to register)"
     lines.append(state)