Skip to content

fix(CODEWIKI-006-2): 2 review findings in cluster_modules.py #37

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.

Sign up for GitHub

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

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −28
Draft

fix(CODEWIKI-006-2): 2 review findings in cluster_modules.py #37

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 16 additions & 28 deletions codewiki/src/be/cluster_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
"""
Module clustering pipeline for CodeWiki.

This module groups leaf-level code components (classes, functions, files)
into higher-level "modules" using LLM-driven clustering. It builds an
integer ID <-> FQDN mapping for components so the LLM prompt/response can
operate on compact integer IDs instead of full fully-qualified names,
normalizes the LLM's returned component IDs back to FQDNs, and recursively
clusters sub-modules until each unit fits under the configured token budget.

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

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

from collections import defaultdict
import logging
Expand Down Expand Up @@ -355,38 +368,13 @@ def cluster_modules(
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
Comment on lines 368 to 380

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down Expand Up @@ -517,4 +505,4 @@ def _find_best_path_match(llm_id: str, candidates: List[str]) -> Optional[str]:
DeprecationWarning,
stacklevel=2
)
return None
return None