fix(CODEWIKI-006-2): CU-86akbhhru 2 review findings in generate_sub_module_documentations.py - #47
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| total_normalized = 0 | ||
| total_failed = 0 | ||
|
|
||
| for sub_module_name, component_ids in sub_module_specs.items(): |
There was a problem hiding this comment.
🦩 🔴 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
| # 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 |
There was a problem hiding this comment.
🦩 🟠 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
flamingo
Bot
changed the title
|
Blocking: this raises normalized_specs[sub_module_name] = normalize_component_ids_by_lookup(
component_ids, deps.components, id_to_fqdn
)The function takes two parameters ( There is also a behaviour loss: the code being deleted tried an exact FQDN match first ( The |
The branch called normalize_component_ids_by_lookup(component_ids,
deps.components, id_to_fqdn) - three positional arguments against a two-argument
function that takes a module tree and returns a dict. That is a TypeError on
every sub-module generation, which is the recursive path the doc pipeline
reaches on any repo deep enough to split a module.
It also dropped a behaviour: the deleted code accepted a component id that was
already an FQDN present in deps.components, since the LLM sometimes echoes one
straight out of the prompt. normalize_component_ids_by_lookup has no such branch.
Introduce normalize_component_id_list(component_ids, id_to_fqdn, components=None,
context='') -> (fqdns, normalized, failed) as the single implementation, and have
both callers use it:
- normalize_component_ids_by_lookup keeps its signature and dict return, and
now delegates per module (verified identical output to main);
- generate_sub_module_documentations passes components=deps.components, so the
exact-FQDN path is preserved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…or change - generate_sub_module_documentations.py: the branch's normalize_component_ids_by_lookup(specs, components, id_to_fqdn) call was a three-argument call against a two-argument function, unpacked as a 3-tuple from a dict return. #47 has since landed the correct dedupe via normalize_component_id_list, so this resolves in favour of main. - analyzers/c.py: reverted the '.' -> '::' component-id change. Changing the separator for C alone is inconsistent with the other seven analyzers, and the FQDN format question belongs with #34/#45 where it can be made coherent end-to-end. The module docstring is kept. What remains is worth having: - deps.py: dict[str, any] -> dict[str, Any]. 'any' is the builtin function, not a type, so the old annotation was meaningless. - agent_orchestrator.__init__: drops a local logger that shadowed the module-level one (same logging.getLogger(__name__) object, so no behaviour change). - typescript.py _get_parent_context: returns 'unknown' instead of falling off the end as None, honouring its '-> str' annotation. Its one caller stores the value and nothing reads it, so this cannot regress. - The tool description now asks for integer IDs, matching what format_potential_core_components actually puts in the prompt. - Module docstrings throughout, plus from_web_job added to the config.py summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes 2 review findings in
codewiki/src/be/agent_tools/generate_sub_module_documentations.py.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/be/agent_tools/generate_sub_module_documentations.py:47codewiki/src/be/agent_tools/generate_sub_module_documentations.py:141What 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:
2cc7a212-e9ac-481a-a76e-5f03d762c00cMerging 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-86akbhhru CodeWiki backend and CLI review findings (12 PRs)