From 7b7b35a3ffb6ff764d33e45f854c85abfa7a0488 Mon Sep 17 00:00:00 2001 From: Michael Jerris Date: Mon, 27 Jul 2026 01:30:34 -0400 Subject: [PATCH 1/5] fix(ci): multi-OS must use the interpreter setup-python provisioned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The multi-OS PACKAGE-SMOKE step invoked bare `python3`, which on the macOS runner does NOT resolve to the interpreter actions/setup-python provisioned. Nightly run 30238061313, job macos-latest: [FAIL] build: exit 1 /Library/Frameworks/Python.framework/Versions/3.12/bin/python3: No module named build Mechanism. `setup-python@v6` with python-version "3.12" provisions /Users/runner/hostedtoolcache/Python/3.12.10/arm64 (the step's own log confirms pythonLocation), and the earlier `pip install` step installed `build` THERE. But the macOS image ships a pre-installed framework Python whose bin dir sits ahead of the toolcache for the name `python3`, so bare `python3` selected /Library/Frameworks/Python.framework/Versions/3.12/bin/python3 — a DIFFERENT interpreter, without `build`. The same run is the proof: the TEST step immediately above uses `python -m pytest` (the setup-python shim) and passed, as did every `pip install`. Only the one step spelled `python3` missed. package_smoke.py is NOT at fault — it correctly uses sys.executable, so it faithfully used whichever interpreter this line handed it, and it reported that path verbatim in the error. Fixed by invoking `python` (setup-python's shim). NOT fixed by `pip install build`: installing into the wrong interpreter would mask the resolution bug rather than repair it, and would leave the step running an interpreter nobody selected. Sweep of the sibling workflows that call setup-python: doc-audit.yml had the only other bare-`python3` `run:` lines (2). They are ubuntu-only, where setup-python front-loads its dir so both names currently resolve to the toolcache — latent, not live — but switched to the shim for consistency so the trap cannot activate if that job ever gains a macOS runner. nightly.yml and live-smoke.yml call setup-python but contain no bare `python3`, and are ubuntu-only. Verified: `grep -n '\bpython3\b' .github/workflows/*.yml` now matches only explanatory comments, no executable line. actionlint on both changed files reports the same 6 pre-existing shellcheck info/style findings as before the change (all at doc-audit.yml's untouched Summary step) — no new findings. Known-latent, deliberately NOT changed here (needs an owner call): scripts/run-ci.sh uses bare `python3` in 51 places. Both workflows that invoke it are ubuntu-only, and that script also runs on developer machines where `python3` is the correct name and `python` may not exist — so a blanket rename is a behavior change beyond this fix. --- .github/workflows/doc-audit.yml | 9 +++++++-- .github/workflows/multi-os.yml | 11 ++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.github/workflows/doc-audit.yml b/.github/workflows/doc-audit.yml index afa1573..c5d03aa 100644 --- a/.github/workflows/doc-audit.yml +++ b/.github/workflows/doc-audit.yml @@ -59,8 +59,13 @@ jobs: # deleted livewire member stops resolving instead of staying excused by a stale # committed file. Mirrors run-ci.sh's SURFACE-NATIVE gate (DOC-AUDIT deps on it). - name: Regenerate the native-only doc-audit sidecar + # `python` (the setup-python shim), not bare `python3` — see the PACKAGE-SMOKE + # note in multi-os.yml. This job is ubuntu-only, where setup-python front-loads + # its dir so both names currently resolve to the toolcache; using the shim keeps + # that true if this workflow ever gains a macOS runner, where bare `python3` + # resolves to the framework Python instead. run: | - python3 signalwire-python/scripts/emit_surface_native.py \ + python signalwire-python/scripts/emit_surface_native.py \ --out signalwire-python/port_surface_native.json - name: Run audit_docs.py against the Python surface @@ -68,7 +73,7 @@ jobs: # surface oracle by design, but livewire/ docs are in this perimeter, so its # real members only resolve via the sidecar. See scripts/emit_surface_native.py. run: | - python3 porting-sdk/scripts/audit_docs.py \ + python porting-sdk/scripts/audit_docs.py \ --root signalwire-python \ --surface porting-sdk/python_surface.json \ --ignore signalwire-python/DOC_AUDIT_IGNORE.md \ diff --git a/.github/workflows/multi-os.yml b/.github/workflows/multi-os.yml index 15d0469..446c547 100644 --- a/.github/workflows/multi-os.yml +++ b/.github/workflows/multi-os.yml @@ -65,4 +65,13 @@ jobs: - name: PACKAGE-SMOKE (build + install + import from the built artifact) shell: bash working-directory: signalwire-python - run: python3 ../porting-sdk/scripts/package_smoke.py --port python --repo . + # `python`, NOT `python3` — `python` is the shim actions/setup-python puts on + # PATH, so it is the 3.12 interpreter provisioned above (and the one the + # `pip install` step installed `build` into). Bare `python3` on the macOS + # runner resolves to /Library/Frameworks/Python.framework/Versions/3.12/bin/ + # python3 — the PRE-INSTALLED framework Python, ahead of the toolcache — a + # different interpreter with no `build` module. That is the nightly failure + # (run 30238061313, macos-latest): "No module named build". package_smoke.py + # is not at fault; it correctly uses sys.executable, so it faithfully used + # whichever interpreter this line handed it. + run: python ../porting-sdk/scripts/package_smoke.py --port python --repo . From d8f9966b09706f5fc10fac446b9d2bbdca865f5d Mon Sep 17 00:00:00 2001 From: Michael Jerris Date: Mon, 27 Jul 2026 06:28:40 -0400 Subject: [PATCH 2/5] =?UTF-8?q?fix(ci):=20declare=20`build`=20=E2=80=94=20?= =?UTF-8?q?PACKAGE-SMOKE=20never=20had=20the=20module=20it=20needs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Corrects my own previous commit on this branch, which mis-attributed the failure to interpreter resolution. Dispatching the workflow disproved that fix: with `python` instead of `python3` the macOS job failed IDENTICALLY, one word different — /Library/Frameworks/Python.framework/Versions/3.12/bin/python: No module named build i.e. the SAME framework interpreter, reached via the other name. So on the macOS/arm64 runner BOTH names resolve to the pre-installed framework Python; setup-python does not win PATH there at all. The real root cause, from the same log: `pip` is that framework Python's pip — every dependency reports installing to `/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages`. The job is therefore CONSISTENTLY one interpreter, and there was never a mismatch to fix. `build` is simply not installed, because **it is not a declared dependency anywhere** — not in requirements-dev.txt, not in requirements.txt, not in pyproject.toml. The gate relied on the runner image happening to ship it. AGENT_RULES §7: a tool a gate needs is DECLARED, not assumed present. So: requirements-dev.txt gains `build>=1.0.0`, and the step above already installs that file. This is not "pip install build into the wrong interpreter" (which the brief rightly forbids as masking a resolution bug) — there is no wrong interpreter here, and declaring a real, undeclared dev dependency is the fix rather than the mask. Note this gate has never passed for python on any OS: multi-os.yml is the ONLY place PACKAGE-SMOKE runs for this port (nightly.yml and scripts/run-ci.sh do not invoke it), so the "ubuntu ships build" path was never exercised either. `python` (not `python3`) is KEPT, on its own merits: it is the same name `pip` above pairs with, so the step provably runs the interpreter the deps went into, and it stays correct if the image's PATH precedence ever changes. The workflow comment is rewritten to state this true mechanism instead of the interpreter-mismatch story. The doc-audit.yml python3→python change from the previous commit also stands — that job is ubuntu-only and green either way; the shim is the consistent choice. Windows remains red on this workflow for an unrelated, separately-assigned reason (RED 3: four test-portability defects in the TEST step — WinError 32 on an unclosed sqlite temp file, a hardcoded "/tmp/custom" assertion, a POSIX 0o755 mode check, and cp1252 UnicodeEncodeErrors). That step fails before PACKAGE-SMOKE runs, so the Windows job cannot confirm this fix either way. --- .github/workflows/multi-os.yml | 21 ++++++++++++--------- requirements-dev.txt | 6 ++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/.github/workflows/multi-os.yml b/.github/workflows/multi-os.yml index 446c547..b59b630 100644 --- a/.github/workflows/multi-os.yml +++ b/.github/workflows/multi-os.yml @@ -65,13 +65,16 @@ jobs: - name: PACKAGE-SMOKE (build + install + import from the built artifact) shell: bash working-directory: signalwire-python - # `python`, NOT `python3` — `python` is the shim actions/setup-python puts on - # PATH, so it is the 3.12 interpreter provisioned above (and the one the - # `pip install` step installed `build` into). Bare `python3` on the macOS - # runner resolves to /Library/Frameworks/Python.framework/Versions/3.12/bin/ - # python3 — the PRE-INSTALLED framework Python, ahead of the toolcache — a - # different interpreter with no `build` module. That is the nightly failure - # (run 30238061313, macos-latest): "No module named build". package_smoke.py - # is not at fault; it correctly uses sys.executable, so it faithfully used - # whichever interpreter this line handed it. + # `python`, not `python3`: use the SAME name `pip` above pairs with, so this + # step provably runs the interpreter the deps were installed into. (On the + # macOS/arm64 runner BOTH names resolve to the pre-installed framework Python + # — setup-python does not win PATH there — and `pip` is that Python's pip, so + # the whole job is consistently one interpreter. Pinning the name keeps it that + # way if the image's precedence ever changes.) + # + # The nightly failure (run 30238061313, macos-latest) was + # "No module named build" — NOT an interpreter mismatch: `build` was never a + # declared dependency at all, so the gate silently relied on the runner image + # shipping it. Now declared in requirements-dev.txt, which the step above + # installs. package_smoke.py is not at fault; it correctly uses sys.executable. run: python ../porting-sdk/scripts/package_smoke.py --port python --repo . diff --git a/requirements-dev.txt b/requirements-dev.txt index 8ff6e38..f137087 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -17,6 +17,12 @@ factory-boy>=3.3.0 # For test data factories faker>=19.0.0 # For generating fake data httpx>=0.24.0 # For async HTTP testing aiofiles>=23.0.0 # For async file operations in tests +build>=1.0.0 # PACKAGE-SMOKE gate runs `python -m build --wheel` (see + # porting-sdk/scripts/package_smoke.py plan_python). It was + # never declared, so the gate depended on the runner image + # happening to ship it — and failed on macOS/Windows, where it + # does not (AGENT_RULES §7: a tool a gate needs is DECLARED, + # not assumed present). # Optional-feature deps required by tests under tests/unit/search/. # These mirror the [search-queryonly] extra in pyproject.toml so the From 95e542b980ed75df5dd3291ad938155fffcddc09 Mon Sep 17 00:00:00 2001 From: Michael Jerris Date: Mon, 27 Jul 2026 06:59:25 -0400 Subject: [PATCH 3/5] =?UTF-8?q?fix(tests):=20Windows=20portability=20?= =?UTF-8?q?=E2=80=94=20sqlite=20handles,=20path=20assertions,=20permission?= =?UTF-8?q?=20bits,=20UTF-8=20encoding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four distinct cross-platform defects, all measured from nightly Multi-OS run 30238061313 (job windows-latest, step TEST: 36 failed / 2 errors / 5671 passed). Two turned out to be product bugs, not test bugs. 1. sqlite handle outlives the temp file (PermissionError [WinError 32]) PRODUCT BUG. search/index_builder.py validate_index() closed its connection only on the success path; the "Missing tables" early return and the except both leaked it. Windows refuses to delete a file with a live handle, so the fixture teardown's os.remove() raised. Fixed with contextlib.closing (NOT `with sqlite3.connect(...)` — a Connection context manager commits but does not close). Audited all 14 sqlite3.connect sites in search/ and found two more unguarded on their error paths: migration.get_index_info() and search_service._get_model_name(). Also fixed the test-side leak in test_search_engine.py, where NamedTemporaryFile(delete=False) held its own handle open while os.unlink ran inside the `with` block. 2. Hardcoded POSIX path assertion str(Path) renders with the platform separator, so `== "/tmp/custom"` can never hold on Windows (it yields "\tmp\custom"). Now compares Path objects. Also removes the /tmp usage itself per the project rule (tmp_path instead). Same fix in test_init_project.py::test_main_custom_dir, which failed the same way via `'/tmp/custom' in 'D:\tmp\custom\testproject'`. 3. POSIX permission bits Windows has no execute bit, so st_mode never carries 0o755. The two observable-mode assertions are skipif(win32) with the reason recorded, and the contract they were checking is now ALSO asserted platform-independently (that chmod 0o755 is requested, and only for deploy.sh). POSIX coverage is unchanged — nothing was deleted or weakened. 4. UnicodeEncodeError: 'charmap' PRODUCT BUG. cli/dokku.py:1964 wrote generated files with write_text() and no encoding, i.e. the platform default (cp1252 on Windows). The templates embed box-drawing rules (U+2500/U+2550, 59-char runs — matching the log's "position 130-188") and arrows, which cp1252 cannot represent. This caused 8 direct failures plus 4 more surfacing as generate() returning False ("Failed to generate project: 'charmap' codec can't encode..."). Fixed at _write_file plus the two app.json reads. cli/init_project.py had the same latent defect — 98 cp1252-hostile characters across 29 unguarded write_text sites — fixed there too before it bites. Proven on POSIX (not merely "looks right"): - Defect 1: 3 new tests assert the platform-independent invariant (every connection opened is closed, via a connect spy). Verified they FAIL against the unfixed product code and pass with it. - Defect 4: reproduced the Windows failure class on macOS with `LC_ALL=C python -X utf8=0`, which yields "'ascii' codec can't encode characters in position 2-80" — the same position range as CI's charmap error. The 3 new UTF-8 round-trip tests fail without the fix; with it, all 210 cli tests pass even under that hostile locale. - Defects 2 and 3 are structural (Path comparison / platform skip) and remain pending real confirmation on the Windows runner. Full local gate run: `bash scripts/run-ci.sh` → CI PASS, exit 0 (37 gates). Unit suite 5621 passed / 100 skipped on macOS. Note: the Windows TEST step failing is what kept PACKAGE-SMOKE from running at all on that job (TEST: failure -> PACKAGE-SMOKE: skipped). If TEST now passes, PACKAGE-SMOKE will execute on Windows for the first time and may surface unrelated failures. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GFKJhLvfV8yGrASwqxdgaf --- signalwire/signalwire/cli/dokku.py | 10 +- signalwire/signalwire/cli/init_project.py | 82 +++++--- signalwire/signalwire/search/index_builder.py | 72 ++++--- signalwire/signalwire/search/migration.py | 25 +-- .../signalwire/search/search_service.py | 14 +- tests/unit/cli/test_dokku.py | 192 +++++++++++++++--- tests/unit/cli/test_init_project.py | 26 ++- tests/unit/search/test_index_builder.py | 126 +++++++++++- tests/unit/search/test_search_engine.py | 76 ++++--- 9 files changed, 458 insertions(+), 165 deletions(-) diff --git a/signalwire/signalwire/cli/dokku.py b/signalwire/signalwire/cli/dokku.py index f181aad..294d7b8 100644 --- a/signalwire/signalwire/cli/dokku.py +++ b/signalwire/signalwire/cli/dokku.py @@ -1961,7 +1961,11 @@ def _write_file(self, path: str, content: str, executable: bool = False) -> None """Write a file to the project directory.""" file_path = self.project_dir / path file_path.parent.mkdir(parents=True, exist_ok=True) - file_path.write_text(content) + # Always UTF-8, never the platform default. Several templates embed + # box-drawing characters (U+2500/U+2550) and arrows, which the Windows + # default codec (cp1252) cannot encode -- writing without an explicit + # encoding raises UnicodeEncodeError there. + file_path.write_text(content, encoding="utf-8") if executable: file_path.chmod(0o755) @@ -2101,7 +2105,7 @@ def cmd_deploy(args: argparse.Namespace) -> int: try: with open( # noqa: PTH123 # tests patch builtins.open while mocking Path; Path.open() would bypass the mock seam - "app.json" + "app.json", encoding="utf-8" ) as f: app_json = json.load(f) app_name = app_json.get("name") @@ -2232,7 +2236,7 @@ def _get_app_name() -> str: try: with open( # noqa: PTH123 # tests patch builtins.open while mocking Path; Path.open() would bypass the mock seam - "app.json" + "app.json", encoding="utf-8" ) as f: # json.load() is typed -> Any; the "name" field is a string # (default "" when absent). Coerce to satisfy the str return. diff --git a/signalwire/signalwire/cli/init_project.py b/signalwire/signalwire/cli/init_project.py index e75d2f6..05af090 100644 --- a/signalwire/signalwire/cli/init_project.py +++ b/signalwire/signalwire/cli/init_project.py @@ -1915,17 +1915,19 @@ def _generate_aws(self) -> bool: # handler.py handler_code = AWS_HANDLER_TEMPLATE.format(**template_vars) - (self.project_dir / "handler.py").write_text(handler_code) + (self.project_dir / "handler.py").write_text(handler_code, encoding="utf-8") print_success("Created handler.py") # requirements.txt - (self.project_dir / "requirements.txt").write_text(AWS_REQUIREMENTS_TEMPLATE) + (self.project_dir / "requirements.txt").write_text( + AWS_REQUIREMENTS_TEMPLATE, encoding="utf-8" + ) print_success("Created requirements.txt") # deploy.sh deploy_code = AWS_DEPLOY_TEMPLATE.format(**template_vars) deploy_path = self.project_dir / "deploy.sh" - deploy_path.write_text(deploy_code) + deploy_path.write_text(deploy_code, encoding="utf-8") deploy_path.chmod(0o755) print_success("Created deploy.sh") @@ -1933,7 +1935,9 @@ def _generate_aws(self) -> bool: self._create_cloud_env_example("aws") # .gitignore - (self.project_dir / ".gitignore").write_text(TEMPLATE_GITIGNORE) + (self.project_dir / ".gitignore").write_text( + TEMPLATE_GITIGNORE, encoding="utf-8" + ) print_success("Created .gitignore") # README.md @@ -1951,17 +1955,19 @@ def _generate_gcp(self) -> bool: # main.py main_code = GCP_MAIN_TEMPLATE.format(**template_vars) - (self.project_dir / "main.py").write_text(main_code) + (self.project_dir / "main.py").write_text(main_code, encoding="utf-8") print_success("Created main.py") # requirements.txt - (self.project_dir / "requirements.txt").write_text(GCP_REQUIREMENTS_TEMPLATE) + (self.project_dir / "requirements.txt").write_text( + GCP_REQUIREMENTS_TEMPLATE, encoding="utf-8" + ) print_success("Created requirements.txt") # deploy.sh deploy_code = GCP_DEPLOY_TEMPLATE.format(**template_vars) deploy_path = self.project_dir / "deploy.sh" - deploy_path.write_text(deploy_code) + deploy_path.write_text(deploy_code, encoding="utf-8") deploy_path.chmod(0o755) print_success("Created deploy.sh") @@ -1969,7 +1975,9 @@ def _generate_gcp(self) -> bool: self._create_cloud_env_example("gcp") # .gitignore - (self.project_dir / ".gitignore").write_text(TEMPLATE_GITIGNORE) + (self.project_dir / ".gitignore").write_text( + TEMPLATE_GITIGNORE, encoding="utf-8" + ) print_success("Created .gitignore") # README.md @@ -1991,31 +1999,37 @@ def _generate_azure(self) -> bool: # function_app/__init__.py init_code = AZURE_INIT_TEMPLATE.format(**template_vars) - (function_dir / "__init__.py").write_text(init_code) + (function_dir / "__init__.py").write_text(init_code, encoding="utf-8") print_success("Created function_app/__init__.py") # function_app/function.json - (function_dir / "function.json").write_text(AZURE_FUNCTION_JSON_TEMPLATE) + (function_dir / "function.json").write_text( + AZURE_FUNCTION_JSON_TEMPLATE, encoding="utf-8" + ) print_success("Created function_app/function.json") # host.json - (self.project_dir / "host.json").write_text(AZURE_HOST_JSON_TEMPLATE) + (self.project_dir / "host.json").write_text( + AZURE_HOST_JSON_TEMPLATE, encoding="utf-8" + ) print_success("Created host.json") # local.settings.json (self.project_dir / "local.settings.json").write_text( - AZURE_LOCAL_SETTINGS_TEMPLATE + AZURE_LOCAL_SETTINGS_TEMPLATE, encoding="utf-8" ) print_success("Created local.settings.json") # requirements.txt - (self.project_dir / "requirements.txt").write_text(AZURE_REQUIREMENTS_TEMPLATE) + (self.project_dir / "requirements.txt").write_text( + AZURE_REQUIREMENTS_TEMPLATE, encoding="utf-8" + ) print_success("Created requirements.txt") # deploy.sh deploy_code = AZURE_DEPLOY_TEMPLATE.format(**template_vars) deploy_path = self.project_dir / "deploy.sh" - deploy_path.write_text(deploy_code) + deploy_path.write_text(deploy_code, encoding="utf-8") deploy_path.chmod(0o755) print_success("Created deploy.sh") @@ -2023,7 +2037,9 @@ def _generate_azure(self) -> bool: self._create_cloud_env_example("azure") # .gitignore - (self.project_dir / ".gitignore").write_text(TEMPLATE_GITIGNORE) + (self.project_dir / ".gitignore").write_text( + TEMPLATE_GITIGNORE, encoding="utf-8" + ) print_success("Created .gitignore") # README.md @@ -2073,7 +2089,7 @@ def _create_cloud_env_example(self, platform: str) -> None: SWML_BASIC_AUTH_USER=admin SWML_BASIC_AUTH_PASSWORD=your-secure-password """ - (self.project_dir / ".env.example").write_text(env_content) + (self.project_dir / ".env.example").write_text(env_content, encoding="utf-8") print_success("Created .env.example") def _create_cloud_readme(self, platform: str) -> None: @@ -2269,7 +2285,7 @@ def _create_cloud_readme(self, platform: str) -> None: Set your phone number's SWML URL to the endpoint URL shown after deployment. """ - (self.project_dir / "README.md").write_text(readme) + (self.project_dir / "README.md").write_text(readme, encoding="utf-8") print_success("Created README.md") def _create_directories(self) -> None: @@ -2291,24 +2307,26 @@ def _create_agent_files(self) -> None: agents_dir = self.project_dir / "agents" # __init__.py - (agents_dir / "__init__.py").write_text(TEMPLATE_AGENTS_INIT) + (agents_dir / "__init__.py").write_text(TEMPLATE_AGENTS_INIT, encoding="utf-8") print_success("Created agents/__init__.py") # main_agent.py agent_code = get_agent_template( self.config.get("agent_type", "basic"), self.features ) - (agents_dir / "main_agent.py").write_text(agent_code) + (agents_dir / "main_agent.py").write_text(agent_code, encoding="utf-8") print_success("Created agents/main_agent.py") # skills/__init__.py - (self.project_dir / "skills" / "__init__.py").write_text(TEMPLATE_SKILLS_INIT) + (self.project_dir / "skills" / "__init__.py").write_text( + TEMPLATE_SKILLS_INIT, encoding="utf-8" + ) print_success("Created skills/__init__.py") def _create_app_file(self) -> None: """Create main app.py entry point.""" app_code = get_app_template(self.features) - (self.project_dir / "app.py").write_text(app_code) + (self.project_dir / "app.py").write_text(app_code, encoding="utf-8") print_success("Created app.py") def _create_config_files(self) -> None: @@ -2342,43 +2360,49 @@ def _create_config_files(self) -> None: DEBUG_WEBHOOK_LEVEL=1 """ - (self.project_dir / ".env").write_text(env_content) + (self.project_dir / ".env").write_text(env_content, encoding="utf-8") print_success("Created .env") # .env.example - (self.project_dir / ".env.example").write_text(TEMPLATE_ENV_EXAMPLE) + (self.project_dir / ".env.example").write_text( + TEMPLATE_ENV_EXAMPLE, encoding="utf-8" + ) print_success("Created .env.example") # .gitignore - (self.project_dir / ".gitignore").write_text(TEMPLATE_GITIGNORE) + (self.project_dir / ".gitignore").write_text( + TEMPLATE_GITIGNORE, encoding="utf-8" + ) print_success("Created .gitignore") # requirements.txt - (self.project_dir / "requirements.txt").write_text(TEMPLATE_REQUIREMENTS) + (self.project_dir / "requirements.txt").write_text( + TEMPLATE_REQUIREMENTS, encoding="utf-8" + ) print_success("Created requirements.txt") def _create_test_files(self) -> None: """Create test files.""" tests_dir = self.project_dir / "tests" - (tests_dir / "__init__.py").write_text(TEMPLATE_TESTS_INIT) + (tests_dir / "__init__.py").write_text(TEMPLATE_TESTS_INIT, encoding="utf-8") print_success("Created tests/__init__.py") test_code = get_test_template(self.features.get("example_tool", True)) - (tests_dir / "test_agent.py").write_text(test_code) + (tests_dir / "test_agent.py").write_text(test_code, encoding="utf-8") print_success("Created tests/test_agent.py") def _create_web_files(self) -> None: """Create web UI files.""" web_dir = self.project_dir / "web" - (web_dir / "index.html").write_text(get_web_index_template()) + (web_dir / "index.html").write_text(get_web_index_template(), encoding="utf-8") print_success("Created web/index.html") def _create_readme(self) -> None: """Create README.md.""" readme = get_readme_template(self.project_name, self.features) - (self.project_dir / "README.md").write_text(readme) + (self.project_dir / "README.md").write_text(readme, encoding="utf-8") print_success("Created README.md") def _create_virtualenv(self) -> None: diff --git a/signalwire/signalwire/search/index_builder.py b/signalwire/signalwire/search/index_builder.py index 71ab1b5..4bcf1ea 100644 --- a/signalwire/signalwire/search/index_builder.py +++ b/signalwire/signalwire/search/index_builder.py @@ -10,6 +10,7 @@ import sqlite3 import json import hashlib +from contextlib import closing from datetime import datetime from pathlib import Path from typing import Any, TYPE_CHECKING @@ -779,39 +780,44 @@ def validate_index(self, index_file: str) -> dict[str, Any]: return {"valid": False, "error": "Index file does not exist"} try: - conn = sqlite3.connect(index_file) - cursor = conn.cursor() - - # Check schema - cursor.execute("SELECT name FROM sqlite_master WHERE type='table'") - tables = [row[0] for row in cursor.fetchall()] - - required_tables = ["chunks", "chunks_fts", "synonyms", "config"] - missing_tables = [t for t in required_tables if t not in tables] - - if missing_tables: - return {"valid": False, "error": f"Missing tables: {missing_tables}"} - - # Get config - cursor.execute("SELECT key, value FROM config") - config = dict(cursor.fetchall()) - - # Get chunk count - cursor.execute("SELECT COUNT(*) FROM chunks") - chunk_count = cursor.fetchone()[0] - - # Get file count - cursor.execute("SELECT COUNT(DISTINCT filename) FROM chunks") - file_count = cursor.fetchone()[0] - - conn.close() - - return { - "valid": True, - "chunk_count": chunk_count, - "file_count": file_count, - "config": config, - } + # `closing` (not `with sqlite3.connect(...)`) — a Connection used as a + # context manager commits/rolls back the transaction but does NOT close + # the handle. On Windows an unclosed handle makes the file undeletable + # (PermissionError/WinError 32), so every exit path must close. + with closing(sqlite3.connect(index_file)) as conn: + cursor = conn.cursor() + + # Check schema + cursor.execute("SELECT name FROM sqlite_master WHERE type='table'") + tables = [row[0] for row in cursor.fetchall()] + + required_tables = ["chunks", "chunks_fts", "synonyms", "config"] + missing_tables = [t for t in required_tables if t not in tables] + + if missing_tables: + return { + "valid": False, + "error": f"Missing tables: {missing_tables}", + } + + # Get config + cursor.execute("SELECT key, value FROM config") + config = dict(cursor.fetchall()) + + # Get chunk count + cursor.execute("SELECT COUNT(*) FROM chunks") + chunk_count = cursor.fetchone()[0] + + # Get file count + cursor.execute("SELECT COUNT(DISTINCT filename) FROM chunks") + file_count = cursor.fetchone()[0] + + return { + "valid": True, + "chunk_count": chunk_count, + "file_count": file_count, + "config": config, + } except Exception as e: return {"valid": False, "error": str(e)} diff --git a/signalwire/signalwire/search/migration.py b/signalwire/signalwire/search/migration.py index e12e36d..091844e 100644 --- a/signalwire/signalwire/search/migration.py +++ b/signalwire/signalwire/search/migration.py @@ -9,6 +9,7 @@ import sqlite3 import json +from contextlib import closing from typing import Any, TYPE_CHECKING from signalwire.core.logging_config import get_logger @@ -449,21 +450,21 @@ def get_index_info(self, index_path: str) -> dict[str, Any]: info["type"] = "sqlite" info["path"] = index_path - conn = sqlite3.connect(index_path) - cursor = conn.cursor() - - # Get config - cursor.execute("SELECT key, value FROM config") - info["config"] = dict(cursor.fetchall()) + # `closing` so the handle is released even when a query raises — + # an unclosed handle makes the file undeletable on Windows. + with closing(sqlite3.connect(index_path)) as conn: + cursor = conn.cursor() - # Get stats - cursor.execute("SELECT COUNT(*) FROM chunks") - info["total_chunks"] = cursor.fetchone()[0] + # Get config + cursor.execute("SELECT key, value FROM config") + info["config"] = dict(cursor.fetchall()) - cursor.execute("SELECT COUNT(DISTINCT filename) FROM chunks") - info["total_files"] = cursor.fetchone()[0] + # Get stats + cursor.execute("SELECT COUNT(*) FROM chunks") + info["total_chunks"] = cursor.fetchone()[0] - conn.close() + cursor.execute("SELECT COUNT(DISTINCT filename) FROM chunks") + info["total_files"] = cursor.fetchone()[0] else: info["type"] = "unknown" diff --git a/signalwire/signalwire/search/search_service.py b/signalwire/signalwire/search/search_service.py index d719371..54a91b9 100644 --- a/signalwire/signalwire/search/search_service.py +++ b/signalwire/signalwire/search/search_service.py @@ -436,12 +436,14 @@ def _get_model_name(self, index_path: str) -> str: # SQLite backend try: import sqlite3 - - conn = sqlite3.connect(index_path) - cursor = conn.cursor() - cursor.execute("SELECT value FROM config WHERE key = 'embedding_model'") - result = cursor.fetchone() - conn.close() + from contextlib import closing + + # `closing` so the handle is released even when the query raises — + # an unclosed handle makes the file undeletable on Windows. + with closing(sqlite3.connect(index_path)) as conn: + cursor = conn.cursor() + cursor.execute("SELECT value FROM config WHERE key = 'embedding_model'") + result = cursor.fetchone() return result[0] if result else "sentence-transformers/all-mpnet-base-v2" except Exception as e: logger.warning(f"Could not get model name from index: {e}") diff --git a/tests/unit/cli/test_dokku.py b/tests/unit/cli/test_dokku.py index 4b5c641..43bf42c 100644 --- a/tests/unit/cli/test_dokku.py +++ b/tests/unit/cli/test_dokku.py @@ -274,9 +274,14 @@ def test_default_project_dir(self) -> None: gen = DokkuProjectGenerator("myapp", {}) assert gen.project_dir == Path("./myapp") - def test_custom_project_dir(self) -> None: - gen = DokkuProjectGenerator("myapp", {'project_dir': '/tmp/custom'}) - assert str(gen.project_dir) == "/tmp/custom" + def test_custom_project_dir(self, tmp_path: Path) -> None: + # Compare Path objects, not str() against a POSIX literal: `str(Path)` + # renders with the platform separator, so a hardcoded "/tmp/custom" can + # never match on Windows (it yields "\tmp\custom"). Using `tmp_path` + # also keeps the test off a hardcoded /tmp. + custom = tmp_path / "custom" + gen = DokkuProjectGenerator("myapp", {'project_dir': str(custom)}) + assert gen.project_dir == custom class TestDokkuProjectGeneratorGenerate: @@ -327,19 +332,47 @@ def test_write_file_creates_file(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("testapp", {'project_dir': str(tmp_path)}) gen._write_file('hello.txt', 'Hello World') assert (tmp_path / 'hello.txt').exists() - assert (tmp_path / 'hello.txt').read_text() == 'Hello World' + assert (tmp_path / 'hello.txt').read_text(encoding="utf-8") == 'Hello World' def test_write_file_creates_nested_dirs(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("testapp", {'project_dir': str(tmp_path)}) gen._write_file('a/b/c.txt', 'nested') assert (tmp_path / 'a' / 'b' / 'c.txt').exists() + @pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX permission bits: Windows has no execute bit, so st_mode never " + "carries 0o755 (it reports 0o666/0o444 from the read-only attribute). " + "The Windows-side behaviour is covered by " + "test_write_file_executable_requests_chmod below.", + ) def test_write_file_executable(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("testapp", {'project_dir': str(tmp_path)}) gen._write_file('script.sh', '#!/bin/bash', executable=True) mode = (tmp_path / 'script.sh').stat().st_mode assert mode & 0o755 == 0o755 + def test_write_file_executable_requests_chmod(self, tmp_path: Path) -> None: + """`executable=True` must chmod 0o755 -- assertable on every platform. + + Windows drops the POSIX bits, so the observable-mode assertion above cannot + run there. Asserting the *request* keeps the contract covered on Windows and + catches a regression that silently stopped chmod-ing. + """ + gen = DokkuProjectGenerator("testapp", {'project_dir': str(tmp_path)}) + with patch.object(Path, 'chmod', autospec=True) as mock_chmod: + gen._write_file('script.sh', '#!/bin/bash', executable=True) + assert (tmp_path / 'script.sh').exists() + mock_chmod.assert_called_once() + assert mock_chmod.call_args[0][1] == 0o755 + + def test_write_file_not_executable_does_not_chmod(self, tmp_path: Path) -> None: + """The default path must not chmod at all (guards the flag's meaning).""" + gen = DokkuProjectGenerator("testapp", {'project_dir': str(tmp_path)}) + with patch.object(Path, 'chmod', autospec=True) as mock_chmod: + gen._write_file('plain.txt', 'data') + mock_chmod.assert_not_called() + class TestDokkuProjectGeneratorCoreFIles: """Tests that _write_core_files creates all expected files.""" @@ -356,7 +389,7 @@ def test_core_files_without_web(self, tmp_path: Path) -> None: assert (tmp_path / 'app.json').exists() assert (tmp_path / 'app.py').exists() # Standard template used (not web) - content = (tmp_path / 'app.py').read_text() + content = (tmp_path / 'app.py').read_text(encoding="utf-8") assert 'AgentBase' in content assert 'AgentServer' not in content @@ -366,40 +399,40 @@ def test_core_files_with_web(self, tmp_path: Path) -> None: 'web': True }) gen._write_core_files() - content = (tmp_path / 'app.py').read_text() + content = (tmp_path / 'app.py').read_text(encoding="utf-8") assert 'AgentServer' in content assert (tmp_path / 'web' / 'index.html').exists() def test_procfile_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_core_files() - content = (tmp_path / 'Procfile').read_text() + content = (tmp_path / 'Procfile').read_text(encoding="utf-8") assert 'gunicorn' in content assert 'uvicorn' in content def test_runtime_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_core_files() - content = (tmp_path / 'runtime.txt').read_text() + content = (tmp_path / 'runtime.txt').read_text(encoding="utf-8") assert 'python-3.11' in content def test_env_example_contains_app_name(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("my-cool-app", {'project_dir': str(tmp_path)}) gen._write_core_files() - content = (tmp_path / '.env.example').read_text() + content = (tmp_path / '.env.example').read_text(encoding="utf-8") assert 'my-cool-app' in content def test_app_json_contains_app_name(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("testbot", {'project_dir': str(tmp_path)}) gen._write_core_files() - content = (tmp_path / 'app.json').read_text() + content = (tmp_path / 'app.json').read_text(encoding="utf-8") data = json.loads(content) assert data['name'] == 'testbot' def test_app_py_uses_correct_class_name(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("my-agent", {'project_dir': str(tmp_path)}) gen._write_core_files() - content = (tmp_path / 'app.py').read_text() + content = (tmp_path / 'app.py').read_text(encoding="utf-8") assert 'class MyAgentAgent' in content assert 'name="my-agent"' in content @@ -417,6 +450,12 @@ def test_simple_files_created(self, tmp_path: Path) -> None: assert (tmp_path / 'deploy.sh').exists() assert (tmp_path / 'README.md').exists() + @pytest.mark.skipif( + sys.platform == "win32", + reason="POSIX permission bits: Windows has no execute bit, so st_mode never " + "carries 0o755. Windows-side coverage is " + "test_deploy_script_requests_executable_mode below.", + ) def test_deploy_script_is_executable(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", { 'project_dir': str(tmp_path), @@ -427,6 +466,22 @@ def test_deploy_script_is_executable(self, tmp_path: Path) -> None: mode = (tmp_path / 'deploy.sh').stat().st_mode assert mode & 0o755 == 0o755 + def test_deploy_script_requests_executable_mode(self, tmp_path: Path) -> None: + """deploy.sh must be written with executable=True on every platform.""" + gen = DokkuProjectGenerator("myapp", { + 'project_dir': str(tmp_path), + 'dokku_host': 'dokku.example.com', + 'route': 'swaig' + }) + with patch.object(Path, 'chmod', autospec=True) as mock_chmod: + gen._write_simple_files() + chmodded = { + Path(c[0][0]).name: c[0][1] for c in mock_chmod.call_args_list + } + assert chmodded == {'deploy.sh': 0o755}, ( + "deploy.sh (and only it) must be made executable" + ) + def test_deploy_script_contains_host(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", { 'project_dir': str(tmp_path), @@ -434,7 +489,7 @@ def test_deploy_script_contains_host(self, tmp_path: Path) -> None: 'route': 'swaig' }) gen._write_simple_files() - content = (tmp_path / 'deploy.sh').read_text() + content = (tmp_path / 'deploy.sh').read_text(encoding="utf-8") assert 'dokku.myhost.com' in content def test_readme_contains_app_name(self, tmp_path: Path) -> None: @@ -444,7 +499,7 @@ def test_readme_contains_app_name(self, tmp_path: Path) -> None: 'route': 'swaig' }) gen._write_simple_files() - content = (tmp_path / 'README.md').read_text() + content = (tmp_path / 'README.md').read_text(encoding="utf-8") assert 'myapp' in content def test_default_dokku_host(self, tmp_path: Path) -> None: @@ -452,10 +507,95 @@ def test_default_dokku_host(self, tmp_path: Path) -> None: 'project_dir': str(tmp_path), }) gen._write_simple_files() - content = (tmp_path / 'deploy.sh').read_text() + content = (tmp_path / 'deploy.sh').read_text(encoding="utf-8") assert 'dokku.yourdomain.com' in content +class TestGeneratedFilesAreUtf8: + """Generated files must be written as UTF-8 regardless of platform locale. + + Several templates embed box-drawing characters (U+2500 '-', U+2550 '=') and + arrows (U+2192). `Path.write_text()` without an explicit `encoding` uses the + platform default, which on Windows is cp1252 -- it cannot represent those + code points and raises `UnicodeEncodeError: 'charmap' codec can't encode + characters in position ...` (nightly Multi-OS run 30238061313, windows-latest; + 8 direct failures plus 4 more surfacing as `generate()` returning False). + + These assertions are platform-independent: they check the bytes on disk are + valid UTF-8 and round-trip to the original text, which is what an explicit + `encoding="utf-8"` guarantees and what the platform default does not. + """ + + def _non_ascii(self, text: str) -> set[str]: + return {c for c in text if ord(c) > 127} + + def test_deploy_script_round_trips_as_utf8(self, tmp_path: Path) -> None: + gen = DokkuProjectGenerator("myapp", { + 'project_dir': str(tmp_path), + 'dokku_host': 'dokku.example.com', + 'route': 'swaig', + }) + gen._write_simple_files() + + script = tmp_path / 'deploy.sh' + raw = script.read_bytes() + # Decodes as UTF-8 (would raise if written in cp1252 or latin-1)... + text = raw.decode('utf-8') + # ...and matches what the template intended, byte for byte. + assert text == script.read_text(encoding='utf-8') + # Guard the premise: this file really does carry the characters that + # break the Windows default codec. If a template edit removes them the + # test still passes but stops proving anything -- so assert they exist. + assert self._non_ascii(text), "expected non-ASCII content in deploy.sh" + + def test_generated_content_is_not_cp1252_encodable(self, tmp_path: Path) -> None: + """The regression premise: this content genuinely cannot be cp1252. + + Without this, an `encoding="utf-8"` fix could silently become untested if + the templates were ever reduced to pure ASCII. + """ + gen = DokkuProjectGenerator("myapp", { + 'project_dir': str(tmp_path), + 'dokku_host': 'dokku.example.com', + 'route': 'swaig', + }) + gen._write_simple_files() + gen._write_cicd_files() + + offenders = [] + for path in sorted(tmp_path.rglob('*')): + if not path.is_file(): + continue + text = path.read_text(encoding='utf-8') # must not raise + try: + text.encode('cp1252') + except UnicodeEncodeError: + offenders.append(path.name) + + assert offenders, ( + "no generated file contains cp1252-hostile characters -- the UTF-8 " + "regression this guards is no longer reachable; re-check the templates" + ) + + def test_all_generated_files_decode_as_utf8(self, tmp_path: Path) -> None: + """Every file a full generate() produces must be valid UTF-8.""" + gen = DokkuProjectGenerator("myapp", { + 'project_dir': str(tmp_path / 'proj'), + 'dokku_host': 'dokku.example.com', + 'route': 'swaig', + 'cicd': True, + 'web': True, + }) + assert gen.generate() is True + + checked = 0 + for path in sorted((tmp_path / 'proj').rglob('*')): + if path.is_file(): + path.read_bytes().decode('utf-8') # raises on a mis-encoded write + checked += 1 + assert checked > 0, "generate() produced no files to check" + + class TestDokkuProjectGeneratorCicdFiles: """Tests for _write_cicd_files.""" @@ -471,35 +611,35 @@ def test_cicd_files_created(self, tmp_path: Path) -> None: def test_deploy_workflow_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_cicd_files() - content = (tmp_path / '.github' / 'workflows' / 'deploy.yml').read_text() + content = (tmp_path / '.github' / 'workflows' / 'deploy.yml').read_text(encoding="utf-8") assert 'Deploy' in content assert 'dokku-deploy-system' in content def test_preview_workflow_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_cicd_files() - content = (tmp_path / '.github' / 'workflows' / 'preview.yml').read_text() + content = (tmp_path / '.github' / 'workflows' / 'preview.yml').read_text(encoding="utf-8") assert 'Preview' in content assert 'pull_request' in content def test_config_yml_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_cicd_files() - content = (tmp_path / '.dokku' / 'config.yml').read_text() + content = (tmp_path / '.dokku' / 'config.yml').read_text(encoding="utf-8") assert 'resources:' in content assert 'healthcheck:' in content def test_services_yml_content(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("myapp", {'project_dir': str(tmp_path)}) gen._write_cicd_files() - content = (tmp_path / '.dokku' / 'services.yml').read_text() + content = (tmp_path / '.dokku' / 'services.yml').read_text(encoding="utf-8") assert 'postgres:' in content assert 'redis:' in content def test_cicd_readme_contains_app_name(self, tmp_path: Path) -> None: gen = DokkuProjectGenerator("superbot", {'project_dir': str(tmp_path)}) gen._write_cicd_files() - content = (tmp_path / 'README.md').read_text() + content = (tmp_path / 'README.md').read_text(encoding="utf-8") assert 'superbot' in content @@ -521,7 +661,7 @@ def test_web_index_html_contains_agent_name(self, tmp_path: Path) -> None: 'route': 'swaig' }) gen._write_web_files() - content = (tmp_path / 'web' / 'index.html').read_text() + content = (tmp_path / 'web' / 'index.html').read_text(encoding="utf-8") assert 'Cool Bot' in content @@ -574,7 +714,7 @@ def test_full_generate_with_web(self, tmp_path: Path) -> None: result = gen.generate() assert result is True assert (out / 'web' / 'index.html').exists() - content = (out / 'app.py').read_text() + content = (out / 'app.py').read_text(encoding="utf-8") assert 'AgentServer' in content @@ -703,7 +843,7 @@ def test_init_custom_dir(self, mock_path_cls: MagicMock, mock_gen: MagicMock) -> mock_path_instance.exists.return_value = False mock_path_cls.return_value = mock_path_instance - args = self._make_args(host='dokku.example.com', dir_val='/tmp/custom') + args = self._make_args(host='dokku.example.com', dir_val='build/custom') result = cmd_init(args) assert result == 0 @@ -1180,12 +1320,12 @@ def test_main_init_force_short(self, mock_cmd_init: MagicMock) -> None: assert args.force is True @patch('signalwire.cli.dokku.cmd_init', return_value=0) - @patch('sys.argv', ['sw-agent-dokku', 'init', 'myapp', '--dir', '/tmp/out']) + @patch('sys.argv', ['sw-agent-dokku', 'init', 'myapp', '--dir', 'build/out']) def test_main_init_custom_dir(self, mock_cmd_init: MagicMock) -> None: result = main() assert result == 0 args = mock_cmd_init.call_args[0][0] - assert args.dir == '/tmp/out' + assert args.dir == 'build/out' @patch('signalwire.cli.dokku.cmd_deploy', return_value=0) @patch('sys.argv', ['sw-agent-dokku', 'deploy', '--app', 'myapp', '--host', 'dokku.test.com']) @@ -1447,7 +1587,7 @@ def test_generate_creates_project_dir_if_missing(self, tmp_path: Path) -> None: @patch('signalwire.cli.dokku.cmd_init', return_value=0) @patch('sys.argv', ['sw-agent-dokku', 'init', 'my-app', '--cicd', '--web', - '--host', 'h', '--dir', '/tmp/d', '-f']) + '--host', 'h', '--dir', 'build/d', '-f']) def test_main_all_init_flags(self, mock_cmd_init: MagicMock) -> None: """All init flags can be passed together.""" result = main() @@ -1457,5 +1597,5 @@ def test_main_all_init_flags(self, mock_cmd_init: MagicMock) -> None: assert args.cicd is True assert args.web is True assert args.host == 'h' - assert args.dir == '/tmp/d' + assert args.dir == 'build/d' assert args.force is True diff --git a/tests/unit/cli/test_init_project.py b/tests/unit/cli/test_init_project.py index e6e47cb..aa03777 100644 --- a/tests/unit/cli/test_init_project.py +++ b/tests/unit/cli/test_init_project.py @@ -359,9 +359,13 @@ class TestProjectGenerator: """Tests for the ProjectGenerator class.""" def _make_config(self, platform: str = 'local', **overrides: Any) -> dict[str, Any]: + # Relative, not '/tmp/...': the project rule forbids a hardcoded /tmp, and a + # rooted POSIX path is not portable anyway. Nothing here touches the disk -- + # every generation path is mocked -- so no real directory is needed. Tests + # that assert on the value pass an explicit `tmp_path`-derived override. config: dict[str, Any] = { 'project_name': 'test-agent', - 'project_dir': '/tmp/test-agent', # noqa: S108 + 'project_dir': 'test-agent', 'platform': platform, 'agent_type': 'basic', 'features': { @@ -379,12 +383,13 @@ def _make_config(self, platform: str = 'local', **overrides: Any) -> dict[str, A config.update(overrides) return config - def test_constructor(self) -> None: - config = self._make_config() + def test_constructor(self, tmp_path: Path) -> None: + target = tmp_path / 'test-agent' + config = self._make_config(project_dir=str(target)) gen = ProjectGenerator(config) assert gen.project_name == 'test-agent' assert gen.platform == 'local' - assert gen.project_dir == Path('/tmp/test-agent') # noqa: S108 + assert gen.project_dir == target @patch.object(ProjectGenerator, '_generate_local', return_value=True) def test_generate_dispatches_to_local(self, mock_gen: MagicMock) -> None: @@ -578,16 +583,21 @@ def test_main_aws_platform(self, mock_gen_class: MagicMock) -> None: assert config['platform'] == 'aws' @patch('signalwire.cli.init_project.ProjectGenerator') - @patch('sys.argv', ['sw-agent-init', 'testproject', '--no-venv', '--dir', '/tmp/custom']) # noqa: S108 - def test_main_custom_dir(self, mock_gen_class: MagicMock) -> None: + def test_main_custom_dir(self, mock_gen_class: MagicMock, tmp_path: Path) -> None: mock_gen = Mock() mock_gen.generate.return_value = True mock_gen_class.return_value = mock_gen - main() + custom = tmp_path / 'custom' + with patch('sys.argv', + ['sw-agent-init', 'testproject', '--no-venv', '--dir', str(custom)]): + main() config = mock_gen_class.call_args[0][0] - assert '/tmp/custom' in config['project_dir'] # noqa: S108 + # main() builds `(Path(args.dir) / args.name).absolute()`. Compare Paths -- + # a substring check against a POSIX literal fails on Windows, where the + # separator is `\` and a rooted POSIX path gains a drive letter. + assert Path(config['project_dir']) == (custom / 'testproject').absolute() @patch('signalwire.cli.init_project.ProjectGenerator') @patch('sys.argv', ['sw-agent-init', 'testproject', '--no-venv']) diff --git a/tests/unit/search/test_index_builder.py b/tests/unit/search/test_index_builder.py index f633340..1233fbb 100644 --- a/tests/unit/search/test_index_builder.py +++ b/tests/unit/search/test_index_builder.py @@ -16,6 +16,8 @@ import os import sqlite3 import json +from contextlib import closing +from typing import Any from unittest.mock import Mock, patch, MagicMock, mock_open from pathlib import Path @@ -500,11 +502,117 @@ def test_validate_index_database_error(self) -> None: with tempfile.NamedTemporaryFile(suffix='.db', delete=False) as f: self.temp_db = f.name f.write(b"invalid sqlite data") - + result = self.builder.validate_index(self.temp_db) - + + assert result["valid"] is False + assert "error" in result + + +class TestValidateIndexClosesConnection: + """`validate_index` must close its sqlite connection on EVERY return path. + + A leaked handle is invisible on POSIX (unlink succeeds regardless) but on + Windows it makes the file undeletable -- `PermissionError: [WinError 32] The + process cannot access the file because it is being used by another process`, + which is how this surfaced (nightly Multi-OS run 30238061313, windows-latest). + + These tests assert the platform-independent invariant -- that every connection + opened is also closed -- so the Windows-only bug is provable on POSIX too. + """ + + def _connect_spy( + self, monkeypatch: pytest.MonkeyPatch + ) -> list[sqlite3.Connection]: + """Record every Connection validate_index opens, so we can assert it closed.""" + opened: list[sqlite3.Connection] = [] + real_connect = sqlite3.connect + + def spy(*args: Any, **kwargs: Any) -> sqlite3.Connection: + # `sqlite3.connect` is overloaded, so calling it through *args widens + # the result to Any; annotate to keep the spy's return type honest. + conn: sqlite3.Connection = real_connect(*args, **kwargs) + opened.append(conn) + return conn + + monkeypatch.setattr( + "signalwire.search.index_builder.sqlite3.connect", spy + ) + return opened + + @staticmethod + def _is_closed(conn: sqlite3.Connection) -> bool: + """A closed Connection raises ProgrammingError on any further use.""" + try: + conn.execute("SELECT 1") + except sqlite3.ProgrammingError: + return True + return False + + def test_missing_tables_path_closes_connection( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The `Missing tables` early return must not leak the handle.""" + db = tmp_path / "missing_tables.db" + with closing(sqlite3.connect(str(db))) as setup: + setup.execute("CREATE TABLE chunks (id INTEGER)") + setup.commit() + + opened = self._connect_spy(monkeypatch) + result = IndexBuilder().validate_index(str(db)) + + assert result["valid"] is False + assert "Missing tables" in result["error"] + assert len(opened) == 1, "expected validate_index to open exactly one connection" + assert self._is_closed(opened[0]), "connection leaked on the missing-tables path" + + # The operation Windows refuses when a handle is still open. + db.unlink() + + def test_database_error_path_closes_connection( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The exception path must not leak the handle either. + + `sqlite3.connect` is lazy, so it succeeds on a non-database file and the + failure surfaces from the first query -- inside the `try`, after the + connection exists. + """ + db = tmp_path / "not_a_database.db" + db.write_bytes(b"invalid sqlite data") + + opened = self._connect_spy(monkeypatch) + result = IndexBuilder().validate_index(str(db)) + assert result["valid"] is False assert "error" in result + assert len(opened) == 1, "expected validate_index to open exactly one connection" + assert self._is_closed(opened[0]), "connection leaked on the error path" + + db.unlink() + + def test_valid_index_path_closes_connection( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The success path must close too (it did before; guard against regression).""" + db = tmp_path / "valid.db" + builder = IndexBuilder() + builder._create_database( + str(db), + [{"content": "Test", "filename": "t.txt", "embedding": b"data"}], + ["en"], + ["/src"], + ["txt"], + ) + + opened = self._connect_spy(monkeypatch) + result = builder.validate_index(str(db)) + + assert result["valid"] is True + assert len(opened) == 1 + assert self._is_closed(opened[0]), "connection leaked on the success path" + + db.unlink() class TestIndexBuilderBuildMethods: @@ -563,10 +671,12 @@ def test_build_index_from_sources_success(self, mock_preprocess: MagicMock) -> N mock_create_db.assert_called_once() assert mock_model.encode.call_count == 2 - def test_build_index_from_sources_no_files(self) -> None: + def test_build_index_from_sources_no_files(self, tmp_path: Path) -> None: """Test index building with no files found""" - # Don't create temp file since method should return early - temp_db = "/tmp/nonexistent.db" + # A path inside tmp_path that is deliberately never created: the method + # must return early. (`tmp_path`, not a hardcoded /tmp -- project rule, + # and it guarantees a clean directory.) + temp_db = str(tmp_path / "nonexistent.db") with patch.object(self.builder, '_discover_files_from_sources', return_value=[]): sources = [Path("/empty/dir")] @@ -578,10 +688,10 @@ def test_build_index_from_sources_no_files(self) -> None: # Database should not be created assert not os.path.exists(temp_db) - def test_build_index_from_sources_no_chunks(self) -> None: + def test_build_index_from_sources_no_chunks(self, tmp_path: Path) -> None: """Test index building with no chunks created""" - # Don't create temp file since method should return early - temp_db = "/tmp/nonexistent2.db" + # Deliberately-absent path; the method must return early. + temp_db = str(tmp_path / "nonexistent2.db") mock_files = [Path("test.txt")] diff --git a/tests/unit/search/test_search_engine.py b/tests/unit/search/test_search_engine.py index 6c9ff6b..5bfb991 100644 --- a/tests/unit/search/test_search_engine.py +++ b/tests/unit/search/test_search_engine.py @@ -16,6 +16,7 @@ import json import tempfile import os +from contextlib import closing from unittest.mock import Mock, patch, MagicMock from pathlib import Path @@ -26,11 +27,14 @@ class TestSearchEngineInit: """Test SearchEngine initialization""" - def test_init_with_valid_index(self) -> None: + def test_init_with_valid_index(self, tmp_path: Path) -> None: """Test initialization with valid index file""" - with tempfile.NamedTemporaryFile(suffix='.db', delete=False) as tmp: - # Create a minimal database - conn = sqlite3.connect(tmp.name) + # `tmp_path`, not NamedTemporaryFile(delete=False): the latter keeps its own + # OS handle open for the life of the `with` block, and on Windows a file with + # a live handle cannot be unlinked (PermissionError/WinError 32). + db_path = str(tmp_path / 'valid_index.db') + # Create a minimal database + with closing(sqlite3.connect(db_path)) as conn: cursor = conn.cursor() cursor.execute(''' CREATE TABLE config (key TEXT, value TEXT) @@ -39,28 +43,23 @@ def test_init_with_valid_index(self) -> None: INSERT INTO config (key, value) VALUES ('embedding_dimensions', '768') ''') conn.commit() - conn.close() - - engine = SearchEngine(backend='sqlite', index_path=tmp.name) - assert engine.index_path == tmp.name - assert engine.embedding_dim == 768 - assert engine.config['embedding_dimensions'] == '768' - os.unlink(tmp.name) + engine = SearchEngine(backend='sqlite', index_path=db_path) + assert engine.index_path == db_path + assert engine.embedding_dim == 768 + assert engine.config['embedding_dimensions'] == '768' - def test_init_with_missing_config(self) -> None: + def test_init_with_missing_config(self, tmp_path: Path) -> None: """Test initialization with missing config table""" - with tempfile.NamedTemporaryFile(suffix='.db', delete=False) as tmp: - # Create empty database - conn = sqlite3.connect(tmp.name) - conn.close() + db_path = str(tmp_path / 'missing_config.db') + # Create empty database + with closing(sqlite3.connect(db_path)): + pass - engine = SearchEngine(backend='sqlite', index_path=tmp.name) - assert engine.index_path == tmp.name - assert engine.embedding_dim == 768 # Default value - assert engine.config == {} - - os.unlink(tmp.name) + engine = SearchEngine(backend='sqlite', index_path=db_path) + assert engine.index_path == db_path + assert engine.embedding_dim == 768 # Default value + assert engine.config == {} def test_init_with_nonexistent_file(self) -> None: """Test initialization with nonexistent index file""" @@ -535,15 +534,15 @@ def test_filter_by_tags_no_metadata(self) -> None: class TestSearchEngineEdgeCases: """Test edge cases and error handling""" - def test_fallback_search(self) -> None: + def test_fallback_search(self, tmp_path: Path) -> None: """Test fallback search functionality""" - with tempfile.NamedTemporaryFile(suffix='.db', delete=False) as tmp: - # Create database with fallback search capability - conn = sqlite3.connect(tmp.name) + db_path = str(tmp_path / 'fallback.db') + # Create database with fallback search capability + with closing(sqlite3.connect(db_path)) as conn: cursor = conn.cursor() - + cursor.execute('CREATE TABLE config (key TEXT, value TEXT)') - + cursor.execute(''' CREATE TABLE chunks ( id INTEGER PRIMARY KEY, @@ -555,23 +554,20 @@ def test_fallback_search(self) -> None: processed_content TEXT ) ''') - + cursor.execute(''' INSERT INTO chunks (content, filename, section, tags, metadata, processed_content) VALUES (?, ?, ?, ?, ?, ?) ''', ('Python tutorial content', 'tutorial.md', 'intro', '["python"]', '{}', 'python tutorial content')) - + conn.commit() - conn.close() - - engine = SearchEngine(backend='sqlite', index_path=tmp.name) - results = engine._fallback_search('Python', count=1) - - assert len(results) == 1 - assert 'Python' in results[0]['content'] - assert results[0]['search_type'] == 'fallback' - - os.unlink(tmp.name) + + engine = SearchEngine(backend='sqlite', index_path=db_path) + results = engine._fallback_search('Python', count=1) + + assert len(results) == 1 + assert 'Python' in results[0]['content'] + assert results[0]['search_type'] == 'fallback' def test_fallback_search_database_error(self) -> None: """Test fallback search with database error""" From 0f5d0eec2d485c30dedb0355383fa12004d8c0e5 Mon Sep 17 00:00:00 2001 From: Michael Jerris Date: Mon, 27 Jul 2026 07:16:11 -0400 Subject: [PATCH 4/5] fix(tests): compare UTF-8 round-trip to the template, not a second read The new test_deploy_script_round_trips_as_utf8 asserted read_bytes().decode('utf-8') == read_text(encoding='utf-8'). Those two views legitimately differ on Windows: text-mode writes translate \n -> \r\n and read_text translates it back (universal newlines), so the assertion compared raw CRLF against normalized LF and failed on the runner -- while the encoding fix it was guarding was working correctly (every U+2550/U+2192/U+2705/U+1F310 round-tripped intact). Line endings are not what the test is about. It now compares against the source DEPLOY_SCRIPT_TEMPLATE line-by-line and asserts the non-ASCII character set survives byte-for-byte, which is the actual regression being guarded. Still verified to fail against the unfixed product code (all 3 tests in the class fail under LC_ALL=C -X utf8=0 without the encoding fix). Caught by Multi-OS run 30260346853 (windows-latest), which this branch dispatched -- Windows TEST went 36 failed -> 16 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GFKJhLvfV8yGrASwqxdgaf --- tests/unit/cli/test_dokku.py | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/tests/unit/cli/test_dokku.py b/tests/unit/cli/test_dokku.py index 43bf42c..ad420f4 100644 --- a/tests/unit/cli/test_dokku.py +++ b/tests/unit/cli/test_dokku.py @@ -538,14 +538,26 @@ def test_deploy_script_round_trips_as_utf8(self, tmp_path: Path) -> None: gen._write_simple_files() script = tmp_path / 'deploy.sh' - raw = script.read_bytes() - # Decodes as UTF-8 (would raise if written in cp1252 or latin-1)... - text = raw.decode('utf-8') - # ...and matches what the template intended, byte for byte. - assert text == script.read_text(encoding='utf-8') - # Guard the premise: this file really does carry the characters that - # break the Windows default codec. If a template edit removes them the - # test still passes but stops proving anything -- so assert they exist. + # Decodes as UTF-8 -- raises if the file was written in cp1252/latin-1. + text = script.read_bytes().decode('utf-8') + + # Compare against the source template, NOT against a second read of the + # file: text-mode writes translate "\n" -> "\r\n" on Windows and + # `read_text` translates it back (universal newlines), so + # `read_bytes().decode()` and `read_text()` legitimately differ there. + # Line endings are not what this test is about -- the encoding of the + # non-ASCII characters is. (Comparing those two reads is what made this + # test fail on the Windows runner while the encoding fix itself was fine.) + expected = DEPLOY_SCRIPT_TEMPLATE.format( + app_name='myapp', dokku_host='dokku.example.com', route='swaig' + ) + assert text.splitlines() == expected.splitlines() + + # The characters that break the Windows default codec must survive + # byte-for-byte -- this is the actual regression being guarded. + assert self._non_ascii(text) == self._non_ascii(expected) + # Guard the premise: if a template edit ever removed these, the test + # would still pass but stop proving anything. assert self._non_ascii(text), "expected non-ASCII content in deploy.sh" def test_generated_content_is_not_cp1252_encodable(self, tmp_path: Path) -> None: From 330319b21c6f83dff355bad395b3621497c8b1be Mon Sep 17 00:00:00 2001 From: Michael Jerris Date: Mon, 27 Jul 2026 10:16:16 -0400 Subject: [PATCH 5/5] =?UTF-8?q?fix(tests):=20finish=20the=20Windows=20port?= =?UTF-8?q?ability=20tail=20=E2=80=94=20path=20separators,=20signal=20hand?= =?UTF-8?q?lers,=20skills=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the 15 Windows TEST failures left by PR #76 (which took the count 36 -> 15), measured from Multi-OS run 30261304144 / windows-latest. One of the three defect classes is a PRODUCT bug, not a test bug. 1. PRODUCT — RelayClient.run() was broken for every Windows user (7 tests) _run_forever() called loop.add_signal_handler(SIGINT, ...) unguarded on its FIRST statement. Loop-level signal handling is a Unix-only asyncio capability: both Windows event loops raise NotImplementedError unconditionally, so the exception escaped before connect() was ever reached — RelayClient.run() could not establish a RELAY connection on Windows at all. Guarded with a NotImplementedError fallback that degrades to KeyboardInterrupt-driven shutdown (Ctrl+C still stops the client; only the graceful _shutdown() handshake is lost). Also replaced the __import__("signal") inline with a module-level import. Covered by a new platform-independent regression test that forces the exact Windows condition (patching the loop method to raise what Windows raises) rather than skipping on win32, so the contract is exercised on every OS. Verified to FAIL against the unfixed product on macOS with the same NotImplementedError at client.py:684 as the Windows traceback. The 7th relay failure was a separate mechanism: the ping-loop test patched _EXECUTE_TIMEOUT=0.01 around client.connect(), putting the auth round-trip under a 10ms deadline. The handshake needs several event-loop turns, and 10ms is at/below the Windows asyncio timer granularity (~15.6ms clock tick), so connect() could time out before the loop ran the recv task. Scoped the patch to the pings the test is actually about. A deadline sweep on POSIX reproduces the identical "Request timeout for signalwire.connect" error deterministically (20/20) once the deadline drops below the turns required. 2. TESTS — POSIX-separator expectations (3 tests) test_schema_utils (2) and test_registry (1) compared product output to hardcoded POSIX literals. The product is separator-correct in all three cases: it returns str(Path(...)), composes with os.path.join(), and splits on os.pathsep. Build the expectation the same way the product builds the value (compare Path objects / computed joins) instead of a POSIX-only spelling. Confirmed under Windows path semantics (ntpath/PureWindowsPath) on POSIX: the old literals match only on POSIX, the new expectations match on both — so Windows keeps real coverage rather than losing it to a skip. 3. TESTS — claude_skills paths (5 tests) - WinError 267 ("directory name is invalid"): the test passed Path("/tmp") as the subprocess cwd. /tmp does not exist on Windows, so the spawn failed before the timeout could fire. Switched to pytest tmp_path. The command was also `sleep 10`, which is not a Windows shell builtin and exits immediately; replaced with a portable python -c sleep so the timeout path is genuinely exercised (test now takes ~1.01s, i.e. it really blocks). - 8.3 short-path mismatches (3 tests): setup() canonicalizes skills_path via .resolve(), which expands the Windows 8.3 temp dir to its long form (RUNNER~1 -> runneradmin). The tests compared an unresolved tempfile path to resolved product output. Resolve both sides. - Removed all 15 hardcoded /tmp uses from this file (a standing project rule bans /tmp outright); they were inert placeholders except where noted above. No test was blanket-skipped: every disposition is either a product fix or an expectation corrected to match platform-correct product behavior. Verification: bash scripts/run-ci.sh exit 0, all gates PASS (TEST, FMT, LINT, TYPECHECK, DRIFT, SPEC-PARITY, REST-COVERAGE, ...); tests/unit 5710 passed, 3 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GFKJhLvfV8yGrASwqxdgaf --- signalwire/signalwire/relay/client.py | 25 ++++- tests/unit/relay/test_client.py | 71 +++++++++++++- tests/unit/skills/test_claude_skills_skill.py | 92 ++++++++++++------- tests/unit/skills/test_registry.py | 10 +- tests/unit/utils/test_schema_utils.py | 30 ++++-- 5 files changed, 177 insertions(+), 51 deletions(-) diff --git a/signalwire/signalwire/relay/client.py b/signalwire/signalwire/relay/client.py index 6dc4252..2d718ae 100644 --- a/signalwire/signalwire/relay/client.py +++ b/signalwire/signalwire/relay/client.py @@ -24,6 +24,7 @@ import json import os import re +import signal import ssl as ssl_module import uuid from typing import Any, TYPE_CHECKING @@ -680,11 +681,27 @@ async def _run_forever(self) -> None: """Connect and maintain the connection with auto-reconnect.""" # Register SIGINT handler so Ctrl+C triggers a clean shutdown # instead of dumping a stack trace. + # + # loop.add_signal_handler() is a Unix-only asyncio capability: the + # Windows Proactor/Selector loops raise NotImplementedError + # unconditionally (CPython Lib/asyncio/events.py). Without this guard a + # bare `NotImplementedError` escaped _run_forever() on the very first + # statement, so RelayClient.run() could never connect on Windows at + # all. Degrade instead: on a platform with no loop-level signal + # handling, Ctrl+C still stops the client — asyncio.run() surfaces it + # as KeyboardInterrupt, which run() suppresses — we just lose the + # graceful _shutdown() handshake. loop = asyncio.get_running_loop() - loop.add_signal_handler( - __import__("signal").SIGINT, - lambda: asyncio.ensure_future(self._shutdown()), - ) + try: + loop.add_signal_handler( + signal.SIGINT, + lambda: asyncio.ensure_future(self._shutdown()), + ) + except NotImplementedError: + logger.debug( + "Loop-level SIGINT handling unavailable on this platform; " + "falling back to KeyboardInterrupt-driven shutdown" + ) while not self._closing: try: diff --git a/tests/unit/relay/test_client.py b/tests/unit/relay/test_client.py index 90e1507..348ac2f 100644 --- a/tests/unit/relay/test_client.py +++ b/tests/unit/relay/test_client.py @@ -1478,6 +1478,59 @@ async def cancel_connect(*args: Any, **kwargs: Any) -> AutoAuthMockWebSocket: assert client._ws is None _active_clients.clear() + @pytest.mark.asyncio + async def test_run_forever_survives_loop_without_signal_handlers(self) -> None: + """_run_forever must not die where loop.add_signal_handler is unsupported. + + asyncio's loop-level signal handling is Unix-only: on Windows BOTH the + Proactor and Selector loops raise NotImplementedError unconditionally. + Before the guard, that exception escaped on the FIRST statement of + _run_forever(), so RelayClient.run() never reached connect() on Windows + — a real product defect, not a test artifact. + + This reproduces the platform condition directly (patching the loop + method to raise exactly what Windows raises) rather than skipping on + win32, so the contract is covered on every OS the suite runs on. + """ + _active_clients.clear() + connect_count = 0 + + async def mock_connect(*args: Any, **kwargs: Any) -> AutoAuthMockWebSocket: + nonlocal connect_count + connect_count += 1 + return AutoAuthMockWebSocket() + + loop = asyncio.get_running_loop() + + def no_signal_handlers(*args: Any, **kwargs: Any) -> None: + raise NotImplementedError + + with ( + patch( + "signalwire.relay.client.websockets.connect", side_effect=mock_connect + ), + patch("signalwire.relay.client._CLIENT_PING_INTERVAL", 999), + patch.object(loop, "add_signal_handler", no_signal_handlers), + ): + client = RelayClient(project="p", token="t") + + async def stop_after_connect() -> None: + while connect_count < 1: + await asyncio.sleep(0.01) + await asyncio.sleep(0.05) + client._closing = True + if client._ws: + await client._ws.close() + + task = asyncio.ensure_future(client._run_forever()) + stopper = asyncio.ensure_future(stop_after_connect()) + # The bug made this raise NotImplementedError instead of connecting. + await asyncio.wait_for(task, timeout=5.0) + stopper.cancel() + # It got PAST the signal registration and did real work. + assert connect_count >= 1 + _active_clients.clear() + # =================================================================== # PY-4 / A6 — bounded reconnect on PERMANENT auth rejection. @@ -1988,18 +2041,26 @@ async def test_ping_loop_max_failures_force_close(self) -> None: return_value=ws, ), patch("signalwire.relay.client._CLIENT_PING_INTERVAL", 0.01), - patch("signalwire.relay.client._EXECUTE_TIMEOUT", 0.01), patch("signalwire.relay.client._MAX_PING_FAILURES", 1), patch("signalwire.relay.client.RECONNECT_MIN_DELAY", 0.01), ): client = RelayClient(project="p", token="t") + # connect() must NOT run under the 10ms ping timeout. The auth + # round-trip needs several event-loop turns (AutoAuthMockWebSocket + # queues the reply, then _recv_task has to be scheduled and drain + # it), and _send_request reads _EXECUTE_TIMEOUT at call time. A 10ms + # deadline is below the Windows asyncio timer granularity (~15.6ms + # clock tick), so the connect request could time out before the loop + # ever ran the recv task — "Request timeout for signalwire.connect". + # Shorten the timeout only for the pings this test is about. await client.connect() - # Don't respond to pings — they'll timeout and trigger force_close - await asyncio.sleep(0.3) + with patch("signalwire.relay.client._EXECUTE_TIMEOUT", 0.01): + # Don't respond to pings — they'll timeout and trigger force_close + await asyncio.sleep(0.3) - # After max failures, should have force-closed - assert client._connected is False + # After max failures, should have force-closed + assert client._connected is False await client.disconnect() _active_clients.clear() diff --git a/tests/unit/skills/test_claude_skills_skill.py b/tests/unit/skills/test_claude_skills_skill.py index d289843..8963761 100644 --- a/tests/unit/skills/test_claude_skills_skill.py +++ b/tests/unit/skills/test_claude_skills_skill.py @@ -9,6 +9,7 @@ Unit tests for the Claude Skills skill module. """ +import sys import tempfile from pathlib import Path from typing import Any @@ -285,19 +286,32 @@ def test_enabled_executes_command(self) -> None: assert "hello" in result.response assert "!`" not in result.response - def test_timeout_handling(self) -> None: - skill = _make_skill({"skills_path": "/tmp", "allow_shell_injection": True}) # noqa: S108 + def test_timeout_handling(self, tmp_path: Path) -> None: + skill = _make_skill( + {"skills_path": str(tmp_path), "allow_shell_injection": True} + ) skill._allow_shell_injection = True skill._shell_timeout = 1 - content = "!`sleep 10`" - result = skill._execute_shell_injection(content, Path("/tmp"), timeout=1) # noqa: S108 + # The command must block for longer than the timeout on EVERY platform: + # `sleep` is not a Windows shell builtin, so it exits immediately there + # and the timeout path is never reached. A python -c sleep is portable + # and is the same interpreter already running the suite. + blocking = f'"{sys.executable}" -c "import time; time.sleep(10)"' + content = f"!`{blocking}`" + # cwd must be a directory that exists on this platform: the product + # passes it to subprocess.run(cwd=...), and a nonexistent cwd fails the + # spawn outright ([WinError 267] The directory name is invalid) before + # the timeout can fire, so the assertion would never see a timeout. + result = skill._execute_shell_injection(content, tmp_path, timeout=1) assert "[command timed out:" in result - def test_error_handling(self) -> None: - skill = _make_skill({"skills_path": "/tmp", "allow_shell_injection": True}) # noqa: S108 + def test_error_handling(self, tmp_path: Path) -> None: + skill = _make_skill( + {"skills_path": str(tmp_path), "allow_shell_injection": True} + ) skill._allow_shell_injection = True content = "!`nonexistent_command_xyz_12345`" - result = skill._execute_shell_injection(content, Path("/tmp"), timeout=5) # noqa: S108 + result = skill._execute_shell_injection(content, tmp_path, timeout=5) # The command will produce stderr but still return (non-zero exit code) # subprocess.run doesn't raise on non-zero exit, so result is stdout (empty) # This is expected behavior — command runs but produces no stdout @@ -311,28 +325,32 @@ def test_error_handling(self) -> None: class TestVariableSubstitution: """Test ${CLAUDE_SKILL_DIR} and ${CLAUDE_SESSION_ID} substitution.""" - def test_skill_dir_replaced(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_skill_dir_replaced(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) content = "Path: ${CLAUDE_SKILL_DIR}/file.txt" - result = skill._substitute_variables(content, Path("/opt/skills/my-skill")) - assert result == "Path: /opt/skills/my-skill/file.txt" - - def test_session_id_replaced(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + # The product substitutes str(skill_dir), so the expectation must be + # built from the same Path rather than a POSIX literal: + # str(Path("/opt/skills/my-skill")) is "\opt\skills\my-skill" on Windows. + skill_dir = Path("/opt/skills/my-skill") + result = skill._substitute_variables(content, skill_dir) + assert result == f"Path: {skill_dir}/file.txt" + + def test_session_id_replaced(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) content = "Session: ${CLAUDE_SESSION_ID}" - result = skill._substitute_variables(content, Path("/tmp"), {"call_id": "abc-123"}) # noqa: S108 + result = skill._substitute_variables(content, tmp_path, {"call_id": "abc-123"}) assert result == "Session: abc-123" - def test_missing_raw_data_graceful(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_missing_raw_data_graceful(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) content = "Session: ${CLAUDE_SESSION_ID}" - result = skill._substitute_variables(content, Path("/tmp"), None) # noqa: S108 + result = skill._substitute_variables(content, tmp_path, None) assert result == "Session: " - def test_missing_call_id_graceful(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_missing_call_id_graceful(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) content = "Session: ${CLAUDE_SESSION_ID}" - result = skill._substitute_variables(content, Path("/tmp"), {"other_key": "val"}) # noqa: S108 + result = skill._substitute_variables(content, tmp_path, {"other_key": "val"}) assert result == "Session: " @@ -343,27 +361,27 @@ def test_missing_call_id_graceful(self) -> None: class TestFallbackArguments: """Test fallback argument appending when body lacks bare $ARGUMENTS.""" - def test_body_with_bare_arguments_no_fallback(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_body_with_bare_arguments_no_fallback(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) result = skill._substitute_arguments("Use $ARGUMENTS here", "some input") assert result == "Use some input here" assert "ARGUMENTS:" not in result - def test_body_without_arguments_appends_fallback(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_body_without_arguments_appends_fallback(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) result = skill._substitute_arguments("Do the thing", "some input") assert "Do the thing" in result assert "\n\nARGUMENTS: some input" in result - def test_indexed_form_triggers_fallback(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_indexed_form_triggers_fallback(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) result = skill._substitute_arguments("Use $ARGUMENTS[0] only", "hello world") # $ARGUMENTS[0] is indexed — bare $ARGUMENTS not present, so fallback appends assert "hello" in result assert "\n\nARGUMENTS: hello world" in result - def test_empty_arguments_no_append(self) -> None: - skill = _make_skill({"skills_path": "/tmp"}) # noqa: S108 + def test_empty_arguments_no_append(self, tmp_path: Path) -> None: + skill = _make_skill({"skills_path": str(tmp_path)}) result = skill._substitute_arguments("Do the thing", "") assert result == "Do the thing" assert "ARGUMENTS:" not in result @@ -563,7 +581,13 @@ class TestHandlerPipeline: def test_full_pipeline_ordering(self) -> None: """Shell injection -> variables -> arguments -> wrapping.""" with tempfile.TemporaryDirectory() as tmpdir: - skill_dir = Path(tmpdir) / "pipeline-skill" + # setup() canonicalizes skills_path via .resolve(), so the dir the + # product reports is the RESOLVED one. On Windows the temp dir is + # handed out in 8.3 short form (C:\Users\RUNNER~1\...) and resolve() + # expands it to the long form (C:\Users\runneradmin\...) — comparing + # an unresolved expectation to resolved output fails. Resolve here + # too, so both sides name the same directory the same way. + skill_dir = (Path(tmpdir).resolve()) / "pipeline-skill" body = "Dir: ${CLAUDE_SKILL_DIR} | Args: $ARGUMENTS" _write_skill_md(skill_dir, "pipeline-skill", body=body) @@ -591,7 +615,9 @@ def test_full_pipeline_ordering(self) -> None: def test_shell_then_variables_then_arguments(self) -> None: """Verify processing order: shell first, then vars, then args.""" with tempfile.TemporaryDirectory() as tmpdir: - skill_dir = Path(tmpdir) / "order-skill" + # .resolve() to match the product's canonicalized skills_path — see + # test_full_pipeline_ordering for the Windows 8.3 short-path detail. + skill_dir = (Path(tmpdir).resolve()) / "order-skill" # Shell outputs something, then variable and arg substitution happens body = "Shell: !`echo shellout` | Dir: ${CLAUDE_SKILL_DIR} | Arg: $ARGUMENTS" _write_skill_md(skill_dir, "order-skill", body=body) @@ -632,7 +658,9 @@ def test_variable_substitution_in_handler(self) -> None: def test_section_loading_with_pipeline(self) -> None: """Test that section files also go through the pipeline.""" with tempfile.TemporaryDirectory() as tmpdir: - skill_dir = Path(tmpdir) / "section-skill" + # .resolve() to match the product's canonicalized skills_path — see + # test_full_pipeline_ordering for the Windows 8.3 short-path detail. + skill_dir = (Path(tmpdir).resolve()) / "section-skill" _write_skill_md(skill_dir, "section-skill", body="Main body") # Create a section file with variable placeholders diff --git a/tests/unit/skills/test_registry.py b/tests/unit/skills/test_registry.py index 373c041..cfd9412 100644 --- a/tests/unit/skills/test_registry.py +++ b/tests/unit/skills/test_registry.py @@ -1408,15 +1408,21 @@ def test_load_on_demand_searches_env_paths(self) -> None: call_log = [] + # The product turns each entry into Path(path_str), so match on the Path + # itself instead of a str() spelling of it: str(Path("/env/skills")) is + # "\env\skills" on Windows, so the literal comparison never matched + # there and the env-var search looked broken when it was not. + env_path = Path("/env/skills") + def fake_load_from_path(name: str, path: Path) -> type[SkillBase] | None: call_log.append((name, path)) - if str(path) == "/env/skills": + if path == env_path: return MockSkill return None with patch.object(registry, '_load_entry_points'): with patch.object(registry, '_load_skill_from_path', side_effect=fake_load_from_path): - with patch.dict('os.environ', {'SIGNALWIRE_SKILL_PATHS': '/env/skills'}): + with patch.dict('os.environ', {'SIGNALWIRE_SKILL_PATHS': str(env_path)}): result = registry._load_skill_on_demand("mock_skill") assert result is MockSkill diff --git a/tests/unit/utils/test_schema_utils.py b/tests/unit/utils/test_schema_utils.py index b68bf1a..73d8634 100644 --- a/tests/unit/utils/test_schema_utils.py +++ b/tests/unit/utils/test_schema_utils.py @@ -69,17 +69,23 @@ def test_get_default_schema_path_importlib_resources_new(self) -> None: def test_get_default_schema_path_importlib_resources_old(self) -> None: """Test default schema path using importlib.resources (Python 3.7-3.8)""" utils = SchemaUtils.__new__(SchemaUtils) - + + # The resource is a Path, and the method returns str(path) — so the + # expected value must be built the same way rather than hardcoded as a + # POSIX literal. str(Path("/old/schema.json")) is "\old\schema.json" on + # Windows, which is correct behavior, not a bug. + resource = Path("/old") / "schema.json" + with patch('importlib.resources.files', side_effect=AttributeError): with patch('importlib.resources.path') as mock_path: mock_context = Mock() - mock_context.__enter__ = Mock(return_value=Path("/old/schema.json")) + mock_context.__enter__ = Mock(return_value=resource) mock_context.__exit__ = Mock(return_value=None) mock_path.return_value = mock_context - + result = utils._get_default_schema_path() - - assert result == "/old/schema.json" + + assert result == str(resource) def test_get_default_schema_path_manual_search(self) -> None: """Test default schema path using manual file search when importlib.resources fails""" @@ -94,15 +100,23 @@ def failing_files(package: str) -> Traversable: raise ImportError("mocked") return original_files(package) + # The product composes candidates with os.path.join(os.getcwd(), ...), + # which yields "/current\schema.json" on Windows. Build the expected + # string with the same join so the test asserts the contract (the cwd + # candidate is searched first and returned) instead of a POSIX-only + # spelling of it. + cwd = os.path.join(os.sep, "current") # noqa: PTH118 + expected = os.path.join(cwd, "schema.json") # noqa: PTH118 + with patch('importlib.resources.files', side_effect=failing_files): with patch('os.path.exists') as mock_exists: - with patch('os.getcwd', return_value="/current"): + with patch('os.getcwd', return_value=cwd): # First path exists - mock_exists.side_effect = lambda path: path == "/current/schema.json" + mock_exists.side_effect = lambda path: path == expected result = utils._get_default_schema_path() - assert result == "/current/schema.json" + assert result == expected def test_get_default_schema_path_not_found(self) -> None: """Test default schema path when file is not found anywhere"""