fix(core): honor injected llm backend in code-quality sub-agent - #409
fix(core): honor injected llm backend in code-quality sub-agent#409ceilf6 wants to merge 1 commit into
Conversation
进程隔离子代理经 stdin 收 JSON 参数,而注入的 llm.backend 是一组函数, JSON 序列化必然丢弃它。worker 于是用 provider 直连新建 LLMService: 调用方指定的路由(代理/网关/本地模型/测试替身)被绕开去打公网 provider, 且失败被 enableRuleFallback 吞掉,LLM 评审静默退化为纯规则评审并报成功。 改为:启用 LLM 评审且存在自定义 backend 时,隔离方式降级为 in-process (该分支直接复用 this.llmService,能拿到注入的 backend)。未注入 backend 的调用方行为不变,仍走进程隔离;关闭 LLM 评审时无需 backend,同样不变。 Closes #407 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
代码评审报告: fix(core): honor injected llm backend in code-quality sub-agent
风险等级: 中
处理建议: 评论
决策摘要: ** 修复机制经源码核实是有效且范围最小的(LLMService 在 config.backend 存在时确实短路 provider,见 llm-service.ts:60,189,248),无阻塞性正确性缺陷;但 'inProcess' 不是 isolationMode 域内的合法取值(应为 'in_memory'),建议这一行改掉后再合入,其余为可跟进项。
级联分析
- 变更符号: 原始模型未提供结构化级联字段。
- 受影响流程: 原始模型未提供结构化级联字段。
- 变更集外调用方: unknown
- 置信度: degraded
问题发现
- [中]
'inProcess'是域外取值,与 canonical 的'in_memory'不一致- 证据:
types.ts:430定义isolationMode?: 'in_memory' | 'process',run.ts:54同样是'process' | 'in_memory';'inProcess'在全仓只出现在agent.ts:182这一处。因为const推断出联合字面量、且下游只判=== 'process',TS 不会报错,行为今天是对的。 - 受影响调用方/流程: 目前无;但任何后续按
'in_memory'分支或做穷举 switch 的代码都会漏掉这个值,属于潜伏错误。 - 最小可行修复: 把
'inProcess'改成'in_memory',并给isolationMode显式标注NonNullable<SubAgentConfig['codeQualityEvaluator']>['isolationMode'],让类型系统挡住未来的自造取值。
- 证据:
- [中] 降级同时丢掉了唯一的超时与崩溃兜底,且新路径无时间上界
- 证据: 超时只在进程分支生效(
process-isolated-code-quality-subagent.ts:95默认 120000ms,:137-145SIGKILL);CodeQualitySubAgent构造参数里没有任何超时字段(code-quality-subagent.ts:45-75),LLMService全文无 timeout/AbortSignal,调用点phase-checks.ts:225也是裸await。 - 受影响调用方/流程: 正是本 PR 的目标调用方——
run.ts注入 backend 的路径与run-eval.mjs的 Claude CLI backend。它们此前是"规则评审、秒回",此后每个阶段会真实发起一次 in-process LLM 评审;后端挂起时该阶段将无限期阻塞,没有任何一层能中断。 - 最小可行修复: 在 in-memory 分支外包一层
Promise.race超时(复用processTimeoutMs的语义,或新增独立字段),或至少在 PR 描述/issue 中把"失去 120s 兜底"列为已知代价并开跟进 issue。
- 证据: 超时只在进程分支生效(
- [低] 降级提示被 debug 门控,对本 PR 的主要受害者依然不可见
- 证据:
agent.ts:335-339的debugWarn只在config.debug为真时输出;run-eval.mjs一类批处理调用方通常不开 debug。仓库已有非门控先例(planner.ts:161的 fallback 用无条件logger.warn)。 - 受影响调用方/流程: 显式设置
isolationMode: 'process'的调用方,其配置被静默覆盖,隔离级别下降无任何信号。 - 最小可行修复: 该降级改用无条件
logger.warn(每个 agent 实例一次),与 planner fallback 保持一致。
- 证据:
- [低]
isolationMode的文档注释未反映新契约- 证据:
types.ts:429仍写"隔离模式:process 为真实上下文隔离(默认 process)",没有说明存在自定义llm.backend且启用 LLM 评审时该值会被强制降级。 - 受影响调用方/流程: 依赖该注释理解配置语义的调用方与
run.ts:54的 CLI 选项。 - 最小可行修复: 在注释中补一句降级条件。
- 证据:
行级发现
- [packages/core/src/agent/agent.ts:182] 'inProcess'
不在isolationMode的'in_memory' | 'process'取值域内,全仓仅此一处;改为'in_memory'` 并给该常量加显式类型标注,避免未来按 canonical 值分支时漏判。 - [packages/core/src/agent/agent.ts:179] 该条件成立时会同时放弃进程分支的 120s SIGKILL 兜底,而 in-memory 路径与
LLMService均无超时;至少为 in-memory 评审加一层超时,或明确记录这一代价。 - [packages/core/src/agent/agent.ts:186] 降级提示走
debugWarn,非 debug 运行(如 eval 批跑)看不到隔离级别下降;参照planner.ts:161改为无条件logger.warn。 - [packages/core/src/agent/agent.test.ts:228] 断言
constructor.name只验证了选了哪个类,验证不到 #407 的验收条件(注入的 backend 被真正调用、不再直连 provider),且对重命名/压缩脆弱;建议补一条经 A2A 驱动一次评审、断言 stub backend 的generateObject被调用的测试。
Karpathy 评审
- 假设: 模型输出需要归一化为固定 Markdown 契约。
- 简洁性: 已提取 summary、finding、evidence 与 fix;原始 prose 不再附在评论中,避免占用下游解析与代理上下文。
- 变更范围: 原始模型未提供结构化范围字段。
- 验证: 需要查看 CI、测试或人工 CR 证据补强合并信心。
缺失覆盖
- 输出未命中 Repo Guard Markdown 契约;建议补充真实模型质量评估覆盖。
| const backendBlocksProcessIsolation = Boolean(config.llm?.backend) && enableLLMReview; | ||
| const isolationMode = | ||
| requestedIsolationMode === 'process' && backendBlocksProcessIsolation | ||
| ? 'inProcess' |
There was a problem hiding this comment.
'inProcess'不在isolationMode的'in_memory' | 'process'取值域内,全仓仅此一处;改为'in_memory'` 并给该常量加显式类型标注,避免未来按 canonical 值分支时漏判。
| // 注入的 backend 是一组函数,随 JSON 越不过进程边界:worker 会静默丢掉它、 | ||
| // 改用 provider 直连,调用方指定的路由意图被违背且 LLM 评审静默退化为规则评审。 | ||
| // 故有 backend 时降级为 in-process 隔离——该分支直接复用 this.llmService。 | ||
| const backendBlocksProcessIsolation = Boolean(config.llm?.backend) && enableLLMReview; |
There was a problem hiding this comment.
该条件成立时会同时放弃进程分支的 120s SIGKILL 兜底,而 in-memory 路径与 LLMService 均无超时;至少为 in-memory 评审加一层超时,或明确记录这一代价。
| : requestedIsolationMode; | ||
|
|
||
| if (isolationMode !== requestedIsolationMode) { | ||
| this.debugWarn( |
There was a problem hiding this comment.
降级提示走 debugWarn,非 debug 运行(如 eval 批跑)看不到隔离级别下降;参照 planner.ts:161 改为无条件 logger.warn。
|
|
||
| function isolationOf(agent: ReturnType<typeof createAgent>): string { | ||
| const sub = (agent as unknown as { codeQualitySubAgent?: object }).codeQualitySubAgent; | ||
| return sub?.constructor.name ?? 'none'; |
There was a problem hiding this comment.
断言 constructor.name 只验证了选了哪个类,验证不到 #407 的验收条件(注入的 backend 被真正调用、不再直连 provider),且对重命名/压缩脆弱;建议补一条经 A2A 驱动一次评审、断言 stub backend 的 generateObject 被调用的测试。
Linked Issue Or Context
Summary
ProcessIsolatedCodeQualitySubAgenthands the worker its config as JSON overstdin.
config.llm.backendis a set of functions, soJSON.stringifydropsit and the worker always sees
backend: undefined. It then builds anLLMServiceon the provider path and issues a real HTTP call to theconfigured public provider — bypassing whatever backend the caller injected.
The failure is silent:
enableRuleFallbackdefaults totrue, so the providercall fails, the error is swallowed, and the worker returns a clean rule-only
review reporting success. The caller cannot tell that LLM review never ran.
Fix: when LLM review is enabled and a custom
llm.backendis present, usethe
inProcessisolation branch, which passesthis.llmServicedirectly andtherefore honours the injected backend. A debug warning records the downgrade.
Deliberately narrow:
enableLLMReview: false→ unchanged, still process-isolated (no backend needed).I did not simply disable the sub-agent: that would change the system under
test and break comparability with the existing benchmark baseline.
Evidence
Direct probe against the sub-agent before the fix, injecting an in-process
backend plus an invalid API key:
The injected backend is never called and the pass comes from the rule fallback.
Scope note for the benchmark
benchmarks/eval/run-eval.mjsinjects a Claude-Code-CLI backend, so code-qualityreview has been rule-only in every eval run to date — including the
2026-07-12 baseline, whose code is identical here (verified at
51fd4bb:same
enabled ?? true, sameisolationMode ?? 'process', same workerJSON.parse, same default model).Because it degrades identically in both arms and in the baseline, it does not
bias the SDD comparison. It is a capability/routing bug, not a benchmark
confound. Merging this does change the system under test for backend-injecting
callers, so the next benchmark run should note the configuration difference
against July.
Impact Scope
packages/core/src/agent/agent.ts— isolation mode selection in the constructor.packages/core/src/agent/agent.test.ts— 3 contract tests.isolationModekeeps its meaning forevery caller that can actually use it.
GitNexus Impact Summary
agent-core(packages/core/src/agent/agent.ts), with matching tests underpackages/core/src/agent/.detect_changes --scope compare --base-ref developreports 2 files / 3 symbols (constructor,FrontAgent,enableLLMReview) and 2 affected execution flows at medium risk;impacton the constructor stays insideFrontAgentconstruction (Constructor → NormalizeConfig,Constructor → ValidateFn) with no downstream callers outside agent construction.pnpm quality:precommitexit 0;pnpm quality:local(incl.build:verify) green on pre-push.Verification
npx vitest run packages/core/src/agent/agent.test.ts— 18 passed, 3 new:uses process isolation when no custom backend is injectedfalls back to in-process isolation when a custom llm backend is injectedkeeps process isolation when LLM review is disabled, since no backend is neededpnpm quality:precommit— exit 0, turbo 25/25,test:workflows34/34.Checklist
pnpm quality:precommit, or explained why it could not run.pnpm quality:localfor critical skeleton changes, or explained why it could not run.