From 82171397a7254c91055aa872a8a48b0ab9063d9a Mon Sep 17 00:00:00 2001 From: Jiong Gong Date: Wed, 23 Sep 2026 05:06:11 +0000 Subject: [PATCH 1/5] compass(tests): make the AST guards refuse the spellings they cannot read Several guards in tests/compass enumerate one spelling of what they check and skip every other one, so a violation written another way passes the whole suite. Each fix below makes the guard refuse the shape it cannot read, naming the file, line and source, instead of enumerating one more spelling. - _self_assigned reads every binding of self.x that states its name (=, tuple and list targets, annotated and augmented =, and setattr with a literal name). A computed setattr, a for/with target or a __dict__/vars write has to be listed by its source text, or the guard refuses. ModelRunner lists its three builder setattrs. - _methods reads def, async def and class-body assignments, and refuses any other statement that could bind a name. It exists once, in test_runner_rpc_surface.py, and test_runner_non_allocating.py imports it. - The runner, clock and ir import guards resolve a relative import. One that climbs out of the package is now read as what it imports. - _arity scores a list, starred, annotated or chained target as None instead of as a use of the whole reply, and a new test names every such site. A broadcast with no positional name joins the non-literal tripwire. - The exit test counts only calls that are statements of exit's own body, and refuses a tuple element that is not a literal. - The get_num_blocks key test refuses a block_info subscript that is not a literal. The unanswered_rpc_names caller check counts a call through a module and refuses any other mention. - The clock set guard refuses any mention of set or frozenset, called or not. - The one-site blob test refuses every write of kv_transfer_params_output that is not a plain assignment. - _reply_attribute_reads refuses a use of the forward reply that is neither an attribute read nor a whole hand-off. Closes #223 Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/compass/test_clock_lp_identity.py | 17 +-- tests/compass/test_ir_data_model.py | 5 + tests/compass/test_kv_blob_doc_table.py | 39 +++++-- tests/compass/test_runner_non_allocating.py | 78 +++++++++++--- tests/compass/test_runner_rpc_surface.py | 114 +++++++++++++++----- tests/compass/test_runner_step_semantics.py | 40 +++++-- 6 files changed, 229 insertions(+), 64 deletions(-) diff --git a/tests/compass/test_clock_lp_identity.py b/tests/compass/test_clock_lp_identity.py index 128f757889..8ab22e943e 100644 --- a/tests/compass/test_clock_lp_identity.py +++ b/tests/compass/test_clock_lp_identity.py @@ -460,6 +460,11 @@ def test_the_package_imports_only_the_standard_library_it_names(module): roots += [alias.name.split(".")[0] for alias in node.names] elif isinstance(node, ast.ImportFrom) and not node.level: roots.append((node.module or "").split(".")[0]) + elif isinstance(node, ast.ImportFrom) and not module.parents[ + node.level - 1 + ].is_relative_to(CLOCK_PACKAGE): + # A relative import that climbs out of this package, named as written. + roots.append("." * node.level + (node.module or "")) strays = sorted({root for root in roots if root not in allowed}) assert not strays, f"{module.name} imports {strays}; allowed: {sorted(allowed)}" @@ -473,16 +478,16 @@ def test_the_package_builds_no_set_at_all(module): # see -- and the package has no use for one -- so the rule it enforces is the # one it can prove: none is constructed, so none can be iterated. tree = ast.parse(module.read_text()) + # Every mention of either type, called or not: `frozenset.union` or an + # alias builds one without the call a narrower match would look for. + called = {id(n.func) for n in ast.walk(tree) if isinstance(n, ast.Call)} offenders = [] for node in ast.walk(tree): if isinstance(node, (ast.Set, ast.SetComp)): offenders.append(f"{type(node).__name__} at line {node.lineno}") - elif ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Name) - and node.func.id in ("set", "frozenset") - ): - offenders.append(f"{node.func.id}() at line {node.lineno}") + elif isinstance(node, ast.Name) and node.id in ("set", "frozenset"): + how = "()" if id(node) in called else " named" + offenders.append(f"{node.id}{how} at line {node.lineno}") assert not offenders, ( f"{module.name} builds {offenders}; use a dict with None values as an " "ordered set, or sort at the point of iteration" diff --git a/tests/compass/test_ir_data_model.py b/tests/compass/test_ir_data_model.py index 66da65babc..c9ce67faf9 100644 --- a/tests/compass/test_ir_data_model.py +++ b/tests/compass/test_ir_data_model.py @@ -729,6 +729,11 @@ def test_the_package_imports_only_the_standard_library_it_names(module): roots += [alias.name.split(".")[0] for alias in node.names] elif isinstance(node, ast.ImportFrom) and not node.level: roots.append((node.module or "").split(".")[0]) + elif isinstance(node, ast.ImportFrom) and not module.parents[ + node.level - 1 + ].is_relative_to(IR_PACKAGE): + # A relative import that climbs out of this package, named as written. + roots.append("." * node.level + (node.module or "")) strays = sorted({root for root in roots if root not in allowed}) assert not strays, f"{module.name} imports {strays}; allowed: {sorted(allowed)}" diff --git a/tests/compass/test_kv_blob_doc_table.py b/tests/compass/test_kv_blob_doc_table.py index 2cffb68ae3..bd6002aeb0 100644 --- a/tests/compass/test_kv_blob_doc_table.py +++ b/tests/compass/test_kv_blob_doc_table.py @@ -54,14 +54,11 @@ def _blob_site(path: Path) -> list[ast.Assign]: """Every `.kv_transfer_params_output = ...` in one module. - Deliberately narrow: plain `ast.Assign` to an `ast.Attribute`, which is the - form both connectors use. A `setattr`, an `AnnAssign` or a walrus would be - invisible to it, so it is narrower than the one-site claim it pins. Swept - for those forms tree-wide on 92f1fdafe and none exists -- the only other - writes to the name are `request.py:16` (a dataclass field default) and - `sequence.py:260` (`= None` in `__init__`), and no dict literal anywhere - else in the tree carries these keys. Widen the matcher, do not trust this - comment, if that has to be re-established. + Narrow on purpose: plain `ast.Assign` to an `ast.Attribute`, which is the + form both connectors use. Every other write of the name is what + `_writes_not_read` returns, and the one-site test refuses those, so a + second site written another way fails there by name instead of passing + unseen. """ tree = ast.parse(path.read_text(encoding="utf-8")) return [ @@ -74,6 +71,30 @@ def _blob_site(path: Path) -> list[ast.Assign]: ] +def _writes_not_read(path: Path) -> list[str]: + """Every write of the name `_blob_site` does not read, by line and source. + + A `setattr`, an annotated or augmented assignment, the name inside a tuple + target, or the name as a string anywhere in the module. + """ + source = path.read_text(encoding="utf-8") + tree = ast.parse(source) + read = { + id(t) for n in ast.walk(tree) if isinstance(n, ast.Assign) for t in n.targets + } + return [ + f"{path.name}:{n.lineno}: {source.splitlines()[n.lineno - 1].strip()}" + for n in ast.walk(tree) + if ( + isinstance(n, ast.Attribute) + and n.attr == BLOB_ATTR + and not isinstance(n.ctx, ast.Load) + and id(n) not in read + ) + or (isinstance(n, ast.Constant) and n.value == BLOB_ATTR) + ] + + def _derived_keys(assign: ast.Assign) -> list[str]: """The blob's keys, in source order, refusing anything not a string literal.""" blob = assign.value @@ -152,6 +173,8 @@ def stated(): @pytest.mark.parametrize("backend", sorted(CONNECTORS)) def test_each_backend_builds_the_blob_at_exactly_one_site(backend): """A second site would mean the table describes one of two shapes.""" + unread = _writes_not_read(CONNECTORS[backend]) + assert not unread, f"{BLOB_ATTR} is written in a form not read here: {unread}" sites = _blob_site(CONNECTORS[backend]) assert len(sites) == 1, ( f"{CONNECTORS[backend].name} assigns {BLOB_ATTR} at " diff --git a/tests/compass/test_runner_non_allocating.py b/tests/compass/test_runner_non_allocating.py index 66767c9a60..bf84fdde56 100644 --- a/tests/compass/test_runner_non_allocating.py +++ b/tests/compass/test_runner_non_allocating.py @@ -35,6 +35,7 @@ import pytest import torch +from test_runner_rpc_surface import _methods from torch.utils._python_dispatch import TorchDispatchMode from atom.compass.runner import COMPASS_RUNNER_QUALNAME @@ -65,10 +66,6 @@ def _classes(path): return {n.name: n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)} -def _methods(node): - return {n.name for n in node.body if isinstance(n, ast.FunctionDef)} - - def _self_calls(node): """Names of `self.x(...)` calls anywhere inside a function definition.""" return { @@ -188,8 +185,12 @@ def _method_def(node, name): ) -def _self_assigned(node): - """Every `self.x = ...` in *node*, as (name, the source of its value). +def _on_self(node): + return isinstance(node, ast.Attribute) and ast.unparse(node.value) == "self" + + +def _self_assigned(node, unreadable=()): + """Every binding of `self.x` in *node*, as (name, the source of its value). Pairs, not a mapping keyed by name. A name can be assigned more than once -- `forward_vars` is bound to the dict of buffers and later rebound to a slot @@ -197,16 +198,46 @@ def _self_assigned(node): walked, which here is the rebind. The rebind names no buffer, so keying by name dropped `forward_vars` out of the holder set entirely. Keeping the pairs is what lets a name count as a holder when *any* of its bindings is. + + Read in every spelling that states the name: a target of `=`, including + one inside a tuple or list, of an annotated or augmented `=`, and + `setattr(self, "x", ...)`. Any other write -- a `for` or `with` target, a + `setattr` whose name is computed, anything through `__dict__` or `vars` -- + binds something this cannot read. Each of those must be listed in + *unreadable* by its source text, or this refuses, so a spelling it cannot + read fails here rather than leaving the set smaller than the class. """ - return { - (t.attr, ast.unparse(n.value)) + bound, read, unread = set(), set(), [] + for n in ast.walk(node): + if isinstance(n, (ast.Assign, ast.AnnAssign, ast.AugAssign)): + for target in n.targets if isinstance(n, ast.Assign) else [n.target]: + for t in ast.walk(target): + if _on_self(t) and isinstance(t.ctx, ast.Store) and n.value: + bound.add((t.attr, ast.unparse(n.value))) + read.add(id(t)) + elif ( + isinstance(n, ast.Call) + and ast.unparse(n.func) in ("setattr", "object.__setattr__", "vars") + and n.args + and ast.unparse(n.args[0]) == "self" + ): + name = n.args[1] if len(n.args) == 3 else None + if isinstance(name, ast.Constant) and isinstance(name.value, str): + bound.add((name.value, ast.unparse(n.args[2]))) + else: + unread.append((n.lineno, ast.unparse(n))) + unread += [ + (n.lineno, ast.unparse(n)) for n in ast.walk(node) - if isinstance(n, ast.Assign) - for t in n.targets - if isinstance(t, ast.Attribute) - and isinstance(t.value, ast.Name) - and t.value.id == "self" - } + if (_on_self(n) and isinstance(n.ctx, ast.Store) and id(n) not in read) + or (isinstance(n, ast.Attribute) and n.attr == "__dict__") + ] + assert sorted(text for _, text in unread) == sorted(unreadable), ( + f"{node.name} binds attributes on self in a form not read here: " + + "; ".join(f"line {line}: {text}" for line, text in sorted(unread)) + + f" -- listed as expected: {sorted(unreadable)}" + ) + return bound def test_the_docstring_names_every_attribute_that_holds_the_ring(): @@ -228,7 +259,12 @@ class the answer is the same two, which is the fact the docstring states. buffer, one attribute deeper -- true, and outside a claim about attributes on the runner. """ - assigned = _self_assigned(_classes(ATOM_RUNNER)["ModelRunner"]) + # The three `setattr`s bind whatever the attention builders return -- + # the KV cache and the per-request state, by names the source never states. + assigned = _self_assigned( + _classes(ATOM_RUNNER)["ModelRunner"], + unreadable=["setattr(self, name, value)"] * 3, + ) holders = {n for n, v in assigned if any(t in v for t in BUFFER_TERMS)} assert holders == {"forward_vars", "_fv_ring"} runner = _classes(PACKAGE / "model_runner.py")["CompassModelRunner"] @@ -447,7 +483,17 @@ def test_the_guard_finds_nothing_when_the_root_moves(monkeypatch, tmp_path): ) def test_only_the_binding_module_reaches_the_engine(path): """Everything else stays runnable where the engine cannot be imported.""" - imported = _import_time_imports(path.read_text()) + # A relative import is resolved against this module's package first, so + # one that climbs out of the package is read as the module it names. + package = path.relative_to(REPO).parent.parts + imported = { + ( + ".".join([*package[: len(package) + 1 - level], name[level:]]).strip(".") + if (level := len(name) - len(name.lstrip("."))) + else name + ) + for name in _import_time_imports(path.read_text()) + } engine = {m for m in imported if m.split(".")[0] == "atom"} - { m for m in imported if m.startswith("atom.compass.runner") } diff --git a/tests/compass/test_runner_rpc_surface.py b/tests/compass/test_runner_rpc_surface.py index 1d1b6fd417..3da21931d2 100644 --- a/tests/compass/test_runner_rpc_surface.py +++ b/tests/compass/test_runner_rpc_surface.py @@ -70,7 +70,7 @@ class Site(NamedTuple): line: int waits: bool aggregated: bool - arity: int # values unpacked from the reply; 0 when it is discarded + arity: int | None # values unpacked; 0 when discarded, None when unread def _classes(path): @@ -79,7 +79,27 @@ def _classes(path): def _methods(node): - return {n.name for n in node.body if isinstance(n, ast.FunctionDef)} + """Every name the body of class *node* binds, and so answers `getattr` with. + + A `def` and an `async def` bind a name the same way an assignment in the + class body does, and the worker's `getattr` finds all three. A statement in + the body that could bind a name some other way -- an `if`, a `try`, a + loop -- is refused rather than skipped, so a name bound there cannot pass + as absent. + """ + names = set() + for n in node.body: + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + names.add(n.name) + elif isinstance(n, (ast.Assign, ast.AnnAssign, ast.AugAssign)): + for target in n.targets if isinstance(n, ast.Assign) else [n.target]: + names |= {t.id for t in ast.walk(target) if isinstance(t, ast.Name)} + else: + assert isinstance(n, (ast.Expr, ast.Pass)), ( + f"{node.name}:{n.lineno} binds names in a form not read here: " + f"{ast.unparse(n).splitlines()[0]}" + ) + return names def _busy_loop(): @@ -124,13 +144,23 @@ def _arity(parent): `ast.Return` is the case worth naming, because reading only `ast.Assign` scores `return self.runner_mgr.call_func(...)` as a discard when the value is in fact the function's result -- `engine_core.py:749`, `dummy_execution`. + + Any other shape -- a list or starred target, an annotated one, a chained + target -- is scored `None` rather than `1`, which would read an unpack as a + use of the whole reply, and + `test_every_reply_is_taken_in_a_shape_the_arity_reads` names its site. """ if isinstance(parent, ast.Expr): return 0 - if isinstance(parent, ast.Assign): - target = parent.targets[0] - return len(target.elts) if isinstance(target, ast.Tuple) else 1 - return 1 + if isinstance(parent, (ast.Return, ast.Call)): + return 1 + target = parent.targets[0] if isinstance(parent, ast.Assign) else None + if isinstance(target, ast.Name) and len(parent.targets) == 1: + return 1 + starred = any(isinstance(e, ast.Starred) for e in getattr(target, "elts", ())) + if isinstance(target, ast.Tuple) and len(parent.targets) == 1 and not starred: + return len(target.elts) + return None def _mentions(roots, needle, base=REPO): @@ -168,13 +198,13 @@ def _call_sites(): isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) ): continue - if node.func.attr not in BROADCAST or not node.args: + if node.func.attr not in BROADCAST: continue where = f"{path.relative_to(REPO)}:{node.lineno}" if "compass" in path.parts: from_compass.append(where) continue - if not isinstance(node.args[0], ast.Constant): + if not node.args or not isinstance(node.args[0], ast.Constant): non_literal.append(where) continue aggregated = node.func.attr == "call_func_with_aggregation" @@ -245,6 +275,14 @@ def test_both_filters_in_the_derivation_drop_nothing(): assert FROM_COMPASS == [] +def test_every_reply_is_taken_in_a_shape_the_arity_reads(): + """A site `_arity` could not score would otherwise vanish from every arity set.""" + unscored = [ + f"{s.file}:{s.line}" for v in SITES.values() for s in v if s.arity is None + ] + assert unscored == [], f"replies taken in a shape not scored: {unscored}" + + def test_the_dispatched_names_outside_the_surface_belong_to_other_runners(): """Without this the intersection above could shrink and look like a pass. @@ -481,13 +519,18 @@ def test_the_base_capture_reaches_a_device_before_it_reaches_the_model(): def test_get_num_blocks_refuses_and_the_keys_its_caller_reads_are_named(): site = SITES["get_num_blocks"][0] tree = ast.parse((ENGINE / site.file).read_text()) - required = { - n.slice.value + subscripts = [ + n for n in ast.walk(tree) - if isinstance(n, ast.Subscript) - and getattr(n.value, "id", None) == "block_info" - and isinstance(n.slice, ast.Constant) - } + if isinstance(n, ast.Subscript) and getattr(n.value, "id", None) == "block_info" + ] + unread = [ + f"{site.file}:{n.lineno}: {ast.unparse(n)}" + for n in subscripts + if not isinstance(n.slice, ast.Constant) + ] + assert not unread, f"a key the caller reads is not written as a literal: {unread}" + required = {n.slice.value for n in subscripts} optional = { n.args[0].value for n in ast.walk(tree) @@ -886,7 +929,13 @@ def test_what_the_comment_says_a_hole_at_exit_loses_is_what_exit_does(): for n in ast.walk(_classes(ATOM_RUNNER)["ModelRunner"]) if isinstance(n, ast.FunctionDef) and n.name == "exit" ) - calls = {ast.unparse(n.func) for n in ast.walk(body) if isinstance(n, ast.Call)} + # Statements of the body itself: a call inside a lambda, a branch or a + # nested def is one `exit` may never make. + calls = { + ast.unparse(n.value.func) + for n in body.body + if isinstance(n, ast.Expr) and isinstance(n.value, ast.Call) + } assert {"destroy_dist_env", "torch.cuda.empty_cache"} <= calls assert "`ModelRunner.exit` never runs" in comment assert "the distributed environment is never destroyed" in comment @@ -906,7 +955,11 @@ def test_what_the_comment_says_a_hole_at_exit_loses_is_what_exit_does(): ] assert len(literal_loops) == 1 kv = literal_loops[0] - assert {e.value for e in kv.iter.elts if isinstance(e, ast.Constant)} == { + unread = [ast.unparse(e) for e in kv.iter.elts if not isinstance(e, ast.Constant)] + assert ( + not unread + ), f"line {kv.lineno} deletes names not written as literals: {unread}" + assert {e.value for e in kv.iter.elts} == { "kv_cache", "kv_scale", "index_cache", @@ -942,16 +995,25 @@ def test_the_unanswered_helper_describes_its_whole_return_and_not_one_half(): word = {10: "ten", 11: "eleven", 12: "twelve", 13: "thirteen"}.get(len(RPC_SURFACE)) assert word is not None, f"no count word for {len(RPC_SURFACE)} names" assert f"all {word}" in doc - callers = [ - str(f.relative_to(REPO)) - for f in sorted((REPO / "atom").rglob("*.py")) - if any( - isinstance(n, ast.Call) - and getattr(n.func, "id", None) == "unanswered_rpc_names" - for n in ast.walk(ast.parse(f.read_text())) - ) - ] - assert callers == ["atom/compass/runner/model_runner.py"] + # Called by bare name or through a module, both are a call; any other + # mention -- an alias, a `partial`, a callback -- is a caller this cannot + # follow, so it is refused rather than left out of the count. + callers, unread = set(), [] + for f in sorted((REPO / "atom").rglob("*.py")): + tree = ast.parse(f.read_text()) + called = {id(n.func) for n in ast.walk(tree) if isinstance(n, ast.Call)} + for n in ast.walk(tree): + if "unanswered_rpc_names" not in ( + getattr(n, "id", 0), + getattr(n, "attr", 0), + ): + continue + if id(n) in called: + callers.add(str(f.relative_to(REPO))) + else: + unread.append(f"{f.relative_to(REPO)}:{n.lineno}: {ast.unparse(n)}") + assert not unread, f"the helper is reached other than by calling it: {unread}" + assert callers == {"atom/compass/runner/model_runner.py"} tree = ast.parse(COMPOSED) bound = next( n.targets[0].id diff --git a/tests/compass/test_runner_step_semantics.py b/tests/compass/test_runner_step_semantics.py index 8783f1f5dc..7341d7a156 100644 --- a/tests/compass/test_runner_step_semantics.py +++ b/tests/compass/test_runner_step_semantics.py @@ -421,16 +421,40 @@ def _decorators(path, class_name, name): def _reply_attribute_reads(): - """Every attribute ATOM reads off a forward reply, from ATOM's own source.""" - names = set() + """Every attribute ATOM reads off a forward reply, from ATOM's own source. + + A read is `fwd_out.x` or `fwd_output.x`. The reply may also be handed on + whole -- passed positionally to a method, returned, or tested against + `None` -- which reads nothing here. Any other use of it, a `getattr`, an + alias, a call of a plain function on it, is refused by file and line: it + could read an attribute this set would never contain. + """ + names, unread = set(), [] for path in (REPO / "atom" / "model_engine").glob("*.py"): - for node in ast.walk(ast.parse(path.read_text())): - if ( - isinstance(node, ast.Attribute) - and isinstance(node.value, ast.Name) - and node.value.id in {"fwd_out", "fwd_output"} + tree = ast.parse(path.read_text()) + parent = {c: n for n in ast.walk(tree) for c in ast.iter_child_nodes(n)} + for node in ast.walk(tree): + if not ( + isinstance(node, ast.Name) + and node.id in {"fwd_out", "fwd_output"} + and isinstance(node.ctx, ast.Load) ): - names.add(node.attr) + continue + up = parent[node] + handed_on = ( + isinstance(up, ast.Return) + or ast.unparse(up) == f"{node.id} is None" + or ( + isinstance(up, ast.Call) + and isinstance(up.func, ast.Attribute) + and node in up.args + ) + ) + if isinstance(up, ast.Attribute): + names.add(up.attr) + elif not handed_on: + unread.append(f"{path.name}:{node.lineno}: {ast.unparse(up)}") + assert not unread, f"the forward reply is used in a form not read here: {unread}" return names From 0364f226d77b1ce8bff3f2342a1dc9882acbf6bc Mon Sep 17 00:00:00 2001 From: Jiong Gong Date: Wed, 23 Sep 2026 06:48:03 +0000 Subject: [PATCH 2/5] compass(tests): refuse the spellings review round 1 found still readable Each of the seven guards the review drove red now refuses what it cannot read instead of matching one more spelling. - _self_assigned: any mention of setattr, __setattr__, __dict__ or vars in the class that is not a setattr(self, "", ...) it read is refused, however it is reached (self.__setattr__, super().__setattr__, builtins.setattr). - _writes_not_read: any mention of setattr, __setattr__, __dict__ or vars in a connector is refused; neither connector has one. - _methods no longer asserts at import. It records a class-body statement it cannot read -- anything but a def, a class, an assignment, a docstring or pass -- and a named test lists them. An expression such as vars().update(...) is one of those. - _arity scores a reply passed to another call as None, not 1. - The forward-reply walk accepts a whole hand-off only to postprocess and send_tokens, the two methods the tree hands it to. - The exit test refuses any return or raise in exit other than the opening guard's and the closing return True. - The unanswered_rpc_names caller check refuses an import of it under another name. - The class-body and self-binding refusals name the file. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/compass/test_kv_blob_doc_table.py | 8 +++- tests/compass/test_runner_non_allocating.py | 43 ++++++++++-------- tests/compass/test_runner_rpc_surface.py | 48 ++++++++++++++++----- tests/compass/test_runner_step_semantics.py | 17 +++++--- 4 files changed, 81 insertions(+), 35 deletions(-) diff --git a/tests/compass/test_kv_blob_doc_table.py b/tests/compass/test_kv_blob_doc_table.py index bd6002aeb0..97deeeacf1 100644 --- a/tests/compass/test_kv_blob_doc_table.py +++ b/tests/compass/test_kv_blob_doc_table.py @@ -74,8 +74,10 @@ def _blob_site(path: Path) -> list[ast.Assign]: def _writes_not_read(path: Path) -> list[str]: """Every write of the name `_blob_site` does not read, by line and source. - A `setattr`, an annotated or augmented assignment, the name inside a tuple - target, or the name as a string anywhere in the module. + An annotated or augmented assignment, the name inside a tuple target, the + name as a string anywhere in the module -- and any mention of `setattr`, + `__setattr__`, `__dict__` or `vars` at all, since a name built at run time + cannot be read and neither connector writes attributes that way. """ source = path.read_text(encoding="utf-8") tree = ast.parse(source) @@ -92,6 +94,8 @@ def _writes_not_read(path: Path) -> list[str]: and id(n) not in read ) or (isinstance(n, ast.Constant) and n.value == BLOB_ATTR) + or getattr(n, "id", getattr(n, "attr", None)) + in ("setattr", "__setattr__", "__dict__", "vars") ] diff --git a/tests/compass/test_runner_non_allocating.py b/tests/compass/test_runner_non_allocating.py index bf84fdde56..0c811e6df6 100644 --- a/tests/compass/test_runner_non_allocating.py +++ b/tests/compass/test_runner_non_allocating.py @@ -63,6 +63,8 @@ def _classes(path): tree = ast.parse(path.read_text()) + for n in ast.walk(tree): + n.file = path.name return {n.name: n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)} @@ -185,6 +187,10 @@ def _method_def(node, name): ) +# Every name through which an attribute can be written without an assignment. +_WRITERS = ("setattr", "__setattr__", "__dict__", "vars") + + def _on_self(node): return isinstance(node, ast.Attribute) and ast.unparse(node.value) == "self" @@ -201,13 +207,13 @@ def _self_assigned(node, unreadable=()): Read in every spelling that states the name: a target of `=`, including one inside a tuple or list, of an annotated or augmented `=`, and - `setattr(self, "x", ...)`. Any other write -- a `for` or `with` target, a - `setattr` whose name is computed, anything through `__dict__` or `vars` -- - binds something this cannot read. Each of those must be listed in + `setattr(self, "x", ...)`. Any other write -- a `for` or `with` target, and + every other mention of `setattr`, `__setattr__`, `__dict__` or `vars`, + however it is reached -- binds something this cannot read. Each of those must be listed in *unreadable* by its source text, or this refuses, so a spelling it cannot read fails here rather than leaving the set smaller than the class. """ - bound, read, unread = set(), set(), [] + bound, read = set(), set() for n in ast.walk(node): if isinstance(n, (ast.Assign, ast.AnnAssign, ast.AugAssign)): for target in n.targets if isinstance(n, ast.Assign) else [n.target]: @@ -217,24 +223,27 @@ def _self_assigned(node, unreadable=()): read.add(id(t)) elif ( isinstance(n, ast.Call) - and ast.unparse(n.func) in ("setattr", "object.__setattr__", "vars") - and n.args - and ast.unparse(n.args[0]) == "self" + and ast.unparse(n.func) == "setattr" + and [ast.unparse(a) for a in n.args[:1]] == ["self"] + and len(n.args) == 3 + and isinstance(n.args[1], ast.Constant) + and isinstance(n.args[1].value, str) ): - name = n.args[1] if len(n.args) == 3 else None - if isinstance(name, ast.Constant) and isinstance(name.value, str): - bound.add((name.value, ast.unparse(n.args[2]))) - else: - unread.append((n.lineno, ast.unparse(n))) - unread += [ - (n.lineno, ast.unparse(n)) + bound.add((n.args[1].value, ast.unparse(n.args[2]))) + read.add(id(n.func)) + calls = {id(n.func): n for n in ast.walk(node) if isinstance(n, ast.Call)} + unread = [ + (n.lineno, ast.unparse(calls.get(id(n), n))) for n in ast.walk(node) - if (_on_self(n) and isinstance(n.ctx, ast.Store) and id(n) not in read) - or (isinstance(n, ast.Attribute) and n.attr == "__dict__") + if id(n) not in read + and ( + (_on_self(n) and isinstance(n.ctx, ast.Store)) + or getattr(n, "id", getattr(n, "attr", None)) in _WRITERS + ) ] assert sorted(text for _, text in unread) == sorted(unreadable), ( f"{node.name} binds attributes on self in a form not read here: " - + "; ".join(f"line {line}: {text}" for line, text in sorted(unread)) + + "; ".join(f"{node.file}:{line}: {text}" for line, text in sorted(unread)) + f" -- listed as expected: {sorted(unreadable)}" ) return bound diff --git a/tests/compass/test_runner_rpc_surface.py b/tests/compass/test_runner_rpc_surface.py index 3da21931d2..b6ba31c668 100644 --- a/tests/compass/test_runner_rpc_surface.py +++ b/tests/compass/test_runner_rpc_surface.py @@ -75,17 +75,33 @@ class Site(NamedTuple): def _classes(path): tree = ast.parse(path.read_text()) + for n in ast.walk(tree): + n.file = path.name return {n.name: n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)} +UNREAD_CLASS_BODY: list[str] = [] + + +def _is_docstring(n): + return ( + isinstance(n, ast.Expr) + and isinstance(n.value, ast.Constant) + and isinstance(n.value.value, str) + ) + + def _methods(node): """Every name the body of class *node* binds, and so answers `getattr` with. A `def` and an `async def` bind a name the same way an assignment in the class body does, and the worker's `getattr` finds all three. A statement in the body that could bind a name some other way -- an `if`, a `try`, a - loop -- is refused rather than skipped, so a name bound there cannot pass - as absent. + loop, an expression such as `vars().update(...)` -- is recorded in + `UNREAD_CLASS_BODY` rather than skipped, and + `test_every_class_body_the_method_sets_are_read_from_is_read` names it. + Recorded, not raised: this runs at import, where a raise would stop every + module importing this one from collecting. """ names = set() for n in node.body: @@ -94,10 +110,9 @@ class body does, and the worker's `getattr` finds all three. A statement in elif isinstance(n, (ast.Assign, ast.AnnAssign, ast.AugAssign)): for target in n.targets if isinstance(n, ast.Assign) else [n.target]: names |= {t.id for t in ast.walk(target) if isinstance(t, ast.Name)} - else: - assert isinstance(n, (ast.Expr, ast.Pass)), ( - f"{node.name}:{n.lineno} binds names in a form not read here: " - f"{ast.unparse(n).splitlines()[0]}" + elif not isinstance(n, ast.Pass) and not _is_docstring(n): + UNREAD_CLASS_BODY.append( + f"{node.file}:{n.lineno}: {ast.unparse(n).splitlines()[0]}" ) return names @@ -137,8 +152,7 @@ def _arity(parent): `0` means the reply is discarded -- the broadcast is a bare statement and nothing can read what came back. `1` means it is used whole: bound to a - name, handed straight back to this function's own caller, or passed on as - an argument. Anything above `1` is a tuple unpack, which is the only shape + name, or handed straight back to this function's own caller. Anything above `1` is a tuple unpack, which is the only shape that fixes a length rather than just a type. `ast.Return` is the case worth naming, because reading only `ast.Assign` @@ -146,13 +160,13 @@ def _arity(parent): is in fact the function's result -- `engine_core.py:749`, `dummy_execution`. Any other shape -- a list or starred target, an annotated one, a chained - target -- is scored `None` rather than `1`, which would read an unpack as a + target, an argument to another call -- is scored `None` rather than `1`, which would read an unpack as a use of the whole reply, and `test_every_reply_is_taken_in_a_shape_the_arity_reads` names its site. """ if isinstance(parent, ast.Expr): return 0 - if isinstance(parent, (ast.Return, ast.Call)): + if isinstance(parent, ast.Return): return 1 target = parent.targets[0] if isinstance(parent, ast.Assign) else None if isinstance(target, ast.Name) and len(parent.targets) == 1: @@ -283,6 +297,11 @@ def test_every_reply_is_taken_in_a_shape_the_arity_reads(): assert unscored == [], f"replies taken in a shape not scored: {unscored}" +def test_every_class_body_the_method_sets_are_read_from_is_read(): + """A statement `_methods` could not read may bind a name it would miss.""" + assert UNREAD_CLASS_BODY == [], f"class bodies not read: {UNREAD_CLASS_BODY}" + + def test_the_dispatched_names_outside_the_surface_belong_to_other_runners(): """Without this the intersection above could shrink and look like a pass. @@ -937,6 +956,13 @@ def test_what_the_comment_says_a_hole_at_exit_loses_is_what_exit_does(): if isinstance(n, ast.Expr) and isinstance(n.value, ast.Call) } assert {"destroy_dist_env", "torch.cuda.empty_cache"} <= calls + # And nothing leaves `exit` between them: its only ways out are the guard + # that opens it and the `return True` that closes it. + exits = sorted( + ast.unparse(n) for n in ast.walk(body) if isinstance(n, (ast.Return, ast.Raise)) + ) + assert exits == ["return", "return True"], f"exit leaves early: {exits}" + assert ast.unparse(body.body[-1]) == "return True" assert "`ModelRunner.exit` never runs" in comment assert "the distributed environment is never destroyed" in comment assert "`torch.cuda.empty_cache()` never runs" in comment @@ -996,7 +1022,7 @@ def test_the_unanswered_helper_describes_its_whole_return_and_not_one_half(): assert word is not None, f"no count word for {len(RPC_SURFACE)} names" assert f"all {word}" in doc # Called by bare name or through a module, both are a call; any other - # mention -- an alias, a `partial`, a callback -- is a caller this cannot + # mention -- an `import ... as`, a `partial`, a callback -- is a caller this cannot # follow, so it is refused rather than left out of the count. callers, unread = set(), [] for f in sorted((REPO / "atom").rglob("*.py")): diff --git a/tests/compass/test_runner_step_semantics.py b/tests/compass/test_runner_step_semantics.py index 7341d7a156..7fe036e05a 100644 --- a/tests/compass/test_runner_step_semantics.py +++ b/tests/compass/test_runner_step_semantics.py @@ -420,13 +420,20 @@ def _decorators(path, class_name, name): return [ast.unparse(d) for d in _function(path, class_name, name).decorator_list] +# The methods the reply is handed to whole. `postprocess` reads it under +# `fwd_output`, which the walk below sees; `send_tokens` pickles it and reads +# nothing. A hand-off to any other method is refused: it would read the reply +# under a name this walk does not follow. +HANDED_TO = ("postprocess", "send_tokens") + + def _reply_attribute_reads(): """Every attribute ATOM reads off a forward reply, from ATOM's own source. A read is `fwd_out.x` or `fwd_output.x`. The reply may also be handed on - whole -- passed positionally to a method, returned, or tested against - `None` -- which reads nothing here. Any other use of it, a `getattr`, an - alias, a call of a plain function on it, is refused by file and line: it + whole -- passed positionally to one of `HANDED_TO`, returned, or tested + against `None`. Any other use of it, a `getattr`, an alias, a hand-off to + any other function or method, is refused by file and line: it could read an attribute this set would never contain. """ names, unread = set(), [] @@ -445,8 +452,8 @@ def _reply_attribute_reads(): isinstance(up, ast.Return) or ast.unparse(up) == f"{node.id} is None" or ( - isinstance(up, ast.Call) - and isinstance(up.func, ast.Attribute) + getattr(up, "func", None) is not None + and getattr(up.func, "attr", None) in HANDED_TO and node in up.args ) ) From 176cac3d4f504cf14d7e7bc0e569a205a2258d47 Mon Sep 17 00:00:00 2001 From: Jiong Gong Date: Wed, 23 Sep 2026 07:00:07 +0000 Subject: [PATCH 3/5] compass(tests): refuse an aliased import of unanswered_rpc_names The previous commit meant to make the caller check refuse an import of the helper under another name, but that hunk did not apply, and an import-as caller still passed. The check now refuses it. This commit also rewraps two docstrings. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/compass/test_runner_non_allocating.py | 7 ++++--- tests/compass/test_runner_rpc_surface.py | 17 +++++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/compass/test_runner_non_allocating.py b/tests/compass/test_runner_non_allocating.py index 0c811e6df6..40f76b81ed 100644 --- a/tests/compass/test_runner_non_allocating.py +++ b/tests/compass/test_runner_non_allocating.py @@ -209,9 +209,10 @@ def _self_assigned(node, unreadable=()): one inside a tuple or list, of an annotated or augmented `=`, and `setattr(self, "x", ...)`. Any other write -- a `for` or `with` target, and every other mention of `setattr`, `__setattr__`, `__dict__` or `vars`, - however it is reached -- binds something this cannot read. Each of those must be listed in - *unreadable* by its source text, or this refuses, so a spelling it cannot - read fails here rather than leaving the set smaller than the class. + however it is reached -- binds something this cannot read. Each of those + must be listed in *unreadable* by its source text, or this refuses, so a + spelling it cannot read fails here rather than leaving the set smaller + than the class. """ bound, read = set(), set() for n in ast.walk(node): diff --git a/tests/compass/test_runner_rpc_surface.py b/tests/compass/test_runner_rpc_surface.py index b6ba31c668..28abd30a7c 100644 --- a/tests/compass/test_runner_rpc_surface.py +++ b/tests/compass/test_runner_rpc_surface.py @@ -152,16 +152,17 @@ def _arity(parent): `0` means the reply is discarded -- the broadcast is a bare statement and nothing can read what came back. `1` means it is used whole: bound to a - name, or handed straight back to this function's own caller. Anything above `1` is a tuple unpack, which is the only shape - that fixes a length rather than just a type. + name, or handed straight back to this function's own caller. Anything + above `1` is a tuple unpack, which is the only shape that fixes a length + rather than just a type. `ast.Return` is the case worth naming, because reading only `ast.Assign` scores `return self.runner_mgr.call_func(...)` as a discard when the value is in fact the function's result -- `engine_core.py:749`, `dummy_execution`. Any other shape -- a list or starred target, an annotated one, a chained - target, an argument to another call -- is scored `None` rather than `1`, which would read an unpack as a - use of the whole reply, and + target, an argument to another call -- is scored `None` rather than `1`, + which would read an unpack as a use of the whole reply, and `test_every_reply_is_taken_in_a_shape_the_arity_reads` names its site. """ if isinstance(parent, ast.Expr): @@ -1022,13 +1023,17 @@ def test_the_unanswered_helper_describes_its_whole_return_and_not_one_half(): assert word is not None, f"no count word for {len(RPC_SURFACE)} names" assert f"all {word}" in doc # Called by bare name or through a module, both are a call; any other - # mention -- an `import ... as`, a `partial`, a callback -- is a caller this cannot - # follow, so it is refused rather than left out of the count. + # mention -- an `import ... as`, a `partial`, a callback -- is a caller + # this cannot follow, so it is refused rather than left out of the count. callers, unread = set(), [] for f in sorted((REPO / "atom").rglob("*.py")): tree = ast.parse(f.read_text()) called = {id(n.func) for n in ast.walk(tree) if isinstance(n, ast.Call)} for n in ast.walk(tree): + if isinstance(n, ast.alias) and n.name == "unanswered_rpc_names": + if n.asname: + unread.append(f"{f.relative_to(REPO)}:{n.lineno}: as {n.asname}") + continue if "unanswered_rpc_names" not in ( getattr(n, "id", 0), getattr(n, "attr", 0), From 051fc93ca239bff6f10cd68ca94afbab11148eb8 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 29 Sep 2026 03:49:30 +0000 Subject: [PATCH 4/5] compass(tests): drop the unanswered-helper caller refusal The re-scope keeps the guards that inspect code structure and drops the ones that hardened tests of prose. The caller refusal in test_the_unanswered_helper_returns_one_list_its_one_caller_partitions was added to harden a test of `unanswered_rpc_names.__doc__`; that test is back to the integration branch's form. The exit test's comment assertions went with the merge, where the upstream deletion won. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/compass/test_runner_rpc_surface.py | 33 +++++++----------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/tests/compass/test_runner_rpc_surface.py b/tests/compass/test_runner_rpc_surface.py index 14d661fde6..21bb15bfdd 100644 --- a/tests/compass/test_runner_rpc_surface.py +++ b/tests/compass/test_runner_rpc_surface.py @@ -1041,29 +1041,16 @@ def test_the_unanswered_helper_returns_one_list_its_one_caller_partitions(): """`unanswered_rpc_names` returns its list unpartitioned, and its only caller in the package splits it on `RPC_SURFACE` before reporting it. """ - # Called by bare name or through a module, both are a call; any other - # mention -- an `import ... as`, a `partial`, a callback -- is a caller - # this cannot follow, so it is refused rather than left out of the count. - callers, unread = set(), [] - for f in sorted((REPO / "atom").rglob("*.py")): - tree = ast.parse(f.read_text()) - called = {id(n.func) for n in ast.walk(tree) if isinstance(n, ast.Call)} - for n in ast.walk(tree): - if isinstance(n, ast.alias) and n.name == "unanswered_rpc_names": - if n.asname: - unread.append(f"{f.relative_to(REPO)}:{n.lineno}: as {n.asname}") - continue - if "unanswered_rpc_names" not in ( - getattr(n, "id", 0), - getattr(n, "attr", 0), - ): - continue - if id(n) in called: - callers.add(str(f.relative_to(REPO))) - else: - unread.append(f"{f.relative_to(REPO)}:{n.lineno}: {ast.unparse(n)}") - assert not unread, f"the helper is reached other than by calling it: {unread}" - assert callers == {"atom/compass/runner/model_runner.py"} + callers = [ + str(f.relative_to(REPO)) + for f in sorted((REPO / "atom").rglob("*.py")) + if any( + isinstance(n, ast.Call) + and getattr(n.func, "id", None) == "unanswered_rpc_names" + for n in ast.walk(ast.parse(f.read_text())) + ) + ] + assert callers == ["atom/compass/runner/model_runner.py"] tree = ast.parse(COMPOSED) bound = next( n.targets[0].id From a96ca646b891aa7fcbd074eb61ab43d47b7eff95 Mon Sep 17 00:00:00 2001 From: root Date: Tue, 29 Sep 2026 06:17:40 +0000 Subject: [PATCH 5/5] compass(tests): restore the unanswered-helper caller refusal This reverts commit 051fc93ca239bff6f10cd68ca94afbab11148eb8. The caller refusal in test_the_unanswered_helper_returns_one_list_its_one_caller_partitions reads syntax-tree nodes only: import aliases, Name ids, Attribute attrs and the identity of each Call's func. It reads no docstring, comment or design document, so it stays under #489. The __doc__ assertions that were deleted by #492 were never part of it. Co-Authored-By: Claude Opus 5.5 (1M context) --- tests/compass/test_runner_rpc_surface.py | 33 +++++++++++++++++------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/compass/test_runner_rpc_surface.py b/tests/compass/test_runner_rpc_surface.py index 21bb15bfdd..14d661fde6 100644 --- a/tests/compass/test_runner_rpc_surface.py +++ b/tests/compass/test_runner_rpc_surface.py @@ -1041,16 +1041,29 @@ def test_the_unanswered_helper_returns_one_list_its_one_caller_partitions(): """`unanswered_rpc_names` returns its list unpartitioned, and its only caller in the package splits it on `RPC_SURFACE` before reporting it. """ - callers = [ - str(f.relative_to(REPO)) - for f in sorted((REPO / "atom").rglob("*.py")) - if any( - isinstance(n, ast.Call) - and getattr(n.func, "id", None) == "unanswered_rpc_names" - for n in ast.walk(ast.parse(f.read_text())) - ) - ] - assert callers == ["atom/compass/runner/model_runner.py"] + # Called by bare name or through a module, both are a call; any other + # mention -- an `import ... as`, a `partial`, a callback -- is a caller + # this cannot follow, so it is refused rather than left out of the count. + callers, unread = set(), [] + for f in sorted((REPO / "atom").rglob("*.py")): + tree = ast.parse(f.read_text()) + called = {id(n.func) for n in ast.walk(tree) if isinstance(n, ast.Call)} + for n in ast.walk(tree): + if isinstance(n, ast.alias) and n.name == "unanswered_rpc_names": + if n.asname: + unread.append(f"{f.relative_to(REPO)}:{n.lineno}: as {n.asname}") + continue + if "unanswered_rpc_names" not in ( + getattr(n, "id", 0), + getattr(n, "attr", 0), + ): + continue + if id(n) in called: + callers.add(str(f.relative_to(REPO))) + else: + unread.append(f"{f.relative_to(REPO)}:{n.lineno}: {ast.unparse(n)}") + assert not unread, f"the helper is reached other than by calling it: {unread}" + assert callers == {"atom/compass/runner/model_runner.py"} tree = ast.parse(COMPOSED) bound = next( n.targets[0].id