fix(review): read the AI-USAGE opt-in at render time, not at review start
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 from4ef62f2andaedea97. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B11e8TZZxJyzHW7jj7KWUN
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user