From 6285f1117d1a675498b9ae8dfbd35ec5f10a464f Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:57:49 -0400 Subject: [PATCH 1/4] [Feat] Agents screen their own diff for risky hunks before they ship --- .../tasks/__tests__/getDiffRiskHints.test.ts | 75 +++++++++ .../src/handlers/tasks/getDiffRiskHints.ts | 76 +++++++++ apps/api/src/handlers/tasks/index.ts | 2 + apps/docs/models.mdx | 3 + .../__tests__/diff-risk-hints.test.ts | 104 ++++++++++++ .../mcp/roomote-mcp-server/diff-risk-hints.ts | 148 ++++++++++++++++++ .../src/mcp/roomote-mcp-server/index.ts | 37 +++++ .../roomote-mcp-server/tasks-api-client.ts | 21 +++ .../server/__tests__/diff-risk-hints.test.ts | 79 ++++++++++ .../src/server/diff-risk-hints.ts | 69 ++++++++ packages/cloud-agents/src/server/index.ts | 4 + .../workflows/githubPrReviewPrescreen.ts | 29 ++-- .../resources/default-workflow.md | 2 + 13 files changed, 635 insertions(+), 14 deletions(-) create mode 100644 apps/api/src/handlers/tasks/__tests__/getDiffRiskHints.test.ts create mode 100644 apps/api/src/handlers/tasks/getDiffRiskHints.ts create mode 100644 apps/worker/src/mcp/roomote-mcp-server/__tests__/diff-risk-hints.test.ts create mode 100644 apps/worker/src/mcp/roomote-mcp-server/diff-risk-hints.ts create mode 100644 packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts create mode 100644 packages/cloud-agents/src/server/diff-risk-hints.ts diff --git a/apps/api/src/handlers/tasks/__tests__/getDiffRiskHints.test.ts b/apps/api/src/handlers/tasks/__tests__/getDiffRiskHints.test.ts new file mode 100644 index 0000000000..c1115d6835 --- /dev/null +++ b/apps/api/src/handlers/tasks/__tests__/getDiffRiskHints.test.ts @@ -0,0 +1,75 @@ +import { Hono } from 'hono'; + +import type { Variables } from '../../../types'; +import type { McpAuth } from '../../mcp/middleware'; + +const mocks = vi.hoisted(() => ({ + screenDiffRiskHints: vi.fn(), + rows: vi.fn(), +})); + +vi.mock('@roomote/cloud-agents/server', () => ({ + DIFF_RISK_HINTS_MAX_DIFF_CHARS: 400_000, + screenDiffRiskHints: mocks.screenDiffRiskHints, +})); + +vi.mock('@roomote/db/server', () => ({ + db: { + select: () => ({ + from: () => ({ + innerJoin: () => ({ where: () => ({ limit: mocks.rows }) }), + }), + }), + }, + eq: vi.fn(), + tasks: {}, + taskRuns: {}, +})); + +import { getDiffRiskHints } from '../getDiffRiskHints'; + +function post(body: unknown, authContext: unknown = { runId: 42 }) { + const app = new Hono<{ Variables: Variables & { mcpAuth: McpAuth } }>(); + app.use('*', async (c, next) => { + c.set('mcpAuth', { userId: undefined, authContext: authContext as never }); + await next(); + }); + app.post('/tasks/runs/:runId/diff_risk_hints', getDiffRiskHints); + + return app.request('/tasks/runs/42/diff_risk_hints', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); +} + +describe('getDiffRiskHints', () => { + beforeEach(() => { + mocks.rows.mockReset().mockResolvedValue([{ title: 'Fix admin check' }]); + mocks.screenDiffRiskHints + .mockReset() + .mockResolvedValue({ available: true, hints: [], text: 'none flagged' }); + }); + + it("screens the diff with the run's task title", async () => { + const response = await post({ diff: 'diff --git a/a b/a' }); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toMatchObject({ available: true }); + expect(mocks.screenDiffRiskHints).toHaveBeenCalledWith({ + title: 'Fix admin check', + diff: 'diff --git a/a b/a', + }); + }); + + it('rejects callers without a matching run token', async () => { + expect((await post({ diff: 'x' }, { userId: 'u' })).status).toBe(403); + expect((await post({ diff: 'x' }, { runId: 7 })).status).toBe(403); + expect(mocks.screenDiffRiskHints).not.toHaveBeenCalled(); + }); + + it('rejects an empty or oversized diff', async () => { + expect((await post({ diff: '' })).status).toBe(400); + expect((await post({ diff: 'x'.repeat(400_001) })).status).toBe(400); + }); +}); diff --git a/apps/api/src/handlers/tasks/getDiffRiskHints.ts b/apps/api/src/handlers/tasks/getDiffRiskHints.ts new file mode 100644 index 0000000000..5b6cf9ce28 --- /dev/null +++ b/apps/api/src/handlers/tasks/getDiffRiskHints.ts @@ -0,0 +1,76 @@ +import type { Context } from 'hono'; +import { z } from 'zod'; + +import { + DIFF_RISK_HINTS_MAX_DIFF_CHARS, + screenDiffRiskHints, +} from '@roomote/cloud-agents/server'; +import { db, eq, tasks, taskRuns } from '@roomote/db/server'; + +import type { Variables } from '../../types'; +import type { McpAuth } from '../mcp/middleware'; +import { isRunTokenContext } from '../mcp/proxy-utils'; +import { logHandlerError } from '../utils'; + +const bodySchema = z.object({ + diff: z.string().min(1).max(DIFF_RISK_HINTS_MAX_DIFF_CHARS), +}); + +/** + * Risk hints for a task's own diff: the pull request review pre-screen, run + * before the agent ships. The sandbox sends the diff; the judgment model key + * stays here. + */ +export async function getDiffRiskHints( + c: Context<{ Variables: Variables & { mcpAuth: McpAuth } }>, +): Promise { + const auth = c.get('mcpAuth').authContext; + + if (!isRunTokenContext(auth)) { + return c.json({ error: 'Diff risk hints require a task run token' }, 403); + } + + const runId = Number(c.req.param('runId')); + + if (!Number.isInteger(runId) || runId <= 0) { + return c.json({ error: 'Invalid task run id' }, 400); + } + + if (auth.runId !== runId) { + return c.json( + { error: 'Task run token does not match requested task run' }, + 403, + ); + } + + const parsed = bodySchema.safeParse(await c.req.json().catch(() => null)); + + if (!parsed.success) { + return c.json( + { + error: `Invalid request: send the diff as \`diff\` (at most ${DIFF_RISK_HINTS_MAX_DIFF_CHARS} characters).`, + }, + 400, + ); + } + + try { + const [row] = await db + .select({ title: tasks.title }) + .from(taskRuns) + .innerJoin(tasks, eq(tasks.id, taskRuns.taskId)) + .where(eq(taskRuns.id, runId)) + .limit(1); + + if (!row) { + return c.json({ error: 'Task run not found' }, 404); + } + + return c.json( + await screenDiffRiskHints({ title: row.title, diff: parsed.data.diff }), + ); + } catch (error) { + logHandlerError('getDiffRiskHints', error); + return c.json({ error: 'Failed to screen the diff' }, 500); + } +} diff --git a/apps/api/src/handlers/tasks/index.ts b/apps/api/src/handlers/tasks/index.ts index 8a4d01e022..536a44e93a 100644 --- a/apps/api/src/handlers/tasks/index.ts +++ b/apps/api/src/handlers/tasks/index.ts @@ -19,6 +19,7 @@ import { manageSourceControl } from './manageSourceControl'; import { updateTaskModelSelection } from './updateModelSelection'; import { listTaskModels } from './listModels'; import { saveTaskMemory } from './saveTaskMemory'; +import { getDiffRiskHints } from './getDiffRiskHints'; import { recordAutomationResult } from './recordAutomationResult'; import { updatePersonalization } from './updatePersonalization'; @@ -43,4 +44,5 @@ tasksRouter.post('/:taskId/task_suggestions', submitTaskSuggestions); tasksRouter.post('/:taskId/automation_result', recordAutomationResult); tasksRouter.post('/:taskId/mcp_recommendations', submitMcpRecommendations); tasksRouter.post('/runs/:runId/memory', saveTaskMemory); +tasksRouter.post('/runs/:runId/diff_risk_hints', getDiffRiskHints); tasksRouter.post('/runs/:runId/personalization', updatePersonalization); diff --git a/apps/docs/models.mdx b/apps/docs/models.mdx index 59338d9cf2..0c88dd1ee9 100644 --- a/apps/docs/models.mdx +++ b/apps/docs/models.mdx @@ -355,6 +355,9 @@ When a judgment model is on, Roomote asks it first for: - choosing the forum tag when Roomote opens a thread in a Discord forum channel - whether a settled session or task turn holds something worth saving to [Memory](/memory) that the agent did not record itself +- ranking a task's own changed hunks by defect risk during its self-review, + before it pushes or opens a pull request (the same pre-screen pull request + reviews use), as advisory hints the agent re-reads Roomote acts on the judgment model only when it is confident. When it is unsure, unavailable, or returns an error, Roomote keeps the behavior it has diff --git a/apps/worker/src/mcp/roomote-mcp-server/__tests__/diff-risk-hints.test.ts b/apps/worker/src/mcp/roomote-mcp-server/__tests__/diff-risk-hints.test.ts new file mode 100644 index 0000000000..e4fe13a23c --- /dev/null +++ b/apps/worker/src/mcp/roomote-mcp-server/__tests__/diff-risk-hints.test.ts @@ -0,0 +1,104 @@ +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +const { getDiffRiskHints } = vi.hoisted(() => ({ + getDiffRiskHints: vi.fn(), +})); + +vi.mock('../tasks-api-client', () => ({ getDiffRiskHints })); + +import { collectBranchDiff, handleGetDiffRiskHints } from '../diff-risk-hints'; + +const tempDirs: string[] = []; +const originalEnv = { ...process.env }; + +function git(cwd: string, ...args: string[]) { + execFileSync('git', args, { + cwd, + env: { + ...process.env, + GIT_AUTHOR_NAME: 'T', + GIT_AUTHOR_EMAIL: 't@example.com', + GIT_COMMITTER_NAME: 'T', + GIT_COMMITTER_EMAIL: 't@example.com', + }, + }); +} + +function createCheckout(): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'roomote-risk-hints-')); + tempDirs.push(root); + const origin = path.join(root, 'origin'); + fs.mkdirSync(origin); + git(origin, 'init', '-q', '-b', 'main'); + fs.writeFileSync(path.join(origin, 'app.ts'), 'export const app = 1;\n'); + git(origin, 'add', '-A'); + git(origin, 'commit', '-q', '-m', 'init'); + const checkout = path.join(root, 'checkout'); + git(root, 'clone', '-q', origin, checkout); + return checkout; +} + +afterAll(() => { + for (const dir of tempDirs) fs.rmSync(dir, { recursive: true, force: true }); +}); + +describe('diff risk hints tool', () => { + beforeEach(() => { + getDiffRiskHints.mockReset(); + process.env.ROOMOTE_TASK_RUN_ID = '42'; + process.env.ROOMOTE_CLOUD_TOKEN = 'run-token'; + process.env.ROOMOTE_PLATFORM_API_URL = 'https://platform.example.com'; + }); + + afterEach(() => { + process.env = { ...originalEnv }; + }); + + it('covers committed, uncommitted, and new files against the default branch', async () => { + const repo = createCheckout(); + git(repo, 'checkout', '-q', '-b', 'task'); + fs.writeFileSync(path.join(repo, 'app.ts'), 'export const app = 2;\n'); + git(repo, 'commit', '-q', '-am', 'bump'); + fs.writeFileSync(path.join(repo, 'app.ts'), 'export const app = 3;\n'); + fs.writeFileSync(path.join(repo, 'new.ts'), 'export const added = true;\n'); + + const diff = await collectBranchDiff(repo); + + expect(diff).toContain('-export const app = 1;'); + expect(diff).toContain('+export const app = 3;'); + expect(diff).toContain('+export const added = true;'); + }); + + it('sends the branch diff and returns the hints', async () => { + const repo = createCheckout(); + fs.writeFileSync(path.join(repo, 'app.ts'), 'export const app = 2;\n'); + getDiffRiskHints.mockResolvedValue({ available: true, text: 'hints' }); + + const result = await handleGetDiffRiskHints({ repositoryPath: repo }); + + expect(getDiffRiskHints).toHaveBeenCalledWith( + expect.objectContaining({ token: 'run-token' }), + 42, + expect.stringContaining('+export const app = 2;'), + ); + expect(JSON.parse(result.content[0]?.text ?? '')).toMatchObject({ + success: true, + available: true, + text: 'hints', + }); + }); + + it('does not call the platform when nothing changed', async () => { + const repo = createCheckout(); + + const result = await handleGetDiffRiskHints({ repositoryPath: repo }); + + expect(getDiffRiskHints).not.toHaveBeenCalled(); + expect(JSON.parse(result.content[0]?.text ?? '')).toMatchObject({ + available: false, + }); + }); +}); diff --git a/apps/worker/src/mcp/roomote-mcp-server/diff-risk-hints.ts b/apps/worker/src/mcp/roomote-mcp-server/diff-risk-hints.ts new file mode 100644 index 0000000000..6be9b1e9b0 --- /dev/null +++ b/apps/worker/src/mcp/roomote-mcp-server/diff-risk-hints.ts @@ -0,0 +1,148 @@ +import { execFile } from 'node:child_process'; +import { statSync } from 'node:fs'; +import { join, resolve } from 'node:path'; + +import { getRoomoteConfig } from './config.js'; +import { getDiffRiskHints } from './tasks-api-client.js'; +import { catchError, errorResult, successResult } from './tool-result.js'; +import type { ToolResult } from './types.js'; + +const GIT_TIMEOUT_MS = 20_000; +const GIT_MAX_BUFFER_BYTES = 32 * 1024 * 1024; +/** The server caps the diff it screens; send no more than it accepts. */ +const MAX_DIFF_CHARS = 400_000; +const MAX_UNTRACKED_FILES = 50; +const MAX_UNTRACKED_FILE_BYTES = 200_000; + +function git( + cwd: string, + args: string[], + allowExitCodes: number[] = [], +): Promise { + return new Promise((resolvePromise) => { + execFile( + 'git', + ['-C', cwd, ...args], + { + timeout: GIT_TIMEOUT_MS, + maxBuffer: GIT_MAX_BUFFER_BYTES, + encoding: 'utf8', + }, + (error, stdout) => { + const code = + error && typeof error.code === 'number' ? error.code : null; + resolvePromise( + error && !(code !== null && allowExitCodes.includes(code)) + ? null + : stdout, + ); + }, + ); + }); +} + +/** + * What this branch changes against the default branch, as a pull request + * would show it, plus uncommitted and untracked work so the agent can screen + * before it commits. + */ +export async function collectBranchDiff( + repositoryPath: string, +): Promise { + const base = + ( + await git(repositoryPath, ['merge-base', 'HEAD', 'origin/HEAD']) + )?.trim() || + ( + await git(repositoryPath, ['rev-parse', '--verify', '-q', 'HEAD~1']) + )?.trim(); + + if (!base) { + return null; + } + + const tracked = await git(repositoryPath, ['diff', '--no-color', base]); + + if (tracked === null) { + return null; + } + + const untracked = ( + (await git(repositoryPath, [ + 'ls-files', + '--others', + '--exclude-standard', + '-z', + ])) ?? '' + ) + .split('\0') + .filter((file) => { + if (!file) return false; + try { + const stats = statSync(join(repositoryPath, file)); + return stats.isFile() && stats.size <= MAX_UNTRACKED_FILE_BYTES; + } catch { + return false; + } + }) + .slice(0, MAX_UNTRACKED_FILES); + const untrackedPatches = await Promise.all( + untracked.map((file) => + // `--no-index` exits 1 whenever the files differ, which is always here. + git( + repositoryPath, + ['diff', '--no-color', '--no-index', '--', '/dev/null', file], + [1], + ), + ), + ); + + return [tracked, ...untrackedPatches.filter(Boolean)].join(''); +} + +export async function handleGetDiffRiskHints(input: { + repositoryPath?: string; +}): Promise { + const runId = Number(process.env.ROOMOTE_TASK_RUN_ID); + + if (!Number.isInteger(runId) || runId <= 0) { + return errorResult('ROOMOTE_TASK_RUN_ID environment variable not set'); + } + + const config = getRoomoteConfig(); + + if (!config) { + return errorResult('Roomote platform credentials are not available'); + } + + const repositoryPath = resolve( + input.repositoryPath?.trim() || process.env.ROOMOTE_WORKSPACE_PATH || '.', + ); + + try { + const diff = await collectBranchDiff(repositoryPath); + + if (diff === null) { + return errorResult( + `Could not compute a diff in ${repositoryPath}. Pass repositoryPath pointing at the git checkout you changed.`, + ); + } + + if (!diff.trim()) { + return successResult({ + available: false, + reason: 'This branch has no changes against the default branch.', + }); + } + + const result = await getDiffRiskHints( + config, + runId, + diff.slice(0, MAX_DIFF_CHARS), + ); + + return successResult({ ...result }); + } catch (error) { + return catchError(error); + } +} diff --git a/apps/worker/src/mcp/roomote-mcp-server/index.ts b/apps/worker/src/mcp/roomote-mcp-server/index.ts index 30c82ae62b..4e7515054f 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/index.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/index.ts @@ -95,6 +95,7 @@ import { handleReportPlatformIssue } from './report-platform-issue.js'; import { handleManageSourceControl } from './source-control.js'; import { getArtifactConfig, getRoomoteConfig } from './config.js'; import { handleSaveTaskMemory } from './task-memory.js'; +import { handleGetDiffRiskHints } from './diff-risk-hints.js'; import { handleUpdatePersonalization } from './user-personalization.js'; import { ABOUT_ME_CONTENT } from './about-me.js'; import { INTEGRATION_SETUP_CONTENT } from './integration-setup.js'; @@ -551,6 +552,20 @@ function shouldRegisterEnvVarRequestTool(): boolean { * and setup-mcps mirrors that into this flag — so agents without a Brain * never see a memory tool that cannot work. */ +/** + * Every coding task run can screen its own diff. Pull request reviews are + * excluded: their reviewer already receives the same hints. + */ +function shouldRegisterDiffRiskHintsTool(): boolean { + const taskType = process.env.ROOMOTE_TASK_TYPE?.trim(); + + return ( + Boolean(process.env.ROOMOTE_TASK_RUN_ID?.trim()) && + taskType !== TaskPayloadKind.GithubPrReview && + taskType !== TaskPayloadKind.GithubPrReviewSync + ); +} + function shouldRegisterTaskMemoryTool(): boolean { return process.env.ROOMOTE_BRAIN_AVAILABLE === 'true'; } @@ -1318,6 +1333,28 @@ if (shouldRegisterOnDemandIntegrationTools()) { ); } +if (shouldRegisterDiffRiskHintsTool()) { + roomoteMcpServer.registerTool( + 'get_diff_risk_hints', + { + title: 'Get Diff Risk Hints', + description: + "Before you push or open a pull request, call this once during your self-review. It runs the same fast pre-screen Roomote's pull request review uses on your branch's diff (committed, uncommitted, and new files against the default branch) and returns up to three changed hunks most likely to contain a defect, each with the kind of issue suspected. Re-read those hunks and fix what the code confirms. The hints are questions, not findings, and they miss about half of real defects, so they never clear the rest of the change. Returns available: false when the deployment has no hosted judgment model.", + inputSchema: { + repositoryPath: z + .string() + .trim() + .optional() + .describe( + 'Absolute path of the git checkout you changed. Defaults to the workspace root; pass it when the workspace holds several repositories.', + ), + }, + annotations: { readOnlyHint: true }, + }, + async (input) => handleGetDiffRiskHints(input), + ); +} + if (shouldRegisterTaskMemoryTool()) { roomoteMcpServer.registerTool( 'save_task_memory', diff --git a/apps/worker/src/mcp/roomote-mcp-server/tasks-api-client.ts b/apps/worker/src/mcp/roomote-mcp-server/tasks-api-client.ts index f046abab75..35aaef8eba 100644 --- a/apps/worker/src/mcp/roomote-mcp-server/tasks-api-client.ts +++ b/apps/worker/src/mcp/roomote-mcp-server/tasks-api-client.ts @@ -743,6 +743,27 @@ export async function saveTaskMemory( ); } +type DiffRiskHintsResponse = + | { available: true; text: string } + | { available: false; reason: string }; + +export async function getDiffRiskHints( + config: RoomoteConfig, + runId: number, + diff: string, +): Promise { + return apiFetch( + config, + `/api/mcp/tasks/runs/${runId}/diff_risk_hints`, + { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ diff }), + }, + 'Failed to get diff risk hints', + ); +} + export async function updatePersonalization( config: RoomoteConfig, runId: number, diff --git a/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts new file mode 100644 index 0000000000..6edba3f2f5 --- /dev/null +++ b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts @@ -0,0 +1,79 @@ +const { mockScreenReviewHunks } = vi.hoisted(() => ({ + mockScreenReviewHunks: vi.fn(), +})); + +vi.mock('../workflows/githubPrReviewPrescreen', async (importOriginal) => ({ + ...(await importOriginal< + typeof import('../workflows/githubPrReviewPrescreen') + >()), + screenReviewHunks: mockScreenReviewHunks, +})); + +import { screenDiffRiskHints } from '../diff-risk-hints'; + +const diff = [ + 'diff --git a/src/guard.ts b/src/guard.ts', + '--- a/src/guard.ts', + '+++ b/src/guard.ts', + '@@ -10,3 +10,3 @@ export function allow(user) {', + '- return user.admin;', + '+ return !user.admin;', + 'diff --git a/src/new.ts b/src/new.ts', + '--- /dev/null', + '+++ b/src/new.ts', + '@@ -0,0 +1 @@', + '+export const x = 1;', +].join('\n'); + +describe('screenDiffRiskHints', () => { + beforeEach(() => mockScreenReviewHunks.mockReset()); + + it('screens the diff with the task title and every changed file', async () => { + mockScreenReviewHunks.mockResolvedValue([ + { + file: 'src/guard.ts', + header: '@@ -10,3 +10,3 @@ export function allow(user) {', + startLine: 10, + endLine: 12, + area: 'security', + defectProbability: 0.6, + rank: 1, + screenedHunks: 2, + }, + ]); + + const result = await screenDiffRiskHints({ + title: 'Fix admin check', + diff, + }); + + expect(mockScreenReviewHunks).toHaveBeenCalledWith({ + title: 'Fix admin check', + changedFiles: ['src/guard.ts', 'src/new.ts'], + diff, + }); + expect(result.available).toBe(true); + if (!result.available) return; + expect(result.text).toContain('`src/guard.ts`'); + expect(result.text).toContain('possible security issue'); + expect(result.text).toContain('Each line is a question, not a finding.'); + }); + + it('says nothing was flagged without claiming the change is clear', async () => { + mockScreenReviewHunks.mockResolvedValue([]); + + const result = await screenDiffRiskHints({ diff }); + + expect(result).toMatchObject({ available: true, hints: [] }); + if (!result.available) return; + expect(result.text).toContain('does not clear the change'); + }); + + it('reports unavailable when there is no judgment model or nothing reviewable', async () => { + mockScreenReviewHunks.mockResolvedValue(undefined); + + await expect(screenDiffRiskHints({ diff })).resolves.toMatchObject({ + available: false, + }); + }); +}); diff --git a/packages/cloud-agents/src/server/diff-risk-hints.ts b/packages/cloud-agents/src/server/diff-risk-hints.ts new file mode 100644 index 0000000000..fa8ae491ae --- /dev/null +++ b/packages/cloud-agents/src/server/diff-risk-hints.ts @@ -0,0 +1,69 @@ +import { + formatHunkRange, + REVIEW_PRESCREEN_AREA_LABELS, + screenReviewHunks, + type ReviewPrescreenHint, +} from './workflows/githubPrReviewPrescreen'; + +/** Bounds what a sandbox may send; the pre-screen caps its own input again. */ +export const DIFF_RISK_HINTS_MAX_DIFF_CHARS = 400_000; + +const CHANGED_FILE_PATTERN = /^diff --git a\/(.+?) b\//gmu; + +type DiffRiskHintsResult = + | { available: true; hints: ReviewPrescreenHint[]; text: string } + | { available: false; reason: string }; + +/** + * The pull request review pre-screen, run on an agent's own diff before it + * ships: the changed hunks the decision model ranks most likely to contain a + * defect. Advisory only. Nothing is held or blocked; the agent reads the hints + * as questions during its self-review, and the pull request still gets its + * full review. + */ +export async function screenDiffRiskHints(input: { + title?: string | null; + diff: string; +}): Promise { + const changedFiles = [ + ...new Set( + [...input.diff.matchAll(CHANGED_FILE_PATTERN)].map((m) => m[1]!), + ), + ]; + const hints = await screenReviewHunks({ + title: input.title, + changedFiles, + diff: input.diff, + }); + + if (!hints) { + return { + available: false, + reason: + 'No risk hints: either this deployment has no hosted judgment model or the diff has no reviewable code.', + }; + } + + return { + available: true, + hints, + text: formatDiffRiskHintsForAuthor(hints), + }; +} + +function formatDiffRiskHintsForAuthor( + hints: readonly ReviewPrescreenHint[], +): string { + if (hints.length === 0) { + return 'The pre-screen flagged no hunks. It misses about half of real defects, so this does not clear the change.'; + } + + return [ + 'These changed hunks ranked most likely to contain a defect. Re-read each one before you ship:', + ...hints.map( + (hint) => + `- \`${hint.file}\` ${formatHunkRange(hint)} (\`${hint.header}\`)${hint.area ? `: possible ${REVIEW_PRESCREEN_AREA_LABELS[hint.area]} issue` : ''}.`, + ), + 'Each line is a question, not a finding. Fix what the code confirms, leave the rest. The pre-screen misses about half of real defects, so it does not clear unflagged code.', + ].join('\n'); +} diff --git a/packages/cloud-agents/src/server/index.ts b/packages/cloud-agents/src/server/index.ts index a1f824283b..81d0ec54b3 100644 --- a/packages/cloud-agents/src/server/index.ts +++ b/packages/cloud-agents/src/server/index.ts @@ -37,6 +37,10 @@ export * from './workflows/githubPrReviewComment'; export * from './linked-task-relay'; export * from './llm-task-title'; export { distillTaskRunTurnMemory } from './task-run-memory-distillation'; +export { + DIFF_RISK_HINTS_MAX_DIFF_CHARS, + screenDiffRiskHints, +} from './diff-risk-hints'; export * from './user-personalization'; export * from './mcp-self-setup'; export * from './mcp-tool-client'; diff --git a/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts b/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts index e4910b698a..b9875da8cf 100644 --- a/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts +++ b/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts @@ -64,17 +64,18 @@ const REVIEW_PRESCREEN_AREAS = { 'Performance: unbounded work, N+1 queries, leaks, or hot-path latency regressions.', } as const; -type ReviewPrescreenArea = keyof typeof REVIEW_PRESCREEN_AREAS; - -const REVIEW_PRESCREEN_AREA_LABELS: Record = { - security: 'security', - correctness: 'correctness', - dataIntegrity: 'data-integrity', - concurrency: 'concurrency or lifecycle', - compatibility: 'compatibility', - failureHandling: 'failure-handling', - performance: 'performance', -}; +export type ReviewPrescreenArea = keyof typeof REVIEW_PRESCREEN_AREAS; + +export const REVIEW_PRESCREEN_AREA_LABELS: Record = + { + security: 'security', + correctness: 'correctness', + dataIntegrity: 'data-integrity', + concurrency: 'concurrency or lifecycle', + compatibility: 'compatibility', + failureHandling: 'failure-handling', + performance: 'performance', + }; export type ReviewPrescreenHunk = { file: string; @@ -91,7 +92,7 @@ export type ReviewPrescreenHunk = { text: string; }; -type ReviewPrescreenHint = { +export type ReviewPrescreenHint = { file: string; header: string; startLine: number; @@ -543,7 +544,7 @@ export function collectReviewPrescreenHints( return hints; } -function formatHunkRange({ +export function formatHunkRange({ startLine, endLine, }: Pick): string { @@ -637,7 +638,7 @@ export function buildReviewPrescreenBatches({ * reviewable hunks; throws when any request fails so callers never act on a * partial screen. */ -async function screenReviewHunks({ +export async function screenReviewHunks({ title, changedFiles, diff, diff --git a/packages/cloud-agents/src/server/workflows/skills/standard/implement-changes/resources/default-workflow.md b/packages/cloud-agents/src/server/workflows/skills/standard/implement-changes/resources/default-workflow.md index 32db9a60ab..d62ad01cbd 100644 --- a/packages/cloud-agents/src/server/workflows/skills/standard/implement-changes/resources/default-workflow.md +++ b/packages/cloud-agents/src/server/workflows/skills/standard/implement-changes/resources/default-workflow.md @@ -34,6 +34,8 @@ By default, run a brief self-review over the task diff before branch/push/PR act For the default review, inspect committed changes with `git diff $(git merge-base HEAD origin/HEAD 2>/dev/null || echo "HEAD~1") HEAD`, staged changes with `git diff --cached`, and unstaged changes with `git diff`; include newly added files. Check `git diff --cached --name-status` against intended deliverables before delivery, and unstage unexpected task-staged paths without modifying unrelated work. +When the `get_diff_risk_hints` tool is available, call it once during the self-review, after validation and before any branch, push, or pull request step. Re-read each hunk it returns and fix what the code confirms; treat the rest as answered. It is advisory: it never blocks delivery, and it does not clear hunks it leaves out. + When runtime instructions expose a hidden `judge` subagent and the pre-delivery `capture-visual-proof` step for this shipped change kept screenshots or keyframes, run one focused Task-tool judge pass after the initial self-review. Supply the final shipped diff (branch base through working tree, including untracked files, computed the same way as the snapshot), the proof report verbatim, the path `/tmp/capture-visual-proof/diff-at-start.patch`, and the local paths of every kept screenshot and keyframe so the judge can open them. The judge checks the visual proof only: whether the images show the shipped change and whether source changed after capture began. When the proof step kept no images, do not run the judge. Treat the judge verdict as review input and fix actionable proof or drift gaps it finds. When those fixes change repository files, re-run the `capture-visual-proof` step once for the updated shipped change, replace prior proof evidence, then run one more focused judge pass against the refreshed proof result before delivery. Fix actionable self-review issues too, re-review changes, and explicitly document unresolved gaps. From 5d8ef10f8f5ea70a5f0fd4e1c2547aeae48c4814 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:03:11 -0400 Subject: [PATCH 2/4] Fail open when the risk pre-screen errors --- .../server/__tests__/diff-risk-hints.test.ts | 9 +++++++ .../src/server/diff-risk-hints.ts | 26 +++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts index 6edba3f2f5..005670629f 100644 --- a/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts +++ b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts @@ -69,6 +69,15 @@ describe('screenDiffRiskHints', () => { expect(result.text).toContain('does not clear the change'); }); + it('fails open when the decision model errors', async () => { + mockScreenReviewHunks.mockRejectedValue(new Error('HTTP 529')); + + await expect(screenDiffRiskHints({ diff })).resolves.toMatchObject({ + available: false, + reason: expect.stringContaining('Continue without it'), + }); + }); + it('reports unavailable when there is no judgment model or nothing reviewable', async () => { mockScreenReviewHunks.mockResolvedValue(undefined); diff --git a/packages/cloud-agents/src/server/diff-risk-hints.ts b/packages/cloud-agents/src/server/diff-risk-hints.ts index fa8ae491ae..4292f356b2 100644 --- a/packages/cloud-agents/src/server/diff-risk-hints.ts +++ b/packages/cloud-agents/src/server/diff-risk-hints.ts @@ -30,11 +30,27 @@ export async function screenDiffRiskHints(input: { [...input.diff.matchAll(CHANGED_FILE_PATTERN)].map((m) => m[1]!), ), ]; - const hints = await screenReviewHunks({ - title: input.title, - changedFiles, - diff: input.diff, - }); + let hints: ReviewPrescreenHint[] | undefined; + + try { + hints = await screenReviewHunks({ + title: input.title, + changedFiles, + diff: input.diff, + }); + } catch (error) { + // Advisory: a judgment-model failure must never become a tool error. + console.warn( + `[DiffRiskHints] Pre-screen failed; returning no hints. ${ + error instanceof Error ? error.message : String(error) + }`, + ); + return { + available: false, + reason: + 'The risk pre-screen is unavailable right now. Continue without it.', + }; + } if (!hints) { return { From 40ddcb8a8ed5fdbbd5082efe951aff3168a67e4b Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:06:33 -0400 Subject: [PATCH 3/4] Log only the error type when the risk pre-screen fails --- .../src/server/__tests__/diff-risk-hints.test.ts | 8 +++++++- packages/cloud-agents/src/server/diff-risk-hints.ts | 7 ++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts index 005670629f..e2cf1af376 100644 --- a/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts +++ b/packages/cloud-agents/src/server/__tests__/diff-risk-hints.test.ts @@ -70,12 +70,18 @@ describe('screenDiffRiskHints', () => { }); it('fails open when the decision model errors', async () => { - mockScreenReviewHunks.mockRejectedValue(new Error('HTTP 529')); + mockScreenReviewHunks.mockImplementationOnce(async () => { + throw new Error('HTTP 529 upstream echoed: +const secret = 1'); + }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); await expect(screenDiffRiskHints({ diff })).resolves.toMatchObject({ available: false, reason: expect.stringContaining('Continue without it'), }); + // Upstream errors can echo the submitted diff; they are not logged. + expect(JSON.stringify(warn.mock.calls)).not.toContain('secret'); + warn.mockRestore(); }); it('reports unavailable when there is no judgment model or nothing reviewable', async () => { diff --git a/packages/cloud-agents/src/server/diff-risk-hints.ts b/packages/cloud-agents/src/server/diff-risk-hints.ts index 4292f356b2..1854b3aec7 100644 --- a/packages/cloud-agents/src/server/diff-risk-hints.ts +++ b/packages/cloud-agents/src/server/diff-risk-hints.ts @@ -39,10 +39,11 @@ export async function screenDiffRiskHints(input: { diff: input.diff, }); } catch (error) { - // Advisory: a judgment-model failure must never become a tool error. + // Advisory: a judgment-model failure must never become a tool error. Only + // the error type is logged; upstream messages can echo the submitted diff. console.warn( - `[DiffRiskHints] Pre-screen failed; returning no hints. ${ - error instanceof Error ? error.message : String(error) + `[DiffRiskHints] Pre-screen failed; returning no hints. errorType=${ + error instanceof Error ? error.name : typeof error }`, ); return { From ee6e123af97867ddae1c071d4a8ae33f6d5fc655 Mon Sep 17 00:00:00 2001 From: Matt Rubens <2600+mrubens@users.noreply.github.com> Date: Tue, 22 Sep 2026 17:16:03 -0400 Subject: [PATCH 4/4] Cut diff header paths at the first tab without a backtracking regex --- .../src/server/workflows/githubPrReviewPrescreen.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts b/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts index b9875da8cf..5dcb4dc612 100644 --- a/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts +++ b/packages/cloud-agents/src/server/workflows/githubPrReviewPrescreen.ts @@ -180,7 +180,12 @@ function stripDiffPathPrefix( prefix: 'a/' | 'b/', ): string | undefined { // Git appends a tab after paths that contain spaces in `---`/`+++` headers. - const path = decodeDiffPath(token.replace(/\t.*$/u, '').trim()); + // Cut at the first tab with indexOf: a `/\t.*$/` replace is quadratic on + // a header full of tabs, and the diff here comes from the sandbox. + const tab = token.indexOf('\t'); + const path = decodeDiffPath( + (tab === -1 ? token : token.slice(0, tab)).trim(), + ); return path.startsWith(prefix) ? path.slice(prefix.length) : undefined; }