-
Notifications
You must be signed in to change notification settings - Fork 1
fix(CODEWIKI-007): 22 review findings across 10 files #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2e05a14
29b973f
085b705
ab54808
0182eef
77cfcd5
89e5ccb
4104794
5016808
51b769f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,7 +84,7 @@ def create_test_config( | |
| Returns: | ||
| Config instance | ||
| """ | ||
| return Config( | ||
| return Config.from_args( | ||
| repo_path=repo_path, | ||
| output_dir=output_dir, | ||
| dependency_graph_dir=os.path.join(output_dir, "graphs"), | ||
|
Comment on lines
84
to
90
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test-multi-path/test_multi_path.py helper function In π€ Prompt for AI agentsfix confidence: π΄ 35 low β review closely β react π/π to teach the reviewer |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,11 +47,43 @@ def patched_call(*call_args, **call_kwargs): | |
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
| from codewiki.src.config import Config | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Config() instantiated directly at call site in test_clustering_debug.py, bypassing from_args/from_cli factories Replaced direct π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer |
||
|
|
||
| # Test repo | ||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
|
|
||
| # Create config | ||
| config = Config( | ||
| class TestResults: | ||
| """Accumulates test results and reports a summary with a non-zero exit on failure.""" | ||
|
|
||
| def __init__(self): | ||
| self.tests = [] | ||
|
|
||
| def add_test(self, name, passed, details=""): | ||
| self.tests.append((name, passed, details)) | ||
|
|
||
| def print_summary(self): | ||
| print("\n" + "=" * 80) | ||
| print("TEST SUMMARY") | ||
| print("=" * 80) | ||
| failed = 0 | ||
| for name, passed, details in self.tests: | ||
| status = "β PASS" if passed else "β FAIL" | ||
| print(f"{status}: {name}") | ||
| if details: | ||
| print(f" {details}") | ||
| if not passed: | ||
| failed += 1 | ||
| print("=" * 80) | ||
| print(f"Total: {len(self.tests)}, Passed: {len(self.tests) - failed}, Failed: {failed}") | ||
| return failed == 0 | ||
|
|
||
|
|
||
| results = TestResults() | ||
|
|
||
| # Test repo (portable: env var override, else a relative fixture path) | ||
| test_repo = os.getenv( | ||
| "CODEWIKI_TEST_REPO", | ||
| os.path.join(os.path.dirname(os.path.abspath(__file__)), "fixtures", "sample_repo") | ||
| ) | ||
|
|
||
| # Create config via factory (not direct construction) to satisfy validation/env-resolution | ||
| config = Config.from_args( | ||
| repo_path=test_repo, | ||
| output_dir="/tmp/codewiki_test", | ||
| dependency_graph_dir="/tmp/codewiki_test/deps", | ||
|
Comment on lines
47
to
89
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π test_clustering_debug.py hardcodes an absolute developer machine path instead of a portable test fixture Replaced the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer |
||
|
|
@@ -111,10 +143,17 @@ def patched_call(*call_args, **call_kwargs): | |
|
|
||
| # Show result | ||
| if len(module_tree) == 0: | ||
| print("\nβ FAILED: Empty module tree") | ||
| details = "Empty module tree" | ||
| if captured_response: | ||
| has_tags = "<GROUPED_COMPONENTS>" in captured_response | ||
| print(f" Has <GROUPED_COMPONENTS> tag: {has_tags}") | ||
| details += f"; Has <GROUPED_COMPONENTS> tag: {has_tags}" | ||
| results.add_test("clustering produces non-empty module tree", False, details) | ||
| else: | ||
| print(f"\nβ SUCCESS: {len(module_tree)} modules created") | ||
| print(json.dumps(module_tree, indent=2, default=str)) | ||
| results.add_test( | ||
| "clustering produces non-empty module tree", | ||
| True, | ||
| f"{len(module_tree)} modules created:\n{json.dumps(module_tree, indent=2, default=str)}" | ||
| ) | ||
|
|
||
| success = results.print_summary() | ||
| sys.exit(0 if success else 1) | ||
|
Comment on lines
143
to
+159
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π test_clustering_debug.py uses ad-hoc print-based assertions instead of the TestResults accumulator pattern Added a π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,27 @@ | |
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
| from codewiki.src.config import Config | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test_clustering_forced.py bypasses required factory methods Replaced direct π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
|
|
||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
|
|
||
| config = Config( | ||
| class TestResults: | ||
| def __init__(self): | ||
| self.tests = [] | ||
|
|
||
| def add_test(self, name, passed, message=""): | ||
| self.tests.append((name, passed, message)) | ||
|
|
||
| def print_summary(self): | ||
| print("\nπ TEST SUMMARY:\n") | ||
| for name, passed, message in self.tests: | ||
| status = "β PASS" if passed else "β FAIL" | ||
| print(f"{status}: {name}" + (f" - {message}" if message else "")) | ||
| return all(passed for _, passed, _ in self.tests) | ||
|
|
||
|
|
||
| results = TestResults() | ||
|
|
||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| config = Config.from_cli( | ||
| repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps", | ||
| docs_dir="/tmp/test/docs", max_depth=2, | ||
| main_model=os.getenv("MAIN_MODEL", "gpt-4o"), | ||
|
Comment on lines
16
to
42
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π sys.path.insert uses os.path.dirname(file) but hardcoded absolute repo path used for test_repo Replaced the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
|
|
@@ -59,15 +77,23 @@ | |
| print("\nπ RESULTS:\n") | ||
|
|
||
| if len(module_tree) == 0: | ||
| print("β FAILED: Empty module tree") | ||
| print(" This means LLM did NOT follow <GROUPED_COMPONENTS> tag format") | ||
| print(" Check logs above for 'Invalid LLM response format' or 'Invalid JSON'") | ||
| sys.exit(1) | ||
| results.add_test( | ||
| "clustering_produces_module_tree", False, | ||
| "Empty module tree - LLM did NOT follow <GROUPED_COMPONENTS> tag format" | ||
| ) | ||
| passed = results.print_summary() | ||
| sys.exit(0 if passed else 1) | ||
| else: | ||
| results.add_test( | ||
| "clustering_produces_module_tree", True, | ||
| f"{len(module_tree)} modules created" | ||
| ) | ||
| print(f"β SUCCESS: {len(module_tree)} modules created") | ||
| print("\nModules generated:") | ||
| for name, info in module_tree.items(): | ||
| comp_count = len(info.get('components', [])) | ||
| print(f" - {name}: {comp_count} components") | ||
| print("\nπ THE FIX WORKS! LLM followed the <GROUPED_COMPONENTS> tag format!") | ||
| sys.exit(0) | ||
| passed = results.print_summary() | ||
| sys.exit(0 if passed else 1) | ||
|
|
||
|
Comment on lines
77
to
+99
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π test_clustering_forced.py uses print/sys.exit ad-hoc pass/fail instead of TestResults accumulator Added a minimal π€ Prompt for AI agentsfix confidence: π‘ 65 medium β react π/π to teach the reviewer |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,27 +18,54 @@ | |
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
| from codewiki.src.config import Config | ||
|
|
||
| def test_clustering(): | ||
|
|
||
| class TestResults: | ||
| """Simple pass/fail accumulator for standalone integration test scripts.""" | ||
|
|
||
| def __init__(self): | ||
| self.results = [] | ||
|
|
||
| def add_test(self, name, passed, message=""): | ||
| self.results.append((name, passed, message)) | ||
|
|
||
| def print_summary(self): | ||
| print("\n" + "=" * 80) | ||
| print("π TEST SUMMARY") | ||
| print("=" * 80) | ||
| for name, passed, message in self.results: | ||
| status = "β PASS" if passed else "β FAIL" | ||
| print(f"{status} - {name}" + (f": {message}" if message else "")) | ||
| total = len(self.results) | ||
| passed_count = sum(1 for _, passed, _ in self.results if passed) | ||
| print(f"\n{passed_count}/{total} tests passed") | ||
| return passed_count == total | ||
|
|
||
| def test_clustering(results): | ||
| """Test clustering on a small sample to verify prompt fix.""" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Hardcoded absolute developer path leaked into committed test script Removed the hardcoded π€ Prompt for AI agentsfix confidence: π‘ 60 medium β react π/π to teach the reviewer |
||
|
|
||
| print("=" * 80) | ||
| print("π§ͺ TESTING CODEWIKI CLUSTERING LOCALLY") | ||
| print("=" * 80) | ||
|
|
||
| # Setup test repo path | ||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
| # Setup test repo path (override with CODEWIKI_TEST_REPO env var) | ||
| test_repo = os.getenv("CODEWIKI_TEST_REPO") | ||
|
|
||
| if not os.path.exists(test_repo): | ||
| if not test_repo or not os.path.exists(test_repo): | ||
| print(f"β Test repo not found: {test_repo}") | ||
| print(" Update test_repo variable to point to your local repo") | ||
| return | ||
| print(" Set the CODEWIKI_TEST_REPO environment variable to point to your local repo") | ||
| results.add_test("test_repo_exists", False, f"Test repo not found: {test_repo}") | ||
| return False | ||
|
|
||
| print(f"\nπ Test repository: {test_repo}") | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test_clustering_local.py bypasses required factory methods Replaced direct π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ test_clustering_local.py Config construction omits the cluster role entirely Added π€ Prompt for AI agentsfix confidence: π΄ 50 low β review closely β react π/π to teach the reviewer |
||
| # Create minimal config | ||
| config = Config( | ||
| # Create minimal config via the required factory method | ||
| config = Config.from_args( | ||
| repo_path=test_repo, | ||
| output_path="/tmp/codewiki_test_output", | ||
| cluster_provider="openai", | ||
| cluster_model="gpt-4o", | ||
| cluster_api_key=os.getenv("OPENAI_API_KEY") or os.getenv("CLUSTER_API_KEY"), | ||
| cluster_base_url="https://api.openai.com/v1", | ||
| main_provider="openai", | ||
| main_model="gpt-4o", # Use gpt-4o instead of gpt-5.2 | ||
| main_api_key=os.getenv("OPENAI_API_KEY") or os.getenv("MAIN_API_KEY"), | ||
|
|
@@ -120,18 +147,21 @@ def test_clustering(): | |
| print("\nβ FAILED: Empty module tree returned") | ||
| print(" This means the LLM did not follow the prompt format") | ||
| print(" Check logs above for 'Invalid LLM response format' error") | ||
| results.add_test("clustering_produces_modules", False, "Empty module tree returned") | ||
| return False | ||
| else: | ||
| print(f"\nβ SUCCESS: Created {len(module_tree)} modules") | ||
| for module_name, module_info in module_tree.items(): | ||
| comp_count = len(module_info.get("components", [])) | ||
| print(f" - {module_name}: {comp_count} components") | ||
| results.add_test("clustering_produces_modules", True, f"Created {len(module_tree)} modules") | ||
| return True | ||
|
|
||
| except Exception as e: | ||
| print(f"\nβ ERROR: {e}") | ||
| import traceback | ||
| traceback.print_exc() | ||
| results.add_test("clustering_produces_modules", False, str(e)) | ||
| return False | ||
|
|
||
| if __name__ == "__main__": | ||
|
Comment on lines
147
to
167
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π test_clustering_local.py uses raw print/sys.exit instead of TestResults accumulator pattern Introduced a minimal π€ Prompt for AI agentsfix confidence: π‘ 60 medium β react π/π to teach the reviewer |
||
|
|
@@ -141,5 +171,8 @@ def test_clustering(): | |
| print(" Set it with: export OPENAI_API_KEY='your-key-here'") | ||
| sys.exit(1) | ||
|
|
||
| success = test_clustering() | ||
| test_results = TestResults() | ||
| success = test_clustering(test_results) | ||
| test_results.print_summary() | ||
| sys.exit(0 if success else 1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,9 @@ | |
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
| from codewiki.src.config import Config | ||
|
|
||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π Hardcoded developer-specific absolute path in test_clustering_proof.py Replaced the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| config = Config( | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test_clustering_proof.py bypasses required factory methods Changed π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
| config = Config.from_args( | ||
| repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps", | ||
| docs_dir="/tmp/test/docs", max_depth=2, | ||
| main_model="gpt-4o", cluster_model="gpt-4o", fallback_model="claude-opus-4-5-20251101", | ||
|
|
@@ -96,3 +96,4 @@ | |
| more = len(info.get('components', [])) - 5 | ||
| print(f" - {name}: {comp_count} components {comp_list}{'...' if more > 0 else ''}") | ||
| sys.exit(0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,9 @@ | |
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π Hardcoded developer-specific absolute path in test_clustering_real.py Replaced the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
| from codewiki.src.config import Config | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test_clustering_real.py bypasses required factory methods Changed π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| config = Config( | ||
| config = Config.from_args( | ||
| repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps", | ||
| docs_dir="/tmp/test/docs", max_depth=2, | ||
| main_model=os.getenv("MAIN_MODEL", "gpt-4o"), | ||
|
|
@@ -62,3 +62,4 @@ | |
| for name, info in module_tree.items(): | ||
| print(f" - {name}: {len(info.get('components', []))} components") | ||
| sys.exit(0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,10 +18,10 @@ | |
| from codewiki.src.config import Config | ||
|
|
||
| # Test repo | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Direct Config(...) instantiation in test_clustering_simple.py bypasses required factory methods Changed π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.join(os.getcwd(), "test_repo")) | ||
|
|
||
| # Create simple config with all required fields | ||
| config = Config( | ||
| config = Config.from_args( | ||
| repo_path=test_repo, | ||
| output_dir="/tmp/codewiki_test", | ||
| dependency_graph_dir="/tmp/codewiki_test/deps", | ||
|
Comment on lines
18
to
27
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π Hardcoded developer-specific absolute path in test_clustering_simple.py Replaced the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
|
|
@@ -106,3 +106,4 @@ | |
| comp_count = len(module_info.get("components", [])) | ||
| print(f" - {module_name}: {comp_count} components") | ||
| sys.exit(0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,6 @@ | ||
| #!/usr/bin/env python3 | ||
| import os | ||
| import sys | ||
| import logging | ||
|
|
||
| # Setup logging FIRST | ||
| logging.basicConfig( | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π logging.basicConfig() called in a standalone script instead of using setup_logging() Removed the π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer |
||
| level=logging.INFO, | ||
| format='[%(levelname)s] %(message)s', | ||
| force=True | ||
| ) | ||
|
|
||
| # Add CodeWiki to path | ||
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) | ||
|
|
@@ -19,13 +11,17 @@ | |
|
|
||
| from codewiki.src.be.cluster_modules import cluster_modules | ||
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π test_with_logging.py imports codewiki without inserting the repo root computed from file before adding path Removed the hardcoded absolute path π€ Prompt for AI agentsfix confidence: π‘ 70 medium β react π/π to teach the reviewer |
||
| from codewiki.src.be.dependency_analyzer.utils.logging_config import setup_logging | ||
| from codewiki.src.config import Config | ||
|
|
||
| # Setup logging FIRST | ||
| setup_logging() | ||
|
|
||
| # Test repo | ||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Config() constructed directly with keyword arguments instead of via from_args/from_cli factory Replaced direct π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| # Create config | ||
| config = Config( | ||
| config = Config.from_args( | ||
| repo_path=test_repo, | ||
| output_dir="/tmp/codewiki_test", | ||
| dependency_graph_dir="/tmp/codewiki_test/deps", | ||
|
|
@@ -82,3 +78,4 @@ | |
| print(f"β SUCCESS: {len(module_tree)} modules created") | ||
| for name, info in module_tree.items(): | ||
| print(f" - {name}: {len(info.get('components', []))} components") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
𦩠π΄ Config() instantiated directly in integration_test.py with fake test credentials
In
IntegrationTestRunner.create_config, replaced the directConfig(...)dataclass instantiation withConfig.from_args(...), keeping identical keyword arguments, so the test exercises the sanctioned factory path per CODEWIKI-007. This assumesConfig.from_argsaccepts the same keyword signature as the dataclass constructor (includingadditional_source_paths); sincecodewiki/src/config.pyis not visible in this task, I cannot verifyfrom_argsexists with this exact signature or that it doesn't require different argument names/positional CLI-style args β if the factory's signature differs, this call will raise aTypeErrorand would need adjustment to match the real factory API.π€ Prompt for AI agents
fix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer