Skip to content

Commit 1b4d817

Browse files
committed
fix(ci): attach the uncapped-pool opt-out to the call it exempts
`_opted_out` accepted the opt-out marker anywhere on the line above the offending call: start = max(node.lineno - 2, 0) # -1 for 0-based, -1 more for a preceding comment end = getattr(node, "end_lineno", node.lineno) return any(OPT_OUT_MARKER in line for line in lines[start:end]) Nothing requires that line to be a comment, or to have anything to do with the call. So a trailing marker annotating one statement also exempts the statement on the next line: with pytest.raises(RuntimeError): DeviceMemoryResource(dev, DeviceMemoryResourceOptions(ipc_enabled=True)) # uncapped-pool-ok: raises first DeviceMemoryResource(dev, DeviceMemoryResourceOptions()) # <- silently exempt This is the shape a reviewer is least likely to catch, because both lines look correctly annotated. The marker text merely appearing in an unrelated string literal has the same effect: msg = "see uncapped-pool-ok in AGENTS.md" DeviceMemoryResource(dev, DeviceMemoryResourceOptions()) # <- silently exempt Bound the marker to the call's own statement instead. `_iter_calls` walks the tree carrying the chain of `ast.stmt` ancestors, and a marker counts when it is inside the call's statement (start of the statement through the end of the call), on the header of a compound statement containing the call, or on a dedicated comment line immediately above the statement. Every documented placement keeps working -- comment line above, inline on the call, and the `pytest.raises` block form from cuda_core/tests/AGENTS.md. Anchoring on the statement rather than the call also fixes a false positive the old window had, where a marker on the first line of a multi-line construction did not reach the inner options call: mr = DeviceMemoryResource( # uncapped-pool-ok: reason dev, DeviceMemoryResourceOptions(), # <- was reported anyway ) cuda_core/tests has no opt-out that relied on the loose behavior, so the tightened rule leaves the tree clean.
1 parent 3bd069a commit 1b4d817

3 files changed

Lines changed: 111 additions & 8 deletions

File tree

ci/tools/check_mempool_hygiene.py

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,21 +53,58 @@ def _dict_is_capped(node: ast.Dict) -> bool:
5353
return False
5454

5555

56-
def _opted_out(lines: list[str], node: ast.AST) -> bool:
57-
"""True if the call, or the line above it, carries the opt-out marker."""
58-
start = max(node.lineno - 2, 0) # -1 for 0-based, -1 more for a preceding comment
56+
def _iter_calls(tree: ast.AST):
57+
"""Yield ``(call, statements)`` for every call in ``tree``.
58+
59+
``statements`` is the chain of ``ast.stmt`` ancestors, outermost first, so
60+
``statements[-1]`` is the statement the call belongs to. The chain is what
61+
bounds an opt-out marker: a marker annotates the statement it sits in (or a
62+
statement containing it), not whatever the next line happens to construct.
63+
"""
64+
stack: list[tuple[ast.AST, tuple[ast.stmt, ...]]] = [(tree, ())]
65+
while stack:
66+
node, stmts = stack.pop()
67+
if isinstance(node, ast.stmt):
68+
stmts = (*stmts, node)
69+
if isinstance(node, ast.Call):
70+
yield node, stmts
71+
stack.extend((child, stmts) for child in ast.iter_child_nodes(node))
72+
73+
74+
def _opted_out(lines: list[str], node: ast.Call, stmts: tuple[ast.stmt, ...]) -> bool:
75+
"""True if an opt-out marker annotates ``node``.
76+
77+
A marker counts when it is on a line belonging to the call's own statement
78+
(including continuation lines of a multi-line construction), on the header
79+
line of a compound statement containing the call, or on a dedicated comment
80+
line immediately above the call's statement.
81+
82+
A marker anywhere else does not count. Previously the whole line above was
83+
accepted unconditionally, so a trailing marker annotating the *previous*
84+
statement silently exempted the next one, and a marker appearing inside an
85+
unrelated string literal exempted whatever followed it.
86+
"""
87+
nearest = stmts[-1] if stmts else node
88+
first = nearest.lineno - 1 # 0-based index of the statement's first line
5989
end = getattr(node, "end_lineno", node.lineno)
60-
return any(OPT_OUT_MARKER in line for line in lines[start:end])
90+
if any(OPT_OUT_MARKER in line for line in lines[first:end]):
91+
return True
92+
# Headers of the compound statements containing the call ("with", "for",
93+
# "def", ...): a marker there annotates a block this call is part of.
94+
if any(OPT_OUT_MARKER in lines[s.lineno - 1] for s in stmts[:-1]):
95+
return True
96+
if first == 0:
97+
return False
98+
above = lines[first - 1].strip()
99+
return above.startswith("#") and OPT_OUT_MARKER in above
61100

62101

63102
def violations_in(path: Path) -> list[str]:
64103
"""Return one message per uncapped pool construction in ``path``."""
65104
source = path.read_text(encoding="utf-8")
66105
lines = source.splitlines()
67106
found = []
68-
for node in ast.walk(ast.parse(source, filename=str(path))):
69-
if not isinstance(node, ast.Call):
70-
continue
107+
for node, stmts in _iter_calls(ast.parse(source, filename=str(path))):
71108
name = _callee_name(node)
72109
if name in CAPPABLE_OPTIONS:
73110
uncapped = not _is_capped(node)
@@ -77,7 +114,7 @@ def violations_in(path: Path) -> list[str]:
77114
uncapped = any(not _dict_is_capped(d) for d in dicts)
78115
else:
79116
continue
80-
if uncapped and not _opted_out(lines, node):
117+
if uncapped and not _opted_out(lines, node, stmts):
81118
found.append(f"{path.as_posix()}:{node.lineno}: {name} without max_size")
82119
return found
83120

ci/tools/tests/test_check_mempool_hygiene.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,3 +94,63 @@ def test_the_live_test_suite_is_clean():
9494
# violation could ride in on a rename or a merge.
9595
assert DEFAULT_TREE.is_dir()
9696
assert main([]) == 0
97+
98+
99+
UNCAPPED_CALL = "DeviceMemoryResource(dev, DeviceMemoryResourceOptions())"
100+
101+
# Placements where the marker does NOT annotate the offending call. Accepting
102+
# the whole preceding line let each of these silence a real violation.
103+
MARKER_DOES_NOT_CARRY = [
104+
pytest.param(
105+
f"{UNCAPPED_CALL} # uncapped-pool-ok: annotates THIS line\n{UNCAPPED_CALL}\n",
106+
id="trailing-marker-on-previous-statement",
107+
),
108+
pytest.param(
109+
'msg = "see uncapped-pool-ok in AGENTS.md"\n' + UNCAPPED_CALL + "\n",
110+
id="marker-inside-an-unrelated-string",
111+
),
112+
pytest.param(
113+
f"# uncapped-pool-ok: detached by a blank line\n\n{UNCAPPED_CALL}\n",
114+
id="blank-line-between-comment-and-call",
115+
),
116+
]
117+
118+
# Placements where the marker does annotate the call and must keep working.
119+
MARKER_CARRIES = [
120+
pytest.param(f"# uncapped-pool-ok: reason\n{UNCAPPED_CALL}\n", id="comment-line-above"),
121+
pytest.param(f"def test_x():\n # uncapped-pool-ok: reason\n {UNCAPPED_CALL}\n", id="indented-comment-above"),
122+
pytest.param(f"{UNCAPPED_CALL} # uncapped-pool-ok: reason\n", id="inline-on-the-call"),
123+
pytest.param(
124+
"DeviceMemoryResource(\n dev, # uncapped-pool-ok: reason\n DeviceMemoryResourceOptions(),\n)\n",
125+
id="continuation-line-of-the-same-call",
126+
),
127+
pytest.param(
128+
"mr = DeviceMemoryResource( # uncapped-pool-ok: reason\n dev,\n DeviceMemoryResourceOptions(),\n)\n",
129+
id="first-line-of-a-multiline-statement",
130+
),
131+
# The documented use is pytest.raises, so a marker on the block header has
132+
# to keep annotating the calls inside the block.
133+
pytest.param(
134+
f"with pytest.raises(RuntimeError): # uncapped-pool-ok: reason\n {UNCAPPED_CALL}\n",
135+
id="containing-with-header",
136+
),
137+
pytest.param(f"def test_x(): # uncapped-pool-ok: reason\n {UNCAPPED_CALL}\n", id="containing-def-header"),
138+
]
139+
140+
141+
@pytest.mark.agent_authored(model="claude-opus-5")
142+
@pytest.mark.parametrize("source", MARKER_DOES_NOT_CARRY)
143+
def test_marker_that_does_not_annotate_the_call_does_not_suppress_it(tmp_path, source):
144+
"""An opt-out must be attached to the call it exempts.
145+
146+
A trailing marker annotating one statement used to exempt the statement on
147+
the next line as well -- the shape a reviewer is least likely to notice,
148+
since both lines look correctly annotated.
149+
"""
150+
assert violations_in(write(tmp_path, source))
151+
152+
153+
@pytest.mark.agent_authored(model="claude-opus-5")
154+
@pytest.mark.parametrize("source", MARKER_CARRIES)
155+
def test_marker_attached_to_the_call_still_suppresses_it(tmp_path, source):
156+
assert violations_in(write(tmp_path, source)) == []

cuda_core/tests/AGENTS.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ with pytest.raises(RuntimeError, match="IPC is not available"):
5858
DeviceMemoryResource(mempool_device, DeviceMemoryResourceOptions(ipc_enabled=True))
5959
```
6060

61+
The marker has to be attached to the statement it exempts: on a comment line
62+
directly above it, anywhere within the statement itself (including the
63+
continuation lines of a multi-line call), or on the header of a block
64+
containing it. A marker somewhere else -- trailing the *previous* statement,
65+
or separated from the call by a blank line -- does not exempt anything.
66+
6167
## Release resources at test boundaries
6268

6369
The `_init_cuda_context` fixture in `conftest.py` runs `gc.collect()` followed

0 commit comments

Comments
 (0)