From a531323865cac1815e79b1c910640b17925d4ddc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 11:48:06 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20diff-fetch=20fallback=20chain=20?= =?UTF-8?q?=E2=80=94=20stop=20failing=20open=20on=20compare=20404?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~23% of reviews were failing open with `diff fetch failed: gitea GET /{owner}/{repo}/compare/{before}...{after}.diff -> 404`. Probing the live forge (Gitea 1.27.1) showed the web compare route answers 404 for ALL refs on private repos — it does not honor `Authorization: token` and resolves as anonymous — while the API compare (JSON) and per-commit /git/commits/{sha}.diff endpoints succeed for the exact same sha pairs. It also 404s legitimately when `before` is unknown (force-push, rebase). fetchDiff now walks a fallback chain instead of failing open on the first 404 (any non-404 error still propagates immediately): PR job: pulls/{n}.diff -> git/commits/{head}.diff range push: web compare {before}...{after}.diff -> web compare {defaultBranch}...{after}.diff -> API compare commit list (falls back to the webhook's commitShas) -> concatenated per-commit git/commits/{sha}.diff, stopping past maxDiffBytes so the existing truncation cap (verdict capped at 'warn') kicks in -> git/commits/{after}.diff alone new branch: git/commits/{after}.diff (unchanged) Only when every source is exhausted does the review fail open, and the log line now lists every URL tried. Webhook jobs carry the repo default_branch so the fallback needs no extra API call; getRepo() covers recovered jobs persisted before this change. Verified against the live forge for the exact failing pair from today's service.log (Nirlabinc/shreai 481d8663...46696b50): the chain recovers a full-range 8,993-byte diff via 2/2 per-commit diffs. Co-Authored-By: Claude Fable 5 --- src/lib/gitea.js | 12 +++- src/lib/review.js | 133 +++++++++++++++++++++++++++++++++++---- src/lib/webhook.js | 2 + test/fetchdiff.test.js | 139 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 273 insertions(+), 13 deletions(-) diff --git a/src/lib/gitea.js b/src/lib/gitea.js index 001efde..a2a4219 100644 --- a/src/lib/gitea.js +++ b/src/lib/gitea.js @@ -42,12 +42,20 @@ export class GiteaClient { } // Full-range diff for a push (base...head) in ONE request. Gitea 1.27's API - // has no compare .diff variant, but the web compare route serves .diff and - // accepts `Authorization: token` (verified live on 1.27.1). + // has no compare .diff variant, but the web compare route serves .diff. + // CAVEAT (probed live on 1.27.1): the web route does NOT honor + // `Authorization: token` for private repos — it resolves as anonymous and + // answers 404 even for valid refs. Callers must be ready to fall back to + // the API-based sources below (fetchDiff in review.js does). getCompareDiff(owner, repo, before, after) { return this.req('GET', `/${owner}/${repo}/compare/${before}...${after}.diff`, { raw: true }); } + // Repo metadata (used for default_branch when the job doesn't carry it) + getRepo(owner, repo) { + return this.req('GET', `/api/v1/repos/${owner}/${repo}`); + } + // Add a collaborator (requires owner/admin token). 204 on success. addCollaborator(owner, repo, username, permission = 'write') { return this.req('PUT', `/api/v1/repos/${owner}/${repo}/collaborators/${username}`, { diff --git a/src/lib/review.js b/src/lib/review.js index 43bc700..1f45984 100644 --- a/src/lib/review.js +++ b/src/lib/review.js @@ -50,7 +50,10 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l // make the model "review" nothing and pass; never send one. let diff = '', truncated = false, diffBytes = 0; try { - diff = await fetchDiff({ gitea, job }); + diff = await fetchDiff({ + gitea, job, maxBytes: cfg.maxDiffBytes, + log: (m) => log(`[${fullRepo}@${sha.slice(0, 8)}] ${m}`) + }); } catch (e) { return await failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, @@ -159,17 +162,125 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l return { ok: true, verdict: review.verdict }; } -// Push reviews cover the FULL push range in ONE compare diff (base=before, -// head=after) — never a per-commit subset, so the head status can't claim -// success for commits that were silently dropped. New-branch pushes -// (before = 0000…) fall back to the head commit's diff. -export async function fetchDiff({ gitea, job }) { - const { owner, repo } = job; - if (job.prIndex) return gitea.getPrDiff(owner, repo, job.prIndex); - if (!job.before || ZERO_SHA.test(job.before)) { - return gitea.getCommitDiff(owner, repo, job.sha); +// Push reviews cover the FULL push range — never a silent per-commit subset, +// so the head status can't claim success for commits that were dropped. +// +// Primary sources: PR jobs use the API PR diff; range pushes use ONE web +// compare diff (base=before, head=after); new-branch pushes (before = 0000…) +// use the head commit's diff. +// +// Fallback chain (a 404 or empty body moves to the next source; any other +// error — 5xx, network, timeout — propagates immediately, those are forge +// failures and trying other refs would only hide them): +// PR job: pulls/{n}.diff → git/commits/{head}.diff +// range push: +// 1. web compare {before}...{after}.diff (404s when `before` is +// unknown — force-push/rebase — AND, probed live on Gitea 1.27.1, for +// ALL refs on private repos: the web route ignores token auth) +// 2. web compare {defaultBranch}...{after}.diff (covers unknown-`before` +// where the web route itself works, e.g. public repos) +// 3. API compare {before}...{after} (JSON commit list; this endpoint DOES +// honor token auth) → concatenated per-commit git/commits/{sha}.diff, +// falling back to the webhook's commitShas for the list. Stops once +// past maxBytes — review.js then truncates and caps the verdict. +// 4. git/commits/{after}.diff (head only, unless 3 already tried it) +// Only when every source is exhausted does the error propagate to fail-open, +// listing every URL tried. +export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinity }) { + const { owner, repo, sha } = job; + const tried = []; + + // Try one diff source: diff text on success, null on 404/empty (recorded + // in `tried`), throw on anything else. + const attempt = async (label, fn) => { + try { + const out = await fn(); + if (out && out.trim()) return out; + tried.push(`${label}: empty diff body`); + return null; + } catch (e) { + if (e.status !== 404) throw e; + tried.push(e.message); + return null; + } + }; + + let diff; + if (job.prIndex) { + diff = await attempt(`pr #${job.prIndex} diff`, () => gitea.getPrDiff(owner, repo, job.prIndex)); + if (diff) return diff; + diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); + if (diff) { + log(`diff via fallback: head commit ${sha.slice(0, 8)} (PR #${job.prIndex} diff unavailable)`); + return diff; + } + throw allDiffSourcesFailed(tried); } - return gitea.getCompareDiff(owner, repo, job.before, job.sha); + + if (job.before && !ZERO_SHA.test(job.before)) { + // 1. full-range web compare (the normal path) + diff = await attempt(`compare ${job.before.slice(0, 8)}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, job.before, sha)); + if (diff) return diff; + + // 2. range against the repo default branch (base the server surely has) + let defaultBranch = job.defaultBranch; + if (!defaultBranch) { + try { defaultBranch = (await gitea.getRepo(owner, repo))?.default_branch; } + catch (e) { tried.push(e.message); } + } + if (defaultBranch) { + diff = await attempt(`compare ${defaultBranch}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, defaultBranch, sha)); + if (diff) { + log(`diff via fallback: compare ${defaultBranch}...${sha.slice(0, 8)}`); + return diff; + } + } + + // 3. reconstruct the range from the API: commit list, then per-commit diffs + let shas = null; + try { + const cmp = await gitea.compare(owner, repo, job.before, sha); + const list = (cmp?.commits || []).map(c => c?.sha).filter(Boolean); + if (list.length) shas = list; + else tried.push(`api compare ${job.before.slice(0, 8)}...${sha.slice(0, 8)}: no commits`); + } catch (e) { + if (e.status !== 404) throw e; + tried.push(e.message); + } + if (!shas && Array.isArray(job.commitShas) && job.commitShas.length) shas = job.commitShas; + if (shas) { + const parts = []; + let total = 0; + for (const s of shas) { + const d = await attempt(`commit ${s.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, s)); + if (d) { parts.push(d); total += Buffer.byteLength(d); } + if (total > maxBytes) break; // review.js truncates + caps the verdict + } + if (total > 0) { + log(`diff via fallback: ${parts.length}/${shas.length} per-commit diff(s) over ${job.before.slice(0, 8)}...${sha.slice(0, 8)}`); + return parts.join('\n'); + } + } + + // 4. last resort: head commit alone (skip if step 3 already tried it) + if (!shas || !shas.includes(sha)) { + diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); + if (diff) { + log(`diff via fallback: head commit ${sha.slice(0, 8)} only`); + return diff; + } + } + throw allDiffSourcesFailed(tried); + } + + // new-branch push (before = 0000… or missing): head commit diff + diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); + if (diff) return diff; + throw allDiffSourcesFailed(tried); +} + +function allDiffSourcesFailed(tried) { + return new Error(`all diff sources failed: ${tried.join(' | ')}`); } async function failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error, state = 'warning', description }) { diff --git a/src/lib/webhook.js b/src/lib/webhook.js index 7976a03..43ed17f 100644 --- a/src/lib/webhook.js +++ b/src/lib/webhook.js @@ -27,6 +27,7 @@ export function jobFromWebhook(event, payload) { sha: after, ref: payload.ref, before: payload.before, + defaultBranch: payload.repository?.default_branch || null, prIndex: null, prTitle: null, pusher: payload.pusher?.username || payload.pusher?.login || null, @@ -48,6 +49,7 @@ export function jobFromWebhook(event, payload) { sha: pr.head?.sha, ref: pr.head?.ref, before: null, + defaultBranch: payload.repository?.default_branch || null, prIndex: pr.number, prTitle: pr.title || null, pusher: payload.sender?.username || payload.sender?.login || null, diff --git a/test/fetchdiff.test.js b/test/fetchdiff.test.js index 54b9283..5c13307 100644 --- a/test/fetchdiff.test.js +++ b/test/fetchdiff.test.js @@ -64,3 +64,142 @@ test('compare failure propagates (no silent placeholder diff)', async () => { /boom 500/ ); }); + +// ---- fallback chain (compare 404: force-push, rebase-then-push, or the web +// ---- compare route ignoring token auth on private repos) ---- + +function nf(path) { + const e = new Error(`gitea GET ${path} -> 404: Not found.`); + e.status = 404; + return e; +} + +test('compare 404 falls back to default-branch compare', async () => { + const g = giteaStub(); + const real = g.getCompareDiff; + g.getCompareDiff = (o, r, before, after) => before === BEFORE + ? Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`)) + : real(o, r, before, after); + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } + }); + assert.equal(out, 'RANGEDIFF'); + assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'main', HEAD]); +}); + +test('compare 404 without defaultBranch on the job looks it up via getRepo', async () => { + const g = giteaStub(); + const real = g.getCompareDiff; + g.getCompareDiff = (o, r, before, after) => before === BEFORE + ? Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`)) + : real(o, r, before, after); + g.getRepo = () => Promise.resolve({ default_branch: 'develop' }); + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE } + }); + assert.equal(out, 'RANGEDIFF'); + assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'develop', HEAD]); +}); + +test('both compares 404 -> API commit list -> concatenated per-commit diffs', async () => { + const g = giteaStub(); + const C1 = 'c'.repeat(40), C2 = 'd'.repeat(40); + g.getCompareDiff = (o, r, before, after) => + Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`)); + g.compare = (o, r, before, after) => { + g.calls.push(['apicompare', o, r, before, after]); + return Promise.resolve({ total_commits: 2, commits: [{ sha: C1 }, { sha: C2 }] }); + }; + g.getCommitDiff = (o, r, sha) => { + g.calls.push(['commit', o, r, sha]); + return Promise.resolve(`DIFF(${sha.slice(0, 1)})`); + }; + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } + }); + assert.equal(out, 'DIFF(c)\nDIFF(d)'); + assert.deepEqual(g.calls.filter(c => c[0] === 'commit'), [['commit', 'o', 'r', C1], ['commit', 'o', 'r', C2]]); +}); + +test('API compare 404 too -> per-commit diffs from the webhook commitShas', async () => { + const g = giteaStub(); + const C1 = 'c'.repeat(40); + g.getCompareDiff = (o, r, before, after) => + Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`)); + g.compare = (o, r, before, after) => + Promise.reject(nf(`/api/v1/repos/o/r/compare/${before}...${after}`)); + g.getCommitDiff = (o, r, sha) => { + g.calls.push(['commit', o, r, sha]); + return Promise.resolve(`DIFF(${sha.slice(0, 1)})`); + }; + const out = await fetchDiff({ + gitea: g, + job: { + owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, + defaultBranch: 'main', commitShas: [C1, HEAD] + } + }); + assert.equal(out, 'DIFF(c)\nDIFF(a)'); +}); + +test('per-commit fallback stops fetching past maxBytes (review truncates after)', async () => { + const g = giteaStub(); + const shas = ['c'.repeat(40), 'd'.repeat(40), 'e'.repeat(40)]; + g.getCompareDiff = () => Promise.reject(nf('/o/r/compare/x...y.diff')); + g.compare = () => Promise.resolve({ commits: shas.map(sha => ({ sha })) }); + const fetched = []; + g.getCommitDiff = (o, r, sha) => { fetched.push(sha); return Promise.resolve('x'.repeat(10)); }; + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' }, + maxBytes: 15 + }); + // first two diffs exceed the cap (20 > 15) -> third commit never fetched + assert.equal(fetched.length, 2); + assert.equal(out, 'x'.repeat(10) + '\n' + 'x'.repeat(10)); +}); + +test('every source 404 -> throws listing every URL tried (then fail-open)', async () => { + const g = giteaStub(); + g.getCompareDiff = (o, r, before, after) => + Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`)); + g.compare = (o, r, before, after) => + Promise.reject(nf(`/api/v1/repos/o/r/compare/${before}...${after}`)); + g.getCommitDiff = (o, r, sha) => + Promise.reject(nf(`/api/v1/repos/o/r/git/commits/${sha}.diff`)); + await assert.rejects( + () => fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } + }), + (e) => { + assert.match(e.message, /^all diff sources failed: /); + assert.match(e.message, new RegExp(`compare/${BEFORE}\\.\\.\\.${HEAD}\\.diff`)); + assert.match(e.message, new RegExp(`compare/main\\.\\.\\.${HEAD}\\.diff`)); + assert.match(e.message, new RegExp(`api/v1/repos/o/r/compare/${BEFORE}`)); + assert.match(e.message, new RegExp(`git/commits/${HEAD}\\.diff`)); + return true; + } + ); +}); + +test('PR diff 404 falls back to the head commit diff', async () => { + const g = giteaStub(); + g.getPrDiff = (o, r, i) => Promise.reject(nf(`/api/v1/repos/o/r/pulls/${i}.diff`)); + const out = await fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: 7, sha: HEAD, before: null } }); + assert.equal(out, 'HEADDIFF'); + assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]); +}); + +test('empty compare body is treated as a miss, not a reviewable diff', async () => { + const g = giteaStub(); + g.getCompareDiff = (o, r, before) => before === BEFORE ? Promise.resolve('') : Promise.resolve('RANGEDIFF'); + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } + }); + assert.equal(out, 'RANGEDIFF'); +}); From ad8c1256d88e467e65c36e7ea3a7735bba7c21dd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 11:58:59 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20dual-review=20round-2=20=E2=80=94=20?= =?UTF-8?q?partial=20diffs=20cap=20the=20verdict,=20bounded=20fallback=20c?= =?UTF-8?q?hain,=20URL-safe=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex adversarial review of the fallback chain found 1 P1 + 3 P2: - P1: head-only and incomplete per-commit fallbacks could present a SUBSET diff as a full passing review. fetchDiff now returns {diff, partial, note}; review.js caps partial reviews at 'warn' (same contract as truncation) and labels the status 'partial review ()'. - getRepo default-branch lookup swallows only 404; auth/5xx propagate. - per-commit reconstruction dedupes + validates shas (40-hex) and hard-caps at 20 requests; capped/short coverage marks the result partial; the all-sources-failed message is bounded to 1500 chars. - gitea.js URL-encodes every webhook-supplied path segment (owner/repo/ref/ sha) — branch names with '/' or '#' can no longer distort request paths. Tests: 61 pass (2 new: sha cap/dedupe, getRepo 401 propagation). Co-Authored-By: Claude Fable 5 --- src/lib/gitea.js | 14 ++++--- src/lib/review.js | 88 +++++++++++++++++++++++++++++------------- test/fetchdiff.test.js | 65 +++++++++++++++++++++++++------ 3 files changed, 125 insertions(+), 42 deletions(-) diff --git a/src/lib/gitea.js b/src/lib/gitea.js index a2a4219..651836c 100644 --- a/src/lib/gitea.js +++ b/src/lib/gitea.js @@ -1,5 +1,9 @@ // Minimal Gitea API client (fetch-based, no deps). +// URL path segment from webhook/API-supplied values (owner, repo, refs, +// shas): never trust them as URL-safe — branch names can carry '/', '#', '?'. +const seg = (s) => encodeURIComponent(String(s)); + export class GiteaClient { constructor({ baseUrl, token, timeoutMs = 30000 }) { this.baseUrl = baseUrl.replace(/\/$/, ''); @@ -28,17 +32,17 @@ export class GiteaClient { // PR diff getPrDiff(owner, repo, index) { - return this.req('GET', `/api/v1/repos/${owner}/${repo}/pulls/${index}.diff`, { raw: true }); + return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/pulls/${seg(index)}.diff`, { raw: true }); } // Single-commit diff getCommitDiff(owner, repo, sha) { - return this.req('GET', `/api/v1/repos/${owner}/${repo}/git/commits/${sha}.diff`, { raw: true }); + return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/git/commits/${seg(sha)}.diff`, { raw: true }); } // Compare (commit list between two shas) compare(owner, repo, before, after) { - return this.req('GET', `/api/v1/repos/${owner}/${repo}/compare/${before}...${after}`); + return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/compare/${seg(before)}...${seg(after)}`); } // Full-range diff for a push (base...head) in ONE request. Gitea 1.27's API @@ -48,12 +52,12 @@ export class GiteaClient { // answers 404 even for valid refs. Callers must be ready to fall back to // the API-based sources below (fetchDiff in review.js does). getCompareDiff(owner, repo, before, after) { - return this.req('GET', `/${owner}/${repo}/compare/${before}...${after}.diff`, { raw: true }); + return this.req('GET', `/${seg(owner)}/${seg(repo)}/compare/${seg(before)}...${seg(after)}.diff`, { raw: true }); } // Repo metadata (used for default_branch when the job doesn't carry it) getRepo(owner, repo) { - return this.req('GET', `/api/v1/repos/${owner}/${repo}`); + return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}`); } // Add a collaborator (requires owner/admin token). 204 on success. diff --git a/src/lib/review.js b/src/lib/review.js index 1f45984..27f2fd8 100644 --- a/src/lib/review.js +++ b/src/lib/review.js @@ -48,12 +48,15 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l // 1. diff — any fetch failure (throw, non-200, or empty body for a job that // has commits) short-circuits to a 'warning' status. A diffless prompt would // make the model "review" nothing and pass; never send one. - let diff = '', truncated = false, diffBytes = 0; + let diff = '', truncated = false, diffBytes = 0, partialDiff = false, partialNote = null; try { - diff = await fetchDiff({ + const fetched = await fetchDiff({ gitea, job, maxBytes: cfg.maxDiffBytes, log: (m) => log(`[${fullRepo}@${sha.slice(0, 8)}] ${m}`) }); + diff = fetched.diff; + partialDiff = fetched.partial; + partialNote = fetched.note; } catch (e) { return await failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, @@ -121,9 +124,11 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l return await failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error: `review unavailable: ${llmError}` }); } - // A truncated diff can never yield a clean 'pass' — part of the change was - // not reviewed. Cap at 'warn' (a confirmed-critical 'fail' still fails). - applyTruncationCap(review, truncated); + // A truncated OR partial diff can never yield a clean 'pass' — part of the + // change was not reviewed. Cap at 'warn' (a confirmed-critical 'fail' still + // fails). Partial = a fallback source covered less than the full range + // (e.g. head commit only); reviewing a subset must not read as a full pass. + applyTruncationCap(review, truncated || partialDiff); // 4. status + comment const stateByVerdict = { pass: 'success', warn: 'success', fail: 'failure' }; @@ -133,13 +138,17 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l warn: `Grade ${review.grade} (${review.overall}/100) — pass with warnings (${review.findings.length} finding(s))`, fail: `Grade ${review.grade} (${review.overall}/100) — BLOCKED: confirmed critical finding (admin merge = override)` }; - const description = truncated - ? `partial review (diff truncated) — ${descByVerdict[review.verdict]}` + const partialReasons = [ + ...(truncated ? ['diff truncated'] : []), + ...(partialDiff ? [partialNote || 'incomplete diff coverage'] : []) + ]; + const description = partialReasons.length + ? `partial review (${partialReasons.join('; ')}) — ${descByVerdict[review.verdict]}` : descByVerdict[review.verdict]; await safe(log, 'final-status', () => postStatus({ state, context: CONTEXT, description, target_url: dashboardUrl })); - const md = renderScorecardMarkdown({ repo: fullRepo, sha, review, dashboardUrl, truncated }); + const md = renderScorecardMarkdown({ repo: fullRepo, sha, review, dashboardUrl, truncated: truncated || partialDiff }); if (job.prIndex) { await safe(log, 'pr-comment', () => postComment(job.prIndex, md)); } else { @@ -155,7 +164,7 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l pusher: job.pusher, authors: job.authors, trailers, axes: review.axes, overall: review.overall, grade: review.grade, verdict: review.verdict, findings: review.findings, - statusState: state, error: null, diffBytes, truncated, + statusState: state, error: null, diffBytes, truncated: truncated || partialDiff, model: servedModel || cfg.model, durationMs: Date.now() - t0 }); log(`[${fullRepo}@${sha.slice(0, 8)}] reviewed: ${review.verdict} grade=${review.grade} overall=${review.overall} findings=${review.findings.length} in ${Date.now() - t0}ms`); @@ -186,6 +195,15 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l // 4. git/commits/{after}.diff (head only, unless 3 already tried it) // Only when every source is exhausted does the error propagate to fail-open, // listing every URL tried. +// Returns { diff, partial, note }. `partial: true` means the source covered +// LESS than the job's full range (head-only fallback, capped/incomplete +// per-commit reconstruction) — review.js caps the verdict at 'warn' so a +// subset review can never read as a full pass. +const SHA_RE = /^[0-9a-f]{7,40}$/i; +const MAX_COMMIT_DIFFS = 20; +const full = (diff) => ({ diff, partial: false, note: null }); +const part = (diff, note) => ({ diff, partial: true, note }); + export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinity }) { const { owner, repo, sha } = job; const tried = []; @@ -208,11 +226,11 @@ export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinit let diff; if (job.prIndex) { diff = await attempt(`pr #${job.prIndex} diff`, () => gitea.getPrDiff(owner, repo, job.prIndex)); - if (diff) return diff; + if (diff) return full(diff); diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); if (diff) { - log(`diff via fallback: head commit ${sha.slice(0, 8)} (PR #${job.prIndex} diff unavailable)`); - return diff; + log(`diff via fallback: head commit ${sha.slice(0, 8)} (PR #${job.prIndex} diff unavailable) — PARTIAL`); + return part(diff, `head commit only, PR #${job.prIndex} diff unavailable`); } throw allDiffSourcesFailed(tried); } @@ -220,19 +238,25 @@ export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinit if (job.before && !ZERO_SHA.test(job.before)) { // 1. full-range web compare (the normal path) diff = await attempt(`compare ${job.before.slice(0, 8)}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, job.before, sha)); - if (diff) return diff; + if (diff) return full(diff); - // 2. range against the repo default branch (base the server surely has) + // 2. range against the repo default branch (base the server surely has). + // Covers a SUPERSET of the push range (from the merge-base), so complete. let defaultBranch = job.defaultBranch; if (!defaultBranch) { + // Optional lookup: only a 404 (repo gone) is a recordable miss; auth + // and server failures must propagate, not silently skip a source. try { defaultBranch = (await gitea.getRepo(owner, repo))?.default_branch; } - catch (e) { tried.push(e.message); } + catch (e) { + if (e.status !== 404) throw e; + tried.push(e.message); + } } if (defaultBranch) { diff = await attempt(`compare ${defaultBranch}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, defaultBranch, sha)); if (diff) { log(`diff via fallback: compare ${defaultBranch}...${sha.slice(0, 8)}`); - return diff; + return full(diff); } } @@ -249,16 +273,25 @@ export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinit } if (!shas && Array.isArray(job.commitShas) && job.commitShas.length) shas = job.commitShas; if (shas) { + // Webhook/API-supplied list: dedupe, drop anything that isn't a sha, + // and hard-cap the request chain so a hostile or huge payload cannot + // fan out into hundreds of sequential fetches. + const cleaned = [...new Set(shas)].filter((s) => typeof s === 'string' && SHA_RE.test(s)); + const capped = cleaned.slice(0, MAX_COMMIT_DIFFS); const parts = []; - let total = 0; - for (const s of shas) { + let total = 0, hitByteCap = false; + for (const s of capped) { const d = await attempt(`commit ${s.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, s)); if (d) { parts.push(d); total += Buffer.byteLength(d); } - if (total > maxBytes) break; // review.js truncates + caps the verdict + if (total > maxBytes) { hitByteCap = true; break; } // review.js truncates + caps the verdict } if (total > 0) { - log(`diff via fallback: ${parts.length}/${shas.length} per-commit diff(s) over ${job.before.slice(0, 8)}...${sha.slice(0, 8)}`); - return parts.join('\n'); + const incomplete = hitByteCap || parts.length < capped.length || cleaned.length > capped.length; + log(`diff via fallback: ${parts.length}/${cleaned.length} per-commit diff(s) over ${job.before.slice(0, 8)}...${sha.slice(0, 8)}${incomplete ? ' — PARTIAL' : ''}`); + const joined = parts.join('\n'); + return incomplete + ? part(joined, `per-commit reconstruction covered ${parts.length}/${cleaned.length} commit(s)`) + : full(joined); } } @@ -266,21 +299,24 @@ export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinit if (!shas || !shas.includes(sha)) { diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); if (diff) { - log(`diff via fallback: head commit ${sha.slice(0, 8)} only`); - return diff; + log(`diff via fallback: head commit ${sha.slice(0, 8)} only — PARTIAL`); + return part(diff, `head commit only, range ${job.before.slice(0, 8)}...${sha.slice(0, 8)} unrecoverable`); } } throw allDiffSourcesFailed(tried); } - // new-branch push (before = 0000… or missing): head commit diff + // new-branch push (before = 0000… or missing): head commit diff is the + // long-standing contract for this event shape — complete by definition. diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha)); - if (diff) return diff; + if (diff) return full(diff); throw allDiffSourcesFailed(tried); } function allDiffSourcesFailed(tried) { - return new Error(`all diff sources failed: ${tried.join(' | ')}`); + let msg = tried.join(' | '); + if (msg.length > 1500) msg = msg.slice(0, 1500) + ` …(+${tried.length} sources)`; + return new Error(`all diff sources failed: ${msg}`); } async function failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error, state = 'warning', description }) { diff --git a/test/fetchdiff.test.js b/test/fetchdiff.test.js index 5c13307..5b8020e 100644 --- a/test/fetchdiff.test.js +++ b/test/fetchdiff.test.js @@ -18,7 +18,8 @@ const BEFORE = 'b'.repeat(40); test('PR jobs use the PR diff', async () => { const g = giteaStub(); const out = await fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: 5, sha: HEAD, before: null } }); - assert.equal(out, 'PRDIFF'); + assert.equal(out.diff, 'PRDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls, [['pr', 'o', 'r', 5]]); }); @@ -32,7 +33,8 @@ test('push uses ONE compare diff over the full before...after range', async () = commitShas: Array.from({ length: 25 }, (_, i) => String(i).padStart(40, '0')) } }); - assert.equal(out, 'RANGEDIFF'); + assert.equal(out.diff, 'RANGEDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls, [['compare', 'o', 'r', BEFORE, HEAD]]); }); @@ -42,7 +44,8 @@ test('new-branch push (before = zero sha) falls back to head commit diff', async gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: '0'.repeat(40), commitShas: [HEAD] } }); - assert.equal(out, 'HEADDIFF'); + assert.equal(out.diff, 'HEADDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]); }); @@ -52,7 +55,8 @@ test('missing before also falls back to head commit diff', async () => { gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: null, commitShas: [HEAD] } }); - assert.equal(out, 'HEADDIFF'); + assert.equal(out.diff, 'HEADDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]); }); @@ -84,7 +88,8 @@ test('compare 404 falls back to default-branch compare', async () => { gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } }); - assert.equal(out, 'RANGEDIFF'); + assert.equal(out.diff, 'RANGEDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'main', HEAD]); }); @@ -99,7 +104,8 @@ test('compare 404 without defaultBranch on the job looks it up via getRepo', asy gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE } }); - assert.equal(out, 'RANGEDIFF'); + assert.equal(out.diff, 'RANGEDIFF'); + assert.equal(out.partial, false); assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'develop', HEAD]); }); @@ -120,7 +126,8 @@ test('both compares 404 -> API commit list -> concatenated per-commit diffs', as gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } }); - assert.equal(out, 'DIFF(c)\nDIFF(d)'); + assert.equal(out.diff, 'DIFF(c)\nDIFF(d)'); + assert.equal(out.partial, false); assert.deepEqual(g.calls.filter(c => c[0] === 'commit'), [['commit', 'o', 'r', C1], ['commit', 'o', 'r', C2]]); }); @@ -142,7 +149,8 @@ test('API compare 404 too -> per-commit diffs from the webhook commitShas', asyn defaultBranch: 'main', commitShas: [C1, HEAD] } }); - assert.equal(out, 'DIFF(c)\nDIFF(a)'); + assert.equal(out.diff, 'DIFF(c)\nDIFF(a)'); + assert.equal(out.partial, false); }); test('per-commit fallback stops fetching past maxBytes (review truncates after)', async () => { @@ -159,7 +167,9 @@ test('per-commit fallback stops fetching past maxBytes (review truncates after)' }); // first two diffs exceed the cap (20 > 15) -> third commit never fetched assert.equal(fetched.length, 2); - assert.equal(out, 'x'.repeat(10) + '\n' + 'x'.repeat(10)); + assert.equal(out.diff, 'x'.repeat(10) + '\n' + 'x'.repeat(10)); + // byte-capped reconstruction covered 2/3 commits -> partial, verdict capped + assert.equal(out.partial, true); }); test('every source 404 -> throws listing every URL tried (then fail-open)', async () => { @@ -190,7 +200,9 @@ test('PR diff 404 falls back to the head commit diff', async () => { const g = giteaStub(); g.getPrDiff = (o, r, i) => Promise.reject(nf(`/api/v1/repos/o/r/pulls/${i}.diff`)); const out = await fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: 7, sha: HEAD, before: null } }); - assert.equal(out, 'HEADDIFF'); + assert.equal(out.diff, 'HEADDIFF'); + // head-only PR fallback reviews a subset -> partial + assert.equal(out.partial, true); assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]); }); @@ -201,5 +213,36 @@ test('empty compare body is treated as a miss, not a reviewable diff', async () gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' } }); - assert.equal(out, 'RANGEDIFF'); + assert.equal(out.diff, 'RANGEDIFF'); +}); + +test('per-commit fallback dedupes, validates, and caps the sha list at 20', async () => { + const g = giteaStub(); + // 25 unique shas + 1 dup + 1 garbage entry: only the first 20 valid uniques fetch + const shas = Array.from({ length: 25 }, (_, i) => String(i).padStart(40, 'f'.charCodeAt ? '0' : '0')); + const payload = [...shas, shas[0], 'not-a-sha']; + g.getCompareDiff = () => Promise.reject(nf('/o/r/compare/x...y.diff')); + g.compare = () => Promise.reject(nf('/api/v1/repos/o/r/compare/x...y')); + const fetched = []; + g.getCommitDiff = (o, r, sha) => { fetched.push(sha); return Promise.resolve('D'); }; + const out = await fetchDiff({ + gitea: g, + job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main', commitShas: payload } + }); + assert.equal(fetched.length, 20); + assert.equal(out.partial, true); // 20/25 covered +}); + +test('getRepo auth failure propagates instead of being swallowed', async () => { + const g = giteaStub(); + g.getCompareDiff = (o, r, before) => before === BEFORE + ? Promise.reject(nf('/o/r/compare/x...y.diff')) + : Promise.resolve('RANGEDIFF'); + const authErr = new Error('gitea GET /api/v1/repos/o/r -> 401: unauthorized'); + authErr.status = 401; + g.getRepo = () => Promise.reject(authErr); + await assert.rejects( + () => fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE } }), + /401: unauthorized/ + ); });