diff --git a/online/README.md b/online/README.md index 8c45886..119adc9 100644 --- a/online/README.md +++ b/online/README.md @@ -43,6 +43,49 @@ This is the core of the benchmark — a three-step LLM analysis: - **Recall** = matched actions / total actions ("what % of real issues did the bot catch?") - **F-beta** = adjustable harmonic mean (F1 when beta=1) +#### Honoring tools' own provenance labels + +A code review bot is often more than a code reviewer. The same bot account may +also post check-run results, PR assistant chatter, approvability verdicts, or +release notices. Those are distinct product surfaces from code review, and +scoring them as review suggestions is a category error: a style/convention check +run is rarely "fixed" by a developer, so counting it in the precision denominator +unfairly drags the tool's precision. + +When a tool tells us which surface produced a comment, the benchmark honors it. +Macroscope stamps every PR comment with a hidden HTML marker carrying a JSON +payload that records the comment's provenance: + +```html + + + +``` + +The `kind` field distinguishes real review (`code_review`) from non-review +surfaces (`check_run`, `pr_assistant`, `approvability`, `notice`, and any added +later). Any other payload fields are ignored, and a malformed payload (or one +without a `kind`) is treated as untagged so a real review comment is never +wrongly excluded. +The benchmark scores **only `code_review`**. Every other kind is *segmented* — recorded +separately as a custom-check comment and excluded from the review-precision +denominator, not silently dropped (see `custom_check` in +`pipeline/analyze.py::_format_bot_comments`). + +Two details matter: + +- The marker is detected on the **raw comment body**, before hidden HTML comments + are stripped for the LLM prompt — the marker itself is an HTML comment, so + checking the cleaned body would never see it. +- Exclusion keys on `kind != code_review` rather than an allowlist of known + non-review kinds, so a new non-review surface is excluded automatically without + a benchmark change. + +This is a per-tool convention: any bot that labels its own non-review comments can +be scored the same way. The `code_review`-only rule is safe against a tool +labelling its false positives away, because excluded comments are *segmented and +recorded*, not dropped — the exclusions remain auditable. + ### 5. Label (optional) An LLM classifies each PR by language, domain (frontend/backend/infra), PR type (feature/bugfix/refactor), issue severity, and more. These labels power the dashboard filters. diff --git a/online/etl/pipeline/analyze.py b/online/etl/pipeline/analyze.py index 3dfc56b..9623b7b 100644 --- a/online/etl/pipeline/analyze.py +++ b/online/etl/pipeline/analyze.py @@ -3,6 +3,8 @@ from __future__ import annotations import asyncio +from dataclasses import dataclass +from dataclasses import field import json import logging import re @@ -25,6 +27,22 @@ _HTML_COMMENT_RE = re.compile(r"", re.DOTALL) +# Macroscope stamps every PR comment with a hidden provenance marker carrying a +# JSON payload whose `kind` identifies the product surface that produced it: +# `code_review` for real review, and other kinds (check_run, pr_assistant, +# approvability, notice, ...) for non-review surfaces. We score only `code_review`; +# everything else is segmented out of the precision denominator. Keying on +# `kind != code_review` (rather than an allowlist of non-review kinds) means new +# non-review surfaces are excluded automatically. +# +# The marker looks like: +# We capture the JSON object between the prefix and the closing "-->" and parse it; +# the regex is whitespace- and newline-tolerant, and the payload may carry extra +# fields (variant, config, check, ...) that we ignore. +_MACROSCOPE_META_RE = re.compile(r"", re.DOTALL) + +_CODE_REVIEW_KIND = "code_review" + def _find_bot_review_commit( reviews: list[dict], @@ -150,13 +168,59 @@ def _clean_bot_comment_body(body: str) -> str: return _HTML_COMMENT_RE.sub("", body).strip() -def _format_bot_comments(events: list[dict], chatbot_username: str) -> str: - """Format bot's review/review_comment/issue_comment events with full context. +def _macroscope_kind(raw_body: str) -> str | None: + """Return the macroscope-meta `kind` stamped on a raw comment body, if present. + + The marker carries a JSON payload: ``. + We extract the JSON object and read its `kind` field. + + The check MUST run on the RAW body, before `_clean_bot_comment_body` strips + HTML comments — the marker lives inside an HTML comment and cleaning would + erase it, silently letting non-review surfaces back into the precision + denominator. + + A missing marker, malformed JSON, or a payload without a string `kind` is + treated as untagged (returns None) so the comment keeps the default scored + behavior rather than being wrongly excluded or throwing. + """ + match = _MACROSCOPE_META_RE.search(raw_body or "") + if not match: + return None + try: + payload = json.loads(match.group(1)) + except json.JSONDecodeError: + return None + kind = payload.get("kind") if isinstance(payload, dict) else None + return kind if isinstance(kind, str) else None + + +@dataclass +class BotCommentSegments: + """Bot comments split by provenance for the analyze step. - Skips review_comment replies (in_reply_to_id set) — these are responses - to other commenters' threads, not original review suggestions. + `review` is the formatted text of `code_review` (and untagged) comments that + feeds EXTRACT_BOT_SUGGESTIONS — i.e. the precision denominator. `custom_check` + holds the comments segmented out by their macroscope-meta kind: recorded here + so they are auditable rather than silently dropped, but excluded from scoring. + """ + + review: str + custom_check: list[dict] = field(default_factory=list) + + +def _format_bot_comments(events: list[dict], chatbot_username: str) -> BotCommentSegments: + """Split and format the bot's review/review_comment/issue_comment events. + + Skips review_comment replies (in_reply_to_id set) — these are responses to + other commenters' threads, not original review suggestions. + + Comments carrying a macroscope-meta marker whose kind is not `code_review` + (check runs, PR assistant, approvability, notices, and any future non-review + surface) are segmented into `custom_check` and kept out of the returned review + text, so they never inflate the precision denominator. """ lines = [] + custom_check = [] comment_num = 1 for e in events: if not same_github_actor(e.get("actor"), chatbot_username): @@ -170,15 +234,21 @@ def _format_bot_comments(events: list[dict], chatbot_username: str) -> str: continue ts = e.get("timestamp", "") + raw_body = data.get("body") or "" + + # Provenance check on the RAW body, before HTML-comment cleaning. + kind = _macroscope_kind(raw_body) + if kind is not None and kind != _CODE_REVIEW_KIND: + custom_check.append({"kind": kind, "event_type": etype, "timestamp": ts}) + continue + body = _clean_bot_comment_body(raw_body) if etype == "review": state = data.get("state", "") - body = _clean_bot_comment_body(data.get("body") or "") lines.append(f"COMMENT C{comment_num} [REVIEW_BODY state={state} timestamp={ts}]") if body: lines.append(body) - elif etype in ("review_comment", "issue_comment"): - body = _clean_bot_comment_body(data.get("body") or "") + else: path = data.get("path") or "" line = data.get("line") or "" diff_hunk = data.get("diff_hunk") or "" @@ -196,7 +266,7 @@ def _format_bot_comments(events: list[dict], chatbot_username: str) -> str: lines.append(body) lines.append("") comment_num += 1 - return "\n".join(lines) if lines else "(no bot comments)" + return BotCommentSegments(review="\n".join(lines) if lines else "(no bot comments)", custom_check=custom_check) def _format_post_review_activity( @@ -342,6 +412,12 @@ def _parse_json_col(col_name: str) -> list[dict]: # Format inputs for LLM commits_under_review = _format_commits_with_diffs(pre_commits, details_by_sha) bot_comments = _format_bot_comments(events, chatbot_username) + if bot_comments.custom_check: + logger.debug( + f"{pr_row['repo_name']}#{pr_row['pr_number']}: segmented " + f"{len(bot_comments.custom_check)} non-review comment(s) out of the precision denominator " + f"(kinds={sorted({c['kind'] for c in bot_comments.custom_check})})" + ) post_review_activity = _format_post_review_activity(post_commits, details_by_sha, events, chatbot_username, hash_x) pr_title = assembled.get("pr_title", "") @@ -355,7 +431,7 @@ def _parse_json_col(col_name: str) -> list[dict]: pr_author=pr_author, repo_name=repo_name, commits_under_review=commits_under_review, - bot_comments=bot_comments, + bot_comments=bot_comments.review, ) suggestions_resp = await llm.structured_completion(prompt1, BotSuggestionsResponse) suggestions = [s.model_dump() for s in suggestions_resp.suggestions] @@ -447,7 +523,10 @@ async def analyze_prs( """ repo = PRRepository(db) prs = await repo.get_assembled_not_analyzed( - chatbot_id=chatbot_id, limit=limit, since=since, until=until, + chatbot_id=chatbot_id, + limit=limit, + since=since, + until=until, sort_by=sort_by, ) diff --git a/online/etl/tests/test_actor_normalization.py b/online/etl/tests/test_actor_normalization.py index 6f34f17..e1b5b90 100644 --- a/online/etl/tests/test_actor_normalization.py +++ b/online/etl/tests/test_actor_normalization.py @@ -29,7 +29,7 @@ def test_analyze_includes_graphql_thread_comments_from_bot_slug() -> None: } ] - formatted = _format_bot_comments(events, "cubic-dev-ai[bot]") + formatted = _format_bot_comments(events, "cubic-dev-ai[bot]").review assert "Fix the missing null check." in formatted assert "main.py:42" in formatted diff --git a/online/etl/tests/test_analyze_formatting.py b/online/etl/tests/test_analyze_formatting.py index 8445c0d..8d145d9 100644 --- a/online/etl/tests/test_analyze_formatting.py +++ b/online/etl/tests/test_analyze_formatting.py @@ -2,6 +2,8 @@ from __future__ import annotations +import json + from pipeline.analyze import _clean_bot_comment_body from pipeline.analyze import _format_bot_comments @@ -83,9 +85,155 @@ def test_format_bot_comments_labels_and_numbers_comments() -> None: }, ] - formatted = _format_bot_comments(events, "reviewer[bot]") + formatted = _format_bot_comments(events, "reviewer[bot]").review assert "COMMENT C1 [INLINE_REVIEW_COMMENT path=src/app.py:42 timestamp=2026-07-01T12:00:00Z]" in formatted assert "Code context:\n```diff\n@@ -1 +1 @@\n-old\n+new\n```" in formatted assert "COMMENT C2 [REVIEW_BODY state=commented timestamp=2026-07-01T12:02:00Z]" in formatted assert "Fixed now." not in formatted + + +def _macroscope_event(kind: str | None, body_text: str, ts: str, **extra: object) -> dict: + """Build an issue_comment event, optionally stamped with a macroscope-meta marker. + + Mirrors how Macroscope emits comments: the provenance marker is a hidden HTML + comment carrying a JSON payload, prepended to the visible body. `kind=None` + produces an untagged comment; `extra` adds sibling payload fields (variant, + config, check, ...) that the parser must ignore. + """ + marker = "" if kind is None else f"\n" + return { + "actor": "macroscopeapp[bot]", + "event_type": "issue_comment", + "timestamp": ts, + "data": {"body": f"{marker}{body_text}"}, + } + + +def test_format_bot_comments_segments_non_code_review_kinds_out_of_review_text() -> None: + """Requirement: a comment marked with a non-code_review kind must not feed the + precision denominator. + + EXTRACT_BOT_SUGGESTIONS is built from the returned `review` text, so a check_run + (or any non-review surface) comment must be absent from it and instead recorded + in `custom_check`. Otherwise convention/style/policy check-run comments — rarely + "fixed" by developers — would drag Macroscope's precision unfairly. + """ + events = [ + _macroscope_event("code_review", "Possible null dereference here.", "2026-07-01T12:00:00Z"), + _macroscope_event("check_run", "Naming convention: use snake_case.", "2026-07-01T12:01:00Z"), + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert "Possible null dereference here." in segments.review + assert "Naming convention: use snake_case." not in segments.review + assert [c["kind"] for c in segments.custom_check] == ["check_run"] + + +def test_format_bot_comments_keys_on_not_code_review_so_future_kinds_are_excluded() -> None: + """Requirement: exclusion keys on `kind != code_review`, not an allowlist of + known non-review kinds. + + A brand-new surface (here `pr_assistant`) that Martian has never heard of must + be excluded automatically, without another benchmark change. + """ + events = [ + _macroscope_event("pr_assistant", "Want me to summarize this PR?", "2026-07-01T12:00:00Z"), + _macroscope_event("approvability", "This PR looks safe to merge.", "2026-07-01T12:01:00Z"), + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert segments.review == "(no bot comments)" + assert sorted(c["kind"] for c in segments.custom_check) == ["approvability", "pr_assistant"] + + +def test_format_bot_comments_keeps_code_review_and_untagged_comments() -> None: + """Requirement: real review (kind=code_review) and legacy untagged comments are + unaffected — they still feed the precision denominator. + + Untagged comments (comments predating the marker, or from bots that never emit + it) must keep the pre-change behavior of being scored. + """ + events = [ + _macroscope_event("code_review", "Tagged review finding.", "2026-07-01T12:00:00Z"), + _macroscope_event(None, "Untagged review finding.", "2026-07-01T12:01:00Z"), + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert "Tagged review finding." in segments.review + assert "Untagged review finding." in segments.review + assert segments.custom_check == [] + assert "COMMENT C1 " in segments.review + assert "COMMENT C2 " in segments.review + + +def test_format_bot_comments_detects_marker_on_raw_body_before_html_cleaning() -> None: + """Requirement (the raw-vs-cleaned gotcha): the marker must be detected on the + RAW body, before `_clean_bot_comment_body` strips HTML comments. + + The marker lives inside an HTML comment, which cleaning removes. If detection + ran on the cleaned body the kind would be invisible and the comment would leak + back into the precision denominator. This asserts the marked comment is excluded + even though the marker is exactly the kind of HTML comment cleaning strips. + """ + stripped_marker = '' + assert _clean_bot_comment_body(stripped_marker) == "" # cleaning erases the marker entirely + + events = [_macroscope_event("check_run", "Style nit.", "2026-07-01T12:00:00Z")] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert segments.review == "(no bot comments)" + assert [c["kind"] for c in segments.custom_check] == ["check_run"] + + +def test_format_bot_comments_ignores_extra_payload_fields() -> None: + """Requirement: the JSON payload may carry sibling fields (variant, config, + check, ...); the parser reads only `kind` and ignores the rest. + + Macroscope stamps additional metadata per surface — e.g. check_run comments + carry `config` and `check`. Those must not affect the code_review-only rule. + """ + events = [ + _macroscope_event("code_review", "Real finding.", "2026-07-01T12:00:00Z", variant="inline"), + _macroscope_event("check_run", "Style nit.", "2026-07-01T12:01:00Z", config="lint", check="naming"), + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert "Real finding." in segments.review + assert "Style nit." not in segments.review + assert [c["kind"] for c in segments.custom_check] == ["check_run"] + + +def test_format_bot_comments_treats_malformed_payload_as_untagged() -> None: + """Requirement: a marker whose payload is not valid JSON (or lacks a string + `kind`) is treated as untagged — the comment stays scored, and parsing never + throws. + + Robustness: a truncated or malformed marker must not silently exclude a real + review comment, nor crash the pipeline. Untagged is the safe default. + """ + events = [ + { # malformed JSON payload + "actor": "macroscopeapp[bot]", + "event_type": "issue_comment", + "timestamp": "2026-07-01T12:00:00Z", + "data": {"body": '\nReview finding A.'}, + }, + { # valid JSON, but no "kind" field + "actor": "macroscopeapp[bot]", + "event_type": "issue_comment", + "timestamp": "2026-07-01T12:01:00Z", + "data": {"body": '\nReview finding B.'}, + }, + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert "Review finding A." in segments.review + assert "Review finding B." in segments.review + assert segments.custom_check == []