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

[flamingo]

Closes 7 review findings across 6 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟢 92 high	Hardcoded absolute developer path leaked into checked-in test script	test_with_logging.py:24
2	🟢 95 high	_get_supported_languages() omits go/rust/csharp despite _filter_supported_languages() including them	codewiki/src/be/dependency_analyzer/analysis/analysis_service.py:331
3	🟢 95 high	Large commented-out README-reading block left in _read_readme_file alongside the active replacement implementation	codewiki/src/be/dependency_analyzer/analysis/analysis_service.py:255
4	🟢 95 high	logger used before assignment in _run_backend_generation when verbose=True at first use site	codewiki/cli/adapters/doc_generator.py:260
5	🟡 75 medium	job_id used to construct filesystem/cache paths without sanitization, potential path traversal	codewiki/src/fe/routes.py:240
6	🟡 60 medium	APIErrorHandler.handle_api_error's fail_fast parameter is accepted but never used inside the method	codewiki/cli/utils/api_errors.py:14
7	🟡 75 medium	Sparse-checkout exclusion patterns contain suspicious hardcoded hash-like path fragments	codewiki/src/be/dependency_analyzer/analysis/cloning.py:135

What 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: 2cc7a212-e9ac-481a-a76e-5f03d762c00c

Merging 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-86akdypw4 Ad hoc sweep fixes across services (14 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

7 finding(s) fixed in this draft — 7 explained inline on the diff.

[flamingo]

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

[flamingo]

🦩 🟠 _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

[flamingo]

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

[flamingo]

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

[flamingo]

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

[flamingo]

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

[flamingo]

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