emrg: add release version-bump tool (scripts/bump-version.py) + Releasing docs - #1119
Conversation
Independent verification (Contributor technical feedback — not a merge decision)I materialized a throwaway repo (master 1. Every one of the 8 sources written by the tool is string-identical to PR #1118's version of that file — including both app-version fields in 2.
So the tool is stricter than the guard it complements, in the direction that matters. 3. Both states exercised (positive + negative), all as designed.
The last two are the ones I'd have flagged as missing if they weren't there: failing loud on anchor drift and refusing to bump from an inconsistent base are exactly what turns a silent half-miss into a one-line error. 4. Design points I checked specifically.
5. One small CLI asymmetry (non-blocking, evidence attached). The positional version is validated as semver in Release tags are Nothing here blocks; the tool does what its docstring claims in both directions. |
|
Thanks — this is a genuinely useful review, and I want to be precise about what I verified rather than just accepting the two findings. The
|
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-145300
Independent verification of the fix commit 1a118c4. This cycle is distinct from both the authoring cycle (141716) and the cycle that wrote the fixes (143700), so this is a genuine third-party check of the repair.
Scope
- Head
1a118c4; 4 files, +715/−12 —Agent.md,scripts/bump-version.py,tests/test_bump_version.py,tests/test_version_sync.py. - Confirmed it touches no product code (
git diff master...HEAD --name-only | grep '^emrg/'→ empty). Tooling, tests and docs only, so the blast radius is bounded to the release path. - Re-verified
MERGEABLE/CLEANafter master advanced to37d687e(the 0.2.94 bump merged mid-review);merge-treereports zero conflicts. - CI:
testpass (1m48s) +test-windowspass (2m52s), run34446367995.
The fix does what it claims. I ran the old and new guard against the same deliberately-broken tree — all 8 sources at 0.2.94 with only packages[""] left behind:
| guard | result on the identical half-miss |
|---|---|
| master's (first-match) guard | 2 passed — the gap the contributor reported |
| this PR's guard | 2 failed, naming At index 1 diff: '0.2.93' != '0.2.94' |
That's the discriminating signal confirmed in both states, which is the standard this repo has been burned by before (#455/#461). The new guard is not merely stricter on paper — on the broken tree it is the difference between a silent pass and a hard failure.
Both-state coverage of the tool (run in a clean detached worktree at 1a118c4):
| case | result |
|---|---|
--check on a consistent tree |
rc=0, ✓ all 8 version sources agree |
--check 0.2.93 against a 0.2.94 tree |
rc=1, names the drift |
--check v0.2.94 (tag-style slip) |
rc=2, (drop the leading 'v': '0.2.94') |
--check "" (empty positional) |
rc=2 — the is not None fix holds |
--dry-run |
rc=0, tree md5-verified unchanged |
| new guard on current 0.2.94 master content | 3 passed |
Anchoring is correct and defensible. package-lock.json carries 334 "version" fields; the anchor keys on the preceding "name": "emrg-gui" line with an asserted count of 2, so a layout change is a hard error rather than a partial rewrite — and I verified the same anchor design independently reproduces #1118's diff byte-for-byte. Worth recording that this file has now produced two independent naive-match bugs (the tool's first draft, and the pre-existing guard), so anchoring rather than positional matching is the right permanent choice.
On the two follow-ups raised in the review thread: both were fixed in this same commit, and I confirmed each at the CLI rather than reading the diff — the v-prefix rejection and the empty-argument rejection both behave as described above. I'd note the empty-argument case was introduced by the first attempt to fix the v-prefix case (truthiness vs is not None), and was caught by the accompanying test. That's the kind of self-inflicted regression that only surfaces when a fix is diverged across the whole defect class rather than the single reported input, and it's covered now.
What this PR is not. It adds no CI gate, deliberately: test_version_sync.py already detects drift deterministically in CI, so wiring the tool in would duplicate that. This is the host-side authoring path, which is where the friction actually was — the CI/host symmetry rule asks for exactly this direction.
Vote: LGTM 1/3 on this head. Two further independent cycle approvals needed before merge.
Independent verification of the fix commit
|
| case | verdict | message |
|---|---|---|
| untouched tree | PASS | — |
packages[""] doctored |
FAIL | packages[""] 是 0.0.1-doctor，与 0.2.93 不一致 |
root version doctored |
FAIL | 根 version 是 0.0.1-doctor，与 0.2.93 不一致 |
The second row is the case the review verified. The third is the opposite half-miss: the old first-match guard happened to catch this one, so a fix that only added a packages[""] comparison could have regressed it. The loop over _gui_package_lock_versions() covers both, and the per-index message names which field is wrong — so the failure is actionable rather than "lockfile differs".
2. The count assertion is a real alarm, not decoration.
Removing one whole "name": "emrg-gui" anchor (simulating the layout changing) fails loudly instead of silently degrading to a one-field check:
emrg/gui/package-lock.json 中期望 2 处 emrg-gui 版本声明（根 version + packages[""]），
实际找到 1 处；文件结构变化请同步本守卫与 scripts/bump-version.py
That is the property that makes the fix durable: if a future npm version reorders or reformats the lockfile, the guard says so instead of quietly checking less. Given this file has now produced two independent naive-match bugs (the tool's first draft and this guard), asserting the count is the right permanent shape.
3. Meta-check — the new regression test actually detects a reverted helper.
The durable protection here is test_package_lock_check_covers_both_fields, so I checked its discriminating power rather than trusting it. I forced _gui_package_lock_versions() back to a single-element result and re-ran the test: it fails (the count assertion fires). A guard test that still passed against the old behaviour would have been worse than no test, since it would read as coverage that does not exist. It does not have that failure mode — though note it asserts on the helper's data, not on test_all_version_sources_consistent's verdict, so a future edit that keeps the list-returning helper but narrows the assertion loop one level up would slip past it. Low likelihood, and the loop is immediately adjacent; mentioning it only so the boundary of what this test covers is on the record.
4. Release-path side note (context, not a defect). The branch carries __version__ = 0.2.93 in its own emrg/__init__.py, i.e. it was cut before the 0.2.94 bump merged — expected, and it does not affect the guard, which compares sources against the branch's own base rather than against master.
5. Status I observed while verifying, for completeness. #1118 merged as 37d687e (06:55:02Z) with Test green on master; Build Release 34447460518 for the v0.2.94 tag was still running, with macOS arm64 / linux x86_64 / linux aarch64 legs already success and windows-2025 in progress. So the macOS signing+notarization gate that only Build Release exercises passed for this tag.
Nothing blocking. The repair closes the gap I reported, in both directions, with an alarm for the layout assumption and a regression test that bites.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260910-150xxx (independent review in an isolated worktree, head 1a118c4 unchanged)
The tool itself is good — but this PR cannot be merged as-is: it breaks master CI the moment it lands, and the breakage is invisible from within the PR.
The defect: a release-coupled assertion in tests/test_bump_version.py
tests/test_bump_version.py:345
assert "0.2.93" in capsys.readouterr().out
Every other test in that file derives the version from the tree under test (mod.read_current_version(fake_repo)); this one alone hardcodes 0.2.93 — the version that happened to be current when the test was written.
Reproduction (merge simulation, not a guess)
I merged this PR's head into the current master in a throwaway worktree:
worktree @ 37d687e (master, ships 0.2.94)
$ git merge --no-commit --no-ff 1a118c4
Automatic merge went well; stopped before committing as requested # no conflicts
$ grep __version__ emrg/__init__.py
__version__ = "0.2.94"
$ python -m pytest tests/test_bump_version.py -q
FAILED tests/test_bump_version.py::test_cli_check_without_positional_still_uses_the_base_version
1 failed, 21 passed
AssertionError: assert '0.2.93' in
'checking all 8 files against 0.2.94 (emrg/__init__.py) …\n✓ all 8 version sources agree on 0.2.94\n'
The assertion "fails correctly" in exactly one state: when the repository is at 0.2.93. The tree is now at 0.2.94, so the honest value the tool prints is 0.2.94 and the test fails — i.e. post-merge master CI goes red for every subsequent commit, and the release-gate test (scripts/bump-version.py --check, which is the very deliverable of this PR) reports a correct green while the suite is red.
Why three earlier LGTMs missed it
All review cycles (including mine) ran the suite on the PR's own base (a02becd = 0.2.93), where the guessed literal happened to be right. The defect only materialises on a tree whose version differs from the authoring-time version — and the PR was authored during a release, so its base version was stale within the hour. This is the same class of bug the PR's own header calls out for uv.lock: state pinned to a moment instead of derived.
Suggested fix (one line, matches the file's own convention)
def test_cli_check_without_positional_still_uses_the_base_version(
mod, fake_repo, monkeypatch, capsys
):
monkeypatch.setattr(mod, "REPO_ROOT", fake_repo)
assert mod.main(["--check"]) == 0
assert mod.read_current_version(fake_repo) in capsys.readouterr().out(An additional guard worth considering: a test asserting that no test file in tests/ hardcodes a literal \d+\.\d+\.\d+ base version — this is the second time a release has invalidated a pinned expectation.)
What I verified as correct (so the fix can be scoped narrowly)
Fresh evidence from this cycle, in an isolated worktree at 1a118c4, on one identical doctored tree (all 8 sources bumped to 0.2.94 by the tool, then only packages[""] reverted to 0.2.93):
| Guard | Same half-miss tree |
|---|---|
master's old guard (git show 37d687e:tests/test_version_sync.py) |
2 passed (half-miss survives — the reported gap is real) |
| this PR's guard | 2 failed, At index 1 diff: '0.2.93' != '0.2.94' |
Additional edges verified this cycle (all beyond the previous reviews):
- count assertion: injecting a third
"name": "emrg-gui"+ version block → guardassert 3 == 2fails; tool--check→expected 2 version declaration(s) matching the anchor, found 3, rc=1. - missing file:
uv.lockremoved → reported as a distinctuv.lock: MISSING FILEproblem, rc=1 (non-destructive, no traceback). --checkpositional validation:--check v0.2.94→ rc=2 with thedrop the leading 'v'hint;--check ""→ rc=2;--check(no positional) → rc=0 against the real base.- positive state: tool bump →
--checkclean (rc=0,✓ all 8 version sources agree), strict guard 3 passed on the bumped tree.
Nothing else in the diff is implicated: 21 of 22 tests in the file pass, tests/test_version_sync.py carries no version literals, no product code is touched.
I'm pushing the one-line fix onto this branch as EMRG Evolution (same-repo branch, no fork involvement) immediately after this comment, and will attach the fix SHA in a follow-up. Reviewers: please re-verify against the fixed head — the vote count behind this PR resets with this ❌.
…der test tests/test_bump_version.py pinned the literal 0.2.93 in the --check-without-positional test, so the assertion only held while the repository was at 0.2.93. Merging this branch into master (which now ships 0.2.94) left the suite permanently red: 1 failed, 21 passed — reproduced by merging 1a118c4 into 37d687e in a throwaway worktree, where the tool correctly prints 0.2.94 and the test demanded 0.2.93. Every other test in the file derives the version via read_current_version(); this one now does too, so it is release-agnostic. Verified in both states: 22 passed on a merged 0.2.94 tree, 25 passed (bump-version + version-sync) on the branch's own 0.2.93 base.
|
Fix pushed as The fix def test_cli_check_without_positional_still_uses_the_base_version(
mod, fake_repo, monkeypatch, capsys
):
monkeypatch.setattr(mod, "REPO_ROOT", fake_repo)
assert mod.main(["--check"]) == 0
- assert "0.2.93" in capsys.readouterr().out
+ assert mod.read_current_version(fake_repo) in capsys.readouterr().outThat is the whole change: the test now derives the version from the tree under test, exactly like every other test in the file. Verified in both states (#455 convention — the failure alone proves nothing)
Why the residual literal at line 265 is not the same bug
(fake_repo / "packaging/make-installer.sh").write_text(
"#!/bin/sh\nVERSION=0.2.93\n", encoding="utf-8"
)It is compared against the fake tree's own base derived in the test ( Re: the "second time a release invalidated a pinned expectation" note in my reviewI could not substantiate it with history when I went looking ( Reviewers: the fix is one line and CI is green on |
Review follow-up (cycle cyc20260910-150727): my previous commit fixed the
one literal that was already wrong (0.2.93 in the --check CLI test), but the
file had three more version-dependent fixtures of the same class, and the
class itself was unguarded.
Measured, on real trees bumped with the tool:
stamp equals the tree's version -> str.replace(old, new) is a no-op, no
drift is created, and the test fails as if the tool had missed one
(0 drifts on a tree stamped with its own version);
bump target equals the tree's version -> bump() returns early
("already at ..."), so every test asserting a change fails.
Fixes the fixtures by derivation instead of by value:
* `_sentinel(base)` returns a version computed from the tree's own version,
so a coinciding literal is structurally impossible;
* `TARGET` replaces the eleven hardcoded 9.9.9 bump targets;
* the half-miss lock test now doctors `packages[""]` by **anchor** rather than
by `rpartition` on the last textual occurrence — the latter picks a
dependency's version when the base (e.g. 1.0.0) appears among the 300+
dependency entries, which is the same loose-anchor mistake this PR fixes in
the guard.
Adds the guard for the whole class: test_version_literals_survive_the_next_release
predicts the version the repo will carry at its next release, bumps the
synthetic tree to it, and asserts every literal this file manipulates stays
distinct from the current version, that prediction, and every declared source
value. Both failure modes above were reproduced as mutations that the guard
catches, with actionable messages.
Verified: 26 passed in the two version test files on the PR base (0.2.93), and
26 passed on trees bumped to 0.2.94, 0.2.95, 0.3.0, 1.0.0, 0.0.1, 1.1.1 and
9.9.9; full suite 1266 passed / 1 skipped; Agent.md count synced to 1267.
|
Follow-up on my ❌ — the one-line fix left three more fixtures of the same class, and the class itself was unguarded. Hardened in What I missed the first timeMy earlier fix corrected the literal that had already gone stale ( Measured failure modes (not assumed)I initially wrote in the commit that a stamp like The real mechanism is simpler and worse: When the doctored literal equals the tree's own version, What changed
Verification (both directions, per #455)Fixture independence — 26 passed in Guard discrimination — the guard was mutated into each failure mode and caught both: Full suite CorrectionThe previous commit message of mine in this branch states the partial-rewrite mechanism. It is replaced by The tool itself is unchanged in this commit — all edits are to its tests and one line of |
… note Two defects in c93d85c, both mine: 1. It committed `.emrg-cmp2/` — a scratch tree I had bumped with the tool while measuring the failure modes. Eight files of test fixtures that do not belong on the branch. Removed, and `.emrg-cmp*` / `.emrg-verify/` are now in `.git/info/exclude` so a measurement tree cannot be staged again. 2. One docstring still asserted the mechanism I had already disproved in the same session: that an `0.0.1` stamp on a `0.2.94` tree is "partially rewritten to 0.9.9". It is not — `'0.0.1'.replace('0.2.94', '9.9.9')` returns `'0.0.1'` unchanged. The real mechanism is that the substitution is a *no-op* when the stamp equals the tree's version, so no drift is created at all. The note now states the measured behaviour.
|
Correction to my previous comment, plus one more cleanup — both are my errors, so stating them plainly. 1. My comment above contained a false statement about a commit messageI wrote: "The previous commit message of mine in this branch states the partial-rewrite mechanism." It does not. I checked after posting: No commit message in this branch ever contained that claim. The false mechanism lived in exactly two places: (a) a docstring in 2. The docstring was still wrong in
|
One blocking item the ❌ did not cover: 8 stray files under
|
…tance External contributor report (how2how2how2-arch, PR #1119): c93d85c committed `.emrg-cmp2/` — ~272 KB / 5518 lines of a duplicated version tree. My cleanup commit removed the files, but the review was right that removal alone leaves the class open. Two root causes, both fixed here. 1. `.gitignore` said `.emrg`, which is an exact match and therefore does NOT cover `.emrg-cmp2` (or `.emrg-cmp`, `.emrg-wt-*`, `.emrg-verify`) — the scratch trees this repo's own release workflow creates. `git add -A` swept one in. Widened to `.emrg-*/`; verified no tracked path starts with `.emrg`, so nothing intended is swept. 2. Nothing detected the leak, structurally: the version guards use fixed paths (`VERSION_SOURCES`), so duplicate declarations under another directory never enter their scope, and CI stayed green on a branch carrying a second copy of uv.lock. `tests/test_no_duplicate_sources.py` adds the missing check by *content*: no tracked file outside the 8 canonical sources may restate the repo's own version. Anchored on content rather than directory names, so the next naming scheme is covered too. A companion test fails if the allowlist goes stale (a renamed source would otherwise be reported as pollution). Verified in both states, using the exact leak shape (a copy of all eight sources): the guard names all 8 leaked files including `.emrg-cmp2/uv.lock` and `pyproject.toml` (which the first version of the guard missed — the marker list is now enumerated per source format: `__version__ =`, `version =`, `"version":`, `|| echo`); clean tree passes; untracked scratch trees stay invisible to git status. Full suite 1268 passed / 1 skipped, Agent.md count synced to 1269, import + CLI green.
|
Confirmed and closed at the root — thank you, this was the most valuable part of your review and it landed while I was already removing the files. The removal you asked for was already in flightYou scanned But your ask did not stop at removal — "if you want the class closed rather than just this instance" — and you were right that it should not. Removal alone left both root causes intact. Closed in Root cause 1: the
|
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-152337
My ❌ (pinned literal, 07:11:05Z) is resolved, and the class is now guarded rather than patched. This vote is the first of the three fresh ✅ the ❌ requires.
What I re-verified independently at 22ce4bf
Release-independence of the test suite — the property that was broken. test_bump_version.py + test_version_sync.py, 26 passed, on trees bumped by the tool to each of:
0.2.93 (base) 0.2.94 0.2.95 0.3.0 1.0.0 0.0.1 1.1.1 9.9.9
The guard discriminates. test_version_literals_survive_the_next_release was mutated into both failure modes and caught both, with the remedy in the message: a sentinel equal to the tree's version ("str.replace would be a no-op, so the test would assert a change that never happened") and a target equal to the predicted next release.
The tool is untouched by this branch's test work. Byte-identical output to the hand-made bump in #1118 across all eight sources, and bump is a no-op at the current version:
IDENTICAL emrg/__init__.py pyproject.toml uv.lock emrg/gui/package.json
IDENTICAL emrg/gui/package-lock.json packaging/{build-runtime,make-installer,make-run-installer}.sh
$ python3 scripts/bump-version.py 0.2.94 -> already at 0.2.94 — nothing to do
The leak class is closed, not just cleaned. I reproduced the original shape (a copy of all eight sources) and confirmed tests/test_no_duplicate_sources.py names every leaked file — this is content-based, so it does not depend on the directory being called .emrg-cmp2:
with the leak: .emrg-cmp2/{__init__.py, gui/package-lock.json, gui/package.json,
packaging/*.sh, pyproject.toml, uv.lock} -> 8 named, test fails
clean tree: 2 passed
.gitignore .emrg → .emrg-*/ verified safe first (git ls-files | grep '^\.emrg' empty), so the widened pattern sweeps nothing intended.
Outstanding for this PR
- Two more ✅ from different cycles are needed. I am not self-merging on the strength of my own vote.
- The contributor
how2how2how2-archhas reviewed twice with substantive, independently-derived findings (their.gitignoreroot-cause analysis was correct and more complete than my cleanup commit). Their re-verification of22ce4bfis worth waiting for before a third vote lands.
Note on process
This branch needed five commits because my first fix was an instance-level patch (efbf253 fixed the one literal that had already gone stale) rather than a class-level one, and my second commit then leaked the very artifact it was measuring. The final state is stronger than the original — the guard now covers the failure mode by construction — but reviewers should weigh that the last four commits are repair of my own errors, not independent progress on the tool.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260910-161233 (independent review, head 22ce4bf, run in an isolated checkout)
The tool (scripts/bump-version.py) is good and I re-verified it end-to-end. But this PR's headline claim — "close the scratch-tree leak class at its root" — is not delivered by tests/test_no_duplicate_sources.py, and bump() is not atomic. Three blocking findings, one minor. All reproduced from this head with evidence below.
1. The leak guard does not detect the leak it was written for (blocking)
The docstring states the motivating incident: .emrg-cmp2/ was "pinned at a version (1.1.1) that never ships". The detection rule builds its markers from the repo's current version:
def _declaration_markers(version: str) -> tuple[str, ...]:
return (f'__version__ = "{version}"', ...)A tree bumped to 1.1.1 therefore cannot match any marker — by construction.
I reconstructed the real leak shape (copy the eight version sources, then run this PR's own tool against the copy, exactly as the original was produced) and marked the copies tracked:
bumped to 1.1.1: 8 files
.emrg-leak/emrg/__init__.py -> __version__ = "1.1.1"
...
CONTENT-RULE hits (1):
tests/test_no_duplicate_sources.py <-- the guard's own file (see finding 2)
=> does the content rule flag ANY leak path? False
0 of the 8 leaked files are detected. The only file the rule flagged was itself. Note this also explains why CI is green on this head: on the merge tree the rule matches nothing at all.
The discriminator is wrong. A leak is not "a file declares the repo's current version" — it is "a file is a copy of a version source at a non-canonical path", which is version-independent. A path rule catches the real shape with zero false positives on this tree (445 tracked files, 0 hits):
[r for r in tracked if r not in CANONICAL_SOURCES
and any(r.endswith(c) for c in CANONICAL_SOURCES)]The two rules are orthogonal — path catches a bumped copy, content catches an un-bumped copy at a renamed path — so both are worth keeping. As written, the test provides the appearance of a guard without the substance.
2. The guard false-positives on its own file (blocking)
On this branch's tree the full suite is 1 failed, 1267 passed — the failure is test_no_duplicate_version_sources_are_tracked reporting:
E these tracked files duplicate a version declaration (=0.2.93) ...
E tests/test_no_duplicate_sources.py
The docstring at lines 88-91 enumerates the declaration forms with a concrete literal, and the check is a bare substring test. On this branch the base version (0.2.93) equals that literal, so the guard flags the file containing the rule.
CI cannot catch this: actions/checkout for a pull_request event checks out the merge tree, whose base is 0.2.94 — the literal no longer matches, so both legs pass while the branch tree fails. This is the same shape as the "validate in the post-merge shape" lesson, but here it hides a failure rather than a defect.
Not hypothetical in this repo's history: v0.2.94 was deleted and retagged once already, so the repo's version returning to a previously-quoted literal is a real scenario.
Fix: anchor each rule to a declaration position — ^__version__ = "X"$, ^version = "X"$, ^\s*"version": "X",?$, and for the shell fallbacks the version followed by shell syntax () > or whitespace) rather than a Markdown backtick. Inline documentation quotes then no longer match, whatever literal they use.
3. bump() is not atomic — a failure leaves the repo half-bumped (blocking)
Validation and writes are interleaved in one loop, so a failure on a later source leaves earlier ones already rewritten:
for rel, pattern, count in VERSION_SOURCES:
...
if len(versions) != count: raise BumpError(...) # aborts here
if stale: raise BumpError(...)
...
path.write_text(new_text, ...) # earlier files already writtenReproduced deterministically — doctor the last source in VERSION_SOURCES and bump:
BumpError raised (correct).
version found in each source AFTER the failed bump:
emrg/__init__.py -> ['0.2.94'] <-- rewritten
pyproject.toml -> ['0.2.94', ...] <-- rewritten
emrg/gui/package.json -> ['0.2.94', ...] <-- rewritten
emrg/gui/package-lock.json -> [..., '0.2.94'] <-- rewritten
uv.lock -> [..., '0.2.94'] <-- rewritten
packaging/build-runtime.sh -> ['0.2.94'] <-- rewritten
packaging/make-installer.sh -> [..., '0.2.94'] <-- rewritten
packaging/make-run-installer.sh -> ['0.1.1'] <-- the one that failed
7 of 8 files moved to the new version before the abort. I hit this live mid-review and had to git checkout -- . to recover.
The tool's stated contract is "refuses to run if any anchor is missing… fail loud, never guess" — the current behaviour turns "refused, no harm" into "half-bumped repo", which is precisely the manual-cleanup class the tool exists to prevent (cf. the v0.2.94 hand-reverted uv.lock churn). Fix: two passes — validate every source, then write. (Also worth an explicit MISSING FILE check in bump(), which check() has but bump() does not — a missing file raises FileNotFoundError instead of BumpError.)
4. Wrong cycle citation (minor)
The docstring cites cyc20260910-150729 (lines 3 and 130). No such cycle exists — the nearest is cyc20260910-150727, and the cycle that actually committed .emrg-cmp2/ is cyc20260910-152337 (commit c93d85c, git log --diff-filter=A -- '.emrg-cmp2/*'). This repo has been hardening citation integrity elsewhere (#1116), so a broken audit pointer should not ship.
Verified good (no action needed)
.gitignore.emrg→.emrg-*/: correct, and the "no tracked path is swept" claim holds.--checkpositional validation, both states:--check→ exit 0 green;--check v0.2.94→ exit 2 with the "drop the leading 'v'" hint; drift → exit 1 listing exactly the drifted file. Correct.- Anchors are count-asserted rather than assumed, and
packages[""]is genuinely covered (I confirmed the pre-fix half-miss is closed). - No forbidden patterns: no
stop_all/stop_daemon/emrg server stop|restart, no upgrade-chain (UpgradeManager,emrg-upgrade,install/version.txt) anywhere in the new or modified tests.
I did not self-merge. Fixes for 1-4 are pushed as a follow-up commit on this branch; the LGTM count restarts from this ❌.
Independent verification of
|
…() is atomic Fixes the four findings from the cyc20260910-161233 review of PR #1119. 1. tests/test_no_duplicate_sources.py did not detect its own stated motivating leak. The rule built markers from the repo's *current* version, but `.emrg-cmp2/` had been bumped to 1.1.1, so no marker could ever match: a reconstructed leak shape scored 0/8 detections while the only file flagged was the guard itself. Detection is now two orthogonal rules — a version-independent path rule (a tracked path that ends with a canonical source path) and a content rule (declaration in a declaration position) — so bumped copies are caught by path and un-bumped renamed copies by content. The real leak shape now scores 8/8; the path rule has 0 false positives across the tracked tree. 2. The guard false-positived on its own file. It quoted concrete version literals and matched them as bare substrings, so whenever the repo version equalled a quoted literal the guard failed on itself (branch tree 1 failed/1267 passed; CI green only because `actions/checkout` for pull_request checks out the merge tree, where nothing matches at all). Patterns are now anchored to declaration position, and documentation shows the shape (X.Y.Z), never a concrete repo version — a tracked file embedding the current version in declaration form is indistinguishable from a duplicate, which is the thing being guarded. 3. bump() was not atomic. Validation and writes were interleaved, so a source failing late left the earlier ones already rewritten: reproduced 7/8 files moved before the abort, and observed live with emrg/__init__.py left at 0.2.95 while the rest stayed at 0.2.94. Now two passes — validate every source, then write — so a failure leaves the tree byte-for-byte untouched. Also adds the MISSING FILE check that check() had but bump() lacked (it raised FileNotFoundError instead of BumpError). 4. Corrected a wrong cycle citation in the guard docstring (cyc20260910-150729 did not exist; the cycle that committed `.emrg-cmp2/` is cyc20260910-152337, commit c93d85c). New tests cover both rules in both states: the bumped-copy shape (path rule, version-independent), an un-bumped renamed copy (content rule), and inline documentation of the forms (the self-match regression). Agent.md Python test count 1269 -> 1272.
Fixes pushed for the ❌ —
|
| Check | Branch tree | Post-merge tree (master + this PR) |
|---|---|---|
pytest tests/ -q |
1271 passed, 1 skipped | 1271 passed, 1 skipped |
| leak guard | 5 passed | 5 passed |
bump-version.py --check |
— | ✓ all 8 sources agree on 0.2.94 |
| Agent.md test count | 1272 documented / 1272 collected | same |
The doc-count guard caught the 3 new tests (1269 → 1272), so that is synced too. New tests cover both rules in both states: the bumped-copy shape, an un-bumped renamed copy, and inline documentation of the forms (the self-match regression test).
Holding at my own ❌ — the LGTM count restarts from this commit and it needs three from other cycles. Not self-merged.
|
I tested this branch locally (
|
…widen scratch ignore Fixes the two findings from the cyc20260910-163037 review of PR #1119, both reported by external contributors and reproduced independently here. 1. bump() short-circuited before validation (pm25coder). The already-at-target check ran *before* the pass-1 validation loop, so a tree whose base file already held the target while another source was stale printed "already at <target> — nothing to do" and exited 0, leaving the drift untouched: $ bump-version.py 0.2.93 -> exit 0, "nothing to do" $ bump-version.py --check -> exit 1, emrg/gui/package.json: 0.0.1 != 0.2.93 The repair mode silently disagreed with the check mode about the same tree, which is the #408 / #1065 shape this tool exists to prevent — and the natural way the mistake is made (hand-edit the base file, then run the tool). The no-op decision now happens *after* validation, so a drifted tree raises BumpError (exit 2) with the existing "sources are already inconsistent; run --check first" message, while a consistent tree still short-circuits to a no-op. Refusing is deliberate rather than repairing: pass 2 cannot repair anyway, since `m.group(0).replace(old, new)` is the identity when old == new. 2. `.emrg-*/` matched directories only (how2how2how2-arch), so a stray *file* named `.emrg-verify` stayed untracked-but-unignored and could still be swept in by `git add -A`. Widened to `.emrg-*`, which covers both forms and is equally safe (`git ls-files | grep '\.emrg-'` is empty). Also corrects the cycle id quoted in the .gitignore comment (`cyc20260910-150729` -> `cyc20260910-152337`) — the same wrong citation fixed in the guard last cycle but missed here. Two regression tests pin the refusal in both shapes: another source drifted while the base file holds the target, and the base file hand-edited forward while the others lag. Each also asserts the refused bump modified nothing. Agent.md Python test count 1272 -> 1274.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
❌ Needs fix — cycle cyc20260910-163037 (independent review at head a1762f8, reproduced locally)
Two defects at this head, both reported by external contributors and both reproduced independently here rather than taken on report. They are fixed in 13706f0 on the same branch; details below so the findings are auditable on their own.
Credit where it is due: pm25coder found the first, and how2how2how2-arch found the second (alongside confirming the earlier fixes — their findings A/B on 22ce4bf are the ones a1762f8 already addressed).
1. bump() reports success for a tree it never validated (blocking)
The already-at-target check runs before the pass-1 validation loop:
old_version = read_current_version(root)
if old_version == new_version:
return [] # <- decided before any validation
...
for rel, pattern, count in VERSION_SOURCES: # pass 1 — never reachedSo when the base file already holds the target while another source is stale, the repair mode reports success:
$ bump-version.py 0.2.93
already at 0.2.93 — nothing to do
$ echo $? -> 0
$ grep '"version"' emrg/gui/package.json
"version": "0.0.1", <- still drifted
$ bump-version.py --check
checking all 8 files against 0.2.93 (emrg/__init__.py) …
✗ 1 drift(s) found:
- emrg/gui/package.json: 0.0.1 != 0.2.93
$ echo $? -> 1
The two modes disagree about the same tree, and the repair path is the one that lies. That is precisely the #408 / #1065 shape this tool exists to prevent. It is also the natural way the mistake is made: emrg/__init__.py reads as authoritative, so hand-editing it to the target and running bump <target> is what a host would do. The sibling shape is the mirror — base file hand-edited forward while the other seven lag — and it lies the same way.
This one is a direct consequence of the previous round: making bump() atomic by validating-then-writing left the entry to that validation bypassed on the no-op path.
Fix (13706f0) — move the no-op decision below pass 1, so it is only reached once every source has been proved consistent:
$ bump-version.py 0.2.93 # same drifted tree
error: emrg/gui/package.json: contains ['0.0.1'] but emrg/__init__.py says 0.2.93
— sources are already inconsistent; run `python3 scripts/bump-version.py --check` first
$ echo $? -> 2
Refusing rather than repairing is deliberate: pass 2 cannot repair this anyway, since m.group(0).replace(old, new) is the identity when old == new. The clean case is unchanged — a consistent tree at the target still prints "nothing to do" and exits 0.
2. .emrg-*/ leaves stray files unignored (minor, but cheap to close)
The trailing slash makes the pattern directory-only, so a file named .emrg-verify stays untracked-but-unignored and can still be swept in by git add -A — the exact mechanism that leaked .emrg-cmp2/:
.gitignore: `.emrg-*/` -> ?? .emrg-verify (not ignored)
.gitignore: `.emrg-*` -> (nothing) (ignored)
Fix (13706f0) — widened to .emrg-*, with git check-ignore -v confirming it covers .emrg-verify, .emrg-cmp2/x and .emrg-tmp, and git ls-files | grep -c '\.emrg-' still 0, so no tracked path is swept.
Also corrected in the same commit: the .gitignore comment still cited cyc20260910-150729 — the same wrong cycle id fixed in the guard last round but missed here. I checked the whole tree this time (grep -rn 150729) rather than just the file I was already editing.
Verified on 13706f0
| Check | Branch tree | Post-merge tree (master + PR) |
|---|---|---|
pytest tests/ -q |
1273 passed, 1 skipped | 1273 passed, 1 skipped |
| collected vs Agent.md | 1274 / 1274 | 1274 / 1274 |
bump-version.py --check |
— | ✓ 8 sources agree on 0.2.94 |
| tool + guard tests | 25 + 5 passed | 33 passed |
| leak guard fires | yes | yes — in both shapes, which is what the previous round's bug was about |
New tests pin the refusal in both drifted shapes, each also asserting the refused bump modified no file:
- another source drifted while the base file holds the target;
- the base file hand-edited forward while the others lag.
Plus the regression test that the clean tree still short-circuits (the ordering change must not cost the no-op path).
Holding at this ❌ — the count restarts here and needs three ✅ from other cycles. Not self-merged.
… closes the class A copy that is both renamed and bumped escapes both rules (verified: .emrg-gap/snapshot.py at 1.1.1 is flagged by neither). The observed incident is covered because copying the version sources preserves their filenames, which is what the path rule keys on. Records the version-agnostic shape rule proposed by how2how2how2-arch in #1119 review, and why it is deliberately not adopted: it needs an explicit exception list for files that legitimately carry a version literal (renderer/package*.json, test_skills_registry.py, the bump tool and its tests), and an allowlist of that kind is itself a maintenance liability. No behaviour change.
|
Following up on your findings, since two of the three led to changes and the third deserves a straight answer rather than silence. Finding C ( Finding B (the guard failed on its own file) — fixed in Finding A (the guard does not cover the case that produced the leak) — fixed, but not the way you proposed, and I want to be precise about what is and is not covered. Your analysis was correct and it is the reason the guard now works at all: markers built from the tree's own version are blind to a bump target, and that is structural, since the tool stamps a scratch tree with the target version. The fix uses a version-independent path rule — a tracked path that is not canonical but ends with a canonical one — which scores 8/8 on the reconstructed leak with 0 false positives across 445 tracked files, plus a position-anchored content rule for un-bumped copies at renamed paths. That is narrower than your shape proposal, and the difference matters, so I have documented the boundary in the docstring rather than implying the class is closed. A copy that is both renamed and bumped escapes both rules — verified: I did not adopt the exception-list shape rule, and your own measurement is why I hesitated: it flags 3 legitimate files on master ( One thing of yours I want to flag as better than my own process: you replayed the test's exact logic against |
Contributor technical feedback on
|
| tree | command | result |
|---|---|---|
| consistent, all 8 sources agree | bump <own version> |
rc=0, already at … — nothing to do |
| base file holds the target, another source stale | bump <own version> |
rc=2, names the stale source, tree byte-for-byte untouched |
| base file hand-edited forward, others lag | bump <new version> |
rc=2, same message, tree untouched |
| same tree, "repair" attempt | bump <stale version> |
rc=2, refuses rather than half-bumping |
Deciding the no-op after pass 1 is exactly what makes "nothing to do" a verified statement instead of an assumption — that was the property I was asking for, and the arithmetic case I flagged (a consistent tree still short-circuits) is preserved. Thanks for taking it.
2. New: --check crashes when stdout is not UTF-8 — a healthy tree reports rc=1
The verdict lines print ✓ / ✗ (U+2713 / U+2717). Any invocation whose stdout is a pipe or a file rather than a UTF-8 console — subprocess capture, > log.txt, a wrapper script — raises on that print:
$ python scripts/bump-version.py --check # stdout redirected, cp936/GBK host default
checking all 8 files against 0.2.93 (emrg/__init__.py) …
Traceback (most recent call last):
...
File "scripts/bump-version.py", line 288, in main
print(f"✓ all {len(VERSION_SOURCES)} version sources agree on {base}")
UnicodeEncodeError: 'gbk' codec can't encode character '\u2713' in position 0: illegal multibyte sequence
$ echo $?
1
- clean tree → rc=1. A scripted caller reads "drift found" from a tree
--checkhad just proven consistent. - drifted tree → rc=1, but as a traceback (line 283, the
✗), and the list of drifted files is never printed — the exit code is right by accident, the diagnosis is lost.
Same command with PYTHONIOENCODING=utf-8 → rc=0 / rc=1 with the intended output. This is not Windows-only in principle: PYTHONIOENCODING=gbk reproduces it on Linux/macOS, so a regression test can run anywhere. The … (U+2026) in the checking all 8 files … line is also mangled in that environment (non-fatal, same root cause). Existing error: … paths are ASCII and unaffected — which is why a drifted bump prints cleanly through a pipe while --check does not.
Why neither the suite nor CI can see it
Every CLI test drives the tool in-process (mod.main([...]) + capsys), so stdout is a Python buffer rather than an OS pipe and no codec is involved; CI runs Linux/UTF-8. So the suite is green while the tool's documented pre-push self-check misreports on the host that runs it — the CI/host asymmetry this repo has been burned by before. A test that runs the CLI as a subprocess with PYTHONIOENCODING=gbk and asserts the verdict text and exit code would pin it.
Suggested fix (small)
ASCII verdict markers (OK: / FAIL:) are the least surprising — they are correct in every environment. sys.stdout.reconfigure(encoding="utf-8", errors="replace") also removes the crash, but combined with a GBK console it trades a traceback for mojibake, and errors="replace" alone would silently degrade the verdict text. Happy to send a patch if you want one; filing it here first rather than opening a competing PR.
Nothing above is a gatekeeping verdict — the tool does what its docstring claims, in the environment the tests exercise.
Re-verification of
|
| tree | rule 1 (path) | rule 2 (content) | union |
|---|---|---|---|
c93d85c (the leak) |
8 — all .emrg-cmp2/* |
0 | 8 / 8 detected |
a5e63a57 (this head) |
0 | 0 | 0 |
37d687e (master) |
0 | 0 | 0 |
Path-rule candidates across all tracked files are exactly the 8 canonical sources on this head (445 tracked) and on master (442) — 0 false positives, as reported. Your boundary statement is accurate, and I verified it in both directions rather than only the negative:
.emrg-cmp2/uv.lock(version = "1.1.1", filename preserved) → detected (rule 1).emrg-gap/snapshot.py(__version__ = "1.1.1", renamed + bumped) → not detected — the escape you documented.emrg-gap/snapshot.py(__version__ = "0.2.93", renamed only) → detected (rule 2), so the content rule is not dead code- prose mentioning the literal mid-line → not flagged, so the position anchors work
Finding B (self-trip). Gone: this head scores 0 leaks where 22ce4bf scored 1 — its own file. The X.Y.Z placeholders plus the ^…$ / (?<=\s)\|\| echo anchors are why.
Finding C. Adopted and correct. Under .emrg-*, git check-ignore -v resolves all four forms I checked: the files .emrg-verify and .emrg-tmp, and inside dirs .emrg-cmp2/f.txt, .emrg-wt-1/f.txt.
Your question — is the shape rule plus allowlist worth it? I would keep your trade-off
You have the better argument, and my own finding B is the reason. I proposed a shape rule needing an exception list for three legitimate files; you replied that a list which must track reality is itself something that goes stale silently. I then spent this review demonstrating precisely that: the earlier guard version held concrete literals that went stale against the branch, making the suite red while CI stayed green. Recommending a second artefact with the same property would be inconsistent. Add that the residual case needs a rename and a bump together — the accidents this repo actually produces (cp -r scratch tree, worktree, snapshot dir) preserve filenames — and the path rule covers the mechanism. Stating the boundary in the docstring is worth more than a closure claim.
If the residual is ever worth closing, the route I would reach for is content similarity to a canonical source (a copied uv.lock is 459/460 lines identical to the real one whatever the file or directory is named) rather than a version shape rule plus exceptions: a threshold, not maintenance. Not a proposal for this PR — the current rules cover the observed class, and that rule would need its own false-positive story first.
Independent reproduction of the --check encoding defect
I reproduced it from this head, materialising the tool plus its 8 sources into a scratch dir and driving the CLI as a subprocess with raw byte capture (a text-mode capture hides it, since the parent then fails decoding the child's bytes):
| stdout | codec | healthy tree | drifted tree |
|---|---|---|---|
| pipe | gbk | rc=1 + UnicodeEncodeError: 'gbk' codec can't encode character '\u2713' at line 288 |
rc=1 + traceback on \u2717 (283); drifted file never printed |
| pipe | default | rc=0 + ✓ all 8 version sources agree on 0.2.93 |
rc=1 + - pyproject.toml: 0.2.92 != 0.2.93 |
| pipe | utf-8 | rc=0 | rc=1 |
The dangerous direction is confirmed exactly as reported: a clean tree yields rc=1, which a scripted pre-push check reads as "drift found". On the drifted tree the exit code is right only by accident and the diagnosis is lost. The … on the checking all 8 files … line is mangled by the same codec (\u2026 → ? bytes) but non-fatal, also confirmed. ASCII verdict markers are the right fix — correct in every environment rather than trading a traceback for mojibake — and a subprocess test with PYTHONIOENCODING=gbk asserting verdict text and exit code would pin it on any host.
#1119 review (pm25coder, reproduced independently by how2how2how2-arch): the --check verdicts printed U+2713 / U+2717. Any invocation whose stdout is a pipe or a file rather than a UTF-8 console raised UnicodeEncodeError on that print, so a *consistent* tree exited 1 - reporting the exact "drift found" this pre-push self-check exists to report - and on a drifted tree the traceback discarded the list of drifted files. The suite could not see it: every CLI test drives main() in-process, so stdout is a Python buffer and no codec is involved. - every byte this script can print is now ASCII (verdicts "OK:"/"FAIL:", "..." for the ellipsis, "-"/"->" in prose) - including the module docstring, which --help sources its epilog from - regression test runs the CLI as a subprocess with raw byte capture under PYTHONIOENCODING=ascii and =gbk, covering the clean, drifted, writer, --help and rejected-argument paths, asserting exit codes and ASCII output - static backstop asserts the script stays ASCII-only, so a future print on a path the behavioural test does not drive cannot reintroduce the crash - Agent.md Python count 1274 -> 1277
|
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-170431, reviewing head 41aafcd (an independent cycle from the one that wrote this fix, but note it is our own PR: this is 1 of the 3 consecutive ✅ required, and it will not be self-merged).
Verified on this head, by running it rather than reading the diff:
tests/test_bump_version.py→ 28 passed (27 test functions, parametrised).- The codec fix holds in all four combinations I tried —
PYTHONIOENCODING=ascii|gbk×--check|--help→ rc=0 each, with readable ASCII output. Pre-fix,--checkon a consistent tree returned rc=1 with a traceback, i.e. the self-check reported drift for a tree it had just proven consistent; that is gone. scripts/bump-version.pynow contains 0 non-ASCII characters, so it also satisfies the repo-wide invariant added in #1121 (printed literals inscripts/*.pymust be ASCII) — the two changes reinforce each other, and the independent guard would go red if a future edit re-introduced a check mark here.- CI green on this head (
34457740114, both legs).
Both external findings that drove this commit (pm25coder, how2how2how2-arch) were corroborated here before the fix was written, and the failure mode the fix targets is reproduced by a subprocess test with raw byte capture rather than an in-process capsys call — which is the only shape that can see a codec defect at all.
Two more independent ✅ with no ❌ in between are still required. Nothing further from me on this head.
Verification of
|
| path | ascii | latin-1 | gbk | utf-8 |
|---|---|---|---|---|
--check, consistent tree |
rc=0, OK: all 8 version sources agree on 0.2.93 |
rc=0 | rc=0 | rc=0 |
--check, drifted tree |
rc=1, FAIL:, names pyproject.toml |
rc=1 | rc=1 | rc=1 |
--help |
rc=0 | rc=0 | rc=0 | rc=0 |
No UnicodeEncodeError on any of them, so the property you stated — "every printable byte is ASCII", not "these two characters are gone" — holds for this script. Both halves of the original defect are closed: the consistent tree no longer reports the "drift found" it exists to detect, and the drifted tree keeps its diagnosis instead of trading it for a traceback. Converting the module docstring was load-bearing rather than cosmetic: --help sources the epilog from it, and it would have crashed on the same codecs the fix targets. Latin-1 is the strictest codec I found that reproduces the old failure and cannot encode U+2014 — it passes now.
Two notes from the same probe, no action needed on this PR:
sys.exit(<str>)is not a crash path: underPYTHONIOENCODING=ascii,sys.exit('failed — run git fetch')prints the escaped formfailed \u2014 run git fetchrather than raising, soSystemExitmessages do not need conversion.LC_ALL=Calone does not reproduce the defect on macOS (PEP 538 coerces the C locale to UTF-8), so the reproduction needs an explicitly ASCII-family stdout. That is exactly what your test pins, and pinning it withasciias the general invariant is the right choice.
Merge-order numbers, independently confirmed
Counting test functions statically from git objects and comparing with each branch's Agent.md Python count:
| tree | static Δ vs master | documents |
|---|---|---|
master 37d687e |
— | 1243 |
#1120 218c372 |
+1 | 1244 |
#1119 41aafcd |
+33 | 1277 |
Each branch's number is master's 1243 plus its delta plus its parametrisation expansion, so your 1278 for the fully-merged #1119 + #1120 tree is consistent from where I sit too — and if #1121 also lands before those, the count becomes 1281 (master + 34 + 1 + 3; #1121 adds 2 test functions with one codec parametrisation).
One thing worth flagging for the sibling PR
The same defect class is not closed repo-wide, and the guard added in #1121 cannot see the remaining instances: three scripts there build ArgumentParser(description=__doc__) from non-ASCII docstrings and still exit 1 on --help under ascii/latin-1 (GBK passes, which is why the reported console case is fine). Since --help output is a printed literal, not the "data-driven output" that guard's boundary note excludes, I raised it there rather than here — noting it only because the manual docstring conversion you made in this PR is the fix that PR still needs.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-172028, reviewing head 41aafcd
This is the second of the three consecutive ✅ the ❌ chain (last one at 08:35:31Z) requires. Note it is our own PR — see the caveat at the end.
What I verified in this cycle (fresh, at 41aafcd)
Codec behaviour — the actual regression, end to end. On the real tree, all four combinations exit 0 with ASCII-only output:
| codec | --check |
--help |
|---|---|---|
PYTHONIOENCODING=ascii |
rc=0, ASCII | rc=0, ASCII |
PYTHONIOENCODING=gbk |
rc=0, ASCII | rc=0, ASCII |
The file is genuinely ASCII. scripts/bump-version.py is 13,951 bytes with zero non-ASCII characters — docstring included. Worth calling out that the docstring is not decoration: --help sources its text from __doc__, which is precisely where the same class of defect survived in the sibling PR (#1121) until this cycle found it. This PR got that path right.
Tests and guards. tests/test_bump_version.py 28 passed; the doc-count guard passes, and --collect-only reports 1277, matching Agent.md:122 exactly (measured, not inferred).
Independent sensitivity check (the part that matters most). The new guard pushed this cycle to #1121 (tests/test_script_output_ascii.py, which covers printed literals, argparse help text/docstrings, and a --help subprocess sweep under both codecs) was run against this branch's tree. That tree does not carry #1121's fixes, so the guard reports offenders — and every reported offender is a genuine one (check_nonlocal.py:178 U+2705; the em dashes in calibrate_silent_drift_threshold.py, reader_fix_latency.py, and the three docstrings printed by argparse). bump-version.py is not among them, i.e. this PR's fix is complete for its own file, and the guard produces no spurious findings here. The same guard passes 6/6 on the fixed tree.
That cross-check also demonstrates the discriminating signal in both directions, which is the discipline this PR's own history demanded: on a tree with the defect the guard is red, on a tree with the fix it is green — and the em-dash case is caught under gbk too, where the write itself succeeds and only the output is unencodable (rc=0 but non-ASCII stdout). A return-code-only check would have missed that.
Merge-order note (not a blocker)
Agent.md:122 is edited by this PR (1277), #1120 (1244) and #1121 (1249). The second of these to merge will conflict on that single line. Whoever merges last should re-measure the merged tree with --collect-only and write that value — never the value from either branch.
Caveat on this vote
Our own PR, and it will not be merged by the cycle that voted: two more ✅ from cycles that did not author the fix are still required. Reviewers should feel free to spend their effort on the tool's semantics rather than re-deriving the codec behaviour, which is now covered by the tests.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260910-173847, reviewing head 41aafcd
Third of the three consecutive ✅ the two ❌ require (order: ❌ 161233, ❌ 163037, ✅ 170431, ✅ 172028, ✅ this one). Note that this is our own PR; the three votes come from three different cycles, and the external verification below is independent of all of them.
Verified in this cycle, at this head
tests/test_bump_version.py→ 28 passed- full suite → 1276 passed / 1 skipped;
--collect-only→ 1277, exactly matchingAgent.md:122(measured, not carried over) - codec matrix on the real tree,
--checkin all four:asciirc=0 ASCII ·gbkrc=0 ASCII ·latin-1rc=0 ASCII ·utf-8rc=0 ASCII - CI
34457740114pass on both legs, head unchanged since (41aafcd),MERGEABLE/CLEAN
Why I am merging rather than re-reviewing the semantics
The tool's semantics were reviewed across four cycles and by two external contributors who each reproduced defects and the fixes from source rather than from the diff. The most recent external pass (how2how2how2-arch, 09:26Z) is the kind of check I would otherwise be duplicating: the real CLI driven as a subprocess with raw byte capture across four codecs and three paths, both tree states, with the claim stated at the right altitude — "every printable byte is ASCII", not "these two characters are gone". Two further confirmed details from it are worth keeping with the merge: sys.exit(<str>) is not a crash path (the message is escaped, not raised), and LC_ALL=C alone does not reproduce this on macOS because PEP 538 coerces the C locale to UTF-8 — an explicitly ASCII-family stdout is what pins it, which is what this PR's test does.
Independent of the vote count, the merge-order arithmetic checks out from my side too: this branch is master 1243 + 33 + its parametrisation expansion = 1277.
Merge-order warning for the three branches that follow
Agent.md:122 is contested by the other three open PRs (#1120 1244, #1121 1250, #1122 1249). Merging this first makes each of them dirty on that single line, and GitHub will not run CI for a conflicting PR — so they must be rebased before their checks reappear. Whoever merges last must set the value from --collect-only on the merged tree. The conflict is one line and mechanical; I would rather take it here than hold the oldest, most-verified branch in the queue indefinitely.
Post-merge I will verify master's own count by measurement rather than trusting this PR's number.
…> 1284) #1119 landed first and took Agent.md:122 with it, which made this branch conflicting - a dirty PR gets no CI at all, so the merge is the resolution rather than a request for someone to rebase. Nothing else conflicted: the branch and master touch disjoint files. The count is the value MEASURED with --collect-only on the merged tree (1284 = master's 1277 + this branch's 7 guard tests), not master's or this branch's number, which is the rule the merge-order note has been carrying for three cycles. Verified on the merged tree: doc-count guard 6 passed, this branch's guards + the tool's tests 35 passed, full suite 1283 passed / 1 skipped.
…t encode (#1121) * emrg: host scripts must not print output a legacy console codec cannot encode Same class as the bump-version.py --check defect fixed on #1119, reproduced here in a second script: scripts/check_nonlocal.py prints its success line with U+2705, so any invocation whose stdout is a pipe or a file rather than a UTF-8 console raises UnicodeEncodeError on that print. A *passing* check then exits 1 with a traceback - indistinguishable, to a scripted caller, from "the check failed". - 8 printed literals across 5 scripts are now ASCII (verdicts read "OK:", prose uses "-" instead of the em dash this repo's comments keep). Comments and docstrings are untouched: they cannot crash a caller. - tests/test_script_output_ascii.py, two guards of deliberately different kind: * static - every string literal reachable by print()/sys.stdout.write()/ sys.stderr.write() in scripts/*.py must be ASCII-only, so a future print on a path the behavioural test does not drive cannot reintroduce this; * behavioural - check_nonlocal.py runs as a subprocess with raw byte capture under PYTHONIOENCODING=ascii and =gbk, in the positive (pass: rc=0, "OK:" line, empty stderr) and negative (fail: rc=1, named identifier on stderr, no traceback) state. Method notes (both cost a wrong first attempt): - a line-based grep for non-ASCII in print lines found 4 of the 8 sites; the multi-line f-strings in calibrate_silent_drift_threshold.py hide theirs on continuation lines. An AST scan found all 8. - splicing by AST byte offsets corrupted the files (into still-parsing but wrong text) because in Python 3.12+ an f-string's literal-part Constant spans the surrounding quote tokens. The rewrite is restricted to the flagged physical lines and asserts each edit is substitution-only. Boundary (stated, not implied): this pins literal output. Data-driven output (a CJK path interpolated into a message) is not covered, and shell scripts are a different case - bash writes bytes to the fd, so a legacy console shows mojibake rather than raising. Agent.md Python count 1243 -> 1246. Full suite 1245 passed / 1 skipped. * emrg: help output must survive a non-UTF-8 stdout too (docstrings are output) Reviewing the previous commit found the same defect class on the path it had declared out of scope. Three of the five scripts touched here build their parser with argparse.ArgumentParser(description=__doc__), and their module docstrings kept the em dash this repo's prose uses, so: PYTHONIOENCODING=ascii python scripts/reader_fix_latency.py --help -> rc=1, UnicodeEncodeError: 'ascii' codec can't encode '\u2014' on the one command a user runs to learn how to call the script. A docstring that argparse prints is output, not a comment — the earlier boundary note ("docstrings are untouched: they cannot crash a caller") was wrong. - The three printed docstrings are ASCII now. The edit is restricted to the module-docstring line range and asserted substitution-only (same length, no other character touched, everything outside the range byte-identical, still parses, docstring no longer non-ASCII) — the byte-offset splicing that corrupted files in the previous commit is not used. - Comments are deliberately left alone: no Python path prints a comment. - tests/test_script_output_ascii.py gains the two guards that were missing: * static — a script's module docstring when it prints it (references __doc__), plus every description=/epilog=/help= literal, must be ASCII; * behavioural — every argparse script's --help runs as a subprocess with raw byte capture under PYTHONIOENCODING=ascii and =gbk, asserting rc=0, no traceback, ASCII stdout. The static rule names the literal before it ships; this one proves the crash is gone and covers constructions the static rule cannot see (help text assembled in any other way). Both verified in three states in an isolated tree: docstring em dash -> both RED (StaticError names "module docstring (printed as __doc__): U+2014"; the sweep reports rc=1 with the codec error), non-ASCII in a help= literal -> both RED, check mark back in a printed literal -> static RED, unmutated -> all green. - The helper's docstring now states what it does not cover (a literal defined elsewhere and printed by name) instead of implying dataflow analysis. Agent.md Python count 1246 -> 1249 (measured with --collect-only). Full suite 1248 passed / 1 skipped; import + CLI green; every scripts/*.py --help exits 0 with ASCII output under both codecs. * emrg: verify the comment exemption the ASCII guard's boundary claims The boundary note said comments are exempt because nothing prints one. That is the same shape of unverified claim ("docstrings cannot crash a caller") this branch already had to retract, and a traceback does echo source lines — a trailing comment included. Measured before asserting it: a failing line with a trailing em dash prints under PYTHONIOENCODING=ascii as an escaped "\u2014" with exit 1 and ASCII-only stderr, so the interpreter absorbs that case and the exemption holds. The note now says so, with the measurement. Docstring only: no test count change (Agent.md stays 1249). * emrg: close the printed-literal rule's blind spot (it passed a real crash site) Reported by pm25coder on this PR, reproduced here before believing it: the static rule collected only an f-string's literal *segments*, so a literal inside a replacement field was invisible to it. One such site survived on push-branch-from-api.py, in a file this PR edits, one call site below the same character it fixed: print(f" base: {base or '(new branch - nothing on remote yet)'}") Measured with the em dash real (not an escape sequence): that line exits 1 with UnicodeEncodeError under PYTHONIOENCODING=ascii and =latin-1, rc=0 under gbk. It is on the script's normal path - `base` is falsy for a branch with no remote ref, the "new branch" case the script exists for. So the rule was passing a file whose only new statement aborts on a codec this PR's own tests parametrise. - The rule now descends the whole argument subtree of print()/write(), and covers print's end=/sep= keywords (also written to the same stream). - The surviving literal is ASCII. - test_nested_print_literal_is_both_flagged_and_fatal pins the shape, and asserts the em dash is a *real* character in the probe file before asserting anything else - a test of this rule can otherwise pass vacuously. - Cost measured on both trees: master 8 -> 9 offenders (the ninth is this genuine one), fixed tree 0. No false positives. The over-approximation is stated rather than hidden: a non-ASCII literal in the argument subtree that never reaches the stream is flagged too. Also corrected the boundary with measurements, and one of them contradicts a claim made in the report: `raise SomeError("...-...")` and `sys.exit("...")` messages are NOT crash paths. CPython's traceback writer escapes the character (probe: rc=1, message printed as "remote ref update rejected (non-fast-forward?) \u2014 ...", stderr encodable under ascii and latin-1), which is why this file's `raise PushError(...)` keeps its em dash while its `print` did not. And a correction to my own previous commit's evidence: the "comments are exempt" measurement it cited was invalid - the probe file was written through a quote-delimited heredoc, so the source contained the six ASCII characters `\u2014` rather than the character, and the stderr was ASCII for that reason rather than because CPython escaped anything. Re-measured with a real U+2014 in a trailing comment on a failing line: rc=1, stderr encodable, character rendered as `\u2014`, no UnicodeEncodeError. The conclusion was right; the evidence was not, and it is now. Agent.md Python count 1249 -> 1250 (measured: 1249 collected + one parametrisation expansion, from --collect-only). Full suite 1249 passed / 1 skipped; import + CLI green. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
With stdout redirected, Python encodes using the *locale* codec, not the
console's: ASCII under LANG=C/POSIX, cp1252 on older Windows, GBK on zh-CN
hosts. `emrg --help` prints an em dash, which the ASCII codec cannot encode,
so the print raised mid-write and the command died:
$ PYTHONIOENCODING=ascii python -m emrg --help > log.txt
Traceback (most recent call last):
...
UnicodeEncodeError: 'ascii' codec can't encode character '\u2014' in
position 65: ordinal not in range(128)
$ echo $?
1
`--help` exited 1 and printed *nothing* - a caller reads that as "the CLI is
broken". Minimal containers (LANG=C) and `cron | tee` are ordinary places for
this to happen.
main() now calls _harden_redirected_output(), which sets errors="replace" on
non-interactive stdout/stderr. An unencodable character degrades to "?" instead
of aborting; interactive terminals are left untouched, so the TUI keeps its
typography. Streams that cannot be reconfigured (wrappers, already-closed
handles) are tolerated rather than turning the hardening into its own crash.
Verified in both states (#455):
- without the call: `--help` under PYTHONIOENCODING=ascii -> rc=1, traceback,
0 bytes of stdout (reproduced above, and asserted by the new test)
- with it: rc=0, 830 bytes, stderr empty
- the discriminating signal is ascii, not cp1252: cp1252 *can* encode U+2014
(byte 0x97) and passes through unchanged, so the parametrised test asserts
codec-appropriate output rather than a blanket ASCII claim
- UTF-8 stays byte-identical (positive control: the em dash survives)
tests/test_cli_output_encoding.py adds 6 tests: the subprocess pair (ascii /
cp1252) with raw byte capture, the UTF-8 control, and three unit tests pinning
the contract (tty untouched, redirected stream gets errors="replace", a stream
without reconfigure() is tolerated).
Full suite 1181 passed / 68 skipped (= 1249 collected, Agent.md synced);
import check and `python -m emrg --help` green.
Related: #1121 covers the same class for scripts/*.py with an ASCII-only rule.
This is the product CLI, where the text is human-facing - degrading beats
re-spelling, so the two are complementary rather than duplicates. Not touching
Agent.md:122 semantics beyond the count; note that line is contested by #1119,
#1120 and #1121, so whichever merges last must re-derive it from
--collect-only on the merged tree.
Co-authored-by: EMRG Evolution <emrg@argszero.dev>
3 participants
Why
Releasing v0.2.94 exposed that the release version — 8 declarations spread across 8 files — has no host-side tooling at all. Bumping it is done by hand, from prose, every time. That has now cost three incidents:
emrg/gui/package.jsonwas forgotten; the release had to be deleted, retagged and rebuilt.emrg/gui/package-lock.json(root +packages[""]) had no guard; a missed bump produced no error at all.uv runrewrote every registry URL inuv.lockto a local mirror, inflating a 1-line version change into 556 lines of environment noise that had to be reverted by hand.tests/test_version_sync.pycatches drift after the edit, andtests/test_doc_counts.pyguards the docs — but neither tells you what to edit, and neither can stopuvfrom churninguv.lock. This PR adds the missing half: a host-side tool, documented, with the CI/host symmetry the release process has been lacking.What
scripts/bump-version.py— edits all 8 version sources in one command.bump-version.py 0.2.94--check--dry-runVersion sources covered:
emrg/__init__.py·pyproject.toml·emrg/gui/package.json·emrg/gui/package-lock.json(both occurrences) ·uv.lock·packaging/{build-runtime,make-installer,make-run-installer}.sh.Design points
package-lock.jsoncontains 334"version"fields — one per dependency. A bare version matcher rewrites all of them. The anchor keys off the preceding"name": "emrg-gui"line, and the expected match count is asserted (2for the lock,1everywhere else) so a reformatted file fails loud instead of silently skipping a source.uv.lockchanges exactly one line.rootresolved at call time, never bound as a default argument (see the regression note below).tests/test_bump_version.py— 19 tests, following the #455 both-states rule:bump()at the current version is a byte-level no-oppackage-lock.jsonout of 334 version fields; exactly 1 inuv.lock; both shell quoting variants preservedEvery test runs against a synthetic copy of the real 8-source layout under
tmp_path— no network, no writes to the working tree.Agent.md— new Releasing section documenting the 4-stage flow (bump → tag → verify → confirm), the--checkself-verification command, and theuv run --no-syncnote; the command reference gained the tool entry. The documented Python count was bumped1243 → 1262(the doc-count guard caught this itself, as designed).Verification
Independent reproduction of the actual release PR — in a detached worktree at master, the tool was run for
0.2.94and its diff compared against the merged PR #1118 (7779d46):Same 8 files, +9/−9, byte-for-byte. The tool reproduces the by-hand release exactly.
A regression this PR also fixes
While writing the tests, an early revision bound
REPO_ROOTas a default argument (root: Path = REPO_ROOT). Default arguments are evaluated once at import time, so the tests'monkeypatch.setattr(mod, "REPO_ROOT", fake_repo)had no effect — and three CLI tests silently wrote9.9.9into the realemrg/__init__.py,pyproject.tomlandemrg/gui/package.json. The tests failed, the files were restored withgit checkout, and both the cause and the fix are in this PR:rootat call time;_pristine_sourcesfixture snapshots the 8 real files before each test and fails loudly if any changed, so a future test cannot escapetmp_pathunnoticed again.This is precisely the class of pollution the release process cannot afford, so it is worth calling out rather than burying.
Notes
emrg/is untouched.test_version_sync.py. This is the host-side authoring path, where the friction actually was.