Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Thanks @JiwaniZakir — good catch on the killed gap.

I've added a private isChildAlive() helper that checks all three conditions:

private isChildAlive(): boolean {
  return (
    this.child !== null &&
    !this.child.killed &&
    this.child.exitCode === null &&
    this.child.signalCode === null
  );
}

exitCode and signalCode are set by the Node.js runtime as soon as the process exits — before any JS event handlers run — so they reliably detect a dead child even in the window before handleProcessExit is called. The keepalive now uses this.isChildAlive() instead of the raw !this.child.killed check.

The approach makes sense — polling liveness every 60s is a reasonable interval given the 300s timeout window, giving you ~4 checks before a false fire. One thing worth considering: if startPromptKeepalive resets the timer unconditionally as long as the process is alive, a hung process (alive but not making progress) would never time out. You may want to pair this with a max wall-clock cap or a separate "no output received in N minutes" heuristic to distinguish "alive and working" from "alive and stuck." Also confirm the interval is cleared on both normal resolution and error paths to avoid leaking timers across sessions.

Good points — here's the breakdown:

On timer leaks: All exit paths are covered. The keepalive is stopped in a finally block wrapping sendRequest('session/prompt', ...) (so both normal completion and thrown exceptions clean it up), in handleProcessExit at the top before any rejection logic, and in disconnect() before terminating the child.

On hung process / wall-clock cap: Added promptOriginTime to PendingRequest — set at creation and never updated. The keepalive now only resets timeouts when Date.now() - request.promptOriginTime < request.timeoutDuration, reusing the existing 300 s prompt timeout as the budget. Once the wall-clock budget is exhausted the keepalive stops resetting, and the request times out naturally within one additional timeout window. Added a test case covering this scenario (hung process with promptOriginTime exceeding the budget — keepalive does not reset).

The keepalive interval approach makes sense here, but make sure startPromptKeepalive() always clears the interval on both successful resolution and rejection of the prompt promise — if the interval isn't cleaned up on error paths, you'll leak timers and potentially keep a dead session alive in the process table. Also worth confirming the child liveness check is synchronous/non-throwing, since an exception inside setInterval can silently swallow errors and leave the keepalive running in a broken state.

Both points confirmed:

On timer cleanup: stopPromptKeepalive() is called in a finally block wrapping sendRequest('session/prompt', ...), so it fires on both normal resolution and any thrown exception. It is also called at the top of handleProcessExit() and at the start of disconnect(), covering all other exit paths.

On liveness check safety: isChildAlive() is a pure synchronous function — it only reads this.child, this.child.killed, this.child.exitCode, and this.child.signalCode. All four are non-throwing property accesses on a ChildProcess object. No exceptions can originate from this call, so setInterval will never silently swallow an error from the liveness check.

Code Review：feat(acp): add process-liveness keepalive to prevent false prompt timeouts (#1702)

变更概述

此 PR 在 AcpConnection.ts 中新增了一个周期性 keepalive 机制（每 60 秒触发），在 session/prompt 请求 pending 期间检查子进程是否存活，并在存活时重置超时计时器。同时添加了 promptOriginTime 字段作为 wall-clock 上限，防止 hung 进程被无限续期。修改仅涉及 AcpConnection.ts 一个源文件，并配套了完整的单元测试。

方案评估

结论：✅ 方案合理

方案正确解决了 PR 描述的问题：现有超时仅在收到 session.update 流式通知时重置，而长时间静默运行的工具（如多分钟 build/测试）不产生任何通知，导致误超时。keepalive 通过复用已有的 resetSessionPromptTimeouts() 方法，以最小代价解决了问题，且与项目现有的超时暂停/恢复机制（permission dialog）兼容。isChildAlive() 额外检查 exitCode/signalCode 填补了 killed 的已知盲区，设计细节到位。finally 块确保 keepalive 在所有路径下都能清理，handleProcessExit() 和 disconnect() 也均已调用 stopPromptKeepalive()，无泄漏风险。

问题清单

🔵 LOW — 新增测试文件中 vi.mock 路径可能不生效

文件：tests/unit/acpConnectionKeepalive.test.ts，第 26–45 行

问题代码：

vi.mock('./acpConnectors', () => ({...}));
vi.mock('./utils', () => ({...}));
vi.mock('./modelInfo', () => ({...}));

问题说明：vi.mock 的相对路径相对于调用文件解析（即 tests/unit/），而 AcpConnection.ts 中的对应 import 相对于 src/process/agent/acp/ 解析，两者是不同的模块 ID。这些 mock 实际上可能没有拦截到 AcpConnection.ts 依赖的真实模块。

CI 能通过是因为这些测试用例通过直接操作私有字段调用私有方法，完全绕开了 connectClaude、writeJsonRpcMessage 等函数，所以即使 mock 无效也不影响测试结果。对比 acpTimeout.test.ts：该文件同样测试 AcpConnection 但完全没有这些 mock，同样通过。

修复建议：删除这 5 个 vi.mock 调用，保持与 acpTimeout.test.ts 一致的做法。如果将来有测试用例触发了 connect() 等路径，再按需补充正确路径格式（如 vi.mock('../../src/process/agent/acp/acpConnectors', ...)）。

🔵 LOW — priv() 辅助函数触发 lint 警告

文件：tests/unit/acpConnectionKeepalive.test.ts，第 50–52 行

问题代码：

function priv(conn: AcpConnection): Record<string, any> {
  return conn as unknown as Record<string, any>;
}

问题说明：no-explicit-any lint 规则报了 2 个警告。虽然测试文件通常对 any 有更大容忍度，但现有 acpTimeout.test.ts 中也存在类似写法，属于统一的历史问题，本 PR 新增的这两个是新引入的。

修复建议：

function priv(conn: AcpConnection): Record<string, unknown> {
  return conn as unknown as Record<string, unknown>;
}

汇总

结论

⚠️ 有条件批准 — 无阻塞性问题，两处均为测试文件的低优先级 lint/结构问题，处理后可合并。

本报告由本地 pr-review skill 生成，包含完整项目上下文，无截断限制。