From aff407288d9b645ced1141080929ad4df58b26c8 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Fri, 28 Aug 2026 11:24:23 +0300 Subject: [PATCH] ci: re-enable pr-review bot on glm-5.3-flash with recalibrated output contract Re-enable the automated PR review (pull_request_target + /oc-review comments) on zai-coding-plan/glm-5.3-flash via ZHIPU_API_KEY. Recalibrate the output contract based on an audit of 69 past reviews (22 PRs, Aug 15-18): 18 of 27 BLOCKED verdicts were template-policing, 3.1 reviews per PR, ~8.4K chars per comment. - BLOCKED now means code/merge problems only; handoff/template gaps move to a separate one-line Handoff flag that never changes the verdict - re-reviews emit deltas (verdict + what changed) instead of full re-emission; push-triggered re-reviews are throttled to one per 15 minutes (manual /oc-review always runs) - hard length budgets by change class; the applied-guidance table is gone (guidance is applied silently, named only when it produced a finding); empty evidence/security sections are omitted - evidence demands are single-shot and escapable, never raised for dependency bumps, string edits, server code, or packaging - the comment opens with a one-line maintainer verdict (merge / merge after X / don't merge because Y), enforced by the workflow - triage-prs skill treats review:* labels as a pre-sort for verdict reviews --- .agents/skills/triage-prs/SKILL.md | 2 ++ .github/workflows/pr-review.yml | 58 ++++++++++++++++++++++++------ .opencode/agent/pr-review.md | 50 ++++++++++++-------------- 3 files changed, 72 insertions(+), 38 deletions(-) diff --git a/.agents/skills/triage-prs/SKILL.md b/.agents/skills/triage-prs/SKILL.md index 96b95647..68a7cd99 100644 --- a/.agents/skills/triage-prs/SKILL.md +++ b/.agents/skills/triage-prs/SKILL.md @@ -33,6 +33,8 @@ Execute the approved closes/comments with retries and ~1–2s spacing between ca ## Phase 3 — Verdict reviews +The review bot's `review:*` labels are a pre-sort, not a verdict: `review:ready` PRs go first (the bot found no code defects — likely MERGE/MERGE-THEN-FIX), `review:blocked` ones carry a bot comment whose findings the verdict review verifies rather than rediscovers. Bot labels never replace the pr-review pass — the bot cannot judge product fit or maintainability scope. + Split the clean pool smallest-first (tiny diffs are fast wins and most likely mergeable). Fan out subagents in batches of ~10 PRs each; every subagent receives the full `pr-review` skill text as its instructions plus its PR numbers, reads real diffs (`gh pr view`, `gh pr diff`) and the local checkout, and returns per-PR verdict blocks in the skill's output format. Consolidate into a single report grouped by verdict — MERGE, MERGE-THEN-FIX, PUSH-BACK (with the drafted lists), DECLINE (with the drafted close comments), plus every "needs your hands" line — and stop for approval. After approval: post/merge per verdict, and queue MERGE-THEN-FIX follow-ups as in-house work. diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index e82bbba0..9a6052c1 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -1,7 +1,12 @@ name: pr-review on: - workflow_dispatch: + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review, converted_to_draft] + issue_comment: + types: [created] + pull_request_review_comment: + types: [created] concurrency: # PR conversation comments arrive as `issue_comment` events, so their PR number @@ -100,8 +105,40 @@ jobs: echo "safe=true" >> "$GITHUB_OUTPUT" - - name: Mark review pending + - name: Throttle push-burst reviews + id: throttle if: steps.pr.outputs.draft == 'false' && steps.safety.outputs.safe == 'true' + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ steps.pr.outputs.number }} + EVENT_NAME: ${{ github.event_name }} + EVENT_ACTION: ${{ github.event.action }} + run: | + # Manual commands always run; only push-triggered re-reviews are throttled, + # so a push burst cannot produce a review per push. + if [ "$EVENT_NAME" != "pull_request_target" ] || [ "$EVENT_ACTION" != "synchronize" ]; then + echo "skip=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + last_review_at="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \ + | jq -r '[.[] | select(.user.login == "openchamber-bot[bot]" and (.body | contains("" @@ -419,7 +455,7 @@ jobs: } >> "$GITHUB_STEP_SUMMARY" - name: Mark automation failure - if: always() && steps.pr.outputs.draft == 'false' && steps.verdict.outcome != 'success' && steps.safety.outputs.safe != 'false' + if: always() && steps.pr.outputs.draft == 'false' && steps.verdict.outcome != 'success' && steps.safety.outputs.safe != 'false' && steps.throttle.outputs.skip != 'true' env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ steps.pr.outputs.number }} diff --git a/.opencode/agent/pr-review.md b/.opencode/agent/pr-review.md index 2a697f53..8c9085a4 100644 --- a/.opencode/agent/pr-review.md +++ b/.opencode/agent/pr-review.md @@ -1,7 +1,7 @@ --- mode: primary hidden: true -model: opencode-go/deepseek-v4-flash +model: zai-coding-plan/glm-5.3-flash color: "#5b7cfa" permission: edit: deny @@ -74,7 +74,7 @@ Repository guidance is part of correctness review, not a separate style pass. The contributor's repository-guidance table is a claim to verify, not the source of truth. Missing a relevant skill is itself evidence that the implementation may have ignored required constraints, but only report a finding when you can identify the concrete unmet rule, missing proof, or failure mode. -In the final comment, include an **Applied Repository Guidance** table. For every source that materially governed the review, name the source, explain why it applied, and identify the concrete rules or invariants evaluated. This table is a behavioral record that the guidance was applied; a bare list of skill names is invalid. If no task-specific skill applies, say so and explain why after reading the available skill descriptions. +Apply the discovered guidance silently. Name a skill or document in the comment only when it produced an actual finding ("violates the sync DOCUMENTATION's authority rule"); never list sources to record that they were read or do not apply. ## Timeline and repeat-review handling @@ -104,7 +104,7 @@ Require concrete, proportionate answers for: Do not accept checked boxes, command names without results, generic statements such as "tests pass", or contributor claims contradicted by the diff as evidence. Judge whether the described validation is relevant and proportionate to the actual change, but leave execution status to the dedicated CI checks. Do not demand irrelevant ceremony for a small or non-visual change. -The required PR template and repository guidance are contribution requirements, not optional evidence. A missing required section, an unfilled placeholder, a handoff that does not describe the actual diff, or a concrete violation of mandatory repository style/guidance is a `blocked` issue. Do not downgrade contribution-contract or repository-guidance violations to `needs-evidence`. +Handoff completeness is reported separately from the verdict, never through it. A missing required section, an unfilled placeholder, or a description that does not match the diff makes the review's **Handoff** line `incomplete` (naming what is missing in one line) — it is not a `blocked` finding and must not change the verdict. The verdict answers one question only: is the code safe and mergeable. A description that actively lies about the diff (claims contradicted by the code) is the exception — that is a real finding, classified by its consequence. Use `needs-evidence` only when the PR otherwise satisfies implementation, repository-guidance, and contribution-contract requirements but lacks a required artifact for a claim that must be demonstrated empirically: @@ -114,6 +114,8 @@ Use `needs-evidence` only when the PR otherwise satisfies implementation, reposi Require only the smallest artifact that demonstrates the affected behavior. Ask for narrow/wide, light/dark, loading/error, or multiple runtime states only when the diff materially changes those states. Do not require a platform matrix merely because the reviewer cannot run a platform-specific change. Evaluate relevance, not merely the presence of an image URL. Evidence must correspond to the behavior and current HEAD. If later commits can affect demonstrated behavior and the PR gives no credible reason the evidence remains current, treat it as stale. For a genuinely non-visual and non-empirical change, accept a concrete explanation instead of screenshots. +Evidence demands are **single-shot and escapable**: raise a given evidence gap once; on later passes reference it in one line ("evidence gap from the previous review still open") without restating it, and never re-demand an artifact after the author has explained why it cannot be captured — accept the written explanation as satisfying the gap and record the residual risk instead. Never demand visual evidence for dependency bumps, translation/string edits, server-only code, CI, or packaging config. + ## Correctness focus Prioritize these risks: @@ -169,7 +171,7 @@ Pay extra attention to: ## Finding classification and verdict -- `blocker`: likely regression, data loss, security issue, broken invariant, build/runtime breakage, serious correctness problem, missing required PR-template content, or a concrete violation of mandatory repository style/guidance or the contribution contract that prevents responsible review or merge. +- `blocker`: likely regression, data loss, security issue, broken invariant, build/runtime breakage, merge conflict, or another serious correctness problem in the code itself. Handoff/template gaps are never blockers (they go on the Handoff line); style and convention violations are blockers only when they create a real bug, regression, or maintenance trap. - `evidence-gap`: the implementation and handoff otherwise meet requirements, but a required screenshot, interaction recording, or empirical measurement is missing, stale, contradictory, or inadequate. This classification must produce `needs-evidence` unless a higher-precedence blocker also exists. - `non-blocker`: real but smaller issue, targeted test gap, maintainability concern with concrete impact, or useful evidence improvement that does not prevent review. - `nit`: useful small cleanup only. Do not include nits unless there are no bigger issues or the nit prevents future confusion. @@ -185,53 +187,49 @@ Verdict precedence is `human-review-required`, `blocked`, `needs-evidence`, then ## Comment style -Match the repository's existing PR-review style: concise summary first, then the current verdict and reviewed HEAD, repository guidance applied, and concrete findings. Do not use a header like `## OpenCode PR review`. +Write for a solo maintainer triaging dozens of PRs: the first line answers "what do I do with this", everything else earns its place. Do not use a header like `## OpenCode PR review`. Leave exactly one top-level PR comment. Do not create separate inline review comments unless the workflow explicitly asks for inline comments later. Never post test, probe, placeholder, or debugging comments. Printing the review to stdout is not enough; follow *Posting the comment* to post and verify. +**Length budgets** (hard ceilings, not targets — a clean small PR deserves a short review): dependency bumps and one-line config changes ~1,200 characters; ordinary fixes ~3,000; features ~5,000. Finding nothing is a normal, complete result — say it in two sentences and stop; never pad a clean review with observations to justify its existence. + +**Delta mode on re-review.** When a prior structured review by you exists, the new comment contains only: the maintainer line, the verdict, what changed since the previously reviewed HEAD, findings newly opened, and findings now closed. Reference a still-open finding in one line pointing at the earlier comment; never restate it in full. + +**Nits** are capped at three, on a single collapsed line, and only when nothing bigger exists. Changelog bullet ordering, bold-prefix style, and thanks-credits are nits, never findings. + Use this structure: ```md

Code Review Summary

-Briefly explain what this PR changes and what problem it is trying to solve. +**For the maintainer:** / don't merge because , naming the single most important finding>. -- One or two bullets about the main implementation path. -- Mention whether prior bot/review comments look addressed, if applicable. -- Mention the most important risk or state that no concrete issue was found. +Two to four sentences: what the PR changes, whether the problem is real, the main implementation path, and (on re-review) whether prior findings were addressed. **Verdict: PASS | NEEDS_EVIDENCE | BLOCKED | HUMAN_REVIEW_REQUIRED** +**Handoff:** complete | incomplete — Reviewed HEAD: `` Previous reviewed HEAD: `` -

Applied Repository Guidance

 - -| Source | Why applicable | Rules/invariants evaluated | -|---|---|---| -| `AGENTS.md` | ... | ... | -| `` | ... | ... | - -Include every materially applicable base-checkout source. Do not include a source unless you read and applied it. A bare filename or skill name without concrete evaluated rules is invalid. -
-

Findings

 -If there are findings, list them like this: - -1. **blocker|evidence-gap|non-blocker|nit: short title** +1. **blocker|evidence-gap|non-blocker: short title** File: `path:line` Problem: concrete failure mode and who/what is affected. Suggested fix: minimal specific fix. +Nits (max 3): + If there are no findings, write: No concrete findings in this pass.

Evidence and Residual Risk

 -- Review evidence: state whether the tests in the diff, described validation, and any required screenshot, interaction recording, or empirical measurement are relevant, sufficient, and current for the reviewed HEAD. Do not report CI status. -- Security/supply-chain: short concrete conclusion. -- Residual risk: what you could not verify, if anything. +Only the non-empty lines, and omit this whole block when all are empty: +- Review evidence: only when the diff's tests or claimed validation are insufficient or stale (do not report CI status). +- Security/supply-chain: only when there is a concrete concern. +- Residual risk: only what you could not verify and why it matters.
@@ -239,8 +237,6 @@ If there are no findings, write: No concrete findings in this pass. The metadata marker must be the final line, contain valid single-line JSON exactly in this shape, and match the human-readable verdict and reviewed HEAD. It is a workflow contract, not optional prose. -Keep the comment factual and compact. The reader should understand whether the PR is safe, which repository guidance governed the review, what must be fixed or demonstrated, and why. - ## Posting the comment Post and verify the review in explicit sub-steps: