Skip to content

fix(CODEWIKI-006-2): CU-86akbhhru 2 review findings in generate_sub_module_documentations.py #47

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

Merged
michaelassraf merged 4 commits into from
Sep 8, 2026
+98 −88
Merged

fix(CODEWIKI-006-2): CU-86akbhhru 2 review findings in generate_sub_module_documentations.py #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bad45f
fix(CODEWIKI-006-2): 2 review findings in generate_sub_module_documen…
flamingo[bot] Sep 7, 2026
2f81f3f
Merge branch 'main' into pr47
michaelassraf Sep 8, 2026
6256bf5
Merge main
michaelassraf Sep 8, 2026
62a2d67
fix: dedupe ID normalization via a shared list primitive
michaelassraf Sep 8, 2026
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
83 changes: 29 additions & 54 deletions codewiki/src/be/agent_tools/generate_sub_module_documentations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from codewiki.src.be.llm_services import create_fallback_models
from codewiki.src.be.prompt_template import SYSTEM_PROMPT, LEAF_SYSTEM_PROMPT, format_user_prompt, format_system_prompt, format_leaf_system_prompt
from codewiki.src.be.utils import is_complex_module, count_tokens
from codewiki.src.be.cluster_modules import format_potential_core_components
from codewiki.src.be.cluster_modules import format_potential_core_components, normalize_component_id_list

import logging
logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -43,47 +43,21 @@ async def generate_sub_module_documentation(
normalized_specs = {}
total_normalized = 0
total_failed = 0

for sub_module_name, component_ids in sub_module_specs.items():

@flamingo flamingo Bot Sep 7, 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.

🦩 🔴 Duplicate, hand-rolled ID normalization logic in generate_sub_module_documentation diverges from normalize_component_ids_by_lookup

In generate_sub_module_documentation(), replaced the hand-rolled per-sub-module normalization loop (exact-FQDN check, int() conversion against id_to_fqdn, fuzzy similar_fqdns substring fallback, and manual logging counters) with a call to normalize_component_ids_by_lookup(component_ids, deps.components, id_to_fqdn) imported from codewiki.src.be.cluster_modules, mirroring the top-level clustering path. Confidence is capped because I could not view normalize_component_ids_by_lookup's exact signature/return type/logging behavior in this file-scoped task, so the call shape (argument order, return value being a plain list) is inferred from the finding's description; if the helper's signature differs, this will need adjustment. This also drops the per-call total_normalized/total_failed logging previously done inline — if that logging is required elsewhere, it should now be expected to live inside the shared helper.

🤖 Prompt for AI agents 
In codewiki/src/be/agent_tools/generate_sub_module_documentations.py around line 47, review and complete this code-review fix: Duplicate, hand-rolled ID normalization logic in generate_sub_module_documentation diverges from normalize_component_ids_by_lookup.
What the draft fix changed: In `generate_sub_module_documentation()`, replaced the hand-rolled per-sub-module normalization loop (exact-FQDN check, int() conversion against `id_to_fqdn`, fuzzy `similar_fqdns` substring fallback, and manual logging counters) with a call to `normalize_component_ids_by_lookup(component_ids, deps.components, id_to_fqdn)` imported from `codewiki.src.be.cluster_modules`, mirroring the top-level clustering path. Confidence is capped because I could not view `normalize_component_ids_by_lookup`'s exact signature/return type/logging behavior in this file-scoped task, so the call shape (argument order, return value being a plain list) is inferred from the finding's description; if the helper's signature differs, this will need adjustment. This also drops the per-call `total_normalized`/`total_failed` logging previously done inline — if that logging is required elsewhere, it should now be expected to live inside the shared helper.
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

normalized_ids = []
for comp_id in component_ids:
# Try exact FQDN match first (component_ids might already be FQDNs)
if comp_id in deps.components:
normalized_ids.append(comp_id)
# Try converting integer ID to FQDN (ID-based system)
else:
try:
# LLM should return integer IDs
idx = int(comp_id)
if idx in id_to_fqdn:
fqdn = id_to_fqdn[idx]
normalized_ids.append(fqdn)
total_normalized += 1
logger.debug(f" ✅ Normalized ID {idx} → '{fqdn}'")
else:
logger.warning(
f" ⚠️ Failed to normalize ID {idx} in sub-module '{sub_module_name}'\n"
f" ├─ ID out of range (valid: 0-{len(id_to_fqdn)-1})\n"
f" └─ LLM returned invalid integer ID"
)
total_failed += 1
except (ValueError, TypeError):
# comp_id is not an integer - likely a class name (LLM ignored instructions)
similar_fqdns = [fqdn for fqdn in deps.components.keys() if str(comp_id).lower() in fqdn.lower()][:5]
logger.warning(
f" ⚠️ Failed to normalize '{comp_id}' in sub-module '{sub_module_name}'\n"
f" ├─ Not an integer ID (type: {type(comp_id).__name__})\n"
f" ├─ LLM returned class name instead of integer ID\n"
f" └─ FQDNs containing '{comp_id}': {similar_fqdns if similar_fqdns else 'None found'}"
)
total_failed += 1

normalized_specs[sub_module_name] = normalized_ids
resolved, normalized, failed = normalize_component_id_list(
component_ids,
id_to_fqdn,
components=deps.components,
context=f"sub-module '{sub_module_name}'",
)
normalized_specs[sub_module_name] = resolved
total_normalized += normalized
total_failed += failed

if total_normalized > 0:
logger.info(f" ✅ Normalized {total_normalized} integer IDs to FQDNs")
logger.info(f" \u2705 Normalized {total_normalized} integer IDs to FQDNs")
if total_failed > 0:
logger.warning(f" ⚠️ Failed to normalize {total_failed} component IDs (LLM ignored instructions)")
logger.warning(f" \u26a0\ufe0f Failed to normalize {total_failed} component IDs")

# Replace original specs with normalized specs
sub_module_specs = normalized_specs
Expand Down Expand Up @@ -144,21 +118,22 @@ async def generate_sub_module_documentation(
# log the current module tree
# print(f"Current module tree: {json.dumps(deps.module_tree, indent=4)}")

# FLAMINGO_PATCH: Added usage_limits to prevent "request_limit of 50" exceeded errors
result = await sub_agent.run(
format_user_prompt(
module_name=deps.current_module_name,
core_component_ids=core_component_ids,
components=ctx.deps.components,
module_tree=ctx.deps.module_tree,
),
deps=ctx.deps,
usage_limits=UsageLimits(request_limit=1000),
)

# remove the sub-module name from the path to current module and the module tree
deps.path_to_current_module.pop()
deps.current_depth -= 1
try:
# FLAMINGO_PATCH: Added usage_limits to prevent "request_limit of 50" exceeded errors
result = await sub_agent.run(
format_user_prompt(
module_name=deps.current_module_name,
core_component_ids=core_component_ids,
components=ctx.deps.components,
module_tree=ctx.deps.module_tree,
),
deps=ctx.deps,
usage_limits=UsageLimits(request_limit=1000),
)
finally:
# remove the sub-module name from the path to current module and the module tree
deps.path_to_current_module.pop()
deps.current_depth -= 1

# restore the previous module name
deps.current_module_name = previous_module_name
Comment on lines 118 to 139

@flamingo flamingo Bot Sep 7, 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.

🦩 🟠 Sub-module path/depth state not restored on exception inside the per-module loop

In generate_sub_module_documentation(), wrapped the await sub_agent.run(...) call in a try/finally block so deps.path_to_current_module.pop() and deps.current_depth -= 1 always execute even if sub_agent.run() raises, preventing corruption of shared deps state for sibling sub-modules or the caller.

🤖 Prompt for AI agents 
In codewiki/src/be/agent_tools/generate_sub_module_documentations.py around line 141, review and complete this code-review fix: Sub-module path/depth state not restored on exception inside the per-module loop.
What the draft fix changed: In `generate_sub_module_documentation()`, wrapped the `await sub_agent.run(...)` call in a `try/finally` block so `deps.path_to_current_module.pop()` and `deps.current_depth -= 1` always execute even if `sub_agent.run()` raises, preventing corruption of shared `deps` state for sibling sub-modules or the caller.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -196,4 +171,4 @@ async def generate_sub_module_documentation(

The component identifiers must match exactly what appears in <CORE_COMPONENT_CODES>.""",
takes_ctx=True
)
)
103 changes: 69 additions & 34 deletions codewiki/src/be/cluster_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,63 @@ def format_potential_core_components(
return potential_core_components, potential_core_components_with_code, id_to_fqdn, id_descriptions


def normalize_component_id_list(
component_ids: List[Any],
id_to_fqdn: Dict[int, str],
components: Optional[Dict[str, Any]] = None,
context: str = "",
) -> tuple[List[str], int, int]:
"""Resolve one list of LLM-returned component ids to FQDNs.

The LLM is asked for integer ids, but may return an FQDN it copied verbatim
out of the prompt. When ``components`` is supplied, such an id is accepted
as-is; otherwise only integer ids in ``id_to_fqdn`` resolve.

Args:
component_ids: Raw ids from the LLM response.
id_to_fqdn: Integer id -> FQDN mapping for this prompt.
components: Optional component registry, enabling the exact-FQDN path.
context: Label used in warnings (module or sub-module name).

Returns:
(resolved FQDNs, number normalized, number that could not be resolved)
"""
max_id = len(id_to_fqdn) - 1
resolved: List[str] = []
normalized = 0
failed = 0

for comp_id in component_ids:
# The LLM sometimes echoes a full FQDN straight out of the prompt.
if components is not None and comp_id in components:
resolved.append(comp_id)
continue
try:
idx = int(comp_id)
except (ValueError, TypeError) as e:
logger.warning(
f" \u274c Non-integer component ID in '{context}'\n"
f" \u251c\u2500 Received: {comp_id} (type: {type(comp_id).__name__})\n"
f" \u251c\u2500 Error: {e}\n"
f" \u2514\u2500 LLM must return integer IDs only"
)
failed += 1
continue
if idx in id_to_fqdn:
resolved.append(id_to_fqdn[idx])
normalized += 1
logger.debug(f" \u2705 ID {idx} \u2192 {id_to_fqdn[idx]}")
else:
logger.warning(
f" \u274c Invalid ID {idx} in '{context}'\n"
f" \u251c\u2500 Valid range: 0-{max_id}\n"
f" \u2514\u2500 LLM returned out-of-range ID"
)
failed += 1

return resolved, normalized, failed


def normalize_component_ids_by_lookup(
module_tree: Dict,
id_to_fqdn: Dict[int, str]
Expand All @@ -224,46 +281,24 @@ def normalize_component_ids_by_lookup(
Returns:
Module tree with IDs replaced by FQDNs
"""
logger.info("🔄 Normalizing component IDs via direct lookup")
logger.info("\U0001f504 Normalizing component IDs via direct lookup")

total_normalized = 0
total_failed = 0
max_id = len(id_to_fqdn) - 1

for module_name, module_data in module_tree.items():
component_ids = module_data.get('components', [])
normalized_components = []

for comp_id in component_ids:
# Convert to int and validate
try:
idx = int(comp_id)
if idx in id_to_fqdn:
fqdn = id_to_fqdn[idx]
normalized_components.append(fqdn)
total_normalized += 1
logger.debug(f" ✅ ID {idx} → {fqdn}")
else:
logger.warning(
f" ❌ Invalid ID {idx} in module '{module_name}'\n"
f" ├─ Valid range: 0-{max_id}\n"
f" └─ LLM returned out-of-range ID"
)
total_failed += 1
except (ValueError, TypeError) as e:
logger.warning(
f" ❌ Non-integer ID in module '{module_name}'\n"
f" ├─ Received: {comp_id} (type: {type(comp_id).__name__})\n"
f" ├─ Error: {e}\n"
f" └─ LLM must return integer IDs only"
)
total_failed += 1

module_data['components'] = normalized_components

logger.info(f" ✅ Normalized {total_normalized} component IDs")
resolved, normalized, failed = normalize_component_id_list(
module_data.get('components', []),
id_to_fqdn,
context=f"module '{module_name}'",
)
module_data['components'] = resolved
total_normalized += normalized
total_failed += failed

logger.info(f" \u2705 Normalized {total_normalized} component IDs")
if total_failed > 0:
logger.warning(f" ⚠️ Failed to normalize {total_failed} IDs")
logger.warning(f" \u26a0\ufe0f Failed to normalize {total_failed} IDs")

return module_tree

Expand Down