Skip to content

Exclude non-review Macroscope comments from precision denominator - #60

Open
kalmanm wants to merge 2 commits into
withmartian:mainfrom
kalmanm:provenance-kind-exclusion
Open

Exclude non-review Macroscope comments from precision denominator#60
kalmanm wants to merge 2 commits into
withmartian:mainfrom
kalmanm:provenance-kind-exclusion

Conversation

@kalmanm

@kalmanm kalmanm commented Aug 25, 2026

Copy link
Copy Markdown

Summary

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 bot account can also post check-run results, PR
assistant chatter, approvability verdicts, or release notices
— distinct
product surfaces from code review. Scoring those 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.

This PR makes the benchmark honor a tool's own provenance label and score
only real code review.

The convention

Macroscope stamps every PR comment with a hidden HTML marker carrying a JSON
payload that records which surface produced it:

<!-- macroscope-meta: {"kind":"code_review","variant":"..."} -->
<!-- macroscope-meta: {"kind":"check_run","config":"...","check":"..."} -->

kind distinguishes real review (code_review) from non-review surfaces
(check_run, pr_assistant, approvability, notice, …). The benchmark should
score only code_review.

The change

In online/etl/pipeline/analyze.py, _format_bot_comments() (the function that
builds the bot-comment list feeding EXTRACT_BOT_SUGGESTIONS, i.e. the precision
denominator):

  • Segment, don't drop. Comments whose kind != code_review are moved into a
    separate custom_check bucket on the returned BotCommentSegments — recorded
    and auditable, kept out of the review text, and excluded from the precision
    denominator. They are not silently discarded. This is deliberate: segmenting
    (not dropping) is what keeps the rule safe against a tool "labelling its false
    positives away" — the exclusions remain visible and countable.
  • Detected on the RAW body, before _clean_bot_comment_body strips HTML
    comments. The marker is itself an HTML comment, so checking the cleaned body
    would never see it. (A regression test pins this gotcha.)
  • Keys on kind != code_review, not an allowlist of known non-review kinds,
    so any future non-review surface is excluded automatically without another
    benchmark change.

Extraction grabs the JSON payload between the marker prefix and -->, then
json.loads it and reads kind (whitespace/newline tolerant, extra fields
ignored). A malformed payload or one lacking a string kind is treated as
untagged — the comment stays scored and parsing never throws:

re.compile(r"<!--\s*macroscope-meta:\s*(\{.*?\})\s*-->", re.DOTALL)

Update: the marker moved from attribute style to a JSON payload (coupled to
an internal Macroscope change); the parser and tests track the JSON shape.

Two approaches considered

  1. Segment (this PR). Return BotCommentSegments(review, custom_check);
    review feeds the LLM, custom_check records the excluded comments. One
    caller updated; the number of segmented comments is logged per PR.
  2. Simple skip. continue past non-code_review comments in the loop —
    fewer lines, but the excluded comments vanish with no record, which is exactly
    the transparency property we want to keep.

We lead with segment because it directly answers the obvious objection to any
provenance-based exclusion ("couldn't a tool just relabel its misses?"): excluded
comments are recorded, not dropped, so the exclusions stay auditable. The segment
change is small and localized — a dataclass return plus one caller — so there was
no need to defer it to a follow-up.

Generality

This is implemented as a per-tool convention, but it is tool-agnostic in spirit:
any bot that labels its own non-review comments can be scored the same way. Other
tools with check/gate features hit the same conflation; a companion Discussion
invites them to adopt a comparable marker. Nothing here requires action from
other tools — the marker is self-contained and the benchmark simply honors it.

Tests

online/etl/tests/test_analyze_formatting.py adds coverage for:

  • a check_run comment is absent from the review text and lands in custom_check;
  • exclusion keys on kind != code_review (a novel pr_assistant kind is excluded);
  • code_review and untagged comments still feed the denominator unchanged;
  • the raw-vs-cleaned gotcha (a marker that cleaning would strip still excludes);
  • marker tolerance of quotes and trailing attributes.
$ uv run pytest        # in online/etl/
286 passed

Docs: online/README.md documents the convention under Analyze.


Happy to adjust scope or framing based on maintainer preference — see the linked discussion.

kalmanm and others added 2 commits August 25, 2026 18:51
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 `<!-- macroscope-meta kind=... -->` 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 <noreply@anthropic.com>
The hidden Macroscope marker is changing from attribute style
(`<!-- macroscope-meta kind=... -->`) to a JSON payload
(`<!-- macroscope-meta: {"kind":"...","..."} -->`). 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 <noreply@anthropic.com>
@kalmanm
kalmanm marked this pull request as ready for review August 26, 2026 21:27
@kalmanm kalmanm changed the title Exclude non-review Macroscope comments from precision denominator [murmur:back/martian-provenance-exclusion] Exclude non-review Macroscope comments from precision denominator Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant