Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #351 +/- ##
==========================================
Coverage ? 87.41581%
==========================================
Files ? 543
Lines ? 52709
Branches ? 0
==========================================
Hits ? 46076
Misses ? 6633
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AI Code Review审查结论不通过 审查范围:base_commit 02509b2..head_commit 6f28a4d(1 个提交“fix: propagate workspace runtime in skill_exec”,2 个文件、110 增 2 删: 发现的问题中等
问题: 触发条件: 调用 实际影响: 调用方按契约( 修正方向: 在 中等
问题: 本次变更使 触发条件: 实际影响: 超时被杀的命令被报告为普通失败:按 修正方向: 从 中等
问题: 本次变更使输出收集( 触发条件: 交互进程已退出而 LLM 未先成功轮询到退出态,直接调用 实际影响: 会话的最终 修正方向: 在 kill 与 TTL 逐出路径中,若 中等
问题: 本次变更修复了 触发条件: 进程退出后收集阶段 实际影响: 用户得到 修正方向: 将收集失败显式表达:在该分支向 中等
问题: 触发条件: 两个协程并发对同一 实际影响: 同一批输出被读取两次、 修正方向: 参照 中等
问题: 本次变更使 触发条件: 实际影响: 静默降级路径变成向上抛异常(本来旨在优雅返回空结果以结束交互链,反而中断整个工具调用); 修正方向: 给异常分支的 较低
问题: 触发条件: 未来改动把错误的(非空) 实际影响: 该断言给出虚假的覆盖信心,无法捕获 #350 这类“参数错误但非缺失”的回归(缺参已被严格签名桩覆盖,但错参不会被外层断言捕获),测试的回归防护弱于注释所宣称的意图。 修正方向: 用捕获的真实 较低
问题: 本次变更使收集路径首次真正执行(本变更行补上 触发条件: 交互命令向 stderr 输出内容(例如 20KB 的错误堆栈)且成功收集;或调用方检查 实际影响: 按 修正方向: 从 |
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) |
There was a problem hiding this comment.
问题: _collect_final_result 在此变更行启动的真实收集路径中,将 omit_inline_content 透传给 fake_run_input(第 781 行),但组装 SkillRunOutput 时从不把 output_files/primary_output 的 content 置空。对照 _skill_run.py:742-747 在 skill_run 路径会按该标志清空内容。本次变更前 _prepare_outputs 因缺参必然抛 TypeError 被吞掉、files 恒为空,该标志没有任何可观察效果;本次变更在调用处(即本行)补上 workspace_runtime 使收集真正生效后,标志被静默忽略成为可观察缺陷。
触发条件: 调用 skill_exec(或经 skill_poll_session/skill_write_stdin 完成最终收集)且 ExecInput 设置 omit_inline_content=True、声明了 output_files 或 outputs 产物,_prepare_outputs 返回非空 files 后进入后续组装。
实际影响: 调用方按契约(ExecInput.omit_inline_content 字段描述及 _skill_processor.py 对模型的引导:设置该标志后 output_files 只返回元数据、用 ref 按需读取)期望省略内联内容,实际却拿到全部文件全文,与 skill_run 行为不一致;大文件内容直接膨胀进工具响应,消耗模型上下文预算,本 PR 之前不会发生。
修正方向: 在 _collect_final_result 组装 SkillRunOutput 前,参照 _skill_run.py:742-747,当 in_data.omit_inline_content 为真时对 files 及选出的 primary_output 置空 content,并补充该场景的回归测试。
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) |
There was a problem hiding this comment.
问题: 本次变更使 _prepare_outputs 收集路径首次真正执行(本行新增的第四个参数 exec_session.workspace_runtime 修复了收集恒抛 TypeError 的问题),但后续组装时丢弃了 WorkspaceRunResult.timed_out 字段(LocalProgramSession.run_result 返回 timed_out=self._timed_out,命令超时被杀时为真),_filter_failed_empty_outputs(exit_code, False, files) 硬编码 timed_out=False,SkillRunOutput.timed_out 恒为默认 False。对照 _skill_run.py:722/731 会把真实 result.timed_out 传入过滤器并写入输出。此前收集恒失败、结果恒为空,本缺陷被掩盖;本次变更使其变为可观察。
触发条件: skill_exec 设置 timeout 且命令实际超时被本地运行器杀死(enforce_timeout 置 _timed_out=True、exit_code 非 0),随后在 status==_STATUS_EXITED 时进入 _collect_final_result。
实际影响: 超时被杀的命令被报告为普通失败:按 skill_run 语义应被丢弃的空输出文件被保留、_WARN_FAILED_RUN_EMPTY_OUTPUTS 不产生,下游(LLM/调用方)无法区分“超时中止”与“正常失败”,输出契约与 skill_run 不一致。
修正方向: 从 run_result() 中取出 timed_out 并透传给 _filter_failed_empty_outputs(exit_code, timed_out, files),同时写入 SkillRunOutput(timed_out=timed_out)(异常分支可保留现有兜底)。
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) |
There was a problem hiding this comment.
问题: 本次变更使输出收集(_collect_final_result 内、本变更行所在的 _prepare_outputs 调用)首次真正生效,但 KillSessionTool._run_async_impl 与 TTL 回收路径(_gc_expired_sessions → cleanup_expired_sessions → _close_session)仍只做 proc.close() 和会话移除,从不调用 _collect_final_result。输出收集仅存在于 SkillExecTool/WriteStdinTool/PollSessionTool 三个入口,且都以 status==_STATUS_EXITED and not exec_session.finalized 为前置,这两条路径仍会把结果静默丢弃。
触发条件: 交互进程已退出而 LLM 未先成功轮询到退出态,直接调用 skill_kill_session(proc.poll(None) 返回已退出时 kill 仍然 ok=True),或会话退出了但 ttl 内没有任何轮询观察到退出而被 _gc_expired_sessions 逐出(_refresh_exit_state 会主动把 exited_at 打上,使逐出成为必然)。
实际影响: 会话的最终 SkillRunOutput(output_files、save_as_artifacts 持久化侧效应、artifact 状态增量)永远不会生成,且 skill_kill_session 仍返回 ok=True, status="exited" 造成结果已交付的假象;快速退出且未轮询的交互会话输出静默丢失。
修正方向: 在 kill 与 TTL 逐出路径中,若 not exec_session.finalized 且进程已退出,先调用 _collect_final_result(带上当时上下文)再关闭/移除会话,使结果与 artifact 侧效应不被静默丢弃。
There was a problem hiding this comment.
这个问题我认同,但是它涉及 skill_kill_session / TTL 回收时的 finalization 语义,以及是否需要在清理路径里触发 artifact/output 收集,范围已经超出 #350 这次修复。
本 PR 先聚焦恢复现有 _collect_final_result 路径,kill / TTL 路径我建议单独开 issue 跟进,避免把这次 bug fix 扩成 session lifecycle 的行为调整。
| try: | ||
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, fake_run_input) | ||
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) | ||
| except Exception as ex: # pylint: disable=broad-except | ||
| logger.warning("skill_exec: collect outputs failed: %s", ex) | ||
| files, manifest = [], None |
There was a problem hiding this comment.
问题: 本次变更修复了 _prepare_outputs 的缺参 TypeError(第 786-787 行),使收集首次真正执行,但其外包的 except Exception: files, manifest = [], None(仅 logger.warning)把所有失败(FS 读写/glob 错误、未来的签名漂移——正是 #350 那一类)都降级为空文件,且 SkillRunOutput 没有任何字段或 warnings 条目用于标识“收集失败”,与“程序真没产出”在消费者侧完全无法区分;_filter_failed_empty_outputs 仅在 exit_code!=0 且有文件被丢弃时才有警告,files==[]、exit_code==0 时无任何提示。新测试只覆盖成功路径,未覆盖该分支。
触发条件: 进程退出后收集阶段 _prepare_outputs 抛出任意异常(如本地 collect_files_with_glob 对权限/路径错误抛出的 Exception,或今后对 _prepare_outputs 签名的再次改动)。
实际影响: 用户得到 output_files==[]、primary_output==None、无任何 warnings 的“成功”结果,LLM 会据此得出“程序没有产出”的错误结论(重复运行或误报),save_as_artifacts 侧效应也静默消失;这正是 #350 的症状通过另一条开口复现的路径。
修正方向: 将收集失败显式表达:在该分支向 warnings 追加可消费者可见的提示(如 output collection failed: ...)或给 SkillRunOutput 增加标识字段,并补充失败路径测试,使 #350 类的调用侧错误不再被静默吞掉。
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) |
There was a problem hiding this comment.
问题: 本次变更使 _collect_final_result 的异常分支首次可达(本变更行所在的 _prepare_outputs 调用不再恒抛 TypeError,run_result() 才可能被真正执行并进入其 except 处理器),该处理器内 await exec_session.proc.log(None, None) 位于 except 内部且无嵌套保护,而三个调用点(475/549/613)外层均无 try/except。若 log 同样抛出,异常将直接逃逸出 _collect_final_result,把整个工具调用打成未分类错误;同时该分支 exit_code = exec_session.exit_code or 0 会把 None(未知退出码)映射成成功 0。
触发条件: run_result() 抛出(如会话正被 GC/关闭等终态竞态)后 log() 在异常会话状态下再次抛出;或 exit_code 尚未被任何轮询填充时走异常分支。
实际影响: 静默降级路径变成向上抛异常(本来旨在优雅返回空结果以结束交互链,反而中断整个工具调用);or 0 则使一次实际失败/被杀但未知退出码的运行被报告为 exit_code=0 的成功结果,_filter_failed_empty_outputs 因而保留空输出文件且无警告。
修正方向: 给异常分支的 log() 调用再加一层 try/except 兜底为 total_out="";exit_code 使用 exec_session.exit_code if exec_session.exit_code is not None else 0 显式处理 None,避免错误地报告成功。
| run_tool._prepare_outputs.assert_awaited_once() | ||
| call_args = run_tool._prepare_outputs.await_args.args | ||
| assert call_args == (ctx, session.ws, workspace_runtime, call_args[3]) |
There was a problem hiding this comment.
问题: assert call_args == (ctx, session.ws, workspace_runtime, call_args[3]) 中第 4 个元素与自身比较,是恒真的同义反复:无论生产代码把什么对象作为第 4 个参数(fake_run_input)传入,该断言都不会失败。真正校验第 4 参数内容的只有 _prepare_outputs 桩内部的两条 assert(runtime is workspace_runtime、input_data.output_files == [...])。另外桩内的 is 恒等断言把实现钉死为“必须原样传递存储的 runtime 对象”,若正确实现改为按 ctx 重新解析出等价实例反而会误报。
触发条件: 未来改动把错误的(非空)SkillRunInput 作为第 4 参传入,或对 _prepare_outputs 的实参顺序再调整而绕过桩内 output_files 断言时,测试仍通过。
实际影响: 该断言给出虚假的覆盖信心,无法捕获 #350 这类“参数错误但非缺失”的回归(缺参已被严格签名桩覆盖,但错参不会被外层断言捕获),测试的回归防护弱于注释所宣称的意图。
修正方向: 用捕获的真实 SkillRunInput 对象参与比较:先构造期望的 fake_run_input,再断言 call_args == (ctx, session.ws, workspace_runtime, expected_input),并移除 call_args[3] 的自比较。
| files, manifest = await run_tool._prepare_outputs(ctx, exec_session.ws, exec_session.workspace_runtime, | ||
| fake_run_input) |
There was a problem hiding this comment.
问题: 本次变更使收集路径首次真正执行(本变更行补上 workspace_runtime 后 _prepare_outputs 不再恒抛 TypeError),但 _collect_final_result 组装 SkillRunOutput 时把 stdout 与 stderr 合并进 stdout(第 794 行 total_out),stderr 字段恒为默认空串,duration_ms 恒为 0;而 _skill_run.py:717-721 分别填充 stderr、duration_ms。结果路径激活后,这些输出契约不一致成为可观察行为。
触发条件: 交互命令向 stderr 输出内容(例如 20KB 的错误堆栈)且成功收集;或调用方检查 result.stderr/result.duration_ms 做下游判断。
实际影响: 按 result.stderr 非空判断失败的下游逻辑拿到空串而误判;合并后的 stdout 被截断时警告文案仍是“stdout truncated”(_truncate_output 只作用于合并串),诊断信息归属错误;duration_ms 缺失使依赖耗时的调用方拿到 0。
修正方向: 从 run_result() 分别取 stdout/stderr 并各自 _truncate_output 后分别写入 SkillRunOutput.stdout/stderr,同时填充 duration_ms(异常分支保留合并兜底),使与 skill_run 的输出契约一致。
|
已在 b34b615 中修复并补充回归测试。 |
Now that _prepare_outputs is actually reached, align skill_exec with the skill_run contract: honor omit_inline_content, propagate timed_out/stderr/duration_ms, and surface collection failures via warnings so silent empty-output degradation cannot hide future regressions of the kind reported in trpc-group#350. Also guard the exception fallback path against a secondary log() failure and make an unknown exit code explicit instead of silently reporting success.
AI Code Review审查结论通过 审查范围:02509b2..b34b615(fix: propagate workspace runtime in skill_exec + fix: harden skill_exec output collection path),变更 2 个文件(_skill_exec.py 与 test_skill_exec.py)。计划符合性:修复了 #350 根因(_prepare_outputs 以 3 参调用触发 TypeError 被 except Exception 吞掉、输出静默退化为空),并将 skill_exec 输出收集与 skill_run 契约对齐(stderr/timed_out/duration_ms/omit_inline_content/告警)。已通过 Git diff、全文件阅读、代码执行器契约(WorkspaceRunResult、LocalProgramSession)及 10 个独立审查角度的交叉验证;经 wrapper/代理追踪确认 session 中存储的 workspace_runtime 与启动进程的 runtime 恒为同一对象,无解析发散。已排除的候选:stderr 回退分支合并缓冲问题(本地 run_result 纯读不抛异常、分支不可达)、超时后空输出过滤行为(与 skill_run 既有行为收敛,属计划意图)、exit_code=0 回退(旧行为等同,新告警为改进)。确认问题 1 条(LOW):_collect_final_result 的 finalized check-then-act 非原子,框架在 parallel_tool_calls=True 时并发执行工具调用,同一 session_id 的两个调用可同时越过守卫;本变更首次使非空 files 与重复 save_artifact(v0/v1 双版本)可达。测试覆盖充分(8 个新测试,签名严格 stub),但未覆盖并发终结;test_collect_final_result_propagates_timed_out 断言 exit_code==124 与本地运行时实际 -15 不符,属测试保真度瑕疵。门禁结论:PASSED(仅 1 条 LOW,不阻断合入)。 发现的问题较低
问题: 触发条件: Agent 配置 实际影响: 重复执行完整的输出收集与 修正方向: 在 |
Fixes #350
skill_exec调用SkillRunTool._prepare_outputs时漏传了自614b07d起新增的必需参数workspace_runtime。抛出的
TypeError被宽泛的except捕获,导致输出收集静默降级为空列表:即使命令正常执行、退出码正常,通过output_files/outputs指定的文件也不会返回。结果随后被标记为finalized,后续轮询不会重试收集。本 PR 将启动会话时获取的
workspace_runtime保存到_ExecSession,并在收集最终结果时传入_prepare_outputs;同时补充签名严格的回归测试,避免参数缺失再次被普通 mock 静默吸收。