feat(ai-review 對話收斂): 讀 PR review 留言判斷解決狀態並收斂 findings #42
+70
-1
@@ -3,7 +3,7 @@ import assert from 'node:assert/strict';
|
||||
import fs from 'node:fs';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import { loadOldFindings, loadExclusions, applyExclusions, filterFalsePositivesWithAI } from './findings.js';
|
||||
import { loadOldFindings, loadExclusions, applyExclusions, filterFalsePositivesWithAI, appendExclusions } from './findings.js';
|
||||
import { EXCLUSIONS_PATH, FINDINGS_PATH } from './config.js';
|
||||
|
||||
describe('findings exclusions', () => {
|
||||
@@ -41,6 +41,41 @@ describe('findings exclusions', () => {
|
||||
assert.equal(exclusions[0].title, 'fetch_package_versions jq overhead');
|
||||
});
|
||||
|
||||
it('appends new exclusion entries and dedupes by file + original text', () => {
|
||||
const fullPath = path.join(workspace, EXCLUSIONS_PATH);
|
||||
fs.mkdirSync(path.dirname(fullPath), { recursive: true });
|
||||
fs.writeFileSync(fullPath, JSON.stringify([
|
||||
{ location: 'app/a.js:1', original_finding: '既有誤報' },
|
||||
], null, 2));
|
||||
|
||||
const merged = appendExclusions(workspace, [
|
||||
{ location: 'app/a.js:9', original_finding: '既有誤報', reason: '行號不同但同檔同原文 → 視為重複' },
|
||||
{ location: 'app/b.js:5', original_finding: '新誤報', reason: '誤報' },
|
||||
]);
|
||||
|
||||
const onDisk = JSON.parse(fs.readFileSync(fullPath, 'utf8'));
|
||||
assert.equal(onDisk.length, 2); // 1 既有 + 1 新增(重複者略過)
|
||||
assert.deepEqual(onDisk.map(e => e.location), ['app/a.js:1', 'app/b.js:5']);
|
||||
assert.equal(merged.length, 2);
|
||||
});
|
||||
|
||||
it('writes appended exclusions to both workspace and mirror dir', () => {
|
||||
const repoRoot = path.join(workspace, 'repo');
|
||||
fs.mkdirSync(repoRoot, { recursive: true });
|
||||
|
||||
appendExclusions(workspace, [{ location: 'app/x.js:3', original_finding: '誤報X', reason: 'r' }], repoRoot);
|
||||
|
||||
const ws = JSON.parse(fs.readFileSync(path.join(workspace, EXCLUSIONS_PATH), 'utf8'));
|
||||
const mirror = JSON.parse(fs.readFileSync(path.join(repoRoot, EXCLUSIONS_PATH), 'utf8'));
|
||||
assert.equal(ws[0].location, 'app/x.js:3');
|
||||
assert.deepEqual(mirror, ws);
|
||||
});
|
||||
|
||||
it('returns null and writes nothing when there are no new entries', () => {
|
||||
assert.equal(appendExclusions(workspace, []), null);
|
||||
assert.ok(!fs.existsSync(path.join(workspace, EXCLUSIONS_PATH)));
|
||||
});
|
||||
|
||||
it('repairs exclusions wrapper format to a top-level array', () => {
|
||||
const fullPath = path.join(workspace, EXCLUSIONS_PATH);
|
||||
fs.mkdirSync(path.dirname(fullPath), { recursive: true });
|
||||
@@ -142,6 +177,40 @@ describe('findings exclusions', () => {
|
||||
assert.ok(capturedUserContent.includes('"suggestion":"update tests"'));
|
||||
});
|
||||
|
||||
it('judges each finding with a parallel defender sub-agent and drops only false positives', async () => {
|
||||
const findings = [
|
||||
{ level: 'critical', role: 'Assassin', location: 'a.js:1', problem: 'p1', suggestion: 's1' },
|
||||
{ level: 'warning', role: 'Mage', location: 'b.js:2', problem: 'p2', suggestion: 's2' },
|
||||
{ level: 'info', role: 'Bard', location: 'c.js:3', problem: 'p3', suggestion: 's3' },
|
||||
];
|
||||
const seenPrompts = [];
|
||||
const chatFn = async (systemPrompt, userContent) => {
|
||||
seenPrompts.push(systemPrompt);
|
||||
const loc = JSON.parse(userContent).location;
|
||||
|
admin marked this conversation as resolved
|
||||
return { verdict: loc === 'b.js:2' ? 'false_positive' : 'confirmed' };
|
||||
};
|
||||
|
||||
const result = await filterFalsePositivesWithAI(findings, [], chatFn);
|
||||
|
||||
assert.deepEqual(result.map(f => f.location), ['a.js:1', 'c.js:3']); // b.js 誤報被剔除
|
||||
assert.equal(seenPrompts.length, 3); // 每條 finding 各一個 sub-agent
|
||||
assert.ok(seenPrompts.every(p => p.includes('Paladin'))); // 套用防守方角色
|
||||
});
|
||||
|
||||
it('keeps a finding when its defender sub-agent call fails (conservative)', async () => {
|
||||
const findings = [
|
||||
{ level: 'critical', role: 'Assassin', location: 'a.js:1', problem: 'p', suggestion: 's' },
|
||||
{ level: 'warning', role: 'Mage', location: 'b.js:2', problem: 'p', suggestion: 's' },
|
||||
];
|
||||
const chatFn = async (_s, userContent) => {
|
||||
if (JSON.parse(userContent).location === 'a.js:1') throw new Error('LLM down');
|
||||
return { verdict: 'false_positive' };
|
||||
};
|
||||
|
||||
const result = await filterFalsePositivesWithAI(findings, [], chatFn);
|
||||
assert.deepEqual(result.map(f => f.location), ['a.js:1']); // a 失敗→保守保留;b 誤報→剔除
|
||||
});
|
||||
|
||||
it('logs exclusions file metadata and repo state when loading exclusions', () => {
|
||||
const fullPath = path.join(workspace, EXCLUSIONS_PATH);
|
||||
fs.mkdirSync(path.dirname(fullPath), { recursive: true });
|
||||
|
||||
+65
-66
@@ -4,7 +4,7 @@ import {
|
||||
parseBotReviewComment,
|
||||
groupConversations,
|
||||
codeWindow,
|
||||
judgeConversationsResolved,
|
||||
judgeConversations,
|
||||
reconcileConversations,
|
||||
dropResolvedFindings,
|
||||
addCarriedFindings,
|
||||
@@ -95,53 +95,46 @@ describe('codeWindow', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('judgeConversationsResolved', () => {
|
||||
it('aligns verdicts by idx and defaults missing/non-true to false', async () => {
|
||||
describe('judgeConversations', () => {
|
||||
it('aligns verdicts by idx and defaults missing entries to open', async () => {
|
||||
const items = [{ idx: 0 }, { idx: 1 }, { idx: 2 }];
|
||||
const chatFn = async () => [{ idx: 0, resolved: true }, { idx: 1, resolved: false }];
|
||||
const verdicts = await judgeConversationsResolved(items, chatFn);
|
||||
const chatFn = async () => [{ idx: 0, verdict: 'resolved' }, { idx: 1, verdict: 'false_positive' }];
|
||||
const verdicts = await judgeConversations(items, chatFn);
|
||||
assert.deepEqual(verdicts, [
|
||||
{ idx: 0, resolved: true },
|
||||
{ idx: 1, resolved: false },
|
||||
{ idx: 2, resolved: false },
|
||||
{ idx: 0, verdict: 'resolved' },
|
||||
{ idx: 1, verdict: 'false_positive' },
|
||||
{ idx: 2, verdict: 'open' }, // 缺項 → open
|
||||
]);
|
||||
});
|
||||
|
||||
it('treats non-array AI output as all unresolved', async () => {
|
||||
const verdicts = await judgeConversationsResolved([{ idx: 0 }], async () => ({}));
|
||||
assert.deepEqual(verdicts, [{ idx: 0, resolved: false }]);
|
||||
it('treats non-array AI output as all open', async () => {
|
||||
const verdicts = await judgeConversations([{ idx: 0 }], async () => ({}));
|
||||
assert.deepEqual(verdicts, [{ idx: 0, verdict: 'open' }]);
|
||||
});
|
||||
|
||||
it('filters out AI results missing idx or resolved and keeps valid ones', async () => {
|
||||
it('ignores entries with unknown verdict or non-integer idx', async () => {
|
||||
const items = [{ idx: 0 }, { idx: 1 }];
|
||||
const chatFn = async () => [{ resolved: true }, { idx: 1, resolved: true }, { idx: 0 }];
|
||||
const verdicts = await judgeConversationsResolved(items, chatFn);
|
||||
const chatFn = async () => [
|
||||
{ idx: 0, verdict: 'maybe' }, // 不合法 verdict → 過濾 → idx0 預設 open
|
||||
{ idx: '1', verdict: 'resolved' }, // 字串 idx → 過濾
|
||||
{ idx: 1, verdict: 'resolved' }, // 有效
|
||||
];
|
||||
const verdicts = await judgeConversations(items, chatFn);
|
||||
assert.deepEqual(verdicts, [
|
||||
{ idx: 0, resolved: false }, // {idx:0} 缺 resolved → 視為 false;缺 idx 的整筆被過濾
|
||||
{ idx: 1, resolved: true },
|
||||
]);
|
||||
});
|
||||
|
||||
it('ignores entries whose idx is not an integer (e.g. a string)', async () => {
|
||||
const items = [{ idx: 0 }, { idx: 1 }];
|
||||
// 字串 idx '0' 不可冒充整數 idx 0 把它改成 resolved
|
||||
const chatFn = async () => [{ idx: '0', resolved: true }, { idx: 1.5, resolved: true }, { idx: 1, resolved: true }];
|
||||
const verdicts = await judgeConversationsResolved(items, chatFn);
|
||||
assert.deepEqual(verdicts, [
|
||||
{ idx: 0, resolved: false },
|
||||
{ idx: 1, resolved: true },
|
||||
{ idx: 0, verdict: 'open' },
|
||||
{ idx: 1, verdict: 'resolved' },
|
||||
]);
|
||||
});
|
||||
|
||||
it('propagates errors thrown by chatFn to the caller', async () => {
|
||||
await assert.rejects(
|
||||
() => judgeConversationsResolved([{ idx: 0 }], async () => { throw new Error('LLM down'); }),
|
||||
() => judgeConversations([{ idx: 0 }], async () => { throw new Error('LLM down'); }),
|
||||
/LLM down/,
|
||||
);
|
||||
});
|
||||
|
||||
it('returns [] for no items', async () => {
|
||||
assert.deepEqual(await judgeConversationsResolved([]), []);
|
||||
assert.deepEqual(await judgeConversations([]), []);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -153,76 +146,79 @@ describe('reconcileConversations', () => {
|
||||
{ id: 3, path: 'c.js', position: 30, body: reviewBody('🔵 建議', 'Bard', 'p3', 'fix three'), resolver: { login: 'dev' } },
|
||||
],
|
||||
getFileContent: async () => 'some code',
|
||||
judge: async (items) => items.map(it => ({ idx: it.idx, resolved: it.path === 'a.js' })),
|
||||
judge: async (items) => items.map(it => ({ idx: it.idx, verdict: 'open' })),
|
||||
resolveComment: async () => ({ ok: true }),
|
||||
});
|
||||
|
||||
it('resolves AI-confirmed conversations and carries unresolved ones back', async () => {
|
||||
const resolvedIds = [];
|
||||
it('closes all open conversations and buckets findings by AI verdict', async () => {
|
||||
const closedIds = [];
|
||||
const deps = baseDeps();
|
||||
deps.resolveComment = async (id) => { resolvedIds.push(id); return { ok: true }; };
|
||||
deps.resolveComment = async (id) => { closedIds.push(id); return { ok: true }; };
|
||||
// a.js → resolved、b.js → false_positive(c.js 已 resolved 略過)
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, verdict: it.path === 'a.js' ? 'resolved' : 'false_positive' }));
|
||||
|
||||
const result = await reconcileConversations(deps);
|
||||
|
||||
assert.deepEqual(resolvedIds, [1]); // a.js resolved, c.js already resolved (skipped)
|
||||
assert.deepEqual(closedIds.sort(), [1, 2]); // 兩個未解決對話都被關閉
|
||||
assert.equal(result.closedCount, 2);
|
||||
assert.equal(result.resolvedCount, 1);
|
||||
assert.equal(result.unresolvedCount, 1); // only b.js (c.js was skipped as already resolved)
|
||||
assert.equal(result.falsePositiveCount, 1);
|
||||
assert.equal(result.openCount, 0);
|
||||
assert.deepEqual(result.resolvedFindings.map(f => f.location), ['a.js:10']);
|
||||
assert.deepEqual(result.carriedFindings.map(f => f.suggestion), ['fix two']);
|
||||
assert.deepEqual(result.excludedFindings.map(e => e.location), ['b.js:20']);
|
||||
assert.equal(result.excludedFindings[0].original_finding, 'fix two');
|
||||
assert.deepEqual(result.carriedFindings, []);
|
||||
});
|
||||
|
||||
it('resolves (not carries) an unresolved conversation already present in old findings', async () => {
|
||||
const resolvedIds = [];
|
||||
it('carries open-verdict conversations into findings while still closing them', async () => {
|
||||
const closedIds = [];
|
||||
const deps = baseDeps();
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, resolved: false })); // 全部未修復
|
||||
deps.resolveComment = async (id) => { resolvedIds.push(id); return { ok: true }; };
|
||||
// b.js 的問題已存在於舊問題(檔案+建議簽章相符,行號不同不影響)→ 應解決對話、不加回
|
||||
deps.oldFindings = [{ location: 'b.js:99', suggestion: 'fix two' }];
|
||||
deps.resolveComment = async (id) => { closedIds.push(id); return { ok: true }; };
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, verdict: 'open' }));
|
||||
|
||||
const result = await reconcileConversations(deps);
|
||||
|
||||
assert.deepEqual(resolvedIds, [2]); // 僅 b.js(id=2) 因已存在舊問題而被 resolve
|
||||
assert.equal(result.duplicateCount, 1);
|
||||
assert.equal(result.resolvedCount, 0);
|
||||
assert.deepEqual(result.carriedFindings.map(f => f.suggestion), ['fix one']); // a.js 不在舊問題 → 加回
|
||||
assert.deepEqual(result.resolvedFindings, []); // duplicate 不從舊問題移除
|
||||
assert.deepEqual(closedIds.sort(), [1, 2]); // 仍全部關閉
|
||||
assert.equal(result.openCount, 2);
|
||||
assert.deepEqual(result.carriedFindings.map(f => f.suggestion).sort(), ['fix one', 'fix two']);
|
||||
assert.deepEqual(result.resolvedFindings, []);
|
||||
assert.deepEqual(result.excludedFindings, []);
|
||||
});
|
||||
|
||||
it('carries the conversation back when the resolve API call fails', async () => {
|
||||
it('still buckets findings even when closing a conversation fails', async () => {
|
||||
const deps = baseDeps();
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, resolved: true }));
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, verdict: 'resolved' }));
|
||||
deps.resolveComment = async () => { throw new Error('403'); };
|
||||
|
||||
const result = await reconcileConversations(deps);
|
||||
assert.equal(result.resolvedCount, 0);
|
||||
assert.equal(result.carriedFindings.length, 2);
|
||||
assert.equal(result.closedCount, 0); // 關閉皆失敗
|
||||
assert.equal(result.resolvedCount, 2); // 判斷照常生效
|
||||
assert.deepEqual(result.resolvedFindings.map(f => f.location).sort(), ['a.js:10', 'b.js:20']);
|
||||
});
|
||||
|
||||
it('treats all conversations as unresolved when the judge throws', async () => {
|
||||
const resolvedIds = [];
|
||||
it('treats all conversations as open when the judge throws, still closing them', async () => {
|
||||
const closedIds = [];
|
||||
const deps = baseDeps();
|
||||
deps.judge = async () => { throw new Error('judge boom'); };
|
||||
deps.resolveComment = async (id) => { resolvedIds.push(id); return { ok: true }; };
|
||||
deps.resolveComment = async (id) => { closedIds.push(id); return { ok: true }; };
|
||||
|
||||
const result = await reconcileConversations(deps);
|
||||
|
||||
assert.deepEqual(resolvedIds, []); // 無任何對話被 resolve
|
||||
assert.equal(result.resolvedCount, 0);
|
||||
assert.equal(result.carriedFindings.length, 2); // a.js + b.js 皆加回(c.js 已解決略過)
|
||||
assert.deepEqual(closedIds.sort(), [1, 2]);
|
||||
assert.equal(result.openCount, 2);
|
||||
assert.deepEqual(result.carriedFindings.map(f => f.suggestion).sort(), ['fix one', 'fix two']);
|
||||
});
|
||||
|
||||
it('treats a file as empty and continues when getFileContent throws', async () => {
|
||||
const deps = baseDeps();
|
||||
deps.getFileContent = async (path) => { if (path === 'a.js') throw new Error('404'); return 'some code'; };
|
||||
// judge 收到的 a.js code 應為空字串,仍照常判斷、不丟例外
|
||||
deps.getFileContent = async (p) => { if (p === 'a.js') throw new Error('404'); return 'some code'; };
|
||||
let seenCode;
|
||||
deps.judge = async (items) => { seenCode = items.find(it => it.path === 'a.js')?.code; return items.map(it => ({ idx: it.idx, resolved: false })); };
|
||||
deps.judge = async (items) => { seenCode = items.find(it => it.path === 'a.js')?.code; return items.map(it => ({ idx: it.idx, verdict: 'open' })); };
|
||||
|
||||
const result = await reconcileConversations(deps);
|
||||
assert.equal(seenCode, '');
|
||||
assert.equal(result.resolvedCount, 0);
|
||||
assert.equal(result.carriedFindings.length, 2); // a.js + b.js 加回(c.js 已解決略過)
|
||||
assert.equal(result.openCount, 2);
|
||||
assert.equal(result.carriedFindings.length, 2);
|
||||
});
|
||||
|
||||
it('skips path-traversal file paths without calling getFileContent', async () => {
|
||||
@@ -232,8 +228,8 @@ describe('reconcileConversations', () => {
|
||||
{ id: 1, path: '../../etc/passwd', position: 1, body: reviewBody('🔴 嚴重', 'Assassin', 'p', 's') },
|
||||
{ id: 2, path: 'b.js', position: 20, body: reviewBody('🟡 警告', 'Mage', 'p2', 'fix two') },
|
||||
];
|
||||
deps.getFileContent = async (path) => { requested.push(path); return 'code'; };
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, resolved: false }));
|
||||
deps.getFileContent = async (p) => { requested.push(p); return 'code'; };
|
||||
deps.judge = async (items) => items.map(it => ({ idx: it.idx, verdict: 'open' }));
|
||||
|
||||
await reconcileConversations(deps);
|
||||
assert.deepEqual(requested, ['b.js']); // 不安全路徑未被請求
|
||||
@@ -241,14 +237,17 @@ describe('reconcileConversations', () => {
|
||||
|
||||
it('returns empty result and does not throw when listing comments fails', async () => {
|
||||
const result = await reconcileConversations({ listComments: async () => { throw new Error('boom'); } });
|
||||
assert.deepEqual(result, { resolvedFindings: [], carriedFindings: [], resolvedCount: 0, unresolvedCount: 0 });
|
||||
assert.equal(result.closedCount, 0);
|
||||
assert.equal(result.resolvedFindings.length, 0);
|
||||
assert.equal(result.excludedFindings.length, 0);
|
||||
assert.equal(result.carriedFindings.length, 0);
|
||||
});
|
||||
|
||||
it('returns empty result when there are no open conversations', async () => {
|
||||
const result = await reconcileConversations({
|
||||
listComments: async () => [{ id: 1, path: 'a.js', position: 1, body: 'x', resolver: { login: 'd' } }],
|
||||
});
|
||||
assert.equal(result.resolvedCount, 0);
|
||||
assert.equal(result.closedCount, 0);
|
||||
assert.equal(result.carriedFindings.length, 0);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user
嚴重等級:🟡 警告
審查員:Maya
問題:未測試
resolveMissingLineNumbers當chatFn回傳無效行號時的處理。建議:補上測試案例:模擬
chatFn回傳無效行號,確保其進入 fallback 邏輯。