From 6f28a4d0fe47de87f1e667fb30191f23618f1927 Mon Sep 17 00:00:00 2001 From: kinolo <2279317070@qq.com> Date: Tue, 29 Sep 2026 22:11:54 +0800 Subject: [PATCH 1/2] fix: propagate workspace runtime in skill_exec --- tests/skills/tools/test_skill_exec.py | 103 +++++++++++++++++++++ trpc_agent_sdk/skills/tools/_skill_exec.py | 9 +- 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/tests/skills/tools/test_skill_exec.py b/tests/skills/tools/test_skill_exec.py index 1593e5d95..7ed598fe2 100644 --- a/tests/skills/tools/test_skill_exec.py +++ b/tests/skills/tools/test_skill_exec.py @@ -17,10 +17,14 @@ from trpc_agent_sdk.skills.tools._skill_exec import SkillExecTool from trpc_agent_sdk.skills.tools._skill_exec import WriteStdinTool from trpc_agent_sdk.skills.tools._skill_exec import _close_session +from trpc_agent_sdk.skills.tools._skill_exec import _collect_final_result from trpc_agent_sdk.skills.tools._skill_exec import _detect_interaction +from trpc_agent_sdk.skills.tools._skill_exec import _ExecSession from trpc_agent_sdk.skills.tools._skill_exec import _has_selection_items from trpc_agent_sdk.skills.tools._skill_exec import _last_non_empty_line +from trpc_agent_sdk.skills.tools._skill_exec import _start_session from trpc_agent_sdk.skills.tools._skill_exec import create_exec_tools +from trpc_agent_sdk.skills.tools._skill_run import SkillRunFile def _make_exec_tool() -> SkillExecTool: @@ -107,3 +111,102 @@ async def test_close_session(self): sess.proc.close = AsyncMock() await _close_session(sess) sess.proc.close.assert_awaited_once() + + +class TestStartSession: + @pytest.mark.asyncio + async def test_start_session_stores_workspace_runtime(self): + workspace_runtime = MagicMock(name="workspace_runtime") + ws = MagicMock(name="ws") + proc = MagicMock(name="proc") + runner = MagicMock() + runner.start_program = AsyncMock(return_value=proc) + inputs = ExecInput(skill="s", command="echo hi") + + session = await _start_session( + runner=runner, + tool_context=MagicMock(), + inputs=inputs, + ws=ws, + workspace_runtime=workspace_runtime, + rel_cwd="skills/s", + env={"A": "1"}, + ) + + assert isinstance(session, _ExecSession) + assert session.workspace_runtime is workspace_runtime + assert session.ws is ws + assert session.proc is proc + assert session.in_data is inputs + + +class TestCollectFinalResult: + """Regression coverage for issue #350. + + ``_prepare_outputs`` requires ``workspace_runtime``. A plain ``AsyncMock`` + swallows the missing-argument ``TypeError``, so the collection path silently + degrades to empty files. These tests bind a signature-strict stub so a + dropped argument fails the assertions instead of being absorbed. + """ + + def _make_session(self, workspace_runtime, in_data=None): + proc = MagicMock() + proc.run_result = AsyncMock(return_value=MagicMock(stdout="done\n", stderr="", exit_code=0)) + return _ExecSession( + proc=proc, + ws=MagicMock(name="ws"), + workspace_runtime=workspace_runtime, + in_data=in_data or ExecInput(skill="s", command="echo hi", output_files=["out/result.txt"]), + ) + + @pytest.mark.asyncio + async def test_collect_final_result_passes_workspace_runtime_and_files(self): + workspace_runtime = MagicMock(name="workspace_runtime") + session = self._make_session(workspace_runtime) + expected_files = [SkillRunFile( + name="out/result.txt", + content="ok", + mime_type="text/plain", + size_bytes=2, + )] + + async def _prepare_outputs(ctx, ws, runtime, input_data): + # Signature-strict: a missing argument raises TypeError here and + # the production `except Exception` would degrade to empty files. + assert runtime is workspace_runtime + assert input_data.output_files == ["out/result.txt"] + return list(expected_files), None + + run_tool = MagicMock() + run_tool._prepare_outputs = AsyncMock(side_effect=_prepare_outputs) + run_tool._attach_artifacts_if_requested = AsyncMock() + run_tool._merge_manifest_artifact_refs = MagicMock() + + ctx = MagicMock() + result = await _collect_final_result(ctx, session, run_tool) + + assert result is not None + assert result.exit_code == 0 + assert result.output_files == expected_files + assert result.primary_output is not None + assert result.primary_output.name == "out/result.txt" + assert session.finalized is True + assert session.final_result is result + + 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]) + + @pytest.mark.asyncio + async def test_collect_final_result_returns_cached_when_finalized(self): + session = self._make_session(MagicMock()) + cached = MagicMock(name="cached") + session.finalized = True + session.final_result = cached + + run_tool = MagicMock() + run_tool._prepare_outputs = AsyncMock() + + result = await _collect_final_result(MagicMock(), session, run_tool) + assert result is cached + run_tool._prepare_outputs.assert_not_awaited() diff --git a/trpc_agent_sdk/skills/tools/_skill_exec.py b/trpc_agent_sdk/skills/tools/_skill_exec.py index b093088b8..20ab96eb7 100644 --- a/trpc_agent_sdk/skills/tools/_skill_exec.py +++ b/trpc_agent_sdk/skills/tools/_skill_exec.py @@ -52,6 +52,7 @@ from pydantic import Field from trpc_agent_sdk.code_executors import BaseProgramRunner from trpc_agent_sdk.code_executors import BaseProgramSession +from trpc_agent_sdk.code_executors import BaseWorkspaceRuntime from trpc_agent_sdk.code_executors import DEFAULT_EXEC_YIELD_MS from trpc_agent_sdk.code_executors import DEFAULT_IO_YIELD_MS from trpc_agent_sdk.code_executors import DEFAULT_SESSION_KILL_SEC @@ -216,6 +217,7 @@ class _ExecSession: proc: BaseProgramSession ws: WorkspaceInfo + workspace_runtime: BaseWorkspaceRuntime in_data: ExecInput # Final state @@ -458,6 +460,7 @@ async def _run_async_impl( tool_context=tool_context, inputs=inputs, ws=ws, + workspace_runtime=workspace_runtime, rel_cwd=rel_cwd, env=merged_env, ) @@ -730,6 +733,7 @@ async def _start_session( tool_context: InvocationContext, inputs: ExecInput, ws: WorkspaceInfo, + workspace_runtime: BaseWorkspaceRuntime, rel_cwd: str, env: dict[str, str], ) -> _ExecSession: @@ -744,7 +748,7 @@ async def _start_session( tty=inputs.tty, ) proc = await runner.start_program(tool_context, ws, spec) - return _ExecSession(proc=proc, ws=ws, in_data=inputs) + return _ExecSession(proc=proc, ws=ws, workspace_runtime=workspace_runtime, in_data=inputs) async def _write_stdin(exec_session: _ExecSession, chars: str, submit: bool) -> None: @@ -779,7 +783,8 @@ async def _collect_final_result( outputs=in_data.outputs, ) 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 From b34b61513a296b5646e66620e72e818a22744cce Mon Sep 17 00:00:00 2001 From: kinolo <2279317070@qq.com> Date: Tue, 29 Sep 2026 23:27:39 +0800 Subject: [PATCH 2/2] fix: harden skill_exec output collection path 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 #350. Also guard the exception fallback path against a secondary log() failure and make an unknown exit code explicit instead of silently reporting success. --- tests/skills/tools/test_skill_exec.py | 129 +++++++++++++++++++-- trpc_agent_sdk/skills/tools/_skill_exec.py | 45 +++++-- 2 files changed, 156 insertions(+), 18 deletions(-) diff --git a/tests/skills/tools/test_skill_exec.py b/tests/skills/tools/test_skill_exec.py index 7ed598fe2..aea41ca07 100644 --- a/tests/skills/tools/test_skill_exec.py +++ b/tests/skills/tools/test_skill_exec.py @@ -25,6 +25,7 @@ from trpc_agent_sdk.skills.tools._skill_exec import _start_session from trpc_agent_sdk.skills.tools._skill_exec import create_exec_tools from trpc_agent_sdk.skills.tools._skill_run import SkillRunFile +from trpc_agent_sdk.skills.tools._skill_run import SkillRunInput def _make_exec_tool() -> SkillExecTool: @@ -149,9 +150,15 @@ class TestCollectFinalResult: dropped argument fails the assertions instead of being absorbed. """ - def _make_session(self, workspace_runtime, in_data=None): + def _make_session(self, workspace_runtime, in_data=None, run_result=None): proc = MagicMock() - proc.run_result = AsyncMock(return_value=MagicMock(stdout="done\n", stderr="", exit_code=0)) + proc.run_result = AsyncMock(return_value=run_result or MagicMock( + stdout="done\n", + stderr="", + exit_code=0, + timed_out=False, + duration=0.0, + )) return _ExecSession( proc=proc, ws=MagicMock(name="ws"), @@ -159,6 +166,16 @@ def _make_session(self, workspace_runtime, in_data=None): in_data=in_data or ExecInput(skill="s", command="echo hi", output_files=["out/result.txt"]), ) + def _make_run_tool(self, prepare_side_effect=None, files=None): + run_tool = MagicMock() + run_tool._prepare_outputs = AsyncMock( + side_effect=prepare_side_effect, + return_value=(files if files is not None else [], None), + ) + run_tool._attach_artifacts_if_requested = AsyncMock() + run_tool._merge_manifest_artifact_refs = MagicMock() + return run_tool + @pytest.mark.asyncio async def test_collect_final_result_passes_workspace_runtime_and_files(self): workspace_runtime = MagicMock(name="workspace_runtime") @@ -177,11 +194,7 @@ async def _prepare_outputs(ctx, ws, runtime, input_data): assert input_data.output_files == ["out/result.txt"] return list(expected_files), None - run_tool = MagicMock() - run_tool._prepare_outputs = AsyncMock(side_effect=_prepare_outputs) - run_tool._attach_artifacts_if_requested = AsyncMock() - run_tool._merge_manifest_artifact_refs = MagicMock() - + run_tool = self._make_run_tool(prepare_side_effect=_prepare_outputs) ctx = MagicMock() result = await _collect_final_result(ctx, session, run_tool) @@ -190,12 +203,19 @@ async def _prepare_outputs(ctx, ws, runtime, input_data): assert result.output_files == expected_files assert result.primary_output is not None assert result.primary_output.name == "out/result.txt" + assert result.timed_out is False + assert result.stderr == "" assert session.finalized is True assert session.final_result is result 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]) + expected_input = SkillRunInput( + skill="s", + command="echo hi", + output_files=["out/result.txt"], + ) + assert call_args == (ctx, session.ws, workspace_runtime, expected_input) @pytest.mark.asyncio async def test_collect_final_result_returns_cached_when_finalized(self): @@ -204,9 +224,98 @@ async def test_collect_final_result_returns_cached_when_finalized(self): session.finalized = True session.final_result = cached - run_tool = MagicMock() - run_tool._prepare_outputs = AsyncMock() + run_tool = self._make_run_tool() result = await _collect_final_result(MagicMock(), session, run_tool) assert result is cached run_tool._prepare_outputs.assert_not_awaited() + + @pytest.mark.asyncio + async def test_collect_final_result_omit_inline_content_clears_file_content(self): + in_data = ExecInput( + skill="s", + command="echo hi", + output_files=["out/result.txt"], + omit_inline_content=True, + ) + session = self._make_session(MagicMock(), in_data=in_data) + collected = [SkillRunFile( + name="out/result.txt", + content="secret payload", + mime_type="text/plain", + size_bytes=14, + )] + run_tool = self._make_run_tool(files=collected) + + result = await _collect_final_result(MagicMock(), session, run_tool) + + assert result is not None + assert len(result.output_files) == 1 + assert result.output_files[0].content == "" + assert result.output_files[0].name == "out/result.txt" + assert result.output_files[0].size_bytes == 14 + assert result.primary_output is not None + assert result.primary_output.content == "" + + @pytest.mark.asyncio + async def test_collect_final_result_propagates_timed_out(self): + run_result = MagicMock(stdout="", stderr="", exit_code=124, timed_out=True, duration=1.5) + session = self._make_session(MagicMock(), run_result=run_result) + collected = [SkillRunFile(name="out/partial.txt", content="", mime_type="text/plain", size_bytes=0)] + run_tool = self._make_run_tool(files=collected) + + result = await _collect_final_result(MagicMock(), session, run_tool) + + assert result is not None + assert result.timed_out is True + assert result.exit_code == 124 + assert result.duration_ms == 1500 + # Failed + timed_out run drops empty output files (same as skill_run). + assert result.output_files == [] + assert any("empty output_files" in w for w in result.warnings) + + @pytest.mark.asyncio + async def test_collect_final_result_fills_stderr(self): + run_result = MagicMock(stdout="out\n", stderr="boom\n", exit_code=1, timed_out=False, duration=0.25) + session = self._make_session(MagicMock(), run_result=run_result) + run_tool = self._make_run_tool() + + result = await _collect_final_result(MagicMock(), session, run_tool) + + assert result is not None + assert result.stdout == "out\n" + assert result.stderr == "boom\n" + assert result.duration_ms == 250 + + @pytest.mark.asyncio + async def test_collect_final_result_warns_on_collection_failure(self): + async def _prepare_outputs(ctx, ws, runtime, input_data): + raise RuntimeError("glob failed") + + session = self._make_session(MagicMock()) + run_tool = self._make_run_tool(prepare_side_effect=_prepare_outputs) + + result = await _collect_final_result(MagicMock(), session, run_tool) + + assert result is not None + assert result.output_files == [] + assert any("output collection failed" in w for w in result.warnings) + # Still finalized so later polls do not retry a broken collection. + assert session.finalized is True + + @pytest.mark.asyncio + async def test_collect_final_result_exception_branch_survives_log_failure(self): + session = self._make_session(MagicMock()) + session.proc.run_result = AsyncMock(side_effect=RuntimeError("session gone")) + session.proc.log = AsyncMock(side_effect=RuntimeError("log gone")) + session.exit_code = None + run_tool = self._make_run_tool() + + result = await _collect_final_result(MagicMock(), session, run_tool) + + assert result is not None + assert result.stdout == "" + # Unknown exit code is surfaced via warnings instead of silent success. + assert result.exit_code == 0 + assert any("exit code unavailable" in w for w in result.warnings) + assert session.finalized is True diff --git a/trpc_agent_sdk/skills/tools/_skill_exec.py b/trpc_agent_sdk/skills/tools/_skill_exec.py index 20ab96eb7..993dc77ec 100644 --- a/trpc_agent_sdk/skills/tools/_skill_exec.py +++ b/trpc_agent_sdk/skills/tools/_skill_exec.py @@ -91,6 +91,8 @@ from ._skill_run import SkillRunInput from ._skill_run import SkillRunOutput from ._skill_run import SkillRunTool +from ._skill_run import _WARN_STDERR_TRUNCATED +from ._skill_run import _WARN_STDOUT_TRUNCATED from ._skill_run import _filter_failed_empty_outputs from ._skill_run import _select_primary_output from ._skill_run import _truncate_output @@ -782,36 +784,56 @@ async def _collect_final_result( inputs=in_data.inputs, outputs=in_data.outputs, ) + warnings: list[str] = [] try: 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 + warnings.append(f"output collection failed: {ex}") + timed_out = False + duration_ms = 0 try: run_result = await exec_session.proc.run_result() - total_out = (run_result.stdout or "") + (run_result.stderr or "") + stdout_raw = run_result.stdout or "" + stderr_raw = run_result.stderr or "" exit_code = run_result.exit_code + timed_out = bool(getattr(run_result, "timed_out", False)) + duration_ms = int((getattr(run_result, "duration", 0) or 0) * 1000) except Exception: # pylint: disable=broad-except - run_log = await exec_session.proc.log(None, None) - total_out = run_log.output or "" - exit_code = exec_session.exit_code or 0 + try: + run_log = await exec_session.proc.log(None, None) + stdout_raw = run_log.output or "" + except Exception: # pylint: disable=broad-except + stdout_raw = "" + stderr_raw = "" + if exec_session.exit_code is not None: + exit_code = exec_session.exit_code + else: + exit_code = 0 + warnings.append("exit code unavailable after run_result failure; reported as 0") # Reuse the same output-quality helpers as skill_run - warnings: list[str] = [] - stdout, trunc = _truncate_output(total_out) + stdout, trunc = _truncate_output(stdout_raw) if trunc: - warnings.append("stdout truncated") + warnings.append(_WARN_STDOUT_TRUNCATED) + stderr, trunc = _truncate_output(stderr_raw) + if trunc: + warnings.append(_WARN_STDERR_TRUNCATED) - files, filter_warns = _filter_failed_empty_outputs(exit_code, False, files) + files, filter_warns = _filter_failed_empty_outputs(exit_code, timed_out, files) warnings.extend(filter_warns) primary = _select_primary_output(files) result = SkillRunOutput( stdout=stdout, + stderr=stderr, exit_code=exit_code, + timed_out=timed_out, + duration_ms=duration_ms, output_files=files, primary_output=primary, warnings=warnings, @@ -825,6 +847,13 @@ async def _collect_final_result( if manifest: run_tool._merge_manifest_artifact_refs(manifest, result) + # omit_inline_content: keep metadata only, match skill_run contract + if in_data.omit_inline_content: + for f in result.output_files: + f.content = "" + if result.primary_output: + result.primary_output.content = "" + exec_session.final_result = result exec_session.finalized = True return result