fix(adhoc-sweep-fixes): CU-86akn96pk 3 review findings across 3 files - #111
flamingo[bot] wants to merge 3 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| """Monkey patch to capture LLM response.""" | ||
| global captured_response | ||
| from codewiki.src.be import cluster_modules as cm_module | ||
| from codewiki.src.be.llm_services import create_llm_client | ||
|
|
||
| original_create = create_llm_client | ||
| original_call_llm = cm_module.call_llm | ||
|
|
||
| def patched_create(*args, **kwargs): | ||
| client = original_create(*args, **kwargs) | ||
| original_call = client.call | ||
| def patched_call_llm(*call_args, **call_kwargs): | ||
| result = original_call_llm(*call_args, **call_kwargs) | ||
| global captured_response | ||
| captured_response = result | ||
| return result | ||
|
|
||
| def patched_call(*call_args, **call_kwargs): | ||
| result = original_call(*call_args, **call_kwargs) | ||
| global captured_response | ||
| captured_response = result | ||
| return result | ||
|
|
||
| client.call = patched_call | ||
| return client | ||
|
|
||
| cm_module.create_llm_client = patched_create | ||
| cm_module.call_llm = patched_call_llm | ||
|
|
||
| capture_llm_response() | ||
|
|
There was a problem hiding this comment.
🦩 🔴 test_clustering_debug.py monkey-patches a non-existent function create_llm_client
Rewrote capture_llm_response() in test_clustering_debug.py to monkey-patch cm_module.call_llm (the name cluster_modules.py actually imports and calls) instead of the nonexistent create_llm_client. The patch now wraps cm_module.call_llm directly with a function that calls the original call_llm, stores its return value into the module-global captured_response, and returns it unchanged — removing the dead create_llm_client import/client-wrapping logic entirely. This assumes cluster_modules.py calls call_llm via the module attribute (cm_module.call_llm(...) or an unqualified call_llm(...) resolved from module globals) rather than via a rebound local import; if cluster_modules.py does from codewiki.src.be.llm_services import call_llm and calls it as a bare name, patching cm_module.call_llm still works because Python looks up globals at call time. A complete verification would require seeing cluster_modules.py's exact call site to confirm the signature/return type matches what's captured here (assumed to be the raw response string, consistent with prior code's use of captured_response[:2000] and <GROUPED_COMPONENTS> substring checks).
🤖 Prompt for AI agents
In test_clustering_debug.py around line 21, review and complete this code-review fix: test_clustering_debug.py monkey-patches a non-existent function `create_llm_client`.
What the draft fix changed: Rewrote `capture_llm_response()` in `test_clustering_debug.py` to monkey-patch `cm_module.call_llm` (the name `cluster_modules.py` actually imports and calls) instead of the nonexistent `create_llm_client`. The patch now wraps `cm_module.call_llm` directly with a function that calls the original `call_llm`, stores its return value into the module-global `captured_response`, and returns it unchanged — removing the dead `create_llm_client` import/client-wrapping logic entirely. This assumes `cluster_modules.py` calls `call_llm` via the module attribute (`cm_module.call_llm(...)` or an unqualified `call_llm(...)` resolved from module globals) rather than via a rebound local import; if `cluster_modules.py` does `from codewiki.src.be.llm_services import call_llm` and calls it as a bare name, patching `cm_module.call_llm` still works because Python looks up globals at call time. A complete verification would require seeing `cluster_modules.py`'s exact call site to confirm the signature/return type matches what's captured here (assumed to be the raw response string, consistent with prior code's use of `captured_response[:2000]` and `<GROUPED_COMPONENTS>` substring checks).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
| 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 |
There was a problem hiding this comment.
🦩 🟠 clone_repository leaves a partially-cloned directory on shallow-clone failure
In clone_repository, the shallow-clone (else) branch now calls shutil.rmtree(target_dir, ignore_errors=True) (guarded by os.path.isdir(target_dir)) before returning False on result.returncode != 0, mirroring the cleanup already done in the commit_id branch, preventing stale/partial directories from blocking future clone attempts.
🤖 Prompt for AI agents
In codewiki/src/fe/github_processor.py around line 86, review and complete this code-review fix: clone_repository leaves a partially-cloned directory on shallow-clone failure.
What the draft fix changed: In `clone_repository`, the shallow-clone (`else`) branch now calls `shutil.rmtree(target_dir, ignore_errors=True)` (guarded by `os.path.isdir(target_dir)`) before returning False on `result.returncode != 0`, mirroring the cleanup already done in the `commit_id` branch, preventing stale/partial directories from blocking future clone attempts.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
| @@ -27,11 +27,21 @@ def assert_safe_path(base_dir: Path, target: Path): | |||
|
|
|||
| def safe_open_text(base_dir: Path, target: Path, encoding="utf-8"): | |||
There was a problem hiding this comment.
🦩 🟠 safe_open_text ignores O_NOFOLLOW fallback risk and permits TOCTOU between symlink check and open
In safe_open_text(), removed the conditional O_NOFOLLOW fallback: the function now raises PermissionError immediately if os.O_NOFOLLOW is unsupported on the platform, instead of silently opening without it. Additionally, after os.open() succeeds, the code now uses os.fstat() on the resulting file descriptor to verify (via stat.S_ISLNK) that the opened file is not a symlink, and re-runs assert_safe_path() against the target to re-check containment, narrowing (though not eliminating, since path-based re-check is still name-based) the TOCTOU window between the initial check and the open. A fully race-proof fix would require resolving containment purely from the fd (e.g. via /proc/self/fd or os.path realpath of the fd on platforms that support it), which is not attempted here since no such helper exists in this file or repo.
🤖 Prompt for AI agents
In codewiki/src/be/dependency_analyzer/utils/security.py around line 28, review and complete this code-review fix: safe_open_text ignores O_NOFOLLOW fallback risk and permits TOCTOU between symlink check and open.
What the draft fix changed: In safe_open_text(), removed the conditional O_NOFOLLOW fallback: the function now raises PermissionError immediately if os.O_NOFOLLOW is unsupported on the platform, instead of silently opening without it. Additionally, after os.open() succeeds, the code now uses os.fstat() on the resulting file descriptor to verify (via stat.S_ISLNK) that the opened file is not a symlink, and re-runs assert_safe_path() against the target to re-check containment, narrowing (though not eliminating, since path-based re-check is still name-based) the TOCTOU window between the initial check and the open. A fully race-proof fix would require resolving containment purely from the fd (e.g. via /proc/self/fd or os.path realpath of the fd on platforms that support it), which is not attempted here since no such helper exists in this file or repo.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 3 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
create_llm_clienttest_clustering_debug.py:21codewiki/src/fe/github_processor.py:86codewiki/src/be/dependency_analyzer/utils/security.py:28What 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:
35794e5e-98a9-41e6-ac3b-f4c4c16f8e9dMerging 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-86akn96pk CodeWiki review findings sweep (10 PRs)