Skip to content

revert: palace_graph ranking tests written against the pre-f68cb8d API (#916) - #2301

Merged
igorls merged 1 commit into
developfrom
fix/revert-916-stale-graph-tests
Aug 19, 2026
Merged

revert: palace_graph ranking tests written against the pre-f68cb8d API (#916)#2301
igorls merged 1 commit into
developfrom
fix/revert-916-stale-graph-tests

Conversation

@igorls

@igorls igorls commented Aug 18, 2026

Copy link
Copy Markdown
Member

Why

develop is red. tests/test_palace_graph_limits.py, merged from #916, fails identically on linux 3.11, macOS and Windows:

FAILED test_find_tunnels_sorted_by_descending_count - assert ['chromadb'] == ['beta', 'alpha', 'gamma']
FAILED test_find_tunnels_caps_results_at_fifty       - assert 1 == 50
FAILED test_graph_stats_counts_unique_rooms_per_wing - assert 1 == 2
FAILED test_graph_stats_top_tunnels_only_lists_...   - assert 1 == 10

This reverts that merge to get develop green again. It is not a judgement on the coverage, which is worth having.

Root cause

#916's CI last ran on 2026-04-15. Its checks were green, but against April's develop — GitHub does not re-run CI when the base branch advances, it just keeps displaying the last result, and mergeStateStatus: CLEAN only means "no textual merge conflict".

palace_graph.py has moved substantially since, in particular f68cb8d fix: scope derived graph state to explicit palace. find_tunnels and graph_stats now take a config argument and resolve derived state through build_graph(col, config):

def find_tunnels(wing_a=None, wing_b=None, col=None, config=None):
    nodes, edges = build_graph(col, config)

The tests hand in a MagicMock collection and expect the graph to be derived purely from it. Under the config-scoped implementation that no longer holds, so the assertions collapse to 1 result. They encode the pre-f68cb8d architecture rather than failing on a real regression — there is no product bug here to fix.

Why revert rather than repair in place

Rewriting the tests would mean guessing at the coverage @fatkobra intended and committing substantive authorship onto their PR. Better to restore develop now and let the tests be rebuilt against the current API, with the original author credited. I'll follow up on #916.

Verification

tests/test_knowledge_graph.py tests/test_split_mega_files.py — 71 passed
ruff check . / ruff format --check . — clean

The other ten PRs merged in the same batch are unaffected and stay on develop.

@igorls
igorls requested a review from milla-jovovich as a code owner August 18, 2026 23:09
Copilot AI lite review requested due to automatic review settings August 18, 2026 23:09
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@igorls
igorls merged commit ec98196 into develop Aug 19, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants