fix(CODEWIKI-003-2): CU-86akbhhru 3 review findings in test_clustering_forced.py - #46
Conversation
| os.path.dirname(os.path.dirname(os.path.abspath(__file__))) | ||
| ) | ||
|
|
||
| config = Config( |
There was a problem hiding this comment.
🦩 🔴 API keys hardcoded via env-var defaults passed directly into Config constructor in test scripts
Replaced direct Config(cluster_api_key=..., main_api_key=..., fallback_api_key=..., ...) constructor call with Config.from_env(...) (no explicit API key args) in the top-level script body, relying on a factory classmethod to source keys (e.g. from keyring/env internally) instead of passing them as plain constructor arguments. This assumes Config.from_env exists and accepts the same non-key kwargs (repo_path, output_dir, model names, base URLs, max_token_per_module, cluster_max_tokens) — this could not be verified against codewiki/src/config.py since that file was not provided, so the fix may require adjusting the factory method name/signature to match the actual implementation, or adding such a factory if it does not yet exist. This is the primary risk in this change.
🤖 Prompt for AI agents
In test_clustering_forced.py around line 21, review and complete this code-review fix: API keys hardcoded via env-var defaults passed directly into Config constructor in test scripts.
What the draft fix changed: Replaced direct `Config(cluster_api_key=..., main_api_key=..., fallback_api_key=..., ...)` constructor call with `Config.from_env(...)` (no explicit API key args) in the top-level script body, relying on a factory classmethod to source keys (e.g. from keyring/env internally) instead of passing them as plain constructor arguments. This assumes `Config.from_env` exists and accepts the same non-key kwargs (repo_path, output_dir, model names, base URLs, max_token_per_module, cluster_max_tokens) — this could not be verified against `codewiki/src/config.py` since that file was not provided, so the fix may require adjusting the factory method name/signature to match the actual implementation, or adding such a factory if it does not yet exist. This is the primary risk in this change.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — 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_forced.py hardcodes a developer's local absolute filesystem path as the test repo
Replaced hardcoded absolute path /Users/michaelassraf/Documents/GitHub/openframe-oss-tenant for test_repo (script body, top level) with a dynamic default derived from __file__ (parent of the script's directory), overridable via TEST_REPO_PATH env var, removing the developer-specific path while preserving configurability.
🤖 Prompt for AI agents
In test_clustering_forced.py around line 19, review and complete this code-review fix: test_clustering_forced.py hardcodes a developer's local absolute filesystem path as the test repo.
What the draft fix changed: Replaced hardcoded absolute path `/Users/michaelassraf/Documents/GitHub/openframe-oss-tenant` for `test_repo` (script body, top level) with a dynamic default derived from `__file__` (parent of the script's directory), overridable via `TEST_REPO_PATH` env var, removing the developer-specific path while preserving configurability.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| os.path.dirname(os.path.dirname(os.path.abspath(__file__))) | ||
| ) | ||
|
|
||
| config = Config( |
There was a problem hiding this comment.
🦩 🟠 Four near-identical clustering test scripts duplicate the same Config-construction and reporting logic
No shared helper module was extracted in this file (out of scope: fixing this properly requires creating a new shared module e.g. test_clustering_common.py and modifying the three sibling files, which were not provided/in-scope per instructions to change only this file). As a partial, in-file mitigation, the Config construction was simplified by removing the duplicated API-key-sourcing boilerplate (now handled by Config.from_env), reducing the amount of duplicated logic per file, but the full duplication-removal (shared fixture/function across all four scripts) is NOT completed here and would require a follow-up change spanning all four files.
🤖 Prompt for AI agents
In test_clustering_forced.py around line 21, review and complete this code-review fix: Four near-identical clustering test scripts duplicate the same Config-construction and reporting logic.
What the draft fix changed: No shared helper module was extracted in this file (out of scope: fixing this properly requires creating a new shared module e.g. `test_clustering_common.py` and modifying the three sibling files, which were not provided/in-scope per instructions to change only this file). As a partial, in-file mitigation, the Config construction was simplified by removing the duplicated API-key-sourcing boilerplate (now handled by `Config.from_env`), reducing the amount of duplicated logic per file, but the full duplication-removal (shared fixture/function across all four scripts) is NOT completed here and would require a follow-up change spanning all four files.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
|
Blocking: config = Config.from_env(repo_path=test_repo, output_dir="/tmp/test", ...)
The PR also drops the three The |
The branch swapped Config(...) for Config.from_env(...) and dropped the three *_api_key kwargs on the assumption that the factory resolves them from the environment. Config has no from_env: only from_args, from_web_job, from_cli and from_config_manager. The script raised AttributeError before reaching a test. Restore direct construction with the API-key kwargs. The valuable part of this PR - replacing the hardcoded /Users/... path with a TEST_REPO_PATH lookup - is unchanged, and the config now constructs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes 3 review findings in
test_clustering_forced.py.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
test_clustering_forced.py:21test_clustering_forced.py:19test_clustering_forced.py:21What 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)