將 AI 審查結果集中到單一 Pull Review #27

Closed
jiantw83 wants to merge 5 commits from ai-review-resolve/20260622094109 into develop
4 changed files with 93 additions and 2 deletions
Showing only changes of commit 228d5ceb4a - Show all commits
+50 -1
View File
@@ -3,7 +3,7 @@ import assert from 'node:assert/strict';
import fs from 'node:fs'; import fs from 'node:fs';
import os from 'node:os'; import os from 'node:os';
import path from 'node:path'; import path from 'node:path';
import { saveFindings, parseLocation, postNewCriticalComments } from './comments.js'; import { saveFindings, parseLocation, buildReviewPayload, postFindingsReview, postNewCriticalComments } from './comments.js';
import { FINDINGS_PATH } from './config.js'; import { FINDINGS_PATH } from './config.js';
describe('saveFindings', () => { describe('saveFindings', () => {
@@ -185,3 +185,52 @@ describe('postNewCriticalComments', () => {
assert.ok(issueCalls.every(b => criticalCommentPattern.test(b))); assert.ok(issueCalls.every(b => criticalCommentPattern.test(b)));
}); });
}); });
describe('review payload', () => {
const intro = '## Reviewers\n\nLeo / Maya';
const findings = [
{ level: 'warning', role: 'Leo', location: 'app/a.js:1', suggestion: '改善命名', is_new: true },
{ level: 'critical', role: 'Rex', location: 'app/b.js:5', suggestion: '修正權限檢查', is_new: true },
{ level: 'critical', role: 'Maya', location: 'app/c.js', suggestion: '補上交易保護', is_new: true },
{ level: 'info', role: 'Bard', location: 'app/d.js:9', suggestion: '補註解', is_new: false },
];
it('builds one review body with role intro and inline comments for every located finding', () => {
const payload = buildReviewPayload(intro, findings);
assert.match(payload.body, /Reviewers/);
assert.doesNotMatch(payload.body, /改善命名/);
assert.match(payload.body, /無法行內標註的新嚴重問題/);
assert.equal(payload.comments.length, 3);
assert.deepEqual(payload.comments.map(c => c.path), ['app/a.js', 'app/b.js', 'app/d.js']);
assert.deepEqual(payload.comments.map(c => c.new_position), [1, 5, 9]);
assert.match(payload.comments[0].body, /新發現問題/);
assert.match(payload.comments[1].body, /新嚴重問題/);
assert.match(payload.comments[2].body, /舊有未解決問題/);
});
it('posts findings as a single pull review', async () => {
const calls = [];
await postFindingsReview(intro, findings, {
postReview: async (payload) => { calls.push(payload); },
});
assert.equal(calls.length, 1);
assert.match(calls[0].body, /Reviewers/);
assert.equal(calls[0].comments.length, 3);
});
it('falls back to one review body when inline comments are rejected', async () => {
const calls = [];
await postFindingsReview(intro, findings, {
postReview: async (payload) => {
calls.push(payload);
if (calls.length === 1) throw new Error('line not in diff');
},
});
assert.equal(calls.length, 2);
assert.equal(calls[1].comments.length, 0);
assert.match(calls[1].body, /改善命名/);
assert.match(calls[1].body, /修正權限檢查/);
assert.match(calls[1].body, /補上交易保護/);
assert.match(calls[1].body, /補註解/);
});
});
+7
View File
@@ -114,6 +114,13 @@ describe('getLLMConfig', () => {
assert.equal(shouldSkipOpenCodeTLSVerify(), false); assert.equal(shouldSkipOpenCodeTLSVerify(), false);
}); });
it('skips OpenCode TLS verification for any value other than false', () => {
for (const value of ['', '0', 'true', 'yes']) {
process.env.OPENCODE_SKIP_TLS_VERIFY = value;
assert.equal(shouldSkipOpenCodeTLSVerify(), true);
}
});
it('openai takes priority over gemini when both set', () => { it('openai takes priority over gemini when both set', () => {
process.env.OPENAI_API_KEY = 'sk-test'; process.env.OPENAI_API_KEY = 'sk-test';
process.env.GEMINI_API_KEY = 'gemini-key'; process.env.GEMINI_API_KEY = 'gemini-key';
+19 -1
View File
@@ -1,7 +1,7 @@
import { describe, it, afterEach, mock } from 'node:test'; import { describe, it, afterEach, mock } from 'node:test';
import assert from 'node:assert/strict'; import assert from 'node:assert/strict';
import axios from 'axios'; import axios from 'axios';
import { getPRDiff, filterDiff, postComment, postPullReviewComment, getCommitMessageBySha, getBranchHeadCommitMessage, shouldSkipBotCommit, getBotReviewOutcome } from './gitea.js'; import { getPRDiff, filterDiff, postComment, createPullReview, postPullReviewComment, getCommitMessageBySha, getBranchHeadCommitMessage, shouldSkipBotCommit, getBotReviewOutcome } from './gitea.js';
afterEach(() => mock.restoreAll()); afterEach(() => mock.restoreAll());
@@ -77,6 +77,24 @@ describe('gitea', () => {
assert.ok(capturedOpts.headers['Authorization'].startsWith('token ')); assert.ok(capturedOpts.headers['Authorization'].startsWith('token '));
}); });
it('createPullReview posts body and comments in one pull review', async () => {
let capturedUrl, capturedBody;
mock.method(axios, 'post', async (url, body) => {
capturedUrl = url;
capturedBody = body;
return { data: { id: 8 } };
});
const result = await createPullReview({
body: 'review body',
comments: [{ path: 'app/a.js', body: 'inline', new_position: 3 }],
});
assert.deepEqual(result, { id: 8 });
assert.ok(capturedUrl.endsWith('/reviews'));
assert.equal(capturedBody.event, 'COMMENT');
assert.equal(capturedBody.body, 'review body');
assert.deepEqual(capturedBody.comments, [{ path: 'app/a.js', body: 'inline', new_position: 3 }]);
});
it('postPullReviewComment propagates axios errors', async () => { it('postPullReviewComment propagates axios errors', async () => {
mock.method(axios, 'post', async () => { throw new Error('not in diff'); }); mock.method(axios, 'post', async () => { throw new Error('not in diff'); });
await assert.rejects(() => postPullReviewComment({ path: 'a.js', line: 1, body: 'x' }), /not in diff/); await assert.rejects(() => postPullReviewComment({ path: 'a.js', line: 1, body: 'x' }), /not in diff/);
+17
View File
@@ -199,6 +199,23 @@ describe('verifyLLM', () => {
assert.equal(agents[1].options.rejectUnauthorized, false); assert.equal(agents[1].options.rejectUnauthorized, false);
}); });
Review

分類:新嚴重問題
等級🔴 嚴重
審查員:Assassin
建議:此測試進一步確認了 OPENCODE_SKIP_TLS_VERIFY = 'true' 會導致 HTTPS 代理設定為 rejectUnauthorized: false,即跳過 TLS 憑證驗證。這是一個嚴重的「不安全預設」或「不安全配置」問題。

雖然這是一個明確的設定,但跳過 TLS 驗證會使應用程式容易受到中間人(MITM)攻擊。攻擊者可以在應用程式與 OpenCode 服務之間偽造伺服器身份,攔截、竊聽或篡改所有通訊內容,進而竊取機密資訊或注入惡意指令。

建議除非在極端受控的環境下,否則應避免跳過 TLS 驗證。如果確實需要此功能,應確保其使用受到嚴格的審查和限制,並在文件上明確標示其安全風險。更安全的做法是配置正確的憑證信任鏈,而不是禁用驗證。

**分類**:新嚴重問題 **等級**:🔴 嚴重 **審查員**:Assassin **建議**:此測試進一步確認了 `OPENCODE_SKIP_TLS_VERIFY = 'true'` 會導致 HTTPS 代理設定為 `rejectUnauthorized: false`,即跳過 TLS 憑證驗證。這是一個嚴重的「不安全預設」或「不安全配置」問題。 雖然這是一個明確的設定,但跳過 TLS 驗證會使應用程式容易受到中間人(MITM)攻擊。攻擊者可以在應用程式與 OpenCode 服務之間偽造伺服器身份,攔截、竊聽或篡改所有通訊內容,進而竊取機密資訊或注入惡意指令。 建議除非在極端受控的環境下,否則應避免跳過 TLS 驗證。如果確實需要此功能,應確保其使用受到嚴格的審查和限制,並在文件上明確標示其安全風險。更安全的做法是配置正確的憑證信任鏈,而不是禁用驗證。
it('passes an insecure https agent for opencode when TLS skip is explicitly true', async () => {
clearLLMEnv();
process.env.OPENCODE_BASE_URL = 'https://opencode.local:4096';
process.env.OPENCODE_SKIP_TLS_VERIFY = 'true';
const agents = [];
mock.method(axios, 'get', async (url, opts) => {
agents.push(opts.httpsAgent);
if (url.endsWith('/global/health')) return { data: { healthy: true } };
return { data: { providers: [{ id: 'google', models: { 'gemini-2.5-flash': { id: 'gemini-2.5-flash' } } }] } };
});
const result = await verifyLLM();
assert.equal(result.ok, true);
assert.equal(agents.length, 2);
assert.equal(agents[0].options.rejectUnauthorized, false);
assert.equal(agents[1].options.rejectUnauthorized, false);
});
it('does not pass an insecure https agent for opencode when TLS verification is enabled', async () => { it('does not pass an insecure https agent for opencode when TLS verification is enabled', async () => {
clearLLMEnv(); clearLLMEnv();
process.env.OPENCODE_BASE_URL = 'https://opencode.local:4096'; process.env.OPENCODE_BASE_URL = 'https://opencode.local:4096';