diff --git a/core/platform_compat.py b/core/platform_compat.py index 715c52eaa..fedc0a303 100644 --- a/core/platform_compat.py +++ b/core/platform_compat.py @@ -118,12 +118,14 @@ def normalize_stdio_command( env: Mapping[str, Any] | None = None, *, inherit_env: bool = True, + map_python_to_current_interpreter: bool = True, ) -> tuple[str, list[str], dict[str, str]]: """Normalize MCP stdio command/env for Windows and virtualenv safety. - - ``python`` / ``python3`` map to the current interpreter on every OS, so - MCP tool servers run in the same environment as the backend — except in - the frozen build, whose executable is not a Python. + - By default, bare ``python`` / ``python3`` map to the current interpreter, + so MCP tool servers run in the backend environment. Callers running + project-owned commands can disable this mapping; frozen builds also leave + Python on PATH because their executable is not a Python. - Windows ``.cmd``/``.bat`` launchers such as ``npx`` are wrapped in ``cmd.exe /d /c`` because CreateProcess does not reliably execute them as stdio children across environments. @@ -136,7 +138,8 @@ def normalize_stdio_command( base = _basename(command) bare_command = "/" not in command and "\\" not in command if ( - bare_command + map_python_to_current_interpreter + and bare_command and base in {"python", "python.exe", "python3", "python3.exe"} and not getattr(sys, "frozen", False) ): diff --git a/core/providers/openai_compat.py b/core/providers/openai_compat.py index 330ca1384..87254ec97 100644 --- a/core/providers/openai_compat.py +++ b/core/providers/openai_compat.py @@ -373,6 +373,7 @@ def _sanitize_messages( messages: list[dict[str, Any]], *, preserve_provider_state: bool = False, + normalize_tool_ids: bool = True, ) -> list[dict[str, Any]]: """Strip non-standard keys, normalize tool_call IDs.""" allowed_keys = ( @@ -384,7 +385,7 @@ def _sanitize_messages( id_map: dict[str, str] = {} def map_id(value: Any) -> Any: - if not isinstance(value, str): + if not normalize_tool_ids or not isinstance(value, str): return value return id_map.setdefault(value, self._normalize_tool_call_id(value)) @@ -624,7 +625,9 @@ def _build_responses_body( self.validate_request_capabilities(messages, tools) model_name = model or self.default_model sanitized_messages = self._sanitize_messages( - self._sanitize_empty_content(messages), preserve_provider_state=True + self._sanitize_empty_content(messages), + preserve_provider_state=True, + normalize_tool_ids=False, ) instructions, input_items = convert_messages(sanitized_messages) diff --git a/core/providers/openai_responses/converters.py b/core/providers/openai_responses/converters.py index 4b8b5120c..e0f322330 100644 --- a/core/providers/openai_responses/converters.py +++ b/core/providers/openai_responses/converters.py @@ -56,13 +56,13 @@ def convert_messages( "id": f"msg_{idx}", } ) - for tool_call in msg.get("tool_calls", []) or []: + for tool_idx, tool_call in enumerate(msg.get("tool_calls", []) or []): fn = tool_call.get("function") or {} call_id, item_id = split_tool_call_id(tool_call.get("id")) input_items.append( { "type": "function_call", - "id": item_id or f"fc_{idx}", + "id": item_id or f"fc_{idx}_{tool_idx}", "call_id": call_id or f"call_{idx}", "name": fn.get("name"), "arguments": fn.get("arguments") or "{}", diff --git a/core/providers/openai_responses/parsing.py b/core/providers/openai_responses/parsing.py index 57d915e4b..78ea86c0b 100644 --- a/core/providers/openai_responses/parsing.py +++ b/core/providers/openai_responses/parsing.py @@ -27,6 +27,11 @@ def map_finish_reason(status: str | None) -> str: return FINISH_REASON_MAP.get(status or "completed", "stop") +def _tool_call_id(call_id: str, item_id: Any) -> str: + """Keep Responses item IDs when present; converters assign missing IDs uniquely.""" + return f"{call_id}|{item_id}" if item_id else call_id + + async def iter_sse(response: httpx.Response) -> AsyncGenerator[dict[str, Any], None]: """Yield parsed JSON events from a Responses API SSE stream.""" buffer: list[str] = [] @@ -83,7 +88,7 @@ async def consume_sse( if not call_id: continue tool_call_buffers[call_id] = { - "id": item.get("id") or "fc_0", + "id": item.get("id"), "name": item.get("name"), "arguments": item.get("arguments") or "", } @@ -125,7 +130,7 @@ async def consume_sse( args = {"raw": args_raw} tool_calls.append( ToolCallRequest( - id=f"{call_id}|{buf.get('id') or item.get('id') or 'fc_0'}", + id=_tool_call_id(call_id, buf.get("id") or item.get("id")), name=buf.get("name") or item.get("name") or "", arguments=args, ) @@ -175,7 +180,7 @@ def parse_response_output(response: Any) -> LLMResponse: reasoning_content = (reasoning_content or "") + s["text"] elif item_type == "function_call": call_id = item.get("call_id") or "" - item_id = item.get("id") or "fc_0" + item_id = item.get("id") args_raw = item.get("arguments") or "{}" try: args = json.loads(args_raw) if isinstance(args_raw, str) else args_raw @@ -194,7 +199,7 @@ def parse_response_output(response: Any) -> LLMResponse: args = {"raw": args_raw} tool_calls.append( ToolCallRequest( - id=f"{call_id}|{item_id}", + id=_tool_call_id(call_id, item_id), name=item.get("name") or "", arguments=args if isinstance(args, dict) else {}, ) @@ -264,7 +269,7 @@ async def consume_sdk_stream( if not call_id: continue tool_call_buffers[call_id] = { - "id": getattr(item, "id", None) or "fc_0", + "id": getattr(item, "id", None), "name": getattr(item, "name", None), "arguments": getattr(item, "arguments", None) or "", } @@ -312,7 +317,9 @@ async def consume_sdk_stream( args = {"raw": args_raw} tool_calls.append( ToolCallRequest( - id=f"{call_id}|{buf.get('id') or getattr(item, 'id', None) or 'fc_0'}", + id=_tool_call_id( + call_id, buf.get("id") or getattr(item, "id", None) + ), name=buf.get("name") or getattr(item, "name", None) or "", arguments=args, ) diff --git a/core/schedule/monitor.py b/core/schedule/monitor.py index 6c0654921..c939406c7 100644 --- a/core/schedule/monitor.py +++ b/core/schedule/monitor.py @@ -493,7 +493,7 @@ def main(argv: list[str] | None = None) -> int: print(f"[monitor] changed: {args.job_id}") if outcome.context_block: print(outcome.context_block) - return 0 + return 1 if outcome.error else 0 if __name__ == "__main__": diff --git a/core/verification.py b/core/verification.py index f85175a27..9fa9f7a50 100644 --- a/core/verification.py +++ b/core/verification.py @@ -11,6 +11,8 @@ from dataclasses import dataclass from pathlib import Path +from core.platform_compat import normalize_stdio_command + MAX_VERIFICATION_OUTPUT = 64 * 1024 MAX_DISCOVERY_FILES = 256 @@ -105,12 +107,18 @@ def run_verification( timeout_seconds: int = 300, ) -> VerificationResult: started = time.monotonic() + executable, arguments, environment = normalize_stdio_command( + command.argv[0], + list(command.argv[1:]), + {"CI": "1", "PYTHONUNBUFFERED": "1"}, + map_python_to_current_interpreter=False, + ) process = subprocess.Popen( - command.argv, + [executable, *arguments], cwd=root, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - env={**os.environ, "CI": "1", "PYTHONUNBUFFERED": "1"}, + env=environment, start_new_session=os.name != "nt", ) stdout_raw, stderr_raw, timed_out, truncated = _capture(process, timeout_seconds) diff --git a/tests/test_monitor_cli_status.py b/tests/test_monitor_cli_status.py new file mode 100644 index 000000000..3b502e48f --- /dev/null +++ b/tests/test_monitor_cli_status.py @@ -0,0 +1,58 @@ +"""Exercise the documented module CLI, its shell source and persistent state.""" + +import json +import subprocess +import sys + +import pytest + +from core.schedule.monitor import MonitorStore + + +@pytest.mark.parametrize("as_json", [False, True]) +def test_source_failure_returns_nonzero_without_changing_state(tmp_path, as_json): + db = tmp_path / "monitor.sqlite3" + store = MonitorStore(str(db)) + store.save_job("sample", "previous-hash", "previous-output") + previous = store.get_job("sample") + command = [ + sys.executable, + "-m", + "core.schedule.monitor", + "--job-id", + "sample", + "--script", + "exit 3", + "--db", + str(db), + ] + if as_json: + command.append("--json") + result = subprocess.run(command, capture_output=True, text=True, timeout=15) + assert store.get_job("sample") == previous + if as_json: + assert json.loads(result.stdout)["error"] + else: + assert "ERROR" in result.stderr + assert result.returncode == 1 + + +def test_json_success_and_unchanged_source_return_zero(tmp_path): + command = [ + sys.executable, + "-m", + "core.schedule.monitor", + "--job-id", + "sample", + "--script", + "echo stable", + "--db", + str(tmp_path / "monitor.sqlite3"), + "--json", + ] + for changed in (True, False): + result = subprocess.run(command, capture_output=True, text=True, timeout=15) + assert result.returncode == 0 + outcome = json.loads(result.stdout) + assert outcome["changed"] is changed + assert not outcome["error"] diff --git a/tests/test_platform_compat.py b/tests/test_platform_compat.py index 87be1e495..b482dc8c6 100644 --- a/tests/test_platform_compat.py +++ b/tests/test_platform_compat.py @@ -24,6 +24,17 @@ def test_normalize_stdio_command_maps_only_bare_python_names() -> None: assert command == explicit +def test_normalize_stdio_command_can_preserve_project_python_name() -> None: + command, _args, _environment = normalize_stdio_command( + "python3", + [], + {}, + inherit_env=False, + map_python_to_current_interpreter=False, + ) + assert command == "python3" + + def test_frozen_build_leaves_python_to_the_path(monkeypatch) -> None: """The Desktop build's executable is the App Server, not a Python: an MCP server configured as ``python server.py`` needs the user's Python.""" diff --git a/tests/test_reasoning_providers.py b/tests/test_reasoning_providers.py index a28f88584..b15faf745 100644 --- a/tests/test_reasoning_providers.py +++ b/tests/test_reasoning_providers.py @@ -375,3 +375,36 @@ async def on_reasoning(delta: str, channel: ReasoningChannel) -> None: ("private-1", expected_channel), ("private-2", expected_channel), ] + + +@pytest.mark.parametrize("compound_ids", [False, True]) +def test_responses_parallel_tool_calls_preserve_distinct_identities(compound_ids): + provider = _openai_provider() + ids = [f"call_{i}|fc_{i}" if compound_ids else f"call_{i}" for i in range(2)] + messages = [ + { + "role": "assistant", + "content": None, + "tool_calls": [ + { + "id": identifier, + "type": "function", + "function": {"name": "probe", "arguments": "{}"}, + } + for identifier in ids + ], + } + ] + [ + {"role": "tool", "tool_call_id": identifier, "content": "ok"} + for identifier in ids + ] + body = provider._build_responses_body( + messages, None, "gpt-5.4", 1024, 0.1, None, None + ) + calls = [item for item in body["input"] if item["type"] == "function_call"] + outputs = [item for item in body["input"] if item["type"] == "function_call_output"] + assert [item["call_id"] for item in calls] == ["call_0", "call_1"] + assert [item["call_id"] for item in outputs] == ["call_0", "call_1"] + assert len({item["id"] for item in calls}) == 2 + if compound_ids: + assert [item["id"] for item in calls] == ["fc_0", "fc_1"] diff --git a/tests/test_responses_identity_roundtrip.py b/tests/test_responses_identity_roundtrip.py new file mode 100644 index 000000000..d4ac0e444 --- /dev/null +++ b/tests/test_responses_identity_roundtrip.py @@ -0,0 +1,131 @@ +"""Responses output must survive the next public provider call unchanged.""" + +import json + +import httpx +import pytest +from openai import AsyncOpenAI + +from core.providers.openai_compat import OpenAICompatProvider +from core.providers.registry import find_by_name + + +@pytest.mark.asyncio +@pytest.mark.parametrize("include_item_ids", [True, False]) +async def test_responses_tool_result_roundtrip(include_item_ids): + bodies = [] + + def endpoint(request): + body = json.loads(request.content) + bodies.append(body) + if len(bodies) == 1: + output = [ + { + **({"id": f"fc_{i}"} if include_item_ids else {}), + "type": "function_call", + "call_id": f"call_{i}", + "name": "probe", + "arguments": "{}", + "status": "completed", + } + for i in range(2) + ] + else: + calls = [x for x in body["input"] if x.get("type") == "function_call"] + results = [ + x for x in body["input"] if x.get("type") == "function_call_output" + ] + expected_item_ids = ( + ["fc_0", "fc_1"] if include_item_ids else ["fc_1_0", "fc_1_1"] + ) + if ( + [x["call_id"] for x in calls] != ["call_0", "call_1"] + or [x["id"] for x in calls] != expected_item_ids + or [x["call_id"] for x in results] != ["call_0", "call_1"] + ): + return httpx.Response( + 400, + json={ + "error": { + "message": "tool identity did not match the preceding response", + "type": "invalid_request_error", + } + }, + ) + output = [ + { + "type": "message", + "id": "msg_final", + "role": "assistant", + "status": "completed", + "content": [ + {"type": "output_text", "text": "done", "annotations": []} + ], + } + ] + return httpx.Response( + 200, + json={ + "id": "resp_fixture", + "object": "response", + "created_at": 1, + "model": "gpt-5.4", + "status": "completed", + "output": output, + }, + ) + + provider = OpenAICompatProvider( + api_key="fixture", + api_base="https://fixture.invalid/v1", + default_model="gpt-5.4", + protocol="openai_responses", + spec=find_by_name("openai"), + ) + await provider._client.close() + provider._client = AsyncOpenAI( + api_key="fixture", + base_url="https://fixture.invalid/v1", + max_retries=0, + http_client=httpx.AsyncClient(transport=httpx.MockTransport(endpoint)), + ) + try: + history = [{"role": "user", "content": "probe twice"}] + tools = [ + { + "type": "function", + "function": { + "name": "probe", + "description": "Return a probe result", + "parameters": {"type": "object", "properties": {}}, + }, + } + ] + first = await provider.chat(history, tools=tools) + assert len(first.tool_calls) == 2 + history.append( + { + "role": "assistant", + "content": first.content, + "tool_calls": [ + { + "id": tc.id, + "type": "function", + "function": { + "name": tc.name, + "arguments": json.dumps(tc.arguments), + }, + } + for tc in first.tool_calls + ], + } + ) + history.extend( + {"role": "tool", "tool_call_id": tc.id, "content": "ok"} + for tc in first.tool_calls + ) + final = await provider.chat(history, tools=tools) + assert final.finish_reason != "error", final.content + assert final.content == "done" + finally: + await provider.aclose() diff --git a/tests/test_responses_missing_item_ids.py b/tests/test_responses_missing_item_ids.py new file mode 100644 index 000000000..ca87ff095 --- /dev/null +++ b/tests/test_responses_missing_item_ids.py @@ -0,0 +1,129 @@ +"""Missing Responses item IDs must not collide across parallel tool calls.""" + +from __future__ import annotations + +import json +from types import SimpleNamespace + +import httpx +import pytest + +from core.providers.openai_responses.converters import convert_messages +from core.providers.openai_responses.parsing import ( + consume_sdk_stream, + consume_sse, + parse_response_output, +) + + +def _assert_converter_assigns_unique_item_ids(tool_calls) -> None: + messages = [ + { + "role": "assistant", + "content": None, + "tool_calls": [ + { + "id": call.id, + "type": "function", + "function": { + "name": call.name, + "arguments": json.dumps(call.arguments), + }, + } + for call in tool_calls + ], + } + ] + _, items = convert_messages(messages) + calls = [item for item in items if item.get("type") == "function_call"] + assert [item["call_id"] for item in calls] == ["call_0", "call_1"] + assert [item["id"] for item in calls] == ["fc_0_0", "fc_0_1"] + + +def test_sdk_response_omitted_item_ids_defer_to_converter_fallback() -> None: + response = parse_response_output( + { + "status": "completed", + "output": [ + { + "type": "function_call", + "call_id": f"call_{index}", + "name": "probe", + "arguments": "{}", + } + for index in range(2) + ], + } + ) + + assert [call.id for call in response.tool_calls] == ["call_0", "call_1"] + _assert_converter_assigns_unique_item_ids(response.tool_calls) + + +@pytest.mark.asyncio +async def test_sse_omitted_item_ids_defer_to_converter_fallback() -> None: + events = [] + for index in range(2): + item = { + "type": "function_call", + "call_id": f"call_{index}", + "name": "probe", + "arguments": "{}", + } + events.extend( + [ + {"type": "response.output_item.added", "item": item}, + {"type": "response.output_item.done", "item": item}, + ] + ) + events.append({"type": "response.completed", "response": {"status": "completed"}}) + payload = "".join(f"data: {json.dumps(event)}\n\n" for event in events) + + _content, tool_calls, finish_reason = await consume_sse( + httpx.Response(200, content=payload.encode("utf-8")) + ) + + assert finish_reason == "stop" + assert [call.id for call in tool_calls] == ["call_0", "call_1"] + _assert_converter_assigns_unique_item_ids(tool_calls) + + +@pytest.mark.asyncio +async def test_sdk_stream_omitted_item_ids_defer_to_converter_fallback() -> None: + events = [] + for index in range(2): + item = SimpleNamespace( + type="function_call", + call_id=f"call_{index}", + name="probe", + arguments="{}", + ) + events.extend( + [ + SimpleNamespace(type="response.output_item.added", item=item), + SimpleNamespace(type="response.output_item.done", item=item), + ] + ) + events.append( + SimpleNamespace( + type="response.completed", + response=SimpleNamespace(status="completed", usage=None, output=[]), + ) + ) + + async def stream(): + for event in events: + yield event + + ( + _content, + tool_calls, + finish_reason, + _usage, + _reasoning, + _state, + ) = await consume_sdk_stream(stream()) + + assert finish_reason == "stop" + assert [call.id for call in tool_calls] == ["call_0", "call_1"] + _assert_converter_assigns_unique_item_ids(tool_calls) diff --git a/tests/test_verification.py b/tests/test_verification.py index c9968b4f4..b1e78f805 100644 --- a/tests/test_verification.py +++ b/tests/test_verification.py @@ -1,8 +1,18 @@ from __future__ import annotations +import json +import os +import shutil +import sys from pathlib import Path -from core.verification import discover_verification_commands, run_verification +import pytest + +from core.verification import ( + VerificationCommand, + discover_verification_commands, + run_verification, +) def test_discovers_real_test_scripts_and_rejects_npm_placeholder( @@ -56,7 +66,25 @@ def test_root_level_pytest_style_files_keep_pytest_runner(tmp_path: Path) -> Non assert [command.id for command in commands] == ["pytest"] -def test_verification_reports_pass_and_keeps_output_bounded(tmp_path: Path) -> None: +def test_verification_reports_pass_and_keeps_output_bounded( + monkeypatch, tmp_path: Path +) -> None: + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + if os.name == "nt": + launcher = bin_dir / "python3.cmd" + launcher.write_text(f'@echo off\r\n"{sys.executable}" %*\r\n', encoding="utf-8") + else: + launcher = bin_dir / "python3" + launcher.write_text( + f'#!/bin/sh\nexec "{sys.executable}" "$@"\n', encoding="utf-8" + ) + launcher.chmod(0o755) + monkeypatch.setenv( + "PATH", + os.pathsep.join(filter(None, (str(bin_dir), os.environ.get("PATH", "")))), + ) + tests_dir = tmp_path / "tests" tests_dir.mkdir() (tests_dir / "test_ok.py").write_text( @@ -70,3 +98,45 @@ def test_verification_reports_pass_and_keeps_output_bounded(tmp_path: Path) -> N assert result.timed_out is False assert len(result.stdout.encode()) <= 64 * 1024 assert len(result.stderr.encode()) <= 64 * 1024 + + +def test_verification_uses_the_project_python_from_path( + monkeypatch, tmp_path: Path +) -> None: + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + if os.name == "nt": + launcher = bin_dir / "python3.cmd" + launcher.write_text("@echo off\r\necho project-python\r\n", encoding="utf-8") + else: + launcher = bin_dir / "python3" + launcher.write_text("#!/bin/sh\nprintf 'project-python\\n'\n", encoding="utf-8") + launcher.chmod(0o755) + monkeypatch.setenv( + "PATH", + os.pathsep.join(filter(None, (str(bin_dir), os.environ.get("PATH", "")))), + ) + + command = VerificationCommand( + "python-probe", "Project Python", ("python3", "-c", "pass") + ) + result = run_verification(tmp_path, command, timeout_seconds=15) + + assert result.passed + assert result.stdout.strip() == "project-python" + + +@pytest.mark.skipif(shutil.which("npm") is None, reason="npm is not installed") +def test_verification_runs_an_actual_npm_test_script(tmp_path: Path) -> None: + root = tmp_path / "project with spaces" + root.mkdir() + (root / "package.json").write_text( + json.dumps({"scripts": {"test": "node -e \"console.log('verification-ok')\""}}), + encoding="utf-8", + ) + command = discover_verification_commands(root)[0] + + result = run_verification(root, command, timeout_seconds=15) + + assert result.passed + assert "verification-ok" in result.stdout