[CXH-2780] Let CI submit an approval, not only a block (verdict_mode) by FeliLucero1 · Pull Request #147 · ConductorOne/github-workflows

FeliLucero1 · 2026-10-11T18:35:57Z

The problem

publish-review-report.py hardcoded VERDICT_MODE = "baseline". Baseline maps the blocking count: N > 0 → REQUEST_CHANGES, N == 0 → COMMENT. It has no APPROVE path, deliberately.

On GitHub a COMMENT review never clears a reviewer's earlier CHANGES_REQUESTED. Only a later APPROVE from that reviewer, or a human dismissal, does. So once the bot blocks, it stays blocked after the author fixes the finding — until a person clicks dismiss.

flowchart TD
    B["The bot has submitted REQUEST_CHANGES.<br/>GitHub now records CHANGES_REQUESTED — the PR is blocked."]
    B --> Q{"What clears it?"}
    Q --> C["Another COMMENT review<br/><b>No effect.</b> GitHub keeps the block.<br/>This is the only other event baseline can send."]
    Q --> H["A human clicks Dismiss<br/><b>Works.</b> 4 of our 4 blocked PRs needed this."]
    Q --> A["The bot sends APPROVE<br/><b>Works.</b> Only judge mode can send it."]

Baseline's whole vocabulary is REQUEST_CHANGES and COMMENT, and neither of
them clears a block. So once it blocks, a person has to come and undo it by
hand.

Measured across 54 recently-merged connector PRs (okta, jira, aws, datadog, github):

4 received a bot CHANGES_REQUESTED
4 of 4 required a manual DISMISSED — none cleared itself
worst case baton-okta#214: blocked 2026-09-16 22:57, dismissed 2026-09-18 19:00 (~2 days)
baton-datadog#40 was blocked four separate times and accumulated 71 bot reviews

What this does

Makes the mode an action input — judge | baseline | none — and points the connector profile at judge. The action's own default stays baseline, so anything that doesn't ask for a mode keeps today's behaviour.

Mode	Event chosen from	Submits
`baseline` (default)	the `Blocking Issues: N` count	`REQUEST_CHANGES` / `COMMENT`, never approves
`judge`	an explicit `Verdict: ...` row, with `DEFER` falling back to the count	`APPROVE` / `REQUEST_CHANGES` / `COMMENT`
`none`	—	nothing; report only

flowchart LR
    S["review summary"] --> M{"verdict_mode"}
    M -->|baseline| B{"blocking<br/>count N"}
    B -->|"N &gt; 0"| RC["REQUEST_CHANGES"]
    B -->|"N = 0"| C["COMMENT"]
    M -->|judge| V{"verdict row"}
    V -->|"APPROVE"| A["APPROVE"]
    V -->|"REQUEST CHANGES"| RC
    V -->|"DEFER"| B
    M -->|none| X["no review submitted"]

DEFER routing back into the count is the point: deferring is a statement
about coverage, not severity, so it declines to override baseline rather
than declining to decide. Judge mode's only addition over baseline is
APPROVE — it can never leave a PR unblocked that baseline would have
blocked.

ductone/github-workflows already ships these three modes; this is a port, not a new design. Two deliberate divergences, both because their judge prompt is a separate 21KB file and ours is a mixin appended to the shared base: the verdict row goes after the count row rather than before, and the judge instructions are ~70 lines rather than a second copy of the whole prompt.

DEFER is the part that makes approving safe. An APPROVE satisfies the PR's review requirement for the whole change regardless of how much of the diff was actually read, so a review that found nothing blocking but couldn't cover everything defers to a human instead of approving by default. An APPROVE written over a positive blocking count is a contradiction between two rows of the same summary — CI refuses to pick a side and fails the run.

Why turning it on is safe here

The connector profile (pr-review.yaml) now defaults to judge. That is a
behaviour change, so here is the evidence it rests on.

Measured, not assumed. I ran a throwaway probe (#148, closed) on a PR
targeting protected main, with a plain GITHUB_TOKEN and no extra config:

	result
bot submits `REQUEST_CHANGES`	works (control)
bot dismisses a review it authored	works — went to `DISMISSED`
bot submits `APPROVE`	works — went to `APPROVED`

The protected-branch caveat in the docs ("you must be a repository
administrator or be included in the list of people or teams who can dismiss")
only applies when a dismissal allow-list is configured, and none of our repos
configure one.

Scope is what makes it safe. 60 of 60 connector repos sampled require 0
approving reviews. So a bot APPROVE there satisfies nothing — its only
effect is clearing the bot's own CHANGES_REQUESTED, which is the entire
point. The three costs of judge mode mostly evaporate at 0 required approvals:

The bot gets a vote — there is no vote to get.
Approvals don't self-retract — a stale approval that grants nothing is inert. (Confirmed: dismiss_stale_reviews_on_push is false across these repos.)
It raises the ceiling on comment-edit and prompt-injection — on a connector repo that buys the attacker nothing they lack, since anyone with write can already dismiss the bot's review by hand. That is literally how all 4 of our blocks were cleared.

The one repo where it would matter is excluded by construction.
github-workflows is the only one requiring an approving review, and it is
reviewed by general-pr-review.yaml — which passes no verdict_mode and so
keeps the action's conservative baseline default. The bot never gets a vote
that could satisfy a human review requirement.

That asymmetry between the two workflows is now load-bearing rather than
incidental, so the README says so: a repo on the connector profile that
starts requiring approvals has to move off it or pin baseline.

What this still costs. Judge adds a row the model has to write. If it
writes it wrong the run goes red and a finished Opus review is lost. I relaxed
the row's required position to the minimum that stays safe — one canonical
line, top level, exactly once — but the risk isn't zero. Closing (3) properly
still wants a check that the summary's last editor was the bot; GraphQL
exposes editor and lastEditedAt. That's a separate PR.

Second commit

A deep review of the first commit found two real defects, both fixed in cb78605:

none skipped the summary parse entirely. A run whose agent got cut off mid-sentence published the fragment as a completed report, advanced last_reviewed_sha and exited 0 — permanently retiring commits nobody reviewed from the next run's incremental diff. Every mode now requires the count row; it's the only structural proof the agent finished.
Judge pinned the verdict row to the second row after the heading. The base prompt — which judge appends to rather than replaces — teaches a template whose second row is **Criteria:**, so the agent had to be argued out of a position it had just been given, and complying with the base meant a red run that threw away a completed Opus review. The row is now accepted anywhere at top level below the heading; position was never the safety property (whole canonical line, outside any fence, exactly one per summary).

Also in that commit: an explicitly-empty verdict_mode hard-failed the action while the Python read it as baseline (a composite action's default: doesn't apply to an input passed as ""); the module docstring, the action.yml step description and the README all still said the reviewer "never approves"; and DEFER's criteria told the agent to defer whenever repo-local criteria failed to load — for most repos "no criteria file" is the normal state, so judge would have deferred on every PR and cleared nothing.

Third commit

The bot's own review of this PR caught one more, and was right: DEFER mapped
to COMMENT at any count, so a judge run could list confirmed blockers and
still leave the PR unblocked — strictly weaker than baseline on the same
summary, and backwards as an incentive, since the hedging word was also the
permissive one. c9b7741 makes DEFER fall back to the baseline mapping of
its own count row.

It suggested either that or rejecting the combination as a contradiction the
way APPROVE over a positive count is rejected. Falling back is better here:
unlike APPROVE, where the two rows genuinely disagree and there is no safe
side to pick, DEFER leaves the count row as the only claim about severity —
so there is something to fall back to, and no reason to spend a finished
review on it.

Fifth commit

Points pr-review.yaml — the connector profile — at judge. Both defaults move, because they cover different callers: the workflow_call input for repos calling this reusably, and the || fallback for the direct pull_request trigger, where inputs is empty and the input default never applies. The second one is how every connector repo actually runs it.

Tests

163 → 166 tests, all passing. These tests had never run anywhere — they were in no Makefile target and no workflow. They're now in make test-scripts.

That said: nothing in this repo runs make test in CI. verify.yaml is a reusable workflow for caller repos; go test there runs against the caller's checkout, not this repo's scripts. So the suite still only runs when someone runs make locally. Wiring up a self-test workflow is a separate PR — happy to open it.

Also left alone: general-pr-review.yaml exposes no inputs at all, so it has no verdict_mode and keeps the baseline default. Pre-existing asymmetry, same as summary_marker.

🤖 Generated with Claude Code

publish-review-report.py hardcoded VERDICT_MODE = "baseline", whose mapping is count-driven and has no APPROVE path: N > 0 submits REQUEST_CHANGES and N == 0 submits COMMENT. On GitHub a COMMENT review never clears a reviewer's earlier CHANGES_REQUESTED — only a later APPROVE from that reviewer, or a human dismissal, does. So a blocking finding keeps a PR blocked after the author fixes it, until someone dismisses the review by hand. Make the mode an action input (judge | baseline | none), defaulting to baseline so an un-upgraded caller is unchanged: - judge reads an explicit "**Verdict: APPROVE | REQUEST CHANGES | DEFER**" row and submits APPROVE / REQUEST_CHANGES / COMMENT from it. DEFER is what makes approval safe: an APPROVE satisfies the PR's review requirement for the whole change, so a review that found nothing blocking but could not cover the whole diff defers to a human instead of approving by default. An APPROVE over a positive blocking count is a contradiction between two rows of the same summary and fails the run rather than picking a side. - none publishes the report and submits no review. With nothing to wait for, its report is composed as publication="completed" directly. ductone/github-workflows ships the same three modes with a separate 21KB judge prompt file. Here the judge instructions are a mixin appended to the shared base prompt, which is also why the verdict row goes after the count row instead of before it: leaving the base template's first row alone lets one template serve both modes. The verdict row gets the same anchoring the count row already had — one canonical top-level row, fences stripped, no value smuggled in through the PR title — and _rows_after_heading is factored out so both parsers locate their rows the same way. The pr-review script tests were in no Makefile target and no workflow, so they have never run anywhere; add them to test-scripts. Nothing in this repo runs `make test` in CI yet — that is still open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two defects in the first commit, plus the prose it left behind. "none" skipped the summary parse entirely, so a run whose agent was cut off mid-sentence published that fragment as a completed report, advanced last_reviewed_sha, and exited 0 — retiring commits nobody reviewed from the next run's incremental diff. The count row is the only structural proof the agent finished, so every mode now requires it; "none" just maps it to a NO_REVIEW sentinel instead of an event. That also makes verdict_to_review total over VERDICT_MODES, so neither caller can reach PARSE_REQUIREMENT with a key it lacks. Judge mode required the verdict row at exactly the second row after the heading. The base prompt — which judge mode appends to rather than replacing — teaches a template whose second row is **Criteria:**, so the agent had to be talked out of a position it was just told to use, and the cost of it complying with the base was a red run that threw away a completed Opus review. The row is now accepted anywhere at top level below the heading. Position was never what made this safe: the row must be a whole canonical line outside any fence, and exactly one may exist in the summary. The count row keeps its prescribed slot and is now read through parse_blocking_count itself, so judge and baseline cannot drift about the same summary. An explicitly-empty verdict_mode hard-failed the action while the Python read it as baseline. A composite action's default does not apply to an input passed as "", so any caller forwarding its own optional input hit that — the review_prompt case two blocks up already handles this; verdict_mode now does too. The module docstring, the action.yml step description and the README still said the reviewer "never approves" and "validates the baseline count row", which is what a maintainer reads before touching the idempotency machinery. All three now describe the three modes. README also contradicted itself twice in prose this branch added, and said nothing about what enabling judge costs: a bot APPROVE counts toward required approving reviews, nothing here dismisses a stale approval when the head moves, and because publication picks the summary comment by author and freshness rather than by a pre-bound id, a write-access user editing the bot's comment goes from suppressing a block (baseline) to manufacturing an approval (judge). That is now written down as the precondition for turning judge on anywhere. DEFER's criteria told the agent to defer whenever repo-local criteria failed to load; for most repos no criteria file is the normal state, so judge would have deferred on every PR and cleared nothing. from __future__ import annotations so the suite the Makefile now runs can be imported by the 3.9 still shipping as system python3 on macOS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-11T18:36:10Z

Superseded — see the current review report for commit cb7860503347

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: full
View review run

Review Summary

This adds a verdict_mode input (baseline by default, plus judge and none) to the pr-review action and the pr-review.yaml reusable workflow. When the mode is judge, the action adds the verdict-judge.md prompt layer, and publish-review-report.py picks the review event from the mode. Judge mode parses a **Verdict: ...** row that must sit at top level, appear exactly once, and come after the heading. That mode can submit APPROVE, and REVIEW_STATE_FOR_EVENT gains the matching entry so replays recognize their own approval. none still has to parse the count row and publishes its report straight to completed. The superseded-comment cleanup is moved into a helper with no change in behaviour. Unknown modes fail closed in both the shell step and the Python script; an empty value becomes baseline.

I scanned the full diff for security and correctness and checked the surrounding code. State selection in _review_state.py and fetch-pr-context.py filters by workflow_ref, not by verdict_mode, so switching modes does not break incremental state. Replay and identity binding include the mode. TerminalError from an invalid mode is caught in main(). The default stays baseline, so existing callers are unaffected. The risks of judge mode that the PR description names (the bot's approval counts as a vote, approvals are never dismissed by the action, and an edited comment or prompt injection could produce an approval) are real but documented, and they apply only to callers who opt in. No repo-local criteria loaded; the base criteria and the shared-library compatibility criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (medium confidence) .github/actions/pr-review/scripts/publish-review-report.py:406: judge DEFER becomes COMMENT whatever the blocking count. A summary that reports N > 0 and defers leaves the PR unblocked where baseline would block it. This is documented as deliberate in the README, but the safer mapping is REQUEST_CHANGES (or rejecting the summary) when N > 0.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/scripts/publish-review-report.py`:
- Around line 400-406 (verdict_to_review, judge branch): DEFER always becomes COMMENT, even when the parsed blocking count is above 0, so a judge run that reports blockers and defers leaves the PR unblocked. Return ("REQUEST_CHANGES", ...) when verdict == "DEFER" and blocking > 0, or reject it in parse_judge_verdict as is done for APPROVE over a positive count. Update the README "Verdict Modes" paragraph, the verdict-judge.md guidance and JudgeVerdictParsingTest (the ("DEFER", 2, "COMMENT") case) to match.

github-actions · 2026-10-11T18:37:42Z

Superseded — see the current review report for commit c9b7741b1722

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: full
View review run

Review Summary

This adds a verdict_mode input (baseline by default, plus judge and none) to the pr-review action and the pr-review.yaml reusable workflow. When the mode is judge, the action adds the verdict-judge.md prompt layer, and publish-review-report.py picks the review event from the mode. Judge mode parses a **Verdict: ...** row that must sit at top level, appear exactly once, and come after the heading. That mode can submit APPROVE, and REVIEW_STATE_FOR_EVENT gains the matching entry so replays recognize their own approval. none still has to parse the count row and publishes its report straight to completed. The superseded-comment cleanup is moved into a helper with no change in behaviour. Unknown modes fail closed in both the shell step and the Python script; an empty value becomes baseline.

I scanned the full diff for security and correctness and checked the surrounding code. State selection in _review_state.py and fetch-pr-context.py filters by workflow_ref, not by verdict_mode, so switching modes does not break incremental state. Replay and identity binding include the mode. TerminalError from an invalid mode is caught in main(). The default stays baseline, so existing callers are unaffected. The risks of judge mode that the PR description names (the bot's approval counts as a vote, approvals are never dismissed by the action, and an edited comment or prompt injection could produce an approval) are real but documented, and they apply only to callers who opt in. No repo-local criteria loaded; the base criteria and the shared-library compatibility criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (medium confidence) .github/actions/pr-review/scripts/publish-review-report.py:406: judge DEFER becomes COMMENT whatever the blocking count. A summary that reports N > 0 and defers leaves the PR unblocked where baseline would block it. This is documented as deliberate in the README, but the safer mapping is REQUEST_CHANGES (or rejecting the summary) when N > 0.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/scripts/publish-review-report.py`:
- Around line 400-406 (verdict_to_review, judge branch): DEFER always becomes COMMENT, even when the parsed blocking count is above 0, so a judge run that reports blockers and defers leaves the PR unblocked. Return ("REQUEST_CHANGES", ...) when verdict == "DEFER" and blocking > 0, or reject it in parse_judge_verdict as is done for APPROVE over a positive count. Update the README "Verdict Modes" paragraph, the verdict-judge.md guidance and JudgeVerdictParsingTest (the ("DEFER", 2, "COMMENT") case) to match.

Reviewed commit: cb7860503347

github-actions

No blocking issues found — see the full review report

DEFER mapped to COMMENT at any blocking count, so a judge run could list confirmed blockers and still leave the PR unblocked — strictly weaker than baseline on the same summary, and backwards as an incentive: the hedging word was also the permissive one. DEFER is a statement about coverage, not severity. It now declines to override rather than declining to decide: it falls back to the baseline mapping of its own count row, COMMENT at zero and REQUEST_CHANGES above. Judge mode's only addition over baseline is APPROVE; everything else is baseline's answer or stricter. Resolving it this way rather than rejecting the combination as a contradiction keeps a merely inconsistent-looking summary from costing a finished review — the count row is already unambiguous about what to do, so there is no side to pick. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-11T18:41:52Z

Superseded — see the current review report for commit c9b7741b1722

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since cb78605
View review run

Review Summary

The new commit c9b7741 changes judge-mode DEFER so it uses the baseline mapping of its own count row. It now submits COMMENT at N == 0 and REQUEST_CHANGES when N > 0 (publish-review-report.py:414-421). The commit also updates the judge prompt, the docstrings and the README to match, and adds unit and end-to-end tests showing that judge mode never submits a weaker review than baseline would. I scanned the full PR diff (action.yml input validation and env passing, the workflow input passthrough, Makefile, publish script, tests, prompt, README) for security and correctness. I found no new issues: verdict_mode reaches shell only through env and is checked against an allowlist with case. No repo-local criteria loaded, so the review used the base criteria only. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (high confidence, docs) README.md:109-115: the new DEFER paragraph sits between the judge and none table rows. That ends the Markdown table, so the none row renders as a stray line of text instead of a table row.

Resolved prior findings

Judge DEFER over N > 0 leaving the PR unblocked: fixed. publish-review-report.py:414-421 now returns REQUEST_CHANGES when a DEFER summary has blocking > 0. It is covered by test_each_verdict_maps_to_its_event (("DEFER", 2/9, "REQUEST_CHANGES")) and test_judge_never_submits_less_than_baseline_would.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `README.md`:
- Around line 105-115: The paragraph starting "In judge mode `APPROVE` and `REQUEST CHANGES` map straight through..." was inserted between the `judge` table row and the `| none | — | nothing; the report is published alone |` row. That ends the Markdown table early, so the `none` row no longer renders as a table row. Move the `none` row up to sit directly under the `judge` row, and put the paragraph after the table, separated by a blank line.

github-actions · 2026-10-11T18:43:09Z

Superseded — see the current review report for commit 4b6f6fc8ee9a

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since cb78605
View review run

Review Summary

The new commit c9b7741 changes judge-mode DEFER so it uses the baseline mapping of its own count row. It now submits COMMENT at N == 0 and REQUEST_CHANGES when N > 0 (publish-review-report.py:414-421). The commit also updates the judge prompt, the docstrings and the README to match, and adds unit and end-to-end tests showing that judge mode never submits a weaker review than baseline would. I scanned the full PR diff (action.yml input validation and env passing, the workflow input passthrough, Makefile, publish script, tests, prompt, README) for security and correctness. I found no new issues: verdict_mode reaches shell only through env and is checked against an allowlist with case. No repo-local criteria loaded, so the review used the base criteria only. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (high confidence, docs) README.md:109-115: the new DEFER paragraph sits between the judge and none table rows. That ends the Markdown table, so the none row renders as a stray line of text instead of a table row.

Resolved prior findings

Judge DEFER over N > 0 leaving the PR unblocked: fixed. publish-review-report.py:414-421 now returns REQUEST_CHANGES when a DEFER summary has blocking > 0. It is covered by test_each_verdict_maps_to_its_event (("DEFER", 2/9, "REQUEST_CHANGES")) and test_judge_never_submits_less_than_baseline_would.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `README.md`:
- Around line 105-115: The paragraph starting "In judge mode `APPROVE` and `REQUEST CHANGES` map straight through..." was inserted between the `judge` table row and the `| none | — | nothing; the report is published alone |` row. That ends the Markdown table early, so the `none` row no longer renders as a table row. Move the `none` row up to sit directly under the `judge` row, and put the paragraph after the table, separated by a blank line.

Reviewed commit: c9b7741b1722

github-actions

No blocking issues found — see the full review report

The paragraph explaining DEFER's fallback landed between the judge and none rows, which ends a Markdown table: none rendered as a stray line of text rather than a row. Move it below the table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-11T18:45:27Z

Superseded — see the current review report for commit 4b6f6fc8ee9a

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since c9b7741
View review run

Review Summary

The new commit (4b6f6fc) only changes README.md. It moves the | none | — | ... | row of the Verdict Modes table up under the judge row, so the DEFER paragraph now comes after the table. I also re-scanned the full PR diff for security and correctness: action.yml input validation and prompt assembly, verdict-judge.md, the publish-review-report.py mode parsing and event mapping, the none path that is published already completed, the workflow input, and the Makefile test target. The new commit adds no new issues. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was complete, with no paths dropped or truncated.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

README Verdict Modes table ended early by the DEFER paragraph, which left the none row outside the table: fixed. The none row is now at README.md:109, directly under the judge row, and the paragraph starts after a blank line at README.md:111.

github-actions · 2026-10-11T18:46:27Z

Superseded — see the current review report for commit d3edc36fa153

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since c9b7741
View review run

Review Summary

The new commit (4b6f6fc) only changes README.md. It moves the | none | — | ... | row of the Verdict Modes table up under the judge row, so the DEFER paragraph now comes after the table. I also re-scanned the full PR diff for security and correctness: action.yml input validation and prompt assembly, verdict-judge.md, the publish-review-report.py mode parsing and event mapping, the none path that is published already completed, the workflow input, and the Makefile test target. The new commit adds no new issues. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was complete, with no paths dropped or truncated.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

README Verdict Modes table ended early by the DEFER paragraph, which left the none row outside the table: fixed. The none row is now at README.md:109, directly under the judge row, and the paragraph starts after a blank line at README.md:111.

Reviewed commit: 4b6f6fc8ee9a

github-actions

No blocking issues found — see the full review report

The mechanism was in place but nothing used it, so the measured problem — 4 of 4 bot blocks needed a manual dismiss, the worst sitting two days — was untouched. Point the connector profile at judge. Measured first, on a PR against a protected main with a plain GITHUB_TOKEN and no extra configuration: github-actions[bot] can submit APPROVE, and can dismiss a review it authored itself. The protected-branch caveat in GitHub's docs ("you must be a repository administrator or be included in the list of people or teams who can dismiss") only bites when a dismissal allow-list is configured, and none of our repos configure one. Scope is the reason this is safe. 60 of 60 connector repos sampled require 0 approving reviews, so a bot APPROVE satisfies nothing there; its only effect is clearing the bot's own CHANGES_REQUESTED, which is the entire point. The one repo that does require an approval is github-workflows, and it is reviewed by general-pr-review.yaml, which passes no verdict_mode and so keeps the action's conservative default. The asymmetry between the two workflows is now carrying weight, so it is written down rather than left to be rediscovered: a repo on the connector profile that starts requiring approvals has to move off it or pin baseline. Both defaults move, because they cover different callers: the workflow_call input for repos that call this reusably, and the `|| ` fallback for the direct pull_request trigger, where inputs is empty and the input default never applies — which is how every connector repo actually runs it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-11T19:06:48Z

Superseded — see the current review report for commit d3edc36fa153

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since 4b6f6fc
View review run

Review Summary

The new commit d3edc36 makes judge the default verdict_mode on the connector profile pr-review.yaml. It covers both the workflow_call input default and the inputs.verdict_mode || 'judge' fallback used by the org-rule pull_request trigger. It also rewrites the README cost section to argue that judge mode is acceptable on connector repos because they require 0 approvals and only same-repo PRs are reviewed. general-pr-review.yaml and direct action calls stay on baseline. I scanned the full PR diff for security and correctness: the verdict_mode validation in action.yml and the Python publisher, judge/DEFER/none mapping, REVIEW_STATE_FOR_EVENT gaining APPROVE, none-mode completion and collapse, and the new tests. No blocking defects found. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New — .github/workflows/pr-review.yaml:69 (confidence: high): the PR description doesn't match the diff. It still says "defaulting to baseline, so nothing changes until a caller opts in", has a section called "The decision this PR does not make", and says the editor-binding fix should land before judge is enabled. This commit now turns judge on for every connector repo.
New — .github/workflows/pr-review.yaml:19 (confidence: medium): every pr-review.yaml@main caller that doesn't set verdict_mode silently starts submitting APPROVE. The README's safety argument ("connector repos require 0 approving reviews", README.md:117) is an org-config assumption that nothing checks. A connector repo that requires an approval would have it satisfied by github-actions[bot], including stale approvals, since stale-approval dismissal is off.

Resolved prior findings

README Verdict Modes table ended early, leaving the none row outside it: fixed. The none row is at README.md:109, directly under the judge row, and the next paragraph starts after a blank line at README.md:111.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/workflows/pr-review.yaml`:
- Around line 65-69: This commit makes `judge` the default for every connector repo, but the PR description still says the default stays `baseline` and that choosing the mode is out of scope. Update the PR description (the "What this does" and "The decision this PR does not make" sections) to state that connector repos now run judge mode, and explain why the editor-binding fix is no longer a prerequisite.
- Around line 16-20 and 69: Making `judge` the default changes behavior for every existing caller of `pr-review.yaml@main` that doesn't set `verdict_mode`, and the safety argument assumes those repos require 0 approving reviews. Either announce the change to callers, or add a guard in the action: read the base branch's `required_approving_review_count` (branch protection or rulesets) and fall back to `baseline`, or fail, when it is above zero, so the bot's APPROVE can never satisfy a human review requirement.

github-actions · 2026-10-11T19:08:24Z

Superseded — see the current review report for commit 350579b1b74e

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since 4b6f6fc
View review run

Review Summary

The new commit d3edc36 makes judge the default verdict_mode on the connector profile pr-review.yaml. It covers both the workflow_call input default and the inputs.verdict_mode || 'judge' fallback used by the org-rule pull_request trigger. It also rewrites the README cost section to argue that judge mode is acceptable on connector repos because they require 0 approvals and only same-repo PRs are reviewed. general-pr-review.yaml and direct action calls stay on baseline. I scanned the full PR diff for security and correctness: the verdict_mode validation in action.yml and the Python publisher, judge/DEFER/none mapping, REVIEW_STATE_FOR_EVENT gaining APPROVE, none-mode completion and collapse, and the new tests. No blocking defects found. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New — .github/workflows/pr-review.yaml:69 (confidence: high): the PR description doesn't match the diff. It still says "defaulting to baseline, so nothing changes until a caller opts in", has a section called "The decision this PR does not make", and says the editor-binding fix should land before judge is enabled. This commit now turns judge on for every connector repo.
New — .github/workflows/pr-review.yaml:19 (confidence: medium): every pr-review.yaml@main caller that doesn't set verdict_mode silently starts submitting APPROVE. The README's safety argument ("connector repos require 0 approving reviews", README.md:117) is an org-config assumption that nothing checks. A connector repo that requires an approval would have it satisfied by github-actions[bot], including stale approvals, since stale-approval dismissal is off.

Resolved prior findings

README Verdict Modes table ended early, leaving the none row outside it: fixed. The none row is at README.md:109, directly under the judge row, and the next paragraph starts after a blank line at README.md:111.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/workflows/pr-review.yaml`:
- Around line 65-69: This commit makes `judge` the default for every connector repo, but the PR description still says the default stays `baseline` and that choosing the mode is out of scope. Update the PR description (the "What this does" and "The decision this PR does not make" sections) to state that connector repos now run judge mode, and explain why the editor-binding fix is no longer a prerequisite.
- Around line 16-20 and 69: Making `judge` the default changes behavior for every existing caller of `pr-review.yaml@main` that doesn't set `verdict_mode`, and the safety argument assumes those repos require 0 approving reviews. Either announce the change to callers, or add a guard in the action: read the base branch's `required_approving_review_count` (branch protection or rulesets) and fall back to `baseline`, or fail, when it is above zero, so the bot's APPROVE can never satisfy a human review requirement.

Reviewed commit: d3edc36fa153

github-actions

No blocking issues found — see the full review report

The case for defaulting the connector profile to judge was that connector repos require 0 approving reviews, so a bot APPROVE there clears the bot's own block and nothing else. That was true of all 60 repos sampled, and nothing kept it true: a repo that quietly starts requiring an approval would hand the bot a vote nobody decided to give it, and the only control was a sentence in the README asking people to notice and reconfigure. Check it instead. Asking for judge is now a request: the action reads the base branch's required_approving_review_count at the start of the run and grants judge only on a confirmed zero. A branch that requires approvals runs baseline, and so does a requirement that could not be read at all — an unreadable answer is not evidence of a zero. Either way it says so in the job log and the review still happens; only the ability to approve is withheld. The endpoint matters: /repos/{repo}/branches/{branch}/protection answers 403 "Resource not accessible by integration" for GITHUB_TOKEN, while /repos/{repo}/rules/branches/{branch} answers it. Both measured. A branch with no pull_request rule requires no approvals and correctly reads as zero. This lands in the shell step rather than the Python because it has to run before the model does — withholding the mode after a review has already been paid for would be worth nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T13:53:42Z

Superseded — see the current review report for commit 350579b1b74e

General PR Review: Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 2 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since d3edc36
View review run

Review Summary

The new commit adds a check when the run starts, in action.yml's review-config step. When verdict_mode is judge, it reads the base branch's rules from GET /repos/{repo}/rules/branches/{base} and switches to baseline unless that returns a confirmed 0 for required_approving_review_count. It also rewrites the README to describe judge as conditional. I scanned the full PR diff for security and correctness, including the verdict parsing, the DEFER→count fallback, and the judge prompt mixin, and found no other issues. The downgraded mode does reach publish-review-report.py, through steps.review-config.outputs.verdict_mode. No repo-local criteria loaded, so only the base criteria were applied. The check is a good addition, but it can read 0 on a branch that does require approvals, and that breaks the README's promise that "anything other than a confirmed zero runs baseline".

Security Issues

None found.

Correctness Issues

New — .github/actions/pr-review/action.yml:130 (confidence: high): the check only reads rulesets. /rules/branches/{branch} doesn't return classic branch-protection settings. On a repo that requires approvals through classic protection, it returns [], which // 0 turns into 0. Judge mode stays on, and a bot APPROVE can then satisfy the human review requirement the check is meant to protect. (On this repo, main happens to use an org ruleset requiring 1 approval, so it would fall back correctly.)
New — .github/actions/pr-review/action.yml:132 (confidence: high): [.[] | select(.type == "pull_request")] | .[0] reads only the first pull_request rule. This endpoint returns one entry per active ruleset, org-level and repo-level. A 0-approval rule listed before a 1-approval rule keeps judge mode on. It should take the max.

Suggestions

None.

Resolved prior findings

The README Verdict Modes table ended early, leaving the none row outside it. Fixed: the none row sits directly under the judge row in the table, and the explanatory text now starts after the table.
From earlier summaries: the description/diff mismatch is fixed (the PR body now says the connector profile defaults to judge and explains why). The request for a check on required approvals has been addressed by this commit's check, apart from the two gaps listed above.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `.github/actions/pr-review/action.yml`:
- Around line 127-146: `GET repos/{repo}/rules/branches/{branch}` returns only ruleset rules, never classic branch-protection settings, so a branch whose approval requirement comes from classic protection reads as 0 and keeps judge mode. Also call `gh api repos/${GITHUB_REPOSITORY}/branches/${BASE_REF} --jq .protected`. If it is true, try to read the classic required approval count; if that can't be read (it 403s for GITHUB_TOKEN), treat the requirement as unknown and set VERDICT_MODE=baseline. Then update the README so it states exactly which sources are checked.
- Around line 131-133: Replace `.[0].parameters.required_approving_review_count` with the strictest value across all pull_request rules, e.g. `[.[] | select(.type == "pull_request") | (.parameters.required_approving_review_count // 0)] | max // 0`, so a 0-approval ruleset listed before a stricter one can't keep judge mode.

linear-code · 2026-10-12T14:26:53Z

CXH-2780

The safety argument was stated as "connector repos require 0 approving reviews, so a bot approval satisfies nothing". The conclusion holds; the reason was wrong, and wrong in a way that reads as "connector PRs have no human gate at all" — which is false and alarming in its own right. They do have one. required_approving_review_count is 0, but the same ruleset carries a required_reviewers entry naming a team with minimum_approvals: 1 over every file but docs/connector.mdx. Checked on okta, datadog, jira, aws and slack; all five identical. A bot is not a member of that team, so its approval cannot satisfy that gate. Judge mode lets the reviewer clear its own block and nothing else — a person from the team still has to approve. That makes the argument stronger than the one it replaces, and it explains why the run-time check reads required_approving_review_count and not required_reviewers: a named-team requirement is one the bot cannot satisfy anyway, so there is nothing to withhold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T14:32:13Z

Superseded — see the current review report for commit cebd0263bd3e

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since 923b501
View review run

Review Summary

The PR adds a verdict_mode input (baseline default, judge, none) to the pr-review action. Judge mode is downgraded to baseline at run time unless rulesets confirm the branch requires 0 approvals. publish-review-report.py maps an explicit **Verdict:** row to APPROVE / REQUEST_CHANGES / COMMENT, where DEFER falls back to the count and an APPROVE over blockers fails the run. The connector workflow defaults to judge.

The new commit (cebd026) changes only the README. It rewrites the connector-profile rationale: connector repos set required_approving_review_count to 0 and gate merges through a ruleset required_reviewers team entry, which a bot approval cannot satisfy. It also explains why the run-time check reads only the count.

I scanned the full PR diff (action.yml, verdict-judge.md, publish script, tests, pr-review.yaml, Makefile, README) for security and correctness. No new issues found. The new README text is consistent with the code. Its claim about connector rulesets describes org config that I could not verify from this repo. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off) — .github/actions/pr-review/action.yml:141-163: the classic .protected fallback (line 159) runs only when no ruleset pull_request rule exists. Classic protection that requires approvals, sitting next to a ruleset rule that requires 0, therefore still keeps judge. This is documented as a deliberate trade-off in README "The limit of the check", so it does not block. The old thread was outdated, so I re-posted it inline.

Resolved prior findings

README Verdict Modes table split by a paragraph: fixed. The mode table in README.md (the baseline/judge/none rows under "### Verdict Modes") is contiguous again.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 141-163: The classic-protection fallback (`gh api repos/.../branches/${BASE_REF} --jq .protected`) only runs when no ruleset pull_request rule exists. If a branch has classic protection requiring approvals alongside a ruleset rule requiring 0, judge mode stays on and a bot APPROVE can count toward the classic requirement. This is a documented trade-off (README "The limit of the check"). If you ever want to close it, also downgrade to baseline when a ruleset reports 0 but the branch is protected and the classic requirement cannot be read. Note that this would downgrade every current connector repo, as the comment explains.

github-actions · 2026-10-12T14:33:27Z

Superseded — see the current review report for commit d8194b449e1a

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since 923b501
View review run

Review Summary

The PR adds a verdict_mode input (baseline default, judge, none) to the pr-review action. Judge mode is downgraded to baseline at run time unless rulesets confirm the branch requires 0 approvals. publish-review-report.py maps an explicit **Verdict:** row to APPROVE / REQUEST_CHANGES / COMMENT, where DEFER falls back to the count and an APPROVE over blockers fails the run. The connector workflow defaults to judge.

The new commit (cebd026) changes only the README. It rewrites the connector-profile rationale: connector repos set required_approving_review_count to 0 and gate merges through a ruleset required_reviewers team entry, which a bot approval cannot satisfy. It also explains why the run-time check reads only the count.

I scanned the full PR diff (action.yml, verdict-judge.md, publish script, tests, pr-review.yaml, Makefile, README) for security and correctness. No new issues found. The new README text is consistent with the code. Its claim about connector rulesets describes org config that I could not verify from this repo. No repo-local criteria loaded, so only the base criteria were applied. The incremental diff was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off) — .github/actions/pr-review/action.yml:141-163: the classic .protected fallback (line 159) runs only when no ruleset pull_request rule exists. Classic protection that requires approvals, sitting next to a ruleset rule that requires 0, therefore still keeps judge. This is documented as a deliberate trade-off in README "The limit of the check", so it does not block. The old thread was outdated, so I re-posted it inline.

Resolved prior findings

README Verdict Modes table split by a paragraph: fixed. The mode table in README.md (the baseline/judge/none rows under "### Verdict Modes") is contiguous again.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 141-163: The classic-protection fallback (`gh api repos/.../branches/${BASE_REF} --jq .protected`) only runs when no ruleset pull_request rule exists. If a branch has classic protection requiring approvals alongside a ruleset rule requiring 0, judge mode stays on and a bot APPROVE can count toward the classic requirement. This is a documented trade-off (README "The limit of the check"). If you ever want to close it, also downgrade to baseline when a ruleset reports 0 but the branch is protected and the classic requirement cannot be read. Note that this would downgrade every current connector repo, as the comment explains.

Reviewed commit: cebd0263bd3e

github-actions

No blocking issues found — see the full review report

Five documentation defects from review, all cases of prose describing an earlier version of the code. "none submits nothing and stops after step 3" — in the module docstring and again in the action's step description. It does not stop: _run() still calls _collapse_superseded, which is step 6, and test_none_mode_still_collapses_the_consumed_working_comment pins that. It skips steps 4 and 5. Step 6's own text already said this correctly ("in the modes that submit one"), so the file contradicted itself. parse_judge_verdict's "when either row is absent, malformed, out of position, or ambiguous" — "out of position" stopped applying to the verdict row when that row was relaxed to anywhere at top level below the heading. The paragraph directly above it in the same docstring explains the relaxation; the sentence below still described the old rule. Both verdict_mode input descriptions presented judge as a setting that simply takes effect, while the README says it is conditional. They now say it is granted only on a confirmed zero and downgraded otherwise. And the empty-string asymmetry between the two layers is now written down where it lives: `|| 'judge'` makes an explicit "" mean judge here, while the action normalizes "" to baseline. That is right for this workflow — "" and "omitted" both mean "whatever the connector profile runs" — but a workflow_call caller forwarding its own empty optional input gets judge rather than the conservative default, and should pass baseline explicitly to opt out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T14:38:56Z

General PR Review:

⏳ Provisional — deeper review still in progress.

Review in progress; this comment will be replaced with the full summary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T14:40:06Z

Superseded — see the current review report for commit d8194b449e1a

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since cebd026
View review run

Review Summary

The PR adds a verdict_mode input (judge / baseline / none) to the pr-review action. Judge mode is downgraded to baseline unless the base branch's rulesets confirm 0 required approvals. publish-review-report.py maps the summary to APPROVE / REQUEST_CHANGES / COMMENT, or to no review at all, and pr-review.yaml now defaults the connector profile to judge.

The new commits since cebd026 change only documentation and comments; no executable lines changed:

The verdict_mode input description now mentions the ruleset gate.
The step-4 comment in action.yml and the module docstring now say that none skips steps 4 and 5 but still runs step 6.
The parse_judge_verdict docstring now lists the rejection cases precisely.
A new comment in pr-review.yaml explains that an explicit "" falls back to judge there, while the action itself treats "" as baseline.

I checked each updated comment against the code and they match:

none really does call _collapse_superseded and return before submitting (publish-review-report.py:1191-1194).
The rejection cases match parse_judge_verdict (lines 333-349).
The action normalizes "" to baseline (action.yml:107-110).

I scanned the full PR diff (action.yml, verdict-judge.md, publish-review-report.py, tests, pr-review.yaml, Makefile, README) for security and correctness issues. The incremental artifact was complete: not partial, no dropped paths. No repo-local criteria loaded, so the base criteria were applied alone.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off, confidence: high) — .github/actions/pr-review/action.yml:143-166: the classic-protection fallback (.protected) only runs when no ruleset pull_request rule exists. If a ruleset requires 0 approvals and classic protection requires 1 or more, judge stays on and a bot APPROVE could count toward the classic requirement. The comment at lines 151-160 and the README document this as deliberate. The old thread was outdated, so I re-posted it inline.

Resolved prior findings

README Verdict Modes table split by a paragraph: fixed. The baseline / judge / none rows under "### Verdict Modes" in README.md form one contiguous table again.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 143-166: The classic-protection fallback (`gh api repos/.../branches/${BASE_REF} --jq .protected`) only runs when no ruleset pull_request rule exists. If a branch has classic protection requiring approvals alongside a ruleset rule requiring 0, judge mode stays on and a bot APPROVE can count toward the classic requirement. This is a documented trade-off (README "The limit of the check"). If you ever want to close it, also downgrade to baseline when a ruleset reports 0 but the branch is protected and the classic requirement cannot be read. Note that this would downgrade every current connector repo, as the comment explains.

github-actions · 2026-10-12T14:41:20Z

Superseded — see the current review report for commit fbed5f9f7b7e

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since cebd026
View review run

Review Summary

The PR adds a verdict_mode input (judge / baseline / none) to the pr-review action. Judge mode is downgraded to baseline unless the base branch's rulesets confirm 0 required approvals. publish-review-report.py maps the summary to APPROVE / REQUEST_CHANGES / COMMENT, or to no review at all, and pr-review.yaml now defaults the connector profile to judge.

The new commits since cebd026 change only documentation and comments; no executable lines changed:

The verdict_mode input description now mentions the ruleset gate.
The step-4 comment in action.yml and the module docstring now say that none skips steps 4 and 5 but still runs step 6.
The parse_judge_verdict docstring now lists the rejection cases precisely.
A new comment in pr-review.yaml explains that an explicit "" falls back to judge there, while the action itself treats "" as baseline.

I checked each updated comment against the code and they match:

none really does call _collapse_superseded and return before submitting (publish-review-report.py:1191-1194).
The rejection cases match parse_judge_verdict (lines 333-349).
The action normalizes "" to baseline (action.yml:107-110).

I scanned the full PR diff (action.yml, verdict-judge.md, publish-review-report.py, tests, pr-review.yaml, Makefile, README) for security and correctness issues. The incremental artifact was complete: not partial, no dropped paths. No repo-local criteria loaded, so the base criteria were applied alone.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off, confidence: high) — .github/actions/pr-review/action.yml:143-166: the classic-protection fallback (.protected) only runs when no ruleset pull_request rule exists. If a ruleset requires 0 approvals and classic protection requires 1 or more, judge stays on and a bot APPROVE could count toward the classic requirement. The comment at lines 151-160 and the README document this as deliberate. The old thread was outdated, so I re-posted it inline.

Resolved prior findings

README Verdict Modes table split by a paragraph: fixed. The baseline / judge / none rows under "### Verdict Modes" in README.md form one contiguous table again.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 143-166: The classic-protection fallback (`gh api repos/.../branches/${BASE_REF} --jq .protected`) only runs when no ruleset pull_request rule exists. If a branch has classic protection requiring approvals alongside a ruleset rule requiring 0, judge mode stays on and a bot APPROVE can count toward the classic requirement. This is a documented trade-off (README "The limit of the check"). If you ever want to close it, also downgrade to baseline when a ruleset reports 0 but the branch is protected and the classic requirement cannot be read. Note that this would downgrade every current connector repo, as the comment explains.

Reviewed commit: d8194b449e1a

github-actions

No blocking issues found — see the full review report

luisina-santos

Approving. All five inline points are fixed in cd0f681, and cebd026 gets the merge gate right in the README: connector repos require a connector-approvers approval through required_reviewers, and the bot can't satisfy that. Thanks!

The PR description still lags the code. Could you update it before merging?

"Scope is what makes it safe" still says "60 of 60 connector repos sampled require 0 approving reviews… a bot APPROVE there satisfies nothing" and "there is no vote to get". The README now says it correctly: the count is 0, but the team's required_reviewers rule is the actual human gate.
"…has to move off it or pin baseline" contradicts the run-time fallback and the README, which says connector repos can't pin via an input.
The commit sections stop at the fifth. The run-time ruleset check (350579b onward) and the later doc commits aren't described.
Tests: "163 → 166" is stale; the diff adds ~30 cases.

gontzess · 2026-10-12T17:51:25Z

 review heading. Custom headings keep summary/state selection separate; they do not
 isolate all inline review feedback or grant additional permissions.

+### Verdict Modes


can we move this to a separate docs file for the pr-review workflow details, so we can keep the readme streamlined please?

Done in fbed5f9 — and you are right that it had gotten out of hand: PR Review was 258 of the README's 562 lines, nearly half of it, and it is the section this branch grew the most.

Followed the convention the other workflows already use — docs/release-workflow.md, docs/verify-workflow.md, docs/connector-docs-verify.md hold the internals while the README keeps a paragraph, a pointer and how to call the thing. PR Review was the only one carrying all its detail inline, so this moves it to docs/pr-review.md rather than inventing a new layout.

I moved the whole section rather than only the part this PR added, since "streamlined" is not really achieved by leaving 100 lines of the old prose behind. The moved text is byte-identical after demoting its headings one level — nothing reworded, nothing dropped — so the diff should read as a move. I verified that programmatically rather than by eye.

The one genuinely new thing is the input table in the README, which replaces prose that explained summary_marker in a paragraph and verdict_mode not at all.

README is now 329 lines; docs/pr-review.md is 261. Shout if you would rather the README kept more than the three inputs, or if the table belongs in the doc too.

Requested in review. PR Review was 258 of the README's 562 lines — nearly half, and the section this branch grew the most. Follows the convention the other workflows already use: docs/release-workflow.md, docs/verify-workflow.md and docs/connector-docs-verify.md hold the internals while the README keeps a paragraph, a pointer, and how to call the thing. PR Review was the one workflow carrying all of its detail inline. The moved text is byte-identical after demoting its headings one level — nothing reworded, nothing dropped — so the diff reads as a move. What is new is the README's input table, which replaces prose that described `summary_marker` in a paragraph and `verdict_mode` not at all. README is now 329 lines, docs/pr-review.md 261. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T18:03:53Z

Superseded — see the current review report for commit fbed5f9f7b7e

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since d8194b4
View review run

Review Summary

The new commit (fbed5f9) touches only documentation. It moves the long PR-review section of README.md into a new docs/pr-review.md and replaces it in the README with a short overview, a link, and an inputs table (review_prompt / summary_marker / verdict_mode). I compared the moved text with what was removed: it matches word for word, apart from heading levels and a short new intro. The README's verdict_mode default of judge matches pr-review.yaml:16-19,77, and both anchor links match headings in the new file.

I also scanned the full PR diff again for security and correctness: the action.yml judge gate, the publish-review-report.py verdict parsing and mapping, the none path, the tests, pr-review.yaml and the Makefile. That code has not changed since the last review, and no new blocking issues turned up. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (confidence: high) — .github/actions/pr-review/action.yml:153: the comment still says "see the README" for the classic-protection limitation. This PR moved that explanation to docs/pr-review.md ("The limit of the check"), so the pointer no longer resolves.
Prior — still present (accepted trade-off, confidence: high) — .github/actions/pr-review/action.yml:143-166: the classic-protection fallback (.protected) only runs when no ruleset pull_request rule exists. If a ruleset requires 0 approvals alongside classic protection that requires approvals, judge stays on. This is documented as deliberate, now in docs/pr-review.md. I re-posted it inline because the earlier thread is outdated.

Resolved prior findings

README Verdict Modes table split by a paragraph: obsolete. The table moved to docs/pr-review.md under "## Verdict Modes", where the baseline / judge / none rows are contiguous. The README now carries only the inputs table.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 153: The comment says "(see the README — the check reads rulesets...)", but the detailed explanation now lives in docs/pr-review.md under "Verdict Modes" / "The limit of the check". Change "see the README" to "see docs/pr-review.md" so the pointer resolves.
- Around line 143-166: The classic-protection fallback only runs when no ruleset pull_request rule exists, so a ruleset requiring 0 alongside classic protection requiring approvals keeps judge mode on. This is a documented, accepted trade-off. Only change it if you decide to downgrade to baseline when a ruleset reports 0 but the branch is protected. Note that this would downgrade every current connector repo.

github-actions · 2026-10-12T18:05:34Z

Superseded — see the current review report for commit 3d0f41811ff7

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since d8194b4
View review run

Review Summary

The new commit (fbed5f9) touches only documentation. It moves the long PR-review section of README.md into a new docs/pr-review.md and replaces it in the README with a short overview, a link, and an inputs table (review_prompt / summary_marker / verdict_mode). I compared the moved text with what was removed: it matches word for word, apart from heading levels and a short new intro. The README's verdict_mode default of judge matches pr-review.yaml:16-19,77, and both anchor links match headings in the new file.

I also scanned the full PR diff again for security and correctness: the action.yml judge gate, the publish-review-report.py verdict parsing and mapping, the none path, the tests, pr-review.yaml and the Makefile. That code has not changed since the last review, and no new blocking issues turned up. No repo-local criteria loaded, so only the base criteria were applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

New (confidence: high) — .github/actions/pr-review/action.yml:153: the comment still says "see the README" for the classic-protection limitation. This PR moved that explanation to docs/pr-review.md ("The limit of the check"), so the pointer no longer resolves.
Prior — still present (accepted trade-off, confidence: high) — .github/actions/pr-review/action.yml:143-166: the classic-protection fallback (.protected) only runs when no ruleset pull_request rule exists. If a ruleset requires 0 approvals alongside classic protection that requires approvals, judge stays on. This is documented as deliberate, now in docs/pr-review.md. I re-posted it inline because the earlier thread is outdated.

Resolved prior findings

README Verdict Modes table split by a paragraph: obsolete. The table moved to docs/pr-review.md under "## Verdict Modes", where the baseline / judge / none rows are contiguous. The README now carries only the inputs table.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 153: The comment says "(see the README — the check reads rulesets...)", but the detailed explanation now lives in docs/pr-review.md under "Verdict Modes" / "The limit of the check". Change "see the README" to "see docs/pr-review.md" so the pointer resolves.
- Around line 143-166: The classic-protection fallback only runs when no ruleset pull_request rule exists, so a ruleset requiring 0 alongside classic protection requiring approvals keeps judge mode on. This is a documented, accepted trade-off. Only change it if you decide to downgrade to baseline when a ruleset reports 0 but the branch is protected. Note that this would downgrade every current connector repo.

Reviewed commit: fbed5f9f7b7e

github-actions

No blocking issues found — see the full review report

The classic-protection comment in action.yml deferred to "the README" for the limit it was describing, and the previous commit moved that explanation to docs/pr-review.md. It now names the section it means, so the pointer resolves. Swept the rest of .github/actions/pr-review and the two review workflows for the same breakage; this was the only one. The moved doc's own reference to the README is still accurate — the README does carry the short version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-10-12T18:09:15Z

Superseded — see the current review report for commit 3d0f41811ff7

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since fbed5f9
View review run

Review Summary

The PR adds a verdict_mode input (judge / baseline / none). Judge mode is granted only after the base branch's rulesets confirm 0 required approvals; otherwise it downgrades to baseline. In judge mode, publish-review-report.py maps an explicit **Verdict:** row to APPROVE / REQUEST_CHANGES / COMMENT, and DEFER falls back to the blocking count. The connector pr-review.yaml now defaults to judge. The new commit since the last review changes one comment only: action.yml:153-156 now points at "The limit of the check" in docs/pr-review.md instead of the README. I scanned the full PR diff for security and correctness across all 8 changed files: action, judge prompt, publisher script, tests, workflow, Makefile, README and docs. The incremental artifact was complete, with no dropped paths and no truncation. No repo-local criteria loaded, so only the base criteria applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off, confidence: high): .github/actions/pr-review/action.yml:151-166. If a ruleset pull_request rule requires 0 approvals, judge mode stays on even when classic branch protection on the same branch requires approvals. The check cannot see classic protection, so a bot APPROVE could satisfy that requirement. This is documented as deliberate under "The limit of the check" in docs/pr-review.md. I re-posted it inline because the earlier thread is outdated.

Resolved prior findings

Fixed: the action.yml comment that pointed at "the README" for the classic-protection limit. It now cites "The limit of the check" in docs/pr-review.md (action.yml:153-155), and that section exists in docs/pr-review.md.
Obsolete: the README Verdict Modes table that a paragraph split in two. The table now lives in docs/pr-review.md under "## Verdict Modes", where its rows are contiguous. The README keeps only the inputs table.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 151-166: When a ruleset `pull_request` rule requires 0 approvals, judge mode is kept even if classic branch protection on the same branch requires approvals. GITHUB_TOKEN cannot read classic protection. This is an accepted, documented trade-off ("The limit of the check" in docs/pr-review.md), so no change is required. The only way to close it is to read classic protection, which needs `administration: read`, or to refuse judge mode whenever `.protected` is true. The second option would disable judge mode on every connector repo.

github-actions · 2026-10-12T18:10:34Z

General PR Review: [CXH-2780] Let CI submit an approval, not only a block (verdict_mode)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since fbed5f9
View review run

Review Summary

The PR adds a verdict_mode input (judge / baseline / none). Judge mode is granted only after the base branch's rulesets confirm 0 required approvals; otherwise it downgrades to baseline. In judge mode, publish-review-report.py maps an explicit **Verdict:** row to APPROVE / REQUEST_CHANGES / COMMENT, and DEFER falls back to the blocking count. The connector pr-review.yaml now defaults to judge. The new commit since the last review changes one comment only: action.yml:153-156 now points at "The limit of the check" in docs/pr-review.md instead of the README. I scanned the full PR diff for security and correctness across all 8 changed files: action, judge prompt, publisher script, tests, workflow, Makefile, README and docs. The incremental artifact was complete, with no dropped paths and no truncation. No repo-local criteria loaded, so only the base criteria applied.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

Prior — still present (accepted trade-off, confidence: high): .github/actions/pr-review/action.yml:151-166. If a ruleset pull_request rule requires 0 approvals, judge mode stays on even when classic branch protection on the same branch requires approvals. The check cannot see classic protection, so a bot APPROVE could satisfy that requirement. This is documented as deliberate under "The limit of the check" in docs/pr-review.md. I re-posted it inline because the earlier thread is outdated.

Resolved prior findings

Fixed: the action.yml comment that pointed at "the README" for the classic-protection limit. It now cites "The limit of the check" in docs/pr-review.md (action.yml:153-155), and that section exists in docs/pr-review.md.
Obsolete: the README Verdict Modes table that a paragraph split in two. The table now lives in docs/pr-review.md under "## Verdict Modes", where its rows are contiguous. The README keeps only the inputs table.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `.github/actions/pr-review/action.yml`:
- Around line 151-166: When a ruleset `pull_request` rule requires 0 approvals, judge mode is kept even if classic branch protection on the same branch requires approvals. GITHUB_TOKEN cannot read classic protection. This is an accepted, documented trade-off ("The limit of the check" in docs/pr-review.md), so no change is required. The only way to close it is to read classic protection, which needs `administration: read`, or to refuse judge mode whenever `.protected` is true. The second option would disable judge mode on every connector repo.

Reviewed commit: 3d0f41811ff7

github-actions

No blocking issues found — see the full review report

gontzess

seeing a few issues with this approach, and in general i think its a wider change than we need in order to fix the "self-dismissal" bug that's causing pain

HIGH / blocking: the guard knowingly allows an approval into an unread classic human-review requirement

The code takes the maximum generic approving-review count from applicable rulesets. If that maximum is zero, it keeps judge. It consults the branch's protected flag only when there is no ruleset pull-request rule. Thus a ruleset requiring zero approvals is incorrectly accepted as evidence that all applicable policy requires zero approvals. Classic branch protection is neither read nor excluded in this branch.

let me draft an alternative approach

FeliLucero1 and others added 2 commits October 11, 2026 15:17

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

Comment thread .github/actions/pr-review/scripts/publish-review-report.py

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

FeliLucero1 self-assigned this Oct 11, 2026

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

Comment thread README.md Outdated

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

FeliLucero1 mentioned this pull request Oct 11, 2026

[probe, do not merge] What may GITHUB_TOKEN do to reviews on a protected base #148

Closed

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

Comment thread .github/workflows/pr-review.yaml

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

Comment thread .github/workflows/pr-review.yaml

github-actions Bot reviewed Oct 11, 2026

View reviewed changes

FeliLucero1 marked this pull request as ready for review October 11, 2026 19:09

FeliLucero1 marked this pull request as draft October 11, 2026 19:20

FeliLucero1 marked this pull request as ready for review October 12, 2026 13:54