[GreenLight] Add decision export and policy replay harness (dev tooling) - #8829
Merged
Merged
Conversation
- New tools/torchci/greenlight_decisions: one CSV row per pull request greenlight has recorded state for, so its verdicts can be reviewed in a spreadsheet. Verdict, PR status, human reviews and PR size come from one ClickHouse query; the size of the diff each verdict actually judged comes from the GitHub compare API - Split by responsibility: sql/query own the ClickHouse side, classify/loc own diff sizing and verdict staleness, rows owns the CSV shape, __main__ owns the CLI and exit codes - Tests cover all four, driven through fake clients and patched compare calls, so the suite touches neither ClickHouse nor GitHub; two trimmed compare-API payloads land in tests/fixtures - README documents every column and the four ways the file is easy to misread; .gitignore covers the default output filename, which lands in the cwd Notes: The export answers "what did greenlight say about the code that shipped", which drives most of the non-obvious choices: A PR accumulates verdicts, and the row keeps the one whose head SHA matches the PR's final head, falling back to run_id/version recency only when no verdict names it. verdict_staleness says which case a row got, and n_terminal_decisions / verdict_flipped mark rows that summarise a sequence. landed and reverted come from the trailers mergebot writes on main, not from misc.greenlight_pr_state's REVERTED rows -- those only exist for PRs still open when a scan listed them, and missed 3 of 10 real reverts, every one a NO_LAND. Because a trailer format change would degrade silently to "closed-abandoned" on every landed PR, fetch_decisions refuses a result in which the trailer matched nothing at all. loc is measured at the judged head, not the current one, and is deliberately a separate column from pr_loc. Comment stripping is line-local and per-extension rather than parser-based: the error is size-bounded, so it is least accurate exactly where accuracy matters least. Failure handling is per row, not per run: a PR that cannot be measured gets blank LOC cells and a reason in loc_status. Only a wholesale failure (more than half the attempted measurements) exits 3, because then the LOC and staleness columns describe the outage rather than the pull requests. A comparison that could not be made reports not-measured, never content-changed, so an outage cannot manufacture an audit list. The CSV is written via a temp file and os.replace, carries a UTF-8 BOM for Excel, and prefixes formula-leading cells with an apostrophe -- decision messages are LLM prose that legitimately open with a "-" bullet. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
jeanschmidt
marked this pull request as draft
September 17, 2026 22:26
- Fold a per-PR max(shadow) into the corpus CTE and surface it as an is_shadow column, coerced to bool alongside landed and reverted - Document what a shadow evaluation withholds, how authority is decided per row, and why the flag describes the PR rather than the verdict - Add a fifth misreading: is_shadow = false over the older half of the file is a DEFAULT-backfilled column, not a measurement - Cover both properties in tests: the flag is an OR over every state row (not a reading off decision_row), and it stays populated and renders true/false on a PR that never reached a verdict Notes: The flag has to be aggregated per PR rather than read off the selected verdict row, because a PR greenlight never reached a verdict on still carries it -- 25 of today's 82 shadow PRs are REVERTED exclusions with no review at all. Reading it off decision_row would report that whole population as enforcing. max() rather than min() is the same attribution the greenlight_quality_coverage and greenlight_quality_reverts tiles make, so this export and the HUD bucket a PR alike. The cost is one error direction, recorded in both sql.py and the README: a PR whose author joins TRUSTED_AUTHORS mid-review holds rows of both kinds and reads shadow even though the verdict that applies carried authority. Not observed as of 2026-09-17 -- every PR's rows agree, and the set's three additions all predate the first recorded shadow row -- and no column in the CSV bounds it, n_terminal_decisions least of all since it counts only LAND/NO_LAND rows while the OR runs over all of them. The README's arithmetic is the reason for the new misreading section: the column exists across the whole file but the question only exists after 2026-09-10T00:52:30Z, so a naive rate reads 22% where the population that can answer reads 51%, or 41.6% once the never-reviewed rows are dropped. None of that split is reproducible from the CSV itself, which is why it is written down here. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
- New tools/torchci/greenlight_replay: takes the decision export, frames it down to PRs whose verdict can be held against a real landing, samples them, and re-reviews each one under a candidate policy PR, writing the export back out with four appended new_decision_* columns. - The policy is materialized from the test-infra PR's own tree and read by shape, not position: prompt, diff caps, review budgets, hooks, verdict schema, canned too-large verdict and sanitize step all come from the ref under test. The workflow's Bedrock inference profile is mapped to a local CLI model rather than defaulted, so an unrecognised one stops the sweep. - Concurrency: a worktree pool over one shared blobless pytorch clone hands out reviewer workspaces, and a PreToolUse hook remaps the skill's three hardcoded /tmp scratch paths per run so parallel reviewers cannot read each other's diffs or overwrite each other's verdicts. - Durability: one fsynced JSONL line per finished PR, and --resume reuses an entry only when the input it records still matches the row in front of it. Exit 3 when more than half the attempted runs reached no verdict. - rows.write_csv gains a columns parameter so the replay reuses the export's BOM, atomic replace and formula guard instead of forking them. PyYAML is added to requirements for reading the policy workflow. Notes: This answers "what would this policy PR have said about these PRs", one judgement apiece. It is not a measured policy effect and does not build a control arm: the stored decision column spans nine policy versions with a 51pp spread in NO_LAND rate, and the reviewer returns a different status on 18.3% of repeated runs against the same head. A row where decision and new_decision disagree is a PR to go read. Sweeps cost real money -- about $1.33 and ten minutes per PR -- so --dry-run resolves the frame, the sample and the bill without invoking a model, and the first PR runs alone as both a prompt-cache warm-up and a circuit breaker for faults that would repeat on every remaining run. The reviewer is invoked as claude directly rather than through a local wrapper: a wrapper that forces --dangerously-skip-permissions makes --restricted unusable and loads MCP tools, including GitHub writes, into a session reading untrusted code. README.md records the five deliberate deviations from CI and the five ways to misread the output. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
- Move the startup checks out of sweep into their own preflight module, and add one that asks the remote whether the policy ref exists at all - Run the full preflight from --dry-run, so a mistyped --policy-pr is caught before a real run has built a scratch tree and a bare clone to discover it - Teach the verdict validator allOf, if/then/else and const, applied recursively for both the support check and the verdict check - Refuse an uninterpretable schema in policy.materialize, once per sweep and before anything is spent, and take the validator from verdict rather than transcript - Gate the CLI's tracebacks on DEBUG, and rewrite the README and dry-run log to say what is checked and what is not Notes: #8814 is the first policy PR to use a schema keyword this harness did not implement: a LAND may carry only the reason "clean", expressed as an if/then under allOf. Accepting the keyword and skipping the constraint would have been worse than refusing the schema, because the sweep would run and the rows would look fine while a LAND stamped with a reason that objects to landing sailed through -- the land-time guard reads only the status. The support check therefore recurses too, so allOf cannot become a place to smuggle unenforced constraints past the harness. policy._refspec becomes public because the preflight check must ask the remote about the same refspec materialize will fetch; resolving it separately would let a dry run bless a ref the run never asks for, which is the exact failure the check exists to catch. The dry run still does not fetch the policy -- materializing it would leave the directory a subsequent real run has to find empty -- so schema, hooks and model mapping remain checked at the start of a real run instead. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
- Extract prepare_policy from open_sweep and call it from the dry-run path, so a dry run materializes the policy and checks the workflow, diff caps, canned verdict, verdict schema, hook scripts and model mapping - Drop preflight.resolve_refspec in favour of policy.refspec, so the ls-remote check and the real fetch cannot resolve a ref differently - Rewrite the dry-run plan log and the README to state what is now checked and what is left as per-run work - Cover the three newly caught faults in test_cli: an unreadable verdict schema, an unrunnable hook and an unmapped inference profile Notes: The dry run existed to price a sweep before spending on one, but it skipped every check that needs the policy tree. A mistyped verdict schema or a hook that cannot exec priced a clean plan and then aborted the real run minutes later, after the pytorch clone. Those faults are properties of the policy, identical for every pull request in the corpus, so the cheapest run should catch them. The old reason for skipping them was that materializing would leave the policy directory non-empty for a subsequent real run. That is not true: materialize clears its destination and re-extracts, which is what --resume already depends on. The cost of fetching is a few seconds and about 34 MB. Still not done in a dry run: the blobless pytorch clone, the worktree slots, and each pull request's diff and metadata. Those are gigabytes and one GitHub round trip per pull request, and none of them can fail in a way that is a property of the policy. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
jeanschmidt
marked this pull request as ready for review
September 21, 2026 03:15
jeanschmidt
changed the title
- Add the missing `-> None` annotation to `FakeGh.__init__` in the greenlight_replay inputs tests Notes: Every other helper in the module is annotated; without a return annotation some type checkers skip the body of `__init__` entirely, so the attribute types declared there were going unchecked. Signed-off-by: Jean Schmidt <contato@jschmidt.me>
huydhn
approved these changes
Sep 21, 2026
huydhn
left a comment
huydhn
left a comment
Contributor
There was a problem hiding this comment.
Stamped to unblock! This is our own tool anyway
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Impact: dev tooling only
Risk: low
What
Two new local-only tools:
python -m torchci.greenlight_decisions— exports one CSV row per PR greenlight has recorded state for (verdict, PR outcome, human reviews, diff size), so decisions can be eyeballed in a spreadsheet. Read-only: one ClickHouse query plus GitHub compare calls.python -m torchci.greenlight_replay— takes that CSV, samples PRs from it, and re-runs the reviewer locally under the policy carried by a test-infra PR, appending fournew_decision_*columns. Used to sanity-check a policy change before shipping it.Neither tool is wired into any workflow, Lambda, or HUD page. Nothing in production imports them, and nothing runs on a schedule. They are development helpers — run by hand, on a laptop, when someone is looking at greenlight's behaviour.
Why
There was no way to get any read on a policy change before it went live. The replay gives a rough before/after on a sample of real PRs.