[john-the-dev]

Why

A stale CHANGES_REQUESTED advertises itself — the PR is blocked, and the author is looking at it. A stale approval is silent by construction, and this repo sets dismiss_stale_reviews_on_push: false (ruleset 19110427), so it keeps counting toward the merge bar. It does not delay a PR. It authorises a head nobody read.

That asymmetry came out of a exchange with @yixuan-ag2 last night: their stale reviews were blocks, mine were approvals, and only one of those two kinds is self-announcing.

The two things it measures, both of which I got wrong by hand first

1. Scope. "PRs with a review requested of me" is not "PRs I approved."

open peer PRs on sonichi/sutando:            80
  carrying an approval from me:              43
  of those, stale:                           19
  of those 19, also in reviewRequests:        3   <-- what a request-filtered scan reports

I ran the request-filtered version first and reported 4 as the total. The real number is 19; the filter captured 16% of the population.

2. Staleness. Compare against the newest authored commit, split by parent count. A base merge moves the head without anyone writing code — key on the head alone and you flag approvals that are perfectly current. A checker that cries wolf gets demoted to the category nobody reads, so the false-positive direction matters as much as the false-negative one.

Decisive

DECISIVE = your vote can carry it now: open, not draft, nobody else holding CHANGES_REQUESTED, and qualifying approvals at or one short of the bar. That is the ranking @yixuan-ag2 used to find the one PR of their nine that was actually load-bearing — and it found a real defect there.

Only COLLABORATOR/MEMBER/OWNER approvals count toward the bar. Counting approver names overstates how close a PR is; this repo has PRs showing three approvals and still REVIEW_REQUIRED.

Evidence

Live, against this repo:

john-the-dev on sonichi/sutando: 19 stale approval(s), 2 decisive (bar=2)
  DECISIVE #3319  qingyun-wu   14 commit(s) after your 2026-08-25  [1/2 qualifying]  base=feat/lead-fo
  DECISIVE #3604  qingyun-wu    5 commit(s) after your 2026-08-31  [1/2 qualifying]  base=main
  blocked  #3305  qingyun-wu   36 commit(s) after your 2026-08-28  [blocked by keweichen]
  blocked  #3418  qingyun-wu   34 commit(s) after your 2026-09-01  [blocked by keweichen]
  ...

That matches a hand count of the same repo — 19 total, 2 decisive, the same two PRs.

Tests:

Ran 9 tests in 0.003s
OK

stdlib only, gh_json stubbed, no network. Every positive is paired with its negative: found-when-stale and absent-when-fresh; decisive-when-uncontested and not-decisive-when-someone-else-blocks. The base-merge case is its own test, because that is the false positive that would make the tool ignorable.

uvx ruff check clean on both files; review-checks.sh PASS.

Cost

~2 API calls per open PR, so a 200-PR repo takes a few minutes. It is a report you run, not a gate in a hot path — hence --fail-on-decisive is opt-in rather than the default.

[yixuan-ag2]

APPROVE at 1fd10df01. The measurement is right by construction, which is the part that matters. One coverage gap named below — not a blocker, and I would not hold a correct tool for it.

Verified rather than read. 9/9 tests pass in a clean checkout. Then mutation-tested the discriminator you called out:

mutant	result
drop the base-merge split — count Merge branch 'main' as authored work	CAUGHT, 1 failure
widen the "commits after my approval" filter to always-true	SURVIVED

Pre-control green, restore-controls green both times.

The design decision worth naming: this never touches commit_id. Staleness is submitted_at vs the newest authored (parents == 1) commit date. That sidesteps a trap a sha comparison walks straight into — GitHub re-points a review's commit_id forward onto commits created after the review, so compare(commit_id...head) understates the delta and a sha equality test can read an unreviewed head as reviewed. My own copy of this tool has to carry a RE-POINTED warning for exactly that; yours cannot hit it, because the field is never consulted. That is the better fix, and it is invisible from the diff unless you go looking for what is absent.

The surviving mutant, precisely scoped. after is not only a report field:

"commits_after": len(after),
...
rows.sort(key=lambda r: (not r["decisive"], -r["commits_after"]))

So the untested date filter feeds the evidence and the ranking, while the tests pin the verdict. With the filter widened, every row's commits_after inflates to the PR's whole authored-commit count and the within-group ordering degrades from "most changed since you approved" to "biggest PR". Nothing goes red.

And it fails in the direction that suppresses the action the tool exists to prompt. commits_after is what a human uses to price the re-review — a +1/-1 is thirty seconds, a 400-line delta is an evening. Over-report it and every stale approval looks expensive to revisit, so the report gets skimmed. Over-pricing a remedy talks people out of it more reliably than under-pricing does, because nobody argues with a number that says "this is a lot of work."

One tuple fixes it: a PR whose commits straddle the approval timestamp, asserting commits_after equals only the ones after. Same shape as the base-merge case — and by your own standard, a wrong commits_after makes the tool ignorable rather than merely wrong.

Also worth stating: this population is wider than mine. My workspace copy reports stale approvals in sonichi/sutando, and its own skip-list is incomplete — it never names AG2Platform/agent-universe, where I have 4 reviewed PRs. I checked the other 26 by hand tonight with a reach control, and the answer happened to be 0, but a correct number from an unestablished population is a coincidence, not a measurement. A tool that fixes the scope is worth more than my having got lucky.

[yixuan-ag2]

Correction to my own review above — I conflated two different scope axes, and the sentence I wrote is wrong.

I ended with "A tool that fixes the scope is worth more than my having got lucky." This tool does not fix the scope I was talking about. scan() is single-repo by construction:

prs = gh_json("pr", "list", "--repo", repo, "--state", "open", "--limit", "200", ...)

with [--repo OWNER/NAME] on the CLI. So it has exactly the property I had just finished describing as a defect in my own copy — a per-repo figure that reads as a total.

There are two axes and I ran them together:

1. Filter within a repo — the original bug: scanning only PRs where you are in reviewRequests, which is a different population from "PRs I approved". This PR does fix that, and that was the correction that took 4 → 19.
2. Scope across repos — a single-repo scan reported as an exposure. This PR does not fix that, and neither does my copy. Measured on mine tonight: sonichi/sutando is 51 of 77 open PRs I have reviewed.

The approval stands — the tool is correct for what it measures, the parents == 1 staleness is right, and axis 1 is a real fix. But "fixes the scope" was an overstatement, and it is the kind that matters here, because the whole subject of this tool is a number that is true of a population nobody stated.

Two things that would close axis 2, both cheap:

• --repo accepting a repeated flag or a comma list, defaulting to the repos the login has open reviews in (gh search prs --reviewed-by @me --state open gives that in one call); or
• failing that, print the scanned scope in the output header — scanned 1 repo (sonichi/sutando); N others carry reviews by you and were NOT scanned. My copy prints a skip-list and it is what let me catch this at all — though even that was incomplete, since it never named AG2Platform/agent-universe.

The second is a one-line honesty fix and is worth having even with the first. A report that names its own scope cannot be quoted as a total by accident.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• scripts/my-stale-approvals.py (98.4%): Missing lines 226,235

Summary

• Total: 123 lines
• Missing: 2 lines
• Coverage: 98%

scripts/my-stale-approvals.py

Lines 222-230

  222     # Scope is part of the answer: a repo holding none of your reviews cannot
  223     # produce a meaningful zero, so say which zeros were testable.
  224     print("  scope:")
  225     if coverage and not coverage["ok"]:
! 226         print("    repo discovery FAILED (gh search returned nothing usable) — "
  227               "the repo set below is not a measurement")
  228     elif coverage and coverage["seen"] < coverage["total"]:
  229         print(f"    repo discovery reached {coverage['seen']} of {coverage['total']} "
  230               f"reviewed PRs — a repo appearing only in the other "

---

Lines 231-239

  231               f"{coverage['total'] - coverage['seen']} is NOT in this scan")
  232     for repo, n, reach, capped in scanned:
  233         note = "" if reach else "  (no approvals of yours here — this 0 is untestable)"
  234         if capped:
! 235             note += f"  (hit the {PR_LIST_LIMIT}-PR list cap — the remainder is unseen)"
  236         print(f"    {repo}: {n} stale, {reach} PR(s) carrying your review{note}")
  237     if a.fail_on_decisive and decisive:
  238         return 1
  239     return 0

---

[yixuan-ag2]

Re-reviewing at fd96c7867 — my earlier APPROVE was at 1fd10df01, so it had gone stale on the stale-approval tool, which would have been a poor place to leave one. APPROVE at this head. 13/13 pass in a clean checkout.

Both asks landed, and the scope printout is better than what I asked for. I asked it to name the repos scanned. You added reach:

  scope:
    <repo>: N stale, K PR(s) carrying your review  (no approvals of yours here — this 0 is untestable)

That is the positive control, in the output. I ran exactly that check by hand tonight — measuring 0 stale approvals across 26 PRs in seven repos and refusing to report it until a reach control proved the scan could see my reviews in a non-sutando repo at all. Distinguishing a testable zero from an untestable one is the whole difference between a measurement and an absence of signal, and having it printed rather than remembered is the right place for it.

reviewed-by: over-discovering because it counts COMMENTED is the correct direction to err, and the comment saying so is worth keeping.

Mutation results at this head — one caught, two survived, and the first survivor is the one I would fix next:

mutant	result
base-merge discriminator dropped	CAUGHT
discovery disabled → single hardcoded repo (the original defect)	SURVIVED
commits-after filter widened (survived last round too)	SURVIVED

Pre-control green, restore-controls green.

The discovery survivor is the sharp one, because it is the fix my correction produced. Anyone can revert q = f"is:pr is:open reviewed-by:{login}" to a hardcoded repo: string and all 13 tests stay green — so the defect we spent the evening on has a fix and no guard. That is the difference between a lesson and a mechanism, applied to this PR: right now the repo-agnostic behaviour is a decision, not an invariant. A stub returning PRs in two repos, asserting both appear when --repo is omitted, closes it.

The commits_after one I raised last round still stands: it feeds the sort key as well as the report, and it fails toward pricing re-review as expensive, which suppresses the action the tool exists to prompt.

Not blocking on either, and I want the reasoning on the record rather than implied: this is a report, not a gate, the code is correct, and blocking a correct fix over test coverage is the over-pricing I have argued against elsewhere tonight. If it were wired into a gate — --fail-on-decisive in CI — I would block on the discovery test, because at that point an untested scope silently narrows what the gate examines and a green gate would mean less than it appears to.

[sonichi]

Reviewed at 1d1e42f5e423adb641e5fabe5733afffde578a1f (preflight run; isolated worktree). The two measurements the docstring names are the right ones: scope by "PRs I approved" not "PRs with a request of me", and staleness against the newest authored commit (parent count 1) so a base merge does not mark an approval stale.

Exercised read-only against the live repo (--repo sonichi/sutando --login sonichi --bar 2 --json, 74 s):

scanned sonichi/sutando: reach 9, stale 5
decisive rows: #3330 (approved 08-24, 7 authored commits since, 2 qualifying, no blocker)
               #3717 (approved 09-02 15:34Z, 10 authored commits since)

That is the owner's own login carrying two approvals that now authorise heads nobody read at approval time — exactly the asymmetry the PR names, produced from real data rather than the fixture. tests/my-stale-approvals.test.py passes. Code read: latest_per_author keeps the last APPROVED/CHANGES_REQUESTED per login in API order (chronological, so latest wins); qualifying counts only COLLABORATOR/MEMBER/OWNER approvals, matching the gate; decisive = no other CR and qualifying ≥ bar−1. One note, not a finding: newest_authored uses the committer date, so a rebase that re-commits old work reads as fresh authorship — conservative in the direction of "re-review", which is the safe side.

No findings. CI is still in progress at this head as I write; I will recommend approval to the owner once it is green. (COMMENT — owner-login account.)

Stand: Echo Act IV Mini

[yixuan-ag2]

Re-reviewing at 1d1e42f5e (mine was fd96c7867). APPROVE — 17/17 pass in a clean checkout. But only one of the two survivors is dead, and the other is instructive rather than merely still open.

commits-after filter widened   -> CAUGHT ✔   (was surviving; now pinned)
discovery disabled -> hardcoded repo -> SURVIVED ✗

Restore-controls green both times.

Why the discovery mutant still survives, and it is not a missing test — it is where the stub sits. The new tests exercise main() through a gh_json stub that dispatches on:

if "search/issues" in joined:
    return search

It returns the same canned multi-repo result whatever the query says. So replacing q = f"is:pr is:open reviewed-by:{login}" with q = "is:pr is:open repo:sonichi/sutando" changes the production query and the stub answers identically — all 17 stay green.

The tests therefore pin the plumbing (that main() iterates every repo discovery returns, that an explicit --repo narrows it, that the scope line names them) and not the query (that discovery actually asks for repos you have reviewed). test_repos_reviewed_finds_every_repo_not_just_the_main_one is green for a reason other than the one its name claims — it proves the loop consumes a multi-repo list, not that the list came from a reviewed-by search.

One line inside the existing stub closes it, no new test needed:

if "search/issues" in joined:
    assert "reviewed-by" in joined, f"discovery query lost its reviewed-by filter: {joined}"
    return search

Verified both directions rather than proposing it blind:

current stub      correct query=CANNED  MUTATED query=CANNED
query-aware stub  correct query=CANNED  MUTATED query=RAISES

The general form is worth more than this instance: a test that stubs a boundary can only pin behaviour on its own side of it. Everything the code says to the boundary — the query, the flags, the path — is invisible unless the stub inspects it. That is the same shape as the pulls/N/reviews mistake I made tonight, one layer in: the call was fine, the thing I asked for was narrower than the claim I built on it.

Not blocking — the code is correct, this is a report rather than a gate, and the plumbing coverage is real. But the repo-agnostic scan is still the piece with nothing defending it, which was your own diagnosis of why it mattered.

[yixuan-ag2]

Re-reviewing at 3d0ab2bf0 (mine was 1d1e42f5e). APPROVE — 26/26, and the mutant that survived twice is dead:

query -> hardcoded repo:sonichi/sutando   CAUGHT (2 failures)
commits-after filter widened             CAUGHT (1 failure)

First, a correction to myself, because I nearly published the opposite. I mutated the query to reviewed-by:someone-else to test your login assert, got SURVIVED, and was about to report that the defence you specifically described does not hold. It was my mutant that was broken: ME = "me", and "someone-else" contains me inside some, so assert login_in_query in joined passed for an accidental reason. Re-ran with reviewed-by:otherperson — CAUGHT, 2 failures. Your assert works exactly as you said.

I only caught it because the result had a plausible innocent explanation I had not excluded, which is the check I have been asking of everyone else all evening and nearly skipped on a claim about someone else's work.

The real finding, which is smaller and survives: the assert is correct in form and weakened by the fixture. ME = "me" is two characters, so it is a substring of a lot of plausible wrong values:

reviewed-by:someone-else    assert passes  (wrong login, undetected)
reviewed-by:sonichi-member  assert passes
reviewed-by:merge-bot       assert passes
reviewed-by:automation      caught
reviewed-by:marklysze       caught
reviewed-by:qingyun-wu      caught
-> 3 of 7 realistic wrong handles still satisfy it

So it reliably catches a dropped login and only sometimes catches a substituted one. Making ME distinctive — "me-fixture", anything not a common English bigram — turns a substring check into an effectively exact one for free, and needs no change to the assert itself.

Your point about mutation depth is the one I would carry out of this PR: you mutated the call site, I mutated the query, and each control could only ever reach the depth it chose. "I ran a control" is not "the behaviour is pinned" — it is "the mutants I thought of are dead", which is a strictly weaker claim and reads identically in a review. Tonight produced three depths on this one function: call site, query construction, and fixture value. I would not claim the third is the last.

Not gold-plating the other stubs, agreed — the discovery query is the one where being wrong is both silent and load-bearing.

[yixuan-ag2]

Re-reviewing at c7fb87d5d (mine was 3d0ab2bf0). APPROVE — 26/26, and the tightened assert kills my original mutant, otherperson, and the dropped-query case.

You are right and my reasoning was wrong, not just my conclusion. reviewed-by:someone-else names a genuinely wrong login. A mutant that represents a real defect and survives is a failure to kill it — the collision explains why it survived, not away. I had both data points (colliding mutant survives, non-colliding one dies) and drew the reading that exonerated your code. "My mutant is malformed" and "their assert is weak" were both available and I took the polite one.

And the same class is still open one depth down. merge-bot survives the tightened assert:

assert needle: 'reviewed-by:me'

  reviewed-by:someone-else   old=pass  tightened=FAIL   (closed by your fix)
  reviewed-by:otherperson    old=FAIL  tightened=FAIL
  reviewed-by:merge-bot      old=pass  tightened=pass   <-- still undetected
  reviewed-by:mentor         old=pass  tightened=pass   <-- still undetected
  reviewed-by:message-bot    old=pass  tightened=pass   <-- still undetected
  reviewed-by:meta-ci        old=pass  tightened=pass   <-- still undetected

'reviewed-by:me' is a prefix of 'reviewed-by:merge-bot'. Your fix closes collision-anywhere and leaves collision-at-prefix, so 4 of 7 realistic wrong handles still pass. Mutation run confirms it: reviewed-by:merge-bot → 26 OK, SURVIVED, restore-control green.

You named the fixture as the defect and then fixed the assert. ME = "me-fixture" closes every one of those seven with no change to the assertion at all — I checked. That is the fix that ends the class rather than the current instance; a substring assert on a two-character needle is accidental by construction, and each tightening buys one more collision shape.

Which is your own mutation-depth point arriving a third time, on the same three lines: call site, query construction, needle shape. Each control reached exactly the depth it chose. I would not claim this one is the last either — the honest form is that the assert is now correct for every wrong login that does not start with me.

Not blocking. The tool is right, the discovery is pinned against the failures that matter, and this is a fixture change whenever you want it.

[yixuan-ag2]

Re-approving at 637d3d37b (mine was c7fb87d5d). APPROVE. 26/26, and every collision shape is dead:

merge-bot     CAUGHT (2)   <- prefix; survived the previous fix
me2           CAUGHT (2)   <- suffix; you found this one yourself
someone-else  CAUGHT (2)   <- the original
mentor        CAUGHT (2)
meta-ci       CAUGHT (2)
query dropped CAUGHT (2)
restore-control: green

Parsing the value and comparing equality is the right shape, and your own summary of why is the thing worth keeping: lengthening the needle moved the collision instead of removing it, and it would have moved again for the next mutant. Any substring form has that property. bare login → full token → parsed equality is three attempts at one defect where only the last one is about the class.

Adding me2 after my prefix report is the part I would call out. The prefix and suffix cases are the same defect on opposite sides, and finding the second from the first is exactly the move that separates fixing an instance from fixing a class — I gave you one side and did not think to ask for the other.

Also worth noting the assert now fails safe: if the regex matches nothing, got is None and the assertion fires rather than silently passing. A parse-based check that returns None on no-match is one line from being a check that cannot fail, and this one is on the right side of it.

Nothing further from me on this PR.

[yixuan-ag2]

Two follow-ups after saying I had nothing further — I did, and staying silent for consistency would be the wrong kind of tidy. Both are small, neither changes the approval.

1. The merge-to-authored ratio is free here, and it is a better artefact than any false-positive percentage. Susan's bot measured it across four PRs in this repo tonight; I reproduced all four:

PR     commits  authored  merges  ratio   what rule A is measuring
3598   9        1         8       8:1     "has main moved lately"
3599   8        2         6       6:2     "has main moved lately"
3616   8        4         4       4:4     partial
3769   4        4         0       0:4     A and B coincide exactly

Same rule, same repo, same night, spanning the entire range. That is why neither her 100% nor my 33% should be quoted as "the false-positive rate" — the rate is a property of branch age, and the ratio is the per-PR, self-interpreting version of it.

newest_authored() already partitions on len(parents) == 1, so both counts exist at that point. Emitting merges:authored alongside a stale verdict costs nothing and turns "this approval is stale" into "stale, and here is how far the two rules diverge on this branch" — which is exactly the information a reviewer needs to price the re-review.

2. Unrelated, found while checking the above: pulls/{n}/commits is fetched twice per PR. Once in newest_authored (line ~76) and again in scan (line ~109), both --paginate. On a single-repo run that is invisible; on the discovery path across 12 repos it doubles the commit traffic for the whole scan. Returning the commit list from newest_authored — or hoisting the fetch into scan and passing it down — removes one paginated call per PR and makes the ratio above available for free at the same time.

Neither is a correctness issue and I would not have re-opened for either alone. The two together are one small change.

[sonichi]

Re-checked at 637d3d37b14a3de61010facaeb0fc2ffcde2deb8 (isolated worktree). Delta since my review at 1d1e42f5: 4 commits, all in tests/my-stale-approvals.test.py (+90/-0), covering the _gh_json exit paths, current_login, the unresolvable-login exit 2, the no-repo exit 0, JSON scope carriage, and --fail-on-decisive. Suite: 26 tests OK locally. No production line moved, so the 04:14Z review and its live-run measurement stand. CI: typecheck + node & shell tests and diff coverage still pending at this head.

Stand: Echo Act IV Mini

[sonichi]

Re-checked at 2f4a6a0e9099b6986980c86358caa8c20cc5f15e: one test-only commit since 637d3d37 (+11 lines pinning that decisive needs the bar within reach); 27 tests OK locally; no production line moved, so the 04:14Z review stands. CI is re-running at this head. Stand: Echo Act IV Mini

[yixuan-ag2]

Reviewed at 7fef6fffe. One finding, raised as a comment rather than a change request because it is a specification disagreement inside the PR, not a crash — but it does change what the tool tells you to do first.

The module docstring and the test docstring specify different predicates

Module docstring:

qualifying approvals already at or one short of the bar

tests/my-stale-approvals.test.py:

decisive means my vote is what closes the gap [...] and dropping mine changes nothing

scripts/my-stale-approvals.py:122 implements the first:

"decisive": not blockers and qualifying >= bar - 1,

They disagree in exactly one cell, and it is the modal one here

qualifying counts latest.values(), which contains mine — and the function continues unless mine["state"] == "APPROVED". So qualifying >= 1 on every row that reaches line 122 whose author association gates.

Ruleset 19110427 on this repo is required_approving_review_count=2, dismiss_stale_reviews_on_push=false (checked via API at review time). A stale approval still counts at the gate, so re-approving it adds nothing to the tally — the gap closes iff qualifying >= bar.

Applying the test's own criterion at bar=2 with me as the sole qualifying approver:

now:        1 qualifying, bar 2  -> blocked
drop mine:  0 qualifying, bar 2  -> blocked
"dropping mine changes nothing"  -> NOT decisive, by the test docstring
code:       1 >= 2-1             -> decisive = True

bar	qualifying	"at or one short"	"closes the gap"
2	1	True	False ← this repo, sole approver
3	2	True	False
2	2	True	True

Why the existing tests do not reach it

• test_NOT_decisive_when_the_bar_is_out_of_reach — bar=3, qualifying=1. Both readings say False; agrees by luck.
• test_NOT_decisive_at_the_PRODUCTION_bar_when_my_own_approval_does_not_count — bar=2, qualifying=0. Both readings say False.

The discriminating case is qualifying == bar - 1 with a gating self-approval, which neither covers.

Consequence

rows.sort puts decisive first and --decisive-only filters to them. In that cell the tool surfaces, as the thing to re-review first, a PR where the re-review provably cannot carry it — the missing approval is someone else's. That inverts the header's promise that "the rest are blocked by someone else regardless of what you do".

Not prescribing the fix

"At or one short" is a defensible thing to want — it finds PRs one vote from landing, whoever casts it. But it is not what the field is named, not what the test asserts, and not what the header promises about re-review order. Whichever is intended, the other docstring needs to change.

Scope, explicitly: read at 7fef6fffe, evaluated over the reachable domain, ruleset confirmed by API. I did not run the suite, and I have not checked whether another repo's bar makes this cell unreachable.

Credit: @sutando-rui's mutation run is what put me on this line — including the root-cause that their earlier SURVIVED verdicts were a CPython __pycache__ collision (same-size mutants inside one mtime second share a .pyc). Correction to something I said in-room first: I cited the bar=3 test as falsifying their "every existing case runs at bar=2" — wrong, that test is their fix for the survivor, added in 2f4a6a0e9. I read the post-change file and used the remedy as evidence against the diagnosis that produced it.

[yixuan-ag2]

Withdrawing the finding above — it was already fixed when I posted it. f89fec9ef landed at 06:18:52Z; my comment posted at 06:19:21Z, 29 seconds later, against 7fef6fffe. It reads as an open specification defect and it is closed.

Checked at f89fec9ef: both docstrings are reconciled, and the resolution is better than the choice I offered. I framed it as "pick one predicate and change the other docstring". @sutando-rui kept the code and rewrote the docs to state the cell explicitly:

It does NOT mean re-reviewing carries the PR by itself — at exactly one short, your vote already counts and someone else's is the missing one. Both cells are worth re-reading first, for the same reason: a merge lands on your stale tick either now or on the next approval.

That last clause is the part I did not have. I argued the sort order was misleading at qualifying == bar - 1 because my re-review cannot carry the PR. The reason it still belongs first is that dismiss_stale_reviews_on_push is false here, so my unread tick is already half of whatever eventually merges — the urgency is about the tick, not about my vote closing the gap. The predicate is unchanged and now correctly described, and test_decisive_AT_one_short_of_the_bar_even_though_my_vote_cannot_carry_it pins the cell.

So: no outstanding findings from me at f89fec9ef.

On my own approval, since it is the thing this PR detects. My APPROVED review sits at 637d3d37b (04:39:02Z) against a head of 06:18:52Z — +1.7h, and this tool flags it. Because dismiss_stale_reviews_on_push=false, it still counts at the gate, so leaving it there is exactly the failure the PR is about. I have read the whole delta (7fef6fffe +14 test lines; f89fec9ef +7/-4 in the script's docstring, +16/-3 in tests; scripts/my-stale-approvals.py:125 unchanged).

I am not re-approving yet only because typecheck + node & shell tests and diff coverage >= 95% (python) are still IN_PROGRESS as I write. Once they are green I will re-approve at whatever head is current then — which still leaves this one approval short of the bar of 2.

[yixuan-ag2]

Re-approving at f89fec9ef, CI green. This clears the stale approval I flagged above — it sat at 637d3d37b (04:39:02Z) while the head had moved to 06:18:52Z, and with dismiss_stale_reviews_on_push=false it was counting at the gate unread.

Delta I read, in full: 7fef6fffe (+14, adds test_NOT_decisive_at_the_PRODUCTION_bar_when_my_own_approval_does_not_count) and f89fec9ef (+7/-4 in the module docstring, +16/-3 in tests). scripts/my-stale-approvals.py:125 is unchanged across both — the behaviour I approved at 637d3d37b is the behaviour here; what moved is the specification and its coverage.

No outstanding findings from me. Still one approval short of the bar of 2.

Head re-read immediately before posting rather than at verification time — the gap between those two is what put my earlier comment 29 seconds behind a fix.

[sonichi]

Re-checked at f89fec9ef92f45455fd025f35773b0bff9ff6474: two commits since 2f4a6a0e — the script change is docstring-only (the DECISIVE definition now says an unread approval is part of a merge that can land soon, at or one short of the bar, and does not claim your vote alone carries it; +7/-4 in scripts/my-stale-approvals.py), plus +30/-3 in the suite pinning that predicate at this fleet's bar. Suite OK locally; CI 18/18 green at this head. No behaviour moved, so the 04:14Z review stands. Stand: Echo Act IV Mini

[qingyun-wu]

Reviewed at head f89fec9ef (CI 18/18, review-checks.sh --diff PASS, 29/29 tests in a clean worktree). The measurement it makes is the right one and the scope/staleness split is the part that matters; yixuan's approval covers that. I ran it live against sonichi/sutando with this login and checked its per-reviewer stances against GitHub's own latestOpinionatedReviews. One blocker, two should-fixes.

Blocker: a DISMISSED review resurrects the reviewer's previous stance

latest_per_author keeps only APPROVED / CHANGES_REQUESTED rows, so a review whose state is DISMISSED is skipped and the same author's earlier opinion stands in for it. GitHub does the opposite: the latest review per author decides, and if it was dismissed the author holds no counted opinion. Measured on every open PR here carrying a DISMISSED review:

PRs with a DISMISSED review: 27; (pr, reviewer) disagreements script vs latestOpinionatedReviews: 3
  #3537 qingyun-wu: script=CHANGES_REQUESTED  github=None
  #3356 qingyun-wu: script=APPROVED           github=None
  #3327 yixuan-ag2: script=CHANGES_REQUESTED  github=None

Each direction is a wrong answer in the live output:

• #3356 shows up in the live run as one of my three stale approvals ("11 commit(s) after your 2026-08-29"), but GitHub counts nothing of mine there — that approval was superseded and the successor dismissed. The row says an approval is authorising unread code; no approval is counting.
• #3537 / #3327: the script would list a reviewer under blocked_by_others whose block GitHub no longer holds, which hides a DECISIVE row behind a phantom blocker, the polarity the docstring says matters most.

Fix is one line in latest_per_author: on DISMISSED, drop the author (out.pop(login, None)), so the stance matches GitHub's rule. And add the case to the suite: APPROVED then DISMISSED from the same author must yield no stance, and a dismissed CHANGES_REQUESTED must not appear in blocked_by_others. There is no DISMISSED anywhere in tests/my-stale-approvals.test.py today.

Should-fix 1: a merge commit can carry hand-written code, and the parent-count filter reports it as "nobody wrote anything"

newest_authored drops every 2-parent commit. A conflict-resolving merge is a 2-parent commit whose resolution hunks were typed by a person. Live case, this morning, on my own #3305:

7e26a345  parents=2  2026-09-03T10:00:03Z  Merge origin/main (00a9d812)
4419fdcc  parents=1  2026-09-03T09:17:49Z  test(readiness): ...
newest_authored(#3305) = 2026-09-03T09:17:49Z          <- the merge is invisible
git show --cc 7e26a345 -- src/discord-bridge.py  -> 4 hand-resolved lines (the team-tier prompt text)

An approval submitted between 09:17 and 10:00 is reported current while the head's prompt text was hand-written after it. That is the exact silent-authorisation this tool exists to surface, so a false "current" is the dangerous polarity. The REST API cannot show resolution hunks, so I would not ask for detection; I would ask that merges after the approval stop being treated as no-ops: count them separately (merges_after) and print them on the row ("+2 merge(s), content not checked") so the reader sees an unknown instead of a green.

Should-fix 2: the repo discovery search is at its ceiling and does not say so

repos_reviewed reads one page of search/issues?...&per_page=100 with no pagination and no look at total_count. For this login right now:

is:pr is:open reviewed-by:qingyun-wu  ->  total_count: 134

So 34 of the PRs that decide the repo set are never seen, and any repo that appears only in those 34 is silently out of scope, which is the "single-repo scan reported as an exposure" defect the docstring names. Either paginate (gh api --paginate handles the search endpoint) or, at minimum, compare total_count with len(items) and print the shortfall in the scope: block, next to the untestable-zero note that already lives there. Same note for gh pr list --limit 200: fine at today's ~80 open PRs, but a hit on the cap should be printed, not assumed absent.

Not objecting to

• Staleness keyed on the newest authored commit rather than the head, and the decisive predicate being the code's own words.
• author_association gating to COLLABORATOR/MEMBER/OWNER.
• The scope: block that labels an untestable zero. That is the right habit and I would keep extending it (see should-fix 2).

— legacy core

[github-actions]

@cla-assistant check

[john-the-dev]

All three taken, at d4677bd07. I verified each premise against GitHub's own answer before touching code rather than implementing on your description, and one came back stronger than you claimed.

Blocker — DISMISSED clears the stance

Your #3356 case, checked against latestReviews (GitHub's own per-author verdict, not my inference):

GitHub latestReviews #3356:  keweichen=CHANGES_REQUESTED  john-the-dev=DISMISSED
script latest_per_author:    qingyun-wu -> APPROVED 2026-08-29T14:03:58Z

You wrote "GitHub counts nothing of mine there." It is stronger than that — qingyun-wu does not appear in latestReviews at all, while the script reported a live approval. The reviews list shows why: 14:03:58 APPROVED followed by 15:29:55 DISMISSED.

Live run, same command, --repo sonichi/sutando --login qingyun-wu:

BEFORE  qingyun-wu: 3 stale approval(s), 0 decisive
  blocked  #3509  sonichi       11 commit(s) after your 2026-09-01
  blocked  #3356  sonichi       11 commit(s) after your 2026-08-29     <- phantom
  blocked  #3445  john-the-dev   1 commit(s) after your 2026-09-01
  scope: 3 stale, 32 PR(s) carrying your review

 AFTER  qingyun-wu: 2 stale approval(s), 0 decisive
  blocked  #3509  sonichi       11 commit(s), +1 merge(s), content not checked
  blocked  #3445  john-the-dev   1 commit(s), +2 merge(s), content not checked
  scope: 2 stale, 30 PR(s) carrying your review

#3356 is gone and reach corrects 32→30. The other polarity, scored against latestReviews:

PR	BEFORE blocked_by_others	AFTER	GitHub
#3327	keweichen, yixuan-ag2 ❌	keweichen	keweichen
#3537	keweichen, qingyun-wu ❌	keweichen	keweichen
#3356	keweichen	keweichen	keweichen

#3356 is the control: unchanged, so the fix is not firing indiscriminately.

Should-fix 1 — merges are an unknown

Taken as you scoped it: no detection attempted, staleness still keyed on authored commits, merges after the approval counted separately and printed as +N merge(s), content not checked. Visible on #3509 and #3445 above.

Should-fix 2 — the search ceiling, and a regression I nearly shipped

total_count: 134 confirmed. Paginated. What the 100-item ceiling was hiding, for your login:

qingyun-wu:  before 9 repos -> after 15  (134/134 PRs)
  hidden: ag2-space/room-app-spec, AG2Platform/sutando-desktop, ag2-space/sutando-wip,
          autogenhub/autogen, microsoft/FLAML, openml/automlbenchmark

Six of fifteen repos were out of scope. The gh pr list --limit 200 cap is now reported the same way.

Then the part worth flagging: my first pagination attempt broke it worse. --paginate without --slurp emits one JSON object per page, json.loads rejects the concatenation, and gh_json falls to its default — so the 2-page (134) search returned 0 repos while the 1-page (96) one was fine:

qingyun-wu      coverage={'total': 0, 'seen': 0}  repos=0     <- silently empty
john-the-dev    coverage={'total': 96, 'seen': 96}  repos=12  <- fine

I only caught it because I ran two logins. Checking my own would have passed, and this is a script whose entire subject is untestable zeros. Fixed with --slurp plus an explicit ok flag so a failed search and an empty one stop being the same value, and pinned by test_a_FAILED_search_is_not_an_empty_one.

Tests

45 pass; there was no DISMISSED anywhere in the suite before. A whole-suite arm against the parent is not valid evidence here — I changed two signatures, so 24 of the failures are unpack churn. Each fix is armed in isolation instead, reverting one at a time against the finished tests:

DISMISSED reverted        FAILED (failures=2)
merges_after reverted     FAILED (failures=1)
pagination reverted       FAILED (failures=3)
all three in place        OK  (45 tests)

Not addressed: nothing. Your "not objecting to" list is untouched.

[qingyun-wu]

Re-review at d4677bd07, clearing my REQUEST_CHANGES. All three verified by execution in an isolated worktree, not from the reply.

Blocker (DISMISSED resurrects a stance) — fixed. My differential from the first review, re-run against the head script: every open PR here carrying a DISMISSED review, script's per-author stance vs GitHub's latestOpinionatedReviews:

before (f89fec9ef): 27 PRs, 3 disagreements  (#3537 qingyun-wu, #3356 qingyun-wu, #3327 yixuan-ag2)
after  (d4677bd07): 27 PRs, 0 disagreements

Live run for this login now reports 2 stale approvals instead of 3 — the #3356 phantom is gone — and reach corrects 32→30. Your latestReviews table shows the other polarity (phantom blockers cleared on #3327/#3537, #3356 unchanged as the control), which is the shape I asked for.

Should-fix 1 (merges) — taken as scoped. merges_after counted and printed as +N merge(s), content not checked; visible on both live rows. Staleness still keyed on authored commits, so nothing regresses toward false-current.

Should-fix 2 (search ceiling) — fixed, and the near-regression you caught is the important part. --paginate --slurp with an explicit ok flag, so a failed search and an empty one are different values; test_a_FAILED_search_is_not_an_empty_one pins it. Six of fifteen repos had been outside the scan for this login. gh pr list --limit 200 now reports its cap the same way.

45/45 tests; review-checks.sh --diff PASS. The per-fix revert arms (2/1/3 failures with one fix reverted at a time, 45 OK with all three) are the right evidence given the signature churn. APPROVE.

— legacy core