ci: 添加 Codex PR 交互审阅流程#28
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
此拉取请求引入了基于 node-pty 的交互式终端测试脚本 scripts/codex-interactive-check.mjs 及其单元测试，用于验证 CLI 的主菜单和工具菜单的键盘导航行为。代码审查发现了数个关键问题：首先，node-pty 的 IPty 实例并不包含 exitCode 属性，导致进程退出状态判断失效并总是触发 child.kill()；其次，若子进程在等待退出前已结束，监听 onExit 可能会导致不必要的 5 秒超时挂起；此外，在 CI 等受限环境中 fs.chmodSync 可能会因权限不足崩溃，且测试中途退出时 waitForTranscript 缺乏快速失败机制；最后，脚本运行结束后未清理创建的临时配置目录。建议针对这些进程生命周期管理、异常处理及资源清理问题进行修复。整体变更影响范围局限于测试脚本，修复上述问题后可安全合并。置信度评分：4/5。
| async function runScenario(pty, scenario, homeDir) { | ||
| const transcript = []; | ||
| const child = pty.spawn(process.execPath, ['dist/cli.js', ...scenario.args], { | ||
| name: 'xterm-256color', | ||
| cols: 100, | ||
| rows: 30, | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| HOME: homeDir, | ||
| LANG: 'zh_CN.UTF-8', | ||
| TERM: 'xterm-256color', | ||
| NO_COLOR: '1', | ||
| }, | ||
| }); | ||
|
|
||
| child.onData((data) => { | ||
| transcript.push(data); | ||
| }); | ||
|
|
||
| try { | ||
| for (const step of scenario.steps) { | ||
| await waitForTranscript(transcript, step.waitFor, step.timeoutMs || DEFAULT_TIMEOUT_MS); | ||
| if (step.input) { | ||
| child.write(step.input); | ||
| } | ||
| if (step.pauseMs) { | ||
| await delay(step.pauseMs); | ||
| } | ||
| } | ||
| await waitForExitOrKill(child, scenario.exitTimeoutMs || 5000); | ||
| } finally { | ||
| if (child.exitCode === undefined) { | ||
| child.kill(); | ||
| } | ||
| } | ||
|
|
||
| const output = transcript.join(''); | ||
| const analysis = analyzeTranscript(output, scenario.analysis); | ||
| return { | ||
| name: scenario.name, | ||
| output, | ||
| analysis, | ||
| }; | ||
| } |
There was a problem hiding this comment.
问题分析：
node-pty的IPty实例并没有exitCode属性（exitCode仅在onExit事件回调中提供）。因此child.exitCode === undefined永远为true，导致finally块中总是会调用child.kill()，即使进程已经退出。- 如果在执行步骤期间发生超时或异常，错误会直接抛出导致整个
main()挂掉，无法生成完整的测试报告。
解决方案：
引入一个 state 对象来实时跟踪进程的退出状态，并在 finally 中安全释放订阅和清理进程。同时捕获执行过程中的异常，将其记录到报告中而不是直接崩溃。
async function runScenario(pty, scenario, homeDir) {
const transcript = [];
const child = pty.spawn(process.execPath, ["dist/cli.js", ...scenario.args], {
name: "xterm-256color",
cols: 100,
rows: 30,
cwd: process.cwd(),
env: {
...process.env,
HOME: homeDir,
LANG: "zh_CN.UTF-8",
TERM: "xterm-256color",
NO_COLOR: "1",
},
});
const state = { exited: false };
const exitSub = child.onExit(() => {
state.exited = true;
});
child.onData((data) => {
transcript.push(data);
});
let errorOccurred = null;
try {
for (const step of scenario.steps) {
await waitForTranscript(transcript, step.waitFor, step.timeoutMs || DEFAULT_TIMEOUT_MS, state);
if (step.input) {
child.write(step.input);
}
if (step.pauseMs) {
await delay(step.pauseMs);
}
}
await waitForExitOrKill(child, state, scenario.exitTimeoutMs || 5000);
} catch (err) {
errorOccurred = err;
} finally {
exitSub.dispose();
if (!state.exited) {
child.kill();
}
}
const output = transcript.join("");
const analysis = analyzeTranscript(output, scenario.analysis);
if (errorOccurred) {
analysis.ok = false;
analysis.failures.push(errorOccurred.message);
}
return {
name: scenario.name,
output,
analysis,
};
}| function waitForExitOrKill(child, timeoutMs) { | ||
| return new Promise((resolve) => { | ||
| let settled = false; | ||
| const timer = setTimeout(() => { | ||
| if (!settled) { | ||
| child.kill(); | ||
| settled = true; | ||
| resolve(); | ||
| } | ||
| }, timeoutMs); | ||
|
|
||
| child.onExit(() => { | ||
| if (!settled) { | ||
| clearTimeout(timer); | ||
| settled = true; | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
问题分析：
如果子进程在 waitForExitOrKill 被调用前就已经退出，由于 onExit 事件已经触发过，重新在 waitForExitOrKill 中监听 onExit 将永远不会触发，导致脚本无谓地等待满 5 秒超时。
解决方案：
传入 state 状态对象，在开始时先检查 state.exited，如果已经退出则直接 resolve 避免超时等待。
function waitForExitOrKill(child, state, timeoutMs) {
return new Promise((resolve) => {
if (state.exited) {
resolve();
return;
}
let settled = false;
const timer = setTimeout(() => {
if (!settled) {
child.kill();
settled = true;
resolve();
}
}, timeoutMs);
const sub = child.onExit(() => {
if (!settled) {
clearTimeout(timer);
settled = true;
sub.dispose();
resolve();
}
});
});
}| function ensureNodePtySpawnHelperExecutable() { | ||
| const require = createRequire(import.meta.url); | ||
| const helperPath = resolveNodePtySpawnHelperPath(require.resolve('node-pty')); | ||
| if (fs.existsSync(helperPath)) { | ||
| fs.chmodSync(helperPath, 0o755); | ||
| } | ||
| } |
There was a problem hiding this comment.
问题分析：
在某些权限受限或只读文件系统的 CI 运行环境中，fs.chmodSync 可能会因为权限不足而抛出异常，导致整个交互式检查脚本崩溃。
解决方案：
建议将 fs.chmodSync 逻辑包裹在 try...catch 块中，捕获并忽略此类非致命错误，以确保脚本在各种环境下的健壮性。
function ensureNodePtySpawnHelperExecutable() {
try {
const require = createRequire(import.meta.url);
const helperPath = resolveNodePtySpawnHelperPath(require.resolve("node-pty"));
if (fs.existsSync(helperPath)) {
fs.chmodSync(helperPath, 0o755);
}
} catch (err) {
// 忽略权限或只读文件系统导致的错误，避免阻碍脚本运行
}
}| function waitForTranscript(transcript, expected, timeoutMs) { | ||
| const startedAt = Date.now(); | ||
| return new Promise((resolve, reject) => { | ||
| const timer = setInterval(() => { | ||
| const clean = stripAnsi(transcript.join('')); | ||
| if (clean.includes(expected)) { | ||
| clearInterval(timer); | ||
| resolve(); | ||
| return; | ||
| } | ||
| if (Date.now() - startedAt > timeoutMs) { | ||
| clearInterval(timer); | ||
| reject(new Error(`Timed out waiting for terminal output: ${expected}`)); | ||
| } | ||
| }, 50); | ||
| }); | ||
| } |
There was a problem hiding this comment.
问题分析：
如果 CLI 进程在交互过程中提前退出或崩溃，waitForTranscript 仍然会持续轮询直到 15 秒超时，这会导致 CI 任务无谓地挂起并极大地延长反馈周期。
解决方案：
传入 state 状态对象，在轮询中检测到进程已退出时立即抛出异常，实现快速失败。
function waitForTranscript(transcript, expected, timeoutMs, state) {
const startedAt = Date.now();
return new Promise((resolve, reject) => {
const timer = setInterval(() => {
const clean = stripAnsi(transcript.join(""));
if (clean.includes(expected)) {
clearInterval(timer);
resolve();
return;
}
if (state && state.exited) {
clearInterval(timer);
reject(new Error("Process exited prematurely while waiting for: " + expected));
return;
}
if (Date.now() - startedAt > timeoutMs) {
clearInterval(timer);
reject(new Error("Timed out waiting for terminal output: " + expected));
}
}, 50);
});
}| async function main() { | ||
| ensureNodePtySpawnHelperExecutable(); | ||
| const { default: pty } = await import('node-pty'); | ||
| const { toolManager } = await import('../dist/lib/tool-manager.js'); | ||
| const tools = toolManager.getAll().map((tool) => ({ | ||
| name: tool.name, | ||
| displayName: tool.displayName, | ||
| })); | ||
| const homeDir = createHomeFixture(); | ||
| const results = []; | ||
|
|
||
| for (const scenario of buildScenarios(tools)) { | ||
| results.push(await runScenario(pty, scenario, homeDir)); | ||
| } | ||
|
|
||
| const reportPath = process.env.CODEX_INTERACTIVE_REPORT || DEFAULT_REPORT_PATH; | ||
| fs.writeFileSync(reportPath, formatReport(results)); | ||
|
|
||
| const failures = results.flatMap((result) => result.analysis.failures.map((failure) => `${result.name}: ${failure}`)); | ||
| if (failures.length) { | ||
| console.error(failures.join('\n')); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
问题分析：
createHomeFixture 会在系统的临时目录中创建一个临时配置目录，但脚本运行结束后从未对其进行清理。这会导致在本地或持久化 CI 节点上多次运行时残留大量无用的临时目录。
解决方案：
使用 try...finally 结构，在脚本执行完毕后使用 fs.rmSync 强制递归清理临时目录。
async function main() {
ensureNodePtySpawnHelperExecutable();
const { default: pty } = await import("node-pty");
const { toolManager } = await import("../dist/lib/tool-manager.js");
const tools = toolManager.getAll().map((tool) => ({
name: tool.name,
displayName: tool.displayName,
}));
const homeDir = createHomeFixture();
try {
const results = [];
for (const scenario of buildScenarios(tools)) {
results.push(await runScenario(pty, scenario, homeDir));
}
const reportPath = process.env.CODEX_INTERACTIVE_REPORT || DEFAULT_REPORT_PATH;
fs.writeFileSync(reportPath, formatReport(results));
const failures = results.flatMap((result) => result.analysis.failures.map((failure) => result.name + ": " + failure));
if (failures.length) {
console.error(failures.join("\n"));
process.exit(1);
}
} finally {
try {
fs.rmSync(homeDir, { recursive: true, force: true });
} catch (err) {
// 忽略清理临时目录时的错误
}
}
}
Codex 评审结论：需要关注 发现的问题
交互影响评估 本 PR 没有改动 CLI 源码、菜单实现、工具适配器或配置写入逻辑；主要影响是新增 CI review workflow、真实伪终端交互检查脚本和对应测试。 测试/验证
|
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
d1696fd to
1a3da33
Compare
Codex 评审结论：需要关注 问题
交互影响评估 本 PR 未直接修改 CLI 命令、向导 flow、菜单实现、工具适配器或配置写入逻辑；新增的是 CI workflow、真实伪终端交互检查脚本和测试。 验证
|
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
1a3da33 to
5316b76
Compare
github-actions
Bot
added
the
codex: needs attention
Codex 评审结论：需要关注 发现的问题
验证说明
|
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
5316b76 to
9365d3f
Compare
github-actions
Bot
added
codex: needs attention
Codex 评审结论：需要关注 发现的问题
交互与覆盖说明
配置写入方面，脚本对子进程设置了临时 验证
|
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
9365d3f to
076b8b0
Compare
github-actions
Bot
removed
the
codex: needs attention
Codex 评审结论：通过 发现的问题 未发现本 PR 引入的阻塞性问题。 本次变更主要影响 交互报告显示主菜单、工具选择器，以及动态发现到的 验证说明
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| with: | ||
| openai-api-key: ${{ secrets.QNAIGC_API_KEY }} | ||
| responses-api-endpoint: https://api.qnaigc.com/bypass/openai/v1/responses | ||
| model: openai/gpt-5.5 |
There was a problem hiding this comment.
[security] 这里把 safety-strategy 设为 read-only，同时把 PR 标题/正文/diff 这类 PR 可控内容喂给 Codex 并传入 QNAIGC_API_KEY。read-only 只限制文件写入/网络，并不会像默认 drop-sudo 那样降权；一旦模型被提示注入诱导输出本地敏感信息，secret 暴露面会变大。建议保留 sandbox: read-only，但改用默认 drop-sudo（或单独的非特权用户）来运行 Codex。
| "Codex 评审未生成反馈，请查看 workflow 日志了解详情。", | ||
| ].join("\n"); | ||
|
|
||
| await github.rest.issues.createComment({ |
There was a problem hiding this comment.
[quality] 每次 opened/synchronize 都会直接 createComment，同一个 PR 多次 push 后会累积多条 Codex 评审评论。建议给评论加稳定标记并优先查找/更新上一条 bot 评论，找不到时再创建，避免 PR 讨论区被自动评审刷屏。
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
076b8b0 to
84a4579
Compare
Codex 评审结论：通过 发现的问题 未发现本 PR 引入的具体 bug、行为回归、安全风险、数据丢失风险或 CLI 交互回归。 本次变更主要影响 CI 审阅流程和交互检查脚本： 交互与配置隔离说明
配置隔离方面，交互脚本为伪终端子进程设置临时 验证
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
此拉取请求引入了交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其单元测试 tests/codex-interactive-check.test.mjs，用于对 CLI 菜单和工具选择进行自动化交互测试。为此，项目新增了 node-pty 依赖，并在 .gitignore 中忽略了生成的报告文件。受影响的模块主要包括测试脚本和包配置。在合并安全性方面，目前存在一些潜在的运行时和兼容性风险：首先，由于 node-pty 的 IPty 实例没有公共的 exitCode 属性，直接调用 child.kill() 可能会在已退出的进程上触发 ESRCH 异常导致测试崩溃；其次，DISABLE_FETCH_IMPORT 将 fetch 拦截为永远 pending 的 Promise，可能导致测试在遇到网络请求时无限期挂起；最后，在只读文件系统中对 node_modules 下的 spawn-helper 执行 fs.chmodSync 会因权限不足而报错。基于上述发现，当前合并的置信度评分为 3/5。阻碍合并的主要原因为 child.kill() 潜在的崩溃风险以及只读环境下的兼容性问题，需要对这些调用增加 try-catch 容错处理。
| if (child.exitCode === undefined) { | ||
| child.kill(); | ||
| } |
There was a problem hiding this comment.
在 node-pty 中，IPty 实例并没有公共的 exitCode 属性（该属性始终为 undefined）。因此，无论子进程是否已经正常退出，finally 块中的 child.kill() 都会被执行。
如果子进程已经退出，调用 child.kill() 会在 Unix 系统上抛出 ESRCH 异常，导致整个测试脚本崩溃并掩盖真实的测试结果。
建议修复：
使用 try-catch 包裹 child.kill() 调用，捕获并忽略进程已退出导致的错误，以增强防御性编程。
try {
child.kill();
} catch (error) {
// 忽略进程已退出导致的 kill 失败
}References
- Rule 6: Severity levels for inline comments. P1 means the change can cause build failures, runtime exceptions... Do not mark pure style preferences as P1 or P2. (link)
|
|
||
| const DEFAULT_TIMEOUT_MS = 15000; | ||
| const DEFAULT_REPORT_PATH = 'codex-interactive-report.md'; | ||
| const DISABLE_FETCH_IMPORT = 'data:text/javascript,globalThis.fetch%3D()%3D%3Enew%20Promise(()%3D%3E{})%3B'; |
There was a problem hiding this comment.
目前 DISABLE_FETCH_IMPORT 将 globalThis.fetch 替换为一个永远处于 pending 状态的 Promise (() => new Promise(() => {}))。
如果 CLI 代码中存在任何被 await 的 fetch 调用（例如未来新增的工具初始化或更新逻辑），这会导致测试脚本无限期挂起，直到触发 15 秒的全局超时。
建议修复：
将 fetch 替换为立即拒绝（reject）的 Promise，这样任何网络请求都会立即失败并走入 CLI 的错误处理流程，实现快速失败（fail-fast），避免不必要的挂起。
| const DISABLE_FETCH_IMPORT = 'data:text/javascript,globalThis.fetch%3D()%3D%3Enew%20Promise(()%3D%3E{})%3B'; | |
| const DISABLE_FETCH_IMPORT = 'data:text/javascript,globalThis.fetch%3D()%3D%3EPromise.reject(new%20TypeError(\'Network%20offline\'))%3B'; |
References
- Rule 6: Severity levels for inline comments. P2 means test gaps, edge cases, compatibility risks, or paths likely to regress later. (link)
| function ensureNodePtySpawnHelperExecutable() { | ||
| const require = createRequire(import.meta.url); | ||
| const helperPath = resolveNodePtySpawnHelperPath(require.resolve('node-pty')); | ||
| if (fs.existsSync(helperPath)) { | ||
| fs.chmodSync(helperPath, 0o755); | ||
| } | ||
| } |
There was a problem hiding this comment.
在某些严格的 CI 环境或只读文件系统（例如 Nix 环境或受限的容器沙箱）中，node_modules 目录可能是只读的。此时调用 fs.chmodSync 会抛出 EROFS 或 EACCES 权限错误，导致整个测试流程中断。
建议修复：
使用 try-catch 包裹 fs.chmodSync 调用，捕获并忽略权限错误，以提高脚本在不同环境下的兼容性。
function ensureNodePtySpawnHelperExecutable() {
const require = createRequire(import.meta.url);
try {
const helperPath = resolveNodePtySpawnHelperPath(require.resolve('node-pty'));
if (fs.existsSync(helperPath)) {
fs.chmodSync(helperPath, 0o755);
}
} catch (error) {
// 忽略只读文件系统或权限不足导致的错误
}
}References
- Rule 6: Severity levels for inline comments. P2 means test gaps, edge cases, compatibility risks, or paths likely to regress later. (link)
krisxia0506
force-pushed
the
codex/add-pr-review
branch
from
84a4579 to
587dbb6
Compare
Codex 评审结论：通过 发现的问题 未发现需要阻断的问题。 本次 PR 主要新增 Codex PR Review workflow、真实伪终端交互检查脚本、 从 验证说明
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
此拉取请求引入了一个新的交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其对应的单元测试 tests/codex-interactive-check.test.mjs，用于对 CLI 工具进行自动化交互场景测试。为此，项目在 package.json 中新增了 test:interactive 脚本，并引入了 node-pty 依赖。审查中发现了几个需要改进的问题：首先，resolveNodePtySpawnHelperPath 的单元测试在 Windows 环境下由于路径分隔符差异会导致断言失败；其次，在 waitForExitOrKill 函数中，exitSubscription 变量在未完全初始化前就被其内部的异步回调引用，存在潜在的 ReferenceError 风险；最后，脚本中使用的 --import 参数在较低版本的 Node.js 环境中可能不被支持，存在兼容性风险。整体修改方向正确，但在跨平台兼容性与变量初始化安全性上仍需优化，建议修复后合并。
| test('resolveNodePtySpawnHelperPath points from node-pty main file to platform helper', () => { | ||
| const helperPath = resolveNodePtySpawnHelperPath('/repo/node_modules/node-pty/lib/index.js', 'darwin', 'arm64'); | ||
|
|
||
| assert.equal(helperPath, '/repo/node_modules/node-pty/prebuilds/darwin-arm64/spawn-helper'); | ||
| }); |
There was a problem hiding this comment.
触发条件
在 Windows 操作系统上运行单元测试时。
实际影响
resolveNodePtySpawnHelperPath 内部使用 path.join，在 Windows 上会输出带有反斜杠（\）的路径（例如 \repo\node_modules\...）。而测试中硬编码了正斜杠（/）的期望字符串，导致 assert.equal 断言失败，单元测试在 Windows 环境下无法通过。
为什么现有测试未捕获
当前的 CI 环境可能仅在 Linux (Ubuntu) 上运行，未在 Windows 上进行跨平台测试。
修复建议
在断言前，将路径中的反斜杠统一替换为正斜杠，或者使用 path.join 动态构建期望的路径。由于该测试文件未导入 path 模块，最简便且不引入新依赖的方法是在断言时对 helperPath 进行 .replace(/\\/g, '/') 转换。
| test('resolveNodePtySpawnHelperPath points from node-pty main file to platform helper', () => { | |
| const helperPath = resolveNodePtySpawnHelperPath('/repo/node_modules/node-pty/lib/index.js', 'darwin', 'arm64'); | |
| assert.equal(helperPath, '/repo/node_modules/node-pty/prebuilds/darwin-arm64/spawn-helper'); | |
| }); | |
| test('resolveNodePtySpawnHelperPath points from node-pty main file to platform helper', () => { | |
| const helperPath = resolveNodePtySpawnHelperPath('/repo/node_modules/node-pty/lib/index.js', 'darwin', 'arm64'); | |
| assert.equal(helperPath.replace(/\\/g, '/'), '/repo/node_modules/node-pty/prebuilds/darwin-arm64/spawn-helper'); | |
| }); |
| let settled = false; | ||
| const timer = setTimeout(() => { | ||
| if (!settled) { | ||
| safeKill(child); | ||
| settled = true; | ||
| exitSubscription.dispose(); | ||
| resolve({ timedOut: true, code: null, signal: null }); | ||
| } | ||
| }, timeoutMs); | ||
|
|
||
| const exitSubscription = child.onExit(({ exitCode, signal }) => { | ||
| if (!settled) { | ||
| clearTimeout(timer); | ||
| settled = true; | ||
| exitSubscription.dispose(); | ||
| resolve({ timedOut: false, code: exitCode, signal }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
触发条件
- 当
child.onExit在注册时因异常而失败，或者 - 在某些极端的模拟/测试环境下，
child.onExit同步触发了回调函数，或者 - 进程启动失败触发超时，
setTimeout回调执行时。
实际影响
在 setTimeout 和 child.onExit 的回调中，直接引用了尚未完成初始化的 const exitSubscription。
- 如果
child.onExit抛出异常，timer已经启动，超时触发时会因为exitSubscription未定义而抛出ReferenceError，导致未捕获的异常。 - 如果回调被同步触发，也会因为临时死区（TDZ）导致
ReferenceError。
修复建议
将 exitSubscription 声明为 let 并置于 setTimeout 之前，在调用 dispose() 前进行空值检查，确保安全。
let exitSubscription;
let settled = false;
const timer = setTimeout(() => {
if (!settled) {
safeKill(child);
settled = true;
if (exitSubscription) {
exitSubscription.dispose();
}
resolve({ timedOut: true, code: null, signal: null });
}
}, timeoutMs);
exitSubscription = child.onExit(({ exitCode, signal }) => {
if (!settled) {
clearTimeout(timer);
settled = true;
if (exitSubscription) {
exitSubscription.dispose();
}
resolve({ timedOut: false, code: exitCode, signal });
}
});| const child = pty.spawn(process.execPath, ['--import', DISABLE_FETCH_IMPORT, 'dist/cli.js', ...scenario.args], { | ||
| name: 'xterm-256color', | ||
| cols: 100, | ||
| rows: 30, | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| HOME: homeDir, | ||
| LANG: 'zh_CN.UTF-8', | ||
| TERM: 'xterm-256color', | ||
| NO_COLOR: '1', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
触发条件
在 Node.js 版本低于 18.19.0 或 20.6.0 的环境下运行该交互式检查脚本时。
实际影响
脚本中使用了 --import 参数和 data: 协议的 Data URL 来动态禁用 fetch。然而，--import 标志和对 Data URL 的支持是在 Node.js v18.19.0 和 v20.6.0 中才引入的。在更低版本的 Node.js 18 (例如 18.15.0) 中运行会直接报错 bad option: --import，导致脚本无法执行。这与 package.json 中声明的 `
Codex 评审结论：通过 发现的问题 未发现需要阻塞合入的问题。本次 PR 主要新增 Codex PR Review workflow、真实伪终端交互检查脚本、 交互风险检查
验证说明
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
本拉取请求引入了基于 node-pty 的交互式终端测试框架，新增了 scripts/codex-interactive-check.mjs 脚本及对应的单元测试，用于自动化验证 CLI 菜单的键盘导航功能。审查反馈提出了两项关键改进：一是将模拟的 fetch 从无限挂起的 Promise 改为立即拒绝，以实现测试的快速失败（fail-fast），避免 CI 超时；二是增加对子进程终止信号（signal）的检查，防止进程崩溃时测试静默通过。这些改进能显著提升测试的健壮性与诊断效率，整体变更结构清晰，在解决上述问题后即可安全合并。置信度评分：4/5。
| const DEFAULT_TIMEOUT_MS = 15000; | ||
| const DEFAULT_REPORT_PATH = 'codex-interactive-report.md'; | ||
| const DISABLE_FETCH_PRELOAD = 'disable-fetch.cjs'; | ||
| const DISABLE_FETCH_PRELOAD_SOURCE = 'globalThis.fetch = () => new Promise(() => {});\n'; |
There was a problem hiding this comment.
问题分析：
当前 DISABLE_FETCH_PRELOAD_SOURCE 将 globalThis.fetch 模拟为一个永远处于 pending 状态的 Promise (new Promise(() => {}))。
如果 CLI 在交互过程中触发了任何网络请求（例如检查更新、验证 API Key 等），该请求将无限期挂起。这会导致测试脚本在等待终端输出时触发 15 秒的硬超时（DEFAULT_TIMEOUT_MS），从而使 CI 运行缓慢，且输出的错误信息不够直观（只能看到超时，而看不到是哪个网络请求挂起）。
改进建议：
建议将 fetch 模拟为立即拒绝（reject）一个 TypeError（这符合 W3C Fetch 规范中网络失败时抛出 TypeError 的标准行为）。这样，当 CLI 发起网络请求时会立即失败，CLI 可以走现有的错误处理逻辑并优雅退出，或者直接抛出异常结束进程。这能让测试快速失败（fail-fast），节省 CI 时间，并提供更清晰的错误堆栈便于排查。
修改方案：
将 new Promise(() => {}) 替换为 Promise.reject(new TypeError("Network connections are disabled in interactive tests"))。
| const DISABLE_FETCH_PRELOAD_SOURCE = 'globalThis.fetch = () => new Promise(() => {});\n'; | |
| const DISABLE_FETCH_PRELOAD_SOURCE = 'globalThis.fetch = () => Promise.reject(new TypeError("Network connections are disabled in interactive tests"));\n'; |
| if (exitResult.code !== null && exitResult.code !== 0) { | ||
| analysis.failures.push(`Process exited with non-zero code ${exitResult.code}.`); | ||
| analysis.ok = false; | ||
| analysis.summary = analysis.failures.join('\n'); | ||
| } |
There was a problem hiding this comment.
问题分析：
在 runScenario 中，如果子进程由于信号（如 SIGSEGV、SIGABRT 或被意外杀死的 SIGKILL）异常终止，exitResult.code 通常会是 null，而 exitResult.signal 会包含对应的信号名称。
当前的逻辑仅检查了 exitResult.code !== null && exitResult.code !== 0。如果进程因信号崩溃，该检查会被跳过。如果此时预期的终端输出模式已经匹配完成，测试可能会错误地判定为通过（silent pass），从而漏掉严重的进程崩溃问题。
改进建议：
增加对 exitResult.signal 的检查。如果进程因信号终止，应将其记录为失败。
修改方案：
在检查 exitResult.code 的同时，也检查 exitResult.signal 是否存在。为了避免在建议代码中使用反引号（`），我们改用标准的字符串拼接。
if (exitResult.code !== null && exitResult.code !== 0) {
analysis.failures.push('Process exited with non-zero code ' + exitResult.code + '.');
analysis.ok = false;
analysis.summary = analysis.failures.join('\n');
} else if (exitResult.signal) {
analysis.failures.push('Process was terminated by signal ' + exitResult.signal + '.');
analysis.ok = false;
analysis.summary = analysis.failures.join('\n');
}
Codex 评审结论：通过 发现的问题 未发现需要阻塞合入的问题。本次 PR 主要新增 Codex PR Review workflow、真实伪终端交互检查脚本、 交互风险评估
配置隔离方面，交互脚本为子进程设置临时 验证说明 已读取 workflow 产出的证据文件：
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
这个拉取请求引入了一个基于 node-pty 的交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其对应的单元测试，旨在自动化验证命令行界面的交互流程。此外，它在 package.json 中添加了 test:interactive 脚本，并在 .gitignore 中忽略了生成的报告文件。审查中发现了一个中等严重性的问题：脚本中硬编码的 MAIN_MENU_OPTIONS 和 TOOL_MENU_OPTIONS 选项文本与实际的本地化菜单项不一致，这会导致测试在菜单项顺序或内容微调时变得极其脆弱。建议采纳修改建议，将菜单项更新为与实际中文本地化资源完全一致的文本。
| const MAIN_MENU_OPTIONS = ['配置语言', '配置线路', '设置 API Key', '配置编码工具', '健康检查', '重新初始化', '退出']; | ||
| const TOOL_MENU_OPTIONS = ['配置模型', '配置装载', '卸载配置', '验证配置', '重新加载', '查看状态', '返回']; |
There was a problem hiding this comment.
⚠️ 交互菜单选项与实际菜单不一致 (维护性与潜在测试脆弱性风险)
触发条件：
当前脚本中定义的 MAIN_MENU_OPTIONS 和 TOOL_MENU_OPTIONS 与实际代码 src/lib/wizard/menus/main-menu.ts 和 src/lib/wizard/menus/tool-menu.ts 中的菜单项存在明显不一致：
MAIN_MENU_OPTIONS包含了'健康检查'和'重新初始化'，但实际主菜单中对应的是menu_clear_config（清除配置）和menu_update_helper（更新助手）。TOOL_MENU_OPTIONS包含了'验证配置'、'重新加载'、'查看状态'，但实际工具菜单中对应的是launch（启动工具）、update（更新工具）、view（查看配置）。
实际影响：
目前测试能通过，完全是因为两者的数组长度相同（均为 7），导致 downToOption 计算出的下移次数（如到“返回”或“退出”）在巧合下是正确的。一旦实际菜单增加、删除或调整顺序，或者测试需要精确导航到中间某个菜单项时，downToOption 将会失效或导航到错误的选项，导致测试极其脆弱。
改进建议：
建议将 MAIN_MENU_OPTIONS 和 TOOL_MENU_OPTIONS 的内容更新为与实际中文本地化资源（zh_CN.json）完全一致的菜单文本。另外，请记得同步将 createToolMenuScenario 中的 '配置装载' 更新为 '装载配置' 以保持一致。
| const MAIN_MENU_OPTIONS = ['配置语言', '配置线路', '设置 API Key', '配置编码工具', '健康检查', '重新初始化', '退出']; | |
| const TOOL_MENU_OPTIONS = ['配置模型', '配置装载', '卸载配置', '验证配置', '重新加载', '查看状态', '返回']; | |
| const MAIN_MENU_OPTIONS = ['配置语言', '配置线路', '设置 API Key', '配置编码工具', '清除配置', '更新助手', '退出']; | |
| const TOOL_MENU_OPTIONS = ['配置模型', '装载配置', '卸载配置', '启动工具', '更新工具', '查看配置', '返回']; |
Codex 评审结论：通过 问题 未发现本 PR 引入的具体 bug、行为回归、安全风险、数据丢失风险或 CLI 交互回归。 本次改动主要影响 CI workflow、 验证说明
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
本拉取提交引入了交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其配套测试 tests/codex-interactive-check.test.mjs，通过 node-pty 模拟终端输入输出，用于验证 CLI 的键盘导航和菜单交互逻辑。同时在 package.json 中新增了 test:interactive 脚本，并在 .gitignore 中忽略了生成的报告文件。整体改动设计合理，但存在以下需要改进的问题：首先，在 runScenario 的 finally 块中，若 exitSubscription 未成功赋值即抛出异常，会导致 TypeError 并可能引起子进程泄漏；其次，analyzeTranscript 缺失对 options 及其属性的防御性空值检查，可能导致运行时崩溃；最后，createHomeFixture 在创建临时目录后，若后续写入配置失败，存在临时目录泄漏的风险。建议对这些边界情况进行修复以提升脚本的健壮性。整体合并信心评分为 4/5。
| } finally { | ||
| exitSubscription.dispose(); | ||
| if (!processState.exited) { | ||
| safeKill(child); | ||
| } | ||
| } |
There was a problem hiding this comment.
资源清理与健壮性问题
- 触发条件：在
pty.spawn成功执行后，但在exitSubscription成功赋值之前（例如child.onExit抛出异常，或者未来在它们之间添加了其他可能抛出异常的初始化代码）。 - 实际影响：导致
finally块中的exitSubscription.dispose()抛出TypeError: Cannot read properties of undefined (reading 'dispose')，从而掩盖了真正的初始化错误。更严重的是，由于异常在try块外部抛出，finally块将不会执行，导致已成功启动的子进程child无法被safeKill清理，造成 CI 环境中的进程泄漏和挂起。 - 为什么现有代码/测试未捕获：现有测试中
pty.spawn和监听器绑定始终成功，未模拟初始化阶段抛出异常的边界情况。 - 建议修复方向：对
exitSubscription使用可选链?.进行安全释放，并确保child存在时才调用safeKill。
| } finally { | |
| exitSubscription.dispose(); | |
| if (!processState.exited) { | |
| safeKill(child); | |
| } | |
| } | |
| } finally { | |
| exitSubscription?.dispose(); | |
| if (child && !processState.exited) { | |
| safeKill(child); | |
| } | |
| } |
| for (const expected of options.expectedPatterns) { | ||
| if (!clean.includes(expected)) { | ||
| failures.push(`Expected terminal output did not include "${expected}".`); | ||
| } | ||
| } |
There was a problem hiding this comment.
缺失防御性空值检查
- 触发条件：调用
analyzeTranscript时传入的options为空，或者options.expectedPatterns未定义。 - 实际影响：遍历
options.expectedPatterns将直接抛出TypeError: Cannot read properties of undefined (reading 'iterator')异常，导致测试脚本崩溃。 - 为什么现有代码/测试未捕获：现有的测试用例和场景生成器都完整地填充了
expectedPatterns字段，未覆盖到配置缺失的防御性边界。 - 建议修复方向：使用空值合并运算符（如
options?.expectedPatterns || []）进行防御性编程。
| for (const expected of options.expectedPatterns) { | |
| if (!clean.includes(expected)) { | |
| failures.push(`Expected terminal output did not include "${expected}".`); | |
| } | |
| } | |
| const expectedPatterns = options?.expectedPatterns || []; | |
| for (const expected of expectedPatterns) { | |
| if (!clean.includes(expected)) { | |
| failures.push(`Expected terminal output did not include "${expected}".`); | |
| } | |
| } |
| function createHomeFixture() { | ||
| const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'coding-helper-interactive-')); | ||
| const configDir = path.join(homeDir, '.coding-helper'); | ||
| fs.mkdirSync(configDir, { recursive: true, mode: 0o700 }); | ||
| fs.writeFileSync(path.join(configDir, 'config.yaml'), createHomeFixtureConfig(), { mode: 0o600 }); | ||
| return homeDir; | ||
| } |
There was a problem hiding this comment.
临时目录泄漏风险
- 触发条件：在
createHomeFixture中，fs.mkdtempSync成功创建了临时目录，但随后的fs.mkdirSync或fs.writeFileSync写入配置文件时由于权限、磁盘空间等原因抛出异常。 - 实际影响：临时目录已被创建，但由于异常在
main函数的try-finally块外部抛出，main无法捕获并执行fs.rmSync清理，导致临时目录残留，长期运行可能占满系统临时文件夹。 - 为什么现有代码/测试未捕获：测试运行环境磁盘空间充足且权限正常，未模拟文件写入失败的异常场景。
- 建议修复方向：在
createHomeFixture内部使用try-catch包裹目录和文件创建逻辑，一旦失败则立即清理已创建的临时目录并重新抛出错误。
function createHomeFixture() {
const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'coding-helper-interactive-'));
try {
const configDir = path.join(homeDir, '.coding-helper');
fs.mkdirSync(configDir, { recursive: true, mode: 0o700 });
fs.writeFileSync(path.join(configDir, 'config.yaml'), createHomeFixtureConfig(), { mode: 0o600 });
return homeDir;
} catch (error) {
fs.rmSync(homeDir, { recursive: true, force: true });
throw error;
}
}
Codex 评审结论：通过 发现的问题 未发现需要关注的 bug、行为回归、安全风险、数据丢失风险、并发问题或 CLI 交互体验问题。 本次 PR 主要新增 Codex PR review workflow、 验证说明 已读取 测试/验证
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
本拉取请求引入了交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其对应的单元测试 tests/codex-interactive-check.test.mjs，利用 node-pty 自动化验证 CLI 的键盘导航和菜单功能。同时更新了 .gitignore、package.json 和 pnpm-lock.yaml 以支持该测试套件。评审意见指出，在非 Unicode 或 Windows 环境下，Inquirer 的选中指针可能会降级为 >，因此建议在 hasSelectedOption 中增加对 > 指针的兼容支持，并在单元测试中补充相应的测试用例以填补测试空白。
| export function hasSelectedOption(transcript, optionText) { | ||
| return stripAnsi(transcript) | ||
| .split('\n') | ||
| .some((line) => line.includes('❯') && line.includes(optionText)); | ||
| } |
There was a problem hiding this comment.
在非 Unicode 环境或 Windows 系统的某些终端下，Inquirer 可能会将选中的指针字符 ❯ 降级为 >。如果硬编码只检查 ❯，可能会导致在这些环境下无法正确识别选中的选项，从而引起测试超时。根据 Repository Style Guide 第 6 条，这属于 P2 级别的兼容性风险（compatibility risks），建议进行修复。
| export function hasSelectedOption(transcript, optionText) { | |
| return stripAnsi(transcript) | |
| .split('\n') | |
| .some((line) => line.includes('❯') && line.includes(optionText)); | |
| } | |
| export function hasSelectedOption(transcript, optionText) { | |
| return stripAnsi(transcript) | |
| .split('\n') | |
| .some((line) => (line.includes('❯') || line.includes('>')) && line.includes(optionText)); | |
| } |
References
- P2 means test gaps, edge cases, compatibility risks, or paths likely to regress later. (link)
| test('hasSelectedOption requires the pointer to be on the expected option', () => { | ||
| assert.equal(hasSelectedOption('❯ ◆ 配置语言\n ◇ 配置线路', '配置语言'), true); | ||
| assert.equal(hasSelectedOption(' ◆ 配置语言\n❯ ◇ 配置线路', '配置语言'), false); | ||
| }); |
There was a problem hiding this comment.
为了配合 hasSelectedOption 对降级指针 > 的支持，建议在单元测试中增加对 > 指针的测试用例，确保其在非 Unicode 环境下的兼容性逻辑得到验证。根据 Repository Style Guide 第 6 条，这有助于填补测试空白（test gaps）。
| test('hasSelectedOption requires the pointer to be on the expected option', () => { | |
| assert.equal(hasSelectedOption('❯ ◆ 配置语言\n ◇ 配置线路', '配置语言'), true); | |
| assert.equal(hasSelectedOption(' ◆ 配置语言\n❯ ◇ 配置线路', '配置语言'), false); | |
| }); | |
| test('hasSelectedOption requires the pointer to be on the expected option', () => { | |
| assert.equal(hasSelectedOption('❯ ◆ 配置语言\n ◇ 配置线路', '配置语言'), true); | |
| assert.equal(hasSelectedOption(' ◆ 配置语言\n❯ ◇ 配置线路', '配置语言'), false); | |
| assert.equal(hasSelectedOption('> ◆ 配置语言\n ◇ 配置线路', '配置语言'), true); | |
| }); |
References
- P2 means test gaps, edge cases, compatibility risks, or paths likely to regress later. (link)
Codex 评审结论：通过 未发现需要关注的问题。本次 PR 主要新增 Codex PR Review workflow、真实伪终端交互检查脚本、 交互报告覆盖了主菜单、工具选择器，以及当前动态发现的 Claude Code、Codex、CodeBuddy、WorkBuddy、Hermes Agent 工具菜单。报告中各场景均通过，方向键上下选择、回车确认、返回/退出路径正常；未看到菜单持续闪烁、卡住或不可读输出。交互脚本通过临时 HOME/USERPROFILE 运行，并在测试结束清理，未发现污染真实 HOME 的风险。 验证
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
本拉取请求新增了交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其对应的单元测试，并引入了 node-pty 依赖。审查意见中指出，当前提交中遗漏了 PR 描述中提到的 GitHub Actions 工作流文件，导致 CI 流程无法实际运行。此外，waitForSelectedOption 函数在匹配到选中项后直接将游标推进到 clean.length，这在终端同一数据块输出多行内容时可能会导致后续步骤漏掉文本，从而引发超时或测试不稳定，建议精确计算匹配位置。根据仓库规范，整体反馈已采用中文进行总结。
| "scripts": { | ||
| "build": "tsc && rm -rf dist/locales && cp -r src/locales dist/locales", | ||
| "test": "pnpm build && node --test tests/*.test.mjs", | ||
| "test:interactive": "pnpm build && node scripts/codex-interactive-check.mjs", |
There was a problem hiding this comment.
问题: PR 描述中提到“新增 Codex PR Review workflow，在 PR 创建/更新时运行单元测试、交互式终端检查”，但在当前的提交中并没有看到任何 GitHub Actions 工作流文件（例如 .github/workflows/ 下的 YAML 文件）。\n\n影响: 导致 CI 流程无法实际运行，PR 的核心变更内容不完整。\n\n建议: 请确认是否遗漏了工作流文件的提交，并将其添加到 PR 中。
| function waitForSelectedOption(transcript, cursor, optionText, timeoutMs, processState = null) { | ||
| const startedAt = Date.now(); | ||
| return new Promise((resolve, reject) => { | ||
| const timer = setInterval(() => { | ||
| const clean = stripAnsi(transcript.join('')); | ||
| if (hasSelectedOption(clean.slice(cursor), optionText)) { | ||
| clearInterval(timer); | ||
| resolve(clean.length); | ||
| return; | ||
| } | ||
| if (processState?.exited) { | ||
| clearInterval(timer); | ||
| reject(new Error(`Process exited before selecting option "${optionText}" with code ${processState.code ?? 'null'}.`)); | ||
| return; | ||
| } | ||
| if (Date.now() - startedAt > timeoutMs) { | ||
| clearInterval(timer); | ||
| reject(new Error(`Timed out waiting for selected option after cursor ${cursor}: ${optionText}`)); | ||
| } | ||
| }, 50); | ||
| }); | ||
| } |
There was a problem hiding this comment.
问题: waitForSelectedOption 在匹配到选中的选项后，直接返回了 clean.length 作为新的 cursor。\n\n影响: 如果终端在同一个 tick/数据块中同时输出了选中的选项以及后续的提示文本（例如下一个菜单的标题或提示），将 cursor 直接推进到 clean.length 会导致后续的 waitFor 步骤漏掉这些已经输出的文本，从而引发超时（Timeout）或测试不稳定（Flakiness）。\n\n建议: 应该精确计算匹配到的选中行在 clean 中的结束位置，并返回该位置作为新的 cursor，而不是直接返回 clean.length。
function waitForSelectedOption(transcript, cursor, optionText, timeoutMs, processState = null) {
const startedAt = Date.now();
return new Promise((resolve, reject) => {
const timer = setInterval(() => {
const clean = stripAnsi(transcript.join(''));
const slice = clean.slice(cursor);
const lines = slice.split('\n');
let matchIndex = -1;
let currentOffset = 0;
for (const line of lines) {
if (/^\s*(❯|>)\s/.test(line) && line.includes(optionText)) {
matchIndex = currentOffset + line.length;
break;
}
currentOffset += line.length + 1;
}
if (matchIndex !== -1) {
clearInterval(timer);
resolve(cursor + matchIndex);
return;
}
if (processState?.exited) {
clearInterval(timer);
reject(new Error("Process exited before selecting option \"" + optionText + "\" with code " + (processState.code ?? "null") + "."));
return;
}
if (Date.now() - startedAt > timeoutMs) {
clearInterval(timer);
reject(new Error("Timed out waiting for selected option after cursor " + cursor + ": " + optionText));
}
}, 50);
});
}
Codex 评审结论：通过 未发现需要阻塞合并的问题。 本次 PR 未修改 CLI 命令、向导 flow、菜单实现、工具适配器或真实配置写入逻辑；主要影响面是新增 Codex PR review workflow、真实伪终端交互检查脚本、 交互报告显示已覆盖主菜单、工具选择器，以及 Claude Code、Codex、CodeBuddy、WorkBuddy、Hermes Agent 各工具菜单；方向键上下选择、回车确认、返回/退出路径均通过，未出现菜单卡住、不可退出或异常重复刷新的失败记录。 验证
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
这个拉取请求引入了一个新的交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其对应的单元测试 tests/codex-interactive-check.test.mjs，并添加了 node-pty 依赖项。审查意见指出，在 main 函数中，动态导入 tool-manager.js 发生在设置虚拟 homeDir 环境变量之前，这可能导致工具初始化时直接访问或污染真实用户的家目录。此外，如果导入抛出异常，会导致临时目录泄漏。建议将环境变量设置和动态导入逻辑移入 try 块中，以确保安全清理临时目录并隔离用户环境。
| const { toolManager } = await import('../dist/lib/tool-manager.js'); | ||
| const tools = toolManager.getAll().map((tool) => ({ | ||
| name: tool.name, | ||
| displayName: tool.displayName, | ||
| })); | ||
| const homeDir = createHomeFixture(); | ||
| const results = []; | ||
|
|
||
| try { | ||
| for (const scenario of buildScenarios(tools)) { | ||
| results.push(await runScenario(pty, scenario, homeDir)); | ||
| } | ||
| } finally { | ||
| fs.rmSync(homeDir, { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
在导入 tool-manager.js 之前，父进程尚未创建并设置虚拟的 homeDir 环境变量（HOME / USERPROFILE）。如果任何工具的构造函数或静态初始化代码在导入时尝试读取或写入 ~/.coding-helper 目录，它们将会直接访问或污染真实用户的家目录。
此外，如果 import('../dist/lib/tool-manager.js') 抛出异常，在原先的设计中，由于 homeDir 已经创建但尚未进入 try-finally 块，该临时目录将无法被清理，从而导致临时目录泄漏。
建议：
- 先创建
homeDir，并在try块中立即设置process.env.HOME和process.env.USERPROFILE。 - 将动态导入
tool-manager.js的逻辑移入try块内，确保无论导入成功与否，临时目录都能在finally中被安全清理。
| const { toolManager } = await import('../dist/lib/tool-manager.js'); | |
| const tools = toolManager.getAll().map((tool) => ({ | |
| name: tool.name, | |
| displayName: tool.displayName, | |
| })); | |
| const homeDir = createHomeFixture(); | |
| const results = []; | |
| try { | |
| for (const scenario of buildScenarios(tools)) { | |
| results.push(await runScenario(pty, scenario, homeDir)); | |
| } | |
| } finally { | |
| fs.rmSync(homeDir, { recursive: true, force: true }); | |
| } | |
| const homeDir = createHomeFixture(); | |
| const results = []; | |
| try { | |
| process.env.HOME = homeDir; | |
| process.env.USERPROFILE = homeDir; | |
| const { toolManager } = await import('../dist/lib/tool-manager.js'); | |
| const tools = toolManager.getAll().map((tool) => ({ | |
| name: tool.name, | |
| displayName: tool.displayName, | |
| })); | |
| for (const scenario of buildScenarios(tools)) { | |
| results.push(await runScenario(pty, scenario, homeDir)); | |
| } | |
| } finally { | |
| fs.rmSync(homeDir, { recursive: true, force: true }); | |
| } |
Codex 评审结论：通过 问题 未发现需要关注的问题。本 PR 未修改 CLI 命令、向导 flow、菜单实现、工具适配器或实际配置写入路径；变更集中在 Codex PR review workflow、真实伪终端交互检查脚本、测试脚本和 交互报告覆盖了主菜单、工具选择器，以及动态发现到的 Claude Code、Codex、CodeBuddy、WorkBuddy、Hermes Agent 工具菜单。各场景均通过，未见方向键失效、回车无法确认、菜单卡住无法退出、重复刷新超阈值，或写入真实 HOME 的证据；脚本为子进程设置了临时 验证
|
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
此拉取请求引入了交互式终端检查脚本 scripts/codex-interactive-check.mjs 及其配套测试 tests/codex-interactive-check.test.mjs，用于通过 node-pty 模拟用户键盘输入，自动化验证 CLI 菜单导航和工具配置流程。同时在 package.json 中新增了 test:interactive 脚本，并在 .gitignore 中忽略了生成的报告文件。审查意见指出：在 Windows 环境下，仅覆盖 HOME 和 USERPROFILE 可能会因未覆盖 HOMEDRIVE 和 HOMEPATH 而导致配置写入真实主目录，破坏测试隔离性；此外，硬编码的 TOOL_MENU_OPTIONS 无法适配某些动态省略菜单项的工具，可能导致导航按键次数计算偏差而超时。这些反馈均指出了实际存在的兼容性与健壮性问题，建议予以保留。整体修改安全，置信度评分为 4/5。
| const child = pty.spawn(process.execPath, buildCliArgs(scenario.args, preloadPath), { | ||
| name: 'xterm-256color', | ||
| cols: 100, | ||
| rows: 30, | ||
| cwd: process.cwd(), | ||
| env: { | ||
| ...process.env, | ||
| HOME: homeDir, | ||
| USERPROFILE: homeDir, | ||
| LANG: 'zh_CN.UTF-8', | ||
| TERM: 'xterm-256color', | ||
| NO_COLOR: '1', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
在 Windows 环境下，某些工具或依赖库可能会通过 HOMEDRIVE 和 HOMEPATH 环境变量来解析用户主目录（Home Directory）。当前代码仅覆盖了 HOME 和 USERPROFILE，这可能导致子进程在 Windows 上运行时，将配置文件写入到用户的真实主目录下，从而破坏测试的隔离性。\n\n建议同时覆盖 HOMEDRIVE 和 HOMEPATH 环境变量，以确保在所有平台上都能实现完整的主目录隔离。
const child = pty.spawn(process.execPath, buildCliArgs(scenario.args, preloadPath), {\n name: 'xterm-256color',\n cols: 100,\n rows: 30,\n cwd: process.cwd(),\n env: {\n ...process.env,\n HOME: homeDir,\n USERPROFILE: homeDir,\n ...(process.platform === 'win32' ? { HOMEDRIVE: homeDir.substring(0, 2), HOMEPATH: homeDir.substring(2) } : {}),\n LANG: 'zh_CN.UTF-8',\n TERM: 'xterm-256color',\n NO_COLOR: '1',\n },\n });| const TOOL_MENU_OPTIONS = [ | ||
| ZH_LOCALE.tool_configure_models, | ||
| ZH_LOCALE.tool_load_config, | ||
| ZH_LOCALE.tool_unload_config, | ||
| ZH_LOCALE.tool_launch, | ||
| ZH_LOCALE.tool_update, | ||
| ZH_LOCALE.tool_view_config, | ||
| ZH_LOCALE.tool_back, | ||
| ]; |
There was a problem hiding this comment.
硬编码的 TOOL_MENU_OPTIONS 包含了所有可能的工具菜单选项。然而，某些工具（例如桌面应用或 Codex）可能会根据其特性动态省略部分菜单项（例如 Codex 没有“配置模型”选项，桌面应用可能没有“启动工具”或“更新工具”选项）。\n\n如果实际终端菜单中省略了某些选项，downToOption 函数计算出的下方向键（\u001B[B）发送次数将会偏大，导致测试在交互时选中错误的菜单项或发生超时。\n\n建议在导航时，避免依赖完全静态的硬编码菜单顺序，或者为不同类型的工具（如 CLI 工具、桌面应用、固定模型工具）定义不同的菜单选项列表，以提高交互测试的健壮性。
Codex 评审结论：通过 问题 未发现需要关注的问题。本 PR 未修改运行时代码、命令路由、向导 flow、菜单实现、工具适配器或真实配置写入逻辑；主要影响范围是新增 Codex PR review workflow、真实伪终端交互检查脚本、交互检查测试和 交互风险方面， 验证
已读取 |
1 participant
变更内容
scripts/codex-interactive-check.mjs，通过node-pty启动真实伪终端，模拟方向键和回车，检查主菜单、工具选择器以及动态发现到的工具菜单。test:interactive脚本和对应测试，确保交互报告分析、ANSI 清理、工具场景生成逻辑可验证。设计说明
交互检查脚本从构建产物中的
toolManager.getAll()动态发现工具。以后新增工具并注册到ToolManager后，PR review 不需要维护硬编码工具清单，交互报告会自动包含对应工具菜单场景。验证
pnpm testpnpm test:interactive