Skip to content

fix(CODEWIKI-009): CU-86akn96pk 4 review findings across 4 files #99

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 4 commits into
base: main
Choose a base branch
from
+264 −63
Draft

fix(CODEWIKI-009): CU-86akn96pk 4 review findings across 4 files #99

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
9eaf29a
fix(CODEWIKI-009): 4 review findings across 4 files
flamingo[bot] Sep 23, 2026
b287376
fix(CODEWIKI-009): 4 review findings across 4 files
flamingo[bot] Sep 23, 2026
9833c4b
fix(CODEWIKI-009): 4 review findings across 4 files
flamingo[bot] Sep 23, 2026
7f1b41a
fix(CODEWIKI-009): 4 review findings across 4 files
flamingo[bot] Sep 23, 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
7 changes: 6 additions & 1 deletion test_clustering_local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,13 +163,18 @@ def test_clustering(results):
return False

if __name__ == "__main__":

@flamingo flamingo Bot Sep 23, 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_local.py test function does not call results.add_test() for every failure path (e.g. exception branch inside try lacks per-assertion granularity) and uses raw sys.exit without full pass/fail bookkeeping for the API-key check

In the if __name__ == "__main__": block, test_results = TestResults() is now created before the API-key check; the missing-key branch now calls test_results.add_test("api_key_configured", False, ...) and test_results.print_summary() before sys.exit(1), and the success branch calls test_results.add_test("api_key_configured", True) so the bookkeeping and summary output reflect the API-key check in all paths.

🤖 Prompt for AI agents 
In test_clustering_local.py around line 165, review and complete this code-review fix: test_clustering_local.py test function does not call results.add_test() for every failure path (e.g. exception branch inside try lacks per-assertion granularity) and uses raw sys.exit without full pass/fail bookkeeping for the API-key check.
What the draft fix changed: In the `if __name__ == "__main__":` block, `test_results = TestResults()` is now created before the API-key check; the missing-key branch now calls `test_results.add_test("api_key_configured", False, ...)` and `test_results.print_summary()` before `sys.exit(1)`, and the success branch calls `test_results.add_test("api_key_configured", True)` so the bookkeeping and summary output reflect the API-key check in all paths.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

test_results = TestResults()

# Check for API keys
if not (os.getenv("OPENAI_API_KEY") or os.getenv("MAIN_API_KEY")):
print("❌ ERROR: OPENAI_API_KEY or MAIN_API_KEY environment variable not set")
print(" Set it with: export OPENAI_API_KEY='your-key-here'")
test_results.add_test("api_key_configured", False, "OPENAI_API_KEY or MAIN_API_KEY environment variable not set")
test_results.print_summary()
sys.exit(1)

test_results = TestResults()
test_results.add_test("api_key_configured", True)

success = test_clustering(test_results)
test_results.print_summary()
sys.exit(0 if success else 1)
Expand Down
58 changes: 46 additions & 12 deletions test_clustering_proof.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,32 @@
from codewiki.src.be.dependency_analyzer.models.core import Node
from codewiki.src.config import Config


class TestResults:
def __init__(self):
self.tests = []

def add_test(self, name, passed, message=""):
self.tests.append((name, passed, message))

def print_summary(self):
print("\n" + "=" * 80)
print("📊 TEST SUMMARY")
print("=" * 80)
for name, passed, message in self.tests:
status = "✅ PASS" if passed else "❌ FAIL"
print(f"{status}: {name}")
if message:
print(f" {message}")
total = len(self.tests)
passed_count = sum(1 for _, passed, _ in self.tests if passed)
print(f"\n{passed_count}/{total} tests passed")
print("=" * 80)
return all(passed for _, passed, _ in self.tests)


results = TestResults()

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

config = Config(
Expand Down Expand Up @@ -78,22 +104,30 @@
print("\n📊 RESULTS:\n")

if len(module_tree) == 0:

@flamingo flamingo Bot Sep 23, 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 uses print()/sys.exit() ad-hoc instead of the TestResults accumulator pattern

Replaced the ad-hoc print()/sys.exit() pass/fail reporting at the bottom of the script with a local TestResults class (add_test()/print_summary()) mirroring the pattern in test_clustering_validation.py, and recorded the single clustering outcome (empty/one/multiple modules) as a test case via add_test(); the script now exits based on print_summary()'s aggregated result instead of branching sys.exit() calls. Risk: TestResults is defined locally in this file rather than imported from a shared module, since no such shared module currently exists in the repo to import from safely; if test_clustering_validation.py's TestResults is later extracted to a shared module, this file should be updated to import it instead of duplicating the class.

🤖 Prompt for AI agents 
In test_clustering_proof.py around line 80, review and complete this code-review fix: test_clustering_proof.py uses print()/sys.exit() ad-hoc instead of the TestResults accumulator pattern.
What the draft fix changed: Replaced the ad-hoc print()/sys.exit() pass/fail reporting at the bottom of the script with a local TestResults class (add_test()/print_summary()) mirroring the pattern in test_clustering_validation.py, and recorded the single clustering outcome (empty/one/multiple modules) as a test case via add_test(); the script now exits based on print_summary()'s aggregated result instead of branching sys.exit() calls. Risk: TestResults is defined locally in this file rather than imported from a shared module, since no such shared module currently exists in the repo to import from safely; if test_clustering_validation.py's TestResults is later extracted to a shared module, this file should be updated to import it instead of duplicating the class.
Verify the change is correct and complete; do not refactor unrelated code.

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

print("❌ FAILED: Empty module tree")
print(" LLM did NOT follow <GROUPED_COMPONENTS> format")
sys.exit(1)
results.add_test(
"clustering_produces_modules",
False,
"Empty module tree - LLM did NOT follow <GROUPED_COMPONENTS> format"
)
elif len(module_tree) == 1:
print("⚠️ LLM returned 1 module (rejected as too small)")
print(" But LLM DID follow the tag format correctly!")
print(f" Module: {list(module_tree.keys())[0]}")
sys.exit(0)
results.add_test(
"clustering_produces_modules",
True,
f"LLM returned 1 module (rejected as too small) but followed tag format correctly. "
f"Module: {list(module_tree.keys())[0]}"
)
else:
print(f"✅✅✅ SUCCESS! {len(module_tree)} modules created ✅✅✅")
print("\n🎉 THE FIX IS PROVEN TO WORK! 🎉\n")
print("Modules generated:")
modules_desc = []
for name, info in module_tree.items():
comp_count = len(info.get('components', []))
comp_list = info.get('components', [])[:5]
more = len(info.get('components', [])) - 5
print(f" - {name}: {comp_count} components {comp_list}{'...' if more > 0 else ''}")
sys.exit(0)
modules_desc.append(f"{name}: {comp_count} components {comp_list}{'...' if more > 0 else ''}")
results.add_test(
"clustering_produces_modules",
True,
f"{len(module_tree)} modules created. Modules generated:\n - " + "\n - ".join(modules_desc)
)

all_passed = results.print_summary()
sys.exit(0 if all_passed else 1)
47 changes: 41 additions & 6 deletions test_clustering_simple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,33 @@
from codewiki.src.be.dependency_analyzer.models.core import Node
from codewiki.src.config import Config


class TestResults:
def __init__(self):
self.tests = []

def add_test(self, name, passed, message=""):
self.tests.append((name, passed, message))

def print_summary(self):
print("\n" + "=" * 80)
print("TEST SUMMARY")
print("=" * 80)
failed = 0
for name, passed, message in self.tests:
status = "✅ PASS" if passed else "❌ FAIL"
print(f"{status}: {name}")
if message:
print(f" {message}")
if not passed:
failed += 1
print("=" * 80)
print(f"Total: {len(self.tests)}, Passed: {len(self.tests) - failed}, Failed: {failed}")
return failed == 0


results = TestResults()

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

Expand Down Expand Up @@ -97,13 +124,21 @@
print("=" * 80)

if len(module_tree) == 0:

@flamingo flamingo Bot Sep 23, 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 uses ad-hoc print/sys.exit instead of TestResults pattern

Replaced the ad-hoc print()/sys.exit() pass/fail reporting at the end of the script (previously if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0)) with a TestResults class defined in this file providing add_test() and print_summary(), matching the CODEWIKI-009 pattern. The script now records the clustering outcome via results.add_test(...) and determines the exit code from results.print_summary(). Risk: I could not see the actual shared TestResults implementation referenced by CODEWIKI-009 elsewhere in the repo, so I defined a local, self-contained class with the same interface (add_test, print_summary) rather than importing an unseen shared module, to avoid introducing a broken import.

🤖 Prompt for AI agents 
In test_clustering_simple.py around line 99, review and complete this code-review fix: test_clustering_simple.py uses ad-hoc print/sys.exit instead of TestResults pattern.
What the draft fix changed: Replaced the ad-hoc print()/sys.exit() pass/fail reporting at the end of the script (previously `if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0)`) with a `TestResults` class defined in this file providing `add_test()` and `print_summary()`, matching the CODEWIKI-009 pattern. The script now records the clustering outcome via `results.add_test(...)` and determines the exit code from `results.print_summary()`. Risk: I could not see the actual shared `TestResults` implementation referenced by CODEWIKI-009 elsewhere in the repo, so I defined a local, self-contained class with the same interface (`add_test`, `print_summary`) rather than importing an unseen shared module, to avoid introducing a broken import.
Verify the change is correct and complete; do not refactor unrelated code.

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

print("\n❌ FAILED: Empty module tree")
print(" LLM did not follow the <GROUPED_COMPONENTS> tag format")
sys.exit(1)
results.add_test(
"clustering produces module tree",
False,
"Empty module tree - LLM did not follow the <GROUPED_COMPONENTS> tag format"
)
else:
print(f"\n✅ SUCCESS: {len(module_tree)} modules created")
detail_lines = [f"{len(module_tree)} modules created"]
for module_name, module_info in module_tree.items():
comp_count = len(module_info.get("components", []))
print(f" - {module_name}: {comp_count} components")
sys.exit(0)
detail_lines.append(f" - {module_name}: {comp_count} components")
results.add_test(
"clustering produces module tree",
True,
"\n".join(detail_lines)
)

success = results.print_summary()
sys.exit(0 if success else 1)
Loading