Skip to content

Commit 272655f

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 4d9d4c8 commit 272655f

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
@@ -172,11 +176,16 @@ def _find_function(
172176

173177

174178
def _parse_documented_keys(desc: str) -> set[str]:
175-
"""Pull identifier tokens from any ``Available/History/Statistics keys: ...`` sentence."""
179+
"""Pull identifier tokens from any ``Available/History/Statistics keys: ...`` sentence.
180+
181+
Parenthesised notes are stripped before the section regex runs so a
182+
period inside a note (e.g. ``foo (since v1.2)``) doesn't truncate the
183+
enumeration.
184+
"""
185+
desc = _PAREN_NOTE_RE.sub("", desc)
176186
keys: set[str] = set()
177187
for m in _KEYS_SECTION_RE.finditer(desc):
178-
chunk = _PAREN_NOTE_RE.sub("", m.group(1))
179-
for tok in _IDENT_RE.finditer(chunk):
188+
for tok in _IDENT_RE.finditer(m.group(1)):
180189
keys.add(tok.group(1))
181190
return keys
182191

@@ -385,11 +394,9 @@ def test_fields_description_lists_every_emitted_key(spec: dict[str, Any]) -> Non
385394
)
386395
# Dual direction: documented keys must appear somewhere in the
387396
# scanned response builders. Catches "docstring lists key, code
388-
# deleted the assignment" — sibling drift to #1381's bug where
389-
# `notifications`/`repairs` were documented but only assigned
390-
# conditionally. (Static AST can flag "never assigned anywhere";
391-
# "assigned only inside `if x:`" requires runtime checking — see
392-
# PR #1381's `TestHaGetOverviewAlwaysEmittedKeys` for that pattern.)
397+
# deleted the assignment". Static AST flags "never assigned
398+
# anywhere"; "assigned only inside `if x:`" (conditional emission
399+
# of a documented-as-always-present key) is a runtime concern.
393400
if not spec.get("skip_dual_check"):
394401
missing_from_code = documented - emitted - _AUTO_RETAINED
395402
if missing_from_code:
@@ -402,3 +409,92 @@ def test_fields_description_lists_every_emitted_key(spec: dict[str, Any]) -> Non
402409
)
403410

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

0 commit comments

Comments
 (0)