From b758addcf0b1df233a6b9b4ea50d9d50e9d607bb Mon Sep 17 00:00:00 2001 From: Joe Esquibel Date: Wed, 2 Sep 2026 22:45:33 -0400 Subject: [PATCH] fix(recorders): a flat dependency graph is not a ranking (#2556) "Top 5 Structural Pillars (Highest 'Imported By' / Blast Radius)" sorted by popularity and took the first five with no check that the maximum was above zero. On a repo with no resolvable internal imports that emits five files with 0 inbound connections under a heading calling them "the most interconnected files" -- the scan that found it listed Changes.md, MAINTAINERS.md and README.md as the load-bearing infrastructure of a COBOL application. The neighbouring "Top 5 Orchestrators" section had the identical defect, which the issue does not mention: with no resolvable imports anywhere it called five files with 0 outbound dependencies "highly coupled and fragile to API changes". Both are guarded here. Both replacements say what a flat graph actually means -- either the codebase has no internal dependency structure, or the engine does not resolve that language's import style -- and tell the reader not to infer that any file is load-bearing. That matters more than suppressing the list, because this brief is written to be consumed by an LLM that will otherwise repeat the ranking as fact. Also answers the issue's secondary question ("why are documentation files surfacing first?"): it is not alphabetical tie-breaking. Python's sort is stable, so when every key is 0 the output is simply scan order. Note the trigger is rarer than it was: #2668 fixed relative imports that carry an extension, which was the reason whole javascript/shell/yaml/ powershell ecosystems presented as flat graphs. A repo with genuinely no internal imports still reaches this path, so the guard is still needed. Co-Authored-By: Claude Opus 5 (1M context) --- gitgalaxy/recorders/llm_recorder.py | 69 ++++++++++++++++------ tests/ruff_audit_baseline.json | 2 +- tests/tools_recorders/test_llm_recorder.py | 65 ++++++++++++++++++++ 3 files changed, 116 insertions(+), 20 deletions(-) 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