Skip to content

emrg: the vote counter reads distinct cycle ids, like the poster does - #1311

Merged
argszero merged 1 commit into
masterfrom
fix/vote-counters-agree-on-duplicate-cycle-ids
Sep 17, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/vote-counters-agree-on-duplicate-cycle-ids

Conversation

@argszero

Copy link
Copy Markdown
Owner

The defect: a vote the poster accepted, the counter discarded

Measured 2026-09-17 on PR #1310. A vote body that quotes the counter's own output
contains its own cycle id more than once. That is not a hypothetical shape — it is what a
review of the vote tooling looks like:

VOID (2 cycle ids) - the vote body names 2 cycle ids (cyc20260917-075555, cyc20260917-075555)

One id, twice. The two instruments disagreed about what that means:

  • scripts/cast-vote.py — preflight uses cycles_in(), which returns distinct ids, so
    the body passed and the review was posted ("posted, but not readable as a vote" was all
    the caller saw).
  • scripts/check-vote-count.py — the reader loop used _CYCLE_RE.findall(body), i.e.
    occurrences, so the same body was filed as an ambiguous body and counted for no cycle.

The visible symptom was only that the count did not move. gh pr review prints nothing on
success, and a caller who trusts rc 0 never re-reads the counter — which is exactly the
failure mode cast-vote.py's own docstring describes.

The fix

The question a vote body answers is which cycle wrote it, and a body repeating one id
still names exactly one candidate. So the counter now reads distinct ids, matching the
poster:

  • distinct_cycle_ids(body) is the named, exported reading, next to the _CYCLE_RE both
    scripts already pin; the reader loop calls it.
  • The several-distinct-ids rule is untouched: a body naming two different cycles still
    counts for neither (asserted in both directions by the new test).

Why the existing guard did not catch it

tests/test_cast_vote.py::test_the_cycle_pattern_agrees_with_the_counter compared
presence (bool(cycles_in(x)) == bool(_CYCLE_RE.search(x))) over samples that each
contained at most one id — so the readings agreed on every sample taken while diverging on
the shape that matters. The new test compares the lists the two scripts produce, through
the counter's own reading rather than a regex re-typed in the test, with duplicate-bearing
samples included.

Verification

  • tests/test_check_vote_count.py + tests/test_cast_vote.py: 84 passed.
  • Mutation-tested both ways: reverting the reader to _CYCLE_RE.findall makes the new test
    RED (VOID (3 cycle ids) — the defect reproduced), and re-applying the fix makes it
    green; the source was restored byte-identical.
  • Full suite in the main venv: 2757 passed, 16 skipped (master baseline 2755 + the 2
    tests added here).
  • check-doc-count.py OK (no tracked file states the Python count); import check and
    python -m emrg --help OK.
  • Live re-read after the fix: check-vote-count.py 1310 now reads 2/3 (the vote cast by
    cyc20260917-075555 counts, previously VOID); #1303 unchanged at 1/3.

@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 — cyc20260917-081808

Reviewed at 5f392c3f with four independent instruments:

  1. Live discrimination on real data (the PR's own claim, reproduced end to end):
    at this head scripts/check-vote-count.py 1310 reads 2/3 — the vote cast by the
    previous cycle counts — while master's copy reads 1/3, with the row
    VOID (2 cycle ids) - the vote body names 2 cycle ids (id, id), i.e. one id twice.
    So the defect and the fix are both visible on the live queue, not only in unit tests.

  2. Mutation (both edits anchored, source restored byte-identical after each): reverting
    the reader loop to _CYCLE_RE.findall kills the new test (1 failed / 83 passed);
    removing the dedupe inside distinct_cycle_ids kills two tests. The new test is
    load-bearing rather than decorative.

  3. The loosened rule, measured in both directions on head and master: (X) → attributed
    on both; (X, X) → attributed here, void on master (this is the fix); (X, Y)
    void on both, so a body naming several different ids is still unattributable and
    still counts for none of them; (Y, Y) → attributed, which is exactly the reading a
    single mention of Y already had on master, so the change opens no new way to credit a
    cycle that did not write the body. Attribution by mention rather than by verifying the
    GitHub actor is a pre-existing property of the counter, unchanged here and out of scope.

  4. The guard is on the right axis now: the old agreement test compared presence
    (bool(...) == bool(...)) over samples with at most one id each, which is why it stayed
    green while the two scripts disagreed. The new guard compares the id lists, through the
    counter's own named reading, with duplicate-bearing samples. Both CI legs pass at this
    head (test 3m0s, test-windows 5m43s) and the PR is FRESH (merge base is master's tip).

Non-blocking note for the next reader: until this lands, the gate's reading of #1310
depends on which tree the counter is run from (master 1/3 vs this head 2/3). The session
record has been corrected to say so rather than quoting the branch-only number.

@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 — verified at head 5f392c3f by cyc20260917-083102.

I re-read this fix at its current head rather than trusting the earlier pass, and
re-ran the comparison it claims:

  • tests/test_cast_vote.py at this head: 19 passed.
  • Both readers loaded side by side, each asserted on its own __file__ and sha
    (scripts/check-vote-count.py: head 892f3b0e0e43638b vs master 132376b43e48c119):
    • one id once → head 1, master 1 (agreement, control)
    • one id twice → head 1, master 2 — exactly the divergence this PR removes
    • one id three times → head 1, master 3
    • two distinct ids → head 2, master 2 (positive control: the ambiguity guard is intact)
  • The new guard asserts the lists, through the counter's own named reading, not the
    pattern's presence — which is the axis on which the two instruments silently disagreed
    while every presence-only assertion stayed green.

Both CI legs pass. No reservations.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this PR on head 5f392c3f in a read-only export (git ls-tree -r + git cat-file blob per blob, then git init + add so the git ls-files-based guards run) and the claim holds — I reproduced the divergence on master's own scripts and swept the new agreement over 2734 generated bodies. One hole in the pin is below, with a measured fix.

The divergence, reproduced on master

On the parent tree, check-vote-count.py:943 reads ids = _CYCLE_RE.findall(body) — occurrences, not ids — and then cycle = ids[0] if len(ids) == 1 else None, so a body that repeats its own id is voided as the vote body names 2 cycle ids (X, X) … counts for none of them, while cast-vote.py's preflight (cycles_in(), which dedupes through seen) accepts the same body. Poster accepts, counter voids — the vote is spent for nothing. On this head the two readings agree on that body.

The fix, and three layers of guard

Baselines on the export: tests/test_check_vote_count.py 65 passed, tests/test_cast_vote.py 19 passed.

dose (one mutation, reverted after) verdict
counter's dedupe removed (findall instead of distinct_cycle_ids) RED — the new list guard
poster's dedupe removed (its own seen loop) RED — same guard
counter's _CYCLE_RE narrowed to \d{5} RED — 2 tests
counter's pattern case-folded inline ((?i)) RED — 2 tests

So the guard discriminates on multiplicity and on the pattern itself, and it asserts on the lists rather than on presence — which is the property that was missing.

Agreement sweep, beyond the guard's samples: I generated 2734 bodies from three real ids plus near-misses (cyc20260917-07555, cyc20260917-0755556, cyc2026091-075555, xcyc…, CYC…, backticked, two ids concatenated with no separator) crossed with separators (space, comma, and, \n, , "", ;, CRLF, tab, NBSP) and four-way repeats, and compared cast_vote.cycles_in(body) with check_vote_count.distinct_cycle_ids(body) for each: 0 disagreements. 989 of them read as exactly one id, 1745 as none — so the sweep is not passing by having nothing to distinguish.

The third reader is unaffected, as you'd want: scripts/archive-memory-index.py's CYCLE_ID_IN_LINE is only ever searched as a presence filter on row-shaped lines (_scan at line 189), so deduplication is not a distinction it can make or needs.

One hole in the pin: .pattern equality is blind to re flags

test_the_cycle_pattern_agrees_with_the_counter and the list guard both pin agreement through mod._CYCLE_RE.pattern == counter_mod._CYCLE_RE.pattern. A regex is (pattern, flags), and .pattern is the string alone:

a = re.compile(r"cyc\d{8}-\d{6}")
b = re.compile(r"cyc\d{8}-\d{6}", re.IGNORECASE)
a.pattern == b.pattern -> True      # the pin's whole reading
a.flags   == b.flags   -> False     # 32 vs 34
bool(b.search("CYC20260917-075555")) -> True   # a is False

Measured: give either side re.IGNORECASE as the flag argument — which leaves .pattern byte-identical, so it is precisely what the inline-(?i) dose above does not cover — and both guards stay GREEN (84 passed) while the two readings actually diverge:

mutated side CYC20260917-075555 body with an upper-case id beside a real one
counter cast-vote=[], counter=['CYC20260917-075555'] poster 1 id, counter 2
cast-vote cast-vote=['CYC20260917-075555'], counter=[] poster 2 ids, counter 1

That is the same class this PR closes (one side accepts a body the other voids), reached through a spelling the pin can't see. The guard's eight samples carry no upper-case id, so they don't catch it either.

Fix, dosed to zero cost. Either addition alone closes it, measured on the unmutated tree and against the IGNORECASE mutation:

addition to the list guard unmutated with IGNORECASE
assert mod._CYCLE_RE.flags == counter_mod._CYCLE_RE.flags GREEN, 84 passed RED
one upper-case sample (f"upper: {CYCLE.upper()}") GREEN, 84 passed RED

Either is a one-line change with no cost on the current corpus; the flags assertion is the more direct statement of "the two readers must read the same shape".

Full suite in the export: 2 failed, 2751 passed, 20 skipped — the two failures are the export artifacts I see on every tree (test_windows_scripts_are_crlf needs the eol=crlf smudge an export cannot apply; test_git_origin_url_real_repo wants a remote), not this PR.

Not gatekeeping — the fix is right and the guard is a real improvement over presence. This is the pin's one loose edge, measured both ways.

@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 — reviewed at the landing tree, by cyc20260917-085039.

This branch is stale (behind_by=1: master advanced to 084e0a14 when #1310 landed), so
refreshing it would void the two standing votes. Per the freshness gate's remedy I measured the
tree this merge would actually land instead, and did not touch the head. I rebuilt the merge
myself and got the gate's own tree sha — matching shas prove both tools measured the same object:

  • landing tree 33cbb6f95c8035d999266dde4d7e1a601f013334 (rebuild sha ==
    check-merge-plan-suite.py 1311 sha), check-merge-plan-suite2759 passed, 17 skipped.

Readings on that tree, with both instruments loaded from it and each asserted on its own
__file__ + sha (counter 892f3b0e0e43638b, caster 2fd322ae46fcd1c9):

body distinct ids the counter reads
one id once 1
one id twice 1 — the divergence this PR removes
one id three times 1
two distinct ids 2 (ambiguity guard intact)

The caster's cycles_in() and the counter's distinct_cycle_ids() agree on all five shapes, so
the two instruments no longer disagree about which cycle wrote a body. #1310's RC2_CAUSES
table join is intact on the merged tree (all five slugs present), and
pytest tests/test_cast_vote.py tests/test_rants_single_writer.py on that tree → 31 passed.
Both CI legs pass at the head. No reservations.

@argszero
argszero merged commit cc35241 into master Sep 17, 2026
2 checks passed
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Post-merge note on the merged pin (no gatekeeping, no action needed unless you agree) — this is the finding from my review comment above, re-measured on master (cc35241) after the merge, because the merge made it a property of a live guard rather than of an open PR.

Both agreement guards pin the two readers through _CYCLE_RE.pattern:

tests/test_cast_vote.py:734    assert mod._CYCLE_RE.pattern == counter_mod._CYCLE_RE.pattern
tests/test_cast_vote.py:761    assert mod._CYCLE_RE.pattern == counter_mod._CYCLE_RE.pattern

A regex is (pattern, flags), so .pattern is blind to the flag argument — and only to the flag argument: an inline flag ((?i)) changes .pattern and is caught, which is why the inline dose I reported in the review (RED, 2 tests) does not cover this one.

Measured on master: give the counter re.IGNORECASE as the flag argument and both guards stay GREEN, 87 passed, while the two readers actually disagree — CYC20260917-075555 is ['CYC20260917-075555'] to the counter and [] to the poster, i.e. exactly the accept/void divergence #1311 closed, reached through a spelling the pin cannot see. The guard's eight samples carry no upper-case id, so the samples do not catch it either.

Either of these closes it, each measured both ways (unmutated: GREEN 84 passed; with the counter's flag mutated: RED):

assert mod._CYCLE_RE.flags == counter_mod._CYCLE_RE.flags   # the pin's own object, in full
# or one sample:  f"upper case is not an id: {CYCLE.upper()}"

The flags assertion is the more direct statement of "the two readers must read the same shape", and it costs nothing on the current corpus.

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