Merge pull request 'fix(審查流程): 修正 bot 跳過、JSON 驗證與排除比對邊界' (#4) from develop into master

Reviewed-on: #4

src/comments.js

@@ -69,7 +69,8 @@ export function parseLocation(location) {
   if (trimmed.includes(',')) return null;
   const match = trimmed.match(/^(.+?):(\d+)(?:-\d+)?$/);
   if (!match) return null;
-  return { file: match[1], line: Number(match[2]) };
+  const line = Number(match[2]);
+  return line > 0 ? { file: match[1], line } : null;
 }
 
 /** 行內 comment 內容：等級／審查員／建議 */

src/findings.js

@@ -548,7 +548,7 @@ export function applyExclusions(findings, exclusions) {
     const fPath = String(f.location).split(':')[0];
     const exPath = ex.filePath || (ex.location ? String(ex.location).split(':')[0] : null);
     const findingText = normalizeText(f.suggestion || f.title || '');
-    const exclusionText = ex.textKey || normalizeText(ex.text || ex.suggestion || ex.title || '');
+    const exclusionText = normalizeText(ex.text || ex.original_finding || ex.suggestion || ex.title || ex.textKey || '');
     const locationMatches = (!exPath || fPath === exPath);
     const roleMatches = (!ex.role || ex.role === f.role);
     const textMatches = !exclusionText || !findingText || findingText.includes(exclusionText) || exclusionText.includes(findingText);

src/gitea.js

@@ -146,10 +146,10 @@ export async function getBranchHeadCommitMessage(branch = PR_HEAD_BRANCH) {
  */
 export async function shouldSkipBotCommit({ sha = PR_HEAD_SHA || process.env.GITHUB_SHA, branch = PR_HEAD_BRANCH } = {}) {
   const shaMessage = await getCommitMessageBySha(sha);
-  if (sha && shaMessage.includes('[ai-review-bot]')) return true;
+  if (sha && shaMessage.includes('[ai-review-bot]') && getBotReviewOutcome(shaMessage) !== 'failure') return true;
 
   const branchMessage = await getBranchHeadCommitMessage(branch);
-  if (branch && branchMessage.includes('[ai-review-bot]')) return true;
+  if (branch && branchMessage.includes('[ai-review-bot]') && getBotReviewOutcome(branchMessage) !== 'failure') return true;
 
   return false;
 }

src/json.js

@@ -109,7 +109,10 @@ export async function validateJSONArrayFile(fullPath, label, repairer = repairJS
       const repaired = await repairer(fullPath, label, original);
       const normalized = repaired.endsWith('\n') ? repaired : `${repaired}\n`;
       // 先驗證修復結果是否為合法 JSON；無效就在寫檔前丟出，避免用毀損內容覆寫原檔。
-      JSON.parse(normalized);
+      const parsed = JSON.parse(normalized);
+      if (!Array.isArray(parsed)) {
+        throw new Error(`${label} 修復後內容不是 JSON 陣列`);
+      }
       fs.writeFileSync(fullPath, normalized, 'utf8');
       ok(`${label} 已由 AI 修正並通過再次驗證`);
       return { exists: true, valid: true, repaired: true };

src/resolve.js

@@ -78,16 +78,18 @@ export function groupConversations(comments) {
     const lineNum = Number(c?.position) || Number(c?.new_position) || Number(c?.original_position) || 0;
     const key = `${filePath}|${lineNum}`;
     if (!groups.has(key)) {
-      groups.set(key, { key, path: filePath, line: lineNum, commentIds: [], bodies: [], resolved: false, botFinding: null });
+      groups.set(key, { key, path: filePath, line: lineNum, commentIds: [], bodies: [], resolved: false, botFinding: null, botFindings: [] });
     }
     const g = groups.get(key);
     if (c?.id != null) g.commentIds.push(c.id);
     const body = typeof c?.body === 'string' ? c.body : '';
     if (body) g.bodies.push(body);
     if (c?.resolver) g.resolved = true;
-    if (!g.botFinding) {
     const finding = parseBotReviewComment(body);
-      if (finding) g.botFinding = { ...finding, location: lineNum ? `${filePath}:${lineNum}` : filePath };
+    if (finding) {
+      const normalizedFinding = { ...finding, location: lineNum ? `${filePath}:${lineNum}` : filePath };
+      g.botFindings.push(normalizedFinding);
+      if (!g.botFinding) g.botFinding = normalizedFinding;
     }
   }
   return [...groups.values()].map(g => ({ ...g, thread: g.bodies.join('\n---\n') }));
@@ -146,8 +148,12 @@ export async function judgeConversations(items, chatFn = chatJSON) {
  * @returns {void}
  */
 function pushCarried(target, conversation) {
-  if (!conversation.botFinding) return;
+  const findings = conversation.botFindings?.length
-  target.push({ ...conversation.botFinding, is_new: false });
+    ? conversation.botFindings
+    : (conversation.botFinding ? [conversation.botFinding] : []);
+  for (const finding of findings) {
+    target.push({ ...finding, is_new: false });
+  }
 }
 
 /**
@@ -273,10 +279,16 @@ export async function reconcileConversations(deps = {}) {
     const verdict = verdictByIdx.get(i) || 'open';
     if (verdict === 'resolved') {
       resolvedCount += 1;
-      if (c.botFinding) resolvedFindings.push({ ...c.botFinding, is_new: false });
+      const findings = c.botFindings?.length ? c.botFindings : (c.botFinding ? [c.botFinding] : []);
+      for (const finding of findings) {
+        resolvedFindings.push({ ...finding, is_new: false });
+      }
     } else if (verdict === 'false_positive') {
       falsePositiveCount += 1;
-      if (c.botFinding) excludedFindings.push(toExclusion(c.botFinding));
+      const findings = c.botFindings?.length ? c.botFindings : (c.botFinding ? [c.botFinding] : []);
+      for (const finding of findings) {
+        excludedFindings.push(toExclusion(finding));
+      }
     } else {
       openCount += 1;
       pushCarried(carriedFindings, c);

src/test/comments.test.js

@@ -87,6 +87,10 @@ describe('parseLocation', () => {
     assert.equal(parseLocation('app/preflight.test.js'), null);
   });
 
+  it('returns null when the parsed line number is zero', () => {
+    assert.equal(parseLocation('app/preflight.js:0'), null);
+  });
+
   it('returns null when multiple files are listed', () => {
     assert.equal(parseLocation('Dockerfile, app/git.js, app/gitea.js'), null);
   });

src/test/findings.test.js

@@ -144,6 +144,21 @@ describe('findings exclusions', () => {
     assert.equal(filtered[0].location, 'README.md:12');
   });
 
+  it('applies pure text exclusions using the original finding text', () => {
+    const findings = [
+      { location: 'src/app.ts:10', role: 'Maya', suggestion: 'Update tests' },
+      { location: 'src/app.ts:11', role: 'Maya', suggestion: 'Keep this' },
+    ];
+    const exclusions = [
+      { original_finding: 'update tests' },
+    ];
+
+    const filtered = applyExclusions(findings, exclusions);
+
+    assert.equal(filtered.length, 1);
+    assert.equal(filtered[0].suggestion, 'Keep this');
+  });
+
   it('dedupes repeated exclusions when loading exclusions', () => {
     const fullPath = path.join(workspace, EXCLUSIONS_PATH);
     fs.mkdirSync(path.dirname(fullPath), { recursive: true });

src/test/gitea.test.js

@@ -197,17 +197,25 @@ describe('gitea', () => {
     assert.equal(await getFileContentAtRef('missing.js', 'ref'), '');
   });
 
-  it('shouldSkipBotCommit returns true when either sha or branch head is bot commit', async () => {
+  it('shouldSkipBotCommit returns true when either sha or branch head is a bot success commit, but not failure', async () => {
     mock.method(axios, 'get', async (url) => {
       if (url.includes('/git/commits/sha-bot')) {
         return { data: { message: 'chore: update ai-review findings [ai-review-bot][failure]' } };
       }
+      if (url.includes('/git/commits/sha-success')) {
+        return { data: { message: 'chore: update ai-review findings [ai-review-bot][success]' } };
+      }
       if (url.includes('/branches/feat%2Ftest')) {
         return { data: { commit: { id: 'sha-bot' } } };
       }
+      if (url.includes('/branches/feat%2Fsuccess')) {
+        return { data: { commit: { id: 'sha-success' } } };
+      }
       return { data: { message: 'regular commit' } };
     });
-    await assert.equal(await shouldSkipBotCommit({ sha: 'sha-bot', branch: 'feat/test' }), true);
+    await assert.equal(await shouldSkipBotCommit({ sha: 'sha-bot', branch: 'feat/test' }), false);
+    await assert.equal(await shouldSkipBotCommit({ sha: 'sha-success', branch: 'feat/success' }), true);
+    await assert.equal(await shouldSkipBotCommit({ sha: 'sha-success', branch: 'feat/test' }), true);
     assert.equal(getBotReviewOutcome('chore: update ai-review findings [ai-review-bot][failure]'), 'failure');
     assert.equal(getBotReviewOutcome('chore: update ai-review findings [ai-review-bot][success]'), 'success');
     assert.equal(getBotReviewOutcome('chore: update ai-review findings [ai-review-bot]'), 'unknown');

src/test/json.test.js

@@ -77,6 +77,18 @@ describe('json helpers', () => {
     assert.equal(fs.readFileSync(fullPath, 'utf8'), '[]\n');
   });
 
+  it('rejects repaired JSON that is not an array', async () => {
+    const fullPath = path.join(workspace, '.gitea/ai-review/findings.json');
+    fs.mkdirSync(path.dirname(fullPath), { recursive: true });
+    fs.writeFileSync(fullPath, '{broken', 'utf8');
+
+    await assert.rejects(
+      () => validateJSONArrayFile(fullPath, '.gitea/ai-review/findings.json', async () => '{"ok":true}'),
+      /不是 JSON 陣列/,
+    );
+    assert.equal(fs.readFileSync(fullPath, 'utf8'), '{broken');
+  });
+
   it('reads a valid JSON file whose size equals the maximum limit', async () => {
     const fullPath = path.join(workspace, '.gitea/ai-review/findings.json');
     fs.mkdirSync(path.dirname(fullPath), { recursive: true });

src/test/resolve.test.js

@@ -83,6 +83,17 @@ describe('groupConversations', () => {
     const convos = groupConversations([{ id: 1, path: 'a.js', original_position: 7, body: 'x' }]);
     assert.equal(convos[0].line, 7);
   });
+
+  it('keeps multiple bot findings on the same path and line', () => {
+    const comments = [
+      { id: 1, path: 'a.js', position: 10, body: reviewBody('🔴 嚴重', 'Assassin', 'p1', 's1') },
+      { id: 2, path: 'a.js', position: 10, body: reviewBody('🟡 警告', 'Mage', 'p2', 's2') },
+    ];
+    const convos = groupConversations(comments);
+    assert.equal(convos.length, 1);
+    assert.equal(convos[0].botFindings.length, 2);
+    assert.deepEqual(convos[0].botFindings.map(f => f.role), ['Assassin', 'Mage']);
+  });
 });
 
 describe('codeWindow', () => {
@@ -207,6 +218,19 @@ describe('reconcileConversations', () => {
     assert.equal(result.closedCount, 3);
   });
 
+  it('preserves multiple bot findings when a grouped conversation is still open', async () => {
+    const deps = baseDeps();
+    deps.listComments = async () => [
+      { id: 10, path: 'a.js', position: 5, body: reviewBody('🔴 嚴重', 'Assassin', 'p', 's10') },
+      { id: 11, path: 'a.js', position: 5, body: reviewBody('🟡 警告', 'Mage', 'p', 's11') },
+    ];
+    deps.judge = async (items) => items.map(it => ({ idx: it.idx, verdict: 'open' }));
+
+    const result = await reconcileConversations(deps);
+
+    assert.deepEqual(result.carriedFindings.map(f => f.suggestion).sort(), ['s10', 's11']);
+  });
+
   it('counts only successful closes when some resolve calls fail', async () => {
     const deps = baseDeps();
     // a.js(id1) 關閉成功、b.js(id2) 關閉失敗（c.js 已 resolved 略過）