Skip to content

Commit 0f4aa48

Browse files
committed
fix: address PR review findings from bot and code review
- Check ~/.claude/pilot/settings.json in _get_forced_claude_version fallback - Add --cov=pilot.hooks to CI pytest coverage tracking - Extract duplicated _run_with_input helper to module-level function - Increase pre-commit tail output from 5 to 20 lines - Add missing "g r" key assertion in search-removal test
1 parent b3c1e6f commit 0f4aa48

5 files changed

Lines changed: 46 additions & 53 deletions

File tree

.githooks/pre-commit

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,13 @@ INSTALLER_CHANGED=$(git diff --cached --name-only -- 'installer/' | head -1)
1414

1515
if [ -n "$LAUNCHER_CHANGED" ]; then
1616
echo "[pre-commit] Python source changed — running unit tests..."
17-
uv run pytest launcher/tests/ pilot/hooks/tests/ -q --tb=short 2>&1 | tail -5
17+
uv run pytest launcher/tests/ pilot/hooks/tests/ -q --tb=short 2>&1 | tail -20
1818
echo "[pre-commit] Python unit tests passed."
1919
fi
2020

2121
if [ -n "$INSTALLER_CHANGED" ]; then
2222
echo "[pre-commit] Installer changed — running installer unit tests..."
23-
uv run pytest installer/tests/unit/ -q --tb=short 2>&1 | tail -5
23+
uv run pytest installer/tests/unit/ -q --tb=short 2>&1 | tail -20
2424
echo "[pre-commit] Installer unit tests passed."
2525
fi
2626

@@ -35,12 +35,12 @@ if [ -n "$CONSOLE_CHANGED" ]; then
3535
fi
3636

3737
echo "[pre-commit] Console source changed — running unit tests..."
38-
(cd console && bun test 2>&1) | tail -5
38+
(cd console && bun test 2>&1) | tail -20
3939
echo "[pre-commit] Console unit tests passed."
4040

4141
# --- 3. Console typecheck + build + stage artifacts ---
4242
echo "[pre-commit] Running typecheck..."
43-
(cd console && bun run typecheck 2>&1) | tail -5
43+
(cd console && bun run typecheck 2>&1) | tail -20
4444
echo "[pre-commit] Console typecheck passed."
4545

4646
echo "[pre-commit] Rebuilding console artifacts..."

.github/workflows/release.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ jobs:
134134
- name: Run unit tests with coverage
135135
run: |
136136
python3 -m pytest installer/tests/unit/ launcher/tests/unit/ pilot/hooks/tests/ -v \
137-
--cov=installer --cov=launcher \
137+
--cov=installer --cov=launcher --cov=pilot.hooks \
138138
--cov-report=term --cov-report=xml
139139
140140
console-tests:

console/tests/ui/search-removal.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ describe("Search view removal", () => {
5353
);
5454
expect(source).not.toContain("Go to Search");
5555
expect(source).not.toContain("navigate:/search");
56+
expect(source).not.toContain('"g r"');
5657
});
5758

5859
it("Dashboard renders 4 workspace cards including VexorStatus", () => {

installer/steps/dependencies.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,14 +93,20 @@ def install_python_tools() -> bool:
9393

9494

9595
def _get_forced_claude_version() -> str | None:
96-
"""Check ~/.claude/settings.json for FORCE_CLAUDE_VERSION in env section."""
97-
settings_path = Path.home() / ".claude" / "settings.json"
98-
if settings_path.exists():
99-
try:
100-
settings = json.loads(settings_path.read_text())
101-
return settings.get("env", {}).get("FORCE_CLAUDE_VERSION")
102-
except (json.JSONDecodeError, OSError):
103-
pass
96+
"""Check settings files for FORCE_CLAUDE_VERSION in env section."""
97+
paths = [
98+
Path.home() / ".claude" / "pilot" / "settings.json",
99+
Path.home() / ".claude" / "settings.json",
100+
]
101+
for settings_path in paths:
102+
if settings_path.exists():
103+
try:
104+
settings = json.loads(settings_path.read_text())
105+
version = settings.get("env", {}).get("FORCE_CLAUDE_VERSION")
106+
if version:
107+
return version
108+
except (json.JSONDecodeError, OSError):
109+
pass
104110
return None
105111

106112

pilot/hooks/tests/test_tool_redirect.py

Lines changed: 26 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -82,101 +82,87 @@ def test_plain_word_not_semantic(self):
8282
assert is_semantic_pattern("config") is False
8383

8484

85+
def _run_with_input(tool_name: str, tool_input: dict | None = None) -> int:
86+
"""Simulate hook invocation with the given tool name and optional input."""
87+
hook_data = {"tool_name": tool_name}
88+
if tool_input is not None:
89+
hook_data["tool_input"] = tool_input
90+
stdin = StringIO(json.dumps(hook_data))
91+
with patch("sys.stdin", stdin):
92+
return run_tool_redirect()
93+
94+
8595
class TestBlockedTools:
8696
"""Tests for tools that should be blocked (exit code 2)."""
8797

88-
def _run_with_input(self, tool_name: str, tool_input: dict | None = None) -> int:
89-
hook_data = {"tool_name": tool_name}
90-
if tool_input is not None:
91-
hook_data["tool_input"] = tool_input
92-
stdin = StringIO(json.dumps(hook_data))
93-
with patch("sys.stdin", stdin):
94-
return run_tool_redirect()
95-
9698
def test_blocks_web_search(self):
97-
result = self._run_with_input("WebSearch", {"query": "python tutorial"})
99+
result = _run_with_input("WebSearch", {"query": "python tutorial"})
98100
assert result == 2
99101

100102
def test_blocks_web_fetch(self):
101-
result = self._run_with_input("WebFetch", {"url": "https://example.com"})
103+
result = _run_with_input("WebFetch", {"url": "https://example.com"})
102104
assert result == 2
103105

104106
def test_blocks_enter_plan_mode(self):
105-
result = self._run_with_input("EnterPlanMode")
107+
result = _run_with_input("EnterPlanMode")
106108
assert result == 2
107109

108110
def test_blocks_exit_plan_mode(self):
109-
result = self._run_with_input("ExitPlanMode")
111+
result = _run_with_input("ExitPlanMode")
110112
assert result == 2
111113

112114

113115
class TestHintedTools:
114116
"""Tests for tools that get hints but are allowed (exit code 0)."""
115117

116-
def _run_with_input(self, tool_name: str, tool_input: dict | None = None) -> int:
117-
hook_data = {"tool_name": tool_name}
118-
if tool_input is not None:
119-
hook_data["tool_input"] = tool_input
120-
stdin = StringIO(json.dumps(hook_data))
121-
with patch("sys.stdin", stdin):
122-
return run_tool_redirect()
123-
124118
def test_hints_grep_with_semantic_pattern(self):
125-
result = self._run_with_input("Grep", {"pattern": "where is config loaded"})
119+
result = _run_with_input("Grep", {"pattern": "where is config loaded"})
126120
assert result == 0
127121

128122
def test_no_hint_grep_with_code_pattern(self):
129-
result = self._run_with_input("Grep", {"pattern": "def save_config"})
123+
result = _run_with_input("Grep", {"pattern": "def save_config"})
130124
assert result == 0
131125

132126
def test_hints_task_explore(self):
133-
result = self._run_with_input("Task", {"subagent_type": "Explore"})
127+
result = _run_with_input("Task", {"subagent_type": "Explore"})
134128
assert result == 0
135129

136130
def test_hints_task_generic_subagent(self):
137131
"""Non-allowed subagent types get a hint."""
138-
result = self._run_with_input("Task", {"subagent_type": "general-purpose"})
132+
result = _run_with_input("Task", {"subagent_type": "general-purpose"})
139133
assert result == 0
140134

141135
def test_no_hint_task_spec_reviewer(self):
142136
"""Spec reviewer sub-agents should be allowed without hints."""
143-
result = self._run_with_input("Task", {"subagent_type": "pilot:spec-reviewer-compliance"})
137+
result = _run_with_input("Task", {"subagent_type": "pilot:spec-reviewer-compliance"})
144138
assert result == 0
145139

146140
def test_no_hint_task_plan_verifier(self):
147-
result = self._run_with_input("Task", {"subagent_type": "pilot:plan-verifier"})
141+
result = _run_with_input("Task", {"subagent_type": "pilot:plan-verifier"})
148142
assert result == 0
149143

150144
def test_no_hint_task_plan_challenger(self):
151-
result = self._run_with_input("Task", {"subagent_type": "pilot:plan-challenger"})
145+
result = _run_with_input("Task", {"subagent_type": "pilot:plan-challenger"})
152146
assert result == 0
153147

154148

155149
class TestAllowedTools:
156150
"""Tests for tools that should pass through without blocks or hints."""
157151

158-
def _run_with_input(self, tool_name: str, tool_input: dict | None = None) -> int:
159-
hook_data = {"tool_name": tool_name}
160-
if tool_input is not None:
161-
hook_data["tool_input"] = tool_input
162-
stdin = StringIO(json.dumps(hook_data))
163-
with patch("sys.stdin", stdin):
164-
return run_tool_redirect()
165-
166152
def test_allows_read(self):
167-
assert self._run_with_input("Read", {"file_path": "/foo.py"}) == 0
153+
assert _run_with_input("Read", {"file_path": "/foo.py"}) == 0
168154

169155
def test_allows_write(self):
170-
assert self._run_with_input("Write", {"file_path": "/foo.py"}) == 0
156+
assert _run_with_input("Write", {"file_path": "/foo.py"}) == 0
171157

172158
def test_allows_edit(self):
173-
assert self._run_with_input("Edit", {"file_path": "/foo.py"}) == 0
159+
assert _run_with_input("Edit", {"file_path": "/foo.py"}) == 0
174160

175161
def test_allows_bash(self):
176-
assert self._run_with_input("Bash", {"command": "ls"}) == 0
162+
assert _run_with_input("Bash", {"command": "ls"}) == 0
177163

178164
def test_allows_task_create(self):
179-
assert self._run_with_input("TaskCreate", {"subject": "test"}) == 0
165+
assert _run_with_input("TaskCreate", {"subject": "test"}) == 0
180166

181167

182168
class TestEdgeCases:

0 commit comments

Comments
 (0)