Skip to content

fix(CODEWIKI-005): CU-86akhf8u6 8 review findings across 4 files #77

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
+36 −16
Draft

fix(CODEWIKI-005): CU-86akhf8u6 8 review findings across 4 files #77

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
5f933d7
fix(CODEWIKI-005): 8 review findings across 4 files
flamingo[bot] Sep 14, 2026
e38c009
fix(CODEWIKI-005): 8 review findings across 4 files
flamingo[bot] Sep 14, 2026
67c0f3f
fix(CODEWIKI-005): 8 review findings across 4 files
flamingo[bot] Sep 14, 2026
468ea6b
fix(CODEWIKI-005): 8 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: 8 additions & 4 deletions codewiki/src/be/dependency_analyzer/analysis/cloning.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ def clone_repository(github_url: str) -> str:
capture_output=True,
text=True,
)
except:

@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.

🦩 🟠 Bare except clauses in cloning.py swallow errors without any logging

In clone_repository, the bare except: pass around the core.longpaths git config subprocess call was changed to except Exception as e: logger.debug(f"Non-fatal git config step failed: {e}"), giving debug-level trace of any failure per CODEWIKI-005.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/cloning.py around line 109, review and complete this code-review fix: Bare except clauses in cloning.py swallow errors without any logging.
What the draft fix changed: In `clone_repository`, the bare `except: pass` around the `core.longpaths` git config subprocess call was changed to `except Exception as e: logger.debug(f"Non-fatal git config step failed: {e}")`, giving debug-level trace of any failure per CODEWIKI-005.
Verify the change is correct and complete; do not refactor unrelated code.

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

pass
except Exception as e:
logger.debug(f"Non-fatal git config step failed: {e}")

subprocess.run(
[
Comment on lines 106 to 113

@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 temp_dir undeleted if the git executable check runs after mkdtemp but the initial 'longpaths' config subprocess.run raises an untyped exception

Added a final except Exception: clause in clone_repository's outer try/except (after the existing TimeoutExpired, CalledProcessError, FileNotFoundError handlers) that cleans up temp_dir via cleanup_repository_safe if it exists and then re-raises the original exception, closing the resource leak for untyped exceptions (e.g. OSError, KeyboardInterrupt) raised during the clone attempt. Note: KeyboardInterrupt is a BaseException, not Exception, so it is still not caught by this handler and temp_dir would still leak in that specific case; a fully complete fix would need a finally-based cleanup keyed on success/failure state or a broader except BaseException, which was avoided here to keep the change minimal and not swallow interrupts silently.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/cloning.py around line 93, review and complete this code-review fix: clone_repository leaves temp_dir undeleted if the git executable check runs after mkdtemp but the initial 'longpaths' config subprocess.run raises an untyped exception.
What the draft fix changed: Added a final `except Exception:` clause in `clone_repository`'s outer try/except (after the existing `TimeoutExpired`, `CalledProcessError`, `FileNotFoundError` handlers) that cleans up `temp_dir` via `cleanup_repository_safe` if it exists and then re-raises the original exception, closing the resource leak for untyped exceptions (e.g. OSError, KeyboardInterrupt) raised during the clone attempt. Note: KeyboardInterrupt is a BaseException, not Exception, so it is still not caught by this handler and temp_dir would still leak in that specific case; a fully complete fix would need a `finally`-based cleanup keyed on success/failure state or a broader `except BaseException`, which was avoided here to keep the change minimal and not swallow interrupts silently.
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

Expand Down Expand Up @@ -158,8 +158,8 @@ def clone_repository(github_url: str) -> str:
capture_output=True,
text=True,
)
except:

@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.

🦩 🟠 Second bare except swallowing sparse-checkout setup errors without logging

In clone_repository, the bare except: pass around the sparse-checkout setup block (config, file write, read-tree) was changed to except Exception as e: logger.debug(f"Non-fatal sparse-checkout setup failed: {e}"), providing a debug log trace instead of silent swallowing.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/cloning.py around line 161, review and complete this code-review fix: Second bare except swallowing sparse-checkout setup errors without logging.
What the draft fix changed: In `clone_repository`, the bare `except: pass` around the sparse-checkout setup block (config, file write, read-tree) was changed to `except Exception as e: logger.debug(f"Non-fatal sparse-checkout setup failed: {e}")`, providing a debug log trace instead of silent swallowing.
Verify the change is correct and complete; do not refactor unrelated code.

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

pass
except Exception as e:
logger.debug(f"Non-fatal sparse-checkout setup failed: {e}")
return temp_dir
except subprocess.TimeoutExpired:
if os.path.exists(temp_dir):
Expand All @@ -178,6 +178,10 @@ def clone_repository(github_url: str) -> str:
f"Git executable not found at '{GIT_EXECUTABLE_PATH}'. "
"Please ensure Git is installed and the path is correct."
)
except Exception:
if os.path.exists(temp_dir):
cleanup_repository_safe(temp_dir)
raise


def cleanup_repository_safe(repo_dir: str) -> bool:
Expand Down
16 changes: 13 additions & 3 deletions codewiki/src/fe/routes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
FastAPI route handlers for the CodeWiki web application.
"""

import logging
import re
from datetime import datetime, timedelta
from pathlib import Path
Comment on lines 3 to 9

@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.

🦩 🟠 codewiki/src/fe/routes.py never defines a module-level logger despite catching/discarding exceptions

Added import logging and logger = logging.getLogger(__name__) at module level in codewiki/src/fe/routes.py, immediately after the imports block, following the CODEWIKI-005/007-2 convention.

🤖 Prompt for AI agents 
In codewiki/src/fe/routes.py around line 1, review and complete this code-review fix: codewiki/src/fe/routes.py never defines a module-level logger despite catching/discarding exceptions.
What the draft fix changed: Added `import logging` and `logger = logging.getLogger(__name__)` at module level in codewiki/src/fe/routes.py, immediately after the imports block, following the CODEWIKI-005/007-2 convention.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -22,6 +23,8 @@
from .config import WebAppConfig
from codewiki.src.utils import file_manager

logger = logging.getLogger(__name__)


class WebRoutes:
"""Handles all web routes for the application."""
Expand Down Expand Up @@ -132,6 +135,7 @@ async def index_post(self, request: Request, repo_url: str = Form(...), commit_i
repo_url = "" # Clear form

except Exception as e:
logger.exception("Failed to add repository %s to queue", normalized_repo_url)
message = f"Failed to add repository to queue: {str(e)}\n{format_exc()}"
message_type = "error"

Expand Down Expand Up @@ -182,6 +186,9 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
if not re.match(r'^[A-Za-z0-9_.-]+$', job_id):
raise HTTPException(status_code=400, detail="Invalid job ID")

if '..' in filename:
raise HTTPException(status_code=400, detail="Invalid file path")

job = self.background_worker.get_job_status(job_id)
docs_path = None
repo_url = None
Expand Down Expand Up @@ -230,7 +237,7 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
try:
module_tree = file_manager.load_json(module_tree_file)
except Exception:
pass
logger.exception("Failed to load module tree from %s", module_tree_file)

# Load metadata
metadata = None
Comment on lines 237 to 243

@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.

🦩 🟠 routes.py uses bare except-pass blocks that silently discard failures instead of logging via module logger

Replaced the silent except Exception: pass blocks in serve_generated_docs (module_tree.json and metadata.json loading) with logger.exception(...) calls that record the failure and file path before continuing with module_tree/metadata left as None. Also added logger.exception(...) to the previously silent _normalize_github_url fallback except-block and to the index_post queueing exception and the serve_generated_docs file-read exception handler, giving every caught exception in the file a log trail while preserving existing control flow (HTTPException raising and pass-through fallback behavior unchanged).

🤖 Prompt for AI agents 
In codewiki/src/fe/routes.py around line 226, review and complete this code-review fix: routes.py uses bare except-pass blocks that silently discard failures instead of logging via module logger.
What the draft fix changed: Replaced the silent `except Exception: pass` blocks in `serve_generated_docs` (module_tree.json and metadata.json loading) with `logger.exception(...)` calls that record the failure and file path before continuing with `module_tree`/`metadata` left as `None`. Also added `logger.exception(...)` to the previously silent `_normalize_github_url` fallback except-block and to the `index_post` queueing exception and the `serve_generated_docs` file-read exception handler, giving every caught exception in the file a log trail while preserving existing control flow (HTTPException raising and pass-through fallback behavior unchanged).
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -239,12 +246,12 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
try:
metadata = file_manager.load_json(metadata_file)
except Exception:
pass
logger.exception("Failed to load metadata from %s", metadata_file)

# Serve the requested file

@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.

🦩 🟠 serve_generated_docs constructs docs_path before validating job_id format, unlike serve_doc's stricter directory-traversal guard

In serve_generated_docs, added an explicit '..' in filename rejection check right after job_id validation, and replaced the manual docs_path_resolved != file_path and docs_path_resolved not in file_path.parents check with file_path.is_relative_to(docs_path_resolved), matching the stricter guard style used in visualise_docs.py. This closes the gap where filename was unchecked before path construction and tightens the containment check. Residual risk: is_relative_to requires Python 3.9+ (assumed available given repo's existing usage patterns elsewhere), and this does not address potential symlink escapes inside docs_path itself (a symlinked file within the docs directory could still resolve outside it) — a complete fix would need docs_path.resolve(strict=True) validation plus symlink auditing, which is out of scope for a minimal fix.

🤖 Prompt for AI agents 
In codewiki/src/fe/routes.py around line 244, review and complete this code-review fix: serve_generated_docs constructs docs_path before validating job_id format, unlike serve_doc's stricter directory-traversal guard.
What the draft fix changed: In `serve_generated_docs`, added an explicit `'..' in filename` rejection check right after job_id validation, and replaced the manual `docs_path_resolved != file_path and docs_path_resolved not in file_path.parents` check with `file_path.is_relative_to(docs_path_resolved)`, matching the stricter guard style used in visualise_docs.py. This closes the gap where `filename` was unchecked before path construction and tightens the containment check. Residual risk: `is_relative_to` requires Python 3.9+ (assumed available given repo's existing usage patterns elsewhere), and this does not address potential symlink escapes inside `docs_path` itself (a symlinked file within the docs directory could still resolve outside it) — a complete fix would need `docs_path.resolve(strict=True)` validation plus symlink auditing, which is out of scope for a minimal fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

docs_path_resolved = docs_path.resolve()
file_path = (docs_path / filename).resolve()
if docs_path_resolved != file_path and docs_path_resolved not in file_path.parents:
if not file_path.is_relative_to(docs_path_resolved):
raise HTTPException(status_code=400, detail="Invalid file path")
if not file_path.exists():
raise HTTPException(status_code=404, detail=f"File {filename} not found")
Expand Down Expand Up @@ -272,6 +279,7 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
return HTMLResponse(content=render_template(DOCS_VIEW_TEMPLATE, context))

except Exception as e:
logger.exception("Error reading %s", filename)
raise HTTPException(status_code=500, detail=f"Error reading {filename}: {e}\n{format_exc()}")

def _normalize_github_url(self, url: str) -> str:
Expand All @@ -281,6 +289,7 @@ def _normalize_github_url(self, url: str) -> str:
repo_info = GitHubRepoProcessor.get_repo_info(url)
return f"https://github.com/{repo_info['full_name']}"
except Exception:
logger.exception("Failed to normalize GitHub URL %s, falling back to basic normalization", url)
# Fallback to basic normalization
return url.rstrip('/').lower()

Expand All @@ -304,3 +313,4 @@ def cleanup_old_jobs(self):
for job_id in expired_jobs:
if job_id in self.background_worker.job_status:
del self.background_worker.job_status[job_id]

17 changes: 10 additions & 7 deletions codewiki/src/fe/web_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"""

import argparse
import logging
from fastapi import FastAPI, Request, Form
from fastapi.responses import HTMLResponse

Expand All @@ -20,6 +21,8 @@
from .config import WebAppConfig


logger = logging.getLogger(__name__)

# Initialize FastAPI app
app = FastAPI(
title="CodeWiki",
Expand Down Expand Up @@ -110,11 +113,11 @@ def main():
# Start background worker
background_worker.start()

print(f"🚀 CodeWiki Web Application starting...")

@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.

🦩 🟠 web_app.py main() uses print() instead of module logger for server startup diagnostics

Replaced all five print() calls in main() (startup banner, server URL, cache dir, temp dir, and the "Press Ctrl+C" hint) with logger.info() calls on a module-level logger created via logging.getLogger(__name__); also replaced the KeyboardInterrupt handler's print("\n👋 Server stopped") with logger.info(...). Added import logging and the logger = logging.getLogger(__name__) module-level declaration, consistent with the established convention referenced in the finding.

🤖 Prompt for AI agents 
In codewiki/src/fe/web_app.py around line 113, review and complete this code-review fix: web_app.py main() uses print() instead of module logger for server startup diagnostics.
What the draft fix changed: Replaced all five print() calls in main() (startup banner, server URL, cache dir, temp dir, and the "Press Ctrl+C" hint) with logger.info() calls on a module-level logger created via `logging.getLogger(__name__)`; also replaced the KeyboardInterrupt handler's print("\n👋 Server stopped") with logger.info(...). Added `import logging` and the `logger = logging.getLogger(__name__)` module-level declaration, consistent with the established convention referenced in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

print(f"🌐 Server running at: http://{args.host}:{args.port}")
print(f"📁 Cache directory: {WebAppConfig.get_absolute_path(WebAppConfig.CACHE_DIR)}")
print(f"🗂️ Temp directory: {WebAppConfig.get_absolute_path(WebAppConfig.TEMP_DIR)}")
print("\nPress Ctrl+C to stop the server")
logger.info("🚀 CodeWiki Web Application starting...")
logger.info(f"🌐 Server running at: http://{args.host}:{args.port}")
logger.info(f"📁 Cache directory: {WebAppConfig.get_absolute_path(WebAppConfig.CACHE_DIR)}")
logger.info(f"🗂️ Temp directory: {WebAppConfig.get_absolute_path(WebAppConfig.TEMP_DIR)}")
logger.info("Press Ctrl+C to stop the server")

try:
uvicorn.run(
Expand All @@ -125,9 +128,9 @@ def main():
log_level="debug" if args.debug else "info"
)
except KeyboardInterrupt:
print("\n👋 Server stopped")
logger.info("👋 Server stopped")
background_worker.stop()


if __name__ == "__main__":
main()
main()
7 changes: 5 additions & 2 deletions test_clustering_forced.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@

import os, sys, logging

# Setup logging
logging.basicConfig(level=logging.INFO, format='[%(levelname)s] %(message)s', force=True)

@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.

🦩 🟠 logging.basicConfig() called directly in a standalone script instead of using centralized setup_logging

Replaced the direct logging.basicConfig(level=logging.INFO, format='[%(levelname)s] %(message)s', force=True) call at module import time with a call to the centralized setup_logging from codewiki.src.be.dependency_analyzer.utils.logging_config, passing the same level, format, and force arguments. This assumes setup_logging accepts these keyword arguments (as implied by CODEWIKI-005/008-2 describing it as the centralized replacement for basicConfig); if its actual signature differs, the call site would need adjusting — this is unverified since the module's source wasn't provided.

🤖 Prompt for AI agents 
In test_clustering_forced.py around line 9, review and complete this code-review fix: logging.basicConfig() called directly in a standalone script instead of using centralized setup_logging.
What the draft fix changed: Replaced the direct `logging.basicConfig(level=logging.INFO, format='[%(levelname)s] %(message)s', force=True)` call at module import time with a call to the centralized `setup_logging` from `codewiki.src.be.dependency_analyzer.utils.logging_config`, passing the same `level`, `format`, and `force` arguments. This assumes `setup_logging` accepts these keyword arguments (as implied by CODEWIKI-005/008-2 describing it as the centralized replacement for `basicConfig`); if its actual signature differs, the call site would need adjusting — this is unverified since the module's source wasn't provided.
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

sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))

from codewiki.src.be.dependency_analyzer.utils.logging_config import setup_logging

# Setup logging
setup_logging(level=logging.INFO, format='[%(levelname)s] %(message)s', force=True)

from dotenv import load_dotenv
load_dotenv('.env.local')

Expand Down