From 1b2e0c8ffacab5645544ef6ac205881a61cd5579 Mon Sep 17 00:00:00 2001 From: Brent Vatne Date: Sat, 8 Aug 2026 16:07:18 -0700 Subject: [PATCH] Let routed CI cache researched reviews too, from one shared policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #71 removed the offline index and dropped the research gate from `cacheAllowed` — but only from runLegacyCi. runRoutedCi, which handles every repo that has adopted routing.jsonc, kept `!rootConfig.research.enabled` and so has been running a fresh review for every researched PR, for a reason that stopped existing when the index did. Both gates were added by the same commit (#61) for the same index reason, so this was a missed half, not a separate constraint. Caught by the reviewer on the roadmap PR (#72), which claimed the fix had already landed. Rather than qualify that claim, make it true. The duplication is the actual defect: one policy written out twice will drift, and did. `reviewCacheAllowed` in review-cache.ts is now the single definition and both paths call it, with a unit test pinning each flag and recording that research is deliberately not among them. --- src/__tests__/review-cache.test.ts | 29 ++++++++++++++++++++++++++- src/commands/ci.ts | 32 +++++++++++++++++------------- src/core/review-cache.ts | 26 ++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 15 deletions(-) diff --git a/src/__tests__/review-cache.test.ts b/src/__tests__/review-cache.test.ts index 43efa83..1197496 100644 --- a/src/__tests__/review-cache.test.ts +++ b/src/__tests__/review-cache.test.ts @@ -4,7 +4,12 @@ import { tmpdir } from "node:os"; import path from "node:path"; import type { LoadedConfig } from "../config/schema.js"; -import { reviewCanBeReused, reviewInputHash, reviewMatchesInput } from "../core/review-cache.js"; +import { + reviewCacheAllowed, + reviewCanBeReused, + reviewInputHash, + reviewMatchesInput, +} from "../core/review-cache.js"; import type { CoordinatorOutput, DiffEntry } from "../core/schema.js"; const config = (over: Partial = {}): LoadedConfig => ({ @@ -171,3 +176,25 @@ test("file hashing refuses traversal and never follows a PR-controlled symlink", await rm(outside, { force: true }); } }); + +test("reviewCacheAllowed is the one policy both CI paths share", () => { + const base = { + bypassTriggerGate: false, + stack: false, + feedback: false, + hasMetadata: true, + }; + expect(reviewCacheAllowed(base)).toBe(true); + + // Each flag independently means "this run has an input the key can't represent". + expect(reviewCacheAllowed({ ...base, bypassTriggerGate: true })).toBe(false); + expect(reviewCacheAllowed({ ...base, stack: true })).toBe(false); + expect(reviewCacheAllowed({ ...base, feedback: true })).toBe(false); + expect(reviewCacheAllowed({ ...base, hasMetadata: false })).toBe(false); + + // Research is deliberately NOT a gate. It was one only because the offline index + // was an artifact the key could not represent; with the index gone, a researched + // review fetches everything live during the run. The routed path kept this gate + // after the legacy path dropped it, which is the drift this function prevents. + expect(reviewCacheAllowed(base)).toBe(true); +}); diff --git a/src/commands/ci.ts b/src/commands/ci.ts index 894c274..a3d1abf 100644 --- a/src/commands/ci.ts +++ b/src/commands/ci.ts @@ -30,7 +30,12 @@ import { summarizePriorReview } from "../core/prior-review.js"; import { dropStaleVerdict, feedbackApplied, feedbackNeedsRunSeam } from "../core/adjudicate.js"; import { runReview } from "../core/review.js"; import type { ReviewRunOptions, ReviewRunResult } from "../core/review.js"; -import { reviewCanBeReused, reviewInputHash, reviewMatchesInput } from "../core/review-cache.js"; +import { + reviewCacheAllowed, + reviewCanBeReused, + reviewInputHash, + reviewMatchesInput, +} from "../core/review-cache.js"; import { GitHubPRSource } from "../sources/github-pr.js"; import { memoizeSource, stackConfirmFromConfig, stackWalkFromConfig } from "../sources/source.js"; import type { PreparedReadRoot, ReviewSource, StackWalkOptions } from "../sources/source.js"; @@ -576,13 +581,12 @@ async function runLegacyCi( const stack = resolveStackWalk(config.stack, noStackAware); const stackConfirm = resolveStackConfirm(config.stack, noStackAware); const feedback = adjudicationSeam(config, reporter); - // Dynamic stack context and model-backed reply adjudication have inputs outside - // the scoped diff. Keep those paths fresh until their inputs join the cache key. - // A maintainer's explicit /review is also always a real rerun. - // Research no longer forces a fresh run: with the local index gone, every result - // is fetched live from an allowlisted host during the run, so there is no mounted - // artifact whose contents could drift out from under the cache key. - const cacheAllowed = !bypassTriggerGate && !stack && !feedback && metadata !== undefined; + const cacheAllowed = reviewCacheAllowed({ + bypassTriggerGate, + stack: Boolean(stack), + feedback: Boolean(feedback), + hasMetadata: metadata !== undefined, + }); let inputHash: string | undefined; // The previous review's embedded comment state, read ONCE: the cache check below @@ -905,12 +909,12 @@ async function runRoutedCi( reporterFor(scopedCommentTag(rootTag, name), true), ); - const cacheAllowed = - !bypassTriggerGate && - !stackWalk && - !feedbackNeedsRunSeam(rootConfig.feedback) && - !rootConfig.research.enabled && - metadata !== undefined; + const cacheAllowed = reviewCacheAllowed({ + bypassTriggerGate, + stack: Boolean(stackWalk), + feedback: feedbackNeedsRunSeam(rootConfig.feedback), + hasMetadata: metadata !== undefined, + }); let cacheReadRoot: string | undefined; if (cacheAllowed) { try { diff --git a/src/core/review-cache.ts b/src/core/review-cache.ts index 0c93c15..aefa9e6 100644 --- a/src/core/review-cache.ts +++ b/src/core/review-cache.ts @@ -137,6 +137,32 @@ export async function reviewInputHash(options: ReviewInputHashOptions): Promise< return createHash("sha256").update(canonicalJson(input)).digest("hex"); } +/** + * Whether a run may reuse a cached result at all — the single definition of that + * policy, shared by the legacy and routed CI paths. + * + * It lives here because it was previously written out twice, once per path, and + * the copies drifted: when the offline index was removed, only the legacy copy + * dropped its research gate, so every routed repo silently kept running fresh + * reviews for a reason that no longer existed. Two expressions of one policy is + * the bug; one function that both paths call is the fix. + * + * Each flag means "this run has an input the cache key does not represent": + * dynamic stack context and model-backed reply adjudication both reach outside + * the scoped diff, and a maintainer's explicit /review is always a real rerun. + */ +export function reviewCacheAllowed(run: { + bypassTriggerGate: boolean; + /** Stack walking is on for this run. */ + stack: boolean; + /** A feedback seam runs this pass (adjudication), not merely annotation. */ + feedback: boolean; + /** PR metadata resolved; without it there is nothing stable to key on. */ + hasMetadata: boolean; +}): boolean { + return !run.bypassTriggerGate && !run.stack && !run.feedback && run.hasMetadata; +} + /** Partial/failed reviews must be retried, never made durable by a cache hit. */ export function reviewCanBeReused(review: CoordinatorOutput): boolean { return review.couldNotComplete !== true && review.incomplete.length === 0;