From e3806f43f57de4b5b83ceb377a1e6fe73460b798 Mon Sep 17 00:00:00 2001 From: EMRG Evolution Date: Mon, 14 Sep 2026 01:31:38 +0800 Subject: [PATCH] emrg: price the refresh a stale verdict asks for, and give each stale state its own remedy --- Agent.md | 2 +- scripts/check-merge-freshness.py | 175 ++++++++++++++++++- tests/test_check_merge_freshness.py | 250 ++++++++++++++++++++++++++++ 3 files changed, 423 insertions(+), 4 deletions(-) diff --git a/Agent.md b/Agent.md index 771d1407..1b83a994 100644 --- a/Agent.md +++ b/Agent.md @@ -124,7 +124,7 @@ GUI: `cd emrg/gui && npm test` (100: 44 daemon_client + 20 conn-manager + 7 inte Renderer: `cd emrg/gui/renderer && npm run typecheck && npm test` (514: 5 snapshot-store + 9 utils + 3 ErrorBoundary + 2 App smoke + 11 commands + 4 copywriting + 11 i18n + 13 markdown + 21 transcript + 11 TranscriptView + 15 history + 31 composer + 41 Composer + 6 LinkDialog + 16 sidebar + 17 Sidebar + 9 fileTree + 9 FileTree + 16 resultPanel + 8 ResultPanel + 27 workspaceView + 10 WorkspaceView + 10 dialog + 6 Dialog + 9 ConfirmDialog + 9 RenameDialog + 10 dialogLists + 3 HelpDialog + 9 MemoryDialog + 6 SkillsDialog + 8 openSession + 6 WelcomeDialog + 9 OpenSessionDialog + 7 NewSessionDialog + 7 rewind + 8 RewindDialog + 7 GithubDeviceDialog + 18 daemonBridge + 7 DaemonBridgeProvider + 30 Shell + 15 DialogHost + 20 SettingsPanel + 6 TaskFormDialog + 5 RantDialog + 4 vendorMarkdown) + `npm run build` → `renderer/dist/` CI: `uv run pytest` (ubuntu + **windows-2025 matrix** — Windows pytest 回归在 PR CI 即失败，v0.2.29 教训 #725) + GUI tests + **actionlint workflow lint** (`rhysd/actionlint@v1.7.12` gate, #444 — workflow 解析错误在 PR CI 即失败，如 `if:` secrets 上下文) Re-trigger: `scripts/re-trigger-ci.sh [branch]` (workflow_dispatch, #527 — 替代空 commit 重触发：Actions outage 会整段丢弃 push 事件，dispatch 走 API 路径不受影响) -Merge freshness: `uv run --no-sync python3 scripts/check-merge-freshness.py ...` — 合并前问一句「这条绿色 CI 说的还是**将要合并的那棵树**吗」。`pull_request` 事件下 GitHub 构建的是 `Merge into `（合到**分叉点**，不是当前 master）：分叉点就是 master 时两者同一棵树，master 一移动就不是了，而 master 移动**不触发** `synchronize`（只有 push 分支才触发），于是绿灯永远保持绿灯却已经过期。#1137 实测：`MERGEABLE/CLEAN` + 双 job 全绿，两侧计数行都写成同一个 1397 ⇒ git **无冲突**自动合并、保留 1397，而合并树实收 1401，两个守卫在 master 上才变红——**干净合并才是危险的那一种**（冲突时人被迫看一眼，反而安全）。判定不用时间戳比较（时钟/秒级竞态会骗人）而用**图结构**：master 的 tip 是否是 head 的祖先（`compare/master...` 的 `identical`/`ahead`），是则 merge base 就是 master 本身、判决可平移。**祖先性只是一半**：head 含 master 但**根本没有 CI 运行**（push 事件丢失 ⇒ `no checks reported`）同样不算新鲜，故第二个条件是「该 SHA 上存在一个**通过**的运行」。按 SHA 而非分支取运行（分支推两次会有两个运行）。exit 0 全部新鲜 / 1 至少一条过期 / 2 问不出来（坏 PR、gh 失败、状态不认识）——不认识的状态一律 fail-loud，绝不默认新鲜 +Merge freshness: `uv run --no-sync python3 scripts/check-merge-freshness.py ...` — 合并前问一句「这条绿色 CI 说的还是**将要合并的那棵树**吗」。`pull_request` 事件下 GitHub 构建的是 `Merge into `（合到**分叉点**，不是当前 master）：分叉点就是 master 时两者同一棵树，master 一移动就不是了，而 master 移动**不触发** `synchronize`（只有 push 分支才触发），于是绿灯永远保持绿灯却已经过期。#1137 实测：`MERGEABLE/CLEAN` + 双 job 全绿，两侧计数行都写成同一个 1397 ⇒ git **无冲突**自动合并、保留 1397，而合并树实收 1401，两个守卫在 master 上才变红——**干净合并才是危险的那一种**（冲突时人被迫看一眼，反而安全）。判定不用时间戳比较（时钟/秒级竞态会骗人）而用**图结构**：master 的 tip 是否是 head 的祖先（`compare/master...` 的 `identical`/`ahead`），是则 merge base 就是 master 本身、判决可平移。**祖先性只是一半**：head 含 master 但**根本没有 CI 运行**（push 事件丢失 ⇒ `no checks reported`）同样不算新鲜，故第二个条件是「该 SHA 上存在一个**通过**的运行」。按 SHA 而非分支取运行（分支推两次会有两个运行）。exit 0 全部新鲜 / 1 至少一条过期 / 2 问不出来（坏 PR、gh 失败、状态不认识）——不认识的状态一律 fail-loud，绝不默认新鲜。**药方按「过期方式」分别给出，并标出价码**（cyc20260914-010711 实测：队列 #1197 2 票 / #1198 1 票 / #1199 #1200 0 票、master `abe6f8b` 四条**全部 STALE**，而此前那句一律打印的「把 master 重新合进每条分支，再让 CI 跑」会让 #1197/#1198 的 3 张票作废、换来的却是它们本来就有的东西——票仍然关于 head，落地树本来就能量）：**ancestry 型过期**（绿灯但说的是旧 master）是**唯一**刷新能治的一种，也是唯一要付费的一种——刷新要移动 head 才能让 CI 的 merge base 跟上，而 `check-vote-count.py` 让 push 之前的票全部作废 ⇒ 票数从**拥有该规则的兄弟工具**读来（不清点第二遍），且**只在 ancestry 型上多花一次查询**（其余型别的药方与票数无关，多查一次只会改变一个药方已定的输出）：0 票 ⇒ 照旧建议刷新（没有可作废的，CI 判真合并树比本地测量更强）；≥1 票 ⇒ **点名票数**并改道**测落地树**（`git fetch origin master` 后 `check-merge-plan-suite.py `，head 不动故票数不变），读法用**普通评论**（`gh pr comment`，不是 review）贴出——普通评论不进票数，故证据与票数互不干扰；只有落地树不过才刷新（那时那些票本来也是关于一棵再也合不了的树）。票数**读不出来就写 unavailable，绝不报 0**（0 是唯一给「放心刷新」放行的值，凭空报 0 是唯一会花掉票的方向）。其余三种**不是刷新能治的**，各给药方而非推去 rebase：`no_run`（head 含 master 但该 SHA 上一个 `Test` 运行都没有，即 push 事件丢失 ⇒ `no checks reported`）⇒ **重新触发同一 head 的 CI**（`gh workflow run test.yml --ref ` / `scripts/re-trigger-ci.sh `，票保留；刷新也会出一次 run，但代价是全部票）；`running` ⇒ 等；`failing` ⇒ 修失败（刷新既花钱又不会让红变绿）。`--json` 另出 `stale_kind` 与 `valid_votes`，供脚本按状态选动作而不必解析散文 PR base check: `uv run --no-sync python3 scripts/check-pr-base.py [--repo owner/name] [PR...]` — 合并前查每张 open PR 的 **base 分支是否还能到达 master**。`gh pr merge` 合的是 PR 的 **base 分支**，而 stacked PR（base 是另一条 feature 分支）的 base 在本仓库被 squash 合并后**永远不是 master 的祖先**：合进去只会落到那条死分支上，master 一无所获，而 PR 状态显示 MERGED、CI 双绿、票数也照常累计——三个现有闸门全都放行。实测（cyc20260911-210746）：#1148 的 base 是 #1147 的分支，而 #1147 已 squash 合入 master，若照原样批准，这个「让 master 的分类器不再重复内容」的修复就会合进死分支，队列里留一张 MERGED 却什么都没改变的 PR。判据不是「base 是不是 master」（stacked PR 在父分支未合并时是正常且有用的工作流，正是 #1149 要服务的情形），而是**「合进这个 base 还能不能到 master」**：base 已在 master 上 ⇒ OK；base 是**某张 open PR 的 head 分支** ⇒ LIVE（父分支仍在飞行中，合进去会落到活着的父分支上、随父 PR 落地；不是死路，但也不是保证——父 PR 仍可能被**未合并关闭**，故不判失败、只提示优先 retarget 到 master）；base 既不在 master 上、也没有任何 open PR 以它为 head ⇒ DEAD（须先 retarget）。**LIVE 与 DEAD 无法由 compare status 区分**（cyc20260911-215935 实测修正：飞行中的父分支**必然**有 master 没有的 commit，status 就是 `ahead`；master 因任何无关原因前进后它又变成 `diverged`，与 squash 合并后的死分支**逐字节相同**——五个 open PR 分支当时全部是 `ahead`，只因 master 尚未前进）。可判定的输入是「有没有 open PR 的 head 等于这条分支」，而 `headRefName` 本就在抓取的字段里；旧版直接用 status 判 OK/DEAD，于是**每个飞行中的 stacked 父分支都被报成 DEAD**（5/5 真实父分支），而它自己文档里的「live stacked base 返回 OK」在旧谓词下**不可达**（能到 OK 的非 master base 只有「内容已在 master 上」的已落地分支，那不是 stacked 工作流）。LIVE/DEAD 两侧各有变异验证测试（删掉 LIVE 分支、或把 `open_heads` 挪到 `prs` 过滤之后，测试即变红——后者是过滤顺序陷阱：父 PR 通常不在被问的 PR 列表里）。status 读法用 compare API（`identical`/`behind` = 在 master 上，`ahead`/`diverged` = 不在），四个状态都有测试钉住（把判据放宽成「不是 ahead 就算 OK」会让 `diverged` 蒙混过关）。exit 0 无死路 / 1 有 PR 停在死路 / 2 问不出来（gh 失败、响应不可解析）——问不出来绝不报 OK。retarget 命令：`gh api -X PATCH repos///pulls/ -f base=master`（`gh pr edit --base` 会因 Projects-classic 弃用报错）。 Release bump: `python3 scripts/bump-version.py ` — 一次改齐 8 处版本声明（`emrg/__init__.py`、`pyproject.toml`、`emrg/gui/package.json`、`emrg/gui/package-lock.json` 根 + `packages[""]`、`uv.lock`、`packaging/{build-runtime,make-installer,make-run-installer}.sh`）；`--check` 只报告漂移（宿主侧自检，与 CI 的 test_version_sync 对称），`--dry-run` 预览不落盘。锚点缺失/数量不符即 fail-loud，绝不猜测；`uv.lock` 只改 `name = "emrg"` 那一行（v0.2.94 教训：直接 `uv run` 会把 lock 里所有 registry URL 重写成镜像，556 行环境噪声）。bump 后用 `uv run --no-sync pytest` 避免 uv 重生成 lock。**测的是「你站着的那个 checkout」**（与 doc/node count 两工具同一处修复：root 原先取自 `__file__` ⇒ 在 worktree 里跑主 checkout 的那份脚本会去**锁主树**——worktree 处于 9.9.9/7 处漂移时它报 `OK: all 8 version sources agree`，而 `bump` 会改写**另一个** checkout 的 8 处版本声明，含决定构建产物的那个）。现 root 从 cwd 解析，输出首行报 `tree: `。详见 Agent.md「Releasing」 Doc count sync: `uv run --no-sync python3 scripts/check-doc-count.py [--measure|--resolve-conflict]` — **「Python 测试数不写进任何 tracked 文件」的守卫**（宿主侧自检，与 CI 的 `tests/test_doc_counts.py::test_no_tracked_file_states_the_python_test_count` 同一份规则，两处读的是本工具里的同一个正则）。默认扫描全仓 tracked 文件（`tests/` 与 `scripts/` 除外——那两个树是规则与它的探针住的地方，必须能拼出「写死的样子」；该排除有实测背书：466 个 tracked 文件里这两处之外的命中只有下面那两条真实声明）并**指名文件+行号+写法**，exit 1。`--measure` 现场测量当前树收集的测试数（`pytest --collect-only`，缺 pytest/解析不出即 fail-loud，绝不报假数）。`--resolve-conflict` 专治计数行的合并冲突：**解法不是选边而是去掉那个数字**（两侧各自去掉计数后文本必须完全相同才动手；措辞也不同即拒绝——那是内容冲突，得人读）。**存在理由（2026-09-13 实测，宿主的决定）**：写死一行派生数字既是冲突磁铁又是静默错误源——14 张在飞 PR 里 11 张冲突，**11/11 冲突的都是这一行**（每张加测试的 PR 都得改同一个数字），而两个分支写**同一个值**时 git 干净合并、悄悄留下陈旧值（实测 #1179+#1180 都写 1601 而合并树实收 1603，守卫只在合并后的树上才红）。故数字不写、现场测量：`README.md`/`README.cn.md` 早就这么做了（改挂 Tests badge）。**测的是「你站着的那个 checkout」**（2026-09-11 实测的坑：解冲突要在 git worktree 里干活，而 root 原先取自 `__file__` ⇒ 跑主 checkout 的脚本、量的却是**主树**，worktree 自己的 Agent.md 写 1401 而工具报 `OK: ... documents 1420`——**读错了树还说一致**）。现在 root 从 cwd 解析（cwd 同时有 `Agent.md` 与 `scripts/` 才算 checkout），拿不准就退回脚本自身 root，并在输出首行**报出所测的树** diff --git a/scripts/check-merge-freshness.py b/scripts/check-merge-freshness.py index dc79d01f..25fd2f87 100644 --- a/scripts/check-merge-freshness.py +++ b/scripts/check-merge-freshness.py @@ -63,6 +63,36 @@ property that failed. A cleanly auto-merged line is the *dangerous* case - when the count line conflicts, a human is forced to look at it. +What a stale verdict costs, and which remedy is the cheap one +------------------------------------------------------------- +A stale verdict has two remedies and their prices are not interchangeable: + +* **Refresh the branch** - re-merge master into it (or rebase) so CI judges the + real merged tree. The head has to move for CI's merge base to move, so this is + the only way to a *pull_request* verdict about the current master. It also + **moves the head**, and `check-vote-count.py` voids every vote that predates a + head push: the refresh is paid for with the review the branch has accumulated. +* **Measure the landing tree** - build the tree this merge would produce against + current master and run the guards on it (`check-merge-plan-suite.py `). + The head does not move, so the count does not change. It is a local reading + rather than a CI verdict, and it is the tree the merge actually lands. + +Measured 2026-09-14 (`cyc20260914-010711`, master `abe6f8b`), the whole queue +stale: `#1197` 2 valid votes, `#1198` 1, `#1199` and `#1200` none. The blanket +advice this tool printed until then - "re-merge master into each stale branch" - +would have voided three cycles of review on `#1197`/`#1198` to buy nothing those +branches did not already have: the votes were still about the head, and the tree +the merge lands was measurable without moving it. + +So the remedy is now per PR and priced by that count. A stale verdict that is +*not* ancestry-shaped (no run for the head, a run still going, a run that +concluded non-success) does not have a refresh for a remedy at all, and is now +named with the remedy that does fit it instead of a rebase that would not answer +its question. The count is read from the sibling tool that owns it (one extra +`gh` query per ancestry-stale PR, and none otherwise); when it cannot be read the +line says so and prices the refresh pessimistically, rather than reporting `0`, +which is the direction that quietly spends votes. + Usage ----- uv run --no-sync python3 scripts/check-merge-freshness.py [ ...] @@ -83,13 +113,20 @@ from __future__ import annotations import argparse +import importlib.util import json import subprocess import sys from dataclasses import dataclass +from pathlib import Path REPO = "argszero/emrg" +# The gate this repo merges on, and therefore the number of votes a refresh puts +# at risk. Passed to the sibling counter explicitly rather than defaulted on both +# sides, so a change to the gate has one place to land. +_VOTES_NEEDED = 3 + # GitHub `compare` statuses, split by the one property that decides freshness: # is master's tip an ancestor of the head? # @@ -116,6 +153,15 @@ # here reads as "the verdict workflow did not run", not as "the branch has no CI". _VERDICT_WORKFLOW = "Test" +# The four ways a verdict can fail to be current. They were prose in `reason` +# before, which is enough to *report* the state and not enough to choose a +# remedy: the state decides which action the tool may recommend, and only one of +# the four is fixed by a refresh (and that one charges the whole vote count). +_KIND_ANCESTRY = "ancestry" # the #1137 case: green, but about an older master +_KIND_NO_RUN = "no_run" # master is an ancestor but nothing ever judged the head +_KIND_RUNNING = "running" # a run exists and has not concluded +_KIND_FAILING = "failing" # a run concluded non-success + def _gh_json(args: list[str]) -> object: """Run `gh` and parse JSON, failing loud rather than guessing. @@ -153,6 +199,8 @@ class Verdict: run_conclusion: str | None stale: bool reason: str + # Which of the four ways (one of the `_KIND_*` names); "" when fresh. + stale_kind: str = "" def _latest_run_for_head(head: str) -> dict | None: @@ -246,6 +294,7 @@ def check_pr(number: int) -> Verdict: return Verdict( **common, stale=True, + stale_kind=_KIND_ANCESTRY, reason=( f"head does not contain master (status={status}, behind_by={behind_by}) " f"- CI's merge base was {merge_base[:8]}, so the verdict is about a tree " @@ -259,6 +308,7 @@ def check_pr(number: int) -> Verdict: return Verdict( **common, stale=True, + stale_kind=_KIND_NO_RUN, reason=( f"master is an ancestor (status={status}) but there is NO {_VERDICT_WORKFLOW} " f"run for head {head_sha[:8]} - an unjudged head, which `gh pr checks` reports " @@ -269,12 +319,14 @@ def check_pr(number: int) -> Verdict: return Verdict( **common, stale=True, + stale_kind=_KIND_RUNNING, reason=f"CI is still {conclusion or 'pending'} on head {head_sha[:8]} - no verdict yet", ) if conclusion != "success": return Verdict( **common, stale=True, + stale_kind=_KIND_FAILING, reason=( f"CI concluded {conclusion!r} on head {head_sha[:8]} - a failing verdict, " "not a stale one; re-running will not make it fresh" @@ -290,6 +342,112 @@ def check_pr(number: int) -> Verdict: ) +# ── the price of a refresh, read from the tool that owns the count ─────────── + +_SIBLING = Path(__file__).resolve().parent / "check-vote-count.py" +_sibling: object | None = None + + +def votes_counter(): + """The sibling module that owns "is this vote still about this head?". + + Loaded from its file rather than imported by name: the scripts in this + directory are not importable modules (hyphenated names, no package), and this + is the same loader `check-merge-pairs.py` uses for its sibling and both test + modules use for theirs. Imported rather than reimplemented because a second + reading of the vote rule is a second answer to "how many votes does this PR + have", and one number now decides the remedy. + """ + global _sibling + if _sibling is None: + spec = importlib.util.spec_from_file_location("check_vote_count", _SIBLING) + if spec is None or spec.loader is None: # pragma: no cover - the file is in this repo + raise RuntimeError(f"could not load {_SIBLING}") + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + _sibling = module + return _sibling + + +def _valid_votes(pr: int) -> tuple[int | None, str]: + """`(valid votes, "")` for this PR, or `(None, why it could not be read)`. + + Advisory, so it degrades instead of failing the run: the freshness verdict + above is answerable without it, and exiting 2 because a *price* could not be + read would trade a real answer for a missing one. It never degrades to `0` + though - zero is the line that says "refresh freely", so reporting it without + having read it would be the direction that spends votes. + """ + try: + verdict = votes_counter().check_pr(pr, _VOTES_NEEDED) + return int(verdict.valid_count), "" + except Exception as exc: # advisory by construction - see the docstring above + return None, f"{type(exc).__name__}: {exc}".replace("\n", " ")[:200] + + +def _remedy(pr: int, kind: str, valid_votes: int | None, unread: str) -> str: + """One line: what to do about this verdict, and what it charges. + + The price is attached only where it is actually paid - an ancestry-stale + verdict is the one a refresh cures. The other three kinds get the action that + fits them, so the output cannot be read as "rebase and move on" for a head + whose CI run is merely missing (a refresh is a remedy there too, but the + expensive one: re-triggering fires a run on the same head and keeps the + votes). + """ + if kind == _KIND_ANCESTRY: + if valid_votes is None: + return ( + f"#{pr}: vote count unavailable ({unread or 'not read'}) - read it before " + f"refreshing (`scripts/check-vote-count.py {pr}`): a refresh moves the head " + "and voids every vote the branch has" + ) + if valid_votes == 0: + return ( + f"#{pr}: 0 valid votes - nothing to void. Re-merge master into the branch " + "(or rebase it) and let CI judge the real merged tree" + ) + return ( + f"#{pr}: {valid_votes} valid vote(s) at risk - a refresh moves the head, and the " + f"vote counter voids all {valid_votes}. Measure the tree this merge would land " + "instead (`git fetch origin master`, then `scripts/check-merge-plan-suite.py " + f"{pr}`) and post that reading as a plain comment (`gh pr comment`, not a " + "review): the head does not move, so the count does not change. Refresh only if " + "that tree fails - those votes were about a tree that can no longer be merged" + ) + if kind == _KIND_NO_RUN: + return ( + f"#{pr}: no run for this head - re-trigger CI on the same head (`gh workflow run " + "test.yml --ref `, or `scripts/re-trigger-ci.sh `), which keeps " + "the votes. A refresh would fire a run too, and cost every vote the branch has" + ) + if kind == _KIND_RUNNING: + return ( + f"#{pr}: wait for the run - neither a refresh nor a re-trigger answers a run " + "that has not concluded" + ) + return ( + f"#{pr}: fix the failure - a refresh costs every vote the branch has, and does not " + "make a failing run pass" + ) + + +def _prices(verdicts: list[Verdict]) -> dict[int, tuple[int | None, str]]: + """The vote count for each verdict whose remedy depends on it. + + Only the ancestry-stale kind: the other kinds are told an action that does not + depend on the count, so reading it for them would be a `gh` query with no + consumer - and, worse, a query that can only change what the tool prints for a + verdict whose remedy is already decided. + """ + return { + v.pr: _valid_votes(v.pr) + for v in verdicts + if v.stale and v.stale_kind == _KIND_ANCESTRY + } + + def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser( prog="check-merge-freshness.py", @@ -305,6 +463,10 @@ def main(argv: list[str] | None = None) -> int: print(f"error: {exc}", file=sys.stderr) return 2 + # After the verdicts, and outside their error path: a failure to read a price + # is not a failure to answer the question above (see `_valid_votes`). + prices = _prices(verdicts) + if args.json: print( json.dumps( @@ -317,6 +479,8 @@ def main(argv: list[str] | None = None) -> int: "behind_by": v.behind_by, "stale": v.stale, "reason": v.reason, + "stale_kind": v.stale_kind, + "valid_votes": prices[v.pr][0] if v.pr in prices else None, } for v in verdicts ], @@ -330,11 +494,16 @@ def main(argv: list[str] | None = None) -> int: if any(v.stale for v in verdicts): print( - "\nRe-merge master into each stale branch, then let CI run. On `pull_request` " - "GitHub builds Merge into ; merging current master in is what " - "moves the merge base to master, so CI then judges the real merged tree.", + "\nA stale verdict has no free remedy: refreshing a branch (re-merge master " + "in, or rebase) moves its head, and `check-vote-count.py` voids every vote " + "predating a head push. Per stale PR:", file=sys.stderr, ) + for v in verdicts: + if not v.stale: + continue + count, unread = prices.get(v.pr, (None, "")) + print(" " + _remedy(v.pr, v.stale_kind, count, unread), file=sys.stderr) return 1 return 0 diff --git a/tests/test_check_merge_freshness.py b/tests/test_check_merge_freshness.py index 099f2132..0e2d6b77 100644 --- a/tests/test_check_merge_freshness.py +++ b/tests/test_check_merge_freshness.py @@ -29,6 +29,26 @@ Nothing here touches the network: `_gh_json` is replaced, and the replacement is asserted to receive the arguments the real helper would (so a test cannot pass by never calling it). + +Per-state remedies (cycle `cyc20260914-010711`) +---------------------------------------------- +The remedy this tool prints used to be one sentence for every stale verdict: +"re-merge master into each stale branch, then let CI run". Measured on the live +queue that day (`#1197` 2 valid votes, `#1198` 1, `#1199`/`#1200` none, master +`abe6f8b`) the sentence was a way to *lose* work: a refresh moves the head, and +`check-vote-count.py` voids every vote predating a head push, so following it +would have paid three cycles of review for a freshness the landing tree can be +measured for free. Both directions are pinned here, because a remedy that is too +cautious is as wrong as one that is too eager: + +* **with votes** - named, priced, and pointed at the landing-tree measurement; +* **without votes** - the refresh is the cheap way to a current verdict and is + recommended unchanged; +* **unreadable count** - said so, never rendered as `0`, which is the line that + means "refresh freely"; +* **not ancestry-shaped** (no run / still running / failing) - the remedy that + fits that state, since a rebase does not answer any of them, and no vote query + is spent on a state whose remedy does not depend on the count. """ from __future__ import annotations @@ -116,6 +136,38 @@ def _install(mod, monkeypatch, fake: FakeGh) -> None: monkeypatch.setattr(mod, "_gh_json", fake) +@pytest.fixture(autouse=True) +def _no_vote_network(mod, monkeypatch): + """No test reaches GitHub through the vote sibling either. + + The price of a refresh is read from `check-vote-count.py`, whose entry points + run real `gh` commands. A test that forgot to fix the count would then not + only make a network call but *pass* with whatever the live queue happened to + be that minute. Both of the sibling's gh entry points are poisoned here, so an + unpatched read degrades to "unavailable" (which `_valid_votes` is built to + survive) instead of quietly answering with real data. + """ + module = mod.votes_counter() + + def boom(*args, **kwargs): + raise AssertionError("the sibling vote counter must not be reached in tests") + + monkeypatch.setattr(module, "_gh_json", boom) + monkeypatch.setattr(module, "_gh_json_paginated", boom) + + +def _votes(mod, monkeypatch, count: int | None, note: str = "") -> list[int]: + """Fix the count seam `main` uses, and record which PRs it was asked about.""" + asked: list[int] = [] + + def fake(pr: int) -> tuple[int | None, str]: + asked.append(pr) + return count, note + + monkeypatch.setattr(mod, "_valid_votes", fake) + return asked + + def _run(mod, monkeypatch, fake: FakeGh, argv: list[str] | None = None) -> int: _install(mod, monkeypatch, fake) return mod.main(argv if argv is not None else ["1"]) @@ -235,6 +287,204 @@ def test_the_newest_run_for_the_head_wins(mod, monkeypatch, capsys): assert "FRESH" in capsys.readouterr().out +# --- the remedy is priced by the state, not printed blanket ---------------- +# +# Every test here asserts on the prose the tool adds *after* the verdicts, which +# is where the action lives: the verdict says what is wrong, the remedy says what +# it costs, and the two were previously disconnected (a stale-with-votes PR was +# told to do the one thing that voids its votes). + + +def test_a_stale_branch_with_votes_is_told_what_a_refresh_would_cost(mod, monkeypatch, capsys): + """The measured queue: stale *and* carrying review. + + #1197 sat at 2/3 and #1198 at 1/3 when the whole queue went stale. The advice + they got - "re-merge master into each stale branch" - would have returned them + to 0/3 to make CI's verdict current, which is not a trade the tool has any + business recommending silently. + """ + fake = FakeGh(_view(), _compare("diverged", 2, 1, base="cb651a4"), [_run_()]) + asked = _votes(mod, monkeypatch, 2) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert asked == [1], "the price is read for the stale PR" + assert "2 valid vote(s) at risk" in err + assert "voids all 2" in err + assert "check-merge-plan-suite.py 1" in err + # A plain comment is named explicitly: it is the vehicle that carries the + # landing-tree reading without moving the head's vote count. + assert "gh pr comment" in err + assert "Re-merge master into each stale branch" not in err + + +def test_a_stale_branch_with_no_votes_is_told_the_refresh_is_free(mod, monkeypatch, capsys): + """The other direction: with nothing to void, the refresh IS the remedy. + + #1199/#1200 were stale at 0/3. Telling them to measure a landing tree by hand + instead would be the cautious-but-wrong output - CI on the real merged tree is + strictly better evidence, and it costs nothing here. + """ + fake = FakeGh(_view(), _compare("diverged", 2, 1, base="cb651a4"), [_run_()]) + _votes(mod, monkeypatch, 0) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert "0 valid votes - nothing to void" in err + assert "Re-merge master into the branch" in err + assert "at risk" not in err + + +def test_an_unreadable_vote_count_is_said_so_and_never_read_as_zero(mod, monkeypatch, capsys): + """`0` is an answer, so a failed read must not produce it. + + Zero is the only count that licenses the destructive action, which makes it + the one value this line must never invent. The failure cause is carried into + the message so a reader can tell a gh outage from a permissions problem. + """ + fake = FakeGh(_view(), _compare("diverged", 2, 1, base="cb651a4"), [_run_()]) + _votes(mod, monkeypatch, None, "RuntimeError: gh failed (rc=1)") + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert "vote count unavailable (RuntimeError: gh failed (rc=1))" in err + assert "0 valid votes" not in err + assert "Re-merge master into the branch" not in err + + +def test_a_missing_run_is_told_to_re_trigger_rather_than_refresh(mod, monkeypatch, capsys): + """An unjudged head is a real state, and a rebase is the expensive way out. + + `gh pr checks` reports "no checks reported" after a dropped push event. A + refresh fixes that too - but by moving the head, so the cheaper remedy that + answers the same question on the same head is named instead. It is also the + arm that pins the query discipline: a state whose remedy does not depend on + the count must not spend a gh query asking for it. + """ + fake = FakeGh(_view(), _compare("ahead", 1, 0), []) + asked = _votes(mod, monkeypatch, 7) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert asked == [], "no count is read for a state whose remedy ignores it" + assert "re-trigger CI on the same head" in err + assert "keeps the votes" in err + assert "Re-merge master into the branch" not in err + + +def test_a_run_still_going_is_told_to_wait(mod, monkeypatch, capsys): + fake = FakeGh(_view(), _compare("ahead", 1, 0), [_run_(conclusion="pending")]) + asked = _votes(mod, monkeypatch, 3) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert asked == [] + assert "wait for the run" in err + + +def test_a_failing_run_is_told_to_fix_the_failure_not_to_refresh(mod, monkeypatch, capsys): + """A red run is not an expired one, so no refresh-shaped advice may appear. + + The verdict reason already says this; the remedy has to agree with it, or the + two halves of one output recommend opposite actions. + """ + fake = FakeGh(_view(), _compare("ahead", 1, 0), [_run_(conclusion="failure")]) + asked = _votes(mod, monkeypatch, 3) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1 + assert asked == [] + assert "fix the failure" in err + assert "does not make a failing run pass" in err + + +def test_json_carries_the_kind_and_the_count(mod, monkeypatch, capsys): + """A caller scripting the queue needs the remedy's inputs, not just the prose.""" + fake = FakeGh(_view(), _compare("diverged", 2, 1, base="cb651a4"), [_run_()]) + _votes(mod, monkeypatch, 3) + rc = _run(mod, monkeypatch, fake, ["1", "--json"]) + payload = json.loads(capsys.readouterr().out) + assert rc == 1 + assert payload[0]["stale_kind"] == "ancestry" + assert payload[0]["valid_votes"] == 3 + + +def test_a_fresh_verdict_carries_neither_a_kind_nor_a_count(mod, monkeypatch, capsys): + """Fresh is not a fifth kind, and it must not pay for a remedy query: a fresh + verdict has no remedy to price, and its query count staying at three is what + keeps the healthy path exactly as cheap as it was before this existed.""" + fake = FakeGh(_view(), _compare("ahead", 4, 0), [_run_()]) + asked = _votes(mod, monkeypatch, 9) + rc = _run(mod, monkeypatch, fake, ["1", "--json"]) + payload = json.loads(capsys.readouterr().out) + assert rc == 0 + assert payload[0]["stale_kind"] == "" + assert payload[0]["valid_votes"] is None + assert asked == [] + assert len(fake.calls) == 3 + + +def test_the_count_comes_from_the_sibling_that_owns_it(mod, monkeypatch): + """The delegation, tested where it actually happens. + + A local re-reading of the vote rule would be a second answer to "how many + votes does this PR have", and the count is now load-bearing for a destructive + recommendation. `needed` is asserted too: the gate is three, and a sibling + called with a made-up threshold would answer the wrong question. + """ + seen: list[tuple[int, int]] = [] + + class _Verdict: + valid_count = 2 + + def fake_check_pr(pr: int, needed: int): + seen.append((pr, needed)) + return _Verdict() + + monkeypatch.setattr(mod.votes_counter(), "check_pr", fake_check_pr) + assert mod._valid_votes(41) == (2, "") + assert seen == [(41, 3)] + + +def test_a_broken_count_read_degrades_to_unavailable(mod, monkeypatch): + """The seam's own failure path: the freshness answer survives a missing price.""" + def boom(pr: int, needed: int): + raise RuntimeError("gh failed (rc=1): gh api repos/...") + + monkeypatch.setattr(mod.votes_counter(), "check_pr", boom) + count, note = mod._valid_votes(41) + assert count is None + assert "gh failed" in note + + +def test_a_broken_count_read_reaches_main_as_unavailable_not_zero(mod, monkeypatch, capsys): + """The same property one layer up, with the *real* reader in place. + + `test_an_unreadable_vote_count_is_said_so_and_never_read_as_zero` fixes the + count seam, so it cannot see what the real reader returns when the sibling + breaks - and that return value is exactly where a helpful-looking `0` would + be written. Here the real `_valid_votes` runs against a broken sibling, and + the assertions are on what the user is told. + """ + fake = FakeGh(_view(), _compare("diverged", 2, 1, base="cb651a4"), [_run_()]) + + def boom(pr: int, needed: int): + raise RuntimeError("gh failed (rc=1): gh api repos/argszero/emrg/commits/...") + + monkeypatch.setattr(mod.votes_counter(), "check_pr", boom) + rc = _run(mod, monkeypatch, fake) + err = capsys.readouterr().err + assert rc == 1, "an unreadable price must not cost the answer to the real question" + assert "vote count unavailable (RuntimeError: gh failed (rc=1)" in err + assert "0 valid votes" not in err + assert "Re-merge master into the branch" not in err + + +def test_the_sibling_is_loaded_once(mod, monkeypatch): + """Cached, not reloaded per PR: this runs once per stale PR in a queue.""" + assert mod.votes_counter() is mod.votes_counter() + + # --- fail loud, never guess ------------------------------------------------