Skip to content

fix(adhoc-sweep-fixes): CU-86akn96pk 3 review findings across 3 files #94

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

fix(adhoc-sweep-fixes): CU-86akn96pk 3 review findings across 3 files #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8766a9
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 21, 2026
83d3ad7
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 21, 2026
02d10e2
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 21, 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
6 changes: 6 additions & 0 deletions codewiki/cli/utils/repo_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@

from pathlib import Path
from typing import Tuple, List
import logging
import os

from codewiki.cli.utils.errors import RepositoryError
from codewiki.cli.utils.validation import validate_repository_path, detect_supported_languages


logger = logging.getLogger(__name__)


# Supported file extensions by language
SUPPORTED_EXTENSIONS = {
'.py', # Python
Expand Down Expand Up @@ -146,6 +150,7 @@ def get_git_commit_hash(repo_path: Path) -> str:
repo = git.Repo(repo_path)
return repo.head.commit.hexsha
except Exception:
logger.debug("Failed to read git commit hash for %s", repo_path, exc_info=True)
return ""


Comment on lines 150 to 156

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

🦩 🟠 repo_validator.py silently swallows all exceptions when reading git metadata, hiding real errors

In get_git_commit_hash and get_git_branch, added logger.debug(..., exc_info=True) calls in the except Exception: blocks before returning "", and added a module-level logger = logging.getLogger(__name__) plus import logging. This logs the swallowed exception details at debug level so real errors (corrupted .git, missing dependency, permissions) are traceable without changing observable behavior (still returns empty string).

🤖 Prompt for AI agents 
In codewiki/cli/utils/repo_validator.py around line 144, review and complete this code-review fix: repo_validator.py silently swallows all exceptions when reading git metadata, hiding real errors.
What the draft fix changed: In `get_git_commit_hash` and `get_git_branch`, added `logger.debug(..., exc_info=True)` calls in the `except Exception:` blocks before returning `""`, and added a module-level `logger = logging.getLogger(__name__)` plus `import logging`. This logs the swallowed exception details at debug level so real errors (corrupted .git, missing dependency, permissions) are traceable without changing observable behavior (still returns empty string).
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -167,6 +172,7 @@ def get_git_branch(repo_path: Path) -> str:
repo = git.Repo(repo_path)
return repo.active_branch.name
except Exception:
logger.debug("Failed to read git branch for %s", repo_path, exc_info=True)
return ""


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
Comment on lines 71 to 78

@flamingo flamingo Bot Sep 21, 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 target_dir cleanup inconsistent between shallow and full clone failure paths

In GitHubRepoProcessor.clone_repository, added shutil.rmtree(target_dir, ignore_errors=True) cleanup (guarded by os.path.isdir(target_dir)) to both the commit_id branch's initial git clone failure path and the shallow-clone branch's git clone failure path, matching the existing cleanup behavior already present in the checkout-failure path and the outer except Exception handler. This ensures partial/corrupted clone directories are removed consistently on any clone failure.

🤖 Prompt for AI agents 
In codewiki/src/fe/github_processor.py around line 66, review and complete this code-review fix: clone_repository leaves target_dir cleanup inconsistent between shallow and full clone failure paths.
What the draft fix changed: In GitHubRepoProcessor.clone_repository, added `shutil.rmtree(target_dir, ignore_errors=True)` cleanup (guarded by `os.path.isdir(target_dir)`) to both the commit_id branch's initial `git clone` failure path and the shallow-clone branch's `git clone` failure path, matching the existing cleanup behavior already present in the checkout-failure path and the outer `except Exception` handler. This ensures partial/corrupted clone directories are removed consistently on any clone failure.
Verify the change is correct and complete; do not refactor unrelated code.

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

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

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 @@ -242,10 +242,19 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
pass

# Serve the requested file
docs_path_resolved = docs_path.resolve()

@flamingo flamingo Bot Sep 21, 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 path-traversal guard only checks parents/equality, not symlink resolution, and differs from visualise_docs.py's is_relative_to check

In serve_generated_docs (routes.py), replaced the docs_path_resolved != file_path and docs_path_resolved not in file_path.parents containment check with file_path.is_relative_to(docs_path_resolved), matching the approach used in visualise_docs.py's serve_doc. Added a .md extension allowlist check on filename before resolving paths, and wrapped the .resolve() calls plus containment check in a try/except that raises HTTPException(400, "Invalid file path") on any exception, mirroring the defensive try/except pattern in visualise_docs.py. This brings the two nearly-identical handlers into alignment. Risk: I did not verify whether existing callers rely on serving non-.md files (e.g. module_tree.json/metadata.json are loaded separately via file_manager.load_json, not through this served-file path, so this should be safe) — the reviewer should confirm no legitimate non-.md filenames are ever routed through this endpoint before merging.

🤖 Prompt for AI agents 
In codewiki/src/fe/routes.py around line 245, review and complete this code-review fix: serve_generated_docs path-traversal guard only checks parents/equality, not symlink resolution, and differs from visualise_docs.py's is_relative_to check.
What the draft fix changed: In `serve_generated_docs` (routes.py), replaced the `docs_path_resolved != file_path and docs_path_resolved not in file_path.parents` containment check with `file_path.is_relative_to(docs_path_resolved)`, matching the approach used in visualise_docs.py's `serve_doc`. Added a `.md` extension allowlist check on `filename` before resolving paths, and wrapped the `.resolve()` calls plus containment check in a try/except that raises `HTTPException(400, "Invalid file path")` on any exception, mirroring the defensive try/except pattern in visualise_docs.py. This brings the two nearly-identical handlers into alignment. Risk: I did not verify whether existing callers rely on serving non-`.md` files (e.g. `module_tree.json`/`metadata.json` are loaded separately via `file_manager.load_json`, not through this served-file path, so this should be safe) — the reviewer should confirm no legitimate non-`.md` filenames are ever routed through this endpoint before merging.
Verify the change is correct and complete; do not refactor unrelated code.

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

file_path = (docs_path / filename).resolve()
if docs_path_resolved != file_path and docs_path_resolved not in file_path.parents:
if not filename.endswith('.md'):
raise HTTPException(status_code=400, detail="Invalid file path")

try:
docs_path_resolved = docs_path.resolve()
file_path = (docs_path / filename).resolve()
if not file_path.is_relative_to(docs_path_resolved):
raise HTTPException(status_code=400, detail="Invalid file path")
except HTTPException:
raise
except Exception:
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 @@ -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]