diff --git a/.github/workflows/doc-audit.yml b/.github/workflows/doc-audit.yml index afa15730..c5d03aa6 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 15d0469e..b59b6309 100644 --- a/.github/workflows/multi-os.yml +++ b/.github/workflows/multi-os.yml @@ -65,4 +65,16 @@ 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`: 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 8ff6e385..f1370874 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 diff --git a/signalwire/signalwire/cli/dokku.py b/signalwire/signalwire/cli/dokku.py index f181aad5..294d7b84 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 e75d2f65..05af090d 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/relay/client.py b/signalwire/signalwire/relay/client.py index 6dc4252e..2d718aea 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/signalwire/signalwire/search/index_builder.py b/signalwire/signalwire/search/index_builder.py index 71ab1b58..4bcf1ea2 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 e12e36d6..091844e8 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 d7193718..54a91b9b 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 4b5c6412..ad420f44 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,107 @@ 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' + # 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: + """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 +623,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 +673,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 +726,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 +855,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 +1332,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 +1599,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 +1609,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 e6e47cbb..aa03777a 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/relay/test_client.py b/tests/unit/relay/test_client.py index 90e15079..348ac2f8 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/search/test_index_builder.py b/tests/unit/search/test_index_builder.py index f6333401..1233fbbe 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 6c9ff6bf..5bfb9915 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""" diff --git a/tests/unit/skills/test_claude_skills_skill.py b/tests/unit/skills/test_claude_skills_skill.py index d2898435..8963761e 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 373c041c..cfd94121 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 b68bf1a4..73d86344 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"""