From a3a5f91528718b8d411df2a6a06b3aefe0a9f9c9 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 11:39:07 +0300 Subject: [PATCH 1/6] fix(pr-watch): split `done` into watch-convergence and merge-authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `decide_done` meant two things at once: "is there more for me to fix?" (what the poll/fix/mark-seen loop needs) and "is this authorized to merge?" (what `dev_session.sh merge` needs). Merge authorization was folded into `done` because that was the only hook `cmd_merge` had. The conflation has two costs. A watch loop that has genuinely finished reports `done: false` forever until someone records a review receipt — so any caller that watches to convergence without recording one wedges, and the operator is pressured into recording a receipt early just to terminate the loop (#19, where a receipt was recorded against a merely-queued CodeRabbit on #16 and four valid findings landed after the merge). Split the predicate: decide_done(checks, new_items, *, settling) # watch loop decide_mergeable(done, *, merge_blockers, review_evidence) # merge gate `build_report` emits both; `cmd_merge` now gates on `mergeable`, reading the flag the engine computes rather than re-deriving the predicate in bash — a restated contract is the drift that bit `parallel.md` last session. The gate is authorization-preserving, not merely still-passing: `mergeable` == the pre-split `done` expression across all 32 input combinations, pinned by test. Missing/false `mergeable` fails CLOSED, so an older or foreign `pr_watch` that predates the split cannot authorize a merge on the old semantics. Per `.claude/rules/safety-critical-changes.md` this is operator-merge class and needs dual-lens review before merge. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- docs/agentic-dev-kit/workflows/pr-watch.md | 37 ++++--- kit-manifest.json | 6 +- scripts/dev_session.sh | 12 ++- scripts/pr_watch.py | 94 +++++++++++++----- scripts/tests/test_portability.py | 75 +++++++++++++- scripts/tests/test_pr_watch.py | 108 ++++++++++++++++++--- 6 files changed, 274 insertions(+), 58 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 07b596d..3226d8a 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -30,13 +30,25 @@ that `dev_session.sh merge ` re-checks. Repeat until the report says **done**: 1. **Poll.** `uv run /pr_watch.py --json` (omit `` for the current - branch). Read `done`, `checks` (`all_green`, `failing[]`, `pending`), + branch). Read `done`, `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: + + - **`done`** — "is there more for me to fix?" Green, nothing new, not settling. + This is what ends *this* loop. + - **`mergeable`** — "is this authorized to merge?" `done` **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 `done` 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. + +1. **If `done`:** 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. Never treat + `done` on its own as clearance to merge. 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 +121,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. +- **Done** — `done: true` (green + clean). Report and finish. This is the goal of the + loop. Check `mergeable` too and state it: `done` alone 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 +136,9 @@ 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 `done`; + acknowledging one clears `done` 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 @@ -138,8 +150,9 @@ Self-pace on a bounded cadence — don't busy-wait: 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 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..c8fa24c 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": "26c1afa4b8231cdb22d6e61d564b4daa420451868e6c8e84a8419b7fab8db921" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "51626c216d361a3dd5e117eab0fdf443c8d3a0438fbd4bc3a8aec63c34d6dd50" + "sha256": "7e8d98feb8ac956a3dff7d7a5957afa493402fa2b5130b33236b1d457363f93b" }, "scripts/hooks/pre-push": { "role": "hook", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "8e1fa0e8e14cc4e26172a91ea675194f7f985592a139ac5a09dd9def97deecd1" + "sha256": "a998b96cbc95459aea8e5eab1eb040dd49f82a17e7765d964da404d42f1e13e2" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 6df4304..3f17500 100755 --- a/scripts/dev_session.sh +++ b/scripts/dev_session.sh @@ -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..70f76e8 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -11,13 +11,22 @@ 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: + +- `done` — checks green, nothing new to act on, not mid-settle. The WATCH-LOOP + predicate: "is there more for me to fix?" +- `mergeable` — `done` 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 `done` 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_done`). 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. +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 @@ -417,10 +426,10 @@ def summarize_checks(rollup: list[dict], *, require_ci: bool | None = None) -> d 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 @@ -592,24 +601,47 @@ def decide_done( 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. + """Done = green, nothing left to act on, and not mid-settle. + + This is the **watch-loop** predicate and nothing more: 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` for that. + + Keeping the two separate is load-bearing. Folding merge authorization into + ``done`` makes a watch loop that has genuinely finished report ``done: + false`` forever until someone records 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( + done: 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_done`: 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 — not ``done`` — is what an autonomous self-merge gates on + (``dev_session.sh merge``). + """ + return done and not merge_blockers and review_evidence # ------------------------------------------------------------------ state I/O @@ -769,8 +801,12 @@ 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``. + - ``done`` — :func:`decide_done`: all checks green, no fresh comments, and + not ``settling``. The **watch-loop** predicate: "is there more to fix?" + A ``done`` PR is NOT necessarily safe to merge. + - ``mergeable`` — :func:`decide_mergeable`: ``done`` AND no + ``merge_blockers`` AND current-head review evidence. The **merge-gate** + predicate, and what ``dev_session.sh merge`` re-checks at act time. """ checks = summarize_checks(view.get("statusCheckRollup") or []) comments = collect_comments(view, inline) @@ -860,12 +896,11 @@ def build_report( {k for c in comments for k in (c["key"], c["content_key"])} ), } - report["done"] = decide_done( - checks, - fresh, + report["done"] = decide_done(checks, fresh, settling=settling) + report["mergeable"] = decide_mergeable( + report["done"], merge_blockers=merge_blockers, review_evidence=review_evidence["valid"], - settling=settling, ) return report @@ -873,7 +908,16 @@ def build_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" + # `done` answers "anything left to fix?"; `mergeable` answers "safe to + # merge?". Rendering only the former would let "✅ DONE" be misread as merge + # authorization — the exact conflation this split exists to remove — so a + # converged-but-unauthorized PR says so on the same line. + if not report["done"]: + state = "⏳ not done" + elif report.get("mergeable"): + state = "✅ DONE — green + clean · mergeable" + else: + state = "✅ DONE — 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..2d7c7d7 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -485,6 +485,65 @@ 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: + """`done` alone must never authorize a merge — the gate reads `mergeable`. + + Two failures at once, both fail-CLOSED by design: + + - the normal case — a green, comment-clean PR with no review receipt is + legitimately ``done`` while ``mergeable`` is false; + - the compatibility case — an OLDER or foreign ``pr_watch`` that predates the + split emits ``done`` and no ``mergeable`` key at all, which must read as + "not authorized" rather than silently merging on the old semantics. + + A gate that failed open here would merge unreviewed work, so the missing-key + path is pinned as deliberately as the false one. + """ + _, 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 +566,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 +626,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..3e5e634 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -40,6 +40,79 @@ def _green_view(**overrides): return view +def test_mergeable_is_exactly_the_pre_split_done_predicate() -> None: + """Splitting `done` must not have weakened the merge gate. + + Before the split, `dev_session.sh merge` gated on + ``all_green and not new_items and not merge_blockers and review_evidence and + not settling``. After it, the gate reads ``mergeable``. This pins that the two + are the same boolean over the whole input space, so the refactor is provably + authorization-preserving rather than merely "tests still pass". + """ + pr_watch = _load_pr_watch() + + def pre_split_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} + done = pr_watch.decide_done( + checks, new_items, settling=settling + ) + assert pr_watch.decide_mergeable( + done, + merge_blockers=blockers, + review_evidence=evidence, + ) is pre_split_done( + checks, new_items, blockers, evidence, settling + ) + + +def test_done_is_watch_convergence_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 `done` (nothing left for + the loop to fix) and NOT `mergeable`. Under the pre-split semantics this + reported `done: false`, which wedged any caller watching to convergence + without recording a receipt — and pressured the operator into recording one + early just to terminate the loop (issue #19). + """ + pr_watch = _load_pr_watch() + + report = pr_watch.build_report(_green_view(), [], set()) + + assert report["done"] is True + assert report["mergeable"] is False + assert ( + "independent review evidence is missing for current head" + in report["merge_blockers"] + ) + + +def test_render_never_lets_done_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 "mergeable" 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 +133,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["done"] 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 +149,9 @@ def test_unknown_or_non_open_pr_state_never_settles_done() -> None: _green_view(state="MERGED", mergeStateStatus="UNKNOWN"), [], set() ) - assert unknown["done"] is False + assert unknown["mergeable"] is False assert "merge state is UNKNOWN" in unknown["merge_blockers"] - assert merged["done"] is False + assert merged["mergeable"] is False assert "PR state is MERGED" in merged["merge_blockers"] @@ -118,11 +194,11 @@ def test_unstable_is_allowed_only_when_remaining_check_is_informational() -> Non review_receipt=receipt, ) - assert informational_only["done"] 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 +242,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["done"] is True + assert report["mergeable"] is False assert ( "independent review evidence is missing for current head" in report["merge_blockers"] @@ -191,9 +270,12 @@ 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 is done for each; + # only the receipt bound to the CURRENT head authorizes the merge. + assert missing["done"] 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 +527,13 @@ 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 + 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( From 74da7c8bd60fd25de8439a9558eee779f1fc1ea0 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 11:40:35 +0300 Subject: [PATCH 2/6] docs(config): correct the require_ci comment to name `mergeable` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment claimed `done` requires a current-head review receipt. After the predicate split that is `mergeable`'s job — `done` is watch-convergence only. Found by sweeping every remaining reference to the old semantics; both runtime adapters were already clean because they delegate to the shared workflow doc rather than restating it. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- config/dev-model.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/dev-model.yaml b/config/dev-model.yaml index 11537aa..e91325f 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -121,8 +121,8 @@ review: # pr_watch will report `done`. 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 From cf512c55f389036bfc0a8e69a4dacba1dcc8720d Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 11:47:29 +0300 Subject: [PATCH 3/6] refactor(pr-watch): make the predicate split purely additive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit found a fail-open. Engine upgrades are per-file (`/upgrade` Step 3 treats `missing` as a supported state — "a sized-down adoption omits engines deliberately"), so a repo can run the new `pr_watch.py` against an older `dev_session.sh`. That gate reads `done` — and with `done` redefined to mean watch-convergence, it would have authorized merges on PRs with no review receipt at all. Redefining a field that a safety gate reads is unsafe regardless of how the new meaning is documented, because the old reader never sees the documentation. So nothing changes meaning; the schema only grows: converged = all_green && !new_items && !settling (new: watch loop) mergeable = converged && !blockers && review_evidence (new: merge gate) done = mergeable (UNCHANGED) `decide_done` keeps its original signature and semantics, now composed from the two new predicates. An older `dev_session.sh` reading `done` still gates on merge authorization; a newer one reads `mergeable` and fails closed when the key is absent, so both skew directions are safe. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- config/dev-model.yaml | 2 +- docs/agentic-dev-kit/workflows/pr-watch.md | 40 ++++--- kit-manifest.json | 4 +- scripts/pr_watch.py | 126 ++++++++++++++------- scripts/tests/test_portability.py | 19 ++-- scripts/tests/test_pr_watch.py | 94 ++++++++++----- 6 files changed, 182 insertions(+), 103 deletions(-) diff --git a/config/dev-model.yaml b/config/dev-model.yaml index e91325f..ec4f9b2 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -118,7 +118,7 @@ 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, `mergeable` still requires a current-head review receipt diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 3226d8a..6409a85 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -27,28 +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`, `mergeable`, `checks` (`all_green`, `failing[]`, `pending`), + branch). Read `converged`, `mergeable`, `checks` (`all_green`, `failing[]`, `pending`), `merge_blockers[]`, `review_evidence`, and `new_comments[]`. The two predicates answer different questions and you need both: - - **`done`** — "is there more for me to fix?" Green, nothing new, not settling. - This is what ends *this* loop. - - **`mergeable`** — "is this authorized to merge?" `done` **plus** no + - **`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 `done` 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. + 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. -1. **If `done`:** 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. Never treat - `done` on its own as clearance to merge. + `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 @@ -121,8 +124,8 @@ Self-pace on a bounded cadence — don't busy-wait: ## Stop conditions -- **Done** — `done: true` (green + clean). Report and finish. This is the goal of the - loop. Check `mergeable` too and state it: `done` alone is not merge clearance. +- **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 @@ -136,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 as new comments and so block `done`; - acknowledging one clears `done` but still leaves the current-head review-evidence - blocker on `mergeable` 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 @@ -148,7 +152,7 @@ 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: `mergeable` still requires a current-head independent-review receipt, which then becomes the only quality gate — so set it diff --git a/kit-manifest.json b/kit-manifest.json index c8fa24c..91a1066 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "26c1afa4b8231cdb22d6e61d564b4daa420451868e6c8e84a8419b7fab8db921" + "sha256": "0b187c8ea7d725872215b4d4a5e56929814011228f7979adf1aa8423652048c8" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "a998b96cbc95459aea8e5eab1eb040dd49f82a17e7765d964da404d42f1e13e2" + "sha256": "9ec8782d747d5d9173219b18700a8d0e10fdac0145aa8c3802b755e6322ce8f3" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 70f76e8..0087f22 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -13,20 +13,25 @@ diffs against a per-PR seen-set so only *new actionable* comments surface, and reports two distinct predicates: -- `done` — checks green, nothing new to act on, not mid-settle. The WATCH-LOOP - predicate: "is there more for me to fix?" -- `mergeable` — `done` AND no deterministic merge blocker AND an +- `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 `done` 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_done`). +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`). -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 and every finding has been handled; `mergeable` additionally -waits on `--record-review`. +`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 @@ -422,7 +427,7 @@ 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. @@ -597,25 +602,24 @@ def new_actionable(comments: list[dict], seen: set[str]) -> list[dict]: ] -def decide_done( +def decide_converged( checks: dict, new_items: list[dict], *, settling: bool = False, ) -> bool: - """Done = green, nothing left to act on, and not mid-settle. + """Converged = green, nothing left to act on, and not mid-settle. - This is the **watch-loop** predicate and nothing more: 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` for that. + 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`. - Keeping the two separate is load-bearing. Folding merge authorization into - ``done`` makes a watch loop that has genuinely finished report ``done: - false`` forever until someone records 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. + 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 @@ -626,22 +630,54 @@ def decide_done( def decide_mergeable( - done: bool, + 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_done`: a PR must first have nothing + 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 — not ``done`` — is what an autonomous self-merge gates on - (``dev_session.sh merge``). + This is what an autonomous self-merge gates on (``dev_session.sh merge``). """ - return done and not merge_blockers and review_evidence + return 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". It keeps that meaning exactly, and the report keeps + emitting a ``done`` key equal to ``mergeable``. + + **This is deliberate, and it is a safety property, not politeness.** Engine + upgrades are per-file (a sized-down adoption may install only some engines), + so a repo can end up running a new ``pr_watch.py`` against an older + ``dev_session.sh`` whose merge gate reads ``done``. Had ``done`` been + redefined to mean mere watch-convergence, that combination would have + authorized merges on PRs with **no review receipt at all** — a silent + fail-open on the merge gate. Making the schema change purely additive is what + removes that hazard. + + 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, + ) # ------------------------------------------------------------------ state I/O @@ -801,12 +837,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, no fresh comments, and - not ``settling``. The **watch-loop** predicate: "is there more to fix?" - A ``done`` PR is NOT necessarily safe to merge. - - ``mergeable`` — :func:`decide_mergeable`: ``done`` AND no + - ``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) @@ -896,28 +935,31 @@ def build_report( {k for c in comments for k in (c["key"], c["content_key"])} ), } - report["done"] = decide_done(checks, fresh, settling=settling) + report["converged"] = decide_converged(checks, fresh, settling=settling) report["mergeable"] = decide_mergeable( - report["done"], + report["converged"], merge_blockers=merge_blockers, review_evidence=review_evidence["valid"], ) + # 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']}"] - # `done` answers "anything left to fix?"; `mergeable` answers "safe to - # merge?". Rendering only the former would let "✅ DONE" be misread as merge - # authorization — the exact conflation this split exists to remove — so a - # converged-but-unauthorized PR says so on the same line. - if not report["done"]: - state = "⏳ 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 + clean · mergeable" + state = "✅ DONE — green, reviewed, merge-ready" else: - state = "✅ DONE — green + clean · NOT mergeable (see merge blockers below)" + 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 2d7c7d7..b395a85 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -488,18 +488,19 @@ def test_self_merge_refuses_wrong_base_and_binds_gh_to_repo(tmp_path: Path) -> N def test_self_merge_refuses_a_report_that_is_done_but_not_mergeable( tmp_path: Path, ) -> None: - """`done` alone must never authorize a merge — the gate reads `mergeable`. + """The gate reads `mergeable`, and fails CLOSED when it is false or absent. - Two failures at once, both fail-CLOSED by design: + Two cases, deliberately pinned together: - - the normal case — a green, comment-clean PR with no review receipt is - legitimately ``done`` while ``mergeable`` is false; - - the compatibility case — an OLDER or foreign ``pr_watch`` that predates the - split emits ``done`` and no ``mergeable`` key at all, which must read as - "not authorized" rather than silently merging on the old semantics. + - **`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 here would merge unreviewed work, so the missing-key - path is pinned as deliberately as the false one. + 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) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 3e5e634..9df27bb 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -40,18 +40,20 @@ def _green_view(**overrides): return view -def test_mergeable_is_exactly_the_pre_split_done_predicate() -> None: - """Splitting `done` must not have weakened the merge gate. - - Before the split, `dev_session.sh merge` gated on - ``all_green and not new_items and not merge_blockers and review_evidence and - not settling``. After it, the gate reads ``mergeable``. This pins that the two - are the same boolean over the whole input space, so the refactor is provably - authorization-preserving rather than merely "tests still pass". +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 pre_split_done(checks, new_items, merge_blockers, review_evidence, settling): + def original_done(checks, new_items, merge_blockers, review_evidence, settling): return ( checks["all_green"] and not new_items @@ -67,40 +69,69 @@ def pre_split_done(checks, new_items, merge_blockers, review_evidence, settling) for evidence in (True, False): for settling in (True, False): checks = {"all_green": all_green} - done = pr_watch.decide_done( - checks, new_items, settling=settling - ) - assert pr_watch.decide_mergeable( - done, - merge_blockers=blockers, - review_evidence=evidence, - ) is pre_split_done( + 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"} -def test_done_is_watch_convergence_and_ignores_merge_authorization() -> None: + 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_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 `done` (nothing left for - the loop to fix) and NOT `mergeable`. Under the pre-split semantics this - reported `done: false`, which wedged any caller watching to convergence - without recording a receipt — and pressured the operator into recording one - early just to terminate the loop (issue #19). + 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["done"] is True + 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_done_read_as_merge_clearance() -> None: +def test_render_never_lets_convergence_read_as_merge_clearance() -> None: pr_watch = _load_pr_watch() receipt = {"head": "abc123", "source": "fallback:codex"} @@ -110,7 +141,8 @@ def test_render_never_lets_done_read_as_merge_clearance() -> None: ) assert "NOT mergeable" in converged - assert "mergeable" in authorized and "NOT mergeable" not in authorized + 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: @@ -134,7 +166,7 @@ def test_changes_requested_and_blocked_merge_state_never_settle_done() -> None: assert report["new_comments"] == [] # The findings were acked and CI is green, so the WATCH loop has converged... - assert report["done"] is True + 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"] @@ -244,7 +276,7 @@ def test_acknowledged_unavailable_notice_still_needs_review_evidence() -> None: assert report["new_comments"] == [] # Acking the unavailability notice ends the watch loop — there is genuinely # nothing left to fix — but it must NOT buy merge clearance. - assert report["done"] is True + assert report["converged"] is True assert report["mergeable"] is False assert ( "independent review evidence is missing for current head" @@ -270,9 +302,9 @@ def test_review_receipt_must_match_current_head() -> None: review_receipt={"head": "abc123", "source": "fallback:codex"}, ) - # All three are green and comment-clean, so the watch loop is done for each; - # only the receipt bound to the CURRENT head authorizes the merge. - assert missing["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. + assert missing["converged"] is True assert missing["mergeable"] is False assert stale["mergeable"] is False assert current["mergeable"] is True From bb4f539c05aeaaae8c00cc24b338d16e09d4575f Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 11:48:38 +0300 Subject: [PATCH 4/6] docs(pr-watch): update the exit-code note to name the current predicates Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- kit-manifest.json | 2 +- scripts/pr_watch.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index 91a1066..53584e2 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "9ec8782d747d5d9173219b18700a8d0e10fdac0145aa8c3802b755e6322ce8f3" + "sha256": "8c890dbe330ba8d3103935cd91b3d8dc6e3eccd48588d341cd940a948686d580" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 0087f22..3ecb1fb 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -66,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`) """ From 3d61512cf13a3c6f76747f5dc0f09201aefbfefd Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 11:50:04 +0300 Subject: [PATCH 5/6] fix(dev-session): declare `mergeable` local; pin `converged` alongside `mergeable` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit's review of a3a5f91. 1. `cmd_merge` still declared `local report done …` after the gate variable was renamed, so `done` was declared-unused and `mergeable` leaked to global scope. Not exploitable (the `read` assigns it fresh every call) but wrong. 2. The test nitpick asked for explicit `done` assertions in four scenarios. Its literal form no longer applies — the rework made `done` an alias of `mergeable`, so asserting `done is True` on a MERGED or BLOCKED PR would now assert the opposite of the intent. Honoured the underlying point instead: those scenarios now pin `converged` alongside `mergeable`, so a regression that re-couples watch convergence to merge authorization fails. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- kit-manifest.json | 2 +- scripts/dev_session.sh | 2 +- scripts/tests/test_pr_watch.py | 19 ++++++++++++++++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index 53584e2..47d8b94 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -59,7 +59,7 @@ }, "scripts/dev_session.sh": { "role": "engine", - "sha256": "7e8d98feb8ac956a3dff7d7a5957afa493402fa2b5130b33236b1d457363f93b" + "sha256": "52d962af8088e70fd7b4682d67ec7d6d4354c70f3d8db49b724d4c42c04d98f6" }, "scripts/hooks/pre-push": { "role": "hook", diff --git a/scripts/dev_session.sh b/scripts/dev_session.sh index 3f17500..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" diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 9df27bb..f6fc2be 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -181,8 +181,13 @@ def test_unknown_or_non_open_pr_state_never_settles_done() -> None: _green_view(state="MERGED", mergeStateStatus="UNKNOWN"), [], set() ) + # 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["converged"] is True assert merged["mergeable"] is False assert "PR state is MERGED" in merged["merge_blockers"] @@ -226,6 +231,11 @@ def test_unstable_is_allowed_only_when_remaining_check_is_informational() -> Non review_receipt=receipt, ) + # 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["mergeable"] is False @@ -304,7 +314,9 @@ def test_review_receipt_must_match_current_head() -> None: # 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. - assert missing["converged"] is True + 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 @@ -559,6 +571,11 @@ def test_zero_check_pr_still_needs_current_head_review_evidence( ) assert without_receipt["checks"]["all_green"] is True + # 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" From 32f3e4f2b54c10d297cc54fa8ec667d14e5ed649 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 12:36:55 +0300 Subject: [PATCH 6/6] fix(pr-watch): bool-type the merge predicate; correct two docstrings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the fallback independent review pass (CodeRabbit's check reads "Review rate limited", and its only completed review predates the additive rework, so the configured fallback ran per the review doctrine). 1. `decide_mergeable` returned its last operand, so a truthy non-bool `review_evidence` would propagate into the report. `dev_session.sh merge` tests the JSON value with `is True` — such a value fails closed, but confusingly, and serializes as e.g. `"mergeable": 1`. Coerced to bool and pinned by test. 2. `decide_done`'s docstring claimed the compatibility guarantee for itself. The thing protecting an older `dev_session.sh` is the report's `done` KEY — that gate shells out to `--json` and never imports this module, and the function now has no in-engine caller. As written, someone could delete the key as a redundant alias, see the function's promise still standing, and conclude the protection survived. It would not. 3. `build_report`'s settling note still pointed at `decide_done`; settling is a convergence concern. Claude-Session: https://claude.ai/code/session_01V27wAgECMEApqAvmJMFyqm --- kit-manifest.json | 2 +- scripts/pr_watch.py | 31 ++++++++++++++++++------------- scripts/tests/test_pr_watch.py | 23 +++++++++++++++++++++++ 3 files changed, 42 insertions(+), 14 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index 47d8b94..41a24ec 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "8c890dbe330ba8d3103935cd91b3d8dc6e3eccd48588d341cd940a948686d580" + "sha256": "735428aa7a06141da7b3d385324a350e7a722fd2c2c47d0e5f97047fda41df37" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 3ecb1fb..037d245 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -643,8 +643,15 @@ def decide_mergeable( 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 converged and not merge_blockers and review_evidence + return bool(converged and not merge_blockers and review_evidence) def decide_done( @@ -659,17 +666,15 @@ def decide_done( ``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". It keeps that meaning exactly, and the report keeps - emitting a ``done`` key equal to ``mergeable``. - - **This is deliberate, and it is a safety property, not politeness.** Engine - upgrades are per-file (a sized-down adoption may install only some engines), - so a repo can end up running a new ``pr_watch.py`` against an older - ``dev_session.sh`` whose merge gate reads ``done``. Had ``done`` been - redefined to mean mere watch-convergence, that combination would have - authorized merges on PRs with **no review receipt at all** — a silent - fail-open on the merge gate. Making the schema change purely additive is what - removes that hazard. + 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. """ @@ -820,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``, diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index f6fc2be..511d069 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -110,6 +110,29 @@ def test_report_done_is_always_identical_to_mergeable() -> None: 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.