Skip to content

Commit 473d005

Browse files
kingpanther13claude
andcommitted
test(fields-projection): harden scanner, add meta-tests, drop PR refs
Review feedback: - Rewrite module docstring + dual-check comment to lead with the substantive failure mode rather than citing PR numbers — PR archeology rots and the convention says don't reference the current task in comments. - Strip parenthesised notes BEFORE the `Available keys:` section regex so a period inside a note (e.g. `(since v1.2)`) doesn't truncate the enumeration. The old order ran paren-stripping after the section match, which can't recover from an early truncation. - Add `test_harvester_finds_dismissed_repair_count_in_ha_get_overview`: pins the bug-catch guarantee. Without it the parametrize would still pass if a refactor moved the assignment somewhere the harvester doesn't scan — both sides of the diff would shrink in lockstep and the regression would go silent. - Add `test_tool_specs_covers_every_fields_using_tool`: AST-discovers every tool function with a `fields` parameter and asserts TOOL_SPECS enumerates them. Without it, deleting a spec entry shrinks coverage silently — remaining cases still pass and the dropped tool gets no drift coverage. - Add `test_exclude_internal_keys_actually_appear_in_raw_harvest`: parametrized over specs with `exclude_internal`. If a listed internal key stops being emitted (e.g. the wrapper rename it documents was undone, or the helper was deleted), the exclusion is dead code masking nothing — surface it instead. All 9 tests pass; ruff lint+format clean; mypy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e27063f commit 473d005

1 file changed

Lines changed: 109 additions & 13 deletions

File tree

tests/src/unit/test_fields_projection_docstring_completeness.py

Lines changed: 109 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,18 @@
22
must be enumerated in the tool's ``fields=`` parameter description so AI
33
agents know it can be projected via ``fields=[...]``.
44
5-
Caught ``dismissed_repair_count`` missing from ``ha_get_overview`` — added
6-
in PR #1309 but not enumerated when PR #1225 introduced the ``Available
7-
keys: ...`` docstring list.
8-
95
The check is purely static. For each tool with a ``fields=`` parameter
106
we AST-walk the function(s) that build the projectable dict, collect
117
every string-literal key that gets assigned at the top level, and
12-
assert that set is documented.
8+
assert that set is documented in both directions:
9+
10+
- ``emitted ⊄ documented`` — code emits a key the docstring doesn't list,
11+
so AI agents reading the description never learn it can be requested.
12+
- ``documented ⊄ emitted-anywhere-in-scanned-source`` — the docstring
13+
promises a key that's no longer assigned (e.g. an assignment was
14+
removed in a refactor but the enumeration wasn't updated). Static AST
15+
flags "never assigned anywhere"; conditional-only assignment requires
16+
runtime checking.
1317
"""
1418

1519
from __future__ import annotations
@@ -168,11 +172,16 @@ def _find_function(
168172

169173

170174
def _parse_documented_keys(desc: str) -> set[str]:
171-
"""Pull identifier tokens from any ``Available/History/Statistics keys: ...`` sentence."""
175+
"""Pull identifier tokens from any ``Available/History/Statistics keys: ...`` sentence.
176+
177+
Parenthesised notes are stripped before the section regex runs so a
178+
period inside a note (e.g. ``foo (since v1.2)``) doesn't truncate the
179+
enumeration.
180+
"""
181+
desc = _PAREN_NOTE_RE.sub("", desc)
172182
keys: set[str] = set()
173183
for m in _KEYS_SECTION_RE.finditer(desc):
174-
chunk = _PAREN_NOTE_RE.sub("", m.group(1))
175-
for tok in _IDENT_RE.finditer(chunk):
184+
for tok in _IDENT_RE.finditer(m.group(1)):
176185
keys.add(tok.group(1))
177186
return keys
178187

@@ -381,11 +390,9 @@ def test_fields_description_lists_every_emitted_key(spec: dict[str, Any]) -> Non
381390
)
382391
# Dual direction: documented keys must appear somewhere in the
383392
# scanned response builders. Catches "docstring lists key, code
384-
# deleted the assignment" — sibling drift to #1381's bug where
385-
# `notifications`/`repairs` were documented but only assigned
386-
# conditionally. (Static AST can flag "never assigned anywhere";
387-
# "assigned only inside `if x:`" requires runtime checking — see
388-
# PR #1381's `TestHaGetOverviewAlwaysEmittedKeys` for that pattern.)
393+
# deleted the assignment". Static AST flags "never assigned
394+
# anywhere"; "assigned only inside `if x:`" (conditional emission
395+
# of a documented-as-always-present key) is a runtime concern.
389396
if not spec.get("skip_dual_check"):
390397
missing_from_code = documented - emitted - _AUTO_RETAINED
391398
if missing_from_code:
@@ -398,3 +405,92 @@ def test_fields_description_lists_every_emitted_key(spec: dict[str, Any]) -> Non
398405
)
399406

400407
assert not errors, f"{spec['tool']}: " + " | ".join(errors)
408+
409+
410+
# ---------------------------------------------------------------------------
411+
# Meta-tests: pin the scanner's own invariants so they can't silently drift.
412+
# ---------------------------------------------------------------------------
413+
414+
415+
def test_harvester_finds_dismissed_repair_count_in_ha_get_overview() -> None:
416+
"""The bug this whole test file exists to prevent.
417+
418+
``dismissed_repair_count`` is conditionally assigned to ``result`` inside
419+
``ha_get_overview``. If the AST harvest ever stops finding it (e.g. a
420+
refactor moves the assignment into a helper not listed in the spec, or
421+
the harvester loses subscript-assignment handling), the parametrized
422+
case for ``ha_get_overview`` would still pass — both sides of the diff
423+
would shrink in lockstep. Pin the find here so regressions surface.
424+
"""
425+
keys = _harvest_var_keys("tools/tools_search.py", "ha_get_overview", "result")
426+
assert "dismissed_repair_count" in keys, (
427+
"AST harvest of `ha_get_overview` lost `dismissed_repair_count`. "
428+
"The regression-catch guarantee this test file provides is broken."
429+
)
430+
documented = _extract_documented_keys("tools/tools_search.py", "ha_get_overview")
431+
assert "dismissed_repair_count" in documented, (
432+
"`dismissed_repair_count` was removed from `ha_get_overview`'s "
433+
"`Available keys:` enumeration. If the response key was genuinely "
434+
"removed, update this meta-test too; otherwise restore the docstring."
435+
)
436+
437+
438+
def test_tool_specs_covers_every_fields_using_tool() -> None:
439+
"""Discover every tool with a ``fields`` parameter and assert TOOL_SPECS
440+
enumerates them all.
441+
442+
Without this guard, deleting a TOOL_SPECS entry shrinks the
443+
parametrize silently — the remaining cases still pass and the dropped
444+
tool gets no drift coverage.
445+
"""
446+
tools_dir = SRC_ROOT / "tools"
447+
discovered: set[str] = set()
448+
for path in sorted(tools_dir.glob("tools_*.py")):
449+
module = ast.parse(path.read_text())
450+
for node in ast.walk(module):
451+
if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
452+
continue
453+
if not node.name.startswith("ha_"):
454+
continue
455+
for arg in (*node.args.args, *node.args.kwonlyargs):
456+
if arg.arg == "fields":
457+
discovered.add(node.name)
458+
break
459+
460+
covered = {spec["tool"] for spec in TOOL_SPECS}
461+
missing = discovered - covered
462+
assert not missing, (
463+
f"Tools with a `fields=` parameter are missing from TOOL_SPECS: "
464+
f"{sorted(missing)!r}. Add a spec entry so the drift check covers "
465+
f"them, or document why the tool is intentionally excluded."
466+
)
467+
468+
469+
@pytest.mark.parametrize(
470+
"spec",
471+
[s for s in TOOL_SPECS if s.get("exclude_internal")],
472+
ids=lambda s: s["tool"],
473+
)
474+
def test_exclude_internal_keys_actually_appear_in_raw_harvest(
475+
spec: dict[str, Any],
476+
) -> None:
477+
"""``exclude_internal`` entries should still be present in the raw
478+
AST harvest. If a key listed there stops being emitted (e.g. the
479+
helper rename it documents was undone, or the internal field was
480+
deleted), the exclusion is dead code — silently masking nothing.
481+
"""
482+
raw: set[str] = set()
483+
for mod, fn, var in spec["var_harvest"]:
484+
raw |= _harvest_var_keys(mod, fn, var)
485+
for mod, fn in spec["return_harvest"]:
486+
raw |= _harvest_return_keys(mod, fn)
487+
for mod, fn, markers in spec.get("marker_harvest", []):
488+
raw |= _harvest_marker_dicts(mod, fn, markers)
489+
490+
dead = spec["exclude_internal"] - raw
491+
assert not dead, (
492+
f"{spec['tool']}: `exclude_internal` lists key(s) {sorted(dead)!r} "
493+
f"that no longer appear in the AST harvest — the exclusion is "
494+
f"masking nothing. Either remove from `exclude_internal` or "
495+
f"investigate whether the rename it documents was undone."
496+
)

0 commit comments

Comments
 (0)