Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 78 additions & 2 deletions server/lib/modelAbuseGuard.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ export function formatPublicReviewInputPrompt(snapshot) {
].join('\n');
}

/**
* Whether a value is a sha256 hex digest — the shape every fingerprint field
* on this boundary takes (content fingerprints, scan keys, intent
* fingerprints), so a validator cannot drift from what the hashes above emit.
*/
export const isSha256Hex = (value) => typeof value === 'string' && /^[a-f0-9]{64}$/i.test(value);

/**
* Fingerprint the exact public content that crossed the abuse boundary. This
* belongs beside the scanner contract so every caller uses the same identity
Expand All @@ -181,18 +188,86 @@ export function modelAbuseContentFingerprint(kind, identity, content) {
.digest('hex');
}

const isIssueNumber = (value) => Number.isInteger(value) && value > 0 && value <= 1_000_000;

const normalizeIssueNumbers = (value) => Array.isArray(value)
? [...new Set(value.filter((number) => Number.isInteger(number) && number > 0 && number <= 1_000_000))]
? [...new Set(value.filter(isIssueNumber))]
.sort((a, b) => a - b)
.slice(0, 50)
: [];

// The filed issue's own text is what "does this PR do what was asked?" is
// judged against, so it is bounded the way every other public input is: a
// linked-issue list is evidence, never a channel for unbounded contributor
// prose.
export const LINKED_ISSUE_MAX_COUNT = 10;
export const LINKED_ISSUE_TITLE_MAX_CHARS = 300;
export const LINKED_ISSUE_BODY_MAX_CHARS = 8_000;

/**
* The intent evidence for one PR: the open issues it links, reduced to number,
* title, and description. `truncated` is carried per issue so a reviewer can
* tell a complete requirement from a clipped one rather than judging a diff
* against half a sentence.
*/
export function normalizeLinkedIssues(value) {
if (!Array.isArray(value)) return [];
const seen = new Set();
const issues = [];
for (const raw of value) {
if (!raw || typeof raw !== 'object' || Array.isArray(raw)) continue;
const { number } = raw;
if (!isIssueNumber(number) || seen.has(number)) continue;
const title = typeof raw.title === 'string' ? raw.title : '';
const body = typeof raw.body === 'string' ? raw.body : '';
seen.add(number);
issues.push({
number,
title: title.slice(0, LINKED_ISSUE_TITLE_MAX_CHARS),
body: body.slice(0, LINKED_ISSUE_BODY_MAX_CHARS),
truncated: title.length > LINKED_ISSUE_TITLE_MAX_CHARS || body.length > LINKED_ISSUE_BODY_MAX_CHARS,
});
}
return issues.sort((a, b) => a.number - b.number).slice(0, LINKED_ISSUE_MAX_COUNT);
}

/**
* The exact linked-issue text that crosses the model-abuse boundary. Composed
* from the NORMALIZED list so the scanned string, the fingerprint, and the
* envelope a reviewer reads are the same bytes.
*/
export function linkedIssueIntentContent(issues) {
return normalizeLinkedIssues(issues).map((issue) => [
`Linked issue #${issue.number} title:`,
issue.title,
`Linked issue #${issue.number} description:`,
issue.body,
].join('\n\n')).join('\n\n');
}

/**
* Stable identity for a PR's screened intent evidence. A linked issue that is
* closed, retitled, rewritten, or swapped after the gate judged the change
* produces a different value, so an old allowlist cannot survive an intent it
* was never evaluated against. `null` means there is no intent evidence at
* all — never treat that as a match.
*/
export function linkedIssueIntentFingerprint(issues) {
const normalized = normalizeLinkedIssues(issues);
const content = linkedIssueIntentContent(normalized);
if (!content) return null;
return modelAbuseContentFingerprint('linked-issue-intent', {
numbers: normalized.map((issue) => issue.number),
}, content);
}

/**
* The server-established facts the Eligibility Gate may rely on, in one
* validated shape. Unknown is deliberately false: a missing facts object is
* not approval. `maintainerTargeted` is set only by the pr-reviewer preflight
* for a per-PR "Review this PR" request and waives the linked-issue
* prerequisite; it is never inferred from PR text.
* prerequisite; it is never inferred from PR text. `intentFingerprint` pins the
* screened issue text the gate judged the diff against.
*/
export function normalizeEligibilityFacts(value) {
const facts = value && typeof value === 'object' && !Array.isArray(value) ? value : {};
Expand All @@ -202,6 +277,7 @@ export function normalizeEligibilityFacts(value) {
openerAssignedIssueNumbers: normalizeIssueNumbers(facts.openerAssignedIssueNumbers),
issueLookupComplete: facts.issueLookupComplete === true,
maintainerTargeted: facts.maintainerTargeted === true,
intentFingerprint: isSha256Hex(facts.intentFingerprint) ? facts.intentFingerprint.toLowerCase() : null,
};
}

Expand Down
52 changes: 52 additions & 0 deletions server/lib/modelAbuseGuard.test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
import { describe, expect, it } from 'vitest';
import {
LINKED_ISSUE_BODY_MAX_CHARS,
LINKED_ISSUE_MAX_COUNT,
LINKED_ISSUE_TITLE_MAX_CHARS,
MODEL_ABUSE_GUARD_MAX_CHUNKS,
MODEL_ABUSE_GUARD_STAGES,
detectDeterministicModelAbuseSignals,
formatPublicReviewInputPrompt,
hasToolFreeTextCapability,
modelAbuseContentFingerprint,
linkedIssueIntentContent,
linkedIssueIntentFingerprint,
modelAbuseGuardStageReadiness,
normalizeEligibilityFacts,
normalizeLinkedIssues,
normalizeModelAbuseGuardResult,
} from './modelAbuseGuard.js';

Expand Down Expand Up @@ -195,3 +202,48 @@ describe('model-abuse guard contract', () => {
expect(prompt).toContain('"diff":"\\u003e"');
});
});

describe('linked-issue intent evidence', () => {
it('bounds and orders the issue text a reviewer judges a diff against', () => {
const issues = normalizeLinkedIssues([
{ number: 9, title: 'b'.repeat(LINKED_ISSUE_TITLE_MAX_CHARS + 5), body: 'short' },
{ number: 4, title: 'Second', body: 'c'.repeat(LINKED_ISSUE_BODY_MAX_CHARS + 5) },
{ number: 4, title: 'duplicate', body: 'dropped' },
{ number: 0, title: 'invalid', body: '' },
'not an issue',
]);

expect(issues.map((issue) => issue.number)).toEqual([4, 9]);
expect(issues[0].body).toHaveLength(LINKED_ISSUE_BODY_MAX_CHARS);
expect(issues[0].truncated).toBe(true);
expect(issues[1].title).toHaveLength(LINKED_ISSUE_TITLE_MAX_CHARS);
// A clipped requirement must announce itself; a reviewer cannot tell a
// complete ask from half of one otherwise.
expect(issues[1].truncated).toBe(true);
expect(normalizeLinkedIssues(
Array.from({ length: LINKED_ISSUE_MAX_COUNT + 5 }, (_, index) => ({ number: index + 1, title: 't', body: 'b' })),
)).toHaveLength(LINKED_ISSUE_MAX_COUNT);
expect(normalizeLinkedIssues(null)).toEqual([]);
});

it('fingerprints the exact screened text, and reports no evidence as null', () => {
const issues = [{ number: 101, title: 'Crash on empty import', body: 'Importing an empty file throws.' }];
const content = linkedIssueIntentContent(issues);

expect(content).toContain('Linked issue #101 title:');
expect(content).toContain('Importing an empty file throws.');
expect(linkedIssueIntentFingerprint(issues)).toBe(linkedIssueIntentFingerprint([{ ...issues[0], extra: 'ignored' }]));
// A rewritten requirement is a different requirement.
expect(linkedIssueIntentFingerprint([{ ...issues[0], body: 'Something else entirely.' }]))
.not.toBe(linkedIssueIntentFingerprint(issues));
expect(linkedIssueIntentFingerprint([])).toBeNull();
expect(linkedIssueIntentFingerprint(null)).toBeNull();
});

it('keeps an unusable intent fingerprint out of the validated fact set', () => {
expect(normalizeEligibilityFacts({ intentFingerprint: 'A'.repeat(64) }).intentFingerprint)
.toBe('a'.repeat(64));
expect(normalizeEligibilityFacts({ intentFingerprint: 'nope' }).intentFingerprint).toBeNull();
expect(normalizeEligibilityFacts({}).intentFingerprint).toBeNull();
});
});
30 changes: 20 additions & 10 deletions server/services/issueWatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { execGh, ensureForgeReachable } from './github.js';
import { mergePR, resolveForgeForRepo } from './git.js';
import { addNotification, NOTIFICATION_TYPES, PRIORITY_LEVELS } from './notifications.js';
import { normalizeEligibilityFacts, runModelAbuseScan } from './modelAbuseGuard.js';
import { issuePrerequisiteWaived } from '../lib/modelAbuseGuard.js';
import { issuePrerequisiteWaived, linkedIssueIntentFingerprint } from '../lib/modelAbuseGuard.js';

const IN_PROGRESS_LABEL_SPEC = dispatchLabelSpec(IN_PROGRESS_LABEL);

Expand Down Expand Up @@ -555,11 +555,11 @@ async function eligibilityFactsStillCurrent(ctx, pr, target) {
)));
if (issues.some((issue, index) => issue?.number !== expected.linkedIssueNumbers[index])) return false;

const openLinkedIssueNumbers = issues
.filter((issue) => !issue.pull_request && String(issue.state || '').toLowerCase() === 'open')
.map((issue) => issue.number);
const openerAssignedIssueNumbers = issues
.filter((issue) => !issue.pull_request && String(issue.state || '').toLowerCase() === 'open')
const openIssues = issues.filter((issue) => (
!issue.pull_request && String(issue.state || '').toLowerCase() === 'open'
));
const openLinkedIssueNumbers = openIssues.map((issue) => issue.number);
const openerAssignedIssueNumbers = openIssues
.filter((issue) => Array.isArray(issue.assignees) && issue.assignees.some((assignee) => (
sameLogin(assignee?.login, authorLogin)
)))
Expand All @@ -570,10 +570,20 @@ async function eligibilityFactsStillCurrent(ctx, pr, target) {
openerAssignedIssueNumbers,
issueLookupComplete: true,
});
return sameNumberList(expected.linkedIssueNumbers, actual.linkedIssueNumbers)
&& sameNumberList(expected.openLinkedIssueNumbers, actual.openLinkedIssueNumbers)
&& sameNumberList(expected.openerAssignedIssueNumbers, actual.openerAssignedIssueNumbers)
&& expected.issueLookupComplete === actual.issueLookupComplete;
if (!sameNumberList(expected.linkedIssueNumbers, actual.linkedIssueNumbers)
|| !sameNumberList(expected.openLinkedIssueNumbers, actual.openLinkedIssueNumbers)
|| !sameNumberList(expected.openerAssignedIssueNumbers, actual.openerAssignedIssueNumbers)
|| expected.issueLookupComplete !== actual.issueLookupComplete) return false;

// The gate judged the diff against the issue text as it read at scan time. A
// requirement that was rewritten since is a different requirement, so the old
// verdict no longer covers it. Facts recorded before intent was screened
// carry no fingerprint and keep their previous meaning.
if (!expected.intentFingerprint) return true;
const currentIntent = linkedIssueIntentFingerprint(openIssues.map((issue) => ({
number: issue.number, title: issue.title, body: issue.body,
})));
return currentIntent === expected.intentFingerprint;
}

async function readBehindBy(ctx, pr) {
Expand Down
63 changes: 63 additions & 0 deletions server/services/issueWatcher.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import {
MAX_PENDING_APPROVAL_TICKS,
MAX_PENDING_ISSUE_COMMENT_TICKS,
} from './issueWatcher.js';
import { linkedIssueIntentFingerprint } from '../lib/modelAbuseGuard.js';
import { MAX_PR_REMEDIATION_ATTEMPTS } from '../lib/prHandbackPolicy.js';
import { IN_PROGRESS_LABEL, dispatchLabelSpec } from '../lib/dispatchLabels.js';

Expand Down Expand Up @@ -601,6 +602,68 @@ describe('processTaskOutput', () => {
))).toBe(true);
});

// The gate approved this diff against the issue as it read at scan time. If
// that requirement was rewritten since, the review answered a question nobody
// is asking any more — so the approval is discarded rather than merged.
it('discards an approval whose linked issue was rewritten after the gate judged it', async () => {
const issue = {
number: 101,
state: 'open',
title: 'Crash on empty import',
body: 'Rewritten after the gate ran.',
assignees: [{ login: 'contributor' }],
};
installDefaultGhMock({ issueDetails: { 101: issue } });
mergePrMock.mockResolvedValue({ success: true });
const staleIntentMetadata = {
issueWatcher: {
...eligibilityMetadata.issueWatcher,
pullRequests: [{
...eligibilityMetadata.issueWatcher.pullRequests[0],
eligibilityFacts: {
...eligibilityFacts,
intentFingerprint: linkedIssueIntentFingerprint([{
number: 101, title: 'Crash on empty import', body: 'Importing an empty file throws.',
}]),
},
}],
},
};
const payload = {
issueComments: [],
pullRequests: [{
number: 7, headSha: 'a'.repeat(40), verdict: 'approve', summary: 'No material issues found.', findings: [],
rebaseRequired: false, ciPolicy: 'required',
}],
};

const result = await processTaskOutput({
appId: APP.id,
success: true,
payload,
task: { metadata: staleIntentMetadata },
requireEligibilityFacts: true,
});

expect(result).toMatchObject({ reviewed: 0, merged: 0 });
expect(mergePrMock).not.toHaveBeenCalled();

// The same approval against the issue text it was actually judged against
// still lands, so the check is the rewrite and not the recheck itself.
execGhMock.mockClear();
mergePrMock.mockClear();
staleIntentMetadata.issueWatcher.pullRequests[0].eligibilityFacts.intentFingerprint =
linkedIssueIntentFingerprint([{ number: 101, title: issue.title, body: issue.body }]);
const current = await processTaskOutput({
appId: APP.id,
success: true,
payload,
task: { metadata: staleIntentMetadata },
requireEligibilityFacts: true,
});
expect(current).toMatchObject({ reviewed: 1, merged: 1 });
});

it('posts non-blocking findings on an approving review and still merges', async () => {
installDefaultGhMock();
mergePrMock.mockResolvedValue({ success: true });
Expand Down
12 changes: 9 additions & 3 deletions server/services/modelAbuseGuard.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ import {
MODEL_ABUSE_GUARD_TIMEOUT_MS,
detectDeterministicModelAbuseSignals,
hasToolFreeTextCapability,
isSha256Hex,
modelAbuseGuardStageReadiness,
normalizeEligibilityFacts,
normalizeLinkedIssues,
normalizeModelAbuseGuardResult,
} from '../lib/modelAbuseGuard.js';
import { findCachedRepoFiles } from '../lib/hfCache.js';
Expand Down Expand Up @@ -80,12 +82,11 @@ export const DETERMINISTIC_ONLY_GUARD_MODEL = 'Deterministic hidden-content chec
const publicReviewModelFailure = (code) => ({ ok: false, code });

const isSha = (value) => typeof value === 'string' && /^[a-f0-9]{40}$/i.test(value);
const isScanKey = (value) => typeof value === 'string' && /^[a-f0-9]{64}$/i.test(value);
const publicReviewInputPath = (scanKey) => isScanKey(scanKey)
const publicReviewInputPath = (scanKey) => isSha256Hex(scanKey)
? join(PUBLIC_REVIEW_INPUT_DIR, `${scanKey}.json`)
: null;

export { normalizeEligibilityFacts };
export { normalizeEligibilityFacts, normalizeLinkedIssues };

export function normalizePublicReviewInput(input) {
if (!input || typeof input !== 'object' || Array.isArray(input)) return null;
Expand All @@ -105,6 +106,10 @@ export function normalizePublicReviewInput(input) {
additions: Number.isInteger(input.additions) ? input.additions : 0,
deletions: Number.isInteger(input.deletions) ? input.deletions : 0,
eligibilityFacts: normalizeEligibilityFacts(input.eligibilityFacts),
// The filed issue's own words. Carried through the same validation
// boundary as the diff so a reviewer can check the change against the
// requirement instead of inferring intent from the PR's own description.
linkedIssues: normalizeLinkedIssues(input.linkedIssues),
diff: input.diff,
};
}
Expand All @@ -115,6 +120,7 @@ function normalizePublicReviewInputs(pullRequests) {
if (normalized.some((item) => !item)) return null;
const contentChars = normalized.reduce((total, item) => (
total + item.title.length + item.body.length + item.diff.length
+ item.linkedIssues.reduce((chars, issue) => chars + issue.title.length + issue.body.length, 0)
), 0);
return contentChars <= MAX_PUBLIC_REVIEW_SNAPSHOT_CHARS ? normalized : null;
}
Expand Down
14 changes: 11 additions & 3 deletions server/services/prReviewerPipeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* response, and eligibility reasons can never cross into the action stage.
*/

import { MODEL_ABUSE_GUARD_ID, issuePrerequisiteWaived, normalizeEligibilityFacts } from '../lib/modelAbuseGuard.js';
import { MODEL_ABUSE_GUARD_ID, isSha256Hex, issuePrerequisiteWaived, normalizeEligibilityFacts } from '../lib/modelAbuseGuard.js';
import { PUBLIC_REVIEW_ACTIONS_EXECUTION_PROFILE, PUBLIC_REVIEW_GATE_EXECUTION_PROFILE } from '../lib/agentExecutionProfiles.js';
import { createPrReviewerDefaultStages } from './taskScheduleRegistry.js';
import {
Expand All @@ -18,7 +18,6 @@ import {
} from './issueWatcher.js';

const HEAD_SHA_RE = /^[a-f0-9]{40}$/i;
const CONTENT_FINGERPRINT_RE = /^[a-f0-9]{64}$/i;
const MAX_REASON_CHARS = 2_000;

const roleForPromptKey = (promptKey) => ({
Expand Down Expand Up @@ -87,7 +86,7 @@ function normalizedExpectedPullRequests(task) {
const pullRequests = [];
for (const item of expected.pullRequests) {
if (!Number.isInteger(item?.number) || item.number < 1 || seen.has(item.number)) return null;
if (!HEAD_SHA_RE.test(item.headSha) || !CONTENT_FINGERPRINT_RE.test(item.contentFingerprint)) return null;
if (!HEAD_SHA_RE.test(item.headSha) || !isSha256Hex(item.contentFingerprint)) return null;
if (typeof item.authorLogin !== 'string' || !item.authorLogin.trim()) return null;
seen.add(item.number);
pullRequests.push({
Expand All @@ -106,6 +105,15 @@ function eligibilityFactsAllow(facts) {
// The model's own quality verdict still applies to a waived PR.
if (issuePrerequisiteWaived(facts)) return true;
if (!facts.issueLookupComplete) return false;
// "Related to a filed issue" is only half the bar: the gate also has to have
// judged the diff against what that issue actually asks for. No screened
// issue text reached it, no intent verdict is possible, so the answer is no —
// a model that answered `eligible` without the requirement in front of it
// guessed. A fact set the current preflight built always carries this
// whenever the open/assigned check below passes, so in practice this rejects
// a set persisted before intent screening existed, or one that never came
// from the preflight at all.
if (!facts.intentFingerprint) return false;
const linked = new Set(facts.linkedIssueNumbers);
const open = new Set(facts.openLinkedIssueNumbers);
return facts.openerAssignedIssueNumbers.some((number) => linked.has(number) && open.has(number));
Expand Down
Loading