fix(CODEWIKI-006-2): 2 review findings in cluster_modules.py - #37
fix(CODEWIKI-006-2): 2 review findings in cluster_modules.py#37flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| logger.error(f"Invalid module tree format - expected dict, got {type(module_tree)}") | ||
| return {} | ||
|
|
||
| # CRITICAL: Validate all component IDs are integers | ||
| max_id = len(id_to_fqdn) - 1 | ||
| for module_name, module_info in module_tree.items(): | ||
| if "components" not in module_info: | ||
| continue | ||
|
|
||
| component_ids = module_info["components"] | ||
| invalid_ids = [] | ||
|
|
||
| for comp_id in component_ids: | ||
| # Check if ID is an integer | ||
| if not isinstance(comp_id, int): | ||
| invalid_ids.append(f"{comp_id} (type: {type(comp_id).__name__})") | ||
| # Check if ID is in valid range | ||
| elif comp_id < 0 or comp_id > max_id: | ||
| invalid_ids.append(f"{comp_id} (out of range 0-{max_id})") | ||
|
|
||
| if invalid_ids: | ||
| logger.error(f"❌ Module '{module_name}' contains invalid component IDs:") | ||
| logger.error(f" Invalid IDs: {invalid_ids}") | ||
| logger.error(f" Expected: Integers in range 0-{max_id}") | ||
| logger.error(f" LLM ignored instructions and returned non-integer IDs!") | ||
| return {} | ||
|
|
||
| logger.info(f"✅ LLM response validation passed: All IDs are integers in valid range") | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to parse LLM response: {e}. Response: {response[:200]}...") | ||
| logger.error(f"Traceback: {traceback.format_exc()}") | ||
| return {} | ||
|
|
||
| # Normalize component IDs using simple lookup (replaces 200+ lines of fuzzy matching) | ||
| # Normalize component IDs using simple lookup (replaces 200+ lines of fuzzy matching | ||
| # and the duplicated inline ID validation that previously lived here) | ||
| module_tree = normalize_component_ids_by_lookup(module_tree, id_to_fqdn) | ||
|
|
||
| # check if the module tree is valid |
There was a problem hiding this comment.
🦩 🔴 cluster_modules() re-implements ID validation inline instead of reusing normalize_component_ids_by_lookup
Removed the duplicated inline ID-validation block (the max_id/invalid_ids/isinstance(comp_id, int) loop) from cluster_modules() that re-checked component ID integer-ness and range before calling normalize_component_ids_by_lookup. cluster_modules() now relies solely on normalize_component_ids_by_lookup() (already defined earlier in the file) to convert IDs via int() and reject/log invalid ones, eliminating the divergent second implementation. Behavior differs slightly: previously an invalid ID caused cluster_modules() to abort and return {} for the whole module; now normalize_component_ids_by_lookup simply drops invalid IDs (logs a ❌ warning) and continues with the valid ones, matching CODEWIKI-006-2's described behavior of accepting quoted-int strings via int() and rejecting bad IDs with a warning rather than a hard failure — reviewer should confirm this relaxed-but-consistent failure mode is acceptable.
🤖 Prompt for AI agents
In codewiki/src/be/cluster_modules.py around line 328, review and complete this code-review fix: cluster_modules() re-implements ID validation inline instead of reusing normalize_component_ids_by_lookup.
What the draft fix changed: Removed the duplicated inline ID-validation block (the `max_id`/`invalid_ids`/`isinstance(comp_id, int)` loop) from `cluster_modules()` that re-checked component ID integer-ness and range before calling `normalize_component_ids_by_lookup`. `cluster_modules()` now relies solely on `normalize_component_ids_by_lookup()` (already defined earlier in the file) to convert IDs via `int()` and reject/log invalid ones, eliminating the divergent second implementation. Behavior differs slightly: previously an invalid ID caused `cluster_modules()` to abort and return `{}` for the whole module; now `normalize_component_ids_by_lookup` simply drops invalid IDs (logs a `❌` warning) and continues with the valid ones, matching CODEWIKI-006-2's described behavior of accepting quoted-int strings via `int()` and rejecting bad IDs with a warning rather than a hard failure — reviewer should confirm this relaxed-but-consistent failure mode is acceptable.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| Also includes a small backward-compatibility layer for functions that were | ||
| used by the older short-ID based clustering approach. | ||
| """ | ||
| from typing import List, Dict, Any, Optional |
There was a problem hiding this comment.
🦩 🟠 cluster_modules.py has no top-level module docstring
Added a multi-line module-level docstring at the top of codewiki/src/be/cluster_modules.py, before the imports, describing the module's role in the pipeline (module clustering, ID mapping, LLM prompt construction, recursive sub-clustering) and the backward-compatibility layer, satisfying the documentation norm for non-trivial modules.
🤖 Prompt for AI agents
In codewiki/src/be/cluster_modules.py around line 1, review and complete this code-review fix: cluster_modules.py has no top-level module docstring.
What the draft fix changed: Added a multi-line module-level docstring at the top of `codewiki/src/be/cluster_modules.py`, before the imports, describing the module's role in the pipeline (module clustering, ID mapping, LLM prompt construction, recursive sub-clustering) and the backward-compatibility layer, satisfying the documentation norm for non-trivial modules.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
Closes 2 review findings in
codewiki/src/be/cluster_modules.py.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/be/cluster_modules.py:328codewiki/src/be/cluster_modules.py:1What 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.