From 7cd2c85888eacfc4e34d703e25fdc9b78a458a4e Mon Sep 17 00:00:00 2001 From: kgod <1257628228@qq.com> Date: Mon, 21 Sep 2026 15:16:06 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20=E6=89=80=E6=9C=89=E9=97=AE=E9=A2=98?= =?UTF-8?q?=E9=83=BD=E5=BB=BA=20Issue=E3=80=81=E5=90=88=E5=B9=B6=E5=A4=B1?= =?UTF-8?q?=E8=B4=A5=E9=87=8D=E8=AF=95=E3=80=81push=20=E4=B8=8E=20PR=20?= =?UTF-8?q?=E4=BA=8B=E4=BB=B6=E5=88=86=E6=B5=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- README.md | 12 +++++ app/lib/gitea.js | 38 ++++++++++++++ app/lib/review.js | 127 +++++++++++++++++++++++++++++++++++++--------- app/server.js | 16 +++--- 4 files changed, 162 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index c870155..5bcb437 100644 --- a/README.md +++ b/README.md @@ -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` 传给模型，也会完整记录到审查历史，方便回溯「当时是按什么需求审的」。 diff --git a/app/lib/gitea.js b/app/lib/gitea.js index b06bdc9..00923cf 100644 --- a/app/lib/gitea.js +++ b/app/lib/gitea.js @@ -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); diff --git a/app/lib/review.js b/app/lib/review.js index ea85e9d..f20b5ec 100644 --- a/app/lib/review.js +++ b/app/lib/review.js @@ -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("", `job #${job.id} · 由 gitea-codereview 生成`); 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; } } diff --git a/app/server.js b/app/server.js index 7f5b4c5..35ff2c6 100644 --- a/app/server.js +++ b/app/server.js @@ -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 };