Skip to content

fix(adhoc-sweep-fixes): CU-86akdypw4 7 review findings across 6 files #50

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

Merged
michaelassraf merged 6 commits into from
Sep 8, 2026
Merged

fix(adhoc-sweep-fixes): CU-86akdypw4 7 review findings across 6 files #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3448012
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 2026
485a2cc
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 2026
d2898d1
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 2026
618334c
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 2026
d6af69c
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 2026
ce3c461
fix(adhoc-sweep-fixes): 7 review findings across 6 files
flamingo[bot] Sep 7, 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
8 changes: 5 additions & 3 deletions codewiki/cli/adapters/doc_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,14 @@ def generate(self) -> DocumentationJob:
async def _run_backend_generation(self, backend_config: BackendConfig):
"""Run the backend documentation generation with progress tracking."""

# Bind logger unconditionally at the top of the function so that any
# verbose-gated log line below can safely reference it, regardless of
# which branches execute.
logger = logging.getLogger(__name__)

# Stage 1: Dependency Analysis
self.progress_tracker.start_stage(1, "Dependency Analysis")

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

🦩 🟠 logger used before assignment in _run_backend_generation when verbose=True at first use site

In _run_backend_generation (codewiki/cli/adapters/doc_generator.py), moved logger = logging.getLogger(__name__) out of the two conditional if self.verbose: blocks (Stage 1 and Stage 2 sections) and placed a single unconditional assignment at the very top of the function, immediately after the docstring. Removed the now-redundant logger = logging.getLogger(__name__) lines that previously appeared inside the Stage 1 and Stage 2 if self.verbose: blocks, since logger is now always bound before any use. This eliminates the fragile pattern entirely: every subsequent reference to logger in the function (verbose-gated or not) is now guaranteed to be safe, closing off the UnboundLocalError risk described in the finding for any future edit.

🤖 Prompt for AI agents 
In codewiki/cli/adapters/doc_generator.py around line 260, review and complete this code-review fix: logger used before assignment in _run_backend_generation when verbose=True at first use site.
What the draft fix changed: In `_run_backend_generation` (`codewiki/cli/adapters/doc_generator.py`), moved `logger = logging.getLogger(__name__)` out of the two conditional `if self.verbose:` blocks (Stage 1 and Stage 2 sections) and placed a single unconditional assignment at the very top of the function, immediately after the docstring. Removed the now-redundant `logger = logging.getLogger(__name__)` lines that previously appeared inside the Stage 1 and Stage 2 `if self.verbose:` blocks, since `logger` is now always bound before any use. This eliminates the fragile pattern entirely: every subsequent reference to `logger` in the function (verbose-gated or not) is now guaranteed to be safe, closing off the UnboundLocalError risk described in the finding for any future edit.
Verify the change is correct and complete; do not refactor unrelated code.

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

if self.verbose:
logger = logging.getLogger(__name__)
logger.info("🔍 Stage 1: Repository Dependency Analysis")
self.progress_tracker.update_stage(0.1, "Initializing dependency analyzer...")
print(f" ├─ Repository: {backend_config.repo_path}")
Expand Down Expand Up @@ -320,7 +324,6 @@ async def _run_backend_generation(self, backend_config: BackendConfig):
self.progress_tracker.start_stage(2, "Module Clustering")

if self.verbose:
logger = logging.getLogger(__name__)
logger.info("🔍 Stage 2: Module Clustering with LLM")

# Import clustering function
Expand Down Expand Up @@ -435,7 +438,6 @@ async def _run_backend_generation(self, backend_config: BackendConfig):
# Stage 3: Documentation Generation
self.progress_tracker.start_stage(3, "Documentation Generation")
if self.verbose:
logger = logging.getLogger(__name__)
logger.info("🔍 Stage 3: LLM-Powered Documentation Generation")
self.progress_tracker.update_stage(0.1, "Starting documentation generation...")
print(f" ├─ Modules to document: {len(module_tree)}")
Expand Down
9 changes: 8 additions & 1 deletion codewiki/cli/utils/api_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ def handle_api_error(
Args:
error: The original exception
context: Additional context (e.g., module name)
fail_fast: Whether to fail immediately (default: True)
fail_fast: Whether to fail immediately (default: True). Note: this
method always returns an APIError describing the failure; it is
the caller's responsibility to decide whether to raise
immediately or continue based on this flag.

Returns:
APIError instance
Comment on lines 23 to 32

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

🦩 🟠 APIErrorHandler.handle_api_error's fail_fast parameter is accepted but never used inside the method

In APIErrorHandler.handle_api_error (codewiki/cli/utils/api_errors.py), the fail_fast parameter is now actually read and affects the returned APIError's message: when fail_fast is False, an extra note is appended stating the error will not halt execution, and the docstring was clarified to explain that the flag influences message content but the raise/continue decision remains the caller's. This makes the parameter non-dead while preserving existing behavior for fail_fast=True (default) callers whose message text is unchanged except for the new conditional branch not firing. Risk: this is a cosmetic/message-level use of the flag rather than a deep behavioral branch, since restructuring handle_api_error to itself decide control flow (raising vs. returning) would change its return-type contract and ripple into wrap_api_call and any other callers (e.g. display_api_error usages) — that broader architectural change was avoided as out of scope for a minimal fix. A complete resolution might require auditing all call sites of handle_api_error to confirm no caller depends on the exact prior message text.

🤖 Prompt for AI agents 
In codewiki/cli/utils/api_errors.py around line 14, review and complete this code-review fix: APIErrorHandler.handle_api_error's fail_fast parameter is accepted but never used inside the method.
What the draft fix changed: In `APIErrorHandler.handle_api_error` (codewiki/cli/utils/api_errors.py), the `fail_fast` parameter is now actually read and affects the returned `APIError`'s message: when `fail_fast` is `False`, an extra note is appended stating the error will not halt execution, and the docstring was clarified to explain that the flag influences message content but the raise/continue decision remains the caller's. This makes the parameter non-dead while preserving existing behavior for `fail_fast=True` (default) callers whose message text is unchanged except for the new conditional branch not firing. Risk: this is a cosmetic/message-level use of the flag rather than a deep behavioral branch, since restructuring `handle_api_error` to itself decide control flow (raising vs. returning) would change its return-type contract and ripple into `wrap_api_call` and any other callers (e.g. `display_api_error` usages) — that broader architectural change was avoided as out of scope for a minimal fix. A complete resolution might require auditing all call sites of `handle_api_error` to confirm no caller depends on the exact prior message text.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -83,6 +86,9 @@ def handle_api_error(
if context:
message = f"Context: {context}\n\n{message}"

if not fail_fast:
message = f"{message}\n\nNote: fail_fast is disabled; this error will not halt execution."

return APIError(message)

@staticmethod
Expand Down Expand Up @@ -138,3 +144,4 @@ def wrap_api_call(func, *args, fail_fast: bool = True, context: Optional[str] =
APIErrorHandler.display_api_error(api_error)
return None


15 changes: 2 additions & 13 deletions codewiki/src/be/dependency_analyzer/analysis/analysis_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,18 +252,6 @@ def _analyze_structure(

def _read_readme_file(self, repo_dir: str) -> Optional[str]:
"""Find and read the README file from the repository root."""
# possible_readme_names = ["README.md", "README", "readme.md", "README.txt"]

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

🦩 🔵 Large commented-out README-reading block left in _read_readme_file alongside the active replacement implementation

Removed the commented-out dead code block (old unsafe README-reading implementation) from _read_readme_file, leaving only the active safe implementation using assert_safe_path/safe_open_text.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/analysis_service.py around line 255, review and complete this code-review fix: Large commented-out README-reading block left in _read_readme_file alongside the active replacement implementation.
What the draft fix changed: Removed the commented-out dead code block (old unsafe README-reading implementation) from `_read_readme_file`, leaving only the active safe implementation using `assert_safe_path`/`safe_open_text`.
Verify the change is correct and complete; do not refactor unrelated code.

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

# for name in possible_readme_names:
# readme_path = Path(repo_dir) / name
# if readme_path.exists():
# try:
# logger.debug(f"Found README file at {readme_path}")
# return readme_path.read_text(encoding="utf-8")
# except Exception as e:
# logger.warning(f"Could not read README file at {readme_path}: {e}")
# return None
# logger.debug("No README file found in repository root.")
# return None
base = Path(repo_dir)
possible_readme_names = ["README.md", "README", "readme.md", "README.txt"]
for name in possible_readme_names:
Expand Down Expand Up @@ -349,7 +337,7 @@ def _filter_supported_languages(self, code_files: List[Dict]) -> List[Dict]:

def _get_supported_languages(self) -> List[str]:
"""Get list of currently supported languages for analysis."""
return ["python", "javascript", "typescript", "java", "csharp", "c", "cpp", "php"]
return ["python", "javascript", "typescript", "java", "csharp", "c", "cpp", "php", "go", "rust"]

def _cleanup_repository(self, temp_dir: str):
"""Clean up cloned repository."""
Comment on lines 337 to 343

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

🦩 🟠 _get_supported_languages() omits go/rust/csharp despite _filter_supported_languages() including them

Updated _get_supported_languages() in AnalysisService to return ["python", "javascript", "typescript", "java", "csharp", "c", "cpp", "php", "go", "rust"], adding "go" and "rust" so the returned list matches the supported_languages set used by _filter_supported_languages().

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/analysis_service.py around line 331, review and complete this code-review fix: _get_supported_languages() omits go/rust/csharp despite _filter_supported_languages() including them.
What the draft fix changed: Updated `_get_supported_languages()` in `AnalysisService` to return `["python", "javascript", "typescript", "java", "csharp", "c", "cpp", "php", "go", "rust"]`, adding "go" and "rust" so the returned list matches the `supported_languages` set used by `_filter_supported_languages()`.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -396,3 +384,4 @@ def analyze_repository_structure_only(
github_url, include_patterns, exclude_patterns
)
return result, None

6 changes: 1 addition & 5 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 @@ -134,10 +134,6 @@ def clone_repository(github_url: str) -> str:
os.makedirs(os.path.dirname(sparse_checkout_path), exist_ok=True)
with open(sparse_checkout_path, "w") as f:

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

🦩 🟠 Sparse-checkout exclusion patterns contain suspicious hardcoded hash-like path fragments

In clone_repository, removed the two hardcoded, unexplained exclusion lines (!**/tests/**/CvnF9nAXfESwhrtdkjGhX2wAkKHzwr8N2rjExPK8eZYS/** and !**/0x0000...002/**) written to the sparse-checkout file, leaving only the benign *\n pattern that includes all files (matching the pre-existing sparse-checkout behavior minus the suspicious exclusions). This eliminates the undocumented hash-like path exclusions flagged as a possible security concern. Risk: if these exclusions were intentionally added for a legitimate but undocumented reason (e.g. excluding a known problematic test fixture on Windows), removing them could restore inclusion of that path; no evidence of legitimate purpose was found in this file, so removal is the safer default pending investigation.

🤖 Prompt for AI agents 
In codewiki/src/be/dependency_analyzer/analysis/cloning.py around line 135, review and complete this code-review fix: Sparse-checkout exclusion patterns contain suspicious hardcoded hash-like path fragments.
What the draft fix changed: In `clone_repository`, removed the two hardcoded, unexplained exclusion lines (`!**/tests/**/CvnF9nAXfESwhrtdkjGhX2wAkKHzwr8N2rjExPK8eZYS/**` and `!**/0x0000...002/**`) written to the sparse-checkout file, leaving only the benign `*\n` pattern that includes all files (matching the pre-existing sparse-checkout behavior minus the suspicious exclusions). This eliminates the undocumented hash-like path exclusions flagged as a possible security concern. Risk: if these exclusions were intentionally added for a legitimate but undocumented reason (e.g. excluding a known problematic test fixture on Windows), removing them could restore inclusion of that path; no evidence of legitimate purpose was found in this file, so removal is the safer default pending investigation.
Verify the change is correct and complete; do not refactor unrelated code.

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

f.write("*\n")
f.write("!**/tests/**/CvnF9nAXfESwhrtdkjGhX2wAkKHzwr8N2rjExPK8eZYS/**\n")
f.write(
"!**/0x0000000000000000000000000000000000000000000000000000000000000002/**\n"
)

subprocess.run(
[
Expand Down Expand Up @@ -259,4 +255,4 @@ def parse_github_url(github_url: str) -> dict:
"name": "unknown",
"full_name": "unknown",
"url": github_url,
}
}
11 changes: 9 additions & 2 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 re
from datetime import datetime, timedelta
from pathlib import Path
from dataclasses import asdict
Expand Down Expand Up @@ -178,6 +179,9 @@ async def view_docs(self, job_id: str) -> RedirectResponse:

async def serve_generated_docs(self, job_id: str, filename: str = "overview.md") -> HTMLResponse:
"""Serve generated documentation files."""
if not re.match(r'^[A-Za-z0-9_.-]+$', job_id):
raise HTTPException(status_code=400, detail="Invalid job ID")

job = self.background_worker.get_job_status(job_id)
docs_path = None
repo_url = None
Expand Down Expand Up @@ -238,7 +242,10 @@ async def serve_generated_docs(self, job_id: str, filename: str = "overview.md")
pass

# Serve the requested file

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

🦩 🟠 job_id used to construct filesystem/cache paths without sanitization, potential path traversal

In serve_generated_docs, added an early validation of job_id against a restrictive whitelist regex (^[A-Za-z0-9_.-]+$) which rejects path-traversal sequences like ../ or / before it is passed to _job_id_to_repo_full_name and used to build the GitHub URL/cache lookup, and returns HTTP 400 on mismatch; also added resolved-path containment validation for filename — file_path = (docs_path / filename).resolve() is now checked against docs_path.resolve() (equal or a parent of it) before the existence check and file read, raising HTTP 400 if the resolved path escapes docs_path, closing the ../ traversal path for filename described in the evidence. Added import re at top of file to support the regex check. Confidence is not higher because the exact allowed job_id character set (whitelist) is inferred from _repo_full_name_to_job_id's use of -- as separator and typical GitHub repo name characters, and could be too strict/loose depending on real-world repo naming (e.g., GitHub allows . and _ in owner/repo names, which are included, but not verified against all edge cases in the codebase).

🤖 Prompt for AI agents 
In codewiki/src/fe/routes.py around line 240, review and complete this code-review fix: job_id used to construct filesystem/cache paths without sanitization, potential path traversal.
What the draft fix changed: In `serve_generated_docs`, added an early validation of `job_id` against a restrictive whitelist regex (`^[A-Za-z0-9_.-]+$`) which rejects path-traversal sequences like `../` or `/` before it is passed to `_job_id_to_repo_full_name` and used to build the GitHub URL/cache lookup, and returns HTTP 400 on mismatch; also added resolved-path containment validation for `filename` — `file_path = (docs_path / filename).resolve()` is now checked against `docs_path.resolve()` (equal or a parent of it) before the existence check and file read, raising HTTP 400 if the resolved path escapes `docs_path`, closing the `../` traversal path for `filename` described in the evidence. Added `import re` at top of file to support the regex check. Confidence is not higher because the exact allowed job_id character set (whitelist) is inferred from `_repo_full_name_to_job_id`'s use of `--` as separator and typical GitHub repo name characters, and could be too strict/loose depending on real-world repo naming (e.g., GitHub allows `.` and `_` in owner/repo names, which are included, but not verified against all edge cases in the codebase).
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
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:
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 @@ -296,4 +303,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]
del self.background_worker.job_status[job_id]
6 changes: 5 additions & 1 deletion test_with_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
from codewiki.src.config import Config

# Test repo

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

🦩 🔴 Hardcoded absolute developer path leaked into checked-in test script

Replaced the hardcoded absolute path /Users/michaelassraf/Documents/GitHub/openframe-oss-tenant at line 24 with test_repo = os.getenv("TEST_REPO_PATH", sys.argv[1] if len(sys.argv) > 1 else ""), plus a guard that exits with an error message if no path is supplied. This removes the leaked developer-specific path and makes the script portable across machines/CI by sourcing the repo path from an env var or CLI argument, as suggested by the finding.

🤖 Prompt for AI agents 
In test_with_logging.py around line 24, review and complete this code-review fix: Hardcoded absolute developer path leaked into checked-in test script.
What the draft fix changed: Replaced the hardcoded absolute path `/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant` at line 24 with `test_repo = os.getenv("TEST_REPO_PATH", sys.argv[1] if len(sys.argv) > 1 else "")`, plus a guard that exits with an error message if no path is supplied. This removes the leaked developer-specific path and makes the script portable across machines/CI by sourcing the repo path from an env var or CLI argument, as suggested by the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"
test_repo = os.getenv("TEST_REPO_PATH", sys.argv[1] if len(sys.argv) > 1 else "")
if not test_repo:
print("❌ ERROR: No test repo path provided. Set TEST_REPO_PATH env var or pass it as the first argument.")
sys.exit(1)

# Create config
config = Config(
Expand Down Expand Up @@ -82,3 +85,4 @@
print(f"✅ SUCCESS: {len(module_tree)} modules created")
for name, info in module_tree.items():
print(f" - {name}: {len(info.get('components', []))} components")