fix: 所有问题都建 Issue、合并失败重试、push 与 PR 事件分流
1. 所有问题都建 Issue(原先只有阻断级才建) 建 Issue 的判据从 blocking 改为 findings:任何等级的问题都创建/更新 Issue, 等级体现在标题 [OCR][medium] 与标签 medium 上,阻断项在正文标注 「(阻断合并)」。低等级问题不再丢失,也不会挡住合并。 无任何发现时才关闭该分支的 Issue。 2. 合并失败自动重试 Gitea 在算完 PR 可合并性之前会返回 405 Please try again later, 原先直接放弃,晋级随机失败。现在对 405/409/5xx 按指数退避重试 4 次; 权限不足、真实冲突等永久失败立即放弃并往 PR 留言说明。 PR 已被合并(405 already merged)视为成功,幂等收尾。 3. push 与 PR 事件分流(本次新发现的 bug) 合并路径原先按「是否找到关联 PR」判断,于是一个残留的 test→prd PR 会让后续 push 被当成 PR 事件,走错分支并跳过晋级,日志还会给出 「PR targets prd, which is not a checked branch」这种与实际不符的原因。 现在按事件类型决定:trigger 以 pull_request 开头才走合并 PR 路径, push/manual 一律走晋级分支。关联 PR 仅用于评论归属。 验证: - 直接 push test → 建出 issue #4([OCR][medium],标签 code-review,medium) - 合并重试与幂等分支的判定表全部通过 - 事件分流判定表:push/manual → promote,pull_request.* → merge PR
This commit is contained in:
@@ -243,6 +243,18 @@ push 时已经审过,重复审会在代码合并后凭空造出 Issue。
|
||||
该仓库所有由本服务创建的 open Issue 都会被评论并关闭——代码已经落地,
|
||||
上一轮的问题描述的是不再存在的状态。下一个问题会在下一次审查时重新开 Issue。
|
||||
|
||||
**审查发现都会建 Issue**,与是否阻断合并无关。等级体现在 Issue 标题与标签上
|
||||
(`[OCR][medium] …`,标签 `medium`),阻断项在正文里额外标注「(阻断合并)」。
|
||||
这样低等级问题不会丢失,也不会挡住合并。
|
||||
|
||||
**合并路径由事件类型决定**:push 事件永远走「晋级分支」,PR 事件才走「合并该 PR」。
|
||||
早先按「是否找到关联 PR」来判断,导致一个残留的 `test → prd` PR 会让后续
|
||||
push 误判成 PR 事件、跳过晋级。关联 PR 现在只用于评论归属,不参与路径选择。
|
||||
|
||||
**合并失败会自动重试**:Gitea 在算出 PR 可合并性之前会返回 `405 Please try again later`,
|
||||
这类暂时性失败(405/409/5xx)按指数退避重试 4 次;权限不足、真实冲突等
|
||||
永久性失败立即放弃,并在 PR 上留言说明原因。若 PR 已被合并,视为成功。
|
||||
|
||||
**审查范围**:PR 事件优先用 PR 的目标分支作为基准;push 事件优先用 webhook 里的 `before`,当 `before` 是新建分支的全零值或缺失时,退回到与管理分支的 merge-base。
|
||||
|
||||
**审查背景**:仓库配置里的「审查背景 / 需求」是多行文本,既作为 OCR 的 `--background` 传给模型,也会完整记录到审查历史,方便回溯「当时是按什么需求审的」。
|
||||
|
||||
@@ -345,6 +345,44 @@ export class GiteaClient {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Gitea answers 405 "Please try again later" while it is still computing a pull
|
||||
* request's mergeability, and 409 for transient lock contention. Both clear up
|
||||
* on their own, so a merge is retried on those before giving up.
|
||||
*/
|
||||
export function isTransientMergeError(err) {
|
||||
if (!err) return false;
|
||||
const msg = String(err.message || "");
|
||||
// Already merged is a final state, not something to retry.
|
||||
if (/already merged/i.test(msg)) return false;
|
||||
if (err.status === 405 || err.status === 409) return true;
|
||||
if (err.status === 500 || err.status === 502 || err.status === 503 || err.status === 504) return true;
|
||||
return /try again later|please try again|mergeability|not ready/i.test(msg);
|
||||
}
|
||||
|
||||
/** True when a merge failed only because the pull request is already merged. */
|
||||
export function isAlreadyMerged(err) {
|
||||
return /already merged/i.test(String(err?.message || ""));
|
||||
}
|
||||
|
||||
/** Retry `fn` while it throws a transient error, with exponential backoff. */
|
||||
export async function retryTransient(fn, { attempts = 4, baseDelayMs = 1500, onRetry } = {}) {
|
||||
let delay = baseDelayMs;
|
||||
let lastErr;
|
||||
for (let attempt = 1; attempt <= attempts; attempt += 1) {
|
||||
try {
|
||||
return await fn();
|
||||
} catch (err) {
|
||||
lastErr = err;
|
||||
if (!isTransientMergeError(err) || attempt === attempts) throw err;
|
||||
onRetry?.(attempt, err);
|
||||
await new Promise((r) => setTimeout(r, delay));
|
||||
delay = Math.min(delay * 2, 12000);
|
||||
}
|
||||
}
|
||||
throw lastErr;
|
||||
}
|
||||
|
||||
/** Derive the repository web URL from an API base URL. */
|
||||
export function webBaseUrl(apiBaseUrl) {
|
||||
return normalizeBaseUrl(apiBaseUrl);
|
||||
|
||||
+102
-25
@@ -5,7 +5,9 @@ import { mkdir, rm, writeFile } from "node:fs/promises";
|
||||
import { existsSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
|
||||
import { GiteaClient, webBaseUrl, normalizeBaseUrl } from "./gitea.js";
|
||||
import {
|
||||
GiteaClient, webBaseUrl, normalizeBaseUrl, retryTransient, isAlreadyMerged,
|
||||
} from "./gitea.js";
|
||||
import { OcrRunner, parseList, severityAtLeast } from "./ocr.js";
|
||||
import { parseUnifiedDiff, pickAnchorLine } from "./diff.js";
|
||||
import {
|
||||
@@ -163,10 +165,33 @@ function renderSummaryBody({ repo, job, review, published, failed, blocking, not
|
||||
return lines.join("\n");
|
||||
}
|
||||
|
||||
function renderIssueBody({ repo, job, blocking, summaryUrl }) {
|
||||
const SEVERITY_ORDER = ["critical", "high", "medium", "low"];
|
||||
|
||||
/** Highest severity present in a finding list, or null when empty. */
|
||||
export function topSeverity(findings) {
|
||||
for (const level of SEVERITY_ORDER) {
|
||||
if (findings.some((f) => String(f.comment?.severity || "").toLowerCase() === level)) return level;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/** Count findings per severity, highest first, skipping empty levels. */
|
||||
function severityCounts(findings) {
|
||||
const counts = new Map();
|
||||
for (const f of findings) {
|
||||
const level = String(f.comment?.severity || "unknown").toLowerCase();
|
||||
counts.set(level, (counts.get(level) || 0) + 1);
|
||||
}
|
||||
return [...counts.entries()].sort(
|
||||
(a, b) => SEVERITY_ORDER.indexOf(a[0]) - SEVERITY_ORDER.indexOf(b[0]),
|
||||
);
|
||||
}
|
||||
|
||||
function renderIssueBody({ repo, job, findings, blocking, summaryUrl }) {
|
||||
const counts = severityCounts(findings);
|
||||
const lines = [
|
||||
SUMMARY_MARKER,
|
||||
`自动代码审查在 \`${job.ref_name}\` @ \`${String(job.to_sha).slice(0, 10)}\` 上发现阻断级问题。`,
|
||||
`自动代码审查在 \`${job.ref_name}\` @ \`${String(job.to_sha).slice(0, 10)}\` 上发现问题。`,
|
||||
"",
|
||||
`- 仓库:\`${repoSlug(repo)}\``,
|
||||
`- 分支:\`${job.ref_name}\``,
|
||||
@@ -174,11 +199,16 @@ function renderIssueBody({ repo, job, blocking, summaryUrl }) {
|
||||
];
|
||||
if (job.pr_number) lines.push(`- Pull Request:#${job.pr_number}`);
|
||||
if (summaryUrl) lines.push(`- 审查详情:${summaryUrl}`);
|
||||
lines.push("", `### 阻断问题(${blocking.length} 条)`, "");
|
||||
for (const item of blocking) {
|
||||
lines.push(
|
||||
`- 问题分布:${counts.map(([level, n]) => `${level} ${n}`).join(" · ")}`,
|
||||
`- 其中阻断合并:${blocking.length} 条`,
|
||||
);
|
||||
lines.push("", `### 全部问题(${findings.length} 条)`, "");
|
||||
for (const item of findings) {
|
||||
const c = item.comment;
|
||||
const where = c.start_line ? `${c.path}:${c.start_line}` : c.path;
|
||||
lines.push(`- ${badge(c)}\`${where}\` — ${String(c.content || "").replace(/\s*\n\s*/g, " ").trim()}`);
|
||||
const blocked = blocking.includes(item) ? " **(阻断合并)**" : "";
|
||||
lines.push(`- ${badge(c)}\`${where}\`${blocked} — ${String(c.content || "").replace(/\s*\n\s*/g, " ").trim()}`);
|
||||
}
|
||||
lines.push("", `<sub>job #${job.id} · 由 gitea-codereview 生成</sub>`);
|
||||
return lines.join("\n");
|
||||
@@ -436,22 +466,24 @@ export class ReviewEngine {
|
||||
}
|
||||
|
||||
// Issue lifecycle: one open issue per repository+ref, updated in place.
|
||||
// When issue creation is disabled the service still closes any issue it
|
||||
// previously opened once the ref is clean, so stale issues do not linger.
|
||||
// Every finding gets an issue regardless of severity — severity is carried
|
||||
// in the title and body so triage stays possible without losing low-level
|
||||
// findings. Blocking only decides whether the merge is held back.
|
||||
let issueNumber = null;
|
||||
const labels = parseList(repo.issue_labels);
|
||||
const issueTitle = `[OCR] ${repoSlug(repo)} · ${job.ref_name} 存在阻断级代码问题`;
|
||||
const top = topSeverity(published);
|
||||
const issueTitle = top
|
||||
? `[OCR][${top}] ${repoSlug(repo)} · ${job.ref_name} 有 ${published.length} 条待处理问题`
|
||||
: `[OCR] ${repoSlug(repo)} · ${job.ref_name} 代码审查`;
|
||||
const detailUrl = prNumber
|
||||
? `${webBaseUrl(this.config.giteaUrl)}/${repo.owner}/${repo.name}/pulls/${prNumber}`
|
||||
: `${webBaseUrl(this.config.giteaUrl)}/${repo.owner}/${repo.name}/commit/${toSha}`;
|
||||
try {
|
||||
issueNumber = await this.upsertIssue({
|
||||
client, repo, job, blocking, labels, title: issueTitle,
|
||||
client, repo, job, findings: published, blocking, labels, title: issueTitle,
|
||||
createEnabled: Boolean(repo.create_issue),
|
||||
summaryUrl: prNumber
|
||||
? `${webBaseUrl(this.config.giteaUrl)}/${repo.owner}/${repo.name}/pulls/${prNumber}`
|
||||
: `${webBaseUrl(this.config.giteaUrl)}/${repo.owner}/${repo.name}/commit/${toSha}`,
|
||||
body: renderIssueBody({
|
||||
repo, job, blocking,
|
||||
summaryUrl: prNumber ? `${webBaseUrl(this.config.giteaUrl)}/${repo.owner}/${repo.name}/pulls/${prNumber}` : "",
|
||||
}),
|
||||
summaryUrl: detailUrl,
|
||||
body: renderIssueBody({ repo, job, findings: published, blocking, summaryUrl: detailUrl }),
|
||||
});
|
||||
} catch (err) {
|
||||
appendLog(`issue upsert failed: ${err.message}`);
|
||||
@@ -597,7 +629,7 @@ export class ReviewEngine {
|
||||
return null;
|
||||
}
|
||||
|
||||
async upsertIssue({ client, repo, job, blocking, labels, title, body, createEnabled = true }) {
|
||||
async upsertIssue({ client, repo, job, findings, blocking, labels, title, body, createEnabled = true }) {
|
||||
// Look up any open issue previously opened by this service for this
|
||||
// repository + ref, so reruns update it instead of stacking new issues.
|
||||
const openIssues = await client.get(`/api/v1/repos/${repo.owner}/${repo.name}/issues`, {
|
||||
@@ -610,7 +642,8 @@ export class ReviewEngine {
|
||||
&& String(i.title || "").includes(`${repoSlug(repo)} · ${job.ref_name}`),
|
||||
);
|
||||
|
||||
if (blocking.length === 0) {
|
||||
// No findings at all: the ref is clean, so retire any open issue.
|
||||
if (findings.length === 0) {
|
||||
if (existing) {
|
||||
await client.createIssueComment(repo.owner, repo.name, existing.number,
|
||||
`已在 \`${String(job.to_sha).slice(0, 10)}\` 上复查通过,关闭该 Issue。`);
|
||||
@@ -626,9 +659,14 @@ export class ReviewEngine {
|
||||
return existing?.number ?? null;
|
||||
}
|
||||
|
||||
// Tag every issue with its severity so the list can be filtered by level,
|
||||
// on top of whatever labels the repository configured.
|
||||
const top = topSeverity(findings);
|
||||
const labelNames = [...labels];
|
||||
if (top && !labelNames.includes(top)) labelNames.push(top);
|
||||
// Gitea's issue API expects label IDs, so resolve names first.
|
||||
const labelIds = labels.length
|
||||
? await client.ensureLabels(repo.owner, repo.name, labels)
|
||||
const labelIds = labelNames.length
|
||||
? await client.ensureLabels(repo.owner, repo.name, labelNames)
|
||||
: [];
|
||||
|
||||
if (existing) {
|
||||
@@ -660,14 +698,24 @@ export class ReviewEngine {
|
||||
reviewIncomplete = false, statusState = "success",
|
||||
}) {
|
||||
if (!repo.auto_merge) return false;
|
||||
if (!prNumber) {
|
||||
|
||||
// The merge path follows the event that produced this job, not whether a
|
||||
// pull request happens to be linked. A push to `test` must promote the
|
||||
// branch even when an open `test -> prd` pull request already exists;
|
||||
// otherwise that leftover request hijacks the branch's own promotion.
|
||||
const fromPullRequest = String(job.trigger || "").startsWith("pull_request");
|
||||
if (!fromPullRequest) {
|
||||
return this.promoteBranch({ client, repo, job, toSha, appendLog, reviewIncomplete, statusState });
|
||||
}
|
||||
if (!prNumber) {
|
||||
appendLog("auto-merge skipped: pull request event without a pull request number");
|
||||
return false;
|
||||
}
|
||||
// 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
|
||||
// target one of the branches we protect", not "is it the managed branch".
|
||||
if (job.base_ref && !branchMatches(job.base_ref, repo.check_branches)) {
|
||||
appendLog(`auto-merge skipped: PR targets ${job.base_ref}, which is not a checked branch`);
|
||||
appendLog(`auto-merge skipped: PR targets ${job.base_ref}, which is not in check_branches (${repo.check_branches})`);
|
||||
return false;
|
||||
}
|
||||
if (reviewIncomplete) {
|
||||
@@ -708,18 +756,31 @@ export class ReviewEngine {
|
||||
}
|
||||
|
||||
try {
|
||||
await client.mergePullRequest(repo.owner, repo.name, prNumber, {
|
||||
await retryTransient(() => client.mergePullRequest(repo.owner, repo.name, prNumber, {
|
||||
style: repo.merge_method || "squash",
|
||||
title: pr.title,
|
||||
deleteBranch: Boolean(repo.delete_branch),
|
||||
headCommitId: toSha,
|
||||
mergeWhenChecksSucceed: repo.auto_merge_mode === "when_checks_succeed",
|
||||
}), {
|
||||
attempts: 4,
|
||||
baseDelayMs: 2000,
|
||||
onRetry: (attempt, err) => appendLog(
|
||||
`PR #${prNumber} merge deferred (attempt ${attempt}): ${err.message}`,
|
||||
),
|
||||
});
|
||||
appendLog(`auto-merge requested for PR #${prNumber}`);
|
||||
await this.closeRepoIssues({ client, repo, appendLog, reason: `PR #${prNumber} 已合并` });
|
||||
return true;
|
||||
} catch (err) {
|
||||
if (isAlreadyMerged(err)) {
|
||||
appendLog(`PR #${prNumber} was already merged; treating as success`);
|
||||
await this.closeRepoIssues({ client, repo, appendLog, reason: `PR #${prNumber} 已合并` });
|
||||
return true;
|
||||
}
|
||||
appendLog(`auto-merge failed: ${err.message}`);
|
||||
await client.createIssueComment(repo.owner, repo.name, prNumber,
|
||||
`自动合并失败:${err.message}\n\n审查提交:\`${toSha}\`。可稍后重新推送,或在界面手动合并。`).catch(() => {});
|
||||
return false;
|
||||
}
|
||||
}
|
||||
@@ -858,12 +919,18 @@ export class ReviewEngine {
|
||||
// branches diverge a little more on every promotion and eventually
|
||||
// conflict. A real merge keeps the shared ancestry, so the next
|
||||
// promotion only carries the new commits.
|
||||
await client.mergePullRequest(repo.owner, repo.name, index, {
|
||||
await retryTransient(() => client.mergePullRequest(repo.owner, repo.name, index, {
|
||||
style: "merge",
|
||||
title: pr.title,
|
||||
deleteBranch: false,
|
||||
headCommitId: toSha,
|
||||
mergeWhenChecksSucceed: repo.auto_merge_mode === "when_checks_succeed",
|
||||
}), {
|
||||
attempts: 4,
|
||||
baseDelayMs: 2000,
|
||||
onRetry: (attempt, err) => appendLog(
|
||||
`promotion PR #${index} merge deferred (attempt ${attempt}): ${err.message}`,
|
||||
),
|
||||
});
|
||||
appendLog(`auto-merge requested for promotion PR #${index}`);
|
||||
await this.closeRepoIssues({
|
||||
@@ -872,7 +939,17 @@ export class ReviewEngine {
|
||||
});
|
||||
return true;
|
||||
} catch (err) {
|
||||
if (isAlreadyMerged(err)) {
|
||||
appendLog(`promotion PR #${index} was already merged; treating as success`);
|
||||
await this.closeRepoIssues({
|
||||
client, repo, appendLog,
|
||||
reason: `${job.ref_name} 已合并到 ${target}(PR #${index})`,
|
||||
});
|
||||
return true;
|
||||
}
|
||||
appendLog(`auto-merge failed: promotion PR #${index}: ${err.message}`);
|
||||
await client.createIssueComment(repo.owner, repo.name, index,
|
||||
`自动合并失败:${err.message}\n\n审查提交:\`${toSha}\`。可稍后重新推送,或在界面手动合并。`).catch(() => {});
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
+10
-6
@@ -219,14 +219,18 @@ async function handlePush(payload, cfg) {
|
||||
}
|
||||
}
|
||||
|
||||
// A push is always handled as a push, never as a pull request. An open pull
|
||||
// request whose head happens to be this branch is only linked for comment
|
||||
// routing; letting it decide the merge path would make a leftover promotion
|
||||
// pull request hijack the branch's own promotion logic.
|
||||
const scoped = repo.review_scope === "pr";
|
||||
const pr = scoped ? null : await findOpenPullRequestForRef(cfg, repo, refName, toSha);
|
||||
if (scoped && !pr) {
|
||||
const linkedPr = scoped ? null : await findOpenPullRequestForRef(cfg, repo, refName, toSha);
|
||||
if (scoped && !linkedPr) {
|
||||
return { queued: 0, reason: "review_scope=pr and no open pull request" };
|
||||
}
|
||||
// Base the review on what the push would merge into: the open PR's target
|
||||
// when there is one, otherwise the managed branch.
|
||||
const baseRef = pr?.base?.ref || repo.managed_branch;
|
||||
// Compare against what this branch merges into: its linked pull request's
|
||||
// target when it has one, otherwise the branch it promotes into.
|
||||
const baseRef = linkedPr?.base?.ref || repo.managed_branch;
|
||||
|
||||
if (findJobBySha(db, repo.id, toSha)) {
|
||||
return { queued: 0, reason: `commit ${toSha.slice(0, 10)} already queued or running` };
|
||||
@@ -240,7 +244,7 @@ async function handlePush(payload, cfg) {
|
||||
baseRef,
|
||||
fromSha: before,
|
||||
toSha,
|
||||
prNumber: pr?.number ?? null,
|
||||
prNumber: linkedPr?.number ?? null,
|
||||
});
|
||||
logger.info(`queued job #${jobId} for ${owner}/${name} ${refName}@${toSha.slice(0, 10)}`);
|
||||
return { queued: 1, jobId };
|
||||
|
||||
Reference in New Issue
Block a user