fix(CODEWIKI-009-2): CU-86akbhhru 4 review findings across 4 files - #53
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| 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" |
There was a problem hiding this comment.
🦩 🟠 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
| 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" |
There was a problem hiding this comment.
🦩 🟠 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
| from codewiki.src.config import Config | ||
|
|
||
| # Test repo | ||
| test_repo = "/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant" |
There was a problem hiding this comment.
🦩 🟠 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
|
|
||
| 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') |
There was a problem hiding this comment.
🦩 🟠 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
flamingo
Bot
changed the title
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts were competing module docstrings in cpp.py, csharp.py and javascript.py, added by both this branch and #55. Resolved in favour of the wording already on main; this branch's _get_component_id changes are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts: - deps.py, typescript.py: competing module docstrings added by both this branch and #55; resolved in favour of the wording on main. - config.py: this branch's new module docstring kept, layered on top of #49's widened dataclasses import (fields, asdict). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflict in test_clustering_simple.py: both this branch and #53 replaced the hardcoded test repo path with a TEST_REPO_PATH env lookup, differing only in the fallback. Resolved in favour of main's fallback, which #53 applied consistently across the other clustering test scripts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). flamingo_guidelines.py is fully superseded by #52, which has merged: this branch's changes to that file are dropped and the file is taken from main wholesale. Resolving the conflict hunk-by-hunk instead left a duplicate 'import logging', since main already has one. What remains is the part of this PR #52 did not cover: the analysis_service.py dead-code cleanup and the background_worker.py print -> logging conversion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes 4 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
test_clustering_proof.py:13test_clustering_real.py:19test_clustering_simple.py:21test_clustering_validation.py:1What 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-5f03d762c00cMerging 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-86akbhhru CodeWiki backend and CLI review findings (12 PRs)