From 4ef62f28bbb5d05575829f7e3e65a96b95ec74c3 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 23:24:54 +0000 Subject: [PATCH 1/3] fix(webhook): re-read PR labels at review start so a late AI-USAGE counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Labelling a PR is two webhook events. AI-REVIEW arrives first, the review claims (repo, index, sha) and starts; the AI-USAGE event that follows a moment later hits the in-flight dedupe and is dropped. `report_usage` was snapshotted from the first payload, which had not seen AI-USAGE yet, so the review posted without its usage block even though the label was on the PR by the time it finished — observed on gitea_admin/pragent#9 (`usage=False` in the pod log, no
section in the posted review). Re-read the labels from the API at review start and upgrade the flag. The read is best-effort: any failure logs and returns [], leaving the payload's verdict intact, because a usage section is not worth failing a review over. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN --- pilot/webhook_server.py | 37 +++++++++++++++++++++++++++ tests/pilot/test_webhook_server.py | 40 ++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/pilot/webhook_server.py b/pilot/webhook_server.py index 7ccbdb1..5a7ecfd 100644 --- a/pilot/webhook_server.py +++ b/pilot/webhook_server.py @@ -39,6 +39,7 @@ import hmac import json import os import threading +import urllib.request from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from ai_review import review_pr @@ -176,10 +177,46 @@ def _release(key: tuple[str, str, str]) -> None: _inflight.discard(key) +def _fetch_current_labels(repo: str, index: str) -> list: + """Re-read the PR's labels from the API. [] on any failure. + + The webhook payload is a snapshot from the instant the event fired. When a + PR is labelled AI-REVIEW first and AI-USAGE a moment later, the review is + already claimed and running, so the second event is deduped and the usage + opt-in is lost — the review posts without its usage block. Reading the + labels again at review start closes that window. + """ + url = f"{GITEA_API}/repos/{repo}/issues/{index}/labels" + req = urllib.request.Request(url, headers={ + "Authorization": f"token {BOT_TOKEN}", + "Accept": "application/json", + }) + try: + with urllib.request.urlopen(req, timeout=15) as resp: + return json.loads(resp.read().decode() or "[]") + except Exception as e: + print(f"pragent-webhook: could not re-read labels for {repo}#{index}: {e}", flush=True) + return [] + + def _run_review( key: tuple[str, str, str], title: str, body: str, report_usage: bool, base_ref: str ) -> None: repo, index, sha = key + # The AI-USAGE opt-in may have been applied *after* the label event that + # triggered this review (labelling is two events, the review claims on the + # first). Re-read the labels now so the later opt-in still counts. + if not report_usage: + report_usage = _labels_have( + _fetch_current_labels(repo, index), AI_USAGE_LABEL + ) + if report_usage: + print( + f"pragent-webhook: {repo}#{index} AI-USAGE found on re-read " + f"(added after the trigger event)", + flush=True, + ) + try: with _review_slots: ok = review_pr( diff --git a/tests/pilot/test_webhook_server.py b/tests/pilot/test_webhook_server.py index 27d87b5..33d3461 100644 --- a/tests/pilot/test_webhook_server.py +++ b/tests/pilot/test_webhook_server.py @@ -134,3 +134,43 @@ def test_missing_label_is_ignored(monkeypatch): def test_review_slots_bound_matches_config(): assert ws.MAX_CONCURRENT >= 1 assert ws._review_slots._value <= ws.MAX_CONCURRENT + + +# --------------------------------------------------------------------------- +# AI-USAGE opt-in applied after the triggering label event +# --------------------------------------------------------------------------- + + +def test_run_review_rereads_labels_for_late_ai_usage(monkeypatch): + # Labelling a PR AI-REVIEW then AI-USAGE fires two events; the review + # claims on the first, so the second is deduped and the payload snapshot + # never sees AI-USAGE. The re-read at review start must recover it. + seen = {} + monkeypatch.setattr(ws, "_fetch_current_labels", + lambda repo, index: [{"name": "AI-REVIEW"}, {"name": "AI-USAGE"}]) + monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) + ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") + assert seen["report_usage"] is True + + +def test_run_review_keeps_usage_off_when_label_absent(monkeypatch): + seen = {} + monkeypatch.setattr(ws, "_fetch_current_labels", + lambda repo, index: [{"name": "AI-REVIEW"}]) + monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) + ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") + assert seen["report_usage"] is False + + +def test_label_reread_failure_is_not_fatal(monkeypatch): + # A dead API must not take the review down with it — the re-read is a + # best-effort upgrade of an opt-in flag, nothing more. + def _boom(*a, **k): + raise OSError("api down") + monkeypatch.setattr(ws.urllib.request, "urlopen", _boom) + assert ws._fetch_current_labels("o/r", "9") == [] + + seen = {} + monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) + ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") + assert seen["report_usage"] is False From aedea973abcbc1bf3a237ad4a5b1ade084891088 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 23:29:03 +0000 Subject: [PATCH 2/3] fix(webhook): label re-read must hit /api/v1, not the bare host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GITEA_API is the host with no version prefix — ai_review._get / _post append /api/v1 per call. The re-read helper didn't, so it 404'd on every review and silently fell back to the payload's verdict: "could not re-read labels for gitea_admin/pragent#10: HTTP Error 404" in the pod log, usage block still missing. Caught by labelling PR #10 AI-REVIEW then AI-USAGE, which is the exact sequence the fix exists to handle. Test asserts the composed URL, since a wrong path here fails silently by design (the helper swallows errors so a review is never lost over a usage section). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN --- pilot/webhook_server.py | 4 +++- tests/pilot/test_webhook_server.py | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pilot/webhook_server.py b/pilot/webhook_server.py index 5a7ecfd..e756cd9 100644 --- a/pilot/webhook_server.py +++ b/pilot/webhook_server.py @@ -186,7 +186,9 @@ def _fetch_current_labels(repo: str, index: str) -> list: opt-in is lost — the review posts without its usage block. Reading the labels again at review start closes that window. """ - url = f"{GITEA_API}/repos/{repo}/issues/{index}/labels" + # GITEA_API is the bare host (no /api/v1) — every caller appends the + # version prefix itself; see ai_review._get / _post. + url = f"{GITEA_API}/api/v1/repos/{repo}/issues/{index}/labels" req = urllib.request.Request(url, headers={ "Authorization": f"token {BOT_TOKEN}", "Accept": "application/json", diff --git a/tests/pilot/test_webhook_server.py b/tests/pilot/test_webhook_server.py index 33d3461..fd0386a 100644 --- a/tests/pilot/test_webhook_server.py +++ b/tests/pilot/test_webhook_server.py @@ -162,6 +162,27 @@ def test_run_review_keeps_usage_off_when_label_absent(monkeypatch): assert seen["report_usage"] is False +def test_fetch_current_labels_hits_the_versioned_api_path(monkeypatch): + # GITEA_API is the bare host; the /api/v1 prefix is the caller's job. + # Getting this wrong 404s silently and the opt-in is lost — which is + # exactly what shipped the first time. + seen = {} + + class _Resp: + def read(self): return b'[{"name": "AI-USAGE"}]' + def __enter__(self): return self + def __exit__(self, *a): return False + + def _urlopen(req, timeout=None): + seen["url"] = req.full_url + return _Resp() + + monkeypatch.setattr(ws.urllib.request, "urlopen", _urlopen) + out = ws._fetch_current_labels("o/r", "9") + assert out == [{"name": "AI-USAGE"}] + assert seen["url"] == f"{ws.GITEA_API}/api/v1/repos/o/r/issues/9/labels" + + def test_label_reread_failure_is_not_fatal(monkeypatch): # A dead API must not take the review down with it — the re-read is a # best-effort upgrade of an opt-in flag, nothing more. From c58c00181e822e1373a114e207167f961f6223d4 Mon Sep 17 00:00:00 2001 From: Marcos Date: Thu, 20 Aug 2026 23:35:04 +0000 Subject: [PATCH 3/3] fix(review): read the AI-USAGE opt-in at render time, not at review start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous two commits put the label re-read in the webhook, at review start. That is too early to help: the review claims on the AI-REVIEW event and the re-read runs milliseconds later, while the reviewer's second click (AI-USAGE) is still a second or two away. It would have kept 404ing quietly if the path fix hadn't landed, and even fixed it caught nothing. Move the check to where the decision is actually used — just before the usage block is rendered, after the model has run. That is a minute or more after the trigger, by which time the label is there. Attribution is computed in the same branch, so a late opt-in still gets its per-comment token lines. `pr_has_label` goes through the existing gitea_get helper, which owns the /api/v1 prefix, so the path can't drift again. Any failure returns False and the payload's verdict stands: a review is never lost over a usage section. Reverts the webhook-side re-read from 4ef62f2 and aedea97. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN --- pilot/ai_review.py | 43 +++++++++++++++++++++ pilot/webhook_server.py | 39 ------------------- tests/pilot/test_ai_review.py | 29 ++++++++++++++ tests/pilot/test_webhook_server.py | 61 ------------------------------ 4 files changed, 72 insertions(+), 100 deletions(-) diff --git a/pilot/ai_review.py b/pilot/ai_review.py index fec53ea..9dfee33 100644 --- a/pilot/ai_review.py +++ b/pilot/ai_review.py @@ -66,6 +66,8 @@ SHA_MARKER = "" _SHA_MARKER_RE = re.compile(r"") AI_REVIEW_LABEL = "AI-REVIEW" +# Opt-in label for the token-usage block. Read at render time — see pr_has_label. +AI_USAGE_LABEL = "AI-USAGE" SEVERITIES = ("critical", "high", "medium", "low") # Severity rank — higher = more severe. Used by `apply_repo_config` to drop # findings below `severity_threshold`. Critical=3, high=2, medium=1, low=0. @@ -156,6 +158,33 @@ def parse_text_blocks(content: list) -> str: return "\n".join(out).strip() +def pr_has_label(api: str, repo: str, index: str, token: str, label: str) -> bool: + """True if the PR currently carries `label`. False on any failure. + + Read at RENDER time, not at review start. A reviewer labels AI-REVIEW and + AI-USAGE seconds apart; the review claims on the first event and the + second is dropped by the in-flight dedupe, so the trigger payload never + sees the opt-in. Re-reading when the review begins is no better — that is + still milliseconds after the first click. Only a read taken once the + review has finished (a minute or more later) reliably sees the label. + """ + try: + code, raw = gitea_get(api, repo, f"issues/{index}/labels", token) + if code >= 300: + return False + data = json.loads(raw.decode() or "[]") + except Exception as e: + print(f"pragent: could not re-read labels for {repo}#{index}: {e}", + file=sys.stderr, flush=True) + return False + if not isinstance(data, list): + return False + return any( + (isinstance(x, dict) and x.get("name") == label) or x == label + for x in data + ) + + def _int_env(name: str, default: int) -> int: """Read an int from the environment, falling back on anything unparseable. @@ -1896,6 +1925,13 @@ def review_pr( file=sys.stderr, flush=True, ) salvaged = salvage_summary(stdout) + # The AI-USAGE opt-in is re-checked HERE, at render time: the label + # is usually applied moments after AI-REVIEW, long after this + # review was claimed and its trigger payload frozen. + if not report_usage: + report_usage = pr_has_label(api, repo, index, token, AI_USAGE_LABEL) + if report_usage and usage and usage.get('output'): + compute_attribution(findings, usage['output']) usage_section = _render_collapsible_usage(usage, model, config=config) if report_usage else "" post_review(api, repo, index, token, format_review_body( salvaged or "AI review produced no parseable output.", @@ -1936,6 +1972,13 @@ def review_pr( # for it. if report_usage and usage and usage.get("output"): compute_attribution(findings, usage["output"]) + # The AI-USAGE opt-in is re-checked HERE, at render time: the label + # is usually applied moments after AI-REVIEW, long after this + # review was claimed and its trigger payload frozen. + if not report_usage: + report_usage = pr_has_label(api, repo, index, token, AI_USAGE_LABEL) + if report_usage and usage and usage.get('output'): + compute_attribution(findings, usage['output']) usage_section = _render_collapsible_usage(usage, model, config=config) if report_usage else "" # Anchor against the RAW diff, never the compressed one. Compression diff --git a/pilot/webhook_server.py b/pilot/webhook_server.py index e756cd9..7ccbdb1 100644 --- a/pilot/webhook_server.py +++ b/pilot/webhook_server.py @@ -39,7 +39,6 @@ import hmac import json import os import threading -import urllib.request from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from ai_review import review_pr @@ -177,48 +176,10 @@ def _release(key: tuple[str, str, str]) -> None: _inflight.discard(key) -def _fetch_current_labels(repo: str, index: str) -> list: - """Re-read the PR's labels from the API. [] on any failure. - - The webhook payload is a snapshot from the instant the event fired. When a - PR is labelled AI-REVIEW first and AI-USAGE a moment later, the review is - already claimed and running, so the second event is deduped and the usage - opt-in is lost — the review posts without its usage block. Reading the - labels again at review start closes that window. - """ - # GITEA_API is the bare host (no /api/v1) — every caller appends the - # version prefix itself; see ai_review._get / _post. - url = f"{GITEA_API}/api/v1/repos/{repo}/issues/{index}/labels" - req = urllib.request.Request(url, headers={ - "Authorization": f"token {BOT_TOKEN}", - "Accept": "application/json", - }) - try: - with urllib.request.urlopen(req, timeout=15) as resp: - return json.loads(resp.read().decode() or "[]") - except Exception as e: - print(f"pragent-webhook: could not re-read labels for {repo}#{index}: {e}", flush=True) - return [] - - def _run_review( key: tuple[str, str, str], title: str, body: str, report_usage: bool, base_ref: str ) -> None: repo, index, sha = key - # The AI-USAGE opt-in may have been applied *after* the label event that - # triggered this review (labelling is two events, the review claims on the - # first). Re-read the labels now so the later opt-in still counts. - if not report_usage: - report_usage = _labels_have( - _fetch_current_labels(repo, index), AI_USAGE_LABEL - ) - if report_usage: - print( - f"pragent-webhook: {repo}#{index} AI-USAGE found on re-read " - f"(added after the trigger event)", - flush=True, - ) - try: with _review_slots: ok = review_pr( diff --git a/tests/pilot/test_ai_review.py b/tests/pilot/test_ai_review.py index 2ab4d08..591bb5e 100644 --- a/tests/pilot/test_ai_review.py +++ b/tests/pilot/test_ai_review.py @@ -616,6 +616,35 @@ def test_reference_non_url_renders_as_plain_text(): assert "](CVE-" not in body +def test_pr_has_label_reads_the_live_labels(monkeypatch): + # The AI-USAGE opt-in is read at render time, not from the trigger + # payload: labelling AI-REVIEW then AI-USAGE is two events, the review + # claims on the first, and the second is dropped by the in-flight dedupe. + seen = {} + + def _get(api, repo, path, token, accept="application/json"): + seen["path"] = path + return 200, b'[{"name": "AI-REVIEW"}, {"name": "AI-USAGE"}]' + + monkeypatch.setattr(ai_review, "gitea_get", _get) + assert ai_review.pr_has_label("http://api", "o/r", "9", "t", "AI-USAGE") is True + assert seen["path"] == "issues/9/labels" + assert ai_review.pr_has_label("http://api", "o/r", "9", "t", "NOPE") is False + + +def test_pr_has_label_survives_a_broken_api(monkeypatch): + def _boom(*a, **k): + raise RuntimeError("gitea down") + monkeypatch.setattr(ai_review, "gitea_get", _boom) + assert ai_review.pr_has_label("http://api", "o/r", "9", "t", "AI-USAGE") is False + + monkeypatch.setattr(ai_review, "gitea_get", lambda *a, **k: (404, b"nope")) + assert ai_review.pr_has_label("http://api", "o/r", "9", "t", "AI-USAGE") is False + + monkeypatch.setattr(ai_review, "gitea_get", lambda *a, **k: (200, b'{"not": "a list"}')) + assert ai_review.pr_has_label("http://api", "o/r", "9", "t", "AI-USAGE") is False + + def test_int_env_falls_back_on_garbage(monkeypatch, capsys): monkeypatch.setenv("PRAGENT_DIFF_CONTEXT", "two") assert ai_review._int_env("PRAGENT_DIFF_CONTEXT", 1) == 1 diff --git a/tests/pilot/test_webhook_server.py b/tests/pilot/test_webhook_server.py index fd0386a..27d87b5 100644 --- a/tests/pilot/test_webhook_server.py +++ b/tests/pilot/test_webhook_server.py @@ -134,64 +134,3 @@ def test_missing_label_is_ignored(monkeypatch): def test_review_slots_bound_matches_config(): assert ws.MAX_CONCURRENT >= 1 assert ws._review_slots._value <= ws.MAX_CONCURRENT - - -# --------------------------------------------------------------------------- -# AI-USAGE opt-in applied after the triggering label event -# --------------------------------------------------------------------------- - - -def test_run_review_rereads_labels_for_late_ai_usage(monkeypatch): - # Labelling a PR AI-REVIEW then AI-USAGE fires two events; the review - # claims on the first, so the second is deduped and the payload snapshot - # never sees AI-USAGE. The re-read at review start must recover it. - seen = {} - monkeypatch.setattr(ws, "_fetch_current_labels", - lambda repo, index: [{"name": "AI-REVIEW"}, {"name": "AI-USAGE"}]) - monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) - ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") - assert seen["report_usage"] is True - - -def test_run_review_keeps_usage_off_when_label_absent(monkeypatch): - seen = {} - monkeypatch.setattr(ws, "_fetch_current_labels", - lambda repo, index: [{"name": "AI-REVIEW"}]) - monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) - ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") - assert seen["report_usage"] is False - - -def test_fetch_current_labels_hits_the_versioned_api_path(monkeypatch): - # GITEA_API is the bare host; the /api/v1 prefix is the caller's job. - # Getting this wrong 404s silently and the opt-in is lost — which is - # exactly what shipped the first time. - seen = {} - - class _Resp: - def read(self): return b'[{"name": "AI-USAGE"}]' - def __enter__(self): return self - def __exit__(self, *a): return False - - def _urlopen(req, timeout=None): - seen["url"] = req.full_url - return _Resp() - - monkeypatch.setattr(ws.urllib.request, "urlopen", _urlopen) - out = ws._fetch_current_labels("o/r", "9") - assert out == [{"name": "AI-USAGE"}] - assert seen["url"] == f"{ws.GITEA_API}/api/v1/repos/o/r/issues/9/labels" - - -def test_label_reread_failure_is_not_fatal(monkeypatch): - # A dead API must not take the review down with it — the re-read is a - # best-effort upgrade of an opt-in flag, nothing more. - def _boom(*a, **k): - raise OSError("api down") - monkeypatch.setattr(ws.urllib.request, "urlopen", _boom) - assert ws._fetch_current_labels("o/r", "9") == [] - - seen = {} - monkeypatch.setattr(ws, "review_pr", lambda **kw: seen.update(kw) or True) - ws._run_review(("o/r", "9", "abc1234"), "t", "b", False, "main") - assert seen["report_usage"] is False