Skip to content

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

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
+23 −18
Draft

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

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
c0ec881
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 28, 2026
c4a30f5
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 28, 2026
0ab04ad
fix(adhoc-sweep-fixes): 3 review findings across 3 files
flamingo[bot] Sep 28, 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
16 changes: 13 additions & 3 deletions codewiki/src/be/dependency_analyzer/utils/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"):

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

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

assert_safe_path(base_dir, target)
flags = os.O_RDONLY
if hasattr(os, "O_NOFOLLOW"):
flags |= os.O_NOFOLLOW
if not hasattr(os, "O_NOFOLLOW"):
raise PermissionError(
f"Cannot safely open {target}: platform does not support O_NOFOLLOW"
)
flags = os.O_RDONLY | os.O_NOFOLLOW
fd = os.open(str(target), flags)
try:
# Re-verify post-open that the opened file descriptor is not a
# symlink and still resolves inside base_dir, closing the TOCTOU
# window between the pre-open check and the open() call.
st = os.fstat(fd)
import stat as _stat
if _stat.S_ISLNK(st.st_mode):
raise PermissionError(f"Symlink blocked: {target}")
assert_safe_path(base_dir, target)
with os.fdopen(fd, "r", encoding=encoding, errors="replace") as f:
return f.read()
finally:
Expand Down
3 changes: 3 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 @@ -91,6 +91,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
Comment on lines 91 to 98

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

Expand All @@ -99,3 +101,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

22 changes: 7 additions & 15 deletions test_clustering_debug.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,16 @@ def capture_llm_response():
"""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()

Comment on lines 22 to 37

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

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

Expand Down