diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 86d074e..64628f3 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -2,8 +2,6 @@ name: Framework PR checks on: pull_request: - branches: - - worktree/framework types: [opened, synchronize, reopened, ready_for_review] permissions: diff --git a/docs/integration/pr-review-check.md b/docs/integration/pr-review-check.md index 4b5b2df..e4c9f70 100644 --- a/docs/integration/pr-review-check.md +++ b/docs/integration/pr-review-check.md @@ -5,7 +5,7 @@ - `Framework PR checks / framework-tests`:在 PR 合并引用上运行完整 pytest。 - `Framework PR checks / ai-pr-review`:读取 PR diff,调用外部审查 endpoint。 -两个 check 任意失败时都会返回失败;只有在仓库保护规则中将它们设为 required 后,失败才会阻止合并。workflow 位于 `worktree/framework` 基础分支,只监听目标为该分支的 PR。 +两个 check 任意失败时都会返回失败;只有在仓库保护规则中将它们设为 required 后,失败才会阻止合并。workflow 监听所有 PR 目标分支,但目标分支必须已经包含该 workflow 文件;长期接收 PR 的分支需要同步同一份 workflow。 ## GitHub 配置 @@ -21,11 +21,16 @@ - `PR_REVIEW_MODEL`:审查模型名,必填。 - `PR_REVIEW_API_MODE`:`responses` 或 `chat_completions`;不配置时默认为 `responses`。 +如果使用 OpenAI GPT-5.6 Sol,填写 `PR_REVIEW_MODEL=gpt-5.6-sol`、 +`PR_REVIEW_API_MODE=responses`。审查请求已经固定使用 high reasoning effort: +Responses 发送 `reasoning.effort=high`,Chat Completions 发送 +`reasoning_effort=high`,不需要额外配置变量。 + API key 不应写入 workflow、代码、PR 描述或普通变量。workflow 只在模型审查步骤注入该 Secret。 ## Endpoint 请求与响应 -Responses 模式发送 `model`、`instructions`、`input` 和 JSON object text format;Chat Completions 模式发送 `model`、`messages` 和 JSON object response format。两种响应都会被转换为同一审查文档: +Responses 模式优先发送 `model`、`instructions`、`input`、`reasoning.effort=high` 和 strict JSON Schema text format;Chat Completions 模式优先发送 `model`、`messages`、`reasoning_effort=high` 和 strict JSON Schema response format。两种响应都会被转换为同一审查文档: ```json { @@ -43,6 +48,10 @@ Responses 模式发送 `model`、`instructions`、`input` 和 JSON object text f } ``` +结构化输出要求 `decision`、`summary` 和 `findings` 始终存在;没有可行动问题时必须返回 `findings: []`。每个 finding 都必须包含 `severity`、`path`、`line`、`message` 和 `suggestion`,其中不适用的可选值使用 `null`,`message` 必须是具体的非空说明。框架仍会在解析后再次校验这些约束,并对非法或不完整响应 fail-closed。 + +如果 endpoint 以 HTTP 400 或 422 明确拒绝 reasoning 或 Structured Outputs 参数,框架会对同一请求自动回退一次历史兼容格式:`json_object`,且不发送 reasoning 参数。如果 endpoint 返回 502、503 或 504 网关错误,框架会先重试一次 strict JSON Schema 但不发送 reasoning 参数;若网关仍拒绝,再回退到不带 reasoning 的 `json_object`,以应对 high reasoning 或 Structured Outputs 超过上游网关能力的情况。所有回退响应都经过同一严格解析器;其他 HTTP 错误不会回退,仍然 fail-closed。 + `decision` 只能是 `pass` 或 `fail`;严重级别只能是 `P0`、`P1`、`P2`、`P3`。`decision=fail` 或任意 P0/P1 finding 会使 check 失败。HTTP 错误、超时、缺少配置、非法 JSON、字段非法也会失败。 ## 安全边界 @@ -53,13 +62,25 @@ PR diff 最大发送 350,000 字节;超过上限时仍发送前缀,并在请 Fork PR 默认无法读取仓库 Secrets。当前策略是 fail-closed:如果没有审查凭据,`ai-pr-review` 会失败,管理员需要批准可信执行或配置适用的仓库策略后才能合并。 +## 首次接入 main 的两阶段 bootstrap + +`main` 首次接入时必须分两步完成。第一步只把 `tools/pr_review.py`、它的测试和本文档放入 `main`,不同时放入 workflow;这是因为审查 job 必须 checkout trusted base revision,而 bootstrap PR 的 base 还没有审查工具。第一步合并后,再提交第二个 PR,把 `.github/workflows/pr-review.yml` 和 workflow 契约测试加入 `main`。第二步成功后,后续 PR 才会正常同时运行两个 check,随后才能在 `main` 上启用 required checks。 + +这个 bootstrap 顺序避免了让 AI job 执行 PR head 中尚未进入 trusted base 的代码,也避免了用一次性的 PR 编号特判 workflow。 + ## 分支保护 -当前仓库已经是 public,GitHub Free 支持 Branch protection rules。请在目标分支 `worktree/framework` 上将以下 checks 设为 required: +当前仓库已经是 public,GitHub Free 支持 Branch protection rules。最终集成分支是 `main`,研发阶段集成分支是 `worktree/framework`;两个分支都应将以下 checks 设为 required: ```text Framework PR checks / framework-tests Framework PR checks / ai-pr-review ``` -公开仓库后,GitHub Actions 检查本身会正常运行;required checks 仍需在仓库设置中启用,启用后失败的 PR 才会被 GitHub 阻止合并。 +其他实际接收 PR 的长期分支也应同步 workflow 并设置相同的 required checks;临时 feature 分支不单独设置保护规则。 + +开发分支先通过 PR 合并到 `worktree/framework`,稳定后再通过 promotion PR 合并到 `main`。不自动执行 merge,required checks 只负责阻止不合格 PR 合并。 + +由于 GitHub 不会为 workflow 加入前的历史事件自动补跑,workflow 首次进入 `main` 后,已有目标为 `main` 的 PR 需要通过新的 commit、synchronize、reopen 或其他新的有效事件重新触发检查。 + +公开仓库后,GitHub Actions 检查本身会正常运行;required checks 仍需在各目标集成分支的设置中启用,启用后失败的 PR 才会被 GitHub 阻止合并。 diff --git a/docs/superpowers/plans/2026-07-26-all-branch-pr-review-integration.md b/docs/superpowers/plans/2026-07-26-all-branch-pr-review-integration.md new file mode 100644 index 0000000..2eb202b --- /dev/null +++ b/docs/superpowers/plans/2026-07-26-all-branch-pr-review-integration.md @@ -0,0 +1,332 @@ +# 全分支 PR 自动审查与 main 集成 Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** 让所有长期 PR 目标分支使用统一的自动测试与 AI 审查,并建立从研发分支经 promotion PR 合并到 `main` 的保护流程。 + +**Architecture:** 删除 PR workflow 的目标分支过滤器,使 `framework-tests` 和 `ai-pr-review` 对所有目标分支生效;将相同 workflow 同步到 `main` 及其他长期接收 PR 的分支。`worktree/framework` 作为研发集成线,`main` 作为最终集成线,两者都使用同名 required checks,最终合并仍由人工执行。 + +**Tech Stack:** GitHub Actions, GitHub Branch Protection API, Python 3.11, pytest, `gh` CLI, standard-library workflow contract tests。 + +## Global Constraints + +- 所有 PR 目标分支使用稳定 job 名称 `framework-tests` 和 `ai-pr-review`。 +- AI job 只 checkout trusted base SHA 并读取 PR diff,不执行 PR head 代码。 +- API key 只能来自 GitHub Secret;缺少配置、endpoint 错误、超时或非法响应必须 fail-closed。 +- `main` 是最终集成分支;`worktree/framework` 是研发阶段中间集成分支。 +- 不自动执行 merge;required checks 只负责阻止不合格 PR 合并。 +- 只有包含 workflow 文件的目标分支才能触发该 PR workflow;长期目标分支必须同步同一份 workflow。 +- 不为临时 feature 分支单独创建 branch protection rule。 + +--- + +### Task 1: Make the PR workflow target-branch agnostic + +**Files:** +- Modify: `.github/workflows/pr-review.yml:3-8` +- Create: `tests/test_pr_review_workflow.py` + +**Interfaces:** +- Consumes: existing `Framework PR checks` workflow and its job IDs. +- Produces: a `pull_request` workflow without a `branches` filter, retaining event types `opened`, `synchronize`, `reopened`, and `ready_for_review`; job names remain `framework-tests` and `ai-pr-review`. + +- [ ] **Step 1: Write the failing workflow contract tests** + +Create `tests/test_pr_review_workflow.py`: + +```python +from pathlib import Path + + +WORKFLOW = Path(__file__).parents[1] / ".github" / "workflows" / "pr-review.yml" + + +def test_pr_review_workflow_listens_to_all_pull_request_base_branches(): + text = WORKFLOW.read_text(encoding="utf-8") + + assert "pull_request:" in text + assert " branches:" not in text + assert "types: [opened, synchronize, reopened, ready_for_review]" in text + + +def test_pr_review_workflow_keeps_required_job_names(): + text = WORKFLOW.read_text(encoding="utf-8") + + assert " framework-tests:" in text + assert " ai-pr-review:" in text + assert "name: framework-tests" in text + assert "name: ai-pr-review" in text +``` + +- [ ] **Step 2: Run the new tests and verify the current workflow fails the new contract** + +Run: + +```bash +UV_CACHE_DIR=/tmp/agentbench-pr-review-uv-cache \ +PYTHONPATH=src uv run --isolated --no-project \ + --with pytest --with psutil --with jinja2 \ + python -m pytest -q tests/test_pr_review_workflow.py +``` + +Expected: the first test fails because the current workflow contains `branches: - worktree/framework`. + +- [ ] **Step 3: Remove only the target branch filter** + +Change the trigger in `.github/workflows/pr-review.yml` from: + +```yaml +on: + pull_request: + branches: + - worktree/framework + types: [opened, synchronize, reopened, ready_for_review] +``` + +to: + +```yaml +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] +``` + +Do not change permissions, checkout refs, diff bounds, model configuration, job names, or the fail-closed behavior. + +- [ ] **Step 4: Run the workflow contract tests and syntax checks** + +Run: + +```bash +UV_CACHE_DIR=/tmp/agentbench-pr-review-uv-cache \ +PYTHONPATH=src uv run --isolated --no-project \ + --with pytest --with psutil --with jinja2 \ + python -m pytest -q tests/test_pr_review_workflow.py +git diff --check +``` + +Expected: both workflow contract tests pass and `git diff --check` reports no output. + +- [ ] **Step 5: Commit the workflow change** + +```bash +git add .github/workflows/pr-review.yml tests/test_pr_review_workflow.py +git commit -m "ci: review pull requests for every target branch" +``` + +### Task 2: Synchronize integration documentation + +**Files:** +- Modify: `docs/integration/pr-review-check.md` +- Reference: `docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md` + +**Interfaces:** +- Consumes: the branch-agnostic workflow from Task 1 and the approved design. +- Produces: user-facing instructions that distinguish PR listening from required protection and describe the `worktree/framework` → `main` promotion flow. + +- [ ] **Step 1: Update the workflow scope paragraph** + +Replace the statement that the workflow only listens to `worktree/framework` with explicit wording that it listens to all PR target branches where the workflow file exists. + +- [ ] **Step 2: Document branch roles and protection** + +Add these exact rules to the branch-protection section: + +```text +main: final integration branch; framework-tests and ai-pr-review are required. +worktree/framework: research integration branch; the same checks remain required. +Other long-lived PR target branches: synchronize the workflow and protect them if they accept PRs. +Temporary feature branches: no separate protection rule by default. +``` + +- [ ] **Step 3: Document bootstrap and existing-PR behavior** + +Document that the workflow must first be merged into `main`; GitHub will not retroactively create checks for PR events that occurred before the workflow existed. Explain that PR #2 needs a new eligible event after bootstrap and PR #3 needs a synchronize, reopen, or new commit event. + +- [ ] **Step 4: Verify documentation consistency** + +Run: + +```bash +rg -n "只监听|worktree/framework|main|required|promotion|bootstrap" \ + docs/integration/pr-review-check.md \ + docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md +git diff --check +``` + +Expected: no statement says that only `worktree/framework` is listened to; the documented required check names remain exactly `framework-tests` and `ai-pr-review`. + +- [ ] **Step 5: Commit the documentation update** + +```bash +git add docs/integration/pr-review-check.md +git commit -m "docs: explain all-branch PR review and promotion flow" +``` + +### Task 3: Apply branch protection and prepare the main bootstrap + +**Files:** +- External state: GitHub branch protection for `main`, `worktree/framework`, and any confirmed long-lived PR target branch. +- External state: a bootstrap promotion PR carrying the workflow into `main`. +- Verify only: `.github/workflows/pr-review.yml` exists in the base revision of each protected target branch. + +**Interfaces:** +- Consumes: job names from Task 1 and documentation from Task 2. +- Produces: branch protection required contexts `framework-tests` and `ai-pr-review`, strict checks, no force-push/delete, and a reviewable path for installing the workflow in `main`. + +- [ ] **Step 1: Verify the live branch tips before changing protection** + +```bash +git fetch origin main worktree/framework +gh api repos/SAST-agent/AgentBenchFramework/branches/main/protection +gh api repos/SAST-agent/AgentBenchFramework/branches/worktree%2Fframework/protection +``` + +Record whether each branch already has the required contexts. Do not delete or reset any branch. + +- [ ] **Step 2: Create or update protection for `main`** + +Use the GitHub Branch Protection API with this exact policy shape: + +```json +{ + "required_status_checks": { + "strict": true, + "contexts": ["framework-tests", "ai-pr-review"] + }, + "enforce_admins": true, + "required_pull_request_reviews": null, + "restrictions": null, + "allow_force_pushes": false, + "allow_deletions": false +} +``` + +Before applying it, confirm the repository and branch are exactly `SAST-agent/AgentBenchFramework` and `main`. + +- [ ] **Step 3: Verify and repair protection for `worktree/framework`** + +Confirm the same required contexts and strict setting. Preserve existing protection fields unless they conflict with the approved design; do not weaken the existing rule. + +- [ ] **Step 4: Identify other long-lived target branches** + +Run: + +```bash +gh api repos/SAST-agent/AgentBenchFramework/branches --paginate \ + --jq '.[].name' +``` + +Only branches that are intentionally used as PR base branches receive the workflow and matching protection. Do not protect remote feature branches merely because they exist. + +- [ ] **Step 5: Synchronize the workflow into each confirmed long-lived target branch** + +For every confirmed target branch other than `main` and `worktree/framework`, create a normal synchronization PR that carries the canonical `.github/workflows/pr-review.yml` and the matching integration documentation into that branch. Do not push directly to a protected branch. Verify the synchronization PR before merging it. + +- [ ] **Step 6: Prepare the bootstrap promotion PR to `main`** + +After Task 1 and Task 2 are present on the research integration line, create a promotion PR whose base is `main` and whose diff includes the workflow and documentation. Since `main` did not previously contain the workflow, manually inspect the workflow, permissions, checkout refs, and Secret usage before allowing the bootstrap PR to merge. + +Do not merge the bootstrap PR automatically. Its purpose is to install the workflow on `main`; subsequent PRs to `main` must receive the normal required checks. + +- [ ] **Step 7: Commit migration metadata if needed** + +If branch names or protection instructions change during live verification, update `docs/integration/pr-review-check.md`, run `git diff --check`, and commit only that documentation correction. + +### Task 4: Validate existing and future PR behavior + +**Files:** +- Verify: `.github/workflows/pr-review.yml` +- Verify: `tests/test_pr_review_workflow.py` +- Verify: GitHub Actions runs and branch protection APIs + +**Interfaces:** +- Consumes: the branch-agnostic workflow and configured protection from Tasks 1–3. +- Produces: evidence that target `main` and target `worktree/framework` both run the same required checks and that failures block merges. + +- [ ] **Step 1: Run the full local regression suite** + +```bash +git diff --check +python -m compileall -q tools tests +UV_CACHE_DIR=/tmp/agentbench-pr-review-uv-cache \ +PYTHONPATH=src uv run --isolated --no-project \ + --with pytest --with psutil --with jinja2 \ + python -m pytest -q +``` + +Expected: all existing tests and the new workflow contract tests pass. + +- [ ] **Step 2: Push the implementation branch and inspect its checks** + +```bash +GIT_SSH_COMMAND='ssh -o ConnectTimeout=20 -o ServerAliveInterval=5 -o ServerAliveCountMax=3' \ + git push origin codex/pr-review-high-reasoning +gh pr checks 4 --repo SAST-agent/AgentBenchFramework +``` + +Expected: both `framework-tests` and `ai-pr-review` use the exact required job names. + +- [ ] **Step 3: Verify the main bootstrap PR** + +After the workflow is present in `main`, create a minimal test PR targeting `main` or use the first real promotion PR. Verify both jobs run and inspect that the AI job checks the base SHA and reads the diff without executing PR head code. + +- [ ] **Step 4: Verify the research integration target** + +Create or update a test PR targeting `worktree/framework` and verify the same two jobs run. Confirm that a failed `framework-tests` or `ai-pr-review` check is reported as a required-check failure by GitHub. + +- [ ] **Step 5: Re-trigger existing PRs without synthetic content commits** + +For PR #2 and PR #3, use a legitimate new commit, synchronize event, or reopen event when available. Do not add an empty commit solely to manufacture a check. Record the resulting check URLs and conclusions. + +- [ ] **Step 6: Perform final repository verification** + +```bash +gh api repos/SAST-agent/AgentBenchFramework/branches/main/protection +gh api repos/SAST-agent/AgentBenchFramework/branches/worktree%2Fframework/protection +PROMOTION_PR_NUMBER="$(gh pr list --repo SAST-agent/AgentBenchFramework \ + --base main --state open --json number --jq '.[0].number')" +gh pr checks "$PROMOTION_PR_NUMBER" --repo SAST-agent/AgentBenchFramework +git status --short --branch +``` + +Expected: both protected branches require `framework-tests` and `ai-pr-review`; the final implementation branch is clean; no Secret value appears in logs or committed files. + +### Task 5: Final documentation and handoff + +**Files:** +- Verify: `docs/integration/pr-review-check.md` +- Verify: `docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md` +- Verify: `docs/superpowers/plans/2026-07-26-all-branch-pr-review-integration.md` + +**Interfaces:** +- Consumes: completed implementation and live GitHub verification. +- Produces: a concise handoff with branch coverage, check URLs, protection state, and any bootstrap or existing-PR action still pending. + +- [ ] **Step 1: Check that documentation matches live configuration** + +```bash +rg -n "only|只监听|main|worktree/framework|framework-tests|ai-pr-review" \ + docs/integration/pr-review-check.md \ + docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md +``` + +Remove or correct any sentence that claims a branch is protected or covered when the live GitHub configuration disagrees. + +- [ ] **Step 2: Run the final diff and status checks** + +```bash +git diff --check +git status --short --branch +``` + +Expected: no whitespace errors and no uncommitted implementation changes. + +- [ ] **Step 3: Commit documentation corrections if required** + +```bash +git add docs/integration/pr-review-check.md \ + docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md \ + docs/superpowers/plans/2026-07-26-all-branch-pr-review-integration.md +git commit -m "docs: finalize all-branch PR review handoff" +``` diff --git a/docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md b/docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md new file mode 100644 index 0000000..4284140 --- /dev/null +++ b/docs/superpowers/specs/2026-07-26-all-branch-pr-review-design.md @@ -0,0 +1,123 @@ +# 全分支 PR 自动审查与 main 集成设计 + +## 状态 + +本设计已经确认,作为后续实现 workflow、分支保护和迁移工作的边界。 + +## 目标 + +1. 所有可能作为 PR 目标的远端长期分支都自动运行统一的确定性测试和 AI 审查。 +2. `main` 成为最终集成分支,只有通过 required checks 的 PR 才允许合并。 +3. `worktree/framework` 保留为研发阶段的中间集成分支。 +4. AI 审查继续保持可信 base checkout、只读 diff、Secrets 隔离和 fail-closed 约束。 + +## 非目标 + +- 不自动替用户执行 merge。 +- 不执行 PR head 中的代码来完成 AI 审查。 +- 不为每个临时 feature 分支配置独立的 branch protection rule。 +- 不因为 endpoint 暂时不可用而伪造审查通过。 + +## 当前上下文 + +现有 `.github/workflows/pr-review.yml` 只监听目标为 `worktree/framework` 的 PR,因此目标为 `main` 的 PR 不会触发该 workflow。PR #2 目标为 `main`,PR #3 目标为 `worktree/framework` 但在 workflow 加入前已经停止更新;PR #4 是当前自动审查链路的验证对象。 + +GitHub 的 PR workflow 必须存在于目标分支的基础版本中。因此删除 `branches` 过滤器是必要条件,但还必须把统一 workflow 纳入每一个长期接收 PR 的目标分支。 + +## 设计 + +### 1. 触发范围 + +将 workflow 的 `pull_request.branches` 过滤器删除,保留以下事件: + +- `opened` +- `synchronize` +- `reopened` +- `ready_for_review` + +这样 workflow 对所有目标分支生效。实际能否触发仍取决于目标分支已经包含该 workflow 文件;因此至少要维护以下长期分支中的同一份 workflow: + +- `main` +- `worktree/framework` +- 任何实际接收 PR 的长期分支,例如 `worktree/ci-report` + +“所有远端分支都监听”在这里指所有作为 PR base 的分支,而不是要求每个临时 head 分支都配置保护规则。 + +### 2. 检查 job 与安全边界 + +所有目标分支使用同一组稳定的 job 名称: + +- `framework-tests` +- `ai-pr-review` + +`framework-tests` checkout PR merge ref 并运行完整测试。`ai-pr-review` checkout PR base SHA,通过 GitHub API 读取 PR metadata 和 diff,不 checkout 或执行 PR head 代码。 + +AI job 继续只拥有 `contents: read` 和 `pull-requests: read` 权限。API key 只从 GitHub Secret 注入;缺少 Secret、HTTP 错误、超时、非法 JSON 或不合规 finding 都使 job 失败。 + +### 3. 分支保护 + +#### `main` + +- 设置 `framework-tests` 和 `ai-pr-review` 为 required checks。 +- 启用 strict required checks。 +- 禁止 force push 和分支删除。 +- 启用管理员也受保护规则约束。 + +#### `worktree/framework` + +保留现有的同名 required checks、strict 检查、管理员约束、禁止 force push 和禁止删除。 + +#### 其他长期分支 + +如果某个长期分支实际接收 PR,则将同一 workflow 纳入该分支,并设置同名 required checks。临时 feature 分支默认不单独设置保护规则。 + +### 4. 集成与 merge 流程 + +开发流程分为两层: + +```text +feature branch + │ PR + tests + AI review + ▼ +worktree/framework + │ 稳定后建立 promotion PR + tests + AI review + ▼ +main +``` + +合并到 `worktree/framework` 是研发阶段集成;合并到 `main` 是最终集成。promotion PR 会相对于 `main` 重新计算 diff 并重新执行审查,不复用之前的通过结果。 + +所有 merge 仍由人工决定,required checks 只负责阻止不合格 PR 合并。 + +### 5. 迁移顺序 + +1. 在 framework 研发分支中删除 workflow 的目标分支过滤器。 +2. 将同一 workflow 纳入 `main`。 +3. 通过 bootstrap promotion PR 将 workflow 合并到 `main`。在 workflow 尚未进入 `main` 前,该 PR 本身可能没有自动 check,需要人工审阅其 workflow 和变更范围。 +4. workflow 进入 `main` 后,在 `main` 上设置 required checks 和分支保护。 +5. 将同一 workflow 同步到其他接收 PR 的长期分支,并设置对应保护规则。 +6. 对已有 PR 通过新的 commit、synchronize 或重新打开事件重新触发检查;GitHub 不会为 workflow 加入前的历史事件自动补跑。 + +### 6. 失败与回退 + +严格 JSON Schema 和 high reasoning 是 AI 审查的首选请求。对于 endpoint 能力拒绝或网关超时,工具允许有限、明确的兼容回退,但所有回退结果仍由同一解析器校验。 + +任何最终请求失败、响应非法或审查结论为阻塞都返回失败。不得用默认 pass、空 finding 或忽略网络错误来绕过 required check。 + +## 验证标准 + +实现完成后必须验证: + +1. workflow YAML 不再只包含 `worktree/framework` 分支过滤器。 +2. `framework-tests` 和 `ai-pr-review` 的 job 名称与分支保护 required contexts 完全一致。 +3. 本地完整测试、YAML 解析和 `git diff --check` 通过。 +4. 目标为 `main` 的测试 PR 能触发两个 job。 +5. 目标为 `worktree/framework` 的测试 PR 能触发两个 job。 +6. 任一 required check 失败时,GitHub 不允许合并。 +7. AI job 的日志不包含 API key,也不执行 PR head 代码。 + +## 风险与边界 + +- 某个新建的远端分支如果没有 workflow 文件,不能仅靠其他分支的 workflow 保证它的 PR 触发;它必须先同步 workflow,或者成为一个已维护长期分支。 +- bootstrap promotion PR 在保护规则完全生效前需要人工复核,这是将 CI 治理引入既有 `main` 的一次性迁移成本。 +- endpoint 服务不可用时 required check 会失败,这是 fail-closed 设计的预期行为。 diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 503d054..d6d5927 100644 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -134,13 +134,25 @@ def test_builds_protocol_specific_json_requests(): assert responses["model"] == "review-model" assert responses["input"] == "diff" - assert responses["text"]["format"] == {"type": "json_object"} + responses_format = responses["text"]["format"] + assert responses_format["type"] == "json_schema" + assert responses_format["name"] == "pr_review" + assert responses_format["strict"] is True + message_schema = responses_format["schema"]["properties"]["findings"]["items"]["properties"]["message"] + assert message_schema["type"] == "string" + assert "non-empty" in message_schema["description"] + assert responses["reasoning"] == {"effort": "high"} assert chat["model"] == "review-model" assert chat["messages"] == [ {"role": "system", "content": "system"}, {"role": "user", "content": "diff"}, ] - assert chat["response_format"] == {"type": "json_object"} + chat_format = chat["response_format"] + assert chat_format["type"] == "json_schema" + assert chat_format["json_schema"]["name"] == "pr_review" + assert chat_format["json_schema"]["strict"] is True + assert chat_format["json_schema"]["schema"] == responses_format["schema"] + assert chat["reasoning_effort"] == "high" def test_rejects_unknown_api_mode(): @@ -199,6 +211,72 @@ def log_message(self, *_args): assert "messages" in seen["body"] +def test_cli_falls_back_to_legacy_json_when_structured_output_is_rejected(tmp_path): + seen = [] + + class Handler(BaseHTTPRequestHandler): + def do_POST(self): # noqa: N802 + seen.append(json.loads(self.rfile.read(int(self.headers["Content-Length"])))) + if len(seen) == 1: + self.send_response(400) + self.end_headers() + return + body = {"output_text": json.dumps({ + "decision": "pass", "summary": "ok", "findings": [], + })} + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(json.dumps(body).encode()) + + def log_message(self, *_args): + return + + with serve(Handler) as endpoint: + result = _run_cli(_cli_env(tmp_path, endpoint)) + + assert result.returncode == 0 + assert len(seen) == 2 + assert seen[0]["text"]["format"]["type"] == "json_schema" + assert seen[0]["reasoning"] == {"effort": "high"} + assert seen[1]["text"]["format"] == {"type": "json_object"} + assert "reasoning" not in seen[1] + + +def test_cli_retries_strict_schema_without_reasoning_after_gateway_timeout(tmp_path): + seen = [] + + class Handler(BaseHTTPRequestHandler): + def do_POST(self): # noqa: N802 + seen.append(json.loads(self.rfile.read(int(self.headers["Content-Length"])))) + if len(seen) <= 2: + self.send_response(504) + self.end_headers() + return + body = {"output_text": json.dumps({ + "decision": "pass", "summary": "ok", "findings": [], + })} + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.end_headers() + self.wfile.write(json.dumps(body).encode()) + + def log_message(self, *_args): + return + + with serve(Handler) as endpoint: + result = _run_cli(_cli_env(tmp_path, endpoint)) + + assert result.returncode == 0 + assert len(seen) == 3 + assert seen[0]["text"]["format"]["type"] == "json_schema" + assert seen[0]["reasoning"] == {"effort": "high"} + assert seen[1]["text"]["format"]["type"] == "json_schema" + assert "reasoning" not in seen[1] + assert seen[2]["text"]["format"] == {"type": "json_object"} + assert "reasoning" not in seen[2] + + @pytest.mark.parametrize("env_name", [ "PR_REVIEW_API_KEY", "PR_REVIEW_ENDPOINT", "PR_REVIEW_MODEL", "PR_REVIEW_INPUT_JSON", diff --git a/tests/test_pr_review_workflow.py b/tests/test_pr_review_workflow.py new file mode 100644 index 0000000..85dd097 --- /dev/null +++ b/tests/test_pr_review_workflow.py @@ -0,0 +1,21 @@ +from pathlib import Path + + +WORKFLOW = Path(__file__).parents[1] / ".github" / "workflows" / "pr-review.yml" + + +def test_pr_review_workflow_listens_to_all_pull_request_base_branches(): + text = WORKFLOW.read_text(encoding="utf-8") + + assert "pull_request:" in text + assert " branches:" not in text + assert "types: [opened, synchronize, reopened, ready_for_review]" in text + + +def test_pr_review_workflow_keeps_required_job_names(): + text = WORKFLOW.read_text(encoding="utf-8") + + assert " framework-tests:" in text + assert " ai-pr-review:" in text + assert "name: framework-tests" in text + assert "name: ai-pr-review" in text diff --git a/tools/pr_review.py b/tools/pr_review.py index a2d6da3..e2aa5a3 100644 --- a/tools/pr_review.py +++ b/tools/pr_review.py @@ -19,12 +19,54 @@ SEVERITIES = frozenset({"P0", "P1", "P2", "P3"}) _FENCED_JSON = re.compile(r"^```(?:json)?\s*(.*?)\s*```$", re.DOTALL | re.IGNORECASE) DEFAULT_API_MODE = "responses" +DEFAULT_REASONING_EFFORT = "high" DEFAULT_TIMEOUT_S = 90.0 MAX_INPUT_BYTES = 400_000 +CAPABILITY_FALLBACK_STATUS_CODES = frozenset({400, 422}) +GATEWAY_RETRY_STATUS_CODES = frozenset({502, 503, 504}) +REVIEW_RESPONSE_SCHEMA = { + "type": "object", + "additionalProperties": False, + "properties": { + "decision": { + "type": "string", + "enum": ["pass", "fail"], + }, + "summary": { + "type": "string", + "description": "A concise non-empty review summary.", + }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": False, + "properties": { + "severity": { + "type": "string", + "enum": ["P0", "P1", "P2", "P3"], + }, + "path": {"type": ["string", "null"]}, + "line": {"type": ["integer", "null"]}, + "message": { + "type": "string", + "description": "A concise non-empty actionable finding message.", + }, + "suggestion": {"type": ["string", "null"]}, + }, + "required": ["severity", "path", "line", "message", "suggestion"], + }, + }, + }, + "required": ["decision", "summary", "findings"], +} REVIEW_INSTRUCTIONS = """You are the blocking code reviewer for AgentBenchFramework. Review the supplied pull request diff for correctness, scientific data integrity, reproducibility, process and secret safety, and forward compatibility. Only -report actionable findings. P0/P1 findings block merging. Return ONLY the JSON +report actionable findings. P0/P1 findings block merging. The response must +conform to the supplied structured review schema. If there are no actionable +findings, set findings to []. Never emit a placeholder finding: every finding +message must contain a concrete, non-empty explanation. Return ONLY the JSON object required by the review response schema; do not use Markdown fences. """ @@ -140,24 +182,59 @@ def review_should_fail(review: dict[str, Any]) -> bool: ) -def build_api_request(api_mode: str, model: str, instructions: str, payload: str) -> dict[str, Any]: +def build_api_request( + api_mode: str, + model: str, + instructions: str, + payload: str, + reasoning_effort: str | None = DEFAULT_REASONING_EFFORT, + structured_outputs: bool = True, +) -> dict[str, Any]: """Build a JSON request body for the selected OpenAI-compatible API.""" if api_mode == "responses": - return { + request = { "model": model, "instructions": instructions, "input": payload, - "text": {"format": {"type": "json_object"}}, + "text": { + "format": ( + { + "type": "json_schema", + "name": "pr_review", + "strict": True, + "schema": REVIEW_RESPONSE_SCHEMA, + } + if structured_outputs + else {"type": "json_object"} + ), + }, } + if reasoning_effort is not None: + request["reasoning"] = {"effort": reasoning_effort} + return request if api_mode == "chat_completions": - return { + request = { "model": model, "messages": [ {"role": "system", "content": instructions}, {"role": "user", "content": payload}, ], - "response_format": {"type": "json_object"}, + "response_format": ( + { + "type": "json_schema", + "json_schema": { + "name": "pr_review", + "strict": True, + "schema": REVIEW_RESPONSE_SCHEMA, + }, + } + if structured_outputs + else {"type": "json_object"} + ), } + if reasoning_effort is not None: + request["reasoning_effort"] = reasoning_effort + return request raise ValueError(f"unsupported api mode: {api_mode}") @@ -199,6 +276,25 @@ def _write_summary(review: dict[str, Any] | None, error: str | None, secret: str handle.write(content) +def _post_review_request( + endpoint: str, + api_key: str, + request_body: dict[str, Any], + timeout: float, +) -> dict[str, Any]: + request = urllib.request.Request( + endpoint, + data=json.dumps(request_body, ensure_ascii=False).encode("utf-8"), + headers={ + "Authorization": f"Bearer {api_key}", + "Content-Type": "application/json", + }, + method="POST", + ) + with urllib.request.urlopen(request, timeout=timeout) as response: + return json.loads(response.read().decode("utf-8")) + + def _emit_review(review: dict[str, Any], secret: str) -> None: print(f"PR review decision={review['decision']} findings={len(review['findings'])}") print(_redact(review["summary"], secret)) @@ -232,19 +328,49 @@ def run_review() -> int: payload_bytes = payload.encode("utf-8") if len(payload_bytes) > MAX_INPUT_BYTES: raise ValueError(f"review input exceeds {MAX_INPUT_BYTES} bytes") - request_body = build_api_request(api_mode, model, REVIEW_INSTRUCTIONS, payload) - request = urllib.request.Request( - endpoint, - data=json.dumps(request_body, ensure_ascii=False).encode("utf-8"), - headers={ - "Authorization": f"Bearer {api_key}", - "Content-Type": "application/json", - }, - method="POST", - ) timeout = float(os.environ.get("PR_REVIEW_TIMEOUT_S", DEFAULT_TIMEOUT_S)) - with urllib.request.urlopen(request, timeout=timeout) as response: - response_data = json.loads(response.read().decode("utf-8")) + request_body = build_api_request(api_mode, model, REVIEW_INSTRUCTIONS, payload) + try: + response_data = _post_review_request(endpoint, api_key, request_body, timeout) + except urllib.error.HTTPError as exc: + if exc.code in GATEWAY_RETRY_STATUS_CODES: + retry_request = build_api_request( + api_mode, + model, + REVIEW_INSTRUCTIONS, + payload, + reasoning_effort=None, + ) + try: + response_data = _post_review_request( + endpoint, api_key, retry_request, timeout + ) + except urllib.error.HTTPError as retry_exc: + if retry_exc.code not in GATEWAY_RETRY_STATUS_CODES: + raise + legacy_request = build_api_request( + api_mode, + model, + REVIEW_INSTRUCTIONS, + payload, + reasoning_effort=None, + structured_outputs=False, + ) + response_data = _post_review_request( + endpoint, api_key, legacy_request, timeout + ) + elif exc.code in CAPABILITY_FALLBACK_STATUS_CODES: + legacy_request = build_api_request( + api_mode, + model, + REVIEW_INSTRUCTIONS, + payload, + reasoning_effort=None, + structured_outputs=False, + ) + response_data = _post_review_request(endpoint, api_key, legacy_request, timeout) + else: + raise if isinstance(response_data, dict) and {"decision", "summary", "findings"} <= response_data.keys(): review = parse_review_document(json.dumps(response_data, ensure_ascii=False)) else: