From d541b7e5726cdf83deb2d5b59a2b419045ac8c7c Mon Sep 17 00:00:00 2001 From: kalmanm Date: Tue, 25 Aug 2026 18:51:57 +0000 Subject: [PATCH 1/2] Exclude non-review Macroscope comments from precision denominator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The online benchmark treats every comment from a bot account as a review suggestion, so precision = fixed / total. But a code review bot is often more than a code reviewer: the same account may post check-run results, PR assistant chatter, approvability verdicts, or notices. Scoring those as review suggestions is a category error — a style/convention check run is rarely "fixed", so it unfairly drags the tool's precision. Honor the tool's own provenance label. Macroscope stamps every PR comment with a hidden `` marker. In `_format_bot_comments` (the function feeding EXTRACT_BOT_SUGGESTIONS, i.e. the precision denominator), detect the marker and segment comments whose kind is not `code_review` into a separate `custom_check` bucket, kept out of the review text but recorded (not silently dropped). Two details: - Detection runs on the RAW body, before `_clean_bot_comment_body` strips HTML comments — the marker is itself an HTML comment. - Exclusion keys on `kind != code_review`, so future non-review surfaces are excluded automatically without another benchmark change. Add unit tests covering exclusion, the not-code_review key, code_review / untagged pass-through, the raw-vs-cleaned gotcha, and marker attribute tolerance. Document the convention in online/README.md. Co-Authored-By: Claude Opus 4.8 --- online/README.md | 38 ++++++ online/etl/pipeline/analyze.py | 80 +++++++++++-- online/etl/tests/test_actor_normalization.py | 2 +- online/etl/tests/test_analyze_formatting.py | 119 ++++++++++++++++++- 4 files changed, 227 insertions(+), 12 deletions(-) diff --git a/online/README.md b/online/README.md index 8c45886..a1e5327 100644 --- a/online/README.md +++ b/online/README.md @@ -43,6 +43,44 @@ 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 recording the +comment's provenance: + +```html + +``` + +The `kind` distinguishes real review (`code_review`) from non-review surfaces +(`check_run`, `pr_assistant`, `approvability`, `notice`, and any added later). +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..80b3500 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,17 @@ _HTML_COMMENT_RE = re.compile(r"", re.DOTALL) +# Macroscope stamps every PR comment with a hidden provenance marker 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 pattern tolerates optional quoting and trailing attributes. +_MACROSCOPE_META_RE = re.compile(r'\n" if kind is not None else "" + 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_marker_tolerates_quotes_and_trailing_attributes() -> None: + """Requirement: the marker regex tolerates optional quoting and trailing + attributes, so `kind="check_run"` with extra metadata still segments out. + + Macroscope may extend the marker with additional attributes over time; the kind + detection must not become brittle to that. + """ + events = [ + { + "actor": "macroscopeapp[bot]", + "event_type": "issue_comment", + "timestamp": "2026-07-01T12:00:00Z", + "data": {"body": '\nStyle nit.'}, + }, + ] + + segments = _format_bot_comments(events, "macroscopeapp[bot]") + + assert segments.review == "(no bot comments)" + assert [c["kind"] for c in segments.custom_check] == ["check_run"] From 964aef075d6ddcaf9f775cc6b87003574ca1b714 Mon Sep 17 00:00:00 2001 From: kalmanm Date: Wed, 26 Aug 2026 17:54:57 +0000 Subject: [PATCH 2/2] Parse macroscope-meta as JSON payload (coupled to back#14633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hidden Macroscope marker is changing from attribute style (``) to a JSON payload (``). Update the parser to match, or the exclusion silently stops matching once back#14633 lands. _macroscope_kind now captures the JSON object between the `macroscope-meta:` prefix and the closing `-->` (whitespace/newline tolerant, DOTALL) and json.loads it, reading the `kind` field. Extra payload fields (variant, config, check, ...) are ignored. A missing marker, malformed JSON, or a payload without a string `kind` is treated as untagged — the comment stays scored rather than being wrongly excluded, and parsing never throws. The segmentation logic and BotCommentSegments(review, custom_check) return are unchanged — only extraction changed. Detection still runs on the RAW body before HTML-comment cleaning. Tests updated to the JSON format (valid extraction, extra-field tolerance, malformed-payload robustness, raw-vs- cleaned gotcha) and README shows the new tag shape. Co-Authored-By: Claude Opus 4.8 --- online/README.md | 15 ++++-- online/etl/pipeline/analyze.py | 37 +++++++++---- online/etl/tests/test_analyze_formatting.py | 57 ++++++++++++++++----- 3 files changed, 82 insertions(+), 27 deletions(-) diff --git a/online/README.md b/online/README.md index a1e5327..119adc9 100644 --- a/online/README.md +++ b/online/README.md @@ -53,15 +53,20 @@ run is rarely "fixed" by a developer, so counting it in the precision denominato 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 recording the -comment's provenance: +Macroscope stamps every PR comment with a hidden HTML marker carrying a JSON +payload that records the comment's provenance: ```html - + + + ``` -The `kind` distinguishes real review (`code_review`) from non-review surfaces -(`check_run`, `pr_assistant`, `approvability`, `notice`, and any added later). +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 diff --git a/online/etl/pipeline/analyze.py b/online/etl/pipeline/analyze.py index 80b3500..9623b7b 100644 --- a/online/etl/pipeline/analyze.py +++ b/online/etl/pipeline/analyze.py @@ -27,14 +27,19 @@ _HTML_COMMENT_RE = re.compile(r"", re.DOTALL) -# Macroscope stamps every PR comment with a hidden provenance marker 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 pattern tolerates optional quoting and trailing attributes. -_MACROSCOPE_META_RE = re.compile(r' +# 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" @@ -166,13 +171,27 @@ def _clean_bot_comment_body(body: str) -> str: 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 "") - return match.group("kind") if match else None + 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 diff --git a/online/etl/tests/test_analyze_formatting.py b/online/etl/tests/test_analyze_formatting.py index 31dd8ef..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 @@ -91,13 +93,15 @@ def test_format_bot_comments_labels_and_numbers_comments() -> None: assert "Fixed now." not in formatted -def _macroscope_event(kind: str | None, body_text: str, ts: str) -> dict: +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 prepended to the visible body. + 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 = f"\n" if kind is not None else "" + marker = "" if kind is None else f"\n" return { "actor": "macroscopeapp[bot]", "event_type": "issue_comment", @@ -175,7 +179,7 @@ def test_format_bot_comments_detects_marker_on_raw_body_before_html_cleaning() - 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 = "" + 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")] @@ -186,23 +190,50 @@ def test_format_bot_comments_detects_marker_on_raw_body_before_html_cleaning() - assert [c["kind"] for c in segments.custom_check] == ["check_run"] -def test_format_bot_comments_marker_tolerates_quotes_and_trailing_attributes() -> None: - """Requirement: the marker regex tolerates optional quoting and trailing - attributes, so `kind="check_run"` with extra metadata still segments out. +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 may extend the marker with additional attributes over time; the kind - detection must not become brittle to that. + 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": '\nStyle nit.'}, + "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 segments.review == "(no bot comments)" - assert [c["kind"] for c in segments.custom_check] == ["check_run"] + assert "Review finding A." in segments.review + assert "Review finding B." in segments.review + assert segments.custom_check == []