Skip to content

fix(CODEWIKI-009): CU-86akhf8u6 7 review findings across 6 files #78

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 6 commits into
base: main
Choose a base branch
from
+421 −123
Draft

fix(CODEWIKI-009): CU-86akhf8u6 7 review findings across 6 files #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b8abf0
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 2026
dc066c5
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 2026
3287283
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 2026
8a87784
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 2026
0312cc8
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 2026
007e499
fix(CODEWIKI-009): 7 review findings across 6 files
flamingo[bot] Sep 14, 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
34 changes: 31 additions & 3 deletions test_clustering_real.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,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)
passed_count = 0
for name, passed, message in self.tests:
status = "✅ PASS" if passed else "❌ FAIL"
print(f"{status} - {name}" + (f": {message}" if message else ""))
if passed:
passed_count += 1
total = len(self.tests)
print("-" * 80)
print(f"Total: {passed_count}/{total} passed")
print("=" * 80)
return passed_count == total


results = TestResults()

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

config = Config(
Comment on lines 16 to 48

@flamingo flamingo Bot Sep 14, 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.

🦩 🟠 logging.basicConfig() called directly in test_clustering_real.py instead of centralized setup_logging()

The finding calls for using a centralized setup_logging() utility instead of logging.basicConfig(), but no such module/function is visible anywhere in the provided material, so I could not import a real one without inventing an identifier/module. Left logging.basicConfig(...) unchanged in this file to avoid introducing a fabricated import; a complete fix requires locating the actual centralized logging module in the repo (not shown to me) and importing its real setup_logging symbol here.

🤖 Prompt for AI agents 
In test_clustering_real.py around line 8, review and complete this code-review fix: logging.basicConfig() called directly in test_clustering_real.py instead of centralized setup_logging().
What the draft fix changed: The finding calls for using a centralized `setup_logging()` utility instead of `logging.basicConfig()`, but no such module/function is visible anywhere in the provided material, so I could not import a real one without inventing an identifier/module. Left `logging.basicConfig(...)` unchanged in this file to avoid introducing a fabricated import; a complete fix requires locating the actual centralized logging module in the repo (not shown to me) and importing its real `setup_logging` symbol here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 20 low — review closely — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -53,13 +80,14 @@
current_module_tree={}, current_module_name=None, current_module_path=[]
)

print("\n" + "=" * 80)

@flamingo flamingo Bot Sep 14, 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, test_clustering_real.py, test_clustering_simple.py use ad-hoc sys.exit checks instead of TestResults accumulator

Replaced the ad-hoc if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0) block at the end of the script with a TestResults class (with add_test and print_summary methods, matching the pattern described for test_clustering_local.py / test_clustering_validation.py) instantiated as results, which records a single pass/fail assertion on module_tree non-emptiness and drives the final sys.exit() via results.print_summary()'s boolean return.

🤖 Prompt for AI agents 
In test_clustering_real.py around line 56, review and complete this code-review fix: test_clustering_proof.py, test_clustering_real.py, test_clustering_simple.py use ad-hoc sys.exit checks instead of TestResults accumulator.
What the draft fix changed: Replaced the ad-hoc `if len(module_tree) == 0: ... sys.exit(1) else: ... sys.exit(0)` block at the end of the script with a `TestResults` class (with `add_test` and `print_summary` methods, matching the pattern described for test_clustering_local.py / test_clustering_validation.py) instantiated as `results`, which records a single pass/fail assertion on `module_tree` non-emptiness and drives the final `sys.exit()` via `results.print_summary()`'s boolean return.
Verify the change is correct and complete; do not refactor unrelated code.

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

if len(module_tree) == 0:
print("❌ FAILED - Check LLM response above")
sys.exit(1)
results.add_test("clustering produced modules", False, "module_tree is empty")
else:
print(f"✅ SUCCESS: {len(module_tree)} modules")
for name, info in module_tree.items():
print(f" - {name}: {len(info.get('components', []))} components")
sys.exit(0)
results.add_test("clustering produced modules", True, f"{len(module_tree)} modules")

success = results.print_summary()
sys.exit(0 if success else 1)
218 changes: 174 additions & 44 deletions test_fqdn_normalization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,42 @@
"""

@flamingo flamingo Bot Sep 14, 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_fqdn_normalization.py uses pytest assertions but doesn't follow the TestResults accumulator pattern required for repo test scripts

Rewrote test_fqdn_normalization.py to remove the pytest dependency and bare assert statements. Added a TestResults class implementing add_test/print_summary as required by CODEWIKI-009, converted every test_* function to accept a results parameter and replaced each assert with a corresponding results.add_test(...) call carrying a descriptive name/message instead of raising AssertionError. Replaced the if __name__ == "__main__": pytest.main(...) runner with a manual invocation of all test functions followed by results.print_summary() and exit(0 if success else 1). Removed the import pytest and the pytest-based docstring/run instructions. Behavior/logic of each test case (the assertions being checked) is unchanged; only the mechanism for recording pass/fail was converted to the accumulator pattern.

🤖 Prompt for AI agents 
In test_fqdn_normalization.py around line 1, review and complete this code-review fix: test_fqdn_normalization.py uses pytest assertions but doesn't follow the TestResults accumulator pattern required for repo test scripts.
What the draft fix changed: Rewrote test_fqdn_normalization.py to remove the pytest dependency and bare assert statements. Added a `TestResults` class implementing `add_test`/`print_summary` as required by CODEWIKI-009, converted every `test_*` function to accept a `results` parameter and replaced each `assert` with a corresponding `results.add_test(...)` call carrying a descriptive name/message instead of raising AssertionError. Replaced the `if __name__ == "__main__": pytest.main(...)` runner with a manual invocation of all test functions followed by `results.print_summary()` and `exit(0 if success else 1)`. Removed the `import pytest` and the pytest-based docstring/run instructions. Behavior/logic of each test case (the assertions being checked) is unchanged; only the mechanism for recording pass/fail was converted to the accumulator pattern.
Verify the change is correct and complete; do not refactor unrelated code.

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

Test Cases for FQDN Normalization Fix

Run with: python -m pytest test_fqdn_normalization.py -v
Run with: python test_fqdn_normalization.py
"""

import pytest
from collections import namedtuple

# Mock Node class for testing
Node = namedtuple('Node', ['short_id'])


def test_strip_deps_prefix():
class TestResults:
def __init__(self):
self.total = 0
self.passed = 0
self.failed = 0
self.failures = []

def add_test(self, name, condition, message=""):
self.total += 1
if condition:
self.passed += 1
else:
self.failed += 1
self.failures.append((name, message))

def print_summary(self):
print(f"\n{'=' * 60}")
print(f"Test Summary: {self.passed}/{self.total} passed")
if self.failures:
print(f"\nFailures ({self.failed}):")
for name, message in self.failures:
print(f" - {name}: {message}")
print(f"{'=' * 60}")
return self.failed == 0


def test_strip_deps_prefix(results):
"""Test that 'deps.' prefix is correctly stripped."""
# Simulated components dictionary
components = {
Expand All @@ -34,11 +59,19 @@ def test_strip_deps_prefix():
# Test normalization
for llm_id, expected_fqdn in zip(llm_output, expected):
stripped = llm_id[5:] if llm_id.startswith("deps.") else llm_id
assert stripped in components, f"Failed to find {stripped} after stripping"
assert stripped == expected_fqdn


def test_fuzzy_component_name_match():
results.add_test(
"test_strip_deps_prefix: stripped in components",
stripped in components,
f"Failed to find {stripped} after stripping",
)
results.add_test(
"test_strip_deps_prefix: stripped == expected_fqdn",
stripped == expected_fqdn,
f"{stripped} != {expected_fqdn}",
)


def test_fuzzy_component_name_match(results):
"""Test fuzzy matching by component name (last segment)."""
components = {
"openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer": Node(short_id="PinotConfigInitializer"),
Expand All @@ -50,16 +83,29 @@ def test_fuzzy_component_name_match():

# Extract component name
component_name = llm_id.split('.')[-1]
assert component_name == "PinotConfigInitializer"
results.add_test(
"test_fuzzy_component_name_match: component_name",
component_name == "PinotConfigInitializer",
f"component_name was {component_name}",
)

# Find matches
matches = [fqdn for fqdn in components.keys() if fqdn.split('.')[-1] == component_name]

assert len(matches) == 1, f"Expected 1 match, found {len(matches)}"
assert matches[0] == "openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer"
results.add_test(
"test_fuzzy_component_name_match: match count",
len(matches) == 1,
f"Expected 1 match, found {len(matches)}",
)
if matches:
results.add_test(
"test_fuzzy_component_name_match: match value",
matches[0] == "openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer",
f"matches[0] was {matches[0]}",
)


def test_path_suffix_matching():
def test_path_suffix_matching(results):
"""Test matching by path suffix (last N segments)."""
components = {
"openframe-oss-lib.different.path.java.config.pinot.PinotConfigInitializer": Node(short_id="PinotConfigInitializer"),
Expand All @@ -73,22 +119,35 @@ def test_path_suffix_matching():

matches = [fqdn for fqdn in components.keys() if fqdn.endswith(suffix_3)]

assert len(matches) == 1
assert matches[0] == "openframe-oss-lib.different.path.java.config.pinot.PinotConfigInitializer"
results.add_test(
"test_path_suffix_matching: match count",
len(matches) == 1,
f"Expected 1 match, found {len(matches)}",
)
if matches:
results.add_test(
"test_path_suffix_matching: match value",
matches[0] == "openframe-oss-lib.different.path.java.config.pinot.PinotConfigInitializer",
f"matches[0] was {matches[0]}",
)


def test_exact_fqdn_match():
def test_exact_fqdn_match(results):
"""Test that exact FQDN matches work without modification."""
components = {
"main-repo.src.services.user_service.UserService": Node(short_id="UserService"),
}

llm_id = "main-repo.src.services.user_service.UserService"

assert llm_id in components
results.add_test(
"test_exact_fqdn_match: llm_id in components",
llm_id in components,
f"{llm_id} not found in components",
)


def test_short_id_mapping():
def test_short_id_mapping(results):
"""Test that short ID → FQDN mapping works."""
components = {
"main-repo.src.services.user_service.UserService": Node(short_id="UserService"),
Expand All @@ -102,11 +161,19 @@ def test_short_id_mapping():
mapping[short_id] = fqdn

# Test mapping
assert mapping["UserService"] == "main-repo.src.services.user_service.UserService"
assert mapping["Logger"] == "main-repo.src.utils.logger.Logger"


def test_partial_path_mapping():
results.add_test(
"test_short_id_mapping: UserService",
mapping.get("UserService") == "main-repo.src.services.user_service.UserService",
f"mapping[UserService] was {mapping.get('UserService')}",
)
results.add_test(
"test_short_id_mapping: Logger",
mapping.get("Logger") == "main-repo.src.utils.logger.Logger",
f"mapping[Logger] was {mapping.get('Logger')}",
)


def test_partial_path_mapping(results):
"""Test that partial paths are mapped correctly."""
components = {
"main-repo.src.services.auth.UserService": Node(short_id="UserService"),
Expand All @@ -128,13 +195,29 @@ def test_partial_path_mapping():
mapping[partial] = fqdn

# Test mappings
assert "UserService" in mapping
assert "auth.UserService" in mapping
assert "services.auth.UserService" in mapping
assert "src.services.auth.UserService" in mapping


def test_collision_detection():
results.add_test(
"test_partial_path_mapping: UserService",
"UserService" in mapping,
"UserService not in mapping",
)
results.add_test(
"test_partial_path_mapping: auth.UserService",
"auth.UserService" in mapping,
"auth.UserService not in mapping",
)
results.add_test(
"test_partial_path_mapping: services.auth.UserService",
"services.auth.UserService" in mapping,
"services.auth.UserService not in mapping",
)
results.add_test(
"test_partial_path_mapping: src.services.auth.UserService",
"src.services.auth.UserService" in mapping,
"src.services.auth.UserService not in mapping",
)


def test_collision_detection(results):
"""Test that collisions are detected when same short ID maps to multiple FQDNs."""
from collections import defaultdict

Expand All @@ -154,11 +237,19 @@ def test_collision_detection():
else:
mapping[short_id] = fqdn

assert "UserService" in collisions
assert len(collisions["UserService"]) >= 1 # At least one collision
results.add_test(
"test_collision_detection: UserService in collisions",
"UserService" in collisions,
"UserService not detected as collision",
)
results.add_test(
"test_collision_detection: collision count",
len(collisions["UserService"]) >= 1,
f"Expected at least one collision, found {len(collisions['UserService'])}",
)


def test_best_path_match_scoring():
def test_best_path_match_scoring(results):
"""Test the path similarity scoring algorithm."""
llm_id = "deps.openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer"
candidates = [
Expand All @@ -180,11 +271,19 @@ def test_best_path_match_scoring():
scores.sort(key=lambda x: x[1], reverse=True)

# Best match should have highest score
assert scores[0][0] == "openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer"
assert scores[0][1] > scores[2][1] # Better than different namespace


def test_non_existent_component():
results.add_test(
"test_best_path_match_scoring: best match",
scores[0][0] == "openframe-oss-lib.src.main.java.config.pinot.PinotConfigInitializer",
f"scores[0][0] was {scores[0][0]}",
)
results.add_test(
"test_best_path_match_scoring: better than different namespace",
scores[0][1] > scores[2][1],
f"scores[0][1]={scores[0][1]} not > scores[2][1]={scores[2][1]}",
)


def test_non_existent_component(results):
"""Test that non-existent components fail normalization."""
components = {
"main-repo.src.services.UserService": Node(short_id="UserService"),
Expand All @@ -194,14 +293,22 @@ def test_non_existent_component():

# Should not match anything
stripped = llm_id[5:] if llm_id.startswith("deps.") else llm_id
assert stripped not in components
results.add_test(
"test_non_existent_component: stripped not in components",
stripped not in components,
f"{stripped} unexpectedly found in components",
)

component_name = llm_id.split('.')[-1]
matches = [fqdn for fqdn in components.keys() if component_name in fqdn]
assert len(matches) == 0
results.add_test(
"test_non_existent_component: no matches",
len(matches) == 0,
f"Expected 0 matches, found {len(matches)}",
)


def test_double_class_name():
def test_double_class_name(results):
"""Test handling of paths with duplicate component names."""
# This tests the scenario: PintoConfigInitializer.PinotConfigInitializer
components = {
Expand All @@ -218,10 +325,14 @@ def test_double_class_name():
# Should match by component name
matches = [fqdn for fqdn in components.keys() if fqdn.split('.')[-1] == component_name]

assert len(matches) == 1
results.add_test(
"test_double_class_name: match count",
len(matches) == 1,
f"Expected 1 match, found {len(matches)}",
)


def test_java_package_path():
def test_java_package_path(results):
"""Test handling of Java package paths with com.openframe prefix."""
components = {
"openframe-oss-lib.src.main.java.com.openframe.management.config.PinotConfig": Node(
Expand All @@ -235,8 +346,27 @@ def test_java_package_path():
# Strip deps prefix
stripped = llm_id[5:] if llm_id.startswith("deps.") else llm_id

assert stripped in components
results.add_test(
"test_java_package_path: stripped in components",
stripped in components,
f"{stripped} not found in components",
)


if __name__ == "__main__":
pytest.main([__file__, "-v"])
results = TestResults()

test_strip_deps_prefix(results)
test_fuzzy_component_name_match(results)
test_path_suffix_matching(results)
test_exact_fqdn_match(results)
test_short_id_mapping(results)
test_partial_path_mapping(results)
test_collision_detection(results)
test_best_path_match_scoring(results)
test_non_existent_component(results)
test_double_class_name(results)
test_java_package_path(results)

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