fix(adhoc-sweep-fixes): CU-86akhf8u6 4 review findings across 4 files - #80
flamingo[bot] wants to merge 4 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| 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): | ||
| """ |
There was a problem hiding this comment.
🦩 🟠 _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
|
|
||
| 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 |
There was a problem hiding this comment.
🦩 🟠 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
| 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 |
There was a problem hiding this comment.
🦩 🟠 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
| CACHE_EXPIRY_DAYS = 365 | ||
|
|
||
| # Job cleanup settings | ||
| JOB_CLEANUP_HOURS = 24000 |
There was a problem hiding this comment.
🦩 🟠 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
flamingo
Bot
changed the title
Closes 4 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/be/dependency_analyzer/analysis/call_graph_analyzer.py:351codewiki/src/fe/github_processor.py:86codewiki/src/be/documentation_generator.py:283codewiki/src/fe/config.py:25What 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:
4b0306d9-ca7f-413c-857e-fc323d1e9f21Merging 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-86akhf8u6 CodeWiki review findings sweep (9 PRs)