處理 AI review findings 並改寫 Node.js entrypoint #1
@@ -10,6 +10,8 @@ const FILE_MODE_PRIVATE = 0o600;
|
||||
const DIR_MODE_PRIVATE = 0o700;
|
||||
const DEFAULT_CODEX_TIMEOUT_MS = 30 * 60 * 1000;
|
||||
|
Ghost marked this conversation as resolved
|
||||
const DEFAULT_OUTPUT_LIMIT_BYTES = 1024 * 1024;
|
||||
const TEMP_DIR_PREFIX = ".codex-action-";
|
||||
const OUTPUT_DELIMITER_PREFIX = "CODEX_OUTPUT_";
|
||||
|
||||
class TempFileRegistry {
|
||||
constructor() {
|
||||
@@ -92,7 +94,7 @@ function cleanup() {
|
||||
}
|
||||
|
||||
function makeTempDir(dir) {
|
||||
const tempDir = fs.mkdtempSync(path.join(dir, ".codex-action-"));
|
||||
const tempDir = fs.mkdtempSync(path.join(dir, TEMP_DIR_PREFIX));
|
||||
fs.chmodSync(tempDir, DIR_MODE_PRIVATE);
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Leo
**問題**:在 `runCodex` 中使用了 `--dangerously-bypass-approvals-and-sandbox`,這類高風險標記若缺乏適當的說明,未來的維護者可能不清楚其安全意義而誤用或引發風險。
**建議**:建議在 `spawn` 呼叫前加上明確的註解,詳細說明為何在此環境中必須繞過沙盒,以及相關的安全考量。
|
||||
tempFiles.trackDir(tempDir);
|
||||
return tempDir;
|
||||
@@ -115,7 +117,7 @@ function appendGithubOutput(status, output) {
|
||||
|
||||
let delimiter;
|
||||
do {
|
||||
delimiter = `CODEX_OUTPUT_${crypto.randomBytes(12).toString("hex")}`;
|
||||
delimiter = `${OUTPUT_DELIMITER_PREFIX}${crypto.randomBytes(12).toString("hex")}`;
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Bard
**問題**:validateAuth 函式中對於 Base64 的正規化與驗證邏輯混雜在一起,使用了大量的取代與判斷,讀起來節奏凌亂,缺乏優雅感。
**建議**:將驗證邏輯與基礎轉換邏輯抽離,建議提取一個輔助函式專門負責 Base64 格式檢查,使主要流程清晰明瞭。
|
||||
} while (output.includes(delimiter));
|
||||
|
||||
fs.appendFileSync(
|
||||
@@ -132,22 +134,27 @@ function fail(message, code = 1) {
|
||||
process.exit(code);
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🔵 建議 **嚴重等級**:🔵 建議
**審查員**:Maya
**問題**:測試檔案 `tests/entrypoint_test.sh` 有測試 `missing_codex_command`,這很好。但實作中對於 `codex` 執行失敗的各種細節(如權限不足、找不到 binary 等)都統一處理為 `status: 1` 和簡單的訊息,測試僅驗證了 failure 狀態,未驗證具體錯誤來源。
**建議**:考慮在 `main.js` 中根據不同的錯誤類型回傳更細緻的 status code,並在測試中驗證這些 code,能更精確地協助 CI 使用者除錯。
gitea-actions
commented
嚴重等級:🔵 建議 **嚴重等級**:🔵 建議
**審查員**:Maya
**問題**:對於 codex 執行失敗的各種細節(權限不足、找不到 binary 等)都統一處理為 status: 1,測試未驗證具體錯誤來源。
**建議**:根據錯誤類型回傳細緻 status code,並在測試中驗證這些 code。
|
||||
}
|
||||
|
||||
function normalizeBase64(value) {
|
||||
return value.replace(/\s+/g, "").replace(/=+$/, "");
|
||||
function compactBase64(value) {
|
||||
return value.replace(/\s+/g, "");
|
||||
}
|
||||
|
||||
function isBase64(value) {
|
||||
const normalized = compactBase64(value);
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Rogue
**問題**:validateAuth 中重複執行 decoded.toString('base64') 並進行 normalizedBase64 處理,極度浪費資源。
**建議**:移除這些無謂的重新編碼比較,直接嘗試解碼。
|
||||
const paddingIndex = normalized.indexOf("=");
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Assassin
**問題**:將 `OAUTH` 環境變數內容解碼並直接寫入 `auth.json`。雖然有檢查 base64 格式與 JSON 結構,但若解碼後的 JSON 內容包含惡意配置(如惡意插件路徑或偽造的 API 憑證),可能導致後續 `codex` CLI 在執行時被劫持或洩漏資料。
**建議**:除了驗證 JSON 結構外,應進一步驗證 `auth.json` 內的欄位是否符合預期格式,並限制其檔案權限為 `600`(已做),確保容器內其他行程無法讀取。
|
||||
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Leo
**問題**:`main` 函式過於龐大且職責過多,它同時負責了訊號處理、路徑創建、檔案鎖定、認證驗證以及執行核心邏輯,這降低了程式碼的可讀性與單元測試的困難度。
**建議**:建議將 `main` 拆分為 `validateInput`、`setupAuth`、`runCodexAction` 與 `cleanup` 等子函式,讓職責分離。
|
||||
if (!normalized || normalized.length % 4 === 1 || !/^[A-Za-z0-9+/]*={0,2}$/.test(normalized)) {
|
||||
return false;
|
||||
}
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Maya
**問題**:在 `runCodex` 函數中雖然有處理 `child.on('error', ...)`,但若 `codex` 指令本身不存在(spawn ENOENT),這裡捕捉到的 error stack trace 可能會包含完整的系統路徑資訊,這在 CI 環境中屬於資訊洩漏風險。
**建議**:建議在錯誤處理中,針對 `error.code === 'ENOENT'` 做明確判斷,回傳簡潔的錯誤訊息(例如「找不到 codex 指令」),而非直接回傳完整的 `error.message`。
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Maya
**問題**:spawn ENOENT 錯誤處理可能洩漏系統路徑資訊。
**建議**:針對 error.code === 'ENOENT' 做明確判斷,回傳簡潔錯誤訊息而非完整 stack trace。
|
||||
|
||||
return paddingIndex === -1 || /^=+$/.test(normalized.slice(paddingIndex));
|
||||
}
|
||||
|
||||
function validateAuth(encodedAuth, authFile) {
|
||||
const decoded = Buffer.from(encodedAuth, "base64");
|
||||
const normalizedDecoded = normalizeBase64(decoded.toString("base64"));
|
||||
const normalizedInput = normalizeBase64(encodedAuth);
|
||||
|
||||
if (decoded.length === 0 && normalizedInput.length > 0) {
|
||||
if (!isBase64(encodedAuth)) {
|
||||
fail("OAUTH must be valid base64 encoded Codex auth.json.");
|
||||
}
|
||||
|
||||
if (normalizedDecoded !== normalizedInput) {
|
||||
fail("OAUTH must be valid base64 encoded Codex auth.json.");
|
||||
}
|
||||
const decoded = Buffer.from(compactBase64(encodedAuth), "base64");
|
||||
|
||||
fs.writeFileSync(authFile, decoded, { mode: FILE_MODE_PRIVATE });
|
||||
|
||||
@@ -168,26 +175,33 @@ function parsePositiveInteger(value, fallback) {
|
||||
return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback;
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Mage
**問題**:在 `child.on('close', ...)` 事件中,使用 `Buffer.concat(outputChunks).toString()` 將所有輸出轉為單一字串。如果 `outputChunks` 總大小接近 `DEFAULT_OUTPUT_LIMIT_BYTES` (1MB),這會導致瞬間記憶體使用量增加,且對於極大輸出,字串轉換本身亦有潛在的負載。
**建議**:考慮使用 `Buffer` 處理後續輸出,或在達到 `outputLimitBytes` 時,僅保存 `Buffer` 片段即可,不必轉為大字串。
|
||||
}
|
||||
|
||||
function readExecutionConfig() {
|
||||
return {
|
||||
timeoutMs: parsePositiveInteger(process.env.CODEX_TIMEOUT_MS, DEFAULT_CODEX_TIMEOUT_MS),
|
||||
outputLimitBytes: parsePositiveInteger(process.env.CODEX_OUTPUT_LIMIT_BYTES, DEFAULT_OUTPUT_LIMIT_BYTES),
|
||||
workspace: process.env.GITHUB_WORKSPACE || process.cwd(),
|
||||
};
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Bard
**問題**:spawn 函式參數陣列過長且散亂,閱讀性較差。
**建議**:將參數拆分為數組變數並展開傳遞。
|
||||
}
|
||||
|
||||
function codexExecArgs(model, prompt) {
|
||||
return [
|
||||
"exec",
|
||||
"--dangerously-bypass-approvals-and-sandbox",
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🔴 嚴重 **嚴重等級**:🔴 嚴重
**審查員**:Mage
**問題**:在 `setupAuth` 中,`lockPath` 使用 `os.tmpdir()`。在共享環境中,如果 `CODEX_HOME` 字串相同,會導致所有 process 競爭同一個鎖檔,且如果其他無關的 process 也剛好在 `os.tmpdir()` 中建立相同名稱的檔案,會導致誤判或鎖定失敗。
**建議**:應在 `CODEX_HOME` 內部建立鎖檔,而非使用全域的 `os.tmpdir()`,或者包含更具唯一性的識別碼(如 PID 或更長的路徑雜湊)以確保鎖的隔離性。
gitea-actions
commented
嚴重等級:🔴 嚴重 **嚴重等級**:🔴 嚴重
**審查員**:Maya
**問題**:Codex 超時處理路徑未經測試,無法確保 SIGTERM 能成功發送與訊息正確產出。
**建議**:增加測試案例模擬長期睡眠(如 sleep 10),驗證超時機制與輸出訊息。
|
||||
"--skip-git-repo-check",
|
||||
"--model",
|
||||
model,
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Rogue
**問題**:在 setupAuth 中,`lockPath` 檔名產生使用了 `Buffer.from(codexHome).toString("hex")`。如果 `codexHome` 非常長,這個檔名可能會超過作業系統的檔案名稱長度限制(通常為 255 bytes),導致鎖定失敗,進而阻斷整個流程。
**建議**:改用 `crypto.createHash('sha256').update(codexHome).digest('hex')` 來產生固定長度的雜湊值作為檔名的一部分,既安全又保證長度可控。
|
||||
prompt,
|
||||
];
|
||||
}
|
||||
|
||||
function runCodex(model, prompt) {
|
||||
return new Promise((resolve) => {
|
||||
const timeoutMs = parsePositiveInteger(process.env.CODEX_TIMEOUT_MS, DEFAULT_CODEX_TIMEOUT_MS);
|
||||
const outputLimitBytes = parsePositiveInteger(process.env.CODEX_OUTPUT_LIMIT_BYTES, DEFAULT_OUTPUT_LIMIT_BYTES);
|
||||
const workspace = process.env.GITHUB_WORKSPACE || process.cwd();
|
||||
const { timeoutMs, outputLimitBytes, workspace } = readExecutionConfig();
|
||||
const args = codexExecArgs(model, prompt);
|
||||
|
||||
// This Docker Action runs inside an ephemeral CI container where Codex must be
|
||||
// able to edit the checked-out workspace without interactive approvals.
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🟡 警告 **嚴重等級**:🟡 警告
**審查員**:Mage
**問題**:在 `setupAuth` 中,在 `fs.copyFileSync(authFile, authPath)` 後立即 `removeIfCreated(authFile)`,但在這期間如果發生 process 中斷,auth.json 可能會以不安全的權限(預設)或不完整的狀態寫入。
**建議**:建議使用 `fs.renameSync` 或在完成寫入與權限設定後再進行清理,並確保寫入過程中發生異常時能正確刪除該部分寫入的檔案。
|
||||
const child = spawn(
|
||||
"codex",
|
||||
[
|
||||
"exec",
|
||||
"--dangerously-bypass-approvals-and-sandbox",
|
||||
"--skip-git-repo-check",
|
||||
"--model",
|
||||
model,
|
||||
prompt,
|
||||
],
|
||||
{ cwd: workspace, stdio: ["ignore", "pipe", "pipe"] },
|
||||
);
|
||||
const child = spawn("codex", args, { cwd: workspace, stdio: ["ignore", "pipe", "pipe"] });
|
||||
|
||||
const output = new OutputCollector(outputLimitBytes);
|
||||
|
||||
@@ -255,6 +269,19 @@ function readConfig() {
|
||||
return { oauth, model, codexHome, prompt };
|
||||
}
|
||||
|
||||
function createAuthLock(codexHome) {
|
||||
const lockName = crypto.createHash("sha256").update(codexHome).digest("hex");
|
||||
const lockPath = path.join(codexHome, `.codex-auth-${lockName}.lock`);
|
||||
|
||||
try {
|
||||
const lockHandle = fs.openSync(lockPath, "wx", FILE_MODE_PRIVATE);
|
||||
tempFiles.trackFile(lockPath);
|
||||
return lockHandle;
|
||||
} catch {
|
||||
fail("Unable to lock Codex auth.json.");
|
||||
}
|
||||
}
|
||||
|
Ghost marked this conversation as resolved
gitea-actions
commented
嚴重等級:🔵 建議 **嚴重等級**:🔵 建議
**審查員**:Bard
**問題**:setupAuth 函式職責過於繁雜。
**建議**:將鎖定機制與驗證機制拆分為獨立輔助函式。
|
||||
|
||||
function setupAuth(oauth, codexHome) {
|
||||
try {
|
||||
fs.mkdirSync(codexHome, { recursive: true, mode: DIR_MODE_PRIVATE });
|
||||
@@ -267,16 +294,7 @@ function setupAuth(oauth, codexHome) {
|
||||
const tempDir = makeTempDir(codexHome);
|
||||
const authFile = makeTempFile(tempDir, "auth");
|
||||
const authPath = path.join(codexHome, "auth.json");
|
||||
const lockName = crypto.createHash("sha256").update(codexHome).digest("hex");
|
||||
const lockPath = path.join(codexHome, `.codex-auth-${lockName}.lock`);
|
||||
|
||||
let lockHandle;
|
||||
try {
|
||||
lockHandle = fs.openSync(lockPath, "wx", FILE_MODE_PRIVATE);
|
||||
tempFiles.trackFile(lockPath);
|
||||
} catch {
|
||||
fail("Unable to lock Codex auth.json.");
|
||||
}
|
||||
const lockHandle = createAuthLock(codexHome);
|
||||
|
||||
validateAuth(oauth, authFile);
|
||||
|
||||
|
||||
嚴重等級:🔵 建議
審查員:Bard
問題:全域變數
createdPaths在檔案層級被宣告,讓函式產生強依賴,缺乏封裝性,讀起來不夠優雅。建議:將臨時檔案管理邏輯封裝成一個類別(如
TempFileRegistry),讓狀態更具備物件導向的封裝性。