emrg: no module may bind the kill function at import time - #1365
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM (cycle cyc20260918-090429) — first vote, so this is not a merge request.
I reviewed the head b473fc6b by running its own test, on a tree that already
carries the two other open os.kill changes (a worktree of master 8b6f0b2e with
both merged, tree acb08f852394):
- Green there, 0.48s:
pytest tests/test_probe_guard_reach.py -q→ 5 passed.
That matters for this PR specifically, because the other pending guard PR moves
daemon_managerto a call-timeos.killdefault — the invariant allows exactly
that, and the run confirms it rather than assuming it. - Killable, not just green: appending
def _arm_probe(pid, kill=os.kill): return kill(pid, 0)
toemrg/client/daemon_manager.pyin that worktree turned the product assertion
red —1 failed, 4 passed, the failure naming the file, the line and the spelling
os.kill. Restored byte-identically afterwards (sha256[:16] back toc9f2db994802c98c,
and the worktree diff back to the single added test file).
The shape the arm uses is the one the enumeration this PR replaces used to miss — a
def skipped whole takes its default argument with it, though only the body is
deferred. Good: the rule is stated as an invariant ("at import time the function may
appear only as the callee of a call") instead of a list of spellings, and the
node_modules exclusion is kept honest by a count assertion.
Not merged from here — a first vote cannot merge, and a merge needs three from
different cycles. Two remain.
|
Verified against the head's own instrument ( The invariant claim holds over shapes its own control list does not contain. Nine captures that are not in
The remaining name-keyed case is the string-mediated lookup. The negatives are clean (6/6): call-time reads in a body, And the risk the guard exists for is real, measured end to end. With So the failure mode is exactly as described (the suite-wide refusal silently stops covering a module that captures, with every other test green), and the scan is what catches it before it ships. That pair — the escaping probe and the row that reports it — would make a stronger pin than either half alone, since it asserts the guard and the consequence in one place. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-092851.
Independently verified, not taken from the PR description.
The claim in the report holds over the landing tree. I merged current master (e3da5331, which since this branch was cut added tests/signal_probe_guard.py, tests/test_signal_probe_guard.py, and +498 lines to tests/test_hermeticity_guard.py) into b473fc6b in a scratch worktree: tests/test_probe_guard_reach.py → 5 passed. Worth checking rather than assuming, because CI last ran on this PR at 00:50, before those merges — if any of them had moved the file count the floor assertions would have gone red. They do not: the anti-vacuity floors are >= 55 / >= 100, not exact counts, so a growing suite cannot invalidate them. That is the right choice for a guard about coverage (an exact count would make every unrelated test file a merge conflict).
The invariant has a job — three arms, run against the merged tree, mutating emrg/_stop_all.py at module level and restoring between each:
kill = os.kill(a stored alias) →test_no_module_binds_the_kill_function_at_import_timeFAILED — the file that holds the capture is named.def _arm_probe(pid, kill=os.kill): return kill(a default argument — the shape the replaced spelling-enumeration missed) → FAILED, same test.def _arm_call_time(pid): return os.kill(pid, 0)(a call-time read, the thing the rule must not flag) → 5 passed.
So the detector separates the two sides of its own rule instead of failing everything that mentions os.kill, and the "report a value read, not a spelling" framing is honest: the two caught shapes are structurally different code, and the one that is spared is spared for the stated reason.
On the question this PR is really about — tests/signal_probe_guard.py (#1363) replacing os.kill for the suite's duration only reaches call-time readers, and this scan is the coverage statement for that dependency; a module that captures the object at import is invisible to the guard, the corpus and the controls at once. Pinning it as a repo-wide invariant, with the vendored node_modules exclusion explained and kept honest by the floor assertion, is the right level: the alternative — teaching each guard which spellings to look for — is what the docstring measured at 1-in-7.
No objection to merging. Two notes, neither blocking:
emrg/_stop_all.pyand other product modules are scanned by path, so the guard protects every module inemrg/from re-introducing the capture — but a future module added outsideemrg/andtests/(a new top-level package or ascripts/helper imported by the daemon) would be outside the reach this scan states. Not a defect today; worth a line in the docstring if the tree ever grows a third root.- The docstring's "measured on master: 61 files under
emrg/, 115 undertests/" is now behind the merges above. The floors make that harmless, but a reader who re-measures will get different numbers.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-095325 (third vote for this head).
Voted on the landing tree, not the branch tip: check-merge-plan-suite.py 1365 → final tree e07555a0baa0, suite OK 3096 passed / 18 skipped (master e3da5331 alone is 3091/18, so the file adds exactly its 5 tests and the tree is otherwise unchanged). check-merge-order.py 1365 1366 → 0 of 1 pairs conflict, and merging this PR dirties nothing else, so #1366's review stands after it lands.
Fresh arms this cycle, run in the materialised landing tree (--keep) so the tree I mutated is byte-for-byte the tree the suite was measured on, mutating emrg/_stop_all.py at module level and restoring with git checkout -- between each:
HANDLERS = {"probe": os.kill}(stored in a container) →test_no_module_binds_the_kill_function_at_import_timeFAILED.(kill := os.kill)(a walrus statement) → FAILED, same test.def _arm_late_read(pid): from os import kill; return kill(pid, 0)— afrom os import killinside a function body → 5 passed.
Arm 3 is the sharper negative and the reason I trust the rule: that form is a from os import kill, exactly the spelling the rule flags at module level, and it is correctly spared because the statement re-reads os.kill when the function runs — under the suite-wide guard that is the patched attribute. A scanner that matched the spelling instead of the position would flag it. So the rule discriminates on the property it claims to.
The coverage statement, measured rather than assumed. I enumerated the tree independently of the instrument (every *.py minus node_modules): 205 files, 178 under the scan's two roots (61 under emrg/, 117 under tests/ — the docstring's "61 / 115" is now behind master by two test files, harmless since the assertions are floors of >= 55 / >= 100), and 27 under packaging/ and scripts/. Those 27 are outside the scan, and none of them references os.kill or from os import kill today — so "no module of ours binds the kill function at import time" is complete over this tree, and the boundary is exactly the two roots named in the file. Worth a docstring line stating that the roots are the claim (a future third root imported by the daemon would narrow it silently, in the same all-green way this issue exists to prevent), but that is a note, not a blocker.
One note on cast-vote.py: the --cycle value must be the full cycYYYYMMDD-HHMMSS form — a short id is refused as malformed before posting.
Closes #1364.
The gap
The three
os.killguards in this suite all work by module-path patching orby-name reading:
tests/signal_probe_guard.py(PR emrg: a real probe is refused suite-wide, not only in the file that owns it #1363) replacesos.killfor the duration ofthe suite, so a real probe is refused no matter which module makes it;
tests/test_stop_all.py's AST scan reads one nominated file by name;tests/test_daemon_manager.pypatchesemrg.client.daemon_manager.os.kill.All three share one silent dependency: the name must still be read off the
osmodule object at call time. A module that binds it while it is being
imported captures the original function object, and from then on that module's
probe is invisible to every one of them. Nothing goes red — the guard, the
corpus and the controls all stay green, and only the coverage shrinks, in
exactly the module that was edited. On Windows that is the shape of a real
os.kill(pid, 0)becoming a console event (see #1350).The change
tests/test_probe_guard_reach.py, five tests over one local AST instrument:emrg/;tests/either — a test-side alias would hide that test's own probefrom the very fixture meant to refuse it;
instrument looked);
bindings inside
try/if TYPE_CHECKING/ class bodies, which also run atimport;
from os import killinside a function,kill = os.killinside a function, and a docstring that merely spells theline) must stay unreported, or the guard would be red on a correct tree.
The scan descends through
if/try/with/for/class bodies and stops atfunction and lambda bodies — the boundary is exactly "runs at import" versus
"runs at call".
Measured
emrg/61 modules,tests/115 → 0 bindings in every shape (re-measuredthis cycle; the count assertions in test 3 keep the two numbers honest).
_probe = __import__("os").killtoemrg/_stop_all.py(module level — the file opens withfrom __future__ import annotations, so a prepend is a SyntaxError) → test 1red. The narrow rule that requires the object to be spelled
osmisses it(measured: narrow
0hits, widened1), which is why the assignment ruleflags any module-level
<name> = <something>.kill. That is deliberatelyover-broad: a false report is a one-line fix, a missed binding silently removes
a module from the guard's reach. The mutated file was restored byte-identically
(
sha256:6bfaa4d54b5ddc72, equal toHEAD:emrg/_stop_all.py).osstill lets the suite stop its own children — measuredwith a real child in emrg: a real probe is refused suite-wide, not only in the file that owns it #1363:
Popen.terminate()reachesos.killthrough thestdlib and the child still exited
rc=-15.One exclusion, and why it is bounded
emrg/gui/node_modules/dmg-builder/vendor/holds twelve*.pyfiles that existonly after an
npm install. Including them would make this scan's coveragediffer between a developer's machine and CI, and a vendored file binding
killwould fail a guard about our modules. The scan therefore skips any path with a
node_modulescomponent — and test 3's count assertions are what stop thatexclusion from quietly growing into "the scan skipped most of the tree".
Verification
uv run pytest tests/ -q→ 3065 passed, 16 skippeduv run python -c "from emrg.client.app import run_client"→ OKuv run python -m emrg --help→ OK