Skip to content

fix(CODEWIKI-009-2): CU-86akbhhru 4 review findings across 4 files #53

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 4 commits into from
Sep 8, 2026
+10 −3
Merged

fix(CODEWIKI-009-2): CU-86akbhhru 4 review findings across 4 files #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3d38dc
fix(CODEWIKI-009-2): 4 review findings across 4 files
flamingo[bot] Sep 7, 2026
0cd39b3
fix(CODEWIKI-009-2): 4 review findings across 4 files
flamingo[bot] Sep 7, 2026
5b07e46
fix(CODEWIKI-009-2): 4 review findings across 4 files
flamingo[bot] Sep 7, 2026
b8d0616
fix(CODEWIKI-009-2): 4 review findings across 4 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
3 changes: 2 additions & 1 deletion test_clustering_proof.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from codewiki.src.be.dependency_analyzer.models.core import Node
from codewiki.src.config import Config

test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"

@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.

🦩 🟠 test_clustering_proof.py hardcodes a developer's local absolute filesystem path

Replaced the hardcoded absolute path /Users/michaelassraf/Documents/GitHub/openframe-oss-tenant on the test_repo assignment with os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))), mirroring the file's existing dynamic-path convention used for sys.path.insert(0, ...). This makes the script portable across machines by default (falling back to the script's own directory) while still allowing an explicit override via the TEST_REPO_PATH environment variable. Since test_repo is only used to build synthetic, non-existent file paths for Node objects (not read from disk), this substitution does not change runtime behavior on any machine.

🤖 Prompt for AI agents 
In test_clustering_proof.py around line 13, review and complete this code-review fix: test_clustering_proof.py hardcodes a developer's local absolute filesystem path.
What the draft fix changed: Replaced the hardcoded absolute path `/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant` on the `test_repo` assignment with `os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__)))`, mirroring the file's existing dynamic-path convention used for `sys.path.insert(0, ...)`. This makes the script portable across machines by default (falling back to the script's own directory) while still allowing an explicit override via the `TEST_REPO_PATH` environment variable. Since `test_repo` is only used to build synthetic, non-existent file paths for `Node` objects (not read from disk), this substitution does not change runtime behavior on any machine.
Verify the change is correct and complete; do not refactor unrelated code.

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

test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__)))

config = Config(
repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps",
Expand Down Expand Up @@ -96,3 +96,4 @@
more = len(info.get('components', [])) - 5
print(f" - {name}: {comp_count} components {comp_list}{'...' if more > 0 else ''}")
sys.exit(0)

2 changes: 1 addition & 1 deletion test_clustering_real.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from codewiki.src.be.dependency_analyzer.models.core import Node
from codewiki.src.config import Config

test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"

@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.

🦩 🟠 test_clustering_real.py hardcodes a developer's local absolute filesystem path

Changed test_repo in the module-level script body from the hardcoded absolute path "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" to os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__))), so the path is now configurable via an environment variable and defaults to a portable, machine-independent location (the script's own directory) rather than a developer's local filesystem path.

🤖 Prompt for AI agents 
In test_clustering_real.py around line 19, review and complete this code-review fix: test_clustering_real.py hardcodes a developer's local absolute filesystem path.
What the draft fix changed: Changed `test_repo` in the module-level script body from the hardcoded absolute path `"/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"` to `os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__)))`, so the path is now configurable via an environment variable and defaults to a portable, machine-independent location (the script's own directory) rather than a developer's local filesystem path.
Verify the change is correct and complete; do not refactor unrelated code.

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

test_repo = os.getenv("TEST_REPO_PATH", os.path.dirname(os.path.abspath(__file__)))

config = Config(
repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps",
Expand Down
3 changes: 2 additions & 1 deletion test_clustering_simple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from codewiki.src.config import Config

# Test repo
test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"

@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.

🦩 🟠 test_clustering_simple.py hardcodes a developer's local absolute filesystem path

Changed the test_repo assignment in the module-level script body (around line 21) from the hardcoded absolute path "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" to os.getenv("TEST_REPO_PATH", os.path.join(os.path.dirname(os.path.abspath(__file__)), "openframe-oss-tenant")), allowing an environment variable override while falling back to a path relative to the script's own location instead of a developer-specific home directory, so the script no longer fails immediately for other users or CI runners.

🤖 Prompt for AI agents 
In test_clustering_simple.py around line 21, review and complete this code-review fix: test_clustering_simple.py hardcodes a developer's local absolute filesystem path.
What the draft fix changed: Changed the `test_repo` assignment in the module-level script body (around line 21) from the hardcoded absolute path `"/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant"` to `os.getenv("TEST_REPO_PATH", os.path.join(os.path.dirname(os.path.abspath(__file__)), "openframe-oss-tenant"))`, allowing an environment variable override while falling back to a path relative to the script's own location instead of a developer-specific home directory, so the script no longer fails immediately for other users or CI runners.
Verify the change is correct and complete; do not refactor unrelated code.

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

test_repo = os.getenv("TEST_REPO_PATH", os.path.join(os.path.dirname(os.path.abspath(__file__)), "openframe-oss-tenant"))

# Create simple config with all required fields
config = Config(
Expand Down Expand Up @@ -106,3 +106,4 @@
comp_count = len(module_info.get("components", []))
print(f" - {module_name}: {comp_count} components")
sys.exit(0)

5 changes: 5 additions & 0 deletions test_clustering_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

import json
import logging
import sys
from pathlib import Path

sys.path.insert(0, str(Path(__file__).parent))

# Setup logging
logging.basicConfig(level=logging.INFO, format='%(levelname)s: %(message)s')
Comment on lines 6 to 15

@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.

🦩 🟠 test_clustering_validation.py does not add repo root to sys.path before codewiki imports

Added import sys, from pathlib import Path, and sys.path.insert(0, str(Path(__file__).parent)) near the top of test_clustering_validation.py, as literally requested by the finding. This makes repo-root-relative imports work regardless of CWD, but the finding's deeper suggestion — replacing simulate_validation() with a live import of the actual validation function from cluster_modules.py — was NOT done, since that function isn't currently extracted/exposed as an importable unit in cluster_modules.py (it's inline lines 338-369 per the finding text), and refactoring that file is out of scope for a single-file fix. This remains a real risk: the test can still silently drift from the real implementation. A complete fix requires extracting the validation logic into a standalone function in cluster_modules.py and importing it here.

🤖 Prompt for AI agents 
In test_clustering_validation.py around line 1, review and complete this code-review fix: test_clustering_validation.py does not add repo root to sys.path before codewiki imports.
What the draft fix changed: Added `import sys`, `from pathlib import Path`, and `sys.path.insert(0, str(Path(__file__).parent))` near the top of test_clustering_validation.py, as literally requested by the finding. This makes repo-root-relative imports work regardless of CWD, but the finding's deeper suggestion — replacing `simulate_validation()` with a live import of the actual validation function from `cluster_modules.py` — was NOT done, since that function isn't currently extracted/exposed as an importable unit in `cluster_modules.py` (it's inline lines 338-369 per the finding text), and refactoring that file is out of scope for a single-file fix. This remains a real risk: the test can still silently drift from the real implementation. A complete fix requires extracting the validation logic into a standalone function in `cluster_modules.py` and importing it here.
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

Expand Down Expand Up @@ -175,3 +179,4 @@ def run_tests():
if __name__ == "__main__":
success = run_tests()
exit(0 if success else 1)