Skip to content

Commit ee253b5

Browse files
committed
fix: resolve all code review issues
Fixes: - Remove bogus import in harness/_take_screenshot - Use tuple-style isinstance() for Python compat - Fix duplicate method extraction (ast.walk -> ast.iter_child_nodes) - Filter __init__ from GUI action mapping - Use __ double-underscore as MCP tool name separator - Add informative messages for empty Phase 7/8 New: - tests/test_analyzer.py (12 tests) - tests/test_mapper.py (10 tests) - tests/test_pipeline.py (10 tests) - .github/workflows/ci.yml (Ruff + MyPy + pytest on 3.11-3.13) All 32 tests pass.
1 parent 7cfc34a commit ee253b5

9 files changed

Lines changed: 367 additions & 11 deletions

File tree

.github/workflows/ci.yml

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
name: CI
2+
3+
on:
4+
push:
5+
branches: [master, main]
6+
pull_request:
7+
branches: [master, main]
8+
9+
jobs:
10+
lint-and-test:
11+
runs-on: ubuntu-latest
12+
strategy:
13+
matrix:
14+
python-version: ["3.11", "3.12", "3.13"]
15+
16+
steps:
17+
- uses: actions/checkout@v4
18+
19+
- name: Set up Python ${{ matrix.python-version }}
20+
uses: actions/setup-python@v5
21+
with:
22+
python-version: ${{ matrix.python-version }}
23+
24+
- name: Install dependencies
25+
run: |
26+
pip install -e ".[dev]"
27+
28+
- name: Lint with Ruff
29+
run: |
30+
ruff check gui_anything/ tests/
31+
ruff format --check gui_anything/ tests/
32+
33+
- name: Type check with MyPy
34+
run: |
35+
mypy gui_anything/ --ignore-missing-imports
36+
continue-on-error: true
37+
38+
- name: Run tests
39+
run: |
40+
pytest tests/ -v --tb=short

.gitignore

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,4 @@ htmlcov/
4545
output/
4646
generated/
4747
test_output/
48-
test_pipeline.py
48+
/test_pipeline.py

gui_anything/analyzer/__init__.py

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -251,8 +251,9 @@ def _analyze_python(self, content: str, file_path: str, result: AnalysisResult)
251251
except SyntaxError:
252252
return
253253

254-
for node in ast.walk(tree):
255-
if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef):
254+
# Only iterate top-level statements to avoid double-counting class methods
255+
for node in ast.iter_child_nodes(tree):
256+
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
256257
func_info = self._extract_python_function(node, file_path)
257258
if func_info:
258259
result.functions.append(func_info)
@@ -269,8 +270,8 @@ def _analyze_python(self, content: str, file_path: str, result: AnalysisResult)
269270
def _extract_python_function(self, node: ast.FunctionDef | ast.AsyncFunctionDef,
270271
file_path: str) -> FunctionInfo | None:
271272
"""Extract function info from Python AST node."""
272-
# Skip private/dunder methods (except __init__)
273-
if node.name.startswith("_") and node.name != "__init__":
273+
# Skip private/dunder methods entirely (including __init__)
274+
if node.name.startswith("_"):
274275
return None
275276

276277
params: list[ParameterInfo] = []
@@ -318,7 +319,7 @@ def _extract_python_class(self, node: ast.ClassDef, file_path: str) -> ClassInfo
318319
"""Extract class info from Python AST node."""
319320
methods: list[FunctionInfo] = []
320321
for item in node.body:
321-
if isinstance(item, ast.FunctionDef | ast.AsyncFunctionDef):
322+
if isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)):
322323
func = self._extract_python_function(item, file_path)
323324
if func:
324325
methods.append(func)

gui_anything/generator/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1251,7 +1251,7 @@ def _generate_mcp_tools(self, schema: GUISchema, mapping: MappingResult,
12511251
for group in mapping.groups:
12521252
for action in group.actions:
12531253
tool = {
1254-
"name": action.command_id.replace(".", "_"),
1254+
"name": action.command_id.replace(".", "__"),
12551255
"description": action.description,
12561256
"inputSchema": {
12571257
"type": "object",

gui_anything/harness/__init__.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ def execute_tool(self, tool_name: str, arguments: dict[str, Any]) -> dict[str, A
5959
return {"status": "navigated", "activeGroup": group}
6060

6161
# Find matching action in schema
62-
action_key = tool_name.replace("_", ".", 1) # Restore dotted ID
62+
# Tool names use "__" double-underscore as group separator
63+
action_key = tool_name.replace("__", ".", 1) # Restore dotted ID
6364
actions = self._schema.get("actions", {}) if self._schema else {}
6465

6566
if action_key in actions:
@@ -81,7 +82,6 @@ def execute_tool(self, tool_name: str, arguments: dict[str, Any]) -> dict[str, A
8182
def _take_screenshot(self) -> dict[str, Any]:
8283
"""Capture screenshot of the GUI."""
8384
try:
84-
from gui_anything.generator import Path as _ # noqa: F401
8585

8686
harness_path = self.project_path / "agent-harness" if self.project_path else None
8787
if harness_path and (harness_path / "screenshot.py").exists():

gui_anything/pipeline.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,9 @@ def _run_phase(self, phase_id: str) -> None:
149149
case "harness":
150150
self._generator.generate_harness(self._schema, self._mapping_result)
151151
case "test":
152-
pass # Phase 7: testing handled separately
152+
console.print("[dim] Validation deferred to post-pipeline step[/dim]")
153153
case "package":
154-
pass # Phase 8: packaging handled separately
154+
console.print("[dim] Packaging deferred — run 'npm run build' in output dir[/dim]")
155155

156156
def _print_summary(self, result: PipelineResult) -> None:
157157
"""Print pipeline execution summary."""

tests/test_analyzer.py

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
"""Unit tests for gui_anything.analyzer — Phase 1."""
2+
3+
from pathlib import Path
4+
from gui_anything.analyzer import CodebaseAnalyzer, AnalysisResult
5+
from gui_anything.pipeline import PipelineConfig
6+
7+
8+
CALC_DIR = Path(__file__).resolve().parent.parent / "examples" / "calculator"
9+
10+
11+
def _make_config(source: str = str(CALC_DIR)) -> PipelineConfig:
12+
return PipelineConfig(source=source)
13+
14+
15+
class TestCodebaseAnalyzer:
16+
"""Tests for the CodebaseAnalyzer on the example calculator."""
17+
18+
def test_detects_python_language(self) -> None:
19+
analyzer = CodebaseAnalyzer(_make_config())
20+
lang = analyzer._detect_language()
21+
assert lang == "python"
22+
23+
def test_collect_source_files(self) -> None:
24+
analyzer = CodebaseAnalyzer(_make_config())
25+
files = analyzer._collect_source_files("python")
26+
assert len(files) >= 1
27+
assert any("calculator.py" in str(f) for f in files)
28+
29+
def test_analyze_extracts_functions(self) -> None:
30+
result = CodebaseAnalyzer(_make_config()).analyze()
31+
names = [f.name for f in result.functions]
32+
assert "add" in names
33+
assert "subtract" in names
34+
assert "divide" in names
35+
36+
def test_no_dunder_methods_extracted(self) -> None:
37+
"""__init__ and __post_init__ should be filtered out."""
38+
result = CodebaseAnalyzer(_make_config()).analyze()
39+
names = [f.name for f in result.functions]
40+
assert "__init__" not in names
41+
assert "__post_init__" not in names
42+
43+
def test_no_duplicate_methods(self) -> None:
44+
"""Class methods should NOT be double-counted as top-level functions."""
45+
result = CodebaseAnalyzer(_make_config()).analyze()
46+
# The calculator has only top-level functions, no class methods
47+
# to appear as top-level. Verify no duplicates by name+file.
48+
seen = set()
49+
for f in result.functions:
50+
key = (f.name, f.file_path)
51+
assert key not in seen, f"Duplicate function: {key}"
52+
seen.add(key)
53+
54+
def test_data_model_detected(self) -> None:
55+
result = CodebaseAnalyzer(_make_config()).analyze()
56+
model_names = [m.name for m in result.data_models]
57+
assert "CalculatorState" in model_names
58+
59+
def test_function_parameters_parsed(self) -> None:
60+
result = CodebaseAnalyzer(_make_config()).analyze()
61+
add_fn = next(f for f in result.functions if f.name == "add")
62+
assert len(add_fn.parameters) == 2
63+
assert add_fn.parameters[0].name == "a"
64+
assert add_fn.parameters[0].type_hint == "float"
65+
66+
def test_function_defaults_detected(self) -> None:
67+
result = CodebaseAnalyzer(_make_config()).analyze()
68+
export_fn = next(f for f in result.functions if f.name == "export_history")
69+
fmt_param = next(p for p in export_fn.parameters if p.name == "format")
70+
assert fmt_param.required is False
71+
assert fmt_param.default == "'txt'"
72+
73+
def test_docstrings_extracted(self) -> None:
74+
result = CodebaseAnalyzer(_make_config()).analyze()
75+
add_fn = next(f for f in result.functions if f.name == "add")
76+
assert add_fn.docstring is not None
77+
assert "Add" in add_fn.docstring
78+
79+
def test_categorization(self) -> None:
80+
result = CodebaseAnalyzer(_make_config()).analyze()
81+
add_fn = next(f for f in result.functions if f.name == "add")
82+
assert add_fn.category == "create"
83+
get_fn = next(f for f in result.functions if f.name == "get_history")
84+
assert get_fn.category == "read"
85+
86+
def test_to_dict_serializable(self) -> None:
87+
result = CodebaseAnalyzer(_make_config()).analyze()
88+
d = result.to_dict()
89+
assert "functions" in d
90+
assert "classes" in d
91+
assert "data_models" in d
92+
import json
93+
json.dumps(d) # Should not raise
94+
95+
def test_unknown_language(self, tmp_path: Path) -> None:
96+
"""An empty directory should return 'unknown' language."""
97+
config = _make_config(str(tmp_path))
98+
analyzer = CodebaseAnalyzer(config)
99+
assert analyzer._detect_language() == "unknown"

tests/test_mapper.py

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
"""Unit tests for gui_anything.mapper — Phase 2."""
2+
3+
from pathlib import Path
4+
from gui_anything.analyzer import CodebaseAnalyzer
5+
from gui_anything.mapper import ActionMapper
6+
from gui_anything.pipeline import PipelineConfig
7+
8+
9+
CALC_DIR = Path(__file__).resolve().parent.parent / "examples" / "calculator"
10+
11+
12+
def _analyze():
13+
config = PipelineConfig(source=str(CALC_DIR))
14+
return config, CodebaseAnalyzer(config).analyze()
15+
16+
17+
class TestActionMapper:
18+
"""Tests for the ActionMapper."""
19+
20+
def test_maps_functions_to_actions(self) -> None:
21+
config, analysis = _analyze()
22+
mapping = ActionMapper(config).map(analysis)
23+
assert mapping.total_actions > 0
24+
25+
def test_groups_are_created(self) -> None:
26+
config, analysis = _analyze()
27+
mapping = ActionMapper(config).map(analysis)
28+
group_names = [g.name for g in mapping.groups]
29+
assert len(group_names) >= 2 # At least 2 categories
30+
31+
def test_action_has_params(self) -> None:
32+
config, analysis = _analyze()
33+
mapping = ActionMapper(config).map(analysis)
34+
all_actions = mapping.get_all_actions()
35+
add_action = next((a for a in all_actions if a.source_function == "add"), None)
36+
assert add_action is not None
37+
assert len(add_action.params) == 2
38+
39+
def test_param_type_inference(self) -> None:
40+
"""Parameters with float type_hint should map to 'number' widget."""
41+
config, analysis = _analyze()
42+
mapping = ActionMapper(config).map(analysis)
43+
all_actions = mapping.get_all_actions()
44+
add_action = next(a for a in all_actions if a.source_function == "add")
45+
assert add_action.params[0].type == "number"
46+
47+
def test_file_param_detection(self) -> None:
48+
"""'path' in parameter name should trigger file picker widget."""
49+
config, analysis = _analyze()
50+
mapping = ActionMapper(config).map(analysis)
51+
all_actions = mapping.get_all_actions()
52+
export_action = next(a for a in all_actions if a.source_function == "export_history")
53+
path_param = next(p for p in export_action.params if p.name == "path")
54+
assert path_param.type == "file"
55+
56+
def test_command_id_format(self) -> None:
57+
"""command_id should be 'group.action_name'."""
58+
config, analysis = _analyze()
59+
mapping = ActionMapper(config).map(analysis)
60+
all_actions = mapping.get_all_actions()
61+
for action in all_actions:
62+
assert "." in action.command_id
63+
64+
def test_delete_actions_require_confirmation(self) -> None:
65+
"""Delete-category actions should require confirmation."""
66+
config, analysis = _analyze()
67+
mapping = ActionMapper(config).map(analysis)
68+
all_actions = mapping.get_all_actions()
69+
delete_actions = [a for a in all_actions if "delete" in a.source_function.lower() or "clear" in a.source_function.lower()]
70+
for action in delete_actions:
71+
assert action.requires_confirmation
72+
73+
def test_name_to_label(self) -> None:
74+
assert ActionMapper._name_to_label("snake_case_name") == "Snake Case Name"
75+
assert ActionMapper._name_to_label("camelCaseName") == "Camel Case Name"
76+
77+
def test_to_dict_serializable(self) -> None:
78+
config, analysis = _analyze()
79+
mapping = ActionMapper(config).map(analysis)
80+
d = mapping.to_dict()
81+
assert "groups" in d
82+
assert "total_actions" in d
83+
import json
84+
json.dumps(d) # Should not raise
85+
86+
def test_short_names_filtered(self) -> None:
87+
"""Functions with names shorter than 3 chars should be skipped."""
88+
config, analysis = _analyze()
89+
mapping = ActionMapper(config).map(analysis)
90+
all_actions = mapping.get_all_actions()
91+
for action in all_actions:
92+
assert len(action.id) >= 3

0 commit comments

Comments
 (0)