feat: 重构被审查分支的选择逻辑

push 审查
  check_branches 现在只表示「要做 push 审查的分支」,不再兼作 PR 目标分支:
  - 勾选具体分支:这些分支的 push 会审查,通过后晋级到管理分支
  - 新增「任意分支」(*):所有分支的 push 都审查
  - 留空:只审 PR,不审 push

PR 审查
  始终覆盖「勾选的分支 + 管理分支」作为合并目标。
  管理分支是最终目标,不需要出现在检查分支里也能被保护。

管理分支从候选中移除
  分支选择器不再列出管理分支——它是终点,不需要再检查自己。

安全边界
  「任意分支」只扩大审查范围,不会把任意分支自动合并进管理分支;
  晋级仍要求分支被显式勾选(promotesToManaged)。

同时修正一处语义错误
  早先 branchMatches 把空列表当成「审所有分支」,导致新建仓库在用户
  还没选分支时就审查全部 push。现在空列表明确表示「只审 PR」。

测试:分支匹配矩阵重写,覆盖 push/PR/晋级/保护目标四类判定,共 20 项通过。
This commit is contained in:
2026-09-21 17:12:57 +08:00
parent 7cd2c85888
commit 216bb51c92
6 changed files with 209 additions and 67 deletions
+9 -3
View File
@@ -162,8 +162,8 @@ curl -fsS http://localhost:8090/api/health
| 字段 | 说明 | | 字段 | 说明 |
| --- | --- | | --- | --- |
| `repo_url` | 仓库 Git 地址,`owner` / `name` 由此自动解析 | | `repo_url` | 仓库 Git 地址,`owner` / `name` 由此自动解析 |
| `managed_branch` | 主管理分支。push 事件没有对应 PR 时用它做对比基准;自动合并不限于它,任一检查分支都可作为合并目标 | | `managed_branch` | 最终目标分支。PR 合入它始终会被审查;push 到检查分支通过后会晋级到它 |
| `check_branches` | 受保护的目标分支列表,逗号分隔。任何**合并进**这些分支的 PR 都会审查(功能分支不必列出),向这些分支推送也会审查 | | `check_branches` | 需要做 push 审查的分支,逗号分隔;`*` 表示任意分支。留空=只审 PR。管理分支不必列在这里(它是 PR 的默认目标,始终受保护) |
| `review_scope` | `both` / `pr` / `push` | | `review_scope` | `both` / `pr` / `push` |
| `block_severity` | 阻断级别阈值,如 `critical,high`;留空则不看级别 | | `block_severity` | 阻断级别阈值,如 `critical,high`;留空则不看级别 |
| `block_categories` | 额外按类别阻断,如 `security` | | `block_categories` | 额外按类别阻断,如 `security` |
@@ -217,7 +217,13 @@ curl -X POST http://localhost:8090/api/review \
## 行为说明 ## 行为说明
**分支与合并**:`check_branches` 是**被保护的目标分支**。任何合并进这些分支的 PR 都会被审查,所以功能分支不必列进去;向这些分支直接推送也会审查。审查基准是该改动实际要合入的分支(PR 用 PR 自己的目标分支,push 用管理分支),也就是「这次改动合进去会怎样」。 **分支与合并**:`check_branches` 是**要做 push 审查的分支**。
- 勾选具体分支:这些分支的 push 会审查,通过后晋级到管理分支
- 勾选「任意分支」(`*`):所有分支的 push 都审查,但**不会**把任意分支自动合并进管理分支
- 留空:只审 PR,不审 push
**PR 审查**始终覆盖「勾选的分支 + 管理分支」作为合并目标,所以管理分支不需要出现在检查列表里。审查基准是该 PR 实际要合入的分支。
自动合并作用于目标为**任一检查分支**的 PR,因此两级流转可以直接用: 自动合并作用于目标为**任一检查分支**的 PR,因此两级流转可以直接用:
+86 -26
View File
@@ -64,26 +64,86 @@ export function parseRepoUrl(input) {
return { owner, name, host }; return { owner, name, host };
} }
/** Whether `refName` is one of the branches this repository reviews. */ /** Branch names are stored with or without the refs/heads/ prefix. */
function shortRef(refName) {
return String(refName || "").replace(/^refs\/heads\//, "");
}
/** The "any branch" token that widens push review to every branch. */
export const ANY_BRANCH = "*";
/**
* Branches explicitly picked for checking.
* The "any branch" token is not a branch name, so it is filtered out.
*/
export function selectedBranches(checkBranches) {
return parseList(checkBranches).filter((b) => b !== ANY_BRANCH);
}
/**
* True when pushes on every branch are reviewed.
* An empty list means "pull requests only", not "every push" — newly added
* repositories must not start reviewing pushes until branches are picked.
*/
export function reviewsAnyPush(checkBranches) {
return parseList(checkBranches).includes(ANY_BRANCH);
}
/**
* Whether a push to `refName` should be reviewed.
* Selecting "any branch" reviews everything; otherwise only the listed ones.
*/
export function branchMatches(refName, checkBranches) { export function branchMatches(refName, checkBranches) {
const list = parseList(checkBranches); if (reviewsAnyPush(checkBranches)) return true;
if (list.length === 0) return true; const name = shortRef(refName);
const short = String(refName || "").replace(/^refs\/heads\//, ""); return selectedBranches(checkBranches).includes(name);
return list.some((b) => b === short || b === refName);
} }
/** /**
* Whether a pull request should be reviewed. * Whether a pull request should be reviewed.
* *
* The check list names the *target* branches worth protecting, so a PR counts * A pull request counts when it merges INTO a checked branch, or into the
* when it merges INTO a checked branch. That keeps "review everything that * managed branch — the managed branch is the final target, so it is protected
* lands on prd/test" working even though feature branches are never listed. * without needing to be listed as a checked branch. Matching the head branch
* As a fallback the head branch is also matched, so explicitly listing a * is kept as a fallback so explicitly listing a long-lived branch still covers
* long-lived branch still reviews pushes and PRs originating from it. * pull requests originating from it.
*/ */
export function pullRequestMatches(headRef, baseRef, checkBranches) { export function pullRequestMatches(headRef, baseRef, checkBranches, managedBranch) {
if (branchMatches(baseRef, checkBranches)) return true; const target = shortRef(baseRef);
return branchMatches(headRef, checkBranches); const head = shortRef(headRef);
const list = selectedBranches(checkBranches);
if (managedBranch) {
// The managed branch is the final target: always protected for pull
// requests, even though it is deliberately absent from the checked list.
if (target === shortRef(managedBranch)) return true;
return list.includes(target) || list.includes(head);
}
// No managed branch configured: an empty list falls back to reviewing all
// pull requests rather than silently reviewing none.
if (list.length === 0) return true;
return list.includes(target) || list.includes(head);
}
/**
* Whether a push to this branch should be promoted into the managed branch.
*
* Only explicitly listed branches promote. Selecting "any branch" widens
* *review* to every branch but must not auto-merge arbitrary branches into the
* managed branch, so it never promotes on its own.
*/
export function promotesToManaged(refName, checkBranches, managedBranch) {
const name = shortRef(refName);
if (!name || name === shortRef(managedBranch)) return false;
return selectedBranches(checkBranches).includes(name);
}
/** Whether `baseRef` is a branch that pull requests may be merged into. */
export function isProtectedTarget(baseRef, checkBranches, managedBranch) {
const target = shortRef(baseRef);
if (!target) return false;
if (managedBranch && target === shortRef(managedBranch)) return true;
return selectedBranches(checkBranches).includes(target);
} }
function badge(comment) { function badge(comment) {
@@ -714,8 +774,8 @@ export class ReviewEngine {
// A two-stage flow (feature -> test -> prd) legitimately merges into a // A two-stage flow (feature -> test -> prd) legitimately merges into a
// checked branch that is not the managed branch, so the gate is "is the // checked branch that is not the managed branch, so the gate is "is the
// target one of the branches we protect", not "is it the managed branch". // target one of the branches we protect", not "is it the managed branch".
if (job.base_ref && !branchMatches(job.base_ref, repo.check_branches)) { if (job.base_ref && !isProtectedTarget(job.base_ref, repo.check_branches, repo.managed_branch)) {
appendLog(`auto-merge skipped: PR targets ${job.base_ref}, which is not in check_branches (${repo.check_branches})`); appendLog(`auto-merge skipped: PR targets ${job.base_ref}, which is neither the managed branch nor a checked branch`);
return false; return false;
} }
if (reviewIncomplete) { if (reviewIncomplete) {
@@ -798,21 +858,21 @@ export class ReviewEngine {
appendLog(`auto-merge skipped: commit status is ${statusState}`); appendLog(`auto-merge skipped: commit status is ${statusState}`);
return false; return false;
} }
if (!branchMatches(job.ref_name, repo.check_branches)) {
appendLog(`auto-merge skipped: ${job.ref_name} is not a checked branch`);
return false;
}
// Walk up the chain: test -> prd. The managed branch is the top, so a push
// to it has nowhere to go.
const managed = repo.managed_branch; const managed = repo.managed_branch;
if (job.ref_name === managed) { // Only explicitly listed branches promote. "任意分支" widens review to every
appendLog(`auto-merge skipped: ${job.ref_name} is the managed branch`); // branch but must not auto-merge arbitrary branches into the managed one.
if (!promotesToManaged(job.ref_name, repo.check_branches, managed)) {
if (reviewsAnyPush(repo.check_branches)) {
appendLog(`auto-merge skipped: ${job.ref_name} is not an explicitly checked branch`);
} else {
appendLog(`auto-merge skipped: ${job.ref_name} is not a checked branch`);
}
return false; return false;
} }
// The managed branch is the top of the chain and has nowhere to promote to.
const target = managed; const target = managed;
if (!branchMatches(target, repo.check_branches) && target !== managed) { if (!target) {
appendLog(`auto-merge skipped: ${target} is not a checked branch`); appendLog("auto-merge skipped: no managed branch configured");
return false; return false;
} }
+3 -3
View File
@@ -17,7 +17,7 @@ import { OcrRunner } from "./lib/ocr.js";
import { JobQueue } from "./lib/queue.js"; import { JobQueue } from "./lib/queue.js";
import { import {
ReviewEngine, SkipJob, branchMatches, parseRepoUrl, pullRequestMatches, ReviewEngine, SkipJob, branchMatches, parseRepoUrl, pullRequestMatches,
PROMOTION_MARKER, promotesToManaged, ANY_BRANCH, PROMOTION_MARKER,
} from "./lib/review.js"; } from "./lib/review.js";
const APP_DIR = resolve(fileURLToPath(new URL(".", import.meta.url))); const APP_DIR = resolve(fileURLToPath(new URL(".", import.meta.url)));
@@ -296,10 +296,10 @@ async function handlePullRequest(payload, cfg) {
if (String(pr.body || "").includes(PROMOTION_MARKER)) { if (String(pr.body || "").includes(PROMOTION_MARKER)) {
return { queued: 0, reason: "self-created promotion pull request; already reviewed on push" }; return { queued: 0, reason: "self-created promotion pull request; already reviewed on push" };
} }
if (!pullRequestMatches(pr.head?.ref, pr.base?.ref, repo.check_branches)) { if (!pullRequestMatches(pr.head?.ref, pr.base?.ref, repo.check_branches, repo.managed_branch)) {
return { return {
queued: 0, queued: 0,
reason: `neither head ${pr.head?.ref} nor base ${pr.base?.ref} is in the checked branch list`, reason: `PR base ${pr.base?.ref} is neither the managed branch nor a checked branch`,
}; };
} }
const toSha = pr.head?.sha; const toSha = pr.head?.sha;
+49 -14
View File
@@ -197,10 +197,14 @@ function shortUrl(url) {
function branchTags(list, managed) { function branchTags(list, managed) {
const names = String(list || "").split(",").map((s) => s.trim()).filter(Boolean); const names = String(list || "").split(",").map((s) => s.trim()).filter(Boolean);
if (!names.length) return '<span class="tag muted">—</span>'; if (names.includes("*")) {
return names.map((n) => n === managed const extra = names.filter((n) => n !== "*");
? `<span class="tag ok">${esc(n)}</span>` return `<span class="tag warn">任意分支</span>` +
: `<span class="tag muted">${esc(n)}</span>`).join(" "); (extra.length ? ` ${extra.map((n) => `<span class="tag muted">${esc(n)}</span>`).join(" ")}` : "");
}
const own = names.filter((n) => n !== managed);
if (!own.length) return '<span class="tag muted">仅 PR</span>';
return own.map((n) => `<span class="tag muted">${esc(n)}</span>`).join(" ");
} }
function scopeLabel(scope) { function scopeLabel(scope) {
@@ -251,30 +255,59 @@ const modal = document.getElementById("modal");
const form = document.getElementById("repo-form"); const form = document.getElementById("repo-form");
const branchList = document.getElementById("branch-list"); const branchList = document.getElementById("branch-list");
function selectedBranches() { const ANY_BRANCH = "*";
function allSelected() {
return String(form.elements.check_branches.value || "") return String(form.elements.check_branches.value || "")
.split(",").map((s) => s.trim()).filter(Boolean); .split(",").map((s) => s.trim()).filter(Boolean);
} }
function setSelectedBranches(list) { /** Branch names only; the "任意分支" token is not a branch. */
form.elements.check_branches.value = list.join(","); function selectedBranches() {
return allSelected().filter((b) => b !== ANY_BRANCH);
}
function reviewsAnyBranch() {
return allSelected().includes(ANY_BRANCH);
}
function setSelectedBranches(list, { any = reviewsAnyBranch() } = {}) {
const names = list.filter((b) => b && b !== ANY_BRANCH);
form.elements.check_branches.value = (any ? [ANY_BRANCH, ...names] : names).join(",");
} }
function renderBranchList() { function renderBranchList() {
const chosen = new Set(selectedBranches()); const chosen = new Set(selectedBranches());
const managed = form.elements.managed_branch.value.trim(); const managed = form.elements.managed_branch.value.trim();
const any = reviewsAnyBranch();
if (!state.branches.length) { if (!state.branches.length) {
branchList.innerHTML = '<span class="hint">点「加载分支」获取远端分支列表。</span>'; branchList.innerHTML = '<span class="hint">点「加载分支」获取远端分支列表。</span>';
return; return;
} }
branchList.innerHTML = state.branches.map((b) => { // The managed branch is the final target and is always reviewed for pull
// requests, so it is not offered as a checked branch.
const selectable = state.branches.filter((b) => b !== managed);
const parts = [
`<button type="button" class="${any ? "on" : ""}" data-any="1">任意分支</button>`,
];
if (selectable.length === 0) {
parts.push('<span class="hint">除管理分支外没有其它分支。</span>');
} else {
parts.push(...selectable.map((b) => {
const on = chosen.has(b); const on = chosen.has(b);
const label = b === managed ? `${b}(管理)` : b; return `<button type="button" class="${on ? "on" : ""}" data-branch="${esc(b)}">${esc(b)}</button>`;
return `<button type="button" class="${on ? "on" : ""}" data-branch="${esc(b)}">${esc(label)}</button>`; }));
}).join(""); }
branchList.innerHTML = parts.join("");
} }
branchList.addEventListener("click", (ev) => { branchList.addEventListener("click", (ev) => {
const anyBtn = ev.target.closest("button[data-any]");
if (anyBtn) {
setSelectedBranches(selectedBranches(), { any: !reviewsAnyBranch() });
renderBranchList();
return;
}
const btn = ev.target.closest("button[data-branch]"); const btn = ev.target.closest("button[data-branch]");
if (!btn) return; if (!btn) return;
const name = btn.dataset.branch; const name = btn.dataset.branch;
@@ -299,7 +332,7 @@ async function openRepo(id) {
if (!repo) { if (!repo) {
if (field === "enabled" || field === "create_issue") input.checked = true; if (field === "enabled" || field === "create_issue") input.checked = true;
if (field === "managed_branch") input.value = "main"; if (field === "managed_branch") input.value = "main";
if (field === "check_branches") input.value = "main"; if (field === "check_branches") input.value = "";
continue; continue;
} }
if (input.type === "checkbox") input.checked = Boolean(repo[field]); if (input.type === "checkbox") input.checked = Boolean(repo[field]);
@@ -334,7 +367,9 @@ async function loadBranches({ silent = false } = {}) {
: await api(`/repos/${id}/discover`, { method: "POST" }); : await api(`/repos/${id}/discover`, { method: "POST" });
state.branches = info.branches || []; state.branches = info.branches || [];
if (!form.elements.managed_branch.value) form.elements.managed_branch.value = info.default_branch || "main"; if (!form.elements.managed_branch.value) form.elements.managed_branch.value = info.default_branch || "main";
if (!form.elements.check_branches.value) form.elements.check_branches.value = state.branches.join(","); // Existing repositories keep their saved selection; a fresh one starts
// with no explicit branches checked (only the managed branch is reviewed).
if (form.elements.check_branches.value === "") setSelectedBranches([], { any: false });
renderBranchList(); renderBranchList();
hint.className = "hint ok"; hint.className = "hint ok";
hint.textContent = hint.textContent =
@@ -496,7 +531,7 @@ document.getElementById("import-list").addEventListener("click", async (ev) => {
body: { body: {
repo_url: remote.clone_url, repo_url: remote.clone_url,
managed_branch: remote.default_branch || "main", managed_branch: remote.default_branch || "main",
check_branches: remote.default_branch || "main", check_branches: "",
}, },
}); });
// Discover immediately so the branch list is populated on first open. // Discover immediately so the branch list is populated on first open.
+5 -4
View File
@@ -185,15 +185,16 @@
<button type="button" id="branch-create">克隆创建</button> <button type="button" id="branch-create">克隆创建</button>
</span> </span>
</label> </label>
<p class="hint">所有检查通过后只合并到这一个分支;它也会出现在下面的检查分支里。</p> <p class="hint">最终目标分支。它是 PR 审查的默认合并目标,但不需要出现在下面的检查分支里。</p>
<label>检查分支(可多选) <label>检查分支(push 审查;可多选)
<span class="inline"> <span class="inline">
<input name="check_branches" placeholder="main,release/1.0" /> <input name="check_branches" placeholder="留空表示只审查 PR;* 表示任意分支" />
<button type="button" id="branch-reload">加载分支</button> <button type="button" id="branch-reload">加载分支</button>
</span> </span>
</label> </label>
<div id="branch-list" class="branch-list"></div> <div id="branch-list" class="branch-list"></div>
<p class="hint">填「被保护的目标分支」。任何合并进这些分支的 PR 都会审查(功能分支不用列出来);向这些分支推送也会审查。只有指向管理分支的 PR 才会自动合并。</p> <p class="hint">选「任意分支」则所有分支的 push 都审查,也可只勾选指定分支。勾选的分支 push 通过后会自动晋级到管理分支;「任意分支」只扩大审查范围,不会自动合并任意分支。</p>
<p class="hint">PR 审查始终覆盖「勾选的分支 + 管理分支」作为合并目标,与管理分支是否列在这里无关。</p>
</fieldset> </fieldset>
<fieldset> <fieldset>
+56 -16
View File
@@ -7,7 +7,10 @@ import { test } from "node:test";
import { rmSync } from "node:fs"; import { rmSync } from "node:fs";
import { parseUnifiedDiff, addedLineNumbers, diffLineNumbers, pickAnchorLine } from "../app/lib/diff.js"; import { parseUnifiedDiff, addedLineNumbers, diffLineNumbers, pickAnchorLine } from "../app/lib/diff.js";
import { branchMatches, parseRepoUrl, pullRequestMatches } from "../app/lib/review.js"; import {
branchMatches, parseRepoUrl, pullRequestMatches,
promotesToManaged, isProtectedTarget, reviewsAnyPush, selectedBranches,
} from "../app/lib/review.js";
import { severityAtLeast, parseList } from "../app/lib/ocr.js"; import { severityAtLeast, parseList } from "../app/lib/ocr.js";
import { normalizeBaseUrl } from "../app/lib/gitea.js"; import { normalizeBaseUrl } from "../app/lib/gitea.js";
import { import {
@@ -91,23 +94,60 @@ test("pickAnchorLine falls back to context then to the start line", () => {
assert.deepEqual(pickAnchorLine(entry, 999, 999), { line: 999, inDiff: false }); assert.deepEqual(pickAnchorLine(entry, 999, 999), { line: 999, inDiff: false });
}); });
test("branchMatches matches an explicit branch list, not globs", () => { test("branchMatches reviews pushes only on the listed branches", () => {
assert.ok(branchMatches("main", "main")); assert.ok(branchMatches("test", "test,dev"), "listed push");
assert.ok(branchMatches("refs/heads/main", "main")); assert.ok(branchMatches("refs/heads/test", "test,dev"), "refs/heads prefix is stripped");
assert.ok(branchMatches("release/1.2", "main,release/1.2")); assert.ok(!branchMatches("feature/x", "test,dev"), "unlisted push");
assert.ok(branchMatches("dev", "")); assert.ok(!branchMatches("prd", "test,dev"), "the managed branch is not implicitly checked");
assert.ok(!branchMatches("feature/x", "main,release/1.2")); // An empty list means "pull requests only", not "every push".
assert.ok(!branchMatches("release/2.0", "release/*"), "globs are no longer patterns"); assert.ok(!branchMatches("anything", ""));
// Globs were never supported and a literal `*` is now the wildcard token.
assert.ok(!branchMatches("release/2.0", "release/*"), "globs are not patterns");
}); });
test("pullRequestMatches keys on the target branch, not the feature branch", () => { test("branchMatches treats the wildcard token as every branch", () => {
// The two-stage flow: feature -> test -> prd. assert.ok(reviewsAnyPush("*"));
assert.ok(pullRequestMatches("fix/x", "test", "test,prd"), "PR into test"); assert.ok(reviewsAnyPush("*,test"));
assert.ok(pullRequestMatches("test", "prd", "test,prd"), "promotion PR into prd"); assert.ok(!reviewsAnyPush("test,dev"));
assert.ok(!pullRequestMatches("fix/x", "master", "test,prd"), "PR into an unprotected branch"); assert.ok(!reviewsAnyPush(""), "empty list means pull requests only");
assert.ok(pullRequestMatches("any", "any", ""), "empty list reviews everything"); assert.ok(branchMatches("feature/whatever", "*"), "wildcard reviews any push");
// Listing a long-lived branch still reviews PRs originating from it. assert.ok(branchMatches("feature/whatever", "*,test"), "wildcard plus explicit branches");
assert.ok(pullRequestMatches("test", "unprotected", "test,prd"), "head branch listed"); assert.deepEqual(selectedBranches("*,test"), ["test"], "wildcard is not a branch name");
});
test("pullRequestMatches covers checked branches and the managed branch", () => {
const checks = "test,dev";
const managed = "prd";
assert.ok(pullRequestMatches("fix/x", "prd", checks, managed), "PR into the managed branch");
assert.ok(pullRequestMatches("fix/x", "test", checks, managed), "PR into a checked branch");
assert.ok(pullRequestMatches("test", "prd", checks, managed), "promotion PR test -> prd");
assert.ok(!pullRequestMatches("fix/x", "other", checks, managed), "PR into an unprotected branch");
assert.ok(pullRequestMatches("test", "other", checks, managed), "head branch explicitly listed");
// The managed branch is protected even though it is absent from the list.
assert.ok(pullRequestMatches("anything", "prd", "", "prd"), "empty list still protects the managed branch");
assert.ok(pullRequestMatches("anything", "other", "", ""), "no managed branch and empty list reviews all");
});
test("promotesToManaged only promotes explicitly checked branches", () => {
const checks = "test,dev";
const managed = "prd";
assert.ok(promotesToManaged("test", checks, managed), "checked branch promotes");
assert.ok(promotesToManaged("dev", checks, managed), "checked branch promotes");
assert.ok(!promotesToManaged("feature/x", checks, managed), "unlisted branch never promotes");
assert.ok(!promotesToManaged("prd", checks, managed), "the managed branch has nowhere to go");
// "任意分支" widens review but must not auto-merge arbitrary branches.
assert.ok(!promotesToManaged("feature/x", "*,test", managed), "wildcard alone does not promote");
assert.ok(promotesToManaged("test", "*,test", managed), "wildcard keeps explicit promotions");
});
test("isProtectedTarget accepts the managed branch and checked branches", () => {
const checks = "test,dev";
const managed = "prd";
assert.ok(isProtectedTarget("prd", checks, managed), "managed branch");
assert.ok(isProtectedTarget("test", checks, managed), "checked branch");
assert.ok(!isProtectedTarget("other", checks, managed), "unprotected branch");
assert.ok(!isProtectedTarget("", checks, managed), "empty ref");
assert.ok(isProtectedTarget("refs/heads/prd", checks, managed), "prefix is stripped");
}); });
test("parseRepoUrl derives owner and name from every supported URL form", () => { test("parseRepoUrl derives owner and name from every supported URL form", () => {