From 30c95712d6dc8133b6048835b1e7614f3d647f8f Mon Sep 17 00:00:00 2001 From: "Strix (Claude Opus 4.6)" Date: Mon, 23 Mar 2026 13:09:54 +0000 Subject: [PATCH 1/2] Add pre-commit hook + protocol conformance tests Pre-commit hook runs pyright (advisory, skipped on low-memory servers) + pytest (blocking) before every commit. Conformance tests exercise LoggingWriteGuardBackend through CompositeBackend with the exact positional arg patterns that caused the Keel crash (PR #55). Also fixes pre-existing broken imports in test_config_folders.py and test_write_guard.py (WriteGuardBackend moved to readonly_backend). Co-Authored-By: Claude Opus 4.6 --- scripts/install_pre_commit_hook.sh | 52 ++++++++ tests/test_config_folders.py | 2 +- tests/test_logging_backend.py | 184 +++++++++++++++++++++++++++++ tests/test_write_guard.py | 2 +- 4 files changed, 238 insertions(+), 2 deletions(-) create mode 100755 scripts/install_pre_commit_hook.sh create mode 100644 tests/test_logging_backend.py diff --git a/scripts/install_pre_commit_hook.sh b/scripts/install_pre_commit_hook.sh new file mode 100755 index 0000000..d33413a --- /dev/null +++ b/scripts/install_pre_commit_hook.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# Install pre-commit hook that runs pyright + pytest before every commit. +# Usage: bash scripts/install_pre_commit_hook.sh + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +HOOK_PATH="$REPO_ROOT/.git/hooks/pre-commit" + +cat > "$HOOK_PATH" << 'HOOK' +#!/usr/bin/env bash +# Pre-commit hook: run pyright + pytest before allowing commits. +# Installed by scripts/install_pre_commit_hook.sh + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +cd "$REPO_ROOT" + +# Activate venv if it exists +if [ -f ".venv/bin/activate" ]; then + source .venv/bin/activate +fi + +echo "=== pre-commit: pyright (advisory, skipped if low memory) ===" +if command -v pyright &> /dev/null; then + # pyright is a Node.js tool that OOMs on <2GB servers + MEM_MB=$(awk '/MemTotal/ {printf "%d", $2/1024}' /proc/meminfo 2>/dev/null || echo 0) + if [ "$MEM_MB" -gt 2000 ]; then + pyright open_strix/ || echo "⚠️ pyright reported errors (advisory, not blocking)" + else + echo "⚠️ ${MEM_MB}MB RAM — skipping pyright (needs >2GB)" + fi +else + echo "⚠️ pyright not installed, skipping type check" +fi + +echo "=== pre-commit: pytest ===" +# --ignore tests that require external tools (uv) or have pre-existing failures +python -m pytest tests/ -x -q \ + --ignore=tests/test_onboarding_flow.py \ + --ignore=tests/test_tools_registration.py \ + || { + echo "❌ tests failed. Fix failing tests before committing." + exit 1 +} + +echo "✅ pre-commit checks passed" +HOOK + +chmod +x "$HOOK_PATH" +echo "✅ Pre-commit hook installed at $HOOK_PATH" diff --git a/tests/test_config_folders.py b/tests/test_config_folders.py index ac98efa..d655232 100644 --- a/tests/test_config_folders.py +++ b/tests/test_config_folders.py @@ -16,7 +16,7 @@ bootstrap_home_repo, load_config, ) -from open_strix.app import WriteGuardBackend +from open_strix.readonly_backend import WriteGuardBackend from open_strix.prompts import render_folders_section diff --git a/tests/test_logging_backend.py b/tests/test_logging_backend.py new file mode 100644 index 0000000..292c0d5 --- /dev/null +++ b/tests/test_logging_backend.py @@ -0,0 +1,184 @@ +"""Conformance tests: LoggingWriteGuardBackend called through CompositeBackend. + +These tests ensure that LoggingWriteGuardBackend implements every method with +the exact positional argument signatures that CompositeBackend uses. The bug +that killed Keel (PR #55) was **kwargs catching only keyword args while +CompositeBackend passes 3 positional args to grep_raw/agrep_raw. + +If a signature mismatch exists, these tests explode with TypeError immediately. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path + +import pytest +from deepagents.backends.composite import CompositeBackend + +from open_strix.readonly_backend import LoggingWriteGuardBackend + + +@pytest.fixture +def tmp_project(tmp_path: Path) -> Path: + """Create a minimal project directory with files to search.""" + (tmp_path / "state").mkdir() + (tmp_path / "state" / "test.md").write_text("hello world\nfoo bar\n") + (tmp_path / "skills").mkdir() + (tmp_path / "skills" / "example.py").write_text("import os\n") + return tmp_path + + +@pytest.fixture +def events_log(tmp_path: Path) -> str: + return str(tmp_path / "events.jsonl") + + +@pytest.fixture +def logging_backend(tmp_project: Path, events_log: str) -> LoggingWriteGuardBackend: + return LoggingWriteGuardBackend( + root_dir=tmp_project, + writable_dirs=["state", "skills"], + events_log_path=events_log, + session_id="test-session", + ) + + +@pytest.fixture +def composite(logging_backend: LoggingWriteGuardBackend) -> CompositeBackend: + """CompositeBackend with LoggingWriteGuardBackend as the default backend. + + This is the exact wiring that open-strix uses in app.py. + """ + return CompositeBackend(default=logging_backend, routes={}) + + +class TestGrepConformance: + """Verify grep_raw/agrep_raw work when called through CompositeBackend. + + CompositeBackend calls backend.grep_raw(pattern, path, glob) with + 3 positional args. If the backend uses **kwargs, this explodes. + """ + + def test_grep_raw_3_positional_args(self, composite: CompositeBackend) -> None: + """The exact call pattern from CompositeBackend line 260.""" + result = composite.grep_raw("hello", "/", None) + assert not isinstance(result, str), f"grep_raw returned error: {result}" + + def test_grep_raw_with_path(self, composite: CompositeBackend) -> None: + result = composite.grep_raw("hello", "/state/", None) + assert not isinstance(result, str), f"grep_raw returned error: {result}" + + def test_grep_raw_with_glob(self, composite: CompositeBackend) -> None: + result = composite.grep_raw("hello", "/", "*.md") + assert not isinstance(result, str), f"grep_raw returned error: {result}" + + def test_grep_raw_no_path(self, composite: CompositeBackend) -> None: + """None path triggers the 'search all backends' branch.""" + result = composite.grep_raw("hello", None, None) + assert not isinstance(result, str), f"grep_raw returned error: {result}" + + def test_agrep_raw_3_positional_args(self, composite: CompositeBackend) -> None: + """The exact call pattern from CompositeBackend line 291.""" + result = asyncio.get_event_loop().run_until_complete( + composite.agrep_raw("hello", "/", None) + ) + assert not isinstance(result, str), f"agrep_raw returned error: {result}" + + def test_agrep_raw_with_path(self, composite: CompositeBackend) -> None: + result = asyncio.get_event_loop().run_until_complete( + composite.agrep_raw("hello", "/state/", None) + ) + assert not isinstance(result, str), f"agrep_raw returned error: {result}" + + +class TestReadConformance: + """Verify read/aread work through CompositeBackend.""" + + def test_read_through_composite(self, composite: CompositeBackend) -> None: + result = composite.read("/state/test.md") + assert "hello world" in result + + def test_aread_through_composite(self, composite: CompositeBackend) -> None: + result = asyncio.get_event_loop().run_until_complete( + composite.aread("/state/test.md") + ) + assert "hello world" in result + + +class TestLsConformance: + """Verify ls_info/als_info work through CompositeBackend.""" + + def test_ls_root(self, composite: CompositeBackend) -> None: + result = composite.ls_info("/") + assert isinstance(result, list) + + def test_als_root(self, composite: CompositeBackend) -> None: + result = asyncio.get_event_loop().run_until_complete( + composite.als_info("/") + ) + assert isinstance(result, list) + + +class TestGlobConformance: + """Verify glob_info/aglob_info work through CompositeBackend.""" + + def test_glob_through_composite(self, composite: CompositeBackend) -> None: + result = composite.glob_info("*.md", "/") + assert isinstance(result, list) + + def test_aglob_through_composite(self, composite: CompositeBackend) -> None: + result = asyncio.get_event_loop().run_until_complete( + composite.aglob_info("*.md", "/") + ) + assert isinstance(result, list) + + +class TestWriteConformance: + """Verify write/edit work through CompositeBackend.""" + + def test_write_through_composite(self, composite: CompositeBackend) -> None: + result = composite.write("/state/new.md", "content") + assert result.error is None + + def test_edit_through_composite(self, composite: CompositeBackend) -> None: + result = composite.edit("/state/test.md", "hello", "goodbye") + assert result.error is None + + +class TestExecuteConformance: + """Verify execute works through CompositeBackend. + + Note: LoggingWriteGuardBackend doesn't implement SandboxBackendProtocol, + so execute raises NotImplementedError. This is expected — execute is + handled by the FilesystemBackend directly in production, not through + the write guard layer. + """ + + def test_execute_not_supported(self, composite: CompositeBackend) -> None: + with pytest.raises(NotImplementedError): + composite.execute("echo hi") + + +class TestEventLogging: + """Verify that events are logged correctly through the composite path.""" + + def test_grep_logs_event( + self, composite: CompositeBackend, events_log: str + ) -> None: + composite.grep_raw("hello", "/", None) + with open(events_log) as f: + events = [json.loads(line) for line in f] + grep_events = [e for e in events if e.get("tool") == "grep"] + assert len(grep_events) >= 1 + assert grep_events[0]["args"]["pattern"] == "hello" + + def test_read_logs_event( + self, composite: CompositeBackend, events_log: str + ) -> None: + composite.read("/state/test.md") + with open(events_log) as f: + events = [json.loads(line) for line in f] + read_events = [e for e in events if e.get("tool") == "read_file"] + assert len(read_events) >= 1 diff --git a/tests/test_write_guard.py b/tests/test_write_guard.py index 6fc8472..961d30e 100644 --- a/tests/test_write_guard.py +++ b/tests/test_write_guard.py @@ -7,7 +7,7 @@ import pytest -from open_strix.app import WriteGuardBackend +from open_strix.readonly_backend import WriteGuardBackend @pytest.fixture From e986eb5c6236c8c9d09fa2ac7c52970dd0965703 Mon Sep 17 00:00:00 2001 From: "Strix (Claude Opus 4.6)" Date: Mon, 23 Mar 2026 15:46:39 +0000 Subject: [PATCH 2/2] Use uv run for pyright and pytest in pre-commit hook Per Tim's review: uv run is cleaner than activating venv directly. Removes venv activation, uses uv run pyright and uv run pytest instead. Adds PATH fix for ~/.local/bin since git hooks don't inherit full shell env. Co-Authored-By: Claude Opus 4.6 --- scripts/install_pre_commit_hook.sh | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/scripts/install_pre_commit_hook.sh b/scripts/install_pre_commit_hook.sh index d33413a..37b429a 100755 --- a/scripts/install_pre_commit_hook.sh +++ b/scripts/install_pre_commit_hook.sh @@ -17,27 +17,21 @@ set -euo pipefail REPO_ROOT="$(cd "$(dirname "$0")/../.." && pwd)" cd "$REPO_ROOT" -# Activate venv if it exists -if [ -f ".venv/bin/activate" ]; then - source .venv/bin/activate -fi +# Ensure uv is on PATH (may not be in git hook environment) +export PATH="$HOME/.local/bin:$PATH" echo "=== pre-commit: pyright (advisory, skipped if low memory) ===" -if command -v pyright &> /dev/null; then - # pyright is a Node.js tool that OOMs on <2GB servers - MEM_MB=$(awk '/MemTotal/ {printf "%d", $2/1024}' /proc/meminfo 2>/dev/null || echo 0) - if [ "$MEM_MB" -gt 2000 ]; then - pyright open_strix/ || echo "⚠️ pyright reported errors (advisory, not blocking)" - else - echo "⚠️ ${MEM_MB}MB RAM — skipping pyright (needs >2GB)" - fi +# pyright is a Node.js tool that OOMs on <2GB servers +MEM_MB=$(awk '/MemTotal/ {printf "%d", $2/1024}' /proc/meminfo 2>/dev/null || echo 0) +if [ "$MEM_MB" -gt 2000 ]; then + uv run pyright open_strix/ || echo "⚠️ pyright reported errors (advisory, not blocking)" else - echo "⚠️ pyright not installed, skipping type check" + echo "⚠️ ${MEM_MB}MB RAM — skipping pyright (needs >2GB)" fi echo "=== pre-commit: pytest ===" # --ignore tests that require external tools (uv) or have pre-existing failures -python -m pytest tests/ -x -q \ +uv run pytest tests/ -x -q \ --ignore=tests/test_onboarding_flow.py \ --ignore=tests/test_tools_registration.py \ || {