fix: dual-review round-2 — partial diffs cap the verdict, bounded fallback chain, URL-safe paths

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 (<reason>)'.
- 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 <[email protected]>

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.

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
+  // A truncated OR partial diff can never yield a clean 'pass' — part of the
-  // not reviewed. Cap at 'warn' (a confirmed-critical 'fail' still fails).
+  // change was not reviewed. Cap at 'warn' (a confirmed-critical 'fail' still
-  applyTruncationCap(review, truncated);
+  // 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
+  const partialReasons = [
-    ? `partial review (diff truncated) — ${descByVerdict[review.verdict]}`
+    ...(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)`);
+      log(`diff via fallback: head commit ${sha.slice(0, 8)} (PR #${job.prIndex} diff unavailable) — PARTIAL`);
-      return diff;
+      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;
+      let total = 0, hitByteCap = false;
-      for (const s of shas) {
+      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)}`);
+        const incomplete = hitByteCap || parts.length < capped.length || cleaned.length > capped.length;
-        return parts.join('\n');
+        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`);
+        log(`diff via fallback: head commit ${sha.slice(0, 8)} only — PARTIAL`);
-        return diff;
+        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 }) {

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/
+  );
 });