處理 AI review findings 並改寫 Node.js entrypoint #1

Merged
jiantw83 merged 58 commits from ai-review-resolve/20260624102518 into develop 2026-06-24 14:09:27 +00:00
Showing only changes of commit 94a6baae2c - Show all commits
+58 -41
View File
6
@@ -39,11 +39,11 @@ class TempFileRegistry {
}
cleanup() {
for (const filePath of Array.from(this.files).reverse()) {
for (const filePath of this.files) {
this.removeFile(filePath);
}
for (const dirPath of Array.from(this.dirs).reverse()) {
for (const dirPath of this.dirs) {
Ghost marked this conversation as resolved
Review

嚴重等級🔵 建議
審查員:Rogue
問題:在 TempFileRegistrycleanup 方法中,每次呼叫都使用 Array.from 將 Set 轉換為陣列,這在頻繁清理時會產生無謂的記憶體開銷。
建議:若無強烈反向迭代的需求,可考慮直接使用 forEach 遍歷 Set。若有嚴格順序需求,建議改用其他結構管理,避免每次 cleanup 都額外配置陣列。

**嚴重等級**:🔵 建議 **審查員**:Rogue **問題**:在 `TempFileRegistry` 的 `cleanup` 方法中,每次呼叫都使用 `Array.from` 將 Set 轉換為陣列,這在頻繁清理時會產生無謂的記憶體開銷。 **建議**:若無強烈反向迭代的需求,可考慮直接使用 `forEach` 遍歷 Set。若有嚴格順序需求,建議改用其他結構管理,避免每次 cleanup 都額外配置陣列。
try {
fs.rmSync(dirPath, { force: true, recursive: true });
this.dirs.delete(dirPath);
@@ -56,6 +56,37 @@ class TempFileRegistry {
const tempFiles = new TempFileRegistry();
class OutputCollector {
constructor(maxBytes) {
this.maxBytes = maxBytes;
this.chunks = [];
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Rogue
問題:在 validateAuth 中,為了驗證 base64 字串是否符合 base64 格式,進行了多次正規表達式替換與編解碼運算(如 encodedAuth.replaceBuffer.fromdecoded.toString('base64') 等),這在每次執行都會發生的情況下,浪費了不必要的 CPU 週期。
建議:如果目的只是驗證結構,建議盡量簡化邏輯。可以直接將字串嘗試轉換為 Buffer 並檢查 toString('base64') 是否匹配,避免多重正規表達式替換。

**嚴重等級**:🟡 警告 **審查員**:Rogue **問題**:在 `validateAuth` 中,為了驗證 base64 字串是否符合 base64 格式,進行了多次正規表達式替換與編解碼運算(如 `encodedAuth.replace`、`Buffer.from`、`decoded.toString('base64')` 等),這在每次執行都會發生的情況下,浪費了不必要的 CPU 週期。 **建議**:如果目的只是驗證結構,建議盡量簡化邏輯。可以直接將字串嘗試轉換為 Buffer 並檢查 `toString('base64')` 是否匹配,避免多重正規表達式替換。
this.size = 0;
this.truncated = false;
}
append(chunk) {
const available = this.maxBytes - this.size;
if (available <= 0) {
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Bard
問題:Base64 的驗證過程充滿了複雜的字串正規化與取代操作,讀起來像是在解迷宮,而非驗證身分。
建議:將驗證邏輯拆解或簡化,明確劃分「解碼」、「正規化」與「比較」三個步驟,提升程式碼的可讀性與可維護性。

**嚴重等級**:🟡 警告 **審查員**:Bard **問題**:Base64 的驗證過程充滿了複雜的字串正規化與取代操作,讀起來像是在解迷宮,而非驗證身分。 **建議**:將驗證邏輯拆解或簡化,明確劃分「解碼」、「正規化」與「比較」三個步驟,提升程式碼的可讀性與可維護性。
this.truncated = true;
return;
}
const storedChunk = chunk.length > available ? chunk.subarray(0, available) : chunk;
this.chunks.push(storedChunk);
this.size += storedChunk.length;
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Mage
問題:在 validateAuth 中,僅透過 JSON.parse 檢查 JSON 格式,但未針對 Codex 預期的 auth.json 結構(如必要的欄位)進行 Schema 驗證。如果傳入的 JSON 格式正確但內容無效,可能會導致 codex exec 在後續執行時失敗。
建議:建議加入對 JSON 內容的簡單結構驗證(例如確認是否有 token 或必要的連線設定欄位)。

**嚴重等級**:🟡 警告 **審查員**:Mage **問題**:在 `validateAuth` 中,僅透過 `JSON.parse` 檢查 JSON 格式,但未針對 Codex 預期的 auth.json 結構(如必要的欄位)進行 Schema 驗證。如果傳入的 JSON 格式正確但內容無效,可能會導致 `codex exec` 在後續執行時失敗。 **建議**:建議加入對 JSON 內容的簡單結構驗證(例如確認是否有 `token` 或必要的連線設定欄位)。
Review

嚴重等級🟡 警告
審查員:Mage
問題:在 appendGithubOutput 函式中,當 output 內容極大時,此處會將整個 output 字串在記憶體中進行檢查(output.includes(delimiter))與多次複製。這可能導致在處理極端長度輸出時發生記憶體不足的問題。
建議:建議限制 delimiter 嘗試次數,或在檢查時避免讀取整個 output 字串,改用串流處理方式。

**嚴重等級**:🟡 警告 **審查員**:Mage **問題**:在 `appendGithubOutput` 函式中,當 `output` 內容極大時,此處會將整個 `output` 字串在記憶體中進行檢查(`output.includes(delimiter)`)與多次複製。這可能導致在處理極端長度輸出時發生記憶體不足的問題。 **建議**:建議限制 `delimiter` 嘗試次數,或在檢查時避免讀取整個 `output` 字串,改用串流處理方式。
Review

嚴重等級🟡 警告
審查員:Maya
問題:OutputCollector 的截斷機制未被測試。
建議:增加測試案例模擬輸出超過 DEFAULT_OUTPUT_LIMIT_BYTES,驗證截斷提示。

**嚴重等級**:🟡 警告 **審查員**:Maya **問題**:OutputCollector 的截斷機制未被測試。 **建議**:增加測試案例模擬輸出超過 DEFAULT_OUTPUT_LIMIT_BYTES,驗證截斷提示。
if (storedChunk.length < chunk.length) {
this.truncated = true;
}
}
toString() {
Ghost marked this conversation as resolved
Review

嚴重等級🔵 建議
審查員:Rogue
問題:在 runCodex 中,使用 new Promise 封裝 child_process.spawn 並手動監聽 data 事件來拼接輸出。在高輸出量的場景下,不斷字串拼接(output += chunk.toString())會導致大量記憶體配置與 garbage collection 壓力。
建議:如果預期輸出量大,建議將 stdout/stderr 直接寫入檔案流或使用 Buffer 陣列收集後最後合併,減少中間字串變更帶來的記憶體浪費。

**嚴重等級**:🔵 建議 **審查員**:Rogue **問題**:在 `runCodex` 中,使用 `new Promise` 封裝 `child_process.spawn` 並手動監聽 data 事件來拼接輸出。在高輸出量的場景下,不斷字串拼接(`output += chunk.toString()`)會導致大量記憶體配置與 garbage collection 壓力。 **建議**:如果預期輸出量大,建議將 stdout/stderr 直接寫入檔案流或使用 `Buffer` 陣列收集後最後合併,減少中間字串變更帶來的記憶體浪費。
const truncationMessage = this.truncated ? "\n[Output truncated]\n" : "";
return `${Buffer.concat(this.chunks, this.size).toString()}${truncationMessage}`;
}
}
function cleanup() {
tempFiles.cleanup();
}
11
@@ -101,13 +132,16 @@ function fail(message, code = 1) {
process.exit(code);
}
Ghost marked this conversation as resolved
Review

嚴重等級🔵 建議
審查員:Maya
問題:測試檔案 tests/entrypoint_test.sh 有測試 missing_codex_command,這很好。但實作中對於 codex 執行失敗的各種細節(如權限不足、找不到 binary 等)都統一處理為 status: 1 和簡單的訊息,測試僅驗證了 failure 狀態,未驗證具體錯誤來源。
建議:考慮在 main.js 中根據不同的錯誤類型回傳更細緻的 status code,並在測試中驗證這些 code,能更精確地協助 CI 使用者除錯。

**嚴重等級**:🔵 建議 **審查員**:Maya **問題**:測試檔案 `tests/entrypoint_test.sh` 有測試 `missing_codex_command`,這很好。但實作中對於 `codex` 執行失敗的各種細節(如權限不足、找不到 binary 等)都統一處理為 `status: 1` 和簡單的訊息,測試僅驗證了 failure 狀態,未驗證具體錯誤來源。 **建議**:考慮在 `main.js` 中根據不同的錯誤類型回傳更細緻的 status code,並在測試中驗證這些 code,能更精確地協助 CI 使用者除錯。
Review

嚴重等級🔵 建議
審查員:Maya
問題:對於 codex 執行失敗的各種細節(權限不足、找不到 binary 等)都統一處理為 status: 1,測試未驗證具體錯誤來源。
建議:根據錯誤類型回傳細緻 status code,並在測試中驗證這些 code。

**嚴重等級**:🔵 建議 **審查員**:Maya **問題**:對於 codex 執行失敗的各種細節(權限不足、找不到 binary 等)都統一處理為 status: 1,測試未驗證具體錯誤來源。 **建議**:根據錯誤類型回傳細緻 status code,並在測試中驗證這些 code。
function validateAuth(encodedAuth, authFile) {
const normalizedAuth = encodedAuth.replace(/\s+/g, "");
const decoded = Buffer.from(encodedAuth, "base64");
const normalizedDecoded = decoded.toString("base64").replace(/=+$/, "");
const normalizedInput = normalizedAuth.replace(/=+$/, "");
function normalizeBase64(value) {
return value.replace(/\s+/g, "").replace(/=+$/, "");
}
if (decoded.length === 0 && normalizedAuth.length > 0) {
function validateAuth(encodedAuth, authFile) {
const decoded = Buffer.from(encodedAuth, "base64");
const normalizedDecoded = normalizeBase64(decoded.toString("base64"));
const normalizedInput = normalizeBase64(encodedAuth);
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Rogue
問題:validateAuth 中重複執行 decoded.toString('base64') 並進行 normalizedBase64 處理,極度浪費資源。
建議:移除這些無謂的重新編碼比較,直接嘗試解碼。

**嚴重等級**:🟡 警告 **審查員**:Rogue **問題**:validateAuth 中重複執行 decoded.toString('base64') 並進行 normalizedBase64 處理,極度浪費資源。 **建議**:移除這些無謂的重新編碼比較,直接嘗試解碼。
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Assassin
問題:將 OAUTH 環境變數內容解碼並直接寫入 auth.json。雖然有檢查 base64 格式與 JSON 結構,但若解碼後的 JSON 內容包含惡意配置(如惡意插件路徑或偽造的 API 憑證),可能導致後續 codex CLI 在執行時被劫持或洩漏資料。
建議:除了驗證 JSON 結構外,應進一步驗證 auth.json 內的欄位是否符合預期格式,並限制其檔案權限為 600(已做),確保容器內其他行程無法讀取。

**嚴重等級**:🟡 警告 **審查員**:Assassin **問題**:將 `OAUTH` 環境變數內容解碼並直接寫入 `auth.json`。雖然有檢查 base64 格式與 JSON 結構,但若解碼後的 JSON 內容包含惡意配置(如惡意插件路徑或偽造的 API 憑證),可能導致後續 `codex` CLI 在執行時被劫持或洩漏資料。 **建議**:除了驗證 JSON 結構外,應進一步驗證 `auth.json` 內的欄位是否符合預期格式,並限制其檔案權限為 `600`(已做),確保容器內其他行程無法讀取。
if (decoded.length === 0 && normalizedInput.length > 0) {
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Leo
問題main 函式過於龐大且職責過多,它同時負責了訊號處理、路徑創建、檔案鎖定、認證驗證以及執行核心邏輯,這降低了程式碼的可讀性與單元測試的困難度。
建議:建議將 main 拆分為 validateInputsetupAuthrunCodexActioncleanup 等子函式,讓職責分離。

**嚴重等級**:🟡 警告 **審查員**:Leo **問題**:`main` 函式過於龐大且職責過多,它同時負責了訊號處理、路徑創建、檔案鎖定、認證驗證以及執行核心邏輯,這降低了程式碼的可讀性與單元測試的困難度。 **建議**:建議將 `main` 拆分為 `validateInput`、`setupAuth`、`runCodexAction` 與 `cleanup` 等子函式,讓職責分離。
fail("OAUTH must be valid base64 encoded Codex auth.json.");
}
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Maya
問題:在 runCodex 函數中雖然有處理 child.on('error', ...),但若 codex 指令本身不存在(spawn ENOENT),這裡捕捉到的 error stack trace 可能會包含完整的系統路徑資訊,這在 CI 環境中屬於資訊洩漏風險。
建議:建議在錯誤處理中,針對 error.code === 'ENOENT' 做明確判斷,回傳簡潔的錯誤訊息(例如「找不到 codex 指令」),而非直接回傳完整的 error.message

**嚴重等級**:🟡 警告 **審查員**:Maya **問題**:在 `runCodex` 函數中雖然有處理 `child.on('error', ...)`,但若 `codex` 指令本身不存在(spawn ENOENT),這裡捕捉到的 error stack trace 可能會包含完整的系統路徑資訊,這在 CI 環境中屬於資訊洩漏風險。 **建議**:建議在錯誤處理中,針對 `error.code === 'ENOENT'` 做明確判斷,回傳簡潔的錯誤訊息(例如「找不到 codex 指令」),而非直接回傳完整的 `error.message`。
Review

嚴重等級🟡 警告
審查員:Maya
問題:spawn ENOENT 錯誤處理可能洩漏系統路徑資訊。
建議:針對 error.code === 'ENOENT' 做明確判斷,回傳簡潔錯誤訊息而非完整 stack trace。

**嚴重等級**:🟡 警告 **審查員**:Maya **問題**:spawn ENOENT 錯誤處理可能洩漏系統路徑資訊。 **建議**:針對 error.code === 'ENOENT' 做明確判斷,回傳簡潔錯誤訊息而非完整 stack trace。
5
@@ -134,18 +168,6 @@ function parsePositiveInteger(value, fallback) {
return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback;
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Maya
問題:parsePositiveInteger 的輸入回退機制未經測試。
建議:針對設定變數傳入無效數字或非法格式場景,驗證預設值套用。

**嚴重等級**:🟡 警告 **審查員**:Maya **問題**:parsePositiveInteger 的輸入回退機制未經測試。 **建議**:針對設定變數傳入無效數字或非法格式場景,驗證預設值套用。
}
function truncateOutput(chunks, nextChunk, maxBytes) {
const currentSize = chunks.reduce((total, chunk) => total + chunk.length, 0);
const available = maxBytes - currentSize;
if (available <= 0) {
return false;
}
chunks.push(nextChunk.length > available ? nextChunk.subarray(0, available) : nextChunk);
return nextChunk.length <= available;
}
function runCodex(model, prompt) {
return new Promise((resolve) => {
const timeoutMs = parsePositiveInteger(process.env.CODEX_TIMEOUT_MS, DEFAULT_CODEX_TIMEOUT_MS);
2
@@ -167,46 +189,39 @@ function runCodex(model, prompt) {
{ cwd: workspace, stdio: ["ignore", "pipe", "pipe"] },
Ghost marked this conversation as resolved
Review

嚴重等級🔴 嚴重
審查員:Mage
問題:在 setupAuth 中,lockPath 使用 os.tmpdir()。在共享環境中,如果 CODEX_HOME 字串相同,會導致所有 process 競爭同一個鎖檔,且如果其他無關的 process 也剛好在 os.tmpdir() 中建立相同名稱的檔案,會導致誤判或鎖定失敗。
建議:應在 CODEX_HOME 內部建立鎖檔,而非使用全域的 os.tmpdir(),或者包含更具唯一性的識別碼(如 PID 或更長的路徑雜湊)以確保鎖的隔離性。

**嚴重等級**:🔴 嚴重 **審查員**:Mage **問題**:在 `setupAuth` 中,`lockPath` 使用 `os.tmpdir()`。在共享環境中,如果 `CODEX_HOME` 字串相同,會導致所有 process 競爭同一個鎖檔,且如果其他無關的 process 也剛好在 `os.tmpdir()` 中建立相同名稱的檔案,會導致誤判或鎖定失敗。 **建議**:應在 `CODEX_HOME` 內部建立鎖檔,而非使用全域的 `os.tmpdir()`,或者包含更具唯一性的識別碼(如 PID 或更長的路徑雜湊)以確保鎖的隔離性。
Review

嚴重等級🔴 嚴重
審查員:Maya
問題:Codex 超時處理路徑未經測試,無法確保 SIGTERM 能成功發送與訊息正確產出。
建議:增加測試案例模擬長期睡眠(如 sleep 10),驗證超時機制與輸出訊息。

**嚴重等級**:🔴 嚴重 **審查員**:Maya **問題**:Codex 超時處理路徑未經測試,無法確保 SIGTERM 能成功發送與訊息正確產出。 **建議**:增加測試案例模擬長期睡眠(如 sleep 10),驗證超時機制與輸出訊息。
);
const outputChunks = [];
let outputTruncated = false;
const appendOutput = (chunk) => {
if (!truncateOutput(outputChunks, chunk, outputLimitBytes)) {
outputTruncated = true;
}
return Buffer.concat(outputChunks).toString();
};
const output = new OutputCollector(outputLimitBytes);
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Rogue
問題:在 setupAuth 中,lockPath 檔名產生使用了 Buffer.from(codexHome).toString("hex")。如果 codexHome 非常長,這個檔名可能會超過作業系統的檔案名稱長度限制(通常為 255 bytes),導致鎖定失敗,進而阻斷整個流程。
建議:改用 crypto.createHash('sha256').update(codexHome).digest('hex') 來產生固定長度的雜湊值作為檔名的一部分,既安全又保證長度可控。

**嚴重等級**:🟡 警告 **審查員**:Rogue **問題**:在 setupAuth 中,`lockPath` 檔名產生使用了 `Buffer.from(codexHome).toString("hex")`。如果 `codexHome` 非常長,這個檔名可能會超過作業系統的檔案名稱長度限制(通常為 255 bytes),導致鎖定失敗,進而阻斷整個流程。 **建議**:改用 `crypto.createHash('sha256').update(codexHome).digest('hex')` 來產生固定長度的雜湊值作為檔名的一部分,既安全又保證長度可控。
const timeout = setTimeout(() => {
child.kill("SIGTERM");
appendOutput(Buffer.from(`Codex execution timed out after ${timeoutMs} ms.\n`));
output.append(Buffer.from(`Codex execution timed out after ${timeoutMs} ms.\n`));
}, timeoutMs);
child.stdout.pipe(process.stdout);
child.stderr.pipe(process.stdout);
child.stdout.on("data", (chunk) => {
if (!truncateOutput(outputChunks, chunk, outputLimitBytes)) {
outputTruncated = true;
}
output.append(chunk);
Ghost marked this conversation as resolved
Review

嚴重等級🟡 警告
審查員:Mage
問題:在 setupAuth 中,在 fs.copyFileSync(authFile, authPath) 後立即 removeIfCreated(authFile),但在這期間如果發生 process 中斷,auth.json 可能會以不安全的權限(預設)或不完整的狀態寫入。
建議:建議使用 fs.renameSync 或在完成寫入與權限設定後再進行清理,並確保寫入過程中發生異常時能正確刪除該部分寫入的檔案。

**嚴重等級**:🟡 警告 **審查員**:Mage **問題**:在 `setupAuth` 中,在 `fs.copyFileSync(authFile, authPath)` 後立即 `removeIfCreated(authFile)`,但在這期間如果發生 process 中斷,auth.json 可能會以不安全的權限(預設)或不完整的狀態寫入。 **建議**:建議使用 `fs.renameSync` 或在完成寫入與權限設定後再進行清理,並確保寫入過程中發生異常時能正確刪除該部分寫入的檔案。
});
child.stderr.on("data", (chunk) => {
if (!truncateOutput(outputChunks, chunk, outputLimitBytes)) {
outputTruncated = true;
}
output.append(chunk);
});
child.on("error", (error) => {
clearTimeout(timeout);
const output = appendOutput(Buffer.from(`${error.message}\n`));
resolve({ status: 1, output });
const message = error.code === "ENOENT" ? "Unable to find codex command.\n" : `${error.message}\n`;
output.append(Buffer.from(message));
resolve({ status: 1, output: output.toString() });
});
child.on("close", (code) => {
child.on("close", (code, signal) => {
clearTimeout(timeout);
const truncationMessage = outputTruncated ? "\n[Output truncated]\n" : "";
const output = `${Buffer.concat(outputChunks).toString()}${truncationMessage}`;
resolve({ status: code ?? 1, output });
if (code === null && signal) {
output.append(Buffer.from(`Codex process terminated by signal ${signal}.\n`));
}
resolve({ status: code ?? 1, output: output.toString() });
});
});
}
1
@@ -243,6 +258,8 @@ function readConfig() {
function setupAuth(oauth, codexHome) {
try {
fs.mkdirSync(codexHome, { recursive: true, mode: DIR_MODE_PRIVATE });
fs.chmodSync(codexHome, DIR_MODE_PRIVATE);
fs.accessSync(codexHome, fs.constants.R_OK | fs.constants.W_OK | fs.constants.X_OK);
} catch {
fail("Unable to create CODEX_HOME.");
}
1