fix: diff-fetch fallback chain — stop failing open on compare 404
~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 <[email protected]>
This commit is contained in:
Claude
+10
-2
@@ -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}`, {
|
||||
|
||||
+122
-11
@@ -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 }) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user