diff --git a/packages/engine/src/core.test.ts b/packages/engine/src/core.test.ts index 1e1f452..14be0a8 100644 --- a/packages/engine/src/core.test.ts +++ b/packages/engine/src/core.test.ts @@ -83,6 +83,30 @@ describe('filterFiles', () => { expect(filterFiles(files, ['!src/**']).included).toHaveLength(0); expect(filterFiles(files, ['bun.lock']).included.map((f) => f.path)).toEqual(['bun.lock']); }); + test('marks only removals the review would cover as deleted', () => { + const removals = parseUnifiedDiff(`diff --git a/.github/dependabot.yml b/.github/dependabot.yml +deleted file mode 100644 +--- a/.github/dependabot.yml ++++ /dev/null +@@ -1,2 +0,0 @@ +-version: 2 +-updates: [] +diff --git a/bun.lock b/bun.lock +deleted file mode 100644 +--- a/bun.lock ++++ /dev/null +@@ -1 +0,0 @@ +-{} +`); + expect(filterFiles(removals).excluded).toEqual([ + { path: '.github/dependabot.yml', reason: 'deleted' }, + { path: 'bun.lock', reason: 'ignored by default' } + ]); + expect(filterFiles(removals, ['!.github/**']).excluded[0]).toEqual({ + path: '.github/dependabot.yml', + reason: 'path filter' + }); + }); }); describe('placeFinding', () => { @@ -575,6 +599,112 @@ describe('runReview', () => { expect(result).toEqual({ status: 'skipped', reason: 'No reviewable changes' }); }); + describe('a pull request that only deletes files', () => { + const removal = `diff --git a/.github/dependabot.yml b/.github/dependabot.yml +deleted file mode 100644 +--- a/.github/dependabot.yml ++++ /dev/null +@@ -1,3 +0,0 @@ +-version: 2 +-updates: +- - package-ecosystem: npm +`; + const review = ( + findings: unknown[], + options: { config?: string; maxDiffChars?: number } = {} + ) => { + const model = new MockLanguageModelV4({ + doGenerate: [toolCall('submit_review', { summary: 'Removes Dependabot.', findings })] + }); + return { + model, + result: runReview({ + repoDir, + diff: removal, + pullRequest: { title: 'Remove Dependabot', body: '', author: 'octocat' }, + config: parseRepoConfig(options.config ?? '').config, + models: { review: { model, provider: 'mock', modelId: 'mock-1' } }, + ...(options.maxDiffChars && { limits: { maxDiffChars: options.maxDiffChars } }) + }) + }; + }; + + test('is reviewed with the removed lines in the prompt, and approved when nothing breaks', async () => { + const { model, result } = review([]); + const completed = await result; + if (completed.status !== 'completed') throw new Error('expected a completed review'); + expect(completed.verdict).toBe('approve'); + const prompt = JSON.stringify(model.doGenerateCalls[0]?.prompt); + expect(prompt).toContain('This pull request only deletes files.'); + expect(prompt).toContain('package-ecosystem: npm'); + }); + + test('is not approved when the reviewer finds something that still depends on it', async () => { + const { result } = review([ + { + path: '.github/workflows/dependabot-automerge.yml', + startLine: 4, + endLine: 4, + severity: 'major', + category: 'bug', + title: 'Auto-merge workflow still expects Dependabot', + body: 'The workflow only runs for Dependabot pull requests.' + } + ]); + const completed = await result; + if (completed.status !== 'completed') throw new Error('expected a completed review'); + expect(completed.posted).toEqual([]); + expect(completed.dropped.map((f) => f.title)).toEqual([ + 'Auto-merge workflow still expects Dependabot' + ]); + expect(completed.verdict).toBe('comment'); + expect(completed.approvalWithheld).toBe( + 'Hansi found problems with this deletion that it could not comment on inline, so it did not approve.' + ); + }); + + test('ignores unplaced findings below minSeverity', async () => { + const { result } = review( + [ + { + path: '.github/workflows/ci.yml', + startLine: 1, + endLine: 1, + severity: 'minor', + category: 'bug', + title: 'Stale comment mentions Dependabot', + body: 'b' + } + ], + { config: '{ "reviews": { "minSeverity": "major" } }' } + ); + const completed = await result; + if (completed.status !== 'completed') throw new Error('expected a completed review'); + expect(completed.verdict).toBe('approve'); + }); + + test('applies path instructions for removed files', async () => { + const { model, result } = review([], { + config: + '{ "pathInstructions": [{ "path": ".github/**", "instructions": "Check the release workflow." }] }' + }); + await result; + const prompt = JSON.stringify(model.doGenerateCalls[0]?.prompt); + expect(prompt).toContain('Check the release workflow.'); + }); + + test('does not approve when the removed lines did not fit in the prompt', async () => { + const { model, result } = review([], { maxDiffChars: 10 }); + const completed = await result; + if (completed.status !== 'completed') throw new Error('expected a completed review'); + expect(completed.verdict).toBe('comment'); + expect(completed.approvalWithheld).toBe( + 'Part of the diff was too large to review, so Hansi did not approve.' + ); + expect(JSON.stringify(model.doGenerateCalls[0]?.prompt)).not.toContain('package-ecosystem'); + }); + }); + test('reviews a lockfile-only bump when a sandbox can run it, and skips it otherwise', async () => { const composerBump = `diff --git a/composer.lock b/composer.lock --- a/composer.lock diff --git a/packages/engine/src/filters.ts b/packages/engine/src/filters.ts index 7d3973a..e752f91 100644 --- a/packages/engine/src/filters.ts +++ b/packages/engine/src/filters.ts @@ -92,25 +92,26 @@ export function filterFiles( const result: FilterResult = { included: [], excluded: [] }; for (const file of files) { - const reason = exclusionReason(file); - if (reason) { - result.excluded.push({ path: file.path, reason }); - } else if (excludes.some((glob) => glob.match(file.path))) { - result.excluded.push({ path: file.path, reason: 'path filter' }); - } else if (includes.length > 0) { - if (includes.some((glob) => glob.match(file.path))) result.included.push(file); - else result.excluded.push({ path: file.path, reason: 'not in path filters' }); - } else if (defaults.some((glob) => glob.match(file.path))) { - result.excluded.push({ path: file.path, reason: 'ignored by default' }); - } else { - result.included.push(file); - } + const reason = filterReason(file) ?? contentReason(file); + if (reason) result.excluded.push({ path: file.path, reason }); + else result.included.push(file); } return result; - function exclusionReason(file: FileDiff) { - if (file.status === 'deleted') return 'deleted'; + // Filters come before content, so `deleted` marks only a removal the review would otherwise + // cover: deleting a lockfile or an ignored path is not a reason to review. + function filterReason(file: FileDiff) { if (file.binary) return 'binary'; + if (excludes.some((glob) => glob.match(file.path))) return 'path filter'; + if (includes.length > 0) { + return includes.some((glob) => glob.match(file.path)) ? null : 'not in path filters'; + } + if (defaults.some((glob) => glob.match(file.path))) return 'ignored by default'; + return null; + } + + function contentReason(file: FileDiff) { + if (file.status === 'deleted') return 'deleted'; if (file.hunks.length === 0) return 'no content changes'; return null; } diff --git a/packages/engine/src/prompts.ts b/packages/engine/src/prompts.ts index 886c292..223fc0a 100644 --- a/packages/engine/src/prompts.ts +++ b/packages/engine/src/prompts.ts @@ -290,6 +290,8 @@ export function buildReviewPrompt(input: { pathInstructions: string[]; diff: string; excludedFiles: { path: string; reason: string }[]; + /** The pull request only deletes files; the diff shows their removed lines. */ + deletionOnly?: boolean; /** Versions the changed lockfiles move. Only set when lockfiles are part of the review. */ dependencyChanges?: DependencyChange[]; /** Changed snapshots and generated code. Only set when the reviewer can run commands. */ @@ -390,6 +392,11 @@ Do not review them line by line. Check them by running something with run_comman `This is an incremental review. The diff below only contains commits pushed since ${input.incrementalFrom.slice(0, 7)}, which was already reviewed. Use the tools to see the rest of the pull request when needed, but only report problems in the new changes.` ); } + if (input.deletionOnly) { + parts.push( + 'This pull request only deletes files. Removed lines cannot carry a comment, so review what the removal breaks: use grep and git to find code, configuration, workflows, scripts and docs that still reference what was deleted, and report each problem on the file and line that still depends on it.' + ); + } if (!input.diff) { parts.push( 'None of the changed files fit in this prompt; they are listed in . Read them with the tools.' diff --git a/packages/engine/src/review.ts b/packages/engine/src/review.ts index 0d34d0a..aead7da 100644 --- a/packages/engine/src/review.ts +++ b/packages/engine/src/review.ts @@ -251,7 +251,14 @@ export async function runReview(input: ReviewInput): Promise { excluded.push({ path: file.path, reason: 'not part of the pull request diff' }); } emit({ type: 'files.filtered', data: { included: included.map((f) => f.path), excluded } }); - if (included.length === 0) { + // A pull request that only deletes files is still reviewed: what matters is whether anything + // still depends on what it removes. Removed lines cannot carry a comment, so they are shown to + // the reviewer but not used for placement. + const removed = changed.filter((file) => + excluded.some((entry) => entry.path === file.path && entry.reason === 'deleted') + ); + const deletionOnly = included.length === 0 && removed.length > 0; + if (included.length === 0 && !deletionOnly) { const reason = input.incrementalFrom ? 'No new reviewable changes since the last review' : 'No reviewable changes'; @@ -278,8 +285,16 @@ export async function runReview(input: ReviewInput): Promise { budget -= rendered.length; shown.push(file); } - // With a sandbox the reviewer can still read the files and run the build and tests. - if (shown.length === 0 && !input.sandbox) { + const removedShown: FileDiff[] = []; + for (const file of deletionOnly ? removed : []) { + const rendered = renderFileDiff(file); + if (rendered.length > budget) continue; + budget -= rendered.length; + removedShown.push(file); + } + // With a sandbox the reviewer can still read the files and run the build and tests. A + // deletion is reviewed by searching for what still uses it, which needs no diff. + if (shown.length === 0 && !deletionOnly && !input.sandbox) { return { status: 'skipped', reason: 'Diff too large to review' }; } // `shown` is only the prompt. Placement uses `included`, so record which files the model @@ -295,7 +310,7 @@ export async function runReview(input: ReviewInput): Promise { const pathInstructions = config.pathInstructions.flatMap((entry) => { const glob = new Bun.Glob(entry.path); - return shown.some((file) => glob.match(file.path)) + return [...shown, ...removedShown].some((file) => glob.match(file.path)) ? [`For files matching ${entry.path}: ${entry.instructions}`] : []; }); @@ -324,8 +339,9 @@ export async function runReview(input: ReviewInput): Promise { guidelines, config, pathInstructions, - diff: shown.map(renderFileDiff).join('\n\n'), - excludedFiles: notShown, + diff: [...shown, ...removedShown].map(renderFileDiff).join('\n\n'), + excludedFiles: notShown.filter((file) => !removedShown.some((f) => f.path === file.path)), + deletionOnly, dependencyChanges: lockfiles.length ? dependencies : undefined, generatedFiles: generated.map((file) => file.path), incrementalFrom: input.incrementalFrom, @@ -531,11 +547,29 @@ export async function runReview(input: ReviewInput): Promise { const stillOpenBlocking = stillOpen.filter((f) => severityAtLeast(f.severity, threshold)).length; let verdict = decideVerdict({ posted, stillOpen, config }); // Approval is the one outcome an attacker would want: only grant it when it is safe to. - const truncated = shown.length < included.length; + const truncated = + shown.length < included.length || (deletionOnly && removedShown.length < removed.length); + // Nothing in a deletion-only pull request can carry an inline comment, so every finding is a + // placement miss listed in the summary. Those still decide whether Hansi approves. + const unplacedDefects = + deletionOnly && + decideVerdict({ + posted: dropped.filter( + (finding) => + isUnattachedDrop(finding.dropReason) && + severityAtLeast(finding.severity, config.reviews.minSeverity) + ), + stillOpen: [], + config + }) !== 'approve'; const approvalWithheld = verdict === 'approve' ? (input.withholdApproval ?? - (truncated ? 'Part of the diff was too large to review, so Hansi did not approve.' : null)) + (truncated + ? 'Part of the diff was too large to review, so Hansi did not approve.' + : unplacedDefects + ? 'Hansi found problems with this deletion that it could not comment on inline, so it did not approve.' + : null)) : openDefectApprovalReason([...posted, ...stillOpen], config); if (verdict === 'approve' && approvalWithheld) verdict = 'comment'; const presented = presentReview({