Skip to content

fix(adhoc-sweep-fixes): CU-86akhf8u6 4 review findings across 4 files #80

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 4 commits into
base: main
Choose a base branch
from
+24 −11
Draft

fix(adhoc-sweep-fixes): CU-86akhf8u6 4 review findings across 4 files #80

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
3d8ba75
fix(adhoc-sweep-fixes): 4 review findings across 4 files
flamingo[bot] Sep 14, 2026
0c11179
fix(adhoc-sweep-fixes): 4 review findings across 4 files
flamingo[bot] Sep 14, 2026
a9911ce
fix(adhoc-sweep-fixes): 4 review findings across 4 files
flamingo[bot] Sep 14, 2026
722f5f8
fix(adhoc-sweep-fixes): 4 review findings across 4 files
flamingo[bot] Sep 14, 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
12 changes: 4 additions & 8 deletions codewiki/src/be/dependency_analyzer/analysis/call_graph_analyzer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,16 +356,11 @@ def _resolve_call_relationships(self):
relationship.is_resolved = True
resolved_count += 1
elif "." in callee_name:
if callee_name in func_lookup:
relationship.callee = func_lookup[callee_name]
method_name = callee_name.split(".")[-1]
if method_name in func_lookup:
relationship.callee = func_lookup[method_name]
relationship.is_resolved = True
resolved_count += 1
else:
method_name = callee_name.split(".")[-1]
if method_name in func_lookup:
relationship.callee = func_lookup[method_name]
relationship.is_resolved = True
resolved_count += 1

def _deduplicate_relationships(self):
"""
Comment on lines 356 to 366

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

🦩 🟠 _resolve_call_relationships redundant/dead branch checking callee_name in func_lookup twice

In _resolve_call_relationships, removed the dead inner if callee_name in func_lookup: re-check inside the elif "." in callee_name: branch (which could never be true since the outer if already excludes that case). The branch now goes straight to computing method_name = callee_name.split(".")[-1] and resolving via func_lookup[method_name], restoring the intended fallback resolution path for dotted/qualified callee names without changing any other behavior.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/call_graph_analyzer.py around line 351, review and complete this code-review fix: _resolve_call_relationships redundant/dead branch checking callee_name in func_lookup twice.
What the draft fix changed: In `_resolve_call_relationships`, removed the dead inner `if callee_name in func_lookup:` re-check inside the `elif "." in callee_name:` branch (which could never be true since the outer `if` already excludes that case). The branch now goes straight to computing `method_name = callee_name.split(".")[-1]` and resolving via `func_lookup[method_name]`, restoring the intended fallback resolution path for dotted/qualified callee names without changing any other behavior.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -539,3 +534,4 @@ def _select_most_connected_nodes(self, target_count: int):
if rel.caller in selected_func_ids and rel.callee in selected_func_ids
]


16 changes: 14 additions & 2 deletions codewiki/src/be/documentation_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ async def generate_module_documentation(self, components: Dict[str, Any], leaf_n
# Process modules in dependency order
final_module_tree = module_tree
processed_modules = set()
failed_modules = []

if len(module_tree) > 0:
logger.info(f"├─ Processing {len(processing_order)} modules...")
Expand Down Expand Up @@ -284,9 +285,20 @@ async def generate_module_documentation(self, components: Dict[str, Any], leaf_n
logger.error(f"│ ├─ [{idx}/{len(processing_order)}] ❌ Failed: {module_key}")
logger.error(f"│ │ └─ Error: {str(e)}")
logger.error(f"│ │ └─ Traceback:\n{traceback.format_exc()}")
# Continue processing other modules (graceful degradation)
# Record the failure so the overall run can be reported/failed
# instead of silently degrading to a stale/incomplete tree.
failed_modules.append((module_key, str(e)))
continue

if failed_modules:
failure_summary = "; ".join(f"{key}: {err}" for key, err in failed_modules)
logger.error(
f"├─ ❌ {len(failed_modules)}/{len(processing_order)} module(s) failed to generate: {failure_summary}"
)
raise RuntimeError(
f"Module documentation generation failed for {len(failed_modules)} module(s): {failure_summary}"
)

# Generate repo overview
logger.info(f"├─ 📚 Generating repository overview...")
import time
Comment on lines 285 to 304

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

🦩 🟠 Broad except Exception in module processing loop silently swallows failures without failing the overall run

In generate_module_documentation, the per-module except Exception block still logs and continues (preserving graceful per-module logging), but now appends (module_key, str(e)) to a new failed_modules list instead of silently proceeding to overview generation. After the processing loop, if failed_modules is non-empty, the function logs an aggregated error summary and raises a RuntimeError before generating the repository overview, so run()'s existing outer try/except (which re-raises) causes the overall pipeline invocation to fail with a non-zero/exception outcome instead of returning as if fully successful. This directly addresses the finding that failures were swallowed with no signal to callers. Risk: this changes behavior from "best-effort partial docs" to "fail the whole run on any module failure," which is an intentional but non-trivial behavior change or callers of run()/generate_module_documentation that previously tolerated partial failures will now see exceptions propagate; a more complete fix might instead return a structured failure report object, but that would require API changes to callers not visible in this file.

🤖 Prompt for AI agents 
In codewiki/src/be/documentation_generator.py around line 283, review and complete this code-review fix: Broad except Exception in module processing loop silently swallows failures without failing the overall run.
What the draft fix changed: In `generate_module_documentation`, the per-module `except Exception` block still logs and `continue`s (preserving graceful per-module logging), but now appends `(module_key, str(e))` to a new `failed_modules` list instead of silently proceeding to overview generation. After the processing loop, if `failed_modules` is non-empty, the function logs an aggregated error summary and raises a `RuntimeError` before generating the repository overview, so `run()`'s existing outer `try/except` (which re-raises) causes the overall pipeline invocation to fail with a non-zero/exception outcome instead of returning as if fully successful. This directly addresses the finding that failures were swallowed with no signal to callers. Risk: this changes behavior from "best-effort partial docs" to "fail the whole run on any module failure," which is an intentional but non-trivial behavior change or callers of `run()`/`generate_module_documentation` that previously tolerated partial failures will now see exceptions propagate; a more complete fix might instead return a structured failure report object, but that would require API changes to callers not visible in this file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -492,4 +504,4 @@ async def run(self) -> None:
except Exception as e:
logger.error(f"Documentation generation failed: {str(e)}")
logger.error(f"Traceback: {traceback.format_exc()}")
raise
raise
2 changes: 1 addition & 1 deletion codewiki/src/fe/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class WebAppConfig:
CACHE_EXPIRY_DAYS = 365

# Job cleanup settings
JOB_CLEANUP_HOURS = 24000

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

🦩 🟠 WebAppConfig.JOB_CLEANUP_HOURS = 24000 is likely a units bug (should be minutes or a much smaller hour count)

Changed WebAppConfig.JOB_CLEANUP_HOURS from 24000 to 24 in codewiki/src/fe/config.py, matching the suggested fix and restoring intended daily cleanup semantics so cleanup_old_jobs() (elsewhere in the codebase, not shown here) will actually expire stale entries in background_worker.job_status instead of retaining them for ~2.74 years.

🤖 Prompt for AI agents 
In codewiki/src/fe/config.py around line 25, review and complete this code-review fix: WebAppConfig.JOB_CLEANUP_HOURS = 24000 is likely a units bug (should be minutes or a much smaller hour count).
What the draft fix changed: Changed `WebAppConfig.JOB_CLEANUP_HOURS` from 24000 to 24 in `codewiki/src/fe/config.py`, matching the suggested fix and restoring intended daily cleanup semantics so `cleanup_old_jobs()` (elsewhere in the codebase, not shown here) will actually expire stale entries in `background_worker.job_status` instead of retaining them for ~2.74 years.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

JOB_CLEANUP_HOURS = 24
RETRY_COOLDOWN_MINUTES = 3

# Server settings
Expand Down
5 changes: 5 additions & 0 deletions codewiki/src/fe/github_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->

if result.returncode != 0:
logger.error(f"Error cloning repository: {result.stderr}")
if os.path.isdir(target_dir):
shutil.rmtree(target_dir, ignore_errors=True)
return False

# Checkout specific commit
Expand All @@ -91,6 +93,8 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->

if result.returncode != 0:
logger.error(f"Error cloning repository: {result.stderr}")
if os.path.isdir(target_dir):
shutil.rmtree(target_dir, ignore_errors=True)
return False

return True
Comment on lines 93 to 100

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

🦩 🟠 clone_repository leaves a partially-cloned directory on timeout/exception during shallow clone path

In GitHubRepoProcessor.clone_repository, added shutil.rmtree(target_dir, ignore_errors=True) cleanup before the return False in the shallow-clone else branch's if result.returncode != 0 check, matching the existing cleanup pattern used in the commit_id checkout-failure branch. Also added the same cleanup to the commit_id branch's initial clone failure check (if result.returncode != 0 after the full clone) for consistency, since it had the identical leftover-directory risk on partial clone failure.

🤖 Prompt for AI agents 
In codewiki/src/fe/github_processor.py around line 86, review and complete this code-review fix: clone_repository leaves a partially-cloned directory on timeout/exception during shallow clone path.
What the draft fix changed: In GitHubRepoProcessor.clone_repository, added `shutil.rmtree(target_dir, ignore_errors=True)` cleanup before the `return False` in the shallow-clone `else` branch's `if result.returncode != 0` check, matching the existing cleanup pattern used in the commit_id checkout-failure branch. Also added the same cleanup to the commit_id branch's initial clone failure check (`if result.returncode != 0` after the full clone) for consistency, since it had the identical leftover-directory risk on partial clone failure.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -99,3 +103,4 @@ def clone_repository(clone_url: str, target_dir: str, commit_id: str = None) ->
if os.path.isdir(target_dir):
shutil.rmtree(target_dir, ignore_errors=True)
return False