fix(adhoc-sweep-fixes): 9 review findings across 7 files - #36
fix(adhoc-sweep-fixes): 9 review findings across 7 files#36flamingo[bot] wants to merge 7 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| return self.failed == 0 | ||
|
|
||
|
|
||
| def simulate_validation(response_content: str, max_id: int): |
There was a problem hiding this comment.
🦩 🔴 Validation logic in test_clustering_validation.py has silently drifted from the real implementation in cluster_modules.py
Removed the hand-copied validation block from simulate_validation() and replaced it with a call to a new imported function validate_cluster_response from codewiki/src/be/cluster_modules.py, adjusting sys.path to locate that module. This assumes cluster_modules.py exposes (or can be refactored to expose) a validate_cluster_response(response_content, max_id) function returning (success, module_tree_or_None). Since I cannot see cluster_modules.py's actual current structure, this fix is INCOMPLETE without a corresponding extraction of the validation logic into that named function in cluster_modules.py — if no such function exists there yet, this import will fail at runtime and someone must add validate_cluster_response to cluster_modules.py (extracting lines 338-369 referenced in the original docstring) for this test to work.
🤖 Prompt for AI agents
In test_clustering_validation.py around line 14, review and complete this code-review fix: Validation logic in test_clustering_validation.py has silently drifted from the real implementation in cluster_modules.py.
What the draft fix changed: Removed the hand-copied validation block from `simulate_validation()` and replaced it with a call to a new imported function `validate_cluster_response` from `codewiki/src/be/cluster_modules.py`, adjusting `sys.path` to locate that module. This assumes `cluster_modules.py` exposes (or can be refactored to expose) a `validate_cluster_response(response_content, max_id)` function returning `(success, module_tree_or_None)`. Since I cannot see `cluster_modules.py`'s actual current structure, this fix is INCOMPLETE without a corresponding extraction of the validation logic into that named function in `cluster_modules.py` — if no such function exists there yet, this import will fail at runtime and someone must add `validate_cluster_response` to `cluster_modules.py` (extracting lines 338-369 referenced in the original docstring) for this test to work.
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
| @@ -136,8 +153,7 @@ def run_tests(): | |||
| print("CODEWIKI CLUSTERING VALIDATION TEST SUITE") | |||
| print("="*70) | |||
|
|
|||
There was a problem hiding this comment.
🦩 🟠 test_clustering_validation.py uses ad-hoc print/logger asserts instead of TestResults accumulator
Replaced manual passed/failed counters and ad-hoc print/logger summary in run_tests() with a new TestResults class providing add_test() and print_summary(), matching the repo's established pattern; run_tests() now calls results.add_test(...) per test case and results.print_summary() at the end, with exit(0 if success else 1) driven by results.success.
🤖 Prompt for AI agents
In test_clustering_validation.py around line 138, review and complete this code-review fix: test_clustering_validation.py uses ad-hoc print/logger asserts instead of TestResults accumulator.
What the draft fix changed: Replaced manual `passed`/`failed` counters and ad-hoc print/logger summary in `run_tests()` with a new `TestResults` class providing `add_test()` and `print_summary()`, matching the repo's established pattern; `run_tests()` now calls `results.add_test(...)` per test case and `results.print_summary()` at the end, with `exit(0 if success else 1)` driven by `results.success`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| if '.' in comp_id: | ||
| # Try matching last 2-4 segments | ||
| segments = comp_id.split('.') | ||
| suffix_matches = [] | ||
| for n in range(2, min(5, len(segments) + 1)): | ||
| suffix = '.'.join(segments[-n:]) | ||
| suffix_matches = [ |
There was a problem hiding this comment.
🦩 🟠 Possible UnboundLocalError: suffix_matches referenced outside its defining loop scope in FQDN_NORMALIZATION_FIX.py
Fixed the UnboundLocalError risk in normalize_component_ids_enhanced (Strategy 5 block): added suffix_matches = [] initialization immediately before the for n in range(2, min(5, len(segments) + 1)): loop, so the subsequent if suffix_matches and len(suffix_matches) == 1: check outside the loop is always safe even in edge cases where the loop body doesn't execute or suffix_matches would otherwise be undefined.
🤖 Prompt for AI agents
In FQDN_NORMALIZATION_FIX.py around line 131, review and complete this code-review fix: Possible UnboundLocalError: `suffix_matches` referenced outside its defining loop scope in FQDN_NORMALIZATION_FIX.py.
What the draft fix changed: Fixed the UnboundLocalError risk in `normalize_component_ids_enhanced` (Strategy 5 block): added `suffix_matches = []` initialization immediately before the `for n in range(2, min(5, len(segments) + 1)):` loop, so the subsequent `if suffix_matches and len(suffix_matches) == 1:` check outside the loop is always safe even in edge cases where the loop body doesn't execute or `suffix_matches` would otherwise be undefined.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -1,6 +1,11 @@ | |||
| """ | |||
There was a problem hiding this comment.
🦩 🟠 FQDN_NORMALIZATION_FIX.py at repo root lacks proper module context and pollutes top-level namespace
Addressed the documentation/placement finding by adding a NOTE paragraph to the module docstring at the top of the file explaining that this is a standalone reference/patch proposal intended for integration into codewiki/src/be/cluster_modules.py, and that it should be merged there or removed. I did not physically move/delete the file or merge it into codewiki/src/be/cluster_modules.py since that is a cross-file architectural change outside the scope of editing this single file; a complete fix would require actually relocating/integrating the code and deleting this root-level file, which a human should decide and perform as a follow-up.
🤖 Prompt for AI agents
In FQDN_NORMALIZATION_FIX.py around line 1, review and complete this code-review fix: FQDN_NORMALIZATION_FIX.py at repo root lacks proper module context and pollutes top-level namespace.
What the draft fix changed: Addressed the documentation/placement finding by adding a NOTE paragraph to the module docstring at the top of the file explaining that this is a standalone reference/patch proposal intended for integration into `codewiki/src/be/cluster_modules.py`, and that it should be merged there or removed. I did not physically move/delete the file or merge it into `codewiki/src/be/cluster_modules.py` since that is a cross-file architectural change outside the scope of editing this single file; a complete fix would require actually relocating/integrating the code and deleting this root-level file, which a human should decide and perform as a follow-up.
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
| provider=OpenAIProvider( | ||
| base_url=base_url, | ||
| api_key=api_key, | ||
| # default_headers removed - use http_client if needed | ||
| default_headers=default_headers if default_headers else None, | ||
| ), | ||
| settings=OpenAIModelSettings(**settings_dict) | ||
| ) |
There was a problem hiding this comment.
🦩 🟠 default_headers dict for Anthropic api-version is built but never passed to the OpenAIProvider/OpenAI client
In create_main_model, the default_headers dict populated with anthropic-version (when main_api_version is set) is now passed to OpenAIProvider(...) via default_headers=default_headers if default_headers else None, replacing the stale "default_headers removed" comment. Same mechanism applied identically in create_fallback_model (using fallback_api_version), create_cluster_model (using cluster_api_version), and create_openai_client (passed to OpenAI(...), using the per-stage api_version resolved earlier in that function). Unverified: whether the installed OpenAIProvider version in this environment actually accepts a default_headers kwarg (pydantic-ai provider APIs have changed across versions) and whether OpenAI() client's default_headers param name/behavior matches expectations — if the provider signature differs, this would raise a TypeError at call time; a complete fix would need to verify against the installed pydantic-ai/openai package versions or fall back to passing an http_client with headers set if default_headers is unsupported.
🤖 Prompt for AI agents
In codewiki/src/be/llm_services.py around line 118, review and complete this code-review fix: default_headers dict for Anthropic api-version is built but never passed to the OpenAIProvider/OpenAI client.
What the draft fix changed: In `create_main_model`, the `default_headers` dict populated with `anthropic-version` (when `main_api_version` is set) is now passed to `OpenAIProvider(...)` via `default_headers=default_headers if default_headers else None`, replacing the stale "default_headers removed" comment. Same mechanism applied identically in `create_fallback_model` (using `fallback_api_version`), `create_cluster_model` (using `cluster_api_version`), and `create_openai_client` (passed to `OpenAI(...)`, using the per-stage `api_version` resolved earlier in that function). Unverified: whether the installed `OpenAIProvider` version in this environment actually accepts a `default_headers` kwarg (pydantic-ai provider APIs have changed across versions) and whether `OpenAI()` client's `default_headers` param name/behavior matches expectations — if the provider signature differs, this would raise a `TypeError` at call time; a complete fix would need to verify against the installed pydantic-ai/openai package versions or fall back to passing an `http_client` with headers set if `default_headers` is unsupported.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
|
|
||
| def build_short_id_to_fqdn_map(components): | ||
| """ |
There was a problem hiding this comment.
🦩 🟠 build_short_id_to_fqdn_map logic duplicated between test_normalization_simple.py and codewiki.src.be.cluster_modules
Removed the local duplicated build_short_id_to_fqdn_map function and its collections.defaultdict import in test_normalization_simple.py, replacing it with from codewiki.src.be.cluster_modules import build_short_id_to_fqdn_map. The rest of test_normalization is unchanged and now exercises the real implementation. Confidence is not higher because I cannot verify in this environment that the import path codewiki.src.be.cluster_modules resolves correctly relative to this test file's location/package structure, or that the real function's signature/return format exactly matches usage here (e.g., whether it also requires additional arguments or prints logging that changes expected test output); a complete fix would require running the test to confirm.
🤖 Prompt for AI agents
In test_normalization_simple.py around line 12, review and complete this code-review fix: build_short_id_to_fqdn_map logic duplicated between test_normalization_simple.py and codewiki.src.be.cluster_modules.
What the draft fix changed: Removed the local duplicated `build_short_id_to_fqdn_map` function and its `collections.defaultdict` import in `test_normalization_simple.py`, replacing it with `from codewiki.src.be.cluster_modules import build_short_id_to_fqdn_map`. The rest of `test_normalization` is unchanged and now exercises the real implementation. Confidence is not higher because I cannot verify in this environment that the import path `codewiki.src.be.cluster_modules` resolves correctly relative to this test file's location/package structure, or that the real function's signature/return format exactly matches usage here (e.g., whether it also requires additional arguments or prints logging that changes expected test output); a complete fix would require running the test to confirm.
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
| """ | ||
| from codewiki.src.be.dependency_analyzer.analyzers.c import analyze_c_file | ||
|
|
||
| functions, relationships = analyze_c_file(file_path, content, repo_path=repo_dir) | ||
| try: | ||
| functions, relationships = analyze_c_file(file_path, content, repo_path=repo_dir) | ||
|
|
||
| for func in functions: | ||
| func_id = func.id if func.id else f"{file_path}:{func.name}" | ||
| self.functions[func_id] = func | ||
| for func in functions: | ||
| func_id = func.id if func.id else f"{file_path}:{func.name}" | ||
| self.functions[func_id] = func | ||
|
|
||
| self.call_relationships.extend(relationships) | ||
| self.call_relationships.extend(relationships) | ||
| except Exception as e: | ||
| logger.error(f"Failed to analyze C file {file_path}: {e}", exc_info=True) | ||
|
|
||
| def _analyze_cpp_file(self, file_path: str, content: str, repo_dir: str): | ||
| """ |
There was a problem hiding this comment.
🦩 🟠 _analyze_c_file and _analyze_cpp_file lack try/except unlike every other language handler
Wrapped the body of _analyze_c_file in a try/except block mirroring the other language handlers (e.g. _analyze_java_file), logging failures via logger.error(f"Failed to analyze C file {file_path}: {e}", exc_info=True), so a malformed C file no longer bypasses the per-file safety net. The same finding also covers _analyze_cpp_file, which was fixed identically: wrapped its body in try/except with logger.error(f"Failed to analyze C++ file {file_path}: {e}", exc_info=True).
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/analysis/call_graph_analyzer.py around line 208, review and complete this code-review fix: _analyze_c_file and _analyze_cpp_file lack try/except unlike every other language handler.
What the draft fix changed: Wrapped the body of `_analyze_c_file` in a try/except block mirroring the other language handlers (e.g. `_analyze_java_file`), logging failures via `logger.error(f"Failed to analyze C file {file_path}: {e}", exc_info=True)`, so a malformed C file no longer bypasses the per-file safety net. The same finding also covers `_analyze_cpp_file`, which was fixed identically: wrapped its body in try/except with `logger.error(f"Failed to analyze C++ file {file_path}: {e}", exc_info=True)`.
_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer
| @@ -154,7 +154,7 @@ async def serve_doc(filename: str): | |||
| try: | |||
| file_path = file_path.resolve() | |||
| docs_folder_resolved = Path(DOCS_FOLDER).resolve() | |||
There was a problem hiding this comment.
🦩 🟠 serve_doc path-containment check is string-prefix based, vulnerable to sibling-directory bypass
In serve_doc, replaced the string-prefix containment check (str(file_path).startswith(str(docs_folder_resolved))) with file_path.is_relative_to(docs_folder_resolved), matching the safer pattern already used by security.py's _inside helper. This closes the sibling-directory bypass (e.g. /data/docs-evil/secret.md no longer passes the check against /data/docs) while preserving existing exception handling and control flow.
🤖 Prompt for AI agents
In codewiki/src/fe/visualise_docs.py around line 156, review and complete this code-review fix: serve_doc path-containment check is string-prefix based, vulnerable to sibling-directory bypass.
What the draft fix changed: In `serve_doc`, replaced the string-prefix containment check (`str(file_path).startswith(str(docs_folder_resolved))`) with `file_path.is_relative_to(docs_folder_resolved)`, matching the safer pattern already used by `security.py`'s `_inside` helper. This closes the sibling-directory bypass (e.g. `/data/docs-evil/secret.md` no longer passes the check against `/data/docs`) while preserving existing exception handling and control flow.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| CACHE_EXPIRY_DAYS = 365 | ||
|
|
||
| # Job cleanup settings | ||
| JOB_CLEANUP_HOURS = 24000 | ||
| JOB_CLEANUP_HOURS = 24 | ||
| RETRY_COOLDOWN_MINUTES = 3 | ||
|
|
||
| # Server settings |
There was a problem hiding this comment.
🦩 🔵 WebAppConfig.JOB_CLEANUP_HOURS set to 24000 hours (~2.7 years), likely a typo for 24 hours
Changed WebAppConfig.JOB_CLEANUP_HOURS from 24000 to 24 in codewiki/src/fe/config.py, restoring the likely intended value (24 hours) so cleanup_old_jobs() actually prunes job_status entries on a realistic schedule instead of effectively never expiring them.
🤖 Prompt for AI agents
In codewiki/src/fe/config.py around line 20, review and complete this code-review fix: WebAppConfig.JOB_CLEANUP_HOURS set to 24000 hours (~2.7 years), likely a typo for 24 hours.
What the draft fix changed: Changed `WebAppConfig.JOB_CLEANUP_HOURS` from 24000 to 24 in `codewiki/src/fe/config.py`, restoring the likely intended value (24 hours) so `cleanup_old_jobs()` actually prunes `job_status` entries on a realistic schedule instead of effectively never expiring them.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
Closes 9 review findings across 7 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
test_clustering_validation.py:14test_clustering_validation.py:138suffix_matchesreferenced outside its defining loop scope in FQDN_NORMALIZATION_FIX.pyFQDN_NORMALIZATION_FIX.py:131FQDN_NORMALIZATION_FIX.py:1codewiki/src/be/llm_services.py:118test_normalization_simple.py:12codewiki/src/be/dependency_analyzer/analysis/call_graph_analyzer.py:208codewiki/src/fe/visualise_docs.py:156codewiki/src/fe/config.py:20What 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:
bef4f5a8-e7f3-478b-8731-2becca2de658Merging 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.