fix(CODEWIKI-009): CU-86akhf8u6 7 review findings across 6 files - #78
flamingo[bot] wants to merge 6 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| current_module_tree={}, current_module_name=None, current_module_path=[] | ||
| ) | ||
|
|
||
| print("\n" + "=" * 80) |
There was a problem hiding this comment.
🦩 🟠 test_clustering_proof.py, test_clustering_real.py, test_clustering_simple.py use ad-hoc sys.exit checks instead of TestResults accumulator
Replaced the ad-hoc if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0) block at the end of the script with a TestResults class (with add_test and print_summary methods, matching the pattern described for test_clustering_local.py / test_clustering_validation.py) instantiated as results, which records a single pass/fail assertion on module_tree non-emptiness and drives the final sys.exit() via results.print_summary()'s boolean return.
🤖 Prompt for AI agents
In test_clustering_real.py around line 56, review and complete this code-review fix: test_clustering_proof.py, test_clustering_real.py, test_clustering_simple.py use ad-hoc sys.exit checks instead of TestResults accumulator.
What the draft fix changed: Replaced the ad-hoc `if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0)` block at the end of the script with a `TestResults` class (with `add_test` and `print_summary` methods, matching the pattern described for test_clustering_local.py / test_clustering_validation.py) instantiated as `results`, which records a single pass/fail assertion on `module_tree` non-emptiness and drives the final `sys.exit()` via `results.print_summary()`'s boolean return.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| from codewiki.src.be.dependency_analyzer.models.core import Node | ||
| from codewiki.src.config import 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" + "=" * 80) | ||
| print("TEST SUMMARY") | ||
| print("=" * 80) | ||
| passed_count = 0 | ||
| for name, passed, message in self.tests: | ||
| status = "✅ PASS" if passed else "❌ FAIL" | ||
| print(f"{status} - {name}" + (f": {message}" if message else "")) | ||
| if passed: | ||
| passed_count += 1 | ||
| total = len(self.tests) | ||
| print("-" * 80) | ||
| print(f"Total: {passed_count}/{total} passed") | ||
| print("=" * 80) | ||
| return passed_count == total | ||
|
|
||
|
|
||
| results = TestResults() | ||
|
|
||
| test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))) | ||
|
|
||
| config = Config( |
There was a problem hiding this comment.
🦩 🟠 logging.basicConfig() called directly in test_clustering_real.py instead of centralized setup_logging()
The finding calls for using a centralized setup_logging() utility instead of logging.basicConfig(), but no such module/function is visible anywhere in the provided material, so I could not import a real one without inventing an identifier/module. Left logging.basicConfig(...) unchanged in this file to avoid introducing a fabricated import; a complete fix requires locating the actual centralized logging module in the repo (not shown to me) and importing its real setup_logging symbol here.
🤖 Prompt for AI agents
In test_clustering_real.py around line 8, review and complete this code-review fix: logging.basicConfig() called directly in test_clustering_real.py instead of centralized setup_logging().
What the draft fix changed: The finding calls for using a centralized `setup_logging()` utility instead of `logging.basicConfig()`, but no such module/function is visible anywhere in the provided material, so I could not import a real one without inventing an identifier/module. Left `logging.basicConfig(...)` unchanged in this file to avoid introducing a fabricated import; a complete fix requires locating the actual centralized logging module in the repo (not shown to me) and importing its real `setup_logging` symbol here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 20 low — review closely — react 👍/👎 to teach the reviewer
| print("\n" + "=" * 80) | ||
|
|
||
| # Show result | ||
| if len(module_tree) == 0: |
There was a problem hiding this comment.
🦩 🟠 test_with_logging.py uses ad-hoc print()-based assertions instead of TestResults pattern
Replaced the ad-hoc print()-based pass/fail reporting at the end of the script with a TestResults class (defining add_test() and print_summary()) instantiated as results, and replaced the final if len(module_tree) == 0 ... else ... print block with calls to results.add_test(...) followed by success = results.print_summary(); sys.exit(0 if success else 1), ensuring the script now exits non-zero on failure as required by CODEWIKI-009.
🤖 Prompt for AI agents
In test_with_logging.py around line 77, review and complete this code-review fix: test_with_logging.py uses ad-hoc print()-based assertions instead of TestResults pattern.
What the draft fix changed: Replaced the ad-hoc print()-based pass/fail reporting at the end of the script with a `TestResults` class (defining `add_test()` and `print_summary()`) instantiated as `results`, and replaced the final `if len(module_tree) == 0 ... else ...` print block with calls to `results.add_test(...)` followed by `success = results.print_summary(); sys.exit(0 if success else 1)`, ensuring the script now exits non-zero on failure as required by CODEWIKI-009.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -1,17 +1,42 @@ | |||
| """ | |||
There was a problem hiding this comment.
🦩 🟠 test_fqdn_normalization.py uses pytest assertions but doesn't follow the TestResults accumulator pattern required for repo test scripts
Rewrote test_fqdn_normalization.py to remove the pytest dependency and bare assert statements. Added a TestResults class implementing add_test/print_summary as required by CODEWIKI-009, converted every test_* function to accept a results parameter and replaced each assert with a corresponding results.add_test(...) call carrying a descriptive name/message instead of raising AssertionError. Replaced the if __name__ == "__main__": pytest.main(...) runner with a manual invocation of all test functions followed by results.print_summary() and exit(0 if success else 1). Removed the import pytest and the pytest-based docstring/run instructions. Behavior/logic of each test case (the assertions being checked) is unchanged; only the mechanism for recording pass/fail was converted to the accumulator pattern.
🤖 Prompt for AI agents
In test_fqdn_normalization.py around line 1, review and complete this code-review fix: test_fqdn_normalization.py uses pytest assertions but doesn't follow the TestResults accumulator pattern required for repo test scripts.
What the draft fix changed: Rewrote test_fqdn_normalization.py to remove the pytest dependency and bare assert statements. Added a `TestResults` class implementing `add_test`/`print_summary` as required by CODEWIKI-009, converted every `test_*` function to accept a `results` parameter and replaced each `assert` with a corresponding `results.add_test(...)` call carrying a descriptive name/message instead of raising AssertionError. Replaced the `if __name__ == "__main__": pytest.main(...)` runner with a manual invocation of all test functions followed by `results.print_summary()` and `exit(0 if success else 1)`. Removed the `import pytest` and the pytest-based docstring/run instructions. Behavior/logic of each test case (the assertions being checked) is unchanged; only the mechanism for recording pass/fail was converted to the accumulator pattern.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| def __init__(self): | ||
| self.tests = [] | ||
|
|
||
| def add_test(self, name: str, passed: bool): | ||
| self.tests.append((name, passed)) | ||
| def add_test(self, name: str, passed: bool, details: str = ""): | ||
| self.tests.append((name, passed, details)) | ||
|
|
||
| def print_summary(self) -> bool: | ||
| print("=" * 60) | ||
| print("TEST SUMMARY") | ||
| print("=" * 60) | ||
|
|
||
| all_passed = True | ||
| for test_name, passed in self.tests: | ||
| for test_name, passed, details in self.tests: | ||
| status = "✅ PASS" if passed else "❌ FAIL" | ||
| print(f"{status}: {test_name}") | ||
| suffix = f" ({details})" if details else "" | ||
| print(f"{status}: {test_name}{suffix}") | ||
| if not passed: | ||
| all_passed = False | ||
|
|
There was a problem hiding this comment.
🦩 🟠 TestResults class in test_id_based_clustering.py lacks the required add_test(details) signature and per-assertion coverage
Fixed TestResults.add_test in the TestResults class to accept an optional details: str = "" parameter and store it as a 3-tuple (name, passed, details); print_summary now prints the details suffix when present. Each of test_json_parsing, test_return_types, test_id_validation, and test_normalization now take a results: TestResults parameter and call results.add_test(...) with a descriptive name/details string at every individual assertion point (both pass and fail branches), in addition to the pre-existing top-level results.add_test(...) calls at the bottom of the file which remain unchanged in call shape (now resolving to the new signature with details defaulted). Risk: the per-assertion granularity/wording is my own invention since the finding only requires the mechanism (signature + per-assertion recording), not exact names, so a reviewer may want different assertion names/messages; functionally the script still exits 0/1 identically to before.
🤖 Prompt for AI agents
In test_id_based_clustering.py around line 15, review and complete this code-review fix: TestResults class in test_id_based_clustering.py lacks the required add_test(details) signature and per-assertion coverage.
What the draft fix changed: Fixed `TestResults.add_test` in the `TestResults` class to accept an optional `details: str = ""` parameter and store it as a 3-tuple `(name, passed, details)`; `print_summary` now prints the details suffix when present. Each of `test_json_parsing`, `test_return_types`, `test_id_validation`, and `test_normalization` now take a `results: TestResults` parameter and call `results.add_test(...)` with a descriptive name/details string at every individual assertion point (both pass and fail branches), in addition to the pre-existing top-level `results.add_test(...)` calls at the bottom of the file which remain unchanged in call shape (now resolving to the new signature with `details` defaulted). Risk: the per-assertion granularity/wording is my own invention since the finding only requires the mechanism (signature + per-assertion recording), not exact names, so a reviewer may want different assertion names/messages; functionally the script still exits 0/1 identically to before.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -208,48 +234,51 @@ def test_llm_service_creation(backend_config): | |||
| def main(): | |||
There was a problem hiding this comment.
🦩 🟠 test_local_config.py uses ad-hoc print()+sys.exit(1) checks instead of the TestResults accumulator pattern
Added a TestResults class (with add_test() and print_summary()) near the top of test_local_config.py, and reworked main() to instantiate TestResults(), call add_test(name, passed, message) after each of the five test steps, and invoke results.print_summary() before both the early-exit sys.exit(1) paths and the final success path. The individual test_* helper functions and their internal print()/return-False semantics are left untouched (out of scope for this finding), but the top-level main() orchestration now reports via the mandated accumulator pattern instead of only ad-hoc prints before exiting. Risk: the per-test helper functions still print their own diagnostics directly (not fully centralized through the accumulator), so a stricter reading of CODEWIKI-009 that requires all pass/fail signaling to flow through TestResults would need further refactor of each test_* function's internals.
🤖 Prompt for AI agents
In test_local_config.py around line 208, review and complete this code-review fix: test_local_config.py uses ad-hoc print()+sys.exit(1) checks instead of the TestResults accumulator pattern.
What the draft fix changed: Added a `TestResults` class (with `add_test()` and `print_summary()`) near the top of `test_local_config.py`, and reworked `main()` to instantiate `TestResults()`, call `add_test(name, passed, message)` after each of the five test steps, and invoke `results.print_summary()` before both the early-exit `sys.exit(1)` paths and the final success path. The individual `test_*` helper functions and their internal `print()`/return-False semantics are left untouched (out of scope for this finding), but the top-level `main()` orchestration now reports via the mandated accumulator pattern instead of only ad-hoc prints before exiting. Risk: the per-test helper functions still print their own diagnostics directly (not fully centralized through the accumulator), so a stricter reading of CODEWIKI-009 that requires *all* pass/fail signaling to flow through `TestResults` would need further refactor of each `test_*` function's internals.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| print("\nThis test demonstrates the fix for ambiguous component resolution") | ||
| print("by using module name context to disambiguate candidates.") | ||
|
|
||
| # Run all tests |
There was a problem hiding this comment.
🦩 🟠 test_module_disambiguation.py accumulates pass/fail counters ad hoc instead of using the TestResults pattern
Replaced the ad hoc tests_passed/tests_failed integer counters and manual print statements in main() with a new TestResults class (added near top of test_module_disambiguation.py) providing add_test() and print_summary(), matching the CODEWIKI-009 accumulator pattern. main() now instantiates TestResults(), calls results.add_test(name, passed, message) for each of the three test cases, calls results.print_summary() for the structured summary, and returns results.exit_code (0 if no failures, 1 otherwise) which is passed to sys.exit() unchanged. This keeps the same three test scenarios and pass/fail semantics but routes them through the mandated accumulator instead of manual counters/print calls. Risk: since no shared TestResults module exists elsewhere in the repo to import, the class is defined locally in this file rather than imported from a common helper — if CODEWIKI-009 mandates a shared implementation across test_*.py scripts, this local definition would need to be consolidated into a common module in a follow-up change.
🤖 Prompt for AI agents
In test_module_disambiguation.py around line 258, review and complete this code-review fix: test_module_disambiguation.py accumulates pass/fail counters ad hoc instead of using the TestResults pattern.
What the draft fix changed: Replaced the ad hoc `tests_passed`/`tests_failed` integer counters and manual print statements in `main()` with a new `TestResults` class (added near top of `test_module_disambiguation.py`) providing `add_test()` and `print_summary()`, matching the CODEWIKI-009 accumulator pattern. `main()` now instantiates `TestResults()`, calls `results.add_test(name, passed, message)` for each of the three test cases, calls `results.print_summary()` for the structured summary, and returns `results.exit_code` (0 if no failures, 1 otherwise) which is passed to `sys.exit()` unchanged. This keeps the same three test scenarios and pass/fail semantics but routes them through the mandated accumulator instead of manual counters/print calls. Risk: since no shared `TestResults` module exists elsewhere in the repo to import, the class is defined locally in this file rather than imported from a common helper — if CODEWIKI-009 mandates a *shared* implementation across test_*.py scripts, this local definition would need to be consolidated into a common module in a follow-up change.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 7 review findings across 6 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
test_clustering_real.py:56test_clustering_real.py:8test_with_logging.py:77test_fqdn_normalization.py:1test_id_based_clustering.py:15test_local_config.py:208test_module_disambiguation.py:258What 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:
4b0306d9-ca7f-413c-857e-fc323d1e9f21Merging 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.
ClickUp task: CU-86akhf8u6 CodeWiki review findings sweep (9 PRs)