diff --git a/config/dev-model.yaml b/config/dev-model.yaml index 11537aa..ec4f9b2 100644 --- a/config/dev-model.yaml +++ b/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 diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 07b596d..6409a85 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -27,16 +27,31 @@ that `dev_session.sh merge ` re-checks. ## Loop -Repeat until the report says **done**: +Repeat until the report says **converged**: 1. **Poll.** `uv run /pr_watch.py --json` (omit `` 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 diff --git a/kit-manifest.json b/kit-manifest.json index 08d36ef..41a24ec 100644 --- a/kit-manifest.json +++ b/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", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 6df4304..92bd926 100755 --- a/scripts/dev_session.sh +++ b/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" diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 2fc93ba..037d245 100755 --- a/scripts/pr_watch.py +++ b/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) diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index 50fae14..b395a85 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -485,6 +485,66 @@ def test_self_merge_refuses_wrong_base_and_binds_gh_to_repo(tmp_path: Path) -> N assert f"{repo}|owner/project|pr list" in calls +def test_self_merge_refuses_a_report_that_is_done_but_not_mergeable( + tmp_path: Path, +) -> None: + """The gate reads `mergeable`, and fails CLOSED when it is false or absent. + + Two cases, deliberately pinned together: + + - **`mergeable: false` alongside `done: true`.** These are equal in any report + the engine actually produces, so this input is intentionally inconsistent: + it proves the gate reads `mergeable` and does not quietly fall back to + `done` if the two ever diverge. + - **`mergeable` absent entirely.** An OLDER or foreign ``pr_watch`` predating + the field emits only ``done``. The gate must read that as "not authorized" + rather than merging on an assumption about the missing key. + + A gate that failed open in either case would merge unreviewed work. + """ + _, engine_dir, sessions = _install_real_trunk_repo(tmp_path) + _prepare_self_merge_session(sessions) + fake_bin, call_log, uv_log = _install_fake_merge_tools(tmp_path) + pr_json = json.dumps( + [ + { + "number": 8, + "baseRefName": "trunk", + "headRefName": "lane/probe", + "headRefOid": "reviewed-head", + "headRepositoryOwner": {"login": "owner"}, + } + ] + ) + base_report = {"pr": 8, "base": "trunk", "head": "reviewed-head", "done": True} + + for label, report in ( + ("explicitly not mergeable", {**base_report, "mergeable": False}), + ("pre-split engine, key absent", base_report), + ): + result = subprocess.run( + ["bash", str(engine_dir / "dev_session.sh"), "merge", "probe"], + cwd=tmp_path, + env={ + **os.environ, + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "DEVKIT_SESSIONS_DIR": str(sessions), + "CALL_LOG": str(call_log), + "UV_LOG": str(uv_log), + "PR_JSON": pr_json, + "REPORT_JSON": json.dumps(report), + "GH_REPO": "attacker/other", + }, + check=False, + capture_output=True, + text=True, + ) + + assert result.returncode != 0, label + assert "not green, review-clean, and merge-ready" in result.stderr, label + assert "pr merge" not in call_log.read_text(encoding="utf-8"), label + + def test_self_merge_pins_validated_head_so_push_race_is_refused(tmp_path: Path) -> None: repo, engine_dir, sessions = _install_real_trunk_repo(tmp_path) session = _prepare_self_merge_session(sessions) @@ -507,7 +567,13 @@ def test_self_merge_pins_validated_head_so_push_race_is_refused(tmp_path: Path) ] ), "REPORT_JSON": json.dumps( - {"pr": 8, "base": "trunk", "head": "reviewed-head", "done": True} + { + "pr": 8, + "base": "trunk", + "head": "reviewed-head", + "done": True, + "mergeable": True, + } ), # Simulate GitHub rejecting --match-head-commit because a new push won # the race after the act-time poll. @@ -561,7 +627,13 @@ def test_scope_pr_watch_and_merge_share_lane_state_and_pinned_repo( ] ), "REPORT_JSON": json.dumps( - {"pr": 8, "base": "trunk", "head": "reviewed-head", "done": True} + { + "pr": 8, + "base": "trunk", + "head": "reviewed-head", + "done": True, + "mergeable": True, + } ), "GH_REPO": "attacker/other", } diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 5c34bc7..511d069 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -40,6 +40,134 @@ def _green_view(**overrides): return view +def test_done_keeps_its_original_merge_authorization_semantics() -> None: + """`done` must still mean exactly what it meant before `converged` existed. + + This is the safety property that makes the schema change purely ADDITIVE. + Engine upgrades are per-file, so a new `pr_watch.py` can run against an older + `dev_session.sh` whose merge gate reads `done`. If `done` were repurposed to + mean watch-convergence, that pairing would authorize merges on PRs carrying + no review receipt at all — a silent fail-open on the merge gate. + + Pinned over the whole boolean input space, not just a happy path. + """ + pr_watch = _load_pr_watch() + + def original_done(checks, new_items, merge_blockers, review_evidence, settling): + return ( + checks["all_green"] + and not new_items + and not merge_blockers + and review_evidence + and not settling + ) + + comment = {"kind": "issue", "author": "a", "path": None, "line": None, "body": "x"} + for all_green in (True, False): + for new_items in ([], [comment]): + for blockers in ([], ["merge state is BLOCKED"]): + for evidence in (True, False): + for settling in (True, False): + checks = {"all_green": all_green} + expected = original_done( + checks, new_items, blockers, evidence, settling + ) + assert ( + pr_watch.decide_done( + checks, + new_items, + merge_blockers=blockers, + review_evidence=evidence, + settling=settling, + ) + is expected + ) + # ...and `mergeable` is that same predicate under its + # clearer name, composed from `converged`. + assert ( + pr_watch.decide_mergeable( + pr_watch.decide_converged( + checks, new_items, settling=settling + ), + merge_blockers=blockers, + review_evidence=evidence, + ) + is expected + ) + + +def test_report_done_is_always_identical_to_mergeable() -> None: + """The alias must never drift from the field it aliases.""" + pr_watch = _load_pr_watch() + receipt = {"head": "abc123", "source": "fallback:codex"} + + for report in ( + pr_watch.build_report(_green_view(), [], set()), + pr_watch.build_report(_green_view(), [], set(), review_receipt=receipt), + pr_watch.build_report(_green_view(isDraft=True), [], set()), + pr_watch.build_report(_green_view(state="MERGED"), [], set()), + ): + assert report["done"] is report["mergeable"] + + +def test_predicates_are_strictly_bool_typed() -> None: + """`dev_session.sh merge` tests the JSON value with `is True`. + + A bare `and` chain returns its LAST operand, so a truthy non-bool reaching + the predicate would land a non-bool in the report. That fails the gate's + identity check — closed, but confusingly, and it would serialize as e.g. + `"mergeable": 1`. Pin the type rather than trusting every caller. + """ + pr_watch = _load_pr_watch() + + assert ( + pr_watch.decide_mergeable( + True, merge_blockers=[], review_evidence=1 # truthy non-bool + ) + is True + ) + report = pr_watch.build_report( + _green_view(), [], set(), review_receipt={"head": "abc123", "source": "x"} + ) + for key in ("converged", "mergeable", "done"): + assert isinstance(report[key], bool), key + + +def test_converged_is_watch_progress_and_ignores_merge_authorization() -> None: + """The whole point of the split: a converged loop is not a merge clearance. + + A green, comment-clean PR with NO review receipt is `converged` (nothing left + for the loop to fix) and NOT `mergeable`. Before `converged` existed a caller + had only `done`, so it kept looping — and the operator was pressured into + recording a receipt early just to terminate it (issue #19). + """ + pr_watch = _load_pr_watch() + + report = pr_watch.build_report(_green_view(), [], set()) + + assert report["converged"] is True + assert report["mergeable"] is False + assert report["done"] is False + assert ( + "independent review evidence is missing for current head" + in report["merge_blockers"] + ) + + +def test_render_never_lets_convergence_read_as_merge_clearance() -> None: + pr_watch = _load_pr_watch() + receipt = {"head": "abc123", "source": "fallback:codex"} + + converged = pr_watch.render(pr_watch.build_report(_green_view(), [], set())) + authorized = pr_watch.render( + pr_watch.build_report(_green_view(), [], set(), review_receipt=receipt) + ) + + assert "NOT mergeable" in converged + assert "DONE" not in converged + assert "DONE" in authorized and "NOT mergeable" not in authorized + + def test_changes_requested_and_blocked_merge_state_never_settle_done() -> None: pr_watch = _load_pr_watch() review = { @@ -60,7 +188,10 @@ def test_changes_requested_and_blocked_merge_state_never_settle_done() -> None: report = pr_watch.build_report(view, [], seen) assert report["new_comments"] == [] - assert report["done"] is False + # The findings were acked and CI is green, so the WATCH loop has converged... + assert report["converged"] is True + # ...but the merge is still refused: blockers are a merge-gate concern. + assert report["mergeable"] is False assert "merge state is BLOCKED" in report["merge_blockers"] assert "review decision is CHANGES_REQUESTED" in report["merge_blockers"] @@ -73,9 +204,14 @@ def test_unknown_or_non_open_pr_state_never_settles_done() -> None: _green_view(state="MERGED", mergeStateStatus="UNKNOWN"), [], set() ) - assert unknown["done"] is False + # Both are green and comment-clean, so the watch loop converged; only the + # merge is refused. Asserting `converged` here too keeps the two predicates + # independently pinned, so a regression that re-couples them fails. + assert unknown["converged"] is True + assert unknown["mergeable"] is False assert "merge state is UNKNOWN" in unknown["merge_blockers"] - assert merged["done"] is False + assert merged["converged"] is True + assert merged["mergeable"] is False assert "PR state is MERGED" in merged["merge_blockers"] @@ -118,11 +254,16 @@ def test_unstable_is_allowed_only_when_remaining_check_is_informational() -> Non review_receipt=receipt, ) - assert informational_only["done"] is True + # All three converged — an informational check never blocks the watch loop. + # Only mergeability varies, and only on the merge-state blocker. + for report in (informational_only, unexplained_unstable, successful_informational): + assert report["converged"] is True + + assert informational_only["mergeable"] is True assert "merge state is UNSTABLE" not in informational_only["merge_blockers"] - assert unexplained_unstable["done"] is False + assert unexplained_unstable["mergeable"] is False assert "merge state is UNSTABLE" in unexplained_unstable["merge_blockers"] - assert successful_informational["done"] is False + assert successful_informational["mergeable"] is False assert "merge state is UNSTABLE" in successful_informational["merge_blockers"] @@ -166,7 +307,10 @@ def test_acknowledged_unavailable_notice_still_needs_review_evidence() -> None: report = pr_watch.build_report(view, [], seen) assert report["new_comments"] == [] - assert report["done"] is False + # Acking the unavailability notice ends the watch loop — there is genuinely + # nothing left to fix — but it must NOT buy merge clearance. + assert report["converged"] is True + assert report["mergeable"] is False assert ( "independent review evidence is missing for current head" in report["merge_blockers"] @@ -191,9 +335,14 @@ def test_review_receipt_must_match_current_head() -> None: review_receipt={"head": "abc123", "source": "fallback:codex"}, ) - assert missing["done"] is False - assert stale["done"] is False - assert current["done"] is True + # All three are green and comment-clean, so the watch loop has converged for + # each; only the receipt bound to the CURRENT head authorizes the merge. + for report in (missing, stale, current): + assert report["converged"] is True + + assert missing["mergeable"] is False + assert stale["mergeable"] is False + assert current["mergeable"] is True assert current["review_evidence"] == { "valid": True, "source": "fallback:codex", @@ -445,13 +594,18 @@ def test_zero_check_pr_still_needs_current_head_review_evidence( ) assert without_receipt["checks"]["all_green"] is True - assert without_receipt["done"] is False + # With require_ci false a zero-check PR converges; the receipt is then the + # ONLY thing standing between convergence and merge authorization. + for report in (without_receipt, stale_receipt, with_receipt): + assert report["converged"] is True + + assert without_receipt["mergeable"] is False assert ( "independent review evidence is missing for current head" in without_receipt["merge_blockers"] ) - assert stale_receipt["done"] is False - assert with_receipt["done"] is True + assert stale_receipt["mergeable"] is False + assert with_receipt["mergeable"] is True def test_zero_check_pr_never_settles_done_while_require_ci_holds(