Merge pull request #831 from agentforce314/fix/ci-windows-and-openai3

Author: agentforce314

.github/workflows/ci.yml

@@ -93,7 +93,21 @@ jobs:
         run: npm run typecheck
 
       - name: Unit tests (ui + electron projects)
-        run: npx vitest run
+        # junit output feeds the same "Test Results" publisher as the pytest
+        # legs (test-results.yml globs every artifact for *.xml), because the
+        # desktop job's failures are otherwise invisible from outside: job
+        # LOGS need an authenticated API call, artifacts do not — and this
+        # job spent its first days red on windows-latest with nothing but a
+        # ✗ to diagnose from.
+        run: npx vitest run --reporter=default --reporter=junit --outputFile.junit=test-results/desktop.xml
+
+      - name: Upload test results
+        if: (!cancelled())
+        uses: actions/upload-artifact@v4
+        with:
+          name: Test Results (Desktop, ${{ matrix.os }})
+          path: ui-desktop/test-results/*.xml
+          retention-days: 3
 
   harbor-adapter:
     # The harbor eval adapter (eval/harbor/clawcodex_agent.py) cannot be

pyproject.toml

@@ -30,7 +30,19 @@ classifiers = [
 
 dependencies = [
     "anthropic>=0.116.0",
-    "openai>=1.109.1",
+    # Capped below 3.0 deliberately. openai 3.0.0 (2026-08-12) is the HTTPX2
+    # migration: the SDK's default transport became httpx2 (httpx is no
+    # longer even a dependency), and requests silently stop flowing through
+    # the httpx layer we are coupled to on both sides of the SDK boundary —
+    # verified against 3.0.0: a ``chat()`` under a patched ``httpx.Client.
+    # send`` sails straight past it onto the real network. Concretely broken:
+    # ``http_client=httpx.Client(verify=False)`` (openai/zai/openrouter/
+    # deepseek + the spec registry's TLS-verify escape hatch) no longer
+    # carries the transport, and ``src/providers/_stream_abort.py`` closes
+    # the httpx ``Response`` behind SDK streams to break blocked reads —
+    # internals httpx2 streams do not expose. Lift only with an httpx2
+    # migration of the above (SDK guide: openai-python httpx2.md).
+    "openai>=1.109.1,<3",
     "python-dotenv>=1.2.2",
     "rich>=13.9.4",
     "prompt-toolkit>=3.0.52",

requirements.txt

@@ -1,7 +1,12 @@
 # Runtime dependencies (production install).
 # Dev/test-only tools live in requirements.dev.txt.
 anthropic>=0.116.0
-openai>=1.109.1
+# Capped below 3.0: openai 3.0.0 (2026-08-12) moved the SDK onto httpx2, so
+# requests silently bypass the httpx layer we hook — the ``http_client=
+# httpx.Client(verify=False)`` escape hatch stops carrying the transport and
+# the stream-abort guard's httpx ``Response`` internals vanish. Lift after
+# migrating those to httpx2. See the longer note in pyproject.toml.
+openai>=1.109.1,<3
 python-dotenv>=1.2.2
 rich>=13.9.4
 prompt-toolkit>=3.0.52

src/skills/loader.py

@@ -927,7 +927,17 @@ def activate_conditional_skills_for_paths(
     # the work for each conditional skill. Filter invalid entries here.
     rel_paths: list[str] = []
     for file_path in file_paths:
-        rel_path = os.path.relpath(file_path, cwd)
+        try:
+            rel_path = os.path.relpath(file_path, cwd)
+        except ValueError:
+            # Windows raises ValueError from relpath when file_path and cwd
+            # live on different drives (e.g. an absolute path like
+            # ``C:\\etc\\passwd`` measured against a ``D:`` workspace, or the
+            # POSIX-looking ``/etc/passwd`` that resolves onto the process's
+            # current drive). A path we cannot express relative to cwd is by
+            # definition outside it — skip it, same as the ``..``/absolute
+            # guards below. (POSIX never raises here, so this is Windows-only.)
+            continue
         if not rel_path or rel_path.startswith("..") or os.path.isabs(rel_path):
             continue
         rel_paths.append(rel_path)

tests/tasks/test_kill_shell_for_agent.py

@@ -20,8 +20,19 @@
 
 
 def _spawn(cmd: str) -> subprocess.Popen:
+    # Resolve bash and the process-group kwargs the way production does. A bare
+    # ``["bash", ...]`` resolves via PATH, and on the Windows CI runner PATH's
+    # first ``bash.exe`` is the WSL launcher (``C:\Windows\System32\bash.exe``),
+    # which exits immediately with no distro installed — so a ``sleep 30`` here
+    # would be dead on arrival and the "still running / untouched" assertions
+    # would fail on Windows only. ``bash_argv`` resolves Git Bash explicitly;
+    # ``popen_tree_kwargs`` supplies ``start_new_session`` on POSIX and
+    # ``CREATE_NEW_PROCESS_GROUP`` on Windows (a bare ``start_new_session=True``
+    # is POSIX-only), matching how the real spawner makes a killable tree.
+    from src.utils.shell_platform import bash_argv, popen_tree_kwargs
+
     return subprocess.Popen(
-        ["bash", "-lc", cmd], stdin=subprocess.DEVNULL, start_new_session=True
+        bash_argv(cmd), stdin=subprocess.DEVNULL, **popen_tree_kwargs()
     )
 
 

tests/test_bash_timeout_vs_esc.py

@@ -35,6 +35,7 @@
 """
 from __future__ import annotations
 
+import sys
 import threading
 import time
 from pathlib import Path
@@ -46,6 +47,15 @@
     _run_bash_with_abort,
 )
 from src.utils.abort_controller import AbortController
+from src.utils.shell_platform import bash_argv
+
+# Resolve bash the way production does. A bare ``["bash", ...]`` argv resolves
+# via PATH, and on the Windows CI runner PATH's first ``bash.exe`` is the WSL
+# launcher at ``C:\Windows\System32\bash.exe`` — which exits immediately with
+# no distro installed, so ``sleep``/``echo`` never run and every timing
+# assertion below fails on Windows only. ``bash_argv`` resolves Git Bash
+# explicitly (the same helper ``_run_bash_with_abort``'s production callers
+# use), keeping these supervisor tests on the real shell on every platform.
 
 
 def test_run_bash_with_abort_sets_only_timed_out_on_timeout(tmp_path: Path) -> None:
@@ -56,7 +66,7 @@ def test_run_bash_with_abort_sets_only_timed_out_on_timeout(tmp_path: Path) -> N
     """
     start = time.monotonic()
     result = _run_bash_with_abort(
-        ["bash", "-lc", "sleep 3; echo done"],
+        bash_argv("sleep 3; echo done"),
         cwd=str(tmp_path),
         timeout_s=1,
         abort_signal=None,
@@ -70,11 +80,27 @@ def test_run_bash_with_abort_sets_only_timed_out_on_timeout(tmp_path: Path) -> N
         "timeout). Conflating them makes the model treat timed-out commands "
         "as user-cancelled and retry them on resume."
     )
-    # Sanity: SIGKILL takes effect near-instantly; the timeout deadline
-    # is 1s + at most one ``_ABORT_POLL_INTERVAL_S`` (50ms) jitter.
-    assert elapsed < 3.0, (
-        f"timeout supervisor should have killed the process well under "
-        f"the 3s sleep, took {elapsed:.2f}s"
+    # Sanity: the supervisor must trip the 1s deadline, not wait out the full
+    # 3s sleep. On POSIX the process-group kill closes the pipe at once, so
+    # elapsed ~1s and the pre-port ``< 3.0`` bound also proves the kill beat
+    # the sleep. On Windows, ``taskkill /T`` cannot reach the MSYS2 grandchild
+    # that ``sleep`` becomes — its Windows parent is not ``bash.exe`` — so that
+    # orphan keeps the stdout pipe open and ``communicate()`` blocks up to
+    # ``_KILL_REAP_TIMEOUT_S`` draining it: elapsed lands at ~1s + drain,
+    # indistinguishable by clock from a natural 3s completion. There the
+    # ``timed_out``/``interrupted`` assertions above carry the regression
+    # guard, and this bound only catches a supervisor that hangs past one
+    # kill-drain.
+    if sys.platform == "win32":
+        from src.tool_system.tools.bash.bash_tool import _KILL_REAP_TIMEOUT_S
+
+        max_elapsed = 1.0 + _KILL_REAP_TIMEOUT_S + 1.5
+    else:
+        max_elapsed = 3.0
+    assert elapsed < max_elapsed, (
+        f"timeout supervisor should have tripped the 1s deadline rather than "
+        f"waited out the 3s sleep; took {elapsed:.2f}s (budget "
+        f"{max_elapsed:.1f}s)"
     )
 
 
@@ -96,7 +122,7 @@ def _trip_abort() -> None:
     threading.Thread(target=_trip_abort, daemon=True).start()
 
     result = _run_bash_with_abort(
-        ["bash", "-lc", "sleep 3; echo done"],
+        bash_argv("sleep 3; echo done"),
         cwd=str(tmp_path),
         timeout_s=30,  # well above the abort time
         abort_signal=ctrl.signal,
@@ -236,7 +262,7 @@ def test_run_bash_with_abort_natural_exit_sets_neither_flag(tmp_path: Path) -> N
     where a code change accidentally sets one of them on the happy path.
     """
     result = _run_bash_with_abort(
-        ["bash", "-lc", "echo hello"],
+        bash_argv("echo hello"),
         cwd=str(tmp_path),
         timeout_s=10,
         abort_signal=None,
@@ -251,11 +277,7 @@ def test_run_bash_with_abort_natural_exit_sets_neither_flag(tmp_path: Path) -> N
 def test_detached_descendant_does_not_discard_captured_stdout(tmp_path: Path) -> None:
     """A detached child may keep the output pipe open after Bash exits."""
     result = _run_bash_with_abort(
-        [
-            "bash",
-            "-lc",
-            "nohup sh -c 'sleep 4' >/dev/null 2>&1 & echo server-started",
-        ],
+        bash_argv("nohup sh -c 'sleep 4' >/dev/null 2>&1 & echo server-started"),
         cwd=str(tmp_path),
         timeout_s=10,
         abort_signal=None,

tests/test_ch10_coordination_round4.py

@@ -98,26 +98,47 @@ class TestBashReaperMakesEligible(unittest.TestCase):
 
     def test_reaper_terminal_state_is_eligible(self):
         import subprocess
+        import tempfile
         import time
 
         from src.tasks.eviction import is_eligible_for_eviction
         from src.tool_system.context import ToolContext
         from src.tool_system.tools.bash.background import spawn_background_bash
 
-        ctx = ToolContext(workspace_root=Path("/tmp"))
+        # A real directory on every platform. ``Path("/tmp")`` reaches Popen's
+        # ``cwd=`` as the drive-relative ``\tmp`` on Windows, and the CI runner
+        # has no ``C:\tmp`` — spawn dies with NotADirectoryError (WinError 267)
+        # before the reaper is ever exercised. (Machines where ``C:\tmp``
+        # happens to exist hid this locally.)
+        tmp = Path(tempfile.gettempdir())
+        ctx = ToolContext(workspace_root=tmp)
         # A command that exits immediately.
         result = spawn_background_bash(
-            command="true", cwd=Path("/tmp"), description="t", context=ctx,
+            command="true", cwd=tmp, description="t", context=ctx,
         )
         task_id = result["backgroundTaskId"]
-        # Wait for the reaper daemon to flip the state terminal.
-        deadline = time.time() + 5.0
+        # Wait for the reaper daemon to flip the state terminal AND deliver the
+        # completion notification. The reaper does this in two separate registry
+        # updates: _patch sets the terminal status + evict_after (schedule_
+        # eviction), then enqueue_shell_notification sets notified=True. Breaking
+        # on the terminal status alone races that second update — on a loaded
+        # runner the loop can snapshot the state in the window where status is
+        # terminal but notified is still False, then fail assertTrue(notified)
+        # on that stale snapshot (flaky in a batch run, fine in isolation). Wait
+        # for notified, which the reaper sets last and which implies the
+        # terminal status + evict_after were already set.
+        deadline = time.time() + 10.0
         state = None
         while time.time() < deadline:
             state = ctx.runtime_tasks.get(task_id)
-            if state is not None and state.status in ("completed", "failed"):
+            if (
+                state is not None
+                and state.status in ("completed", "failed")
+                and state.notified
+                and state.evict_after is not None
+            ):
                 break
-            time.sleep(0.05)
+            time.sleep(0.02)
         self.assertIsNotNone(state)
         self.assertIn(state.status, ("completed", "failed"))
         # The reaper set notified + evict_after (WI-2), so it is eligible

tests/test_shell_completion_notification.py

@@ -126,13 +126,30 @@ class TestSpawnReapIntegration:
     """critic #4: a REAL spawn_background_bash → reap → notification, not just
     the unit builder."""
 
-    def test_bg_bash_completion_notifies_the_model(self, tmp_path):
+    def test_bg_bash_completion_notifies_the_model(self, tmp_path, monkeypatch):
         import time
         from pathlib import Path
 
+        import src.utils.task_notification as tn
         from src.tool_system.context import ToolContext, ToolUseOptions
         from src.tool_system.tools.bash.background import spawn_background_bash
 
+        # Observe delivery at the enqueue call, not by polling the global
+        # queue: the queue is process-global and anything else alive in the
+        # process may legitimately drain it (the agent-server worker loop —
+        # exercised by tests/server/*, which run earlier — consumes exactly
+        # this mode). CI run 31579051653 lost the race that way: the reaper
+        # delivered, but 100 peek() polls saw only an already-drained queue.
+        # The spy records AND forwards, so the production path stays intact.
+        delivered = []
+        real_enqueue = tn.enqueue_pending_notification
+
+        def _spy(*, value, mode="task-notification"):
+            delivered.append(value)
+            return real_enqueue(value=value, mode=mode)
+
+        monkeypatch.setattr(tn, "enqueue_pending_notification", _spy)
+
         clear_pending_notifications()
         ctx = ToolContext(workspace_root=tmp_path)
         ctx.options = ToolUseOptions(tools=[])
@@ -141,15 +158,21 @@ def test_bg_bash_completion_notifies_the_model(self, tmp_path):
             description="quick fail", context=ctx,
         )
         task_id = out["backgroundTaskId"]
-        # wait for the reap thread to deliver
-        for _ in range(100):
-            if peek_pending_notifications():
+        # Wait for the reap thread to deliver. Deadline-based and generous:
+        # a loaded 2-core runner can stall a bash spawn well past a flat 5s.
+        deadline = time.monotonic() + 15.0
+        while time.monotonic() < deadline:
+            if delivered:
                 break
-            time.sleep(0.05)
-        q = peek_pending_notifications()
-        assert q, "no completion notification delivered"
-        joined = "\n".join(str(n) for n in q)
+            time.sleep(0.02)
+        assert delivered, "no completion notification delivered"
+        joined = "\n".join(delivered)
         assert 'Background command "quick fail" failed with exit code 3' in joined
+        # The check-and-set committed before the enqueue: the registry state
+        # must show notified=True (and the terminal facts the envelope claims).
+        st = ctx.runtime_tasks.get(task_id)
+        assert st is not None and st.notified is True
+        assert st.status == "failed" and st.exit_code == 3
         clear_pending_notifications()
 
 

tests/test_stream_watchdog.py

@@ -24,6 +24,27 @@
 )
 
 
+def _await_fire(watchdog: StreamWatchdog, response: MagicMock, timeout: float = 5.0) -> None:
+    """Wait (bounded) until the watchdog has fired AND closed the response.
+
+    ``_on_timeout`` sets ``fired`` under the lock but closes the response
+    after releasing it, so there is a real window where ``fired`` is True
+    and ``close()`` hasn't happened yet — that ordering is deliberate (the
+    consumer must be able to classify a timeout the moment it is decided).
+    A fixed ``time.sleep`` then ``close.assert_called()`` races that window
+    plus Windows' ~15.6ms timer granularity and runner scheduling stalls
+    (the observed CI flake). Polling keeps the assertion ("a fire closes
+    the response") while dropping the bet on scheduler punctuality. The
+    caller still asserts afterward, so a genuine no-fire/no-close bug fails
+    loudly at the deadline rather than hanging.
+    """
+    deadline = time.monotonic() + timeout
+    while time.monotonic() < deadline:
+        if watchdog.fired and response.close.called:
+            return
+        time.sleep(0.01)
+
+
 class TestStreamIdleTimeoutResolution(unittest.TestCase):
     """Env-var resolution for ``CLAUDE_STREAM_IDLE_TIMEOUT_MS``."""
 
@@ -202,7 +223,7 @@ def test_survives_first_event_wait_then_fires_on_inter_event_idle(self):
         self.assertFalse(watchdog.fired, "must not fire during first-event grace")
         resp.close.assert_not_called()
         watchdog.reset()  # first event arrived → tighten to inter-event
-        time.sleep(0.25)  # past inter-event now
+        _await_fire(watchdog, resp)  # inter-event idle lapses
         self.assertTrue(watchdog.fired, "must fire on inter-event idle after start")
         resp.close.assert_called()
         watchdog.disarm()
@@ -217,7 +238,7 @@ def test_first_event_timeout_still_fires_a_truly_dead_stream(self):
             stream, timeout_s=0.1, first_event_timeout_s=0.2
         )
         watchdog.arm()
-        time.sleep(0.35)  # never any event; first-event grace lapses
+        _await_fire(watchdog, resp)  # never any event; first-event grace lapses
         self.assertTrue(watchdog.fired)
         resp.close.assert_called()
         watchdog.disarm()
@@ -246,8 +267,12 @@ def test_byte_progress_prevents_fire(self):
             response.num_bytes_downloaded += 128
         self.assertFalse(watchdog.fired, "byte progress must re-arm, not fire")
         response.close.assert_not_called()
-        # Once bytes stop, the next deadline fires.
-        time.sleep(0.2)
+        # Once bytes stop, the next deadline fires. Polled, not slept: the
+        # last byte bump re-arms one more ~0.08s deadline, and a fixed 0.2s
+        # sleep left <0.1s margin for the timer thread on a loaded runner —
+        # plus the fired-vs-close window (see _await_fire) — the exact
+        # windows-latest CI failure ("Expected 'close' to have been called").
+        _await_fire(watchdog, response)
         self.assertTrue(watchdog.fired)
         response.close.assert_called()
         watchdog.disarm()
@@ -263,7 +288,7 @@ def test_no_byte_progress_still_fires(self):
             stream, timeout_s=0.08, first_event_timeout_s=0.08
         )
         watchdog.arm()
-        time.sleep(0.25)
+        _await_fire(watchdog, response)
         self.assertTrue(watchdog.fired)
         response.close.assert_called()
         watchdog.disarm()