Skip to content

fix(CODEWIKI-003-2): CU-86akbhhru 3 review findings in test_clustering_forced.py #46

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
+5 −1
Merged

fix(CODEWIKI-003-2): CU-86akbhhru 3 review findings in test_clustering_forced.py #46

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
279c0ea
fix(CODEWIKI-003-2): 3 review findings in test_clustering_forced.py
flamingo[bot] Sep 7, 2026
a8eceeb
Merge branch 'main' into pr46
michaelassraf Sep 8, 2026
6e015a4
Merge main
michaelassraf Sep 8, 2026
372df3d
fix: Config.from_env does not exist - restore direct construction
michaelassraf Sep 8, 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
6 changes: 5 additions & 1 deletion test_clustering_forced.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
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_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

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

config = Config(

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

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

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

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

repo_path=test_repo, output_dir="/tmp/test", dependency_graph_dir="/tmp/test/deps",
Expand Down Expand Up @@ -71,3 +74,4 @@
print(f" - {name}: {comp_count} components")
print("\n🎉 THE FIX WORKS! LLM followed the <GROUPED_COMPONENTS> tag format!")
sys.exit(0)