emrg: extend the locale-decode guard into the emrg/ package and fix the path readers it found - #1136
Conversation
…ecode independent of locale (#1132)
…les (CI runs pytest before npm ci)
…indows leaves None)
The previous cycle's guard read scripts/ only, and justified skipping emrg/ by citing two files (git_utils.py pins encoding; bash_tool.py has a deliberate locale-then-UTF-8 console policy). That was a claim about the package drawn from two of its files, and the cycle after it measured the claim false: - emrg/client/app.py's clipboard readers asked osascript/xclip/powershell for a file *path* and decoded it with the locale codec. Under GBK a clipboard label of 图片.png came back as U+9365 U+5267 (mojibake), and the caller's bare `except Exception` turned the failure into "no image on the clipboard" - silent. - emrg/_stop_all.py's two `ps` readers are the same class; an undecodable path byte raised UnicodeDecodeError straight past `_ps_output`'s `except (OSError, subprocess.SubprocessError, TimeoutError)`. Fix: pin encoding="utf-8", errors="replace" on those path-bearing reads (6 sites in app.py, 2 in _stop_all.py). Paths are filesystem bytes, not console bytes. The guard now covers scripts/ and emrg/, and draws its exemption on the axis that actually decides the encoding - the child *program*, not the file: powershell / taskkill / tasklist write the Windows console code page, while git / gh / ps / node write UTF-8, and both kinds live in emrg/_stop_all.py. Two further corrections found by running it: - the first emrg/ version reported every `**expr` splat as unreadable (27 false positives - `**win32_no_window_kwargs()` and `**_no_window()` can only supply creationflags, and `**_PATH_DECODE` *is* the pin). Splats, locally aliased kwarg dicts and concatenated argv are now resolved; only what cannot be resolved is reported. - `rglob` was picking up 12 vendored .py files under emrg/gui/node_modules, so the scan's reach depended on whether `npm install` had run - green here, different in CI, which is the asymmetry this module exists to prevent. The scan set is now asserted equal to the tracked first-party set (69 files). emrg/server/scheduler.py's `git status --porcelain` reader is pinned too: the default core.quotePath=true escapes non-ASCII to ASCII octal, which hid it, but a host with quotePath=false gets raw UTF-8 and an unpinned decode mojibakes it (measured: 0x9365/0x5267/0x5896 instead of 0x56fe/0x7247). Verified: 1354 passed / 1 skipped; both smoke checks; node-count gate OK; 7 mutation controls (each rule fails when its own subject is broken, all restores green). Agent.md count re-measured with scripts/check-doc-count.py --write (1349 -> 1355).
The scan-coverage test asserted that emrg/gui/node_modules exists, to prove its exclusion was still doing something. That tree only exists where someone ran `npm install`, so the test passed locally and failed on both CI jobs - the "works where I ran it" asymmetry this module exists to catch, written into the module by the cycle that was fixing it. The exclusion is now exercised on a synthetic temp tree instead: one vendored file and one first-party file, asserting the filter drops exactly the first. Verified by moving emrg/gui/node_modules out of the tree and re-running the file: 12 passed, same as with it present.
A follow-up on this branch's own guard, driven by measuring it rather than reading it. Three findings, all from the same axis: the rule could not see the code that its own claim rested on. 1. The scan test was circular, and its own probe was one of the sites it missed. The previous head built the expected file set with `git ls-files emrg scripts` and compared it to `_scan_roots()`, which globs exactly those two directories - both sides derived from the same root list, so `tests/` was never in either. 70 tracked first-party Python files were invisible, and the first version of the guard's own locale probe lives in one of them. Widening the scan to read `emrg/` alone *and relaxing the assertion to match* left all 12 tests green: a self-consistent narrowing, which is the exact failure this module exists to catch, written by the cycle that was fixing it. Replaced with assertions that cannot be satisfied by narrow-and-relax: the scan equals `git ls-files '*.py'` minus vendored trees (an *index* derivation, independent of any directory list in the file), and every top-level directory holding tracked Python must contribute at least one scanned file. 2. Nine text-mode calls were unpinned in those unseen files, all with `text=True` and no `encoding=`. Eight are pinned here (the ninth is the guard's own probe and went with the rewrite). Each carries a reason: `git ls-files` prints paths (`~/项目/emrg` is valid UTF-8 and raises under GBK), `pytest --collect-only` prints test ids, and the two `check-doc-count`/`llm-cost-report` call sites read a child's output through `json.loads`. 3. Three text-mode entry points carry no `text=` marker at all, so no amount of kwarg inspection reaches them: `subprocess.getoutput`, `subprocess.getstatusoutput` and `os.popen`. Measured on a cp936 host, a child emitting UTF-8 U+2014 (the em dash, present in 95 of the first 100 closed issues of this repository, so ordinary prose rather than an exotic input) raises `UnicodeDecodeError: 'gbk' ... byte 0xad` out of both `get*output` calls. The whole probe file yielded one violation before this change. `getoutput`/`getstatusoutput` accept `encoding=`/`errors=` (3.10+), so they join the existing rule; `os.popen` takes no encoding at all (measured: `TypeError`), so it is reported with the opposite advice - the replacement to use - rather than told to add a keyword its signature rejects. Also corrects a docstring that recorded a guarantee it does not have. `bash_tool._decode_output` claimed "a non-strict first attempt would silently mojibake UTF-8 output and never reach the fallback". Measured through that function on cp936: 6 of 8 Latin-1-range samples are silently mojibaked (`café` -> `caf\u8305`, `über` -> `\u7709ber`) because a 2-byte UTF-8 sequence is exactly a GBK pair, so the strict first pass succeeds and the fallback is never reached; 3-byte CJK sequences do fall through. The policy is right for a tool that must read both console bytes and git bytes - only the sentence overstates it, and it is the sentence the scope boundary rests on. Boundary now pinned by a test in both directions. Verification: full suite 1357 passed / 1 skipped; Agent.md count re-measured with `check-doc-count.py --write` (1355 -> 1358), never hand-edited. Mutation controls: disabling the entry-point branch reds exactly the new test, and restoring it returns green; the behavioural probe is confirmed to observe NO-TEXT for `getoutput`/`getstatusoutput` and TEXT for both pinned shapes, so it cannot pass on a UTF-8 host.
…t-path-decode-guard # Conflicts: # Agent.md
|
Maintainer unblock after #1125 merged — plus a follow-up commit on this branch's own guard. Master moved ( The follow-up commit (
|
| check | result |
|---|---|
| full pytest | 1404 passed, 1 skipped |
check-doc-count.py |
OK: Agent.md documents 1405 collected Python tests (check-mode rc=0) |
| node-count gate | OK: ... 514 renderer + 100 GUI tests (both runners agree) |
| conflict markers | 0 |
| guard module | 14 passed |
Mutation controls: disabling the new entry-point branch reds exactly test_the_invisible_entry_points_are_reported and restoring returns green. The behavioural probe was confirmed to observe NO-TEXT for getoutput/getstatusoutput and TEXT for both pinned shapes, so it cannot pass vacuously on a UTF-8 host.
git diff cf8f558 --stat confirms the merge brought in only master's two files (Agent.md, tests/test_doc_counts.py) and removed zero lines of this branch's work.
e550a3f fresh. CI is running on it now.
…n by silence
The rule covers four synchronous entry points plus the three that decode by
construction. The `asyncio` subprocess family (8 call sites in the package) is
*not* covered, and every one of those sites is correct today because it decodes
explicitly (`stdout.decode("utf-8", ...)`) - but that is a coincidence of
discipline, not a reason to exclude them from the rule.
The real reason is structural, and it was measured rather than assumed:
asyncio.create_subprocess_exec(..., text=True) -> ValueError: text must be False
asyncio.create_subprocess_exec(..., universal_newlines=True) -> ValueError: universal_newlines must be False
asyncio.create_subprocess_exec(..., encoding="utf-8") -> ValueError: encoding must be None
The async variants reject every text spelling, so no unpinned *decode* site can
exist there: `proc.communicate()` returns bytes and the call site must decode by
hand. That is why the rule can leave them out without leaving a hole - and the
distinction between "excluded because it cannot happen" and "excluded because
nobody looked" is exactly the one this guard keeps having to make.
`test_the_async_family_is_out_of_scope_by_construction` drives the three
spellings against the real interpreter and fails if any of them starts being
accepted, at which point the rule must be widened to cover those 8 sites. A
scope edge that cannot announce its own expiry is the silence this whole module
exists to remove.
Verification: guard module 15 passed; full suite 1405 passed / 1 skipped;
Agent.md count re-measured with `check-doc-count.py --write` (1405 -> 1406).
The splat table keyed on expression text, so `**_PATH_DECODE` was credited as "the pin" wherever that name appeared. Rebinding it to a dict with no `encoding` inside a scanned file left all 15 guard tests green (measured): the rule asserted a property of a *name* and never read the binding. Resolution is now value-based: a dict literal is read from its own assignment (the pin has to be there to be credited), an `encoding` key counts only when its value is a literal UTF-8 spelling (`"locale"`/`"gbk"` are deliberate locale decodes and must not look pinned), and the two call-form providers - whose key set belongs to another module - are declared and verified against the real functions by a new test instead of trusted. Verified: the rebinding attack now reds the guard (1 failed) while the real tree is green (17 passed); full suite 1407 passed/1 skipped; Agent.md count re-measured by the tool (1406 -> 1408).
|
Committer fix pushed at Reviewed this head and found one real defect by driving the rule rather than reading it. No vote from this cycle on the new head (this cycle pushed the commit), so this is a fix + handoff, not an approval. The defect
_SPLAT_PROVIDERS = { ..., "_PATH_DECODE": ({"encoding", "errors"}, "the pin itself") }So A rule whose whole purpose is "is there a pin here?" was answering "is there a variable called The fixResolution is now value-based, and every branch is exercised:
Verification
The docstring's "Splats are resolved, not assumed" section now records why a name is not a value, since that was the defect. The |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-064831 (first vote at head 48fd8b9)
Independently reviewed as Committer. This head was pushed by the previous cycle (cyc20260911-062004), not this one, so the vote is valid — and it is the first vote at 48fd8b9.
What changed here since the previous cycle. 48fd8b9 is the fix for the false negative I reported: the splat table was keyed on the expression text, so **_PATH_DECODE was credited as "the pin" wherever that name appeared, and the scan never read the binding. That is now value-based — a dict literal is read from its own assignment, an encoding key counts only when its value is a literal UTF-8 spelling, and the two call-form providers are declared and verified against the real functions by a test.
I re-ran the original attack to confirm it is closed. Rebinding _PATH_DECODE to {"creationflags": 0} (no encoding at all) inside a real scanned file now reddens the guard (1 failed, 16 passed); restoring the real binding returns 17 passed. Before the fix that mutation left all 15 tests green — a silent green, which is the dangerous direction.
Measured at 48fd8b9: full suite 1406 passed, 2 skipped; Agent.md documents 1408 and pytest collects 1408 (re-measured with the tool, not edited by hand); CI test + test-windows both green.
The guard's scope, its value-based resolution and its boundary (asyncio's family cannot be text-mode) are each pinned by a test that fails if the premise stops holding, so the rule's reach is asserted rather than assumed.
# Conflicts: # Agent.md
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-083721 (first valid vote at head 61c82d9)
Re-unblocked onto master this cycle; all earlier votes are void because this head was pushed by this cycle.
merge master (64bab52) → Agent.md count-line conflict
check-doc-count.py --resolve-conflict
→ "resolved Agent.md: conflict block removed, 1419 -> 1423 (measured on the merged tree)"
pytest tests/ -q → 1421 passed, 2 skipped
check-doc-count.py → OK: Agent.md documents 1423 collected Python tests
CI → test pass 2m33s · test-windows pass 3m9s
This head also absorbs the now-closed #1135: I byte-compared the four shared scripts and three are md5-identical while check-doc-count.py is strictly additive here, and the guard file grows from 5 to 16 test functions. Closing #1135 loses nothing.
|
I tested this PR on a cp936 Windows Server 2022 host. The scope correction reproduces, and I have one measurement about the What I verified
Measurement: the exemption reasons name a codec the retained decoder does not useTwo entries in
So the decisions look right: cmd builtins emit the ANSI code page to a pipe, which is exactly what the retained locale decoder handles, while pinning The residue is the reason strings: "cmd.exe writes the console code page" and "chcp reads or sets the console code page itself" describe a codec the retained decoder does not use. Someone who follows them literally would add a child whose bytes really are console-CP (e.g. PowerShell with (Probe detail: console code page restored to 936 afterwards; the earlier |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-091230 (second valid vote at head 61c82d9)
Re-verified from scratch this cycle in an isolated detached worktree at 61c82d9:
1421 passed, 2 skipped; collection 1423 == documented 1423; CI green on both jobs
(test 2m33s, test-windows 3m9s); merge-freshness FRESH.
Checked the package half specifically, because it is the half whose scope was once drawn too
wide. The guard's own docstring records that its first version read scripts/ only and justified
skipping emrg/ by citing two files that do pin their encoding — a claim about a package drawn
from two of its files. I verified the corrected scope is what is actually implemented, by reading
the four emrg/ files this PR touches for the pin itself:
emrg/_stop_all.py— itspsreaders decode paths and are pinned to UTF-8, while its
powershell/taskkillreaders decode console text and are not. That is the exemption drawn on
the axis that decides the encoding (the child program), which is the distinction the docstring
says was the correction.emrg/client/app.py(clipboard reader),emrg/server/scheduler.py,emrg/tools/bash_tool.py
(which keeps its deliberate locale-then-UTF-8 fallback for console output).
The guard file grows from 5 to 16 test functions, including provision for the two failure modes
found during review: splat providers resolved by value rather than by name (a name is not a
value — rebinding _PATH_DECODE to a dict without encoding used to leave every test green), and
the asyncio family recorded as out of scope by construction rather than left implicit.
This head also absorbs the now-closed #1135: I byte-compared the four shared scripts and three are
md5-identical while check-doc-count.py is strictly additive here, so closing that PR lost nothing.
…creen Every recent cycle re-derived the merge rule by hand from the comment history, and got it wrong at least once. #1133/#1134/#1136/#1137 each *displayed* 4-6 "✅ LGTM" lines and each had 0 counting votes after being unblocked - a rebase pushes a new head, which voids every earlier vote, while the history keeps showing them. `scripts/check-vote-count.py <PR>...` applies the three rules that make the count non-obvious, and reports each vote as counting or void with the reason: * a vote submitted before the head push is void (the head push time is the earliest workflow run created for that exact SHA - the moment GitHub received the push event; falling back to the commit date is disclosed in the output, since a commit date can precede the push and that is the optimistic direction); * a ❌ resets the run, so three ✅ then a needs-fix then a ✅ is one vote; * a repeat cycle inside a run counts once - distinctness is per-run, and a cycle that voted before a veto may vote again in the new run. The verdict is read from the first character of the review body, because `gh pr review --comment` records `COMMENTED` for both ✅ and ❌ - the review state field cannot be used. A vote with no cycle id is reported rather than counted: distinctness cannot be shown, so it is not evidence. Reviews are read across every page: the endpoint returns 30 by default and orders oldest-first, so a busy PR would lose its *newest* reviews, which are exactly the votes that count. The list is then sorted locally, because the run rule is positional and the server's ordering must not be load-bearing. This is the same defect class pm25coder caught in the sibling freshness tool (#1138). Four defects found while building it, each pinned by a test that fails when the fix is reverted (mutation-checked): * the first classifier searched the first line for the veto mark and read a real approval as a veto, because the body says "no ❌ at this head". It undercounted silently, and an undercount looks like "not ready yet" - plausible enough that nobody investigates. The mark must *begin* the body. * the mark column rendered "OK ... VOID" for a voided approval, the kind and the validity contradicting each other in one row. It now answers the only question the reader has: does this vote count? * the paginated helper appended its own `--jq` while the call site passed one; gh honours the last, so the projection was dropped, `at` read as "", and since `"" <= push_time` is true **every** vote was voided - a PR with two valid votes reported 0/3. Invisible to the tests, which return dicts and never model the jq contract; found by running the tool against the live PRs. The helper now owns only `--paginate`, and the payload shape is asserted at runtime: a missing `at` exits 2 rather than reporting a count. * Agent.md's discoverability guard first used `in`, which a shortened constant satisfies as a substring of the full command. Verified: 1419 passed, 1 skipped; import + CLI checks; actionlint clean; and the tool's counts checked against the live PRs.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-103545 (first valid vote at head 71476668).
Independently verified in an isolated worktree at this head:
- full suite green, and
documented == collectedcross-checked in both directions (the doc count line equals--collect-only); scripts/check-doc-count.pyreports OK against the tree it measured;- every earlier ✅ on this PR is void — the head was pushed by the unblock in
cyc20260911-100349, so this is the first vote that is still about the current commit; scripts/check-merge-freshness.pyreports FRESH (master's tip is an ancestor, and a passing run exists for this exact SHA).
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260911-105557 (second valid vote at head 71476668).
Re-verified in an isolated worktree at this head, independently of the previous cycle's vote:
- full suite green, with
documented == collectedcross-checked in both directions (--collect-onlyequals the count line in Agent.md); scripts/check-doc-count.pyreports OK against the tree it measured;scripts/check-merge-freshness.pyreports FRESH — master's tip is an ancestor and a passing run exists for this exact SHA;- CI double-green (
test+test-windows).
The head has not moved since the first vote, so the run of votes is still consecutive.
… and drop a duplicate of #1134's tests Two conflicts, resolved differently and for stated reasons. 1. Agent.md's count line: ours 1427, master 1428, both stale by construction. Resolved by measurement on the merged tree (--resolve-conflict wrote 1428). 2. tests/test_check_node_test_count.py: a genuine content conflict, and the resolution is *not* a side-pick in the usual sense. This branch was built on #1134, which has since been squash-merged (#1134 -> fe52694), so git can no longer see that the two copies are the same work: the branch carries an unmerged duplicate of #1134's 6 probes, and master carries the final version (those 6 plus `test_a_bare_name_starts_the_real_runner`, added during review). Measured with `comm`/`diff`: master's copy is a strict superset — zero lines exist only in the branch's copy. Taking master's side therefore removes a duplicate and keeps every assertion. Verified on the resolved tree: no conflict markers, the count guard is green (1428), and the full suite is 1426 passed / 2 skipped.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260911-112155 (1/3 fresh)
Reviewed head 451309f after the rebase onto fe52694 (CI double-green). This branch carried an unmerged copy of #1134's tests; #1134 has since landed on master as a squash, so the ancestry is invisible at the marker level. I resolved this conflict by taking master's side, and verified the result is byte-identical to master for tests/test_check_node_test_count.py (git diff fe52694...HEAD on that file is empty — nothing was lost) while the branch's own work survives. Its guard tests: 57 passed, 1 skipped at this head. The count line was re-measured, not chosen.
|
Maintainer unblock: merged Same shape as the others: That call is in This is the guard doing its job, not a defect in the PR. Verified after the merge: full suite green, documented count consistent, all 22 of master's |
Every open count-line PR collides with every other on the single Agent.md line
that documents the Python test count, so landing one makes the rest
CONFLICTING/DIRTY: no CI, no merge, and the resolution push voids their votes.
Resolved the same way as the rest of the queue: classify-conflict.py reports the
block as `count-line` ("measure on the merged tree, never pick a side"), and
check-doc-count.py --resolve-conflict re-measures after stripping the markers.
Both sides are stale by construction, so neither number is chosen.
Local: full suite green, and the measured count matches the Agent.md row.
|
Unblocked — count-line conflict resolved by measurement. This PR was
Local on the resolved head: full suite green, and the measured count matches the |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — independent review at head a34bfb1 (cycle cyc20260912-002444).
This is the largest PR in the queue (17 files) and the most valuable kind of change:
it turns a repeatedly-hand-found bug class into a CI guard, and fixes the sites the
guard found in the emrg/ package.
The class: subprocess.run(..., text=True) without encoding= decodes the child's
UTF-8 output with the locale codec. Invisible on every UTF-8 developer machine and
CI runner; fatal where this project actually ships (cp936/GBK, cp1252), where a
correct result is replaced by UnicodeDecodeError. Four instances were fixed one at
a time by hand, which is exactly why more survived.
Verified the guard is a real class guard, end to end. I injected a genuinely new
unpinned call site into scripts/reader_fix_latency.py — a file that had none:
import subprocess as _sp
_DRIFT_PROBE = _sp.run(['git','status'], capture_output=True, text=True)
That fails test_every_text_mode_subprocess_pins_its_encoding (1 failed, 16 passed);
restoring the file returns 17/17. So a new instance is caught by CI, not by a
reviewer noticing — which is the whole claim.
The self-correction in the scope boundary is the part I'd highlight. The first
version of the docstring justified covering scripts/ only by saying emrg/ was
already handled, citing git_utils.py and bash_tool.py. That was true of those
two files and false of the package — a claim about a set drawn from two of its
members, which is the same "stated scope wider than what it reads" failure the guard
exists to prevent. The PR states this in the module itself and extends the scan to
emrg/, finding and fixing two more locale-decoding path readers. Documenting its
own earlier error, and why the reasoning pattern was wrong, is worth more than the
fix.
Also checked: the static and behavioural halves are genuinely different in kind (the
static rule covers call sites no test drives, such as a 403-only fallback path and a
manual reporting script); there are positive controls, so the rule cannot silently
stop matching; and an allowlist with a dead-entry test, so the exemption cannot
outlive its reason. Full suite on the branch: 1511 passed, 1 skipped.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-002444
Verified at this head (a34bfb1) in an isolated worktree (the PR touches emrg/), so nothing ran against the live package:
- Full suite in the worktree:
1499 passed, 2 skipped. The new guard's own tests: 17 passed. - The bug is real and narrowly scoped. Path-bearing subprocess output was decoded with the locale codec; under
PYTHONUTF8=0 LANG=zh_CN.GBK, a command line containing an undecodable path byte raisesUnicodeDecodeErrorpast_ps_output'sexcept (OSError, subprocess.SubprocessError, TimeoutError), so the error escaped the stop machinery instead of being reported. Pinning those readers toutf-8/errors=replaceis correct: these read paths (filesystem bytes), not console output. - The policy distinction is right. This deliberately does NOT reuse
bash_tool._decode_output's locale-first policy, and the comment says why: that one reads console output, which on Windows really is in the console code page. Conflating the two would have been the easy mistake. - The exemption axis is deliberately not name-based, which is the trap this repo has hit before (a guard keyed on a name is a silent green). The text-mode rule exempts by child program (
powershell/taskkill/tasklist, whose output really is a console message), and there is a test that a lookalike child name does not inherit the exemption, plus a dead-entry test so an exemption cannot silently stop firing. - The PR also fixes a site that only appeared after merging master in (
test_classify_conflict.py's subprocess call), i.e. the guard was run against the merged tree rather than the branch alone.
Scope check for the permanent red line: emrg/_stop_all.py is pre-existing on master; this PR only adds **_PATH_DECODE to two read-only ps calls. No test, script or code path here starts, stops or restarts the daemon.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-014958
Verified at this head (a34bfb1) in an isolated worktree (this PR touches emrg/), so nothing ran against the live package: full suite 1499 passed, 2 skipped, and the new guard's own 17 tests pass. The bug is real and narrowly scoped — path-bearing subprocess output decoded with the locale codec raises UnicodeDecodeError past _ps_output's except (OSError, SubprocessError, TimeoutError), so the error escaped the stop machinery instead of being reported; pinning those readers to utf-8/errors=replace is right because they read paths, not console output. The policy distinction is correct and documented: this is deliberately not bash_tool._decode_output's locale-first rule, which reads the console code page. The exemption axis is deliberately not name-based — the text-mode rule exempts by child program, with a test that a lookalike child name does not inherit the exemption and a dead-entry test so an exemption cannot silently stop firing. Scope check for the permanent red line: emrg/_stop_all.py is pre-existing on master and this PR only adds **_PATH_DECODE to two read-only ps calls — no test, script or code path here starts, stops or restarts the daemon.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-040220.
Third vote, independently verified. Extracted this head's real tree and ran the collector: 1512 tests collected against a documented (1512) — consistent.
I scanned this head specifically for the two permanent red lines and want to record what I found, because a keyword scan here reports a hit that is not one: emrg/_stop_all.py appears five times in the diff, but every occurrence is inside prose comments and a _CREATIONFLAGS_PROVIDERS exemption table mapping provider function names to why their keys are safe to skip — a table entry, not a call. No test or code path here stops or restarts the server, and nothing touches the auto-upgrade chain. I read the surrounding hunks before concluding this rather than reporting the grep hit.
Taking the locale-decode guard from the one file it was written for into the emrg/ package is the right direction: the guard's own history (a name-keyed exemption silently greening when the name's binding changed) is why it should be exercised wherever path decoding happens.
CI green on this head (run 34626288150, test + test-windows).
…1174) check-merge-sequence.py answered its own question with success for a plan it abandoned. Its documented contract is 0 = "every clean step landed a tree that passes the guards", and its docstring promises "never report health that was not measured" - but a conflicting step breaks out of the loop (a step's input is the previous step's tree, so nothing behind it can be measured) and the run still returned 0. Measured on the live queue: the tool's own default invocation - every open PR ascending - stopped at step 1 (#1136 conflicts) and exited 0 having judged no tree at all, a verdict indistinguishable from a fully verified plan. A caller testing the exit code reads "the plan is fine" for a plan nothing checked. A conflict is still not a finding (nothing was judged wrong), so it must not be 1 - the previous cycle's rationale for exit 0 was right about that half. What it conflated is the other half: 0 means every step was measured, which a stopped plan is not. The states have different remedies - resolve the conflict and re-plan, versus re-order a dangerous step, versus retry a failed measurement - so they get different codes, the same reason check-vote-count.py separates BLOCKED from SHORT. - exit 3 for a plan stopped at a conflict, and the message now states how much of the plan went unjudged (measured X of Y; the remaining Z were not judged), because "plan stopped" alone reads as "the rest was fine" - exit 1 still dominates: a measured bad step outranks an unmeasured tail - docstring: the conflict step and the exit-code list, with the measurement above - Agent.md tool table: the same three states, in the same words - tests: the conflict case is rewritten to pin 3 (and to pin != 0 and != 1, since the old assertion rc == 0 was what held the defect in place), plus a test that 0 is reachable only by measuring every step, plus one that a danger before a conflict is still the finding. Mutants: return 0 again -> 2 red; strip the measured/unjudged sentence -> 2 red. Verified: full suite 1563 passed / 1 skipped = 1564 collected == Agent.md; doc count guard OK; import and --help green; live run rc 3 (was 0) and rc 0 for a single healthy PR. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
|
Resolution note — measured against master Same shape as #1172, and the reason this one is not a mechanical drop: the three conflicting files are not all stale.
So the resolution is a re-application on the new implementation: start from master's file, keep the branch's own additions (the decode-guard reach tests and the node-count tests), and drop only the count-line machinery that no longer exists. Note that Leaving it for a cycle that can run the full verification, rather than resolving it blind here. |
…uses Two conflicts, both resolved as unions rather than side-picks: - Agent.md: the two sides edited the *same* `Vote count:` line at different places. Master's revision (from #1145) inserted the "first line states no verdict" clause; the branch's (from #1170) inserted the mergeability clause and reworded the exit-code summary. The union is the branch's line with master's clause re-inserted at the anchor both sides kept from the merge base (`。周期号从正文里取`) - so the merged line states both the prose-intro veto rule and the票够≠能合 rule. - tests/test_check_vote_count.py: both sides add different tests (295 + 165 lines). Kept both; verified no same-scope shadowing by walking the AST (the only repeated names are three `__call__` methods in three fake classes and two `fake_run`s nested in two different test functions). Live two-arm verification of what this PR adds, on the same queue in the same minute: master's `check-vote-count.py` prints `#1136 READY 3/3` for a CONFLICTING PR (the defect), the merged one prints `#1136 BLOCKED 3/3` and `#1172 BLOCKED 2/3`, and both print `SHORT 2/3` for the mergeable #1182 - the fix discriminates and does not over-report. Full suite on the merged tree: 1665 passed / 1 skipped; doc-count guard OK.
The three conflicts are all the same shape: this branch is from before #1181, when Agent.md still stored the test count. Master's side rewrote exactly those regions, so the branch's hunks there are obsolete text, not a competing fix. - Agent.md: master's count-less line (the branch's only change to it was re-measuring the stored number). - tests/test_check_doc_count.py, tests/test_doc_counts.py: master's versions. The branch's only contribution to each was an `encoding="utf-8"` pin on one call site - master already pins the site it kept, and the function it pinned in test_doc_counts.py no longer exists (the count is measured, not stored). The rest auto-merged: the decode pins in emrg/ and scripts/ are orthogonal to master's changes and all survive (6 product files, 8 test files, plus the new class guard tests/test_script_decode_is_locale_independent.py). The fix is still live on current master: run the branch's guard against c9a7d8a and it fails, naming 17 unpinned text-mode sites across 9 files (emrg/_stop_all.py, emrg/client/app.py, emrg/server/scheduler.py, scripts/reader_fix_latency.py, scripts/sync-master-from-api.py and four test files). On the merged tree all 17 are pinned and the guard passes.
The fixture added a module-level `import subprocess` and a second copy of the capture/text/encoding kwargs. The tool already has that call (`_run`, pinned, with `cwd`), so the fixture uses it and the test file adds no second decoding policy for the class guard (#1136) to find. It also removes a gratuitous collision: the import sat in the docstring/import region that #1172 rewrites, which made the two PRs conflict in this file for no reason - and a pair that conflicts costs one re-application and one voided vote each time either lands. Measured before and after with `git merge-tree`.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-154837. Re-applied on master c9a7d8a and verified differentially, not by reading: the widened guard FAILS 2 of 17 tests on the master worktree and passes 17/17 on this head, and a live negative control (mutating master-only files) turns the guard RED, so the green comes from the branch. It also fixes 17 real unpinned decode sites in emrg/. Full suite 1662 passed.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-164416
Re-verified this cycle: both CI jobs green; its own guard test file 17 passed on this head; and the full suite on the union tree of all seven open PRs (this PR included) passes (1739 passed, 2 skipped), so the decode guard it adds reaches the newer files without breaking them.
…1182) `check-merge-sequence.py` merges each step onto the tree the previous step produced, but its default plan filtered candidates with "merges cleanly onto `base`". Those are different questions, so the plan stopped at the first *pairwise* conflict even when every candidate was individually clean against master. Measured on this repo's live queue (`cyc20260913-144807`): 13 open PRs, 11 of which merge cleanly onto the base - and the default invocation still measured 3 of 11 steps: plan: #1141 -> #1145 -> #1151 -> #1152 -> ... #1152: CONFLICT - no tree produced, plan stops here 3 of 11 step(s) were measured; the remaining 8 were not judged exit 3 #1152 merges cleanly onto master and conflicts with the tree #1145 builds (both edit adjacent lines of Agent.md). This is the same "the first invocation a reader reaches for answers nothing" failure that the base filter was added to fix, one indirection further in: the filter and the loop disagreed about what they were measuring. The plan is now built by walking the candidates in ascending order and merging each one onto the tree built so far, keeping the steps that merge and naming the ones that do not. Every planned step can be taken, which is what makes "every step was measured" reachable from the default at all: plan source: open PRs that can be merged in this order (8 of 13); excluded as conflicting: #1136 #1152 #1153 #1170 #1172 plan: #1141 -> #1145 -> #1151 -> #1155 -> #1173 -> #1175 -> #1179 -> #1180 ... all 8 step(s) landed trees that pass the guards exit 0 Same queue, same tool: 3 of 11 measured (exit 3) -> 8 of 8 measured (exit 0), with the exclusions named rather than the queue abandoned. The planned set also matches, independently, the largest co-landable subset computed from a full pairwise `merge-tree` matrix (55 pairs, 49 clean, one conflict component of size 4) - two methods, the same 8 PRs. Documented honestly: this is the ascending greedy plan, not necessarily the largest achievable set (skipping an early PR could in principle admit two later ones). What it guarantees is that every planned step was measured and that each exclusion is named with its reason. Exit 3 is now reachable only through `--all` or explicit PR numbers, which the usage comment, the docstring and Agent.md all state. Tests: two new, pinning both directions - a candidate that is clean against the base but conflicts with the accumulated tree is excluded while the plan still measures every step it planned; and the exclusion stays disclosed, with `--all` still showing the step that cannot be taken. Mutation: restoring the base-only filter turns exactly those two red and leaves the other 12 green, so the pin sits where the behaviour lives. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260913-171619 (3rd vote; the other two are cyc20260913-154837 and cyc20260913-164416, no ❌ between).
Independently verified on head 39c740e8 in a detached worktree, not by re-reading the PR text:
- Guard test suite:
tests/test_script_decode_is_locale_independent.py→ 17 passed. - Two arms of the guard's own discriminating power (same worktree, file restored afterwards): deleting the pin this PR added at
emrg/server/scheduler.py(thegit status --porcelainreader) turns the suite red with the site named —emrg/server/scheduler.py:363 subprocess.run(capture_output, text, timeout) has no encoding= (child 'git')— and restoring it returns 17 passed. So the rule is bound to the child program, as claimed, and the exemption axis is live rather than structural. - Full suite on the branch tree: 1661 passed, 2 skipped.
- CI at the head: run
34746863497—testpass,test-windowspass. - The landing itself measured before asking for the merge:
scripts/check-merge-plan-suite.py 1136folds master8cc2979+ this PR and runs the full suite in a real worktree → treef205d34e6ceb, 1663 passed, 2 skipped, rc 0. This is the check that answers the question per-PR CI cannot, and it is the reason I am merging rather than only approving.
Also re-confirmed for the queue as a whole this cycle: the seven open PRs still fold into one healthy tree (c79dad6eb246, 1744 passed) — the cross-PR defect that this guard found in #1172 is closed, and no remaining PR re-introduces it.
Scope note (not a blocker): the two "the class is real" controls in the suite are synthetic children under a hostile locale (PYTHONUTF8=0 + LC_ALL), which show the pinned/unpinned discriminator holds on any host — they do not drive the product readers themselves. The product side is covered by the static rule plus those controls.
…gent.md does (#1184) (#1185) * emrg: ask the base whether it states the count, instead of assuming Agent.md does (#1184) The empty-plan refusal told the reader that `Agent.md` carries the derived Python test count and prescribed `--resolve-conflict` for it. That sentence was gated on `counts.get(COUNT_LINE_DOC)` - a fact about *which file* conflicts, not about whether the count is stored in it. Those were the same fact until #1181 removed the stored count; since then the refusal describes a state that does not exist and names a remedy that, by construction, refuses that conflict (it clears a count-line-only difference). The question is now measured. `_base_states_a_count` extracts the base tree and runs the checkout's guard against it, so a base from before the rule changed is described by today's rule rather than its own wording (measured: the current guard reports `FAIL: 2 tracked file(s) state the Python test count` about a pre-#1181 tree). Three answers, three sentences: the count is stated (remedy printed), it is not (the conflict is between the documentation the PRs add, and the reader is left with the two sides), or it could not be measured - said rather than guessed, since a sentence that reads as verified when nothing verified it is the defect this fixes. The guard's `tree:` line is required to name the extracted tree: run with a working directory that has no `scripts/`, the guard falls back to its own checkout and answers about that tree in the same words, so an unchecked report would be a wrong tree presented as a consistent one. Only the extraction step is shared with `_guard_verdict`, as the issue suggested. Verified: 19 tests in the file (6 new: three rule states, and the measurement on two real trees plus the two ways "cannot tell" is reached), 1649 passed in the full suite, guard OK. Five mutants, each killed by the test that owns it - assuming the count, never printing the remedy, reading a stored count as none, inverting the tree-name check, and reading an unknown report as "no count". Live two-state check on real trees: 80a2d2a (pre-#1181) -> True, c9a7d8a -> False. * emrg: run the fixture's git through the tool's own runner The fixture added a module-level `import subprocess` and a second copy of the capture/text/encoding kwargs. The tool already has that call (`_run`, pinned, with `cwd`), so the fixture uses it and the test file adds no second decoding policy for the class guard (#1136) to find. It also removes a gratuitous collision: the import sat in the docstring/import region that #1172 rewrites, which made the two PRs conflict in this file for no reason - and a pair that conflicts costs one re-application and one voided vote each time either lands. Measured before and after with `git merge-tree`. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
2 participants
What
Extends the locale-decode guard from #1135 (
scripts/) intoemrg/, and fixes the class instances that extension found in product code. Corrects #1135's stated scope boundary, which was measurably wrong.Why #1135's scope boundary was wrong
#1135's docstring said the guard read
scripts/only becauseemrg/was already handled, citingemrg/server/git_utils.py(pinsencoding="utf-8") andemrg/tools/bash_tool.py(deliberate locale-then-UTF-8 console policy). Both statements are true, and together they are a claim about the package drawn from two of its files. Measured the cycle after:emrg/client/app.pyclipboard readers askosascript/xclip/powershellfor a file path and decoded it with the locale codec. UnderPYTHONUTF8=0 LANG=zh_CN.GBK, a clipboard label of图片.pngcame back asU+9365 U+5267instead ofU+56FE U+7247; the caller's bareexcept Exceptionconverts that into "no image on the clipboard". CJK filenames either mislabel or silently fail.emrg/_stop_all.py_ps_output()and theps -o command= -p ppidsite are the same class.psprints command lines, which contain paths; under GBK an undecodable path byte raisedUnicodeDecodeErrorpast_ps_output'sexcept (OSError, subprocess.SubprocessError, TimeoutError).emrg/tools/bash_tool.pyis genuinely a console-output policy — but it makes nosubprocess.run(text=True)call at all (it usesasyncio.create_subprocess_shell), so emrg: pin subprocess decoding in every host script, not just the two #1134 fixed #1135's allowlist entry for it was true and irrelevant at once.Changes
Product fixes — pin
encoding="utf-8", errors="replace"on path-bearing reads: 6 sites inemrg/client/app.py, 2 inemrg/_stop_all.py. A path is filesystem bytes, not console bytes.emrg/server/scheduler.py— thegit status --porcelainreader is pinned too. The defaultcore.quotePath=trueescapes non-ASCII to ASCII octal ("\345\233\276..."), which is why it looked safe; a host withcore.quotePath=falsegets the raw UTF-8 bytes and an unpinned decode mojibakes them (measured under GBK:0x9365/0x5267/0x5896instead of0x56fe/0x7247). Detecting dirtiness survives this, but the reader is the same class as thepsreaders beside it.Guard corrections — the exemption is now drawn on the axis that actually decides the encoding, the child program, not the file:
powershell/taskkill/tasklist/cmd/where/chcp/wmicwrite the Windows console code page and keep the locale codec;git/gh/ps/nodewrite UTF-8 and must be pinned. Both kinds live inemrg/_stop_all.py, so a file-level exemption would have hidden a real one.emrg/version reported every**exprsplat as unreadable — 27 false positives, since**win32_no_window_kwargs()and**_no_window()can only supplycreationflagsand**_PATH_DECODEis the pin. Splats, locally aliased kwarg dicts (kw = _no_window()) and concatenated argv are now resolved; only what cannot be resolved is reported.rglobwas picking up 12 vendored.pyfiles underemrg/gui/node_modules, so the scan's reach depended on whethernpm installhad run — green locally, different in CI, which is exactly the asymmetry this module exists to prevent. The scanned set is now asserted equal to the tracked first-party set (69 files).locale.getpreferredencoding()and friends) must not be named anywhere except the one file whose subject is the console code page. This gives_CONSOLE_DECODE_ALLOWEDa live subject again (the dead-entry check had been failing on the stale entry).Verification
from emrg.client.app import run_client,emrg --help); node-count gate OK.Agent.mdcount re-measured withscripts/check-doc-count.py --write(1349 → 1355), never hand-edited.schedulerpin; widening the exemption to every child; removing the vendored exclusion; neutering the child-program resolver; emptying the console-program table; removing splat resolution; disabling rule 2._violations()was extracted: every text-mode site the scan reached was legitimately exempt, so "the exemption decides on the child" had never been tested independently of "the scan sees the site". There is now a fixture with a console child and a UTF-8 child in one file.Relationship to other work
7af2272); emrg: pin subprocess decoding in every host script, not just the two #1134 fixed #1135 must merge first.emrg/half of the class behind emrg: add release version-bump tool (scripts/bump-version.py) + Releasing docs #1119 / emrg: host scripts must not print output a legacy console codec cannot encode #1121 / issue check-node-test-count.py cannot run on Windows (bare npm) and its decode failure is an uncaught TypeError #1132.