Skip to content

Commit f507fef

Browse files
committed
Emit AI_REVIEW_DISPATCHED marker at scan dispatch
- Add state_emit module as the single source for greenlight_pr_state S3 rows; verdict now routes through it and the scan emits directly - Introduce STATUS_AI_REVIEW_DISPATCHED / SCAN_ONLY_STATUSES: in-flight for decide() but excluded from the verdict CLI's emittable set - Scan PUTs the marker (run_id = prior+1) via boto3 the instant it fires the reviewer workflow; emit failure is logged and swallowed, IterationTimeout still propagates - Read run_id from state and carry it on PRState so a dispatch row can supersede the PR's prior row - Grant the scan workflow id-token:write + OIDC AWS creds, add boto3, bump the in-flight timeout default 30 -> 45 minutes A reviewer run can sit in GitHub's Actions queue well past a scan interval; recording it as in-flight at dispatch time (rather than only when the run starts) stops the scan from storming re-dispatches while it waits. emit_id now uniquifies the S3 object key so two emits for the same PR in the same millisecond can't overwrite each other. The row JSONEachRow field order/values are the positional S3 -> ClickHouse replicator contract, so both producers share state_emit.emit_row and a golden test pins the byte layout. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
1 parent 81903bf commit f507fef

18 files changed

Lines changed: 877 additions & 89 deletions

.github/workflows/greenlight-review.yml

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ on:
3030
timeout_minutes:
3131
description: "In-flight / re-dispatch timeout (minutes)"
3232
required: false
33-
default: "30"
33+
default: "45"
3434
type: string
3535
log_level:
3636
description: "Log verbosity"
@@ -51,6 +51,7 @@ concurrency:
5151

5252
permissions:
5353
contents: read
54+
id-token: write
5455

5556
defaults:
5657
run:
@@ -105,6 +106,14 @@ jobs:
105106
- name: Sync dependencies
106107
run: just setup
107108

109+
# The scan PUTs an AI_REVIEW_DISPATCHED state row to S3 via boto3 the instant it fires the
110+
# reviewer workflow, so it needs the same OIDC role/arc the reviewer and record jobs use.
111+
- name: Configure AWS credentials via OIDC
112+
uses: aws-actions/configure-aws-credentials@7474bc4690e29a8392af63c5b98e7449536d5c3a # v4.3.1
113+
with:
114+
role-to-assume: arn:aws:iam::308535385114:role/arc
115+
aws-region: us-east-1
116+
108117
- name: Scan and dispatch
109118
env:
110119
# Whichever token step ran (exactly one does): read-only for the listing scan,

greenlight/pyproject.toml

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ name = "greenlight"
33
version = "0.0.0"
44
description = "CLI-invoked periodic iteration service, daemon-capable."
55
requires-python = ">=3.14,<3.15"
6-
dependencies = ["pygithub>=2.6.1", "clickhouse-connect>=0.10", "pyyaml>=6"]
6+
dependencies = ["pygithub>=2.6.1", "clickhouse-connect>=0.10", "pyyaml>=6", "boto3>=1.34"]
77

88
[project.scripts]
99
greenlight = "greenlight.cli:main"
@@ -88,3 +88,10 @@ warn_unused_configs = true
8888
[[tool.mypy.overrides]]
8989
module = "tests.*"
9090
disallow_untyped_defs = false
91+
92+
# boto3/botocore ship no inline types and their stubs are separate packages; the S3 put_object
93+
# call is typed at the seam via a local Protocol and the client Config is a plain data object, so
94+
# these module imports can be treated as untyped.
95+
[[tool.mypy.overrides]]
96+
module = ["boto3", "botocore.config"]
97+
ignore_missing_imports = true

greenlight/src/greenlight/comment_format.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,10 @@ def incomplete_body(reason: str, job_url: str, run_id: int | None) -> str:
103103
def marker_body(status: str, job_url: str, run_id: int | None) -> str:
104104
if status == STATUS_AI_REVIEW_STARTED:
105105
return reviewing_body(job_url, run_id)
106-
# The only remaining marker statuses are the retry outcomes (CANCELLED / FAILED); their
107-
# lowercased name is the human-readable reason shown in the "did not complete" comment.
106+
# AI_REVIEW_STARTED is handled above and AI_REVIEW_DISPATCHED is scan-only (never reaches the
107+
# verdict CLI), so the only marker statuses left here are the retry outcomes (CANCELLED /
108+
# FAILED); their lowercased name is the human-readable reason shown in the "did not complete"
109+
# comment.
108110
return incomplete_body(status.lower(), job_url, run_id)
109111

110112

greenlight/src/greenlight/constants.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,24 @@
99
STATUS_CANCELLED = "CANCELLED"
1010
STATUS_FAILED = "FAILED"
1111
STATUS_AI_REVIEW_STARTED = "AI_REVIEW_STARTED"
12+
STATUS_AI_REVIEW_DISPATCHED = "AI_REVIEW_DISPATCHED"
1213

1314
TERMINAL_STATUSES: frozenset[str] = frozenset({STATUS_LAND, STATUS_NO_LAND})
14-
IN_FLIGHT_STATUSES: frozenset[str] = frozenset({STATUS_AI_REVIEW_STARTED})
15+
IN_FLIGHT_STATUSES: frozenset[str] = frozenset({STATUS_AI_REVIEW_STARTED, STATUS_AI_REVIEW_DISPATCHED})
1516
RETRY_STATUSES: frozenset[str] = frozenset({STATUS_CANCELLED, STATUS_FAILED})
16-
VERDICT_STATUSES: frozenset[str] = TERMINAL_STATUSES | IN_FLIGHT_STATUSES | RETRY_STATUSES
17+
# AI_REVIEW_DISPATCHED is written only by the scan (via state_emit's direct S3 emit) and must stay
18+
# in IN_FLIGHT_STATUSES so decide() treats a queued run as in-flight; but it is never an accepted
19+
# verdict status, so it is subtracted out of the emittable set the verdict CLI validates against.
20+
SCAN_ONLY_STATUSES: frozenset[str] = frozenset({STATUS_AI_REVIEW_DISPATCHED})
21+
VERDICT_STATUSES: frozenset[str] = (TERMINAL_STATUSES | IN_FLIGHT_STATUSES | RETRY_STATUSES) - SCAN_ONLY_STATUSES
1722

1823
# GitHub labels are case-sensitive; the pytorch stale bot uses the exact name "Stale".
1924
STALE_LABEL = "Stale"
2025
EXCLUDED_LABELS: frozenset[str] = frozenset({STALE_LABEL})
2126

27+
# The reviewer and record workflows already write greenlight state rows to this bucket via
28+
# ``aws s3 cp``; the scan's direct boto3 upload targets the same bucket, single-sourced here.
29+
S3_BUCKET = "gha-artifacts"
2230
S3_KEY_PREFIX = "greenlight_pr_state"
2331
EVAL_HASH_RE = re.compile(r"[0-9a-f]{64}")
2432
HEAD_SHA_RE = re.compile(r"[0-9a-fA-F]{40}")

greenlight/src/greenlight/review.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
from typing import TYPE_CHECKING
2424

2525
from greenlight import dispatch as dispatch_module
26-
from greenlight import github_client, scan_runner, state
26+
from greenlight import github_client, scan_runner, state, state_emit
2727
from greenlight.constants import (
2828
DEFAULT_DISPATCH_REF,
2929
DEFAULT_TIMEOUT_MINUTES,
@@ -188,6 +188,7 @@ def run(
188188
fingerprint: FingerprintFn = _default_fingerprint,
189189
read_state: Callable[[str, Sequence[int]], dict[int, PRState]] = state.read_latest_states,
190190
dispatch: Callable[[Github, int, str, str, str], None] = dispatch_module.dispatch_review,
191+
emit_dispatched: Callable[..., None] = state_emit.emit_ai_review_dispatched,
191192
get_pr: Callable[[Github, str, int], VerdictPR] = github_client.get_pr,
192193
upsert_comment: Callable[..., None] = github_client.upsert_issue_comment,
193194
resolve_authorized: Callable[[], frozenset[str]],
@@ -284,7 +285,7 @@ def run(
284285
force=force,
285286
)
286287
dispatch_failed = scan_runner._dispatch_pending(
287-
client, pending, ref=ref, max_dispatches=max_dispatches, dispatch=dispatch
288+
client, pending, ref=ref, max_dispatches=max_dispatches, dispatch=dispatch, emit_dispatched=emit_dispatched
288289
)
289290
# Only the --pr recheck path posts refusals; a listing-scan skip is dropped silently
290291
# (already logged). skips can hold a refusal only when skip_on_approval is False (--pr),

greenlight/src/greenlight/scan_runner.py

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
from datetime import datetime
1515
from typing import TYPE_CHECKING
1616

17-
from greenlight import comment_format
17+
from greenlight import comment_format, constants
1818
from greenlight.decision import Decision, decide
1919
from greenlight.guards import IterationTimeout
2020
from greenlight.review_gate import CHANGES_REQUESTED, ReviewSkip
@@ -199,13 +199,38 @@ def _fingerprint_until_dispatchable(
199199
return pending
200200

201201

202+
def _emit_dispatch_marker(candidate: _Candidate, emit_dispatched: Callable[..., None]) -> None:
203+
# run_id is prior_run_id + 1 so this AI_REVIEW_DISPATCHED row supersedes the PR's prior row,
204+
# while the reviewer run's own later, higher github.run_id supersedes it in turn once that run
205+
# starts. A None state (never-reviewed PR) has no prior run, so it bases at 0 -> run_id 1.
206+
# The workflow was already fired, so an emit failure is logged and swallowed: a missing marker
207+
# self-heals (next scan re-dispatches, the reviewer's per-PR concurrency group cancels the dup),
208+
# and must not fail the scan or block dispatching the remaining candidates.
209+
prior_run_id = candidate.state.run_id if candidate.state else 0
210+
try:
211+
emit_dispatched(
212+
repo=constants.TARGET_REPO,
213+
pr_number=candidate.pr_number,
214+
head_sha=candidate.head_sha,
215+
eval_hash=candidate.eval_hash,
216+
run_id=prior_run_id + 1,
217+
)
218+
except IterationTimeout:
219+
raise
220+
except Exception as exc:
221+
logger.error(
222+
"failed to emit AI_REVIEW_DISPATCHED marker for PR #%d: %s", candidate.pr_number, exc, exc_info=True
223+
)
224+
225+
202226
def _dispatch_pending(
203227
client: Github,
204228
pending: list[_Candidate],
205229
*,
206230
ref: str,
207231
max_dispatches: int | None,
208232
dispatch: Callable[[Github, int, str, str, str], None],
233+
emit_dispatched: Callable[..., None],
209234
) -> list[int]:
210235
ordered = sorted(pending, key=_staleness_key)
211236
limit = len(ordered) if max_dispatches is None else max(0, max_dispatches)
@@ -220,6 +245,7 @@ def _dispatch_pending(
220245
dispatch_failed.append(candidate.pr_number)
221246
continue
222247
logger.info("dispatched review for PR #%d (%s)", candidate.pr_number, candidate.reason)
248+
_emit_dispatch_marker(candidate, emit_dispatched)
223249
for candidate in ordered[limit:]:
224250
logger.info("deferred PR #%d dispatch: --max cap reached", candidate.pr_number)
225251
return dispatch_failed

greenlight/src/greenlight/state.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@
2828

2929
__all__ = ["PRState", "naive_utc", "read_latest_states"]
3030

31-
_QUERY = "SELECT pr_number, status, eval_hash, head_sha, version FROM misc.greenlight_pr_state WHERE repo = %(repo)s"
31+
_QUERY = (
32+
"SELECT pr_number, status, eval_hash, head_sha, version, run_id FROM misc.greenlight_pr_state WHERE repo = %(repo)s"
33+
)
3234
_PR_FILTER = " AND pr_number IN %(pr_numbers)s"
3335
_ORDER_LIMIT = " ORDER BY pr_number, run_id DESC, version DESC LIMIT 1 BY pr_number"
3436

@@ -40,6 +42,7 @@ class PRState:
4042
eval_hash: str
4143
head_sha: str
4244
version: datetime
45+
run_id: int
4346

4447

4548
def naive_utc(value: datetime) -> datetime:
@@ -83,6 +86,7 @@ def read_latest_states(
8386
eval_hash=row["eval_hash"],
8487
head_sha=row["head_sha"],
8588
version=naive_utc(row["version"]),
89+
run_id=int(row["run_id"]),
8690
)
8791
for row in result.named_results()
8892
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
"""Build and emit a ``misc.greenlight_pr_state`` row to S3 for replicator ingestion.
2+
3+
Two producers write state rows and MUST agree byte-for-byte on the JSONEachRow field
4+
order/values and the object-key layout, or the positional S3 -> ClickHouse replicator
5+
silently drops rows. This module is that single source: ``emit_row`` serializes the row and
6+
``object_key`` computes its bucket-relative key. The ``verdict`` command feeds its rows
7+
through here; the scan calls ``emit_ai_review_dispatched`` the instant it fires the reviewer
8+
workflow, so a queued run (which can sit in GitHub's Actions queue well past a scan interval)
9+
is recorded as in-flight immediately and never re-dispatched while it waits.
10+
11+
``verdict`` writes the row to a fixed local path that its workflow ``aws s3 cp``s after a
12+
``success()`` gate; the scan has no such gate, so its default ``upload`` puts the object to
13+
``s3://{S3_BUCKET}/{key}`` directly via boto3's default credential chain (the scan workflow's
14+
OIDC env supplies the AWS creds at runtime).
15+
"""
16+
17+
from __future__ import annotations
18+
19+
import gzip
20+
import json
21+
import uuid
22+
from datetime import UTC, datetime
23+
from typing import TYPE_CHECKING, Protocol, cast
24+
25+
from greenlight.constants import S3_BUCKET, S3_KEY_PREFIX, STATUS_AI_REVIEW_DISPATCHED
26+
27+
if TYPE_CHECKING:
28+
from collections.abc import Callable
29+
30+
__all__ = ["default_emit_id", "emit_ai_review_dispatched", "emit_row", "object_key"]
31+
32+
33+
class _S3Putter(Protocol):
34+
def put_object(self, *, Bucket: str, Key: str, Body: bytes) -> object: ... # pragma: no cover
35+
36+
37+
def _utcnow() -> datetime:
38+
return datetime.now(UTC)
39+
40+
41+
def default_emit_id() -> str:
42+
"""The row schema's default emit-id: a fresh uuid4 hex, single-sourced for every producer."""
43+
return uuid.uuid4().hex
44+
45+
46+
def object_key(repo: str, pr_number: int, version: str, emit_id: str) -> str:
47+
compact = version.replace("-", "").replace(":", "").replace(" ", "T").replace(".", "_")
48+
return f"{S3_KEY_PREFIX}/{repo}/{pr_number}/{compact}-{emit_id}.json.gz"
49+
50+
51+
def emit_row(
52+
*,
53+
repo: str,
54+
pr_number: int,
55+
head_sha: str,
56+
status: str,
57+
reason: str,
58+
eval_hash: str,
59+
message: str,
60+
eval_job: str,
61+
agent_job: str,
62+
run_id: int,
63+
now: Callable[[], datetime],
64+
emit: Callable[[bytes, str], None],
65+
new_emit_id: Callable[[], str],
66+
) -> str:
67+
"""Serialize one state row as a gzipped single-line JSONEachRow and hand it to ``emit``.
68+
69+
Field order and values are the replicator contract; ``emit`` receives ``(gzip_bytes, key)``
70+
where ``key`` is the bucket-relative object key. Returns that key for logging.
71+
"""
72+
version = now().replace(tzinfo=None).isoformat(sep=" ", timespec="milliseconds")
73+
# emit_id is a fresh per-emit UUID that uniquifies both storage keys: the ClickHouse sort key
74+
# (repo, pr_number, run_id, emit_id) so the ReplacingMergeTree never collapses a row, and the
75+
# S3 object key so two emits for the same (repo, pr_number) in the same millisecond do not
76+
# overwrite each other. It is storage-only and never read back.
77+
emit_id = new_emit_id()
78+
row = {
79+
"repo": repo,
80+
"pr_number": pr_number,
81+
"head_sha": head_sha,
82+
"status": status,
83+
"reason": reason,
84+
"eval_hash": eval_hash,
85+
"message": message,
86+
"eval_job": eval_job,
87+
"agent_job": agent_job,
88+
"version": version,
89+
"run_id": run_id,
90+
"emit_id": emit_id,
91+
}
92+
line = json.dumps(row, separators=(",", ":"), ensure_ascii=False) + "\n"
93+
key = object_key(repo, pr_number, version, emit_id)
94+
emit(gzip.compress(line.encode("utf-8"), mtime=0), key)
95+
return key
96+
97+
98+
def _s3_client() -> _S3Putter:
99+
import boto3
100+
from botocore.config import Config
101+
102+
# The emit runs on the scan's main thread; without explicit bounds a hung PUT would inherit
103+
# botocore's 60s connect/read defaults and stall dispatch of the remaining candidates, tripping
104+
# the iteration timeout. Cap both and bound retries so a slow S3 fails fast instead.
105+
config = Config(connect_timeout=5, read_timeout=5, retries={"max_attempts": 3, "mode": "standard"})
106+
return cast("_S3Putter", boto3.client("s3", config=config))
107+
108+
109+
def _default_upload(row_gzip: bytes, key: str) -> None:
110+
_s3_client().put_object(Bucket=S3_BUCKET, Key=key, Body=row_gzip)
111+
112+
113+
def emit_ai_review_dispatched(
114+
*,
115+
repo: str,
116+
pr_number: int,
117+
head_sha: str,
118+
eval_hash: str,
119+
run_id: int,
120+
upload: Callable[[bytes, str], None] | None = None,
121+
) -> None:
122+
"""Emit an ``AI_REVIEW_DISPATCHED`` row the instant the scan fires the reviewer workflow.
123+
124+
``run_id`` is supplied by the caller (the scan computes the next run id); reason/message and
125+
the job URLs are empty, matching the ``AI_REVIEW_STARTED`` marker. ``upload`` is an injectable
126+
``(gzip_bytes, key)`` seam for tests; the default puts the object via boto3.
127+
"""
128+
emit_row(
129+
repo=repo,
130+
pr_number=pr_number,
131+
head_sha=head_sha,
132+
status=STATUS_AI_REVIEW_DISPATCHED,
133+
reason="",
134+
eval_hash=eval_hash,
135+
message="",
136+
eval_job="",
137+
agent_job="",
138+
run_id=run_id,
139+
now=_utcnow,
140+
emit=upload if upload is not None else _default_upload,
141+
new_emit_id=default_emit_id,
142+
)

0 commit comments

Comments
 (0)