Skip to content

fix(core): honor injected llm backend in code-quality sub-agent - #409

Open
ceilf6 wants to merge 1 commit into
developfrom
fix/subagent-backend-isolation
Open

fix(core): honor injected llm backend in code-quality sub-agent#409
ceilf6 wants to merge 1 commit into
developfrom
fix/subagent-backend-isolation

Conversation

@ceilf6

@ceilf6 ceilf6 commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Linked Issue Or Context

Summary

ProcessIsolatedCodeQualitySubAgent hands the worker its config as JSON over
stdin. config.llm.backend is a set of functions, so JSON.stringify drops
it and the worker always sees backend: undefined. It then builds an
LLMService on the provider path and issues a real HTTP call to the
configured public provider — bypassing whatever backend the caller injected.

The failure is silent: enableRuleFallback defaults to true, so the provider
call 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.backend is present, use
the inProcess isolation branch, which passes this.llmService directly and
therefore honours the injected backend. A debug warning records the downgrade.

Deliberately narrow:

  • No custom backend → unchanged, still process-isolated.
  • enableLLMReview: false → unchanged, still process-isolated (no backend needed).
  • Only the case that is currently broken changes behaviour.

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:

stub backend calls : 0        <-- injected backend NEVER used
response success   : true
summary            : "CodeQualitySubAgent reviewed 1 file(s): 0 error(s), 0 warning(s), score 100/100."

The injected backend is never called and the pass comes from the rule fallback.

Scope note for the benchmark

benchmarks/eval/run-eval.mjs injects a Claude-Code-CLI backend, so code-quality
review 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, same isolationMode ?? 'process', same worker
JSON.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.
  • No public API or config surface change; isolationMode keeps its meaning for
    every caller that can actually use it.

GitNexus Impact Summary

  • Risk level: MEDIUM
  • Critical skeleton changes: yes — agent-core (packages/core/src/agent/agent.ts), with matching tests under packages/core/src/agent/.
  • GitNexus impact: detect_changes --scope compare --base-ref develop reports 2 files / 3 symbols (constructor, FrontAgent, enableLLMReview) and 2 affected execution flows at medium risk; impact on the constructor stays inside FrontAgent construction (Constructor → NormalizeConfig, Constructor → ValidateFn) with no downstream callers outside agent construction.
  • Verification: pnpm quality:precommit exit 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 injected
    • falls back to in-process isolation when a custom llm backend is injected
    • keeps process isolation when LLM review is disabled, since no backend is needed
  • pnpm quality:precommit — exit 0, turbo 25/25, test:workflows 34/34.

Checklist

  • I have linked an issue or explained why this PR stands alone.
  • I have kept the diff focused on the stated change.
  • I have run pnpm quality:precommit, or explained why it could not run.
  • I have run pnpm quality:local for critical skeleton changes, or explained why it could not run.
  • I have updated docs or tests when behavior, public APIs, or Harness contracts changed.
  • For critical skeleton changes, I have filled the GitNexus impact summary with concrete results.

进程隔离子代理经 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>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ ceilf6/repo-guard

代码评审报告: fix(core): honor injected llm backend in code-quality sub-agent

风险等级:
处理建议: 评论
决策摘要: ** 修复机制经源码核实是有效且范围最小的(LLMServiceconfig.backend 存在时确实短路 provider,见 llm-service.ts:60,189,248),无阻塞性正确性缺陷;但 'inProcess' 不是 isolationMode 域内的合法取值(应为 'in_memory'),建议这一行改掉后再合入,其余为可跟进项。

级联分析

  • 变更符号: 原始模型未提供结构化级联字段。
  • 受影响流程: 原始模型未提供结构化级联字段。
  • 变更集外调用方: unknown
  • 置信度: degraded

问题发现

  1. [中] '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'],让类型系统挡住未来的自造取值。
  2. [中] 降级同时丢掉了唯一的超时与崩溃兜底,且新路径无时间上界
    • 证据: 超时只在进程分支生效(process-isolated-code-quality-subagent.ts:95 默认 120000ms,:137-145 SIGKILL);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。
  3. [低] 降级提示被 debug 门控,对本 PR 的主要受害者依然不可见
    • 证据: agent.ts:335-339debugWarn 只在 config.debug 为真时输出;run-eval.mjs 一类批处理调用方通常不开 debug。仓库已有非门控先例(planner.ts:161 的 fallback 用无条件 logger.warn)。
    • 受影响调用方/流程: 显式设置 isolationMode: 'process' 的调用方,其配置被静默覆盖,隔离级别下降无任何信号。
    • 最小可行修复: 该降级改用无条件 logger.warn(每个 agent 实例一次),与 planner fallback 保持一致。
  4. [低] 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'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

该条件成立时会同时放弃进程分支的 120s SIGKILL 兜底,而 in-memory 路径与 LLMService 均无超时;至少为 in-memory 评审加一层超时,或明确记录这一代价。

: requestedIsolationMode;

if (isolationMode !== requestedIsolationMode) {
this.debugWarn(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

降级提示走 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';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

断言 constructor.name 只验证了选了哪个类,验证不到 #407 的验收条件(注入的 backend 被真正调用、不再直连 provider),且对重命名/压缩脆弱;建议补一条经 A2A 驱动一次评审、断言 stub backend 的 generateObject 被调用的测试。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Process-isolated code-quality sub-agent drops the injected LLM backend and calls the provider directly

1 participant