diff --git a/gitgalaxy/recorders/llm_recorder.py b/gitgalaxy/recorders/llm_recorder.py index 7c414963f..a2e4bb519 100644 --- a/gitgalaxy/recorders/llm_recorder.py +++ b/gitgalaxy/recorders/llm_recorder.py @@ -469,17 +469,33 @@ def _build_markdown( reverse=True, )[:5] lines.append("### Top 5 Structural Pillars (Highest 'Imported By' / Blast Radius)") - lines.append( - "These are the most interconnected files relative to the rest of this repository. On a repo with dense " - "internal coupling, that means core load-bearing infrastructure -- changes carry real cascading-break " - "risk. On a repo with a flatter internal architecture, the gap between #1 and #5 may be small, and this " - "list is a weaker signal accordingly; compare the connection counts below before treating it as a verdict.\n" - ) - for rank, file_data in enumerate(pillars, 1): - name = file_data.get("name", "Unknown") - path = file_data.get("path", "Unknown") - count = file_data.get("telemetry", {}).get("popularity", 0) - lines.append(f"{rank}. **{name}** (`{path}`) — {count} inbound connections") + # #2556: ranking by popularity is meaningless when the maximum is 0 -- + # the sort is stable, so an all-zero graph just emits the first five + # files in scan order (documentation, usually) under a heading that + # calls them "the most interconnected files". An LLM consuming this + # brief repeats that as fact. A flat graph is a real finding; say so + # instead of dressing scan order up as a ranking. + if not any(f.get("telemetry", {}).get("popularity", 0) > 0 for f in pillars): + lines.append( + "No file in this repository is imported by another file that GitGalaxy could resolve, so there is " + "no blast-radius ranking to report. That is itself a finding: either the codebase genuinely has no " + "internal dependency structure (a collection of scripts, documents or configuration rather than a " + "coupled system), or its import style is one the engine does not resolve for this language. Do not " + "infer that any file is load-bearing from this section.\n" + ) + else: + lines.append( + "These are the most interconnected files relative to the rest of this repository. On a repo with " + "dense internal coupling, that means core load-bearing infrastructure -- changes carry real " + "cascading-break risk. On a repo with a flatter internal architecture, the gap between #1 and #5 may " + "be small, and this list is a weaker signal accordingly; compare the connection counts below before " + "treating it as a verdict.\n" + ) + for rank, file_data in enumerate(pillars, 1): + name = file_data.get("name", "Unknown") + path = file_data.get("path", "Unknown") + count = file_data.get("telemetry", {}).get("popularity", 0) + lines.append(f"{rank}. **{name}** (`{path}`) — {count} inbound connections") lines.append("") orchestrators = sorted( @@ -488,14 +504,29 @@ def _build_markdown( reverse=True, )[:5] lines.append("### Top 5 Orchestrators (Highest 'Imports' / Fragility Index)") - lines.append( - "These files pull in the most external dependencies. They are highly coupled and fragile to API changes.\n" - ) - for rank, file_data in enumerate(orchestrators, 1): - name = file_data.get("name", "Unknown") - path = file_data.get("path", "Unknown") - count = len(file_data.get("raw_imports", [])) if isinstance(file_data.get("raw_imports"), list) else 0 - lines.append(f"{rank}. **{name}** (`{path}`) — {count} outbound dependencies") + + def _outbound(file_data): + raw = file_data.get("raw_imports", []) + return len(raw) if isinstance(raw, list) else 0 + + # #2556: the same zero-guard as the pillar list above. This section had + # the identical defect and the issue did not mention it -- with no + # resolvable imports anywhere it called five files with 0 outbound + # dependencies "highly coupled and fragile to API changes". + if not any(_outbound(f) > 0 for f in orchestrators): + lines.append( + "No file in this repository declares an import that GitGalaxy resolved, so there is no coupling " + "ranking to report. See the note above -- the same caveat applies.\n" + ) + else: + lines.append( + "These files pull in the most external dependencies. They are highly coupled and fragile to API " + "changes.\n" + ) + for rank, file_data in enumerate(orchestrators, 1): + name = file_data.get("name", "Unknown") + path = file_data.get("path", "Unknown") + lines.append(f"{rank}. **{name}** (`{path}`) — {_outbound(file_data)} outbound dependencies") lines.append("") import heapq diff --git a/tests/ruff_audit_baseline.json b/tests/ruff_audit_baseline.json index 8966c82af..a7d86aa1b 100644 --- a/tests/ruff_audit_baseline.json +++ b/tests/ruff_audit_baseline.json @@ -22,7 +22,7 @@ "gitgalaxy/recorders/audit_recorder.py:298: C414": "Unnecessary `list()` call within `sorted()`", "gitgalaxy/recorders/audit_recorder.py:358: PERF401": "Use `list.extend` to create a transformed list", "gitgalaxy/recorders/gpu_recorder.py:285: C414": "Unnecessary `list()` call within `sorted()`", - "gitgalaxy/recorders/llm_recorder.py:1018: SIM102": "Use a single `if` statement instead of nested `if` statements", + "gitgalaxy/recorders/llm_recorder.py:1049: SIM102": "Use a single `if` statement instead of nested `if` statements", "gitgalaxy/recorders/record_keeper.py:187: W291": "Trailing whitespace", "gitgalaxy/recorders/record_keeper.py:188: W291": "Trailing whitespace", "gitgalaxy/recorders/record_keeper.py:189: W291": "Trailing whitespace", diff --git a/tests/tools_recorders/test_llm_recorder.py b/tests/tools_recorders/test_llm_recorder.py index 555dee32e..1ed767581 100644 --- a/tests/tools_recorders/test_llm_recorder.py +++ b/tests/tools_recorders/test_llm_recorder.py @@ -313,3 +313,68 @@ def test_generate_artifacts_integration(recorder, mock_pipeline_state, tmp_path) assert (tmp_path / "TestProject_galaxy_llm.md").exists() assert (tmp_path / "TestProject_galaxy_graph.sqlite").exists() + + +# ============================================================================== +# #2556: A FLAT DEPENDENCY GRAPH IS NOT A RANKING +# ============================================================================== +def _flat_graph_state(mock_pipeline_state): + """The same state with every inbound and outbound connection removed.""" + parsed, unparsable, summary, session = mock_pipeline_state + flat = [] + for f in parsed: + g = dict(f) + g["telemetry"] = {**g.get("telemetry", {}), "popularity": 0} + g["raw_imports"] = [] + flat.append(g) + return flat, unparsable, summary, session + + +def test_structural_pillars_reports_no_ranking_on_a_flat_graph(recorder, mock_pipeline_state): + """ + #2556: scanning cicsdev/cics-genapp produced a "Top 5 Structural Pillars + (Highest 'Imported By')" list of five files with 0 inbound connections + each -- presented as "the most interconnected files". Ranking by a key + that is 0 for everything just emits scan order, and an LLM consuming the + brief repeats it as fact. + """ + parsed, unparsable, summary, session = _flat_graph_state(mock_pipeline_state) + + md_text = recorder._build_markdown(parsed, unparsable, summary, session, {}) + + assert "Top 5 Structural Pillars" in md_text, "the heading itself should still be present" + assert "no blast-radius ranking to report" in md_text + assert "0 inbound connections" not in md_text + assert "most interconnected files" not in md_text + + +def test_orchestrators_reports_no_ranking_on_a_flat_graph(recorder, mock_pipeline_state): + """ + The neighbouring section had the identical defect, which #2556 does not + mention: with no resolvable imports it called five files with 0 outbound + dependencies "highly coupled and fragile to API changes". + """ + parsed, unparsable, summary, session = _flat_graph_state(mock_pipeline_state) + + md_text = recorder._build_markdown(parsed, unparsable, summary, session, {}) + + assert "Top 5 Orchestrators" in md_text + assert "no coupling ranking to report" in md_text + assert "0 outbound dependencies" not in md_text + assert "highly coupled and fragile" not in md_text + + +def test_rankings_still_render_when_the_graph_has_any_connection(recorder, mock_pipeline_state): + """The guard must fire only on an all-zero graph, never on a sparse one.""" + parsed, unparsable, summary, session = _flat_graph_state(mock_pipeline_state) + parsed[0]["telemetry"]["popularity"] = 1 + parsed[0]["raw_imports"] = ["os"] + + md_text = recorder._build_markdown(parsed, unparsable, summary, session, {}) + + assert "most interconnected files" in md_text + assert "1 inbound connections" in md_text + assert "no blast-radius ranking to report" not in md_text + assert "highly coupled and fragile" in md_text + assert "1 outbound dependencies" in md_text + assert "no coupling ranking to report" not in md_text