fix: fix binary inclusion of hook scripts + test

[lbeurerkellner]

No description provided.

[qodo-merge-etso]

Review Summary by Qodo

Fix hook scripts inclusion in PyInstaller binary and add E2E tests

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Fix binary inclusion of hook scripts via PyInstaller --add-data flag
• Add E2E tests verifying hook scripts accessible in binary
• Test both Claude and Cursor guard install configurations

Diagram

flowchart LR
  A["PyInstaller Build"] -->|"--add-data flag"| B["Bundle hook scripts"]
  B --> C["Binary includes hooks"]
  C --> D["E2E Tests"]
  D -->|"Verify hooks"| E["Claude/Cursor configs"]

Loading

File Changes

1. Makefile 🐞 Bug fix +1/-1

Include hook scripts in PyInstaller binary

• Added --add-data 'src/agent_scan/hooks:agent_scan/hooks' to PyInstaller commands
• Applied to both signed and unsigned binary build paths
• Ensures hook scripts are bundled in the final binary

Makefile

---

2. tests/e2e/test_guard_install.py 🧪 Tests +96/-0

E2E tests for guard install hook scripts

• New E2E test file for guard install functionality
• Includes fake HTTP server fixture for hook event handshake
• Tests Claude and Cursor guard install with hook configuration validation
• Verifies hook scripts are accessible from both uv and binary execution paths

tests/e2e/test_guard_install.py

---

[qodo-merge-etso]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. Windows add-data separator 🐞 Bug ☼ Reliability

Description

make ci runs make binary on Windows CI, but the Makefile uses Unix-style --add-data SRC:DEST;
PyInstaller expects Windows SRC;DEST, so the binary build will fail on windows-latest and block
tests/releases.

Code

Makefile[45]

+	if [ -n "${APPLE_SIGNING_IDENTITY}" ]; then uv run pyinstaller --onefile --name agent-scan --add-data 'src/agent_scan/hooks:agent_scan/hooks' src/agent_scan/run.py --codesign-identity "${APPLE_SIGNING_IDENTITY}"; else uv run pyinstaller --onefile --name agent-scan --add-data 'src/agent_scan/hooks:agent_scan/hooks' src/agent_scan/run.py; fi

Evidence

The CI workflow runs make ci on a matrix including Windows; make ci invokes the binary target,
whose PyInstaller command now uses --add-data 'src/agent_scan/hooks:agent_scan/hooks' (colon
separator). On Windows, PyInstaller’s --add-data separator must be os.pathsep (semicolon), so
this invocation is not portable and will fail during Windows binary build.

Makefile[26-29]
Makefile[42-45]
.github/workflows/tests.yml[19-40]
Best Practice: PyInstaller documentation (Adding data files / --add-data uses os.pathsep: '
Best Practice: ' on Windows, ':' on Unix)

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Makefile uses `--add-data 'src/agent_scan/hooks:agent_scan/hooks'`, which is Unix-only. Since CI runs `make ci` on Windows, the Windows job will fail building the binary because PyInstaller expects `SRC;DEST` on Windows.

### Issue Context
`make ci` invokes `make binary`, so the Makefile’s PyInstaller invocation must be cross-platform.

### Fix Focus Areas
- Makefile[26-29]
- Makefile[42-45]

### Suggested fix
Update the `binary` target to compute the correct separator at runtime (e.g., via Python `os.pathsep`) and pass it unquoted in a double-quoted `--add-data` string, e.g.:

```make
binary:
\tuv sync
\tuv pip install -e .[dev]
\tSEP=$$(uv run python -c "import os; print(os.pathsep)"); \
\tif [ -n "${APPLE_SIGNING_IDENTITY}" ]; then \
\t  uv run pyinstaller --onefile --name agent-scan --add-data "src/agent_scan/hooks$${SEP}agent_scan/hooks" src/agent_scan/run.py --codesign-identity "${APPLE_SIGNING_IDENTITY}"; \
\telse \
\t  uv run pyinstaller --onefile --name agent-scan --add-data "src/agent_scan/hooks$${SEP}agent_scan/hooks" src/agent_scan/run.py; \
\tfi
```

(Any equivalent OS-conditional solution is fine as long as Windows gets `;` and Unix gets `:`.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Duplicate subprocess.run install tests 📘 Rule violation ⚙ Maintainability

Description

The E2E tests for claude and cursor repeat nearly identical subprocess.run(...) setup and
assertions, increasing maintenance burden and divergence risk. This should be refactored into a
shared helper or a single parametrized test over target/expected hook keys.

Code

tests/e2e/test_guard_install.py[R44-96]

+    @pytest.mark.parametrize("agent_scan_cmd", ["uv", "binary"], indirect=True)
+    def test_guard_install_claude(self, agent_scan_cmd, tmp_path, fake_hook_server):
+        config_file = tmp_path / "settings.json"
+        result = subprocess.run(
+            [
+                *agent_scan_cmd,
+                "guard",
+                "install",
+                "claude",
+                "--file",
+                str(config_file),
+                "--url",
+                fake_hook_server,
+            ],
+            capture_output=True,
+            text=True,
+            timeout=30,
+            env={**os.environ, "PUSH_KEY": "test-pk-e2e"},
+        )
+        assert result.returncode == 0, f"guard install failed:\nstdout: {result.stdout}\nstderr: {result.stderr}"
+
+        # The config file should exist and contain valid JSON with hooks
+        settings = json.loads(config_file.read_text())
+        assert "hooks" in settings
+        # Should have entries for standard Claude hook events
+        assert "PreToolUse" in settings["hooks"]
+        assert "Stop" in settings["hooks"]
+
+    @pytest.mark.parametrize("agent_scan_cmd", ["uv", "binary"], indirect=True)
+    def test_guard_install_cursor(self, agent_scan_cmd, tmp_path, fake_hook_server):
+        config_file = tmp_path / "hooks.json"
+        result = subprocess.run(
+            [
+                *agent_scan_cmd,
+                "guard",
+                "install",
+                "cursor",
+                "--file",
+                str(config_file),
+                "--url",
+                fake_hook_server,
+            ],
+            capture_output=True,
+            text=True,
+            timeout=30,
+            env={**os.environ, "PUSH_KEY": "test-pk-e2e"},
+        )
+        assert result.returncode == 0, f"guard install failed:\nstdout: {result.stdout}\nstderr: {result.stderr}"
+
+        data = json.loads(config_file.read_text())
+        assert "hooks" in data
+        assert "preToolUse" in data["hooks"]
+        assert "stop" in data["hooks"]

Evidence

PR Compliance ID 5 requires avoiding copy-pasted logic and reusing shared utilities; the added tests
contain two near-identical blocks for guard install that differ only by a few literals (target name,
output filename, and expected hook keys).

Rule 5: Avoid duplication (DRY)—reuse utilities instead of copy-pasting
tests/e2e/test_guard_install.py[44-96]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Two E2E tests (`test_guard_install_claude` and `test_guard_install_cursor`) duplicate almost the same `subprocess.run(...)` invocation and post-run JSON assertions.

## Issue Context
This repetition increases future maintenance cost and can cause the tests to diverge when updating flags/env/expected structure.

## Fix Focus Areas
- tests/e2e/test_guard_install.py[44-96]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Hardcoded timeout=30 values 📘 Rule violation ⚙ Maintainability

Description

The tests use the magic number 30 for subprocess.run(..., timeout=30) in multiple places without
a named constant. This reduces clarity and makes it harder to adjust test timing consistently.

Code

tests/e2e/test_guard_install.py[R58-60]

+            capture_output=True,
+            text=True,
+            timeout=30,

Evidence

PR Compliance ID 40 requires replacing unexplained numeric literals with named constants;
timeout=30 is repeated and not self-documenting.

Rule 40: Replace magic numbers with named constants
tests/e2e/test_guard_install.py[58-61]
tests/e2e/test_guard_install.py[86-90]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`subprocess.run(..., timeout=30)` is used with a hardcoded numeric literal in multiple places.

## Issue Context
A named constant (e.g., `E2E_TIMEOUT_S = 30`) improves readability and makes future adjustments straightforward.

## Fix Focus Areas
- tests/e2e/test_guard_install.py[58-61]
- tests/e2e/test_guard_install.py[86-90]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. HTTPServer not closed 🐞 Bug ☼ Reliability

Description

fake_hook_server only calls server.shutdown() and never closes the listening socket or joins the
server thread, which can leak resources and cause intermittent E2E flakiness (especially on
Windows).

Code

tests/e2e/test_guard_install.py[R27-35]

+@pytest.fixture()
+def fake_hook_server():
+    server = HTTPServer(("127.0.0.1", 0), _FakeHookServer)
+    port = server.server_address[1]
+    t = threading.Thread(target=server.serve_forever, daemon=True)
+    t.start()
+    yield f"http://127.0.0.1:{port}"
+    server.shutdown()
+

Evidence

The fixture starts HTTPServer in a background thread and yields, but cleanup only calls
server.shutdown(). Proper cleanup should also close the underlying socket via
server.server_close() and ideally join the thread to ensure deterministic teardown.

tests/e2e/test_guard_install.py[27-35]
Best Practice: Python stdlib docs for socketserver/HTTPServer (shutdown() stops serve_forever
Best Practice: server_close() releases the socket)

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `fake_hook_server` fixture shuts down the HTTP server loop but doesn’t close the server socket or join the background thread, which can leak resources and introduce test flakiness.

### Issue Context
The fixture is used by subprocess-based E2E tests; deterministic teardown reduces intermittent failures across platforms.

### Fix Focus Areas
- tests/e2e/test_guard_install.py[27-35]

### Suggested fix
Use `try/finally` and add `server.server_close()` plus a bounded `t.join(...)`:

```python
@pytest.fixture()
def fake_hook_server():
   server = HTTPServer(("127.0.0.1", 0), _FakeHookServer)
   port = server.server_address[1]
   t = threading.Thread(target=server.serve_forever)
   t.start()
   try:
       yield f"http://127.0.0.1:{port}"
   finally:
       server.shutdown()
       server.server_close()
       t.join(timeout=5)
```

(Keeping `daemon=True` is OK if you still close the socket, but joining is preferable.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Stale binary build helper 🐞 Bug ⚙ Maintainability

Description

The Makefile now includes hook scripts via --add-data, but the test helper _build_binary()
(documented as “same steps as make binary”) still uses a different PyInstaller invocation, so if
it’s used it will build a binary missing bundled hooks.

Code

Makefile[45]

+	if [ -n "${APPLE_SIGNING_IDENTITY}" ]; then uv run pyinstaller --onefile --name agent-scan --add-data 'src/agent_scan/hooks:agent_scan/hooks' src/agent_scan/run.py --codesign-identity "${APPLE_SIGNING_IDENTITY}"; else uv run pyinstaller --onefile --name agent-scan --add-data 'src/agent_scan/hooks:agent_scan/hooks' src/agent_scan/run.py; fi

Evidence

The PR changed the canonical binary build command in the Makefile to include hooks data, but the
repository still contains a helper intended to replicate that build which does not include the new
flag and even uses a different --name. This is a drift hazard for anyone relying on that helper
for local/Windows builds without make.

Makefile[42-45]
tests/conftest.py[58-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`tests/conftest.py:_build_binary()` claims to run the same steps as `make binary`, but it does not include the newly-added `--add-data` hook bundling (and uses a different binary name). This can mislead developers and produce a binary that fails `guard install` in the same way this PR is trying to prevent.

### Issue Context
Even if CI doesn’t call `_build_binary()` today, the helper’s intent is explicitly to mirror `make binary`.

### Fix Focus Areas
- Makefile[42-45]
- tests/conftest.py[58-66]

### Suggested fix
Either:
- Update `_build_binary()` to exactly match the Makefile’s PyInstaller flags (including the OS-correct `--add-data` separator and `--name agent-scan`), or
- Remove `_build_binary()` if it’s truly unused, to avoid drift.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more