fix(CODEWIKI-007): 22 review findings across 10 files - #35
fix(CODEWIKI-007): 22 review findings across 10 files#35flamingo[bot] wants to merge 10 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| return False | ||
|
|
||
| print(f"\n📂 Test repository: {test_repo}") | ||
|
|
There was a problem hiding this comment.
🦩 🔴 Direct Config(...) instantiation in test_clustering_local.py bypasses required factory methods
Replaced direct Config(...) instantiation in test_clustering() with Config.from_args(...), keeping the same keyword arguments. This assumes Config.from_args exists and accepts these same kwargs; unverified against the actual Config class definition (not shown), so the call signature may need adjustment to match the real factory method's parameters.
🤖 Prompt for AI agents
In test_clustering_local.py around line 37, review and complete this code-review fix: Direct Config(...) instantiation in test_clustering_local.py bypasses required factory methods.
What the draft fix changed: Replaced direct `Config(...)` instantiation in `test_clustering()` with `Config.from_args(...)`, keeping the same keyword arguments. This assumes `Config.from_args` exists and accepts these same kwargs; unverified against the actual `Config` class definition (not shown), so the call signature may need adjustment to match the real factory method's parameters.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| return False | ||
|
|
||
| print(f"\n📂 Test repository: {test_repo}") | ||
|
|
There was a problem hiding this comment.
🦩 🔴 test_clustering_local.py Config construction omits the cluster role entirely
Added cluster_provider, cluster_model, cluster_api_key, and cluster_base_url arguments to the Config.from_args(...) call in test_clustering(), defaulting to OpenAI gpt-4o with CLUSTER_API_KEY/OPENAI_API_KEY env var fallback, so all three roles (cluster/main/fallback) are populated. The actual field names and required values for the cluster role are assumed based on the main/fallback pattern shown; a complete fix requires confirming these match Config's real field names.
🤖 Prompt for AI agents
In test_clustering_local.py around line 37, review and complete this code-review fix: test_clustering_local.py Config construction omits the cluster role entirely.
What the draft fix changed: Added `cluster_provider`, `cluster_model`, `cluster_api_key`, and `cluster_base_url` arguments to the `Config.from_args(...)` call in `test_clustering()`, defaulting to OpenAI gpt-4o with `CLUSTER_API_KEY`/`OPENAI_API_KEY` env var fallback, so all three roles (cluster/main/fallback) are populated. The actual field names and required values for the cluster role are assumed based on the main/fallback pattern shown; a complete fix requires confirming these match `Config`'s real field names.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 50 low — review closely — react 👍/👎 to teach the reviewer
| return passed_count == total | ||
|
|
||
| def test_clustering(results): | ||
| """Test clustering on a small sample to verify prompt fix.""" |
There was a problem hiding this comment.
🦩 🔴 Hardcoded absolute developer path leaked into committed test script
Removed the hardcoded /Users/michaelassraf/... path in test_clustering() and replaced it with os.getenv("CODEWIKI_TEST_REPO"), updating the failure message to instruct setting that env var instead of editing the script. Behavior when the env var is unset is now a clean "not found" message rather than crashing, but callers must now set CODEWIKI_TEST_REPO to run the test at all.
🤖 Prompt for AI agents
In test_clustering_local.py around line 22, review and complete this code-review fix: Hardcoded absolute developer path leaked into committed test script.
What the draft fix changed: Removed the hardcoded `/Users/michaelassraf/...` path in `test_clustering()` and replaced it with `os.getenv("CODEWIKI_TEST_REPO")`, updating the failure message to instruct setting that env var instead of editing the script. Behavior when the env var is unset is now a clean "not found" message rather than crashing, but callers must now set `CODEWIKI_TEST_REPO` to run the test at all.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| 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__": |
There was a problem hiding this comment.
🦩 🟠 test_clustering_local.py uses raw print/sys.exit instead of TestResults accumulator pattern
Introduced a minimal TestResults class (with add_test/print_summary) at module level, threaded a results parameter through test_clustering(), recorded pass/fail outcomes at the success, empty-tree-failure, and exception-failure paths, and updated the __main__ block to construct TestResults(), pass it in, and call print_summary() before sys.exit. The TestResults implementation is written from scratch to match the described pattern since test_clustering_integration.py was not shown, so its exact shape/API may not match the sibling file's real implementation.
🤖 Prompt for AI agents
In test_clustering_local.py around line 100, review and complete this code-review fix: test_clustering_local.py uses raw print/sys.exit instead of TestResults accumulator pattern.
What the draft fix changed: Introduced a minimal `TestResults` class (with `add_test`/`print_summary`) at module level, threaded a `results` parameter through `test_clustering()`, recorded pass/fail outcomes at the success, empty-tree-failure, and exception-failure paths, and updated the `__main__` block to construct `TestResults()`, pass it in, and call `print_summary()` before `sys.exit`. The `TestResults` implementation is written from scratch to match the described pattern since `test_clustering_integration.py` was not shown, so its exact shape/API may not match the sibling file's real implementation.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| @@ -16,9 +16,27 @@ | |||
| from codewiki.src.be.dependency_analyzer.models.core import Node | |||
| from codewiki.src.config import Config | |||
There was a problem hiding this comment.
🦩 🔴 Direct Config(...) instantiation in test_clustering_forced.py bypasses required factory methods
Replaced direct Config(...) instantiation with Config.from_cli(...) at the module-level config construction block, keeping all keyword arguments identical. This satisfies the rule that Config must be constructed via factory methods, but it is unverified that Config.from_cli accepts these exact keyword arguments (e.g. cluster_api_key, cluster_base_url) with identical semantics to __init__, since the factory's signature/behavior is not visible in this file. If from_cli has a different parameter contract (e.g. expects a parsed args namespace instead of kwargs), this call will need adjustment.
🤖 Prompt for AI agents
In test_clustering_forced.py around line 17, review and complete this code-review fix: Direct Config(...) instantiation in test_clustering_forced.py bypasses required factory methods.
What the draft fix changed: Replaced direct `Config(...)` instantiation with `Config.from_cli(...)` at the module-level config construction block, keeping all keyword arguments identical. This satisfies the rule that Config must be constructed via factory methods, but it is unverified that `Config.from_cli` accepts these exact keyword arguments (e.g. `cluster_api_key`, `cluster_base_url`) with identical semantics to `__init__`, since the factory's signature/behavior is not visible in this file. If `from_cli` has a different parameter contract (e.g. expects a parsed args namespace instead of kwargs), this call will need adjustment.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -16,9 +16,9 @@ | |||
| from codewiki.src.be.dependency_analyzer.models.core import Node | |||
There was a problem hiding this comment.
🦩 🟠 Hardcoded developer-specific absolute path in test_clustering_real.py
Replaced the hardcoded absolute path "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" with os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) at the test_repo assignment (line 16), making the script portable via an environment variable with a sensible fallback to the script's own directory.
🤖 Prompt for AI agents
In test_clustering_real.py around line 16, review and complete this code-review fix: Hardcoded developer-specific absolute path in test_clustering_real.py.
What the draft fix changed: Replaced the hardcoded absolute path `"/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"` with `os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__)))` at the `test_repo` assignment (line 16), making the script portable via an environment variable with a sensible fallback to the script's own directory.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -18,10 +18,10 @@ | |||
| from codewiki.src.config import Config | |||
|
|
|||
| # Test repo | |||
There was a problem hiding this comment.
🦩 🔴 Direct Config(...) instantiation in test_clustering_simple.py bypasses required factory methods
Changed config = Config(...) to config = Config.from_args(...) at the config construction block, replacing the direct struct-literal instantiation with the factory method as required. UNVERIFIED: I cannot see Config's actual definition in this file/module, so I don't know if from_args exists with this exact signature/keyword-argument set, or if it instead expects a namespace/args object (e.g. from argparse) rather than keyword arguments. If from_args has a different signature, this call will fail at runtime. A complete fix requires inspecting codewiki/src/config.py to confirm the correct factory method name and expected argument shape, and adjusting the call accordingly.
🤖 Prompt for AI agents
In test_clustering_simple.py around line 20, review and complete this code-review fix: Direct Config(...) instantiation in test_clustering_simple.py bypasses required factory methods.
What the draft fix changed: Changed `config = Config(...)` to `config = Config.from_args(...)` at the config construction block, replacing the direct struct-literal instantiation with the factory method as required. UNVERIFIED: I cannot see `Config`'s actual definition in this file/module, so I don't know if `from_args` exists with this exact signature/keyword-argument set, or if it instead expects a namespace/args object (e.g. from argparse) rather than keyword arguments. If `from_args` has a different signature, this call will fail at runtime. A complete fix requires inspecting `codewiki/src/config.py` to confirm the correct factory method name and expected argument shape, and adjusting the call accordingly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| from codewiki.src.config import Config | ||
|
|
||
| # Test repo | ||
| 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", |
There was a problem hiding this comment.
🦩 🟠 Hardcoded developer-specific absolute path in test_clustering_simple.py
Replaced the hardcoded absolute path "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" at the test_repo assignment with os.getenv("TEST_REPO_PATH", os.path.join(os.getcwd(), "test_repo")), making the path configurable via environment variable with a portable relative-to-cwd default. Risk: the default fallback directory test_repo under cwd may not exist in all environments, so the script may still fail if TEST_REPO_PATH is not set and no such directory is present; a complete fix might also create/document this expected test fixture directory.
🤖 Prompt for AI agents
In test_clustering_simple.py around line 17, review and complete this code-review fix: Hardcoded developer-specific absolute path in test_clustering_simple.py.
What the draft fix changed: Replaced the hardcoded absolute path `"/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"` at the `test_repo` assignment with `os.getenv("TEST_REPO_PATH", os.path.join(os.getcwd(), "test_repo"))`, making the path configurable via environment variable with a portable relative-to-cwd default. Risk: the default fallback directory `test_repo` under cwd may not exist in all environments, so the script may still fail if `TEST_REPO_PATH` is not set and no such directory is present; a complete fix might also create/document this expected test fixture directory.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| output_dir = str(self.test_dir / "output") | ||
|
|
||
| # Create config with main path as root, others as additional | ||
| self.config = Config( | ||
| self.config = Config.from_args( | ||
| repo_path=str(self.main_path), | ||
| output_dir=output_dir, | ||
| dependency_graph_dir=output_dir, |
There was a problem hiding this comment.
🦩 🔴 Config() instantiated directly in integration_test.py with fake test credentials
In IntegrationTestRunner.create_config, replaced the direct Config(...) dataclass instantiation with Config.from_args(...), keeping identical keyword arguments, so the test exercises the sanctioned factory path per CODEWIKI-007. This assumes Config.from_args accepts the same keyword signature as the dataclass constructor (including additional_source_paths); since codewiki/src/config.py is not visible in this task, I cannot verify from_args exists 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 a TypeError and would need adjustment to match the real factory API.
🤖 Prompt for AI agents
In test-multi-path/integration_test.py around line 213, review and complete this code-review fix: Config() instantiated directly in integration_test.py with fake test credentials.
What the draft fix changed: In `IntegrationTestRunner.create_config`, replaced the direct `Config(...)` dataclass instantiation with `Config.from_args(...)`, keeping identical keyword arguments, so the test exercises the sanctioned factory path per CODEWIKI-007. This assumes `Config.from_args` accepts the same keyword signature as the dataclass constructor (including `additional_source_paths`); since `codewiki/src/config.py` is not visible in this task, I cannot verify `from_args` exists 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 a `TypeError` and would need adjustment to match the real factory API.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
| 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"), |
There was a problem hiding this comment.
🦩 🔴 Direct Config(...) instantiation in test-multi-path/test_multi_path.py helper function
In create_test_config (test-multi-path/test_multi_path.py, line ~78), changed return Config(...) to return Config.from_args(...), keeping all existing keyword arguments unchanged, to route construction through the sanctioned factory method as required by the finding. UNVERIFIED: I do not have visibility into codewiki/src/config.py to confirm that Config.from_args exists, accepts these exact keyword arguments, or has the same signature/semantics as the direct constructor (e.g. it may parse CLI-style args, apply defaults differently, or not exist at all under this name — the finding also mentions from_cli as an alternative). If from_args does not exist or has an incompatible signature, this change will break every test in the file at runtime. A complete fix requires inspecting config.py to confirm the correct factory name and signature (positional vs keyword, argument parsing behavior) and adjusting the call accordingly, and running the test suite to confirm validate_source_paths() and downstream behavior are unaffected.
🤖 Prompt for AI agents
In test-multi-path/test_multi_path.py around line 78, review and complete this code-review fix: Direct Config(...) instantiation in test-multi-path/test_multi_path.py helper function.
What the draft fix changed: In `create_test_config` (test-multi-path/test_multi_path.py, line ~78), changed `return Config(...)` to `return Config.from_args(...)`, keeping all existing keyword arguments unchanged, to route construction through the sanctioned factory method as required by the finding. UNVERIFIED: I do not have visibility into `codewiki/src/config.py` to confirm that `Config.from_args` exists, accepts these exact keyword arguments, or has the same signature/semantics as the direct constructor (e.g. it may parse CLI-style args, apply defaults differently, or not exist at all under this name — the finding also mentions `from_cli` as an alternative). If `from_args` does not exist or has an incompatible signature, this change will break every test in the file at runtime. A complete fix requires inspecting `config.py` to confirm the correct factory name and signature (positional vs keyword, argument parsing behavior) and adjusting the call accordingly, and running the test suite to confirm `validate_source_paths()` and downstream behavior are unaffected.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
Closes 22 review findings across 10 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
test_clustering_local.py:37test_clustering_local.py:37test_clustering_local.py:22test_clustering_local.py:100test_clustering_forced.py:17test_clustering_forced.py:15test_clustering_forced.py:51test_with_logging.py:25test_with_logging.py:21test_with_logging.py:7test_clustering_debug.py:48test_clustering_debug.py:44test_clustering_debug.py:108test_clustering_proof.py:15test_clustering_proof.py:13test_clustering_real.py:18test_clustering_real.py:16test_clustering_simple.py:20test_clustering_simple.py:17test-multi-path/integration_test.py:213test-multi-path/test_multi_path.py:78What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
bef4f5a8-e7f3-478b-8731-2becca2de658Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.