From 952a88634f93cfbc505e989a83130ba6c4d3b2f9 Mon Sep 17 00:00:00 2001 From: tzhouam Date: Tue, 29 Sep 2026 17:53:22 +0800 Subject: [PATCH] Ask for a unit test when a PR adds a public function no test names A PR can add public API with no test, and nothing in either review mode looked for that. The check has two halves. ut_coverage.py finds the public functions and methods a diff adds and whether any test file names them, in the diff or, when a checkout is available, in the reviewed tree. A name with no reference is only a candidate. The reviewer reads each one and decides whether it is covered through a caller, trivial, or a real gap, and only a real gap becomes a comment. Strict gets the candidates as an evidence section next to the changed-symbol consumers, searched against the PR-time worktree. After dispositions are final, at most three comments anchored on a candidate def publish; the rest are named in one summary line. Direct gets them from an optional diff on direct_review_plan, the SDK request and the MCP review tool; the provider has no checkout of the head, so the plan lists diff-only candidates and a checklist step has the agent search the head's tests, or find the functions itself when no diff was passed. What counts as a source, a test or an exclusion is adapter data (ut_coverage: in the manifest), so the core stays repo-neutral. Fixes #164 Signed-off-by: tzhouam Co-Authored-By: Claude Opus 5.5 --- adapters/vllm_omni/manifest.yaml | 8 + doc/architecture/SPEC/direct_routing.md | 6 +- doc/architecture/SPEC/engine/steps/review.md | 4 + doc/architecture/SPEC/sdk.md | 6 +- doc/architecture/SPEC/thin_mcp_server.md | 4 +- doc/architecture/SPEC/ut_coverage.md | 45 +++ src/infermatrix_copilot/direct_routing.py | 25 ++ .../engine/steps/review/steps.py | 27 +- src/infermatrix_copilot/sdk/v1/direct.py | 2 + src/infermatrix_copilot/sdk/v1/models.py | 6 + src/infermatrix_copilot/thin_mcp_server.py | 5 + src/infermatrix_copilot/ut_coverage.py | 310 ++++++++++++++++++ test/test_thin_mcp_server.py | 2 + test/test_ut_coverage.py | 291 ++++++++++++++++ 14 files changed, 736 insertions(+), 5 deletions(-) create mode 100644 doc/architecture/SPEC/ut_coverage.md create mode 100644 src/infermatrix_copilot/ut_coverage.py create mode 100644 test/test_ut_coverage.py diff --git a/adapters/vllm_omni/manifest.yaml b/adapters/vllm_omni/manifest.yaml index 82e4cc37..7abf994a 100644 --- a/adapters/vllm_omni/manifest.yaml +++ b/adapters/vllm_omni/manifest.yaml @@ -423,3 +423,11 @@ knowledge_lifecycle: protected_rules: [] calibration_set: kb-calibration # adapters/vllm_omni/kb-calibration/cases/*.json model_dir: repos/vllm-omni/models + +# Unit-test coverage candidates for new public functions (#164). Sources are +# the package; tests live under tests/. Entry points and generated/registry +# files are skipped: their coverage is end-to-end by design. +ut_coverage: + source_roots: [vllm_omni/] + test_globs: ["tests/*", "*/test_*.py", "*/conftest.py"] + exclude: ["*/__init__.py", "vllm_omni/entrypoints/cli/*", "vllm_omni/version.py"] diff --git a/doc/architecture/SPEC/direct_routing.md b/doc/architecture/SPEC/direct_routing.md index 355e0738..920c3e9c 100644 --- a/doc/architecture/SPEC/direct_routing.md +++ b/doc/architecture/SPEC/direct_routing.md @@ -1,6 +1,6 @@ # direct_routing.py —— 规范 - + `LOC ~880 · Direct 模式完整策略包与仓库中立的知识路由 · refactor-status: stable` @@ -47,6 +47,10 @@ Direct policy bundle）、`direct_knowledge_routes`、 `evidence_head_sha` 7–40 位十六进制、`existing_feedback_status` 枚举、 `finding_dispositions` 的 anchor/disposition/existing_thread/ head_recheck 约束。 +- **未测公开函数候选（#164）**：`direct_review_plan(..., diff="")` 有 diff 时带 + `untested_public_api`（`status=ok` + 候选 + 评审规则），只基于 diff 自身的测试 + 文件——provider 没有 PR head 的 checkout，树内测试搜索由 agent 按 checklist 完成； + 无 diff 为 `{"status": "no_diff"}`，adapter 关闭时为 `disabled`。 - 叶子模块：**绝不** import 任何 server 模块 （`test_contract.py::test_direct_routing_does_not_import_a_server_module`）。 diff --git a/doc/architecture/SPEC/engine/steps/review.md b/doc/architecture/SPEC/engine/steps/review.md index 9897772d..b1ee0b60 100644 --- a/doc/architecture/SPEC/engine/steps/review.md +++ b/doc/architecture/SPEC/engine/steps/review.md @@ -54,6 +54,10 @@ agent、评审结果的有界精炼、评测调优过的 prompt 数据和确定 要做的改动、`evidence` 引用该 `[resolved]` 行）；覆盖补充（coverage editor） 的输入也剔除 `[resolved]` 行。此前按关键词从散文里“提拔”残留，会把明确 写着不再追究的行发成 P2（#155）。 +- **未测公开函数（#164）**：evidence 带 `untested_public_api`——`ut_coverage.analyze` + 在 PR-time worktree 上找出无测试引用的新增公开函数，由 lens 判断间接覆盖/trivial/ + 真缺口；评审者把缺测试评论标为 `kind: untested_api`；finalize 之后 `cap_gap_comments` + 只对这类评论至多保留 3 条，其余名字追加到 summary，其他发现不受影响。 - **`review_verdict` 是发布的 state 字段，不是散文**：由 `_review_verdict(review_comments, pr_state)` 计算（**与渲染器同一个 helper，绝不第二份校准规则**），随 `review_text`/`review_summary`/ diff --git a/doc/architecture/SPEC/sdk.md b/doc/architecture/SPEC/sdk.md index 36f0a03d..98cb04a8 100644 --- a/doc/architecture/SPEC/sdk.md +++ b/doc/architecture/SPEC/sdk.md @@ -1,6 +1,6 @@ # sdk/ —— 规范 - + `Python SDK v1 · 跨仓库唯一 typed 边界 · refactor-status: ok` @@ -133,3 +133,7 @@ Direct 1.1 / Strict 1.3 accept typed `CarriedFinding` inputs (unique IDs, source `DirectReviewPlan` 新增 `knowledge_snapshot`（激活快照 ID、`packaged` 或 `unverified`，从不暴露开发树路径） 与 `knowledge_tree_sha256`（已验证快照的清单树哈希，否则为空）。同一 `review_context_id` 下的文档读取都来自这份 快照，RB 应随每次 review 记录这两个值。 + +## 2026-09-29 未测公开函数候选 +`DirectReviewRequest` 新增可选 `diff`（冻结 head 的 unified diff），`DirectReviewPlan` 新增 +`untested_public_api`（`direct_routing` 的同名块，默认空字典）。两者都是可选增量字段，旧调用方不受影响（#164）。 diff --git a/doc/architecture/SPEC/thin_mcp_server.md b/doc/architecture/SPEC/thin_mcp_server.md index 11186d3d..5f7ffb51 100644 --- a/doc/architecture/SPEC/thin_mcp_server.md +++ b/doc/architecture/SPEC/thin_mcp_server.md @@ -1,6 +1,6 @@ # thin_mcp_server.py —— 规范 - + `LOC ~490 · 默认 MCP：Direct 门面 + Strict 入口 · refactor-status: ok` @@ -26,6 +26,8 @@ 委托 —— 没有任何东西从那两个模块向上 import 回 server。下面关于 quick-map fail-closed、路由不静默替换、仓库守卫先跑的不变量**仍然为真**， 但其实现体在 `direct_routing.py`（规范见其页）。 +- `review` 的可选 `diff` 只对 Direct 生效：原样交给 `direct_review_plan`， + 由其生成 `untested_public_api` 候选（#164）；仍然零模型。 - **Strict 分支透传快照绑定**：`_strict_review_request` 把 `expected_head_sha`、`repo_path`、`idempotency_key` 一并送进内部请求； `review()` 的 Strict 路径按 `strict_readiness(repo, repo_path)`（两参， diff --git a/doc/architecture/SPEC/ut_coverage.md b/doc/architecture/SPEC/ut_coverage.md new file mode 100644 index 00000000..4b957da7 --- /dev/null +++ b/doc/architecture/SPEC/ut_coverage.md @@ -0,0 +1,45 @@ +# ut_coverage.py —— 规范 + + + +`LOC ~290 · 新增公开函数的单测覆盖候选（#164） · refactor-status: ok` + +## 职责 +两段式检查的**确定性一半**：从 PR diff 找出新增的公开函数/方法，再看 diff 中的 +测试文件、以及（有 checkout 时）被评审树的测试文件是否按名字引用它们。没有任何 +测试引用的名字只是**候选**，从不直接判定为“缺测试”——由评审者（Strict lens 或 +Direct agent）逐个判断：经调用方被间接覆盖、trivial，还是真缺口。 + +## 公开契约 +`UTCoverageRules`（`from_manifest(manifest)`：读 adapter 的 `ut_coverage:` +段——`source_roots`、`test_globs`、`exclude`、`enabled`；非 Python 仓库或 +`enabled: false` 返回 `None`）；`PublicDef`；`CoverageReport`； +`new_public_defs(diff, rules)`；`analyze(diff, rules, repo=None)`； +`render(report)`（Strict evidence 文本，无候选时为空）； +`cap_gap_comments(comments, report)`；`REVIEWER_INSTRUCTIONS`； +`MAX_CANDIDATES=15`；`MAX_COMMENTS=3`。 + +## 不变量 +- **只提名，不裁决**：模块不产生评论；评论只来自评审者对候选的判断。 +- 新增 = diff 中 `+def` 且同一**限定名**（`类.方法` 或模块函数名）没有在 diff + 任何位置被删除（移动、改签名不算新 API）；另一个类的同名方法不受影响。跳过：`_` 开头与 dunder 名、嵌套函数、`@overload`/ + `@abstractmethod` 桩、测试文件、`exclude` 命中的文件；私有类的方法跳过。 +- 方法判定只看同一 hunk 内可见的 `class`（上下文或新增行）或 hunk 头；看不到 + 父作用域的缩进 def 按模块函数处理。 +- 树内搜索用 `git grep -l -w -F`，只计入 `test_globs` 命中的文件，逐名限时； + 失败按“无引用”处理（只会多提名，交给评审者判断）。 +- 发布上限在评审之后确定性执行，且只作用于评审者**显式分类**为 + `kind: untested_api` 的评论：最多保留 3 条，其余的名字进 summary 一行，不丢失。 + 同一函数上的其他发现（不论锚点、严重度）从不被截断；`kind` 是内部字段， + 发布前一律移除。 +- 仓库中立：仓库差异全部来自 adapter 的 `ut_coverage:` 段。 + +## 边界 —— 不属于这里 +评审 prompt 与评论发布（`engine/steps/review/`）；Direct 策略包 +（`direct_routing.py`）；运行任何测试或覆盖率工具。 + +## 依赖（允许） +标准库（`re`、`subprocess`、`fnmatch`、`dataclasses`）。 + +## 测试 +`test/test_ut_coverage.py`。 diff --git a/src/infermatrix_copilot/direct_routing.py b/src/infermatrix_copilot/direct_routing.py index 02af7d12..ef860dc9 100644 --- a/src/infermatrix_copilot/direct_routing.py +++ b/src/infermatrix_copilot/direct_routing.py @@ -28,6 +28,8 @@ from .adapters import AdapterError, AdapterRegistry, RepoAdapter from .knowledge_view import KnowledgeView from .sdk._resources import adapters_root +from .ut_coverage import UTCoverageRules, analyze as _ut_analyze +from .ut_coverage import REVIEWER_INSTRUCTIONS as _UT_INSTRUCTIONS _ROOT = Path(__file__).resolve().parents[2] _ADAPTERS = adapters_root() @@ -158,6 +160,7 @@ def _validate_routes(data: object, relative: str, view: KnowledgeView) -> dict: "Classify every candidate finding as new, duplicate, extends_existing, or resolved_or_outdated. Suppress duplicates; for extensions, point to the existing thread instead of opening a parallel inline comment. Reverify resolved/outdated concerns at the pinned head and suppress them only when fixed. Use disabled only for PR_CONTEXT_MODE=no_discussion evaluation, record unavailable feedback as a validation gap, and use not_applicable only for local/worktree reviews.", "Run subtraction only when the diff adds or expands a helper, class, fallback, compatibility branch, or public behavior; otherwise mark no subtraction signal.", "When subtraction is triggered, read the mandatory simplification guide and prove consumers, trust boundaries, and lifecycle ownership before calling code dead or over-defensive.", + "When the plan's untested_public_api lists candidates, judge each new public function as indirectly covered, trivial, or a real unit-test gap after reading it and searching the head's tests; when the host passed no diff, list the diff's new public functions and search the tests yourself. File at most three real gaps as minor findings on the def's file:line and name any further ones in one line.", "Report the non-test added-line count the host supplies rather than counting the diff yourself; above the stated budget, ask for a split plan or a concrete exemption on the largest contributing file, and record the reason instead when the PR body already gives one. Size is never a correctness finding and never raises another finding's severity.", "Plan exactly one consolidated final review comment.", ) @@ -586,9 +589,14 @@ def direct_review_plan( body: str = "", changed_files: list[str] | None = None, view: KnowledgeView | None = None, + diff: str = "", ) -> dict: """Return the complete Direct policy bundle for one frozen review. + ``diff`` is optional: with it, the bundle carries the new public + functions no test in the diff names (#164) for the agent to judge; without + it, the checklist asks the agent to find them itself. + This is the canonical provider operation used by both the Python SDK and the MCP adapter. Keeping the full bundle here prevents downstream hosts from reconstructing a smaller, divergent protocol out of routing helpers. @@ -620,6 +628,7 @@ def direct_review_plan( ), ) budget_ms = int((time.perf_counter() - budget_started) * 1000) + untested = _direct_untested_public_api(repo, diff) return { "mode": "direct", "knowledge_entry": ( @@ -648,6 +657,7 @@ def direct_review_plan( "fallback_entry": _knowledge_path("AGENTS.md", view), }, "execution_budget": execution_budget, + "untested_public_api": untested, "first_review_checklist": list(DIRECT_REVIEW_CHECKLIST), "progress_update": { **DIRECT_PROGRESS_UPDATE, @@ -697,6 +707,21 @@ def direct_review_plan( } +def _direct_untested_public_api(repo: str, diff: str) -> dict: + """Candidates from the diff alone: the provider holds no checkout of the + PR head, so the head-tree test search is the agent's (see checklist).""" + if not diff: + return {"status": "no_diff"} + adapter = _adapter_for_repo(repo) + rules = UTCoverageRules.from_manifest( + adapter.manifest if adapter is not None else None) + if rules is None: + return {"status": "disabled"} + report = _ut_analyze(diff, rules) + return {"status": "ok", **report.to_dict(), + "instructions": _UT_INSTRUCTIONS if report.candidates else ""} + + def _direct_completion_result( subtraction_signal: str = "", subtraction: list[dict[str, str]] | None = None, diff --git a/src/infermatrix_copilot/engine/steps/review/steps.py b/src/infermatrix_copilot/engine/steps/review/steps.py index 0d3df6cf..6feb44e2 100644 --- a/src/infermatrix_copilot/engine/steps/review/steps.py +++ b/src/infermatrix_copilot/engine/steps/review/steps.py @@ -19,6 +19,8 @@ from pathlib import Path from ....review.planner import DEFAULT_STANDARD_LENSES, DEPTHS, plan_review +from ....ut_coverage import UTCoverageRules, analyze as _ut_analyze +from ....ut_coverage import cap_gap_comments, render as _ut_render from ...step import FailureKind, StepContext, StepResult from .._common import gh_read_tools as _gh_read_tools from .._common import repo_path as _repo_path @@ -145,6 +147,17 @@ async def _review_diff(ctx: StepContext) -> StepResult: except OSError: continue + # #164: new public functions no test names — candidates for the lenses + # to judge (indirect coverage, trivial, or a real gap), never findings + ut_rules = UTCoverageRules.from_manifest( + adapter.manifest if adapter is not None else None) + ut_report = (await asyncio.to_thread(_ut_analyze, str(diff), ut_rules, + _repo_path(ctx)) + if ut_rules is not None else None) + if ut_report is not None: + ctx.trace.record("review_ut_candidates", step="agent.review_diff", + **ut_report.to_dict()) + common = dict( step_name="agent.review_diff", purpose=f"Review PR #{spec.get('pr')} like an engaged maintainer: " @@ -158,14 +171,18 @@ async def _review_diff(ctx: StepContext) -> StepResult: "gate_report": ctx.state.get("gate_report", ""), "sweep_targets": _sweep_targets(str(diff), language), "changed_symbol_consumers": await asyncio.to_thread( - _consumer_sweep, _repo_path(ctx), str(diff))}, + _consumer_sweep, _repo_path(ctx), str(diff)), + "untested_public_api": (_ut_render(ut_report) + if ut_report is not None else "")}, output_extension={"review_comments": "list of {file, line, anchor_snippet, severity: " "blocker|major|minor|nit, comment, evidence, " "suggestion: optional replacement code for the " "cited lines — the concrete edit, no prose, " "disposition: publish|excluded|duplicate|resolved|" - "no_issue — only publish is published}"}, + "no_issue — only publish is published, kind: " + "optional, `untested_api` only for a missing unit " + "test on a listed untested-function candidate}"}, extra_tools={**_gh_read_tools(_repo_path(ctx)), **review_repo_tools(_repo_path(ctx))}, ) @@ -301,6 +318,12 @@ async def _review_diff(ctx: StepContext) -> StepResult: # finalized set (#141). all_candidates = list(output.get("review_comments") or []) published, withheld = finalize_review_dispositions(output) + # at most three untested-function comments; the rest are named, not lost + published, ut_dropped = cap_gap_comments(published, ut_report) + if ut_dropped: + output["summary"] = (str(output.get("summary") or "").rstrip() + + "\n\nAlso without a unit test: " + + ", ".join(f"`{n}`" for n in ut_dropped) + ".").lstrip() output["review_comments"] = published output["_withheld_findings"] = withheld comments = sorted(output.get("review_comments") or [], diff --git a/src/infermatrix_copilot/sdk/v1/direct.py b/src/infermatrix_copilot/sdk/v1/direct.py index 26e97902..92e45859 100644 --- a/src/infermatrix_copilot/sdk/v1/direct.py +++ b/src/infermatrix_copilot/sdk/v1/direct.py @@ -312,6 +312,7 @@ def plan(self, request: DirectReviewRequest) -> DirectReviewPlan: body=request.body, changed_files=changed_files, view=view, + diff=request.diff, ) routes = tuple( KnowledgeRoute( @@ -380,6 +381,7 @@ def plan(self, request: DirectReviewRequest) -> DirectReviewPlan: carried_findings=request.carried_findings, knowledge_snapshot=view.public_snapshot, knowledge_tree_sha256=view.tree_sha256, + untested_public_api=dict(raw.get("untested_public_api") or {}), ) self._remember_context(review_context_id, expected_head, carried, view) return plan diff --git a/src/infermatrix_copilot/sdk/v1/models.py b/src/infermatrix_copilot/sdk/v1/models.py index 8306bab9..d8ad8f9d 100644 --- a/src/infermatrix_copilot/sdk/v1/models.py +++ b/src/infermatrix_copilot/sdk/v1/models.py @@ -229,6 +229,9 @@ class DirectReviewRequest(_Serializable): body: str changed_paths: tuple[ChangedPath, ...] carried_findings: tuple[CarriedFinding, ...] = () + # optional unified diff of the frozen head; enables the provider's + # untested-public-function candidates (#164) + diff: str = "" @dataclass(frozen=True) @@ -254,6 +257,9 @@ class DirectReviewPlan(_Serializable): # review_context_id) was served from: record it with the review knowledge_snapshot: str = "" knowledge_tree_sha256: str = "" + # new public functions no test in the request's diff names (#164); + # {"status": "no_diff"} when the request carried no diff + untested_public_api: dict[str, Any] = field(default_factory=dict) FeedbackStatus = Literal[ "checked", "disabled", "unavailable", "not_applicable" diff --git a/src/infermatrix_copilot/thin_mcp_server.py b/src/infermatrix_copilot/thin_mcp_server.py index 716b2c11..88b40b56 100644 --- a/src/infermatrix_copilot/thin_mcp_server.py +++ b/src/infermatrix_copilot/thin_mcp_server.py @@ -273,6 +273,7 @@ def review( repo_path: str = "", expected_head_sha: str = "", idempotency_key: str = "", + diff: str = "", ) -> dict: """Begin a Direct or Strict review. @@ -289,6 +290,9 @@ def review( ``expected_head_sha`` (Strict only) pins the review to one snapshot: pass the full 40-hex head you observed, and the run stops as stale rather than reviewing a different commit if the PR moved in between. + ``diff`` (Direct only, optional) is the frozen unified diff; with it the + plan lists the new public functions no test in the diff names + (``untested_public_api``) for you to judge. ``idempotency_key`` (Strict only) makes a retry safe: pass a stable id for the attempt and a repeated call returns the SAME run — including a finished one, whose result you can read — instead of starting a second @@ -311,6 +315,7 @@ def run() -> dict: title=title, body=body, changed_files=changed_files, + diff=diff, ) if post: diff --git a/src/infermatrix_copilot/ut_coverage.py b/src/infermatrix_copilot/ut_coverage.py new file mode 100644 index 00000000..916bb737 --- /dev/null +++ b/src/infermatrix_copilot/ut_coverage.py @@ -0,0 +1,310 @@ +"""Unit-test coverage signal for new public functions in a PR (#164). + +Deterministic half of a two-part check. This module finds the public +functions and methods a diff ADDS and whether any test file references them +by name — in the diff itself or, when a checkout is available, anywhere in +the reviewed tree. It never decides that a function is untested: a name with +no test reference is only a CANDIDATE, and the reviewer (Strict lenses or the +Direct agent) judges each one — covered indirectly through a caller, trivial, +or a real gap — before anything is published. Repository specifics (which +files are sources, which are tests, what to skip) are adapter data +(`ut_coverage:` in the manifest), so the core stays repo-neutral. +""" + +from __future__ import annotations + +import re +import subprocess +from dataclasses import dataclass, field +from fnmatch import fnmatch + +# At most this many candidates are handed to a reviewer, and at most +# MAX_COMMENTS of its confirmed gaps publish as comments; the rest are named +# in one summary line. +MAX_CANDIDATES = 15 +MAX_COMMENTS = 3 + +_DEFAULT_TEST_GLOBS = ("tests/*", "test/*", "*/tests/*", "*/test/*", + "test_*.py", "*/test_*.py", "*_test.py", "conftest.py", + "*/conftest.py") +_DEF = re.compile(r"^(?P[ \t]*)(?:async[ \t]+)?def[ \t]+" + r"(?P[A-Za-z_]\w*)[ \t]*\(") +_CLASS = re.compile(r"^(?P[ \t]*)class[ \t]+(?P[A-Za-z_]\w*)") +_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(?P\d+)(?:,\d+)? @@(?P.*)$") +_SKIP_DECORATORS = ("@overload", "@typing.overload", "@abstractmethod", + "@abc.abstractmethod") + + +@dataclass(frozen=True) +class UTCoverageRules: + source_roots: tuple[str, ...] = () + test_globs: tuple[str, ...] = _DEFAULT_TEST_GLOBS + exclude: tuple[str, ...] = () + + @classmethod + def from_manifest(cls, manifest: dict | None) -> "UTCoverageRules | None": + """None when the adapter disables the check (`enabled: false`) or its + language is not Python — the extractor only reads Python defs.""" + manifest = manifest or {} + language = str((manifest.get("repo") or {}).get("language") or "python") + section = manifest.get("ut_coverage") or {} + if language.lower() != "python" or section.get("enabled") is False: + return None + return cls( + source_roots=tuple(str(p) for p in section.get("source_roots") or ()), + test_globs=tuple(str(p) for p in section.get("test_globs") or ()) + or _DEFAULT_TEST_GLOBS, + exclude=tuple(str(p) for p in section.get("exclude") or ()), + ) + + def is_test(self, path: str) -> bool: + return any(fnmatch(path, g) for g in self.test_globs) + + def is_source(self, path: str) -> bool: + if not path.endswith(".py") or self.is_test(path): + return False + if any(fnmatch(path, g) for g in self.exclude): + return False + return not self.source_roots or path.startswith(self.source_roots) + + +@dataclass(frozen=True) +class PublicDef: + path: str + line: int + name: str + owner: str = "" # enclosing class for a method + + @property + def qualname(self) -> str: + return f"{self.owner}.{self.name}" if self.owner else self.name + + +@dataclass +class CoverageReport: + candidates: list[PublicDef] = field(default_factory=list) + referenced: dict[str, list[str]] = field(default_factory=dict) + searched_tree: bool = False + truncated: int = 0 + + def to_dict(self) -> dict: + return { + "candidates": [{"file": d.path, "line": d.line, "name": d.qualname} + for d in self.candidates], + "referenced": {k: v for k, v in self.referenced.items()}, + "searched_tree": self.searched_tree, + "truncated": self.truncated, + } + + +def _is_public(name: str) -> bool: + return not name.startswith("_") + + +def _files(diff: str): + """Yield (path, lines) per file of a unified diff. The path is the b/ + side, or the a/ side for a deleted file: its removed defs still matter, + because a function moved out of a deleted module is not new API.""" + path, old_path, lines = "", "", [] + for line in diff.splitlines(): + if line.startswith("diff --git "): + if path: + yield path, lines + path, old_path, lines = "", "", [] + elif line.startswith("--- ") and not path: + source = line[4:].strip() + old_path = "" if source == "/dev/null" else source.removeprefix("a/") + elif line.startswith("+++ ") and not path: + target = line[4:].strip() + path = old_path if target == "/dev/null" else target.removeprefix("b/") + elif path: + lines.append(line) + if path: + yield path, lines + + +def _side_defs(lines: list[str], side: str): + """Walk one side of a file's hunks ("+" = new, "-" = old) and yield + (new_line, qualname, owner, name, previous_line, nested, owner_public) + for every def written on that side. Scope comes from the hunk header and + the lines visible on that side (context + that side's own lines), so a + method is named by its enclosing class, never by its bare name.""" + other = "-" if side == "+" else "+" + new_line = 0 + scope: list[tuple[int, str, str]] = [] # (indent, kind, name) + previous = "" + for raw in lines: + hunk = _HUNK.match(raw) + if hunk: + new_line = int(hunk.group("start")) + scope = [] + # git writes one space, then the context line WITH its source + # indentation; keep that indentation or a method header would + # look like a module-level def and its siblings like nested ones + header = hunk.group("ctx").removeprefix(" ").rstrip() + m = _CLASS.match(header) or _DEF.match(header) + if m: + kind = "class" if header.lstrip().startswith("class") else "def" + scope.append((len(m.group("indent")), kind, m.group("name"))) + previous = "" + continue + if raw.startswith((other, "\\")): # the other side / "\ No newline" + continue + text = raw[1:] if raw[:1] in ("+", "-", " ") else raw + own = raw.startswith(side) + if text.strip(): + indent = len(text) - len(text.lstrip()) + m_class, m_def = _CLASS.match(text), _DEF.match(text) + if m_class or m_def: + while scope and scope[-1][0] >= indent: + scope.pop() + if m_def and own: + name = m_def.group("name") + parent = scope[-1] if scope else None + owner = parent[2] if parent is not None and parent[1] == "class" else "" + nested = parent is not None and parent[1] == "def" + yield (new_line, f"{owner}.{name}" if owner else name, owner, + name, previous, nested) + if m_class: + scope.append((indent, "class", m_class.group("name"))) + elif m_def: + scope.append((indent, "def", m_def.group("name"))) + previous = text + if side == "+": + new_line += 1 + + +def new_public_defs(diff: str, rules: UTCoverageRules) -> list[PublicDef]: + """Public functions and methods the diff adds to source files. + + A def is a method when an enclosing `class` is visible in the same hunk + (context or added lines) or named in the hunk header; an indented def with + a `def` as its visible parent is a nested helper and is skipped. Private + names, dunders, and @overload/@abstractmethod stubs are skipped. A def + whose qualified name (class + method, or module function) the diff also + removes is an edit or a move, not new API; a same-named method of a + DIFFERENT class is unrelated and still counts.""" + files = list(_files(diff)) + removed = {qual for _path, lines in files + for (_l, qual, *_rest) in _side_defs(lines, "-")} + found: list[PublicDef] = [] + for path, lines in files: + if not rules.is_source(path): + continue + for line, qual, owner, name, previous, nested in _side_defs(lines, "+"): + if (_is_public(name) and not nested and qual not in removed + and not previous.strip().startswith(_SKIP_DECORATORS) + and (not owner or _is_public(owner))): + found.append(PublicDef(path, line, name, owner)) + return found + + +def _word(name: str) -> re.Pattern[str]: + return re.compile(rf"\b{re.escape(name)}\b") + + +def _diff_test_references(diff: str, rules: UTCoverageRules, + defs: list[PublicDef]) -> dict[str, list[str]]: + hits: dict[str, list[str]] = {} + for path, lines in _files(diff): + if not rules.is_test(path): + continue + added = "\n".join(l[1:] for l in lines if l.startswith("+")) + for d in defs: + if _word(d.name).search(added): + hits.setdefault(d.qualname, []).append(path) + return hits + + +def _tree_test_references(repo: str, rules: UTCoverageRules, name: str, + timeout: float) -> list[str]: + try: + r = subprocess.run(["git", "grep", "-l", "-w", "-F", "-e", name], + cwd=repo, capture_output=True, text=True, + timeout=timeout) + except (OSError, subprocess.SubprocessError): + return [] + return [p for p in (r.stdout or "").splitlines() if rules.is_test(p)][:3] + + +def analyze(diff: str, rules: UTCoverageRules, repo: str | None = None, + *, timeout: float = 10.0) -> CoverageReport: + """Candidates = new public defs with no test reference in the diff nor, + when `repo` (the reviewed checkout) is given, in its test files.""" + report = CoverageReport(searched_tree=bool(repo)) + defs = new_public_defs(diff, rules) + report.referenced = _diff_test_references(diff, rules, defs) + for d in defs: + if d.qualname in report.referenced: + continue + if repo: + tree = _tree_test_references(repo, rules, d.name, timeout) + if tree: + report.referenced[d.qualname] = tree + continue + report.candidates.append(d) + report.truncated = max(0, len(report.candidates) - MAX_CANDIDATES) + report.candidates = report.candidates[:MAX_CANDIDATES] + return report + + +REVIEWER_INSTRUCTIONS = ( + "For EACH candidate, read the function and search the tests (by its " + "name and by its callers) before deciding: (a) covered indirectly — a " + "test exercises it through a caller; name that test; (b) trivial — pure " + "delegation, a constant, or a stub, where a dedicated test adds nothing; " + "or (c) a real gap. File each real gap as ONE comment: disposition " + "`publish`, severity `minor`, `kind: untested_api`, anchored on the " + "def's file:line given here, naming the behavior a unit test should " + "pin. Any other finding about the same function is a separate comment " + "without that kind. At most " + f"{MAX_COMMENTS} such comments; name any further gaps in one summary " + "line. Never file (a) or (b).") + + +def render(report: CoverageReport) -> str: + """The reviewer-facing evidence block; empty when there is nothing to + judge.""" + if not report.candidates: + return "" + where = ("the diff and the reviewed tree's test files" if report.searched_tree + else "the diff's test files") + lines = [f"- {d.path}:{d.line} `{d.qualname}`" for d in report.candidates] + if report.truncated: + lines.append(f"- … {report.truncated} more not listed") + return ("NEW PUBLIC FUNCTIONS WITH NO TEST REFERENCE — candidates only: " + f"no file in {where} names them.\n" + "\n".join(lines) + + "\n" + REVIEWER_INSTRUCTIONS) + + +GAP_KIND = "untested_api" + + +def cap_gap_comments(comments: list[dict], report: CoverageReport | None + ) -> tuple[list[dict], list[str]]: + """Keep at most MAX_COMMENTS comments the reviewer classified as a + missing unit test (`kind: untested_api`); return (kept comments, names of + the dropped gaps) so the caller can name them in the summary instead of + losing them. Only that explicit classification counts: any other finding + on the same function, whatever its anchor or severity, is never capped. + The `kind` field is internal and is removed from every comment.""" + anchors = {(d.path, d.line): d.qualname + for d in (report.candidates if report is not None else [])} + kept: list[dict] = [] + dropped: list[str] = [] + seen = 0 + for c in comments: + is_gap = str(c.pop("kind", "") or "").strip().lower() == GAP_KIND + if not is_gap: + kept.append(c) + continue + seen += 1 + if seen <= MAX_COMMENTS: + kept.append(c) + continue + f = str(c.get("file") or "") + dropped.append(next( + (name for (p, l), name in anchors.items() + if l == c.get("line") and f and (f.endswith(p) or p.endswith(f))), + f"{f}:{c.get('line')}")) + return kept, dropped diff --git a/test/test_thin_mcp_server.py b/test/test_thin_mcp_server.py index 7f6fec9f..c8edca37 100644 --- a/test/test_thin_mcp_server.py +++ b/test/test_thin_mcp_server.py @@ -126,12 +126,14 @@ def test_direct_entrypoints_do_not_resolve_repo(monkeypatch): "routing", "navigation_policy", "execution_budget", + "untested_public_api", "first_review_checklist", "progress_update", "completion_gate", "diagnostics", } assert review["mode"] == "direct" + assert review["untested_public_api"] == {"status": "no_diff"} assert core.requests == [] assert Path(review["knowledge_entry"]).parts[-2:] == ("knowledge", "AGENTS.md") assert review["knowledge_routes"] == [] diff --git a/test/test_ut_coverage.py b/test/test_ut_coverage.py new file mode 100644 index 00000000..ac2a3c57 --- /dev/null +++ b/test/test_ut_coverage.py @@ -0,0 +1,291 @@ +"""#164: unit-test coverage candidates for new public functions. + +The deterministic half only nominates; the reviewer judges. These tests pin +what gets nominated, what the reviewer is shown in each mode, and the +three-comment cap.""" + +import asyncio +import json +import subprocess + +from infermatrix_copilot.ut_coverage import ( + MAX_COMMENTS, + CoverageReport, + PublicDef, + UTCoverageRules, + analyze, + cap_gap_comments, + new_public_defs, + render, +) + +RULES = UTCoverageRules(source_roots=("pkg/",), exclude=("*/__init__.py",)) + +DIFF = """\ +diff --git a/pkg/sched.py b/pkg/sched.py +--- a/pkg/sched.py ++++ b/pkg/sched.py +@@ -10,3 +10,22 @@ class Scheduler: + def existing(self): + return 1 + ++ def admit(self, req): ++ def _inner(): ++ return req ++ return _inner() ++ ++ def _private(self): ++ return 0 ++ ++ def __repr__(self): ++ return "S" ++ ++ ++def plan_batch(items): ++ def helper(x): ++ return x ++ return [helper(i) for i in items] ++ ++@overload ++def plan_batch_typed(items): ... +diff --git a/pkg/moved.py b/pkg/moved.py +--- a/pkg/moved.py ++++ b/pkg/moved.py +@@ -1,2 +1,2 @@ +-def relocate(x): ++def relocate(x, y=0): + return x +diff --git a/pkg/__init__.py b/pkg/__init__.py +--- a/pkg/__init__.py ++++ b/pkg/__init__.py +@@ -1 +1,3 @@ + import os ++def exported(): ++ return os +diff --git a/tests/test_sched.py b/tests/test_sched.py +--- /dev/null ++++ b/tests/test_sched.py +@@ -0,0 +1,3 @@ ++from pkg.sched import plan_batch ++def test_plan_batch(): ++ assert plan_batch([1]) == [1] +""" + + +def test_only_new_public_source_defs_are_nominated(): + defs = {(d.qualname, d.line) for d in new_public_defs(DIFF, RULES)} + # admit is a method (class named in the hunk header); plan_batch is a + # module function. Skipped: the nested defs, private and dunder names, + # the @overload stub, a def edited in place, __init__.py, and tests. + assert defs == {("Scheduler.admit", 13), ("plan_batch", 25)} + + +def test_an_edit_to_one_class_does_not_hide_a_new_method_of_another(): + """Removal matching is by qualified name: changing Existing.forward's + signature is an edit, but NewModel.forward in another file is new API.""" + diff = """\ +diff --git a/pkg/old.py b/pkg/old.py +--- a/pkg/old.py ++++ b/pkg/old.py +@@ -3,2 +3,2 @@ class Existing: +- def forward(self, x): ++ def forward(self, x, y=None): + return x +diff --git a/pkg/new.py b/pkg/new.py +--- /dev/null ++++ b/pkg/new.py +@@ -0,0 +1,3 @@ ++class NewModel: ++ def forward(self, x): ++ return x +""" + defs = [(d.qualname, d.path, d.line) for d in new_public_defs(diff, RULES)] + assert defs == [("NewModel.forward", "pkg/new.py", 2)] + + +def test_a_sibling_of_the_hunk_header_method_is_not_nested(): + """The hunk header keeps its source indentation: under a header naming + an existing method, a new method at the same indentation is a sibling.""" + diff = """\ +diff --git a/pkg/svc.py b/pkg/svc.py +--- a/pkg/svc.py ++++ b/pkg/svc.py +@@ -20,2 +20,5 @@ def existing(self): + return 1 ++ ++ def added(self): ++ return 2 +""" + assert [(d.name, d.line) for d in new_public_defs(diff, RULES)] == [ + ("added", 22)] + + +def test_a_function_moved_out_of_a_deleted_module_is_not_new(): + """A deleted file's removed defs count for move detection, so moving + `move()` out of a module the PR deletes nominates nothing.""" + diff = """\ +diff --git a/pkg/old.py b/pkg/old.py +deleted file mode 100644 +--- a/pkg/old.py ++++ /dev/null +@@ -1,2 +0,0 @@ +-def move(x): +- return x +diff --git a/pkg/new.py b/pkg/new.py +--- a/pkg/new.py ++++ b/pkg/new.py +@@ -1 +1,4 @@ + import os ++ ++def move(x): ++ return x +""" + assert new_public_defs(diff, RULES) == [] + + +def test_a_test_in_the_diff_covers_its_function(): + report = analyze(DIFF, RULES) + assert [d.qualname for d in report.candidates] == ["Scheduler.admit"] + assert report.referenced == {"plan_batch": ["tests/test_sched.py"]} + assert report.searched_tree is False + + +def test_a_test_already_in_the_tree_covers_its_function(tmp_path): + repo = tmp_path / "repo" + (repo / "tests").mkdir(parents=True) + (repo / "tests" / "test_admit.py").write_text("s.admit(r)\n") + (repo / "pkg").mkdir() + (repo / "pkg" / "sched.py").write_text("def admit(): pass\n") + for cmd in (["git", "init", "-q"], ["git", "add", "-A"]): + subprocess.run(cmd, cwd=repo, check=True) + report = analyze(DIFF, RULES, str(repo)) + assert report.candidates == [] + # the defining source file never counts as a test reference + assert report.referenced["Scheduler.admit"] == ["tests/test_admit.py"] + + +def test_rules_come_from_the_adapter_and_can_switch_the_check_off(): + assert UTCoverageRules.from_manifest(None) == UTCoverageRules() + assert UTCoverageRules.from_manifest( + {"ut_coverage": {"enabled": False}}) is None + assert UTCoverageRules.from_manifest( + {"repo": {"language": "rust"}}) is None + rules = UTCoverageRules.from_manifest( + {"ut_coverage": {"source_roots": ["src/"], "test_globs": ["t/*"]}}) + assert rules.is_source("src/a.py") and not rules.is_source("lib/a.py") + assert rules.is_test("t/a.py") and not rules.is_source("t/a.py") + + +def test_render_lists_candidates_with_the_judgement_rules(): + assert render(CoverageReport()) == "" + text = render(analyze(DIFF, RULES)) + assert "pkg/sched.py:13 `Scheduler.admit`" in text + assert "indirectly" in text and "trivial" in text + assert f"At most {MAX_COMMENTS}" in text + + +def test_only_three_gap_comments_publish_and_the_rest_are_named(): + report = CoverageReport(candidates=[ + PublicDef("pkg/m.py", n, f"f{n}") for n in range(1, 6)]) + other = {"file": "pkg/m.py", "line": 99, "comment": "real bug"} + gaps = [{"file": "m.py" if n == 1 else "pkg/m.py", "line": n, + "kind": "untested_api", "comment": f"test f{n}"} + for n in range(1, 6)] + kept, dropped = cap_gap_comments([other, *gaps], report) + assert [c["comment"] for c in kept] == ["real bug", "test f1", "test f2", + "test f3"] + assert dropped == ["f4", "f5"] + assert all("kind" not in c for c in kept) + assert cap_gap_comments([other], None) == ([other], []) + + +def test_a_correctness_finding_on_a_candidate_def_is_never_capped(): + """Only the explicit `untested_api` classification is capped: a blocker + anchored on the same def line survives three gap comments before it.""" + report = CoverageReport(candidates=[ + PublicDef("pkg/m.py", n, f"f{n}") for n in range(1, 5)]) + gaps = [{"file": "pkg/m.py", "line": n, "kind": "untested_api", + "severity": "minor", "comment": f"test f{n}"} for n in range(1, 4)] + blocker = {"file": "pkg/m.py", "line": 4, "severity": "blocker", + "comment": "mutable default argument is shared across calls"} + kept, dropped = cap_gap_comments([*gaps, blocker], report) + assert blocker in kept and dropped == [] + + +def test_direct_plan_carries_candidates_only_when_given_a_diff(): + from infermatrix_copilot.direct_routing import direct_review_plan + + diff = DIFF.replace("pkg/", "vllm_omni/") + plan = direct_review_plan("vllm-omni", changed_files=["vllm_omni/sched.py"], + diff=diff) + block = plan["untested_public_api"] + assert block["status"] == "ok" + assert [c["name"] for c in block["candidates"]] == ["Scheduler.admit"] + assert block["searched_tree"] is False and block["instructions"] + assert any("untested_public_api" in item + for item in plan["first_review_checklist"]) + assert direct_review_plan("vllm-omni")["untested_public_api"] == { + "status": "no_diff"} + + +def test_sdk_request_diff_reaches_the_typed_plan(): + from infermatrix_copilot.sdk.v1 import ( + ChangedPath, DirectClient, DirectReviewRequest, RepositoryRef) + + request = DirectReviewRequest( + review_id="attempt-1", repository=RepositoryRef(alias="vllm-omni"), + pr_number=7, expected_head_sha="a" * 40, title="t", body="b", + changed_paths=(ChangedPath("vllm_omni/sched.py", "modified"),), + diff=DIFF.replace("pkg/", "vllm_omni/")) + plan = DirectClient().plan(request) + assert plan.untested_public_api["candidates"][0]["name"] == "Scheduler.admit" + assert plan.to_dict()["untested_public_api"]["status"] == "ok" + + +def test_strict_reviewer_sees_candidates_and_the_cap_holds(settings, trace, + tmp_path, git_repo): + from infermatrix_copilot.engine.registry import StepRegistry + from infermatrix_copilot.engine.step import StepContext + from infermatrix_copilot.engine.steps import register_builtin_steps + from infermatrix_copilot.llm import Block, Reply + + diff = "".join( + f"diff --git a/pkg/m{n}.py b/pkg/m{n}.py\n--- a/pkg/m{n}.py\n" + f"+++ b/pkg/m{n}.py\n@@ -0,0 +1,2 @@\n+def build_{n}(x):\n+ return x\n" + for n in range(5)) + comments = [{"file": f"pkg/m{n}.py", "line": 1, "severity": "minor", + "comment": f"build_{n} has no unit test", "evidence": "e", + "disposition": "publish", "kind": "untested_api"} + for n in range(5)] + + class LLM: + available = True + + def __init__(self): + self.calls = [] + + def create(self, *, system, messages, **_kw): + self.calls.append(messages[0]["content"]) + return Reply(blocks=[Block(type="text", text=json.dumps({ + "status": "success", "summary": "reviewed", "findings": [], + "files_read": [], "files_modified": [], "tests_requested": [], + "tests_run": [], "assumptions": [], "blockers": [], + "confidence": "high", "failure_kind": None, + "next_action": "post", "review_comments": comments}))]) + + settings.review_ensemble = False + llm = LLM() + state = {"diff_text": diff, "task_spec": {"pr": 9}, + "repo_path": str(git_repo)} + handler = register_builtin_steps(StepRegistry()).get("agent.review_diff") + result = asyncio.run(handler.handler(StepContext( + settings=settings, state=state, params={}, run_dir=tmp_path / "run", + trace=trace, llm=llm))) + assert result.ok, result.summary + assert "NEW PUBLIC FUNCTIONS WITH NO TEST REFERENCE" in llm.calls[0] + assert "pkg/m0.py:1 `build_0`" in llm.calls[0] + published = result.outputs["state_updates"]["review_comments"] + assert len([c for c in published if "no unit test" in c["comment"]]) == 3 + assert all("kind" not in c for c in published) + assert "Also without a unit test: `build_3`, `build_4`." in \ + result.outputs["state_updates"]["review_summary"]