Skip to content

Commit 189f824

Browse files
authored
fix(review): invalidate linked issue cache on prompt edits (#4114)
1 parent 49f3d67 commit 189f824

4 files changed

Lines changed: 92 additions & 22 deletions

File tree

src/queue/processors.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7751,14 +7751,18 @@ export async function runLinkedIssueSatisfactionForAdvisory(
77517751
model: args.settings.aiReviewModel ?? storedKey.model,
77527752
}
77537753
: null;
7754-
// #linked-issue-satisfaction-cache: the assessment's LLM call is fully deterministic given the same head SHA
7755-
// + linked issue number (no RAG/grounding/enrichment feeds into it), so a repeated scheduled sweep pass at
7756-
// an unchanged head+issue reuses the stored result instead of re-spending up to 6 free-tier attempts (or a
7757-
// BYOK call) on every tick — mirrors ai_slop_cache's confirmed-in-production motivation exactly.
7754+
const diff = buildAiReviewDiff(args.files);
7755+
// #linked-issue-satisfaction-cache: the assessment's LLM call is fully deterministic for the same
7756+
// reviewer configuration and prompt. GitHub issue/PR text can be edited without changing the head SHA, so
7757+
// those prompt fields are part of this fingerprint rather than relying only on the row key.
77587758
const inputFingerprint = await linkedIssueSatisfactionCacheInputFingerprint({
77597759
byok: Boolean(providerKey),
77607760
provider: providerKey?.provider,
77617761
model: providerKey?.model,
7762+
issueText,
7763+
prTitle: args.pr.title,
7764+
prBody: args.pr.body ?? undefined,
7765+
diff,
77627766
});
77637767
const cached = await getCachedLinkedIssueSatisfaction(
77647768
env,
@@ -7798,7 +7802,7 @@ export async function runLinkedIssueSatisfactionForAdvisory(
77987802
issueText,
77997803
prTitle: args.pr.title,
78007804
prBody: args.pr.body ?? undefined,
7801-
diff: buildAiReviewDiff(args.files),
7805+
diff,
78027806
actor: args.author,
78037807
providerKey,
78047808
});
Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,20 @@
11
import { sha256Hex } from "../utils/crypto";
22

3-
// #linked-issue-satisfaction-cache: mirrors ai-slop-cache-input.ts's fingerprint discipline exactly (kept as
4-
// its own small module -- not reused directly -- so the two caches' version strings never collide/alias each
5-
// other in stored rows). The satisfaction assessment's only input that can change independently of the PR's
6-
// head SHA is which provider writes the opinion: the free/default reviewer vs. a maintainer's BYOK key/model
7-
// (see LinkedIssueSatisfactionRunInput in ../services/linked-issue-satisfaction-run). Issue title/body/diff are
8-
// pinned to the head SHA (a fresh commit is what invalidates the cache row itself) and the linked issue number
9-
// is a SEPARATE primary-key column (not folded into this fingerprint) -- see the cache table's migration doc
10-
// for why a changed primary linked issue must miss the cache rather than replay a different issue's verdict.
11-
export const LINKED_ISSUE_SATISFACTION_CACHE_INPUT_VERSION = "linked-issue-satisfaction-input:v1";
3+
// #linked-issue-satisfaction-cache: mirrors ai-slop-cache-input.ts's fingerprint discipline, but includes
4+
// every prompt input that is not already represented by the row key. The row key handles repo/pull/head SHA
5+
// and primary linked issue number; this fingerprint handles reviewer configuration plus mutable GitHub text
6+
// (issue title/body and PR title/body) so edits cannot replay a verdict for an older prompt. Diff text is
7+
// included defensively too, keeping the cache tied to exactly the model prompt that produced the opinion.
8+
export const LINKED_ISSUE_SATISFACTION_CACHE_INPUT_VERSION = "linked-issue-satisfaction-input:v2";
129

1310
export type LinkedIssueSatisfactionCacheInput = {
1411
byok: boolean;
1512
provider: string | null | undefined;
1613
model: string | null | undefined;
14+
issueText?: string | null | undefined;
15+
prTitle?: string | null | undefined;
16+
prBody?: string | null | undefined;
17+
diff?: string | null | undefined;
1718
};
1819

1920
export async function linkedIssueSatisfactionCacheInputFingerprint(input: LinkedIssueSatisfactionCacheInput): Promise<string> {
@@ -22,6 +23,10 @@ export async function linkedIssueSatisfactionCacheInputFingerprint(input: Linked
2223
input.byok ? "1" : "0",
2324
input.provider ?? "",
2425
input.model ?? "",
26+
input.issueText ?? "",
27+
input.prTitle ?? "",
28+
input.prBody ?? "",
29+
input.diff ?? "",
2530
].join("|");
2631
return `${LINKED_ISSUE_SATISFACTION_CACHE_INPUT_VERSION}:${await sha256Hex(payload)}`;
2732
}

test/unit/linked-issue-satisfaction-cache.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,38 @@ describe("linkedIssueSatisfactionCacheInputFingerprint", () => {
122122
expect(withUndefined).toBe(withNull);
123123
});
124124

125+
it("differs when mutable prompt text changes (regression for stale linked-issue gate cache)", async () => {
126+
const before = await linkedIssueSatisfactionCacheInputFingerprint({
127+
byok: false,
128+
provider: null,
129+
model: null,
130+
issueText: "Need an SSE endpoint",
131+
prTitle: "Add SSE endpoint",
132+
prBody: "Closes the SSE issue",
133+
diff: "+app.get('/stream', sse)",
134+
});
135+
const editedIssue = await linkedIssueSatisfactionCacheInputFingerprint({
136+
byok: false,
137+
provider: null,
138+
model: null,
139+
issueText: "Need a GraphQL subscription",
140+
prTitle: "Add SSE endpoint",
141+
prBody: "Closes the SSE issue",
142+
diff: "+app.get('/stream', sse)",
143+
});
144+
const editedPr = await linkedIssueSatisfactionCacheInputFingerprint({
145+
byok: false,
146+
provider: null,
147+
model: null,
148+
issueText: "Need an SSE endpoint",
149+
prTitle: "Add GraphQL subscription",
150+
prBody: "Closes the GraphQL issue",
151+
diff: "+app.get('/stream', sse)",
152+
});
153+
expect(editedIssue).not.toBe(before);
154+
expect(editedPr).not.toBe(before);
155+
});
156+
125157
it("never collides with the ai_slop_cache fingerprint namespace even for identical inputs", async () => {
126158
const { aiSlopCacheInputFingerprint } = await import("../../src/review/ai-slop-cache-input");
127159
const slop = await aiSlopCacheInputFingerprint({ byok: false, provider: null, model: null });

test/unit/linked-issue-satisfaction-run.test.ts

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
22
import { runGittensoryLinkedIssueSatisfaction, type LinkedIssueSatisfactionRunInput } from "../../src/services/linked-issue-satisfaction-run";
3-
import { processJob, runLinkedIssueSatisfactionForAdvisory } from "../../src/queue/processors";
3+
import { buildAiReviewDiff, processJob, runLinkedIssueSatisfactionForAdvisory } from "../../src/queue/processors";
44
import { evaluateGateCheck } from "../../src/rules/advisory";
55
import {
66
getCachedLinkedIssueSatisfaction,
@@ -294,6 +294,9 @@ describe("runLinkedIssueSatisfactionForAdvisory (processor wiring, #1961/#3906)"
294294
{ repoFullName: "acme/widgets", pullNumber: 7, path: "src/a.ts", status: "modified", additions: 40, deletions: 2, changes: 42, payload: { patch: "@@\n+app.get('/stream', sse);" } },
295295
];
296296
const pr = { number: 7, title: "Add SSE stream endpoint", body: "Implements the requested SSE stream.", linkedIssues: [1275] };
297+
const issueText = "Enrich SN74 Gittensor — add SSE stream\n\nWe need a live SSE stream surface for SN74 Gittensor.";
298+
const processorFingerprint = () =>
299+
linkedIssueSatisfactionCacheInputFingerprint({ byok: false, provider: null, model: null, issueText, prTitle: pr.title, prBody: pr.body, diff: buildAiReviewDiff(files) });
297300
const advisoryMode = { linkedIssueSatisfactionGateMode: "advisory", aiReviewByok: false } as RepositorySettings;
298301
const blockMode = { linkedIssueSatisfactionGateMode: "block", aiReviewByok: false } as RepositorySettings;
299302

@@ -532,7 +535,7 @@ describe("runLinkedIssueSatisfactionForAdvisory (processor wiring, #1961/#3906)"
532535
stubIssueFetch();
533536
const run = vi.fn(async () => ({ response: satisfactionJson({ status: "unaddressed", confidence: 0.9 }) }));
534537
const env = enabledEnv(run);
535-
const fingerprint = await linkedIssueSatisfactionCacheInputFingerprint({ byok: false, provider: null, model: null });
538+
const fingerprint = await processorFingerprint();
536539
await putCachedLinkedIssueSatisfaction(env, "acme/widgets", 7, "sha7", 1275, fingerprint, {
537540
status: "ok",
538541
result: { status: "addressed", rationale: "cached: looks done", confidence: 0.8 },
@@ -548,7 +551,7 @@ describe("runLinkedIssueSatisfactionForAdvisory (processor wiring, #1961/#3906)"
548551
stubIssueFetch();
549552
const run = vi.fn();
550553
const env = enabledEnv(run);
551-
const fingerprint = await linkedIssueSatisfactionCacheInputFingerprint({ byok: false, provider: null, model: null });
554+
const fingerprint = await processorFingerprint();
552555
await putCachedLinkedIssueSatisfaction(env, "acme/widgets", 7, "sha7", 1275, fingerprint, {
553556
status: "ok",
554557
result: { status: "addressed", rationale: "cached: looks done", confidence: 0.8 },
@@ -589,7 +592,7 @@ describe("runLinkedIssueSatisfactionForAdvisory (processor wiring, #1961/#3906)"
589592
await runLinkedIssueSatisfactionForAdvisory(env, { settings: advisoryMode, advisory: adv, repoFullName: "acme/widgets", pr, author: "alice", files, confirmedContributor: true, installationId: 1 });
590593
expect(run).toHaveBeenCalledTimes(1);
591594

592-
const fingerprint = await linkedIssueSatisfactionCacheInputFingerprint({ byok: false, provider: null, model: null });
595+
const fingerprint = await processorFingerprint();
593596
const cached = await getCachedLinkedIssueSatisfaction(env, "acme/widgets", 7, "sha7", 1275, fingerprint);
594597
expect(cached).toMatchObject({ status: "ok", result: { status: "addressed" } });
595598

@@ -598,6 +601,27 @@ describe("runLinkedIssueSatisfactionForAdvisory (processor wiring, #1961/#3906)"
598601
expect(run).toHaveBeenCalledTimes(1); // still 1 — second pass was a cache hit
599602
});
600603

604+
it("misses the cache when editable issue or PR text changes at the same head SHA", async () => {
605+
stubIssueFetch({ body: "We need a live SSE stream surface for SN74 Gittensor." });
606+
const run = vi.fn(async () => ({ response: satisfactionJson({ status: "addressed", rationale: "The SSE endpoint satisfies the original ask." }) }));
607+
const env = enabledEnv(run);
608+
const adv = advisory();
609+
await expect(
610+
runLinkedIssueSatisfactionForAdvisory(env, { settings: blockMode, advisory: adv, repoFullName: "acme/widgets", pr, author: "alice", files, confirmedContributor: true, installationId: 1 }),
611+
).resolves.toMatchObject({ status: "addressed" });
612+
613+
stubIssueFetch({ body: "We now need a GraphQL subscription instead of an SSE stream." });
614+
run.mockResolvedValueOnce({ response: satisfactionJson({ status: "unaddressed", confidence: 0.9, rationale: "The changed issue asks for GraphQL, but the diff still adds SSE." }) });
615+
const changedAdvisory = advisory();
616+
const changedPr = { ...pr, title: "Add SSE endpoint for the old issue", body: "Still only implements SSE." };
617+
await expect(
618+
runLinkedIssueSatisfactionForAdvisory(env, { settings: blockMode, advisory: changedAdvisory, repoFullName: "acme/widgets", pr: changedPr, author: "alice", files, confirmedContributor: true, installationId: 1 }),
619+
).resolves.toMatchObject({ status: "unaddressed" });
620+
621+
expect(run).toHaveBeenCalledTimes(2);
622+
expect(changedAdvisory.findings).toContainEqual(expect.objectContaining({ code: "linked_issue_scope_mismatch" }));
623+
});
624+
601625
it("misses the cache when the PR's primary linked issue number changes, even at the same head SHA", async () => {
602626
stubIssueFetch();
603627
const run = vi.fn(async () => ({ response: satisfactionJson({ status: "addressed" }) }));
@@ -785,10 +809,15 @@ describe("linked-issue satisfaction wired end-to-end through the real webhook pi
785809
expect(gatePatchBody.conclusion).toBe("failure");
786810
expect(gatePatchBody.output?.title).toContain("Linked issue does not appear to be satisfied");
787811

788-
// The assessment was cached under the PR's primary linked issue number.
789-
const fingerprint = await linkedIssueSatisfactionCacheInputFingerprint({ byok: false, provider: null, model: null });
790-
const cached = await getCachedLinkedIssueSatisfaction(env, "JSONbored/metagraphed", 3910, "realvenus3910", 1275, fingerprint);
791-
expect(cached).toMatchObject({ status: "ok", result: { status: "unaddressed" } });
812+
// The assessment was cached under the PR's primary linked issue number. The fingerprint itself includes
813+
// prompt text, which this end-to-end test does not need to reconstruct from the webhook pipeline.
814+
const cached = await env.DB.prepare(
815+
"SELECT status, result_json AS resultJson FROM linked_issue_satisfaction_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ? AND linked_issue_number = ?",
816+
)
817+
.bind("JSONbored/metagraphed", 3910, "realvenus3910", 1275)
818+
.first<{ status: string; resultJson: string }>();
819+
expect(cached?.status).toBe("ok");
820+
expect(JSON.parse(cached?.resultJson ?? "{}")?.status).toBe("unaddressed");
792821
});
793822

794823
it("OFF mode (default): no fetch, no model spend, no cache row, and the comment never mentions linked-issue satisfaction at all — byte-identical to before this feature existed", async () => {

0 commit comments

Comments
 (0)