fix(CODEWIKI-006-2): CU-86akbhg0r 2 review findings in cluster_modules.py - #37
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
flamingo
Bot
changed the title
|
Hold: this converts a loud failure into a silent one. The removed block returned So the two are not equivalent:
That is a real change in failure mode for the doc pipeline, where a silently thin The module docstring is good. |
Removing the inline validation was right - normalize_component_ids_by_lookup
already checks the same conditions - but the two are not equivalent. The inline
block returned {} and failed clustering visibly; the normalizer logs a warning
and drops the id. A module could therefore end up with an empty component list,
get documented anyway, and the run would report success with a thin overview.md.
That is a worse failure mode for the doc pipeline than an outright failure.
Restore the contract by comparing the component-id count either side of
normalization and aborting when any id was dropped. Verified to abort on exactly
the same inputs as main's inline validation (out-of-range ids, bare class names)
and to pass on the same ones (all-valid, no components key).
One deliberate relaxation: a string integer such as "1" now resolves, where
main's isinstance(comp_id, int) check rejected it. An LLM returning "1" for 1
is answering correctly in the wrong type.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
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.
ClickUp task: CU-86akbhg0r Code review fixes: CodeWiki backend and CLI review findings (5 PRs)