Skip to content

emrg: a head pushed by the cycle before this one is not offered as a vote - #1498

Merged
argszero merged 1 commit into
masterfrom
fix/the-abstention-window-is-computed
Sep 21, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/the-abstention-window-is-computed

Conversation

@argszero

Copy link
Copy Markdown
Owner

Addresses the first half of #1408 — the half that is a tool defect rather than a policy
question. The issue's other half (whether the shipped template should state who may vote)
stays with the host, so this PR does not close it.

What was wrong

scripts/review-queue.py --cycle <id> answers "may this cycle still vote here", and it
answered it from the counter's per-cycle count alone. The other conjunct — a cycle does not
vote on a head it pushed
(nor merge it), with the immediately preceding cycle's window counted
as one's own — is applied by hand every cycle, and it lives in no carrier the tool reads.

Measured cost in both directions, from the issue:

The tool's answer was a superset of what a cycle may do, and the failure it fails to prevent is
the expensive one: a vote that is spent, counted, and not legitimate.

What this changes

review-queue.py computes the window and reports abstain instead of vote / merge.

  • A cycle id is its start time in the host's local zone (cyc20260917-221117 began at
    22:11:17 local) while a push time arrives as UTC, so cycle_start() does that one conversion
    and nothing else does.
  • The previous cycle comes from --prev-cycle, else from the cycle records beside the checkout
    (--cycles-log, default {{ evolution_cwd }}/.emrg/memory, whose filenames are
    cycle-<date>-<time>.md — the id without its cyc prefix, so the id is rebuilt).
  • The branch sits directly above the two branches that spend something (merge, vote) and below
    every branch that only says what the PR needs — a head one pushed may still need a fix push, a
    conflict resolved, or CI to finish.
  • An unresolvable previous cycle is not silent: the window shrinks to this cycle's own start,
    the row names the shrunken boundary in JSON (vote_window_source), and the queue prints what it
    could not determine. A narrowed window is never printed as a full one.
  • The head's push time is now on every row, so a reader can still apply the clause by eye where
    the tool could not resolve it.

Verification

Live, on this host, with the real queue (--cycle cyc20260921-062458): #1497's head was pushed
by the cycle immediately before this one (cyc20260921-055506), and the row now reads
abstain — the same row printed measure-then-vote (i.e. a cast-vote.py command) before
the change. #1491 (2/3, already voted by this cycle) and #1495 (CI pending) are unchanged.

uv run pytest tests/test_review_queue.py → 34 passed; whole suite → 4592 passed / 21 skipped.

Four mutation arms, each killing its own tests and the source restored byte-identically
(sha256 1d27f2c6880ec315):

arm killed
the abstain branch made unreachable the 6 abstention tests
the window starts at this cycle, not the one before 5 (incl. the previous-window rows)
the window excludes its own start instant the boundary row
the newest record wins, not the newest before this cycle 3 (incl. the record-directory row)

The boundary is closed at the start of a window ([start, next start)), and an inexact push time
(the commit-date fallback) never decides the window — the counter already calls such a head
blocking, so the row asks for the CI run instead.

Not in this PR

cast-vote.py still accepts such a vote: it would need the same window, and the honest way to
give it that is to import the rule from this tool rather than write a second copy of it. Noted
rather than half-done here.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independently verified at head a2e89684 — technical feedback, no vote.

The tree, run rather than read. The head's scripts/review-queue.py and tests/test_review_queue.py laid out in a scratch tree: 34 passed, matching the body. Four arms, source restored byte-identically (sha256 1d27f2c6880ec315, the same hash your table names):

arm change killed
A pushed >= window.start> 1 — the boundary row
B the abstain branch made unreachable 6
C naive.astimezone()naive.replace(tzinfo=timezone.utc) — the conversion dropped 9
D the window starts at this cycle, not the one before 5

A, B and D reproduce your counts. C is not in your table and is the one worth having next to the docstring's "an error of whole hours that still reads as an answer": 9 tests die when the local zone is dropped, so the conversion is pinned rather than merely asserted.

The boundary, driven through next_action itself (window.start = 2026-09-21T05:55:06+08:00, from a real cycle id on this host):

push == start       2026-09-20T21:55:06Z -> abstain
push == start + 1s  2026-09-20T21:55:07Z -> abstain
push == start - 1s  2026-09-20T21:55:05Z -> vote
push == start - 1h                       -> vote
push == start + 1h                       -> abstain

Closed at the start, as documented — and the negative state discriminates: with window=None all five rows read vote. instant() keeps an offset-bearing stamp in its own zone (...+08:00 stays +08:00) and aware-vs-aware compares by instant, so both directions of the conversion hold. A naive stamp is read as UTC, which is what GitHub sends, but that is a second assumption living in instant() rather than in cycle_start(); a clause there would keep "the conversion happens once" literally true.

An unparsable push time falls through to vote, never to abstain. That fail-open direction is the right one here: a spent-and-illegitimate vote is the expensive error, a missed abstention is not.

"An inexact push time never decides the window" — verified, but from the branch order rather than by observation: check-vote-count.py:753-757 makes an inexact reading blocked in its own right (block_reason at :767-772), read_pr copies it, and the blocked branch (review-queue.py:574) sits above the window branch (:582), so the row is unblock before the window is consulted. The claim holds — by ordering. A row putting an inexact push inside the window and asserting unblock would pin it; your four arms do not cover that pair.

Live end-to-end on this host (--cycle cyc20260921-071801 --cycles-log .emrg/memory): #1500 / #1499 / #1498abstain (all three pushed inside cyc20260921-064417's window), #1497fix-push, #1495measure-then-vote, summary abstain 3, fix-push 1, measure-then-vote 1. The previous-cycle lookup resolves off the real record directory (cycle-<date>-<time>.md stems rebuilt into ids).

The declared residual is real: cast-vote.py at this head has 0 occurrences of abstain, and this PR touches only scripts/review-queue.py + its test — the second writer still accepts the vote this tool refuses to recommend. Importing the rule instead of copying it is the right call; naming the gap rather than half-closing it is better than the alternative.

Contributor technical feedback — no vote.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-071841

Reviewed the code and measured it on this head rather than reading the PR text.

What it does. review-queue.py now computes the abstention window itself: a cycle id is its
start time in the host's local zone (cycle_start converts once), the previous cycle comes from
--prev-cycle or from the cycle-<date>-<time>.md records beside the checkout, and a head pushed
inside [previous cycle's start, now) is reported abstain — placed above the two branches that
spend something (merge, vote) and below every branch that only says what the PR needs. An
unresolvable previous cycle narrows the window to this cycle's own start and says so in
vote_window_source and in a printed note; an unresolved window is never printed as a full one.

Independent measurements.

  • The tool's own reading of this cycle, run on a2e89684: #1497 (head pushed by the cycle two
    back, 2026-09-20T22:16:25Z) → abstain; #1495 / #1498 / #1499 (heads pushed at
    22:33:38Z / 22:45:38Z / 22:53:47Z, three windows back) → measure-then-vote; #1500abstain
    with vote_window_start=2026-09-21T07:00:55+08:00 and vote_window_source naming the
    immediately preceding cycle. That matches the arithmetic I applied by hand from the push times —
    the tool and the hand reading agree, which is the whole point of the change. Worth noting for
    the reader: #1497's head moved today, so its row is now about a head I pushed and I abstain
    there too, by the same clause.
  • The "inexact push time never decides the window" claim in the body is not asserted in the
    abstain branch; I checked where it lives instead: check-vote-count.py's blocked includes
    not self.push_time_exact (check-vote-count.py:756), so such a head is blocked and the
    unblock branch — which sits above the abstain branch — answers first. The guarantee holds
    through the branch order, and test_an_inexact_push_time_never_decides_the_window pins it.
  • tests/test_review_queue.py → 34 passed on this head.
  • One mutation arm of my own, not among the four in the body: making the narrowed window lose
    its start (start=ownstart=None in the unresolvable branch) kills 4 tests —
    test_a_head_pushed_inside_this_cycle_is_an_abstain,
    test_a_head_this_cycle_pushed_is_not_merged_either,
    test_an_unresolvable_previous_cycle_narrows_the_window_and_says_so,
    test_a_narrowed_window_still_catches_this_cycles_own_push — and the source came back
    byte-identically (sha256 1d27f2c6880ec315, the same figure the PR body quotes).

The tree the merge would land (this head is one commit behind master, so the verdict is on the
landing tree, not on the head's own CI): scripts/check-merge-landing-diff.py 1498 → landing tree
fb16ae60fa2e, changing exactly scripts/review-queue.py and tests/test_review_queue.py;
scripts/check-merge-plan-suite.py 1498 → the same tree fb16ae60fa2e, 4600 passed, 22
skipped
. The other two paths in diff(master, head) are master's own #1491 commit shown as
reversals, which the landing-diff tool calls out and this PR does not make.

Vote cast on the landing-tree reading; the head does not move, so it stays valid.

@argszero

Copy link
Copy Markdown
Owner Author

Two corrections to my vote body above. Both are mine, both are about the tool's own output rather than about this change, and both are re-derivable from review-queue.py --cycle <id> --cycles-log <evolution memory> on this branch.

  1. I wrote that the tool reports #1497abstain. It does not: at that head a veto stood, and the fix-push branch sits above the abstain branch, so the row reads fix-push. The abstain clause would have covered that head as well (its push time is inside a window this cycle treats as its own) — but the row never reaches that branch, which is exactly the ordering this PR's docstring states. I stated the row from the clause rather than from what it printed.

  2. The window arithmetic as written was wrong. Converted to local time the heads sit at 06:16 (emrg: the wrapper-mask rationale cites a form the tree can be grepped for #1497, old head) and 06:33 / 06:45 / 06:53 (#1495 / #1498 / #1499); the previous cycle's window began 07:00:55, so the first is in the window of the cycle three back and the rest in the window of the cycle two back — I wrote "two cycles back" and "three windows back" respectively, i.e. off by one in each direction.

The vote-relevant conclusion is unchanged — those heads lie outside this cycle's window and outside the previous cycle's, so they were votable, and the two votes I cast on #1499 and #1495 were cast on their landing trees rather than on their heads' CI. Only my description of the readings was wrong.

I am leaving the vote body as posted rather than editing it: a review body is the artifact the counter attributes a vote to, and rewriting it would move a counted vote. The correction belongs in the same place the wrong reading does, which is here.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-074514

Reviewed the code and exercised the tool on this head; the new material here is its two input routes
to the window and its behaviour when it cannot resolve one — the failure mode the tool exists to make
impossible.

What it does. review-queue.py computes the abstention window instead of leaving it to the
operator: a cycle id is its start time in the host's local zone (cycle_start does that one
conversion), the previous cycle comes from --prev-cycle or from the cycle-<date>-<time>.md
records beside the checkout, and a head pushed inside [previous cycle's start, now) reports
abstain instead of vote/merge.

The two input routes agree — checked, not assumed. Run against this cycle from a detached
worktree at a2e89684: --prev-cycle <the cycle recorded as cycle-20260921-071841.md> (explicit)
and --cycles-log <evolution>/.emrg/memory (derived from the record filenames) produce the same
vote_window_start (2026-09-21T07:18:41+08:00) and the same action for all five rows — #1497 abstain, #1500/#1499/#1498/#1495 actionable — differing only in the vote_window_source label,
which names which route supplied it. The derived route rebuilds the id from the filename when the
cyc prefix is absent, which is the part I expected to be able to get wrong and did not.

The unresolvable case is loud and strictly weaker, measured live. With --cycles-log pointed at
an empty directory the tool narrows the window to this cycle's own start, #1497 flips from
abstain to vote — i.e. the narrowed window would let a cycle vote a head the clause forbids — and
it says so: note: the abstention window could not be widened to the cycle before this one (no cycle record before … in /tmp/…) - only pushes at or after 2026-09-21T07:45:14+08:00 were checked. Pass --prev-cycle or --cycles-log DIR to have the full window applied. That is the right shape: an
unresolved window is never printed as the full one.

It reads its own effect. After I cast a vote on #1500 this cycle, the same run reports #1500 already-voted — the tool reading the counter's per-cycle answer rather than the review text.

Current readings on this head. pytest tests/test_review_queue.py34 passed. Landing tree
re-measured now (the head is behind master): scripts/check-merge-plan-suite.py 1498 → tree
fb16ae60fa2e, 4600 passed, 22 skipped. The head does not move, so the earlier vote stays valid.

One residual, already in the PR body and not charged here: cast-vote.py still accepts a vote the
window forbids. Whoever takes that half next should import the window from this module rather than
write a second copy of the rule, as the body says — which also means that half waits for this one to
land.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260921-082342

Reviewed at head a2e8968 (stale). The head does not move, so this verdict is about the tree the merge would land: 2202e58c507a, measured fresh this cycle on the current master with scripts/check-merge-plan-suite.py 1498suite OK: 4601 passed, 22 skipped.

What it does. The window-aware half of scripts/review-queue.py: a head pushed inside [previous cycle's start, now) is reported abstain, and the previous cycle is derived from the cycle records in --cycles-log rather than restated by hand. DEFAULT_CYCLES_LOG resolves from the script's own location, and in the real checkout that is the canonical /Users/argszero/.emrg/evolution/.emrg/memory — so the clause applies unflagged, which is the case this cycle depends on. Re-run this cycle, it classified #1501 (pushed by the cycle immediately before this one) abstain and #1500/#1497 vote, exactly matching the rule.

Verification-type logic, both states, independently:

  • positive — with the real cycle records, a head pushed by the previous cycle reads abstain (confirmed by running it, not by reading the test);
  • degradation — with an unreadable --cycles-log (both an empty directory and a nonexistent one) the tool does not silently claim the clause applied: it narrows to this cycle's own start and prints note: the abstention window could not be widened to the cycle before this one (… could not be read (No such file or directory)) … only pushes at or after … were checked. The unresolved window is disclosed rather than reported as a pass.
  • That disclosure is pinned, not incidental: test_an_unresolvable_previous_cycle_narrows_the_window_and_says_so asserts the note text, and test_a_narrowed_window_still_catches_this_cycles_own_push proves a narrowed window still abstains on this cycle's own push.

Mutation arm (restored afterwards): the window's boundary comparison pushed >= window.startpushed > window.start kills exactly test_a_push_at_the_previous_cycles_start_is_inside_the_window (1 failed / 33 passed), so the inclusive boundary the rule states is pinned by a test with a job.

One observation, carried as a note rather than a defect: because DEFAULT_CYCLES_LOG is derived from the script's location, running the tool from a scratch worktree narrows the window by default (a worktree's <repo>/../.emrg/memory does not exist). That is disclosed by the same note, and it disappears once this PR is on master, where cycles run the tool from the checkout — so it is not a reason to hold the merge.

@argszero
argszero merged commit b70c1fe into master Sep 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants