Show a merged game's addresses on the page its slug redirects to - #190
Merged
Merged
Conversation
A merge says two listings are one game, so every address either answered at is an address of that game. The absorbed game keeps its endpoint rows — nothing moves, and the merge stays a redirect — but the page its slug 301s to read only the winner's own endpoints. The loser's addresses went on being crawled and were shown nowhere a reader could reach. Found about to merge ChatMUD's two listings for #188: `chatmud.com:7443`, the TLS door the crawl discovered yesterday, belongs to the listing being absorbed. The merge would have taken it off the only page left. It is probably not the only one — "two ports of one game" is the commonest reason to merge — and this un-hides whatever earlier merges are hiding too. `NpgsqlEndpointStore.ForListingAsync` is the union, one hop through `merge_log` (`merge_log_no_chains` makes one hop the whole walk), and a reverted merge gives the addresses back. It is deliberately not `ForGameAsync` and deliberately not on `IEndpointStore`: that method also answers `IdentityMatcher` and `DnsClaim`, where "this game's addresses" has to mean the ones it answered at itself. Widening it would let a DNS claim verified against the winner vouch for an absorbed game's hosts — a change to who can prove ownership of what, made as a side effect of a display fix. Living only on the concrete store keeps it out of reach of everything that consumes the interface. 3,208 tests pass across all six suites. The reverted-merge test passed before the change and was verified by mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughThe change adds listing-scoped endpoint retrieval for active merged games. Game pages use this retrieval path. PostgreSQL tests cover endpoint visibility after merges and reverted merges. Documentation records the distinction from game-scoped retrieval. ChangesMerged game endpoint visibility
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant GamePage
participant NpgsqlEndpointStore
participant PostgreSQL
GamePage->>NpgsqlEndpointStore: ForListingAsync(gameId)
NpgsqlEndpointStore->>PostgreSQL: Load endpoints for the listing and active merged games
PostgreSQL-->>NpgsqlEndpointStore: Endpoint rows
NpgsqlEndpointStore-->>GamePage: GameEndpoint records
Merge Risk: ⚪ Minimal · up to Active merged endpoints appear on the winner’s page, while reverted merges remain separate. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #188. The ChatMUD merge that closes it waits on this landing.
The defect
A merge says two listings are one game, so every address either answered at is an address of that game. The absorbed game keeps its endpoint rows — nothing moves, and the merge stays a redirect — but the page its slug 301s to read only the winner's own endpoints (
NpgsqlEndpointStore.ForGameAsync(row.Id)). The loser's addresses went on being crawled and were shown nowhere a reader could reach.Found about to run
game_merge chatmud ← chatmud-2.chatmud.com:7443, the TLS door #184 discovered, is the listing being absorbed. The merge would have taken the one new fact about ChatMUD off the only page left. It's probably not the only case: "two ports of one game" is the commonest reason to merge, and this also un-hides whatever earlier merges are hiding. I couldn't measure how many from here, because it needs amerge_logjoin the MCP tools don't expose.The fix
ForListingAsyncis the union: the game's own endpoints, plus those of any game with an un-revertedmerge_logrow pointing here. One hop is the whole walk, becausemerge_log_no_chainsforbids a game that is both absorbed and absorbing. A reverted merge is two games again, so its addresses stay on their own page.Deliberately not
ForGameAsync, and deliberately not onIEndpointStore. That method also answersIdentityMatcherandDnsClaim, where "this game's addresses" has to mean the ones it answered at itself. Widening it would let a DNS claim verified against the winner vouch for an absorbed game's hosts: a change to who can prove ownership of what, made as a side effect of a display fix. Because the union exists only on the concrete store, nothing that consumes the interface can reach it.Testing
Two new Postgres tests in
MergedGameVisibilityPostgresTests. The winner's page lists both addresses, with the TLS door stillTlsMeasured. A reverted merge keeps them apart: that test passed before the change, so I verified it by mutation (droppingreverted_at IS NULLfails it). 3,208 pass across all six suites.After this lands
game_merge winner=chatmud loser=chatmud-2, with the evidence from #188 as thebecause: same host,NAME = ChatMUDon both, the same count, a byte-identical connect screen.The other half of #188 needs no change.
110-10-160-150-4001is a real Dead Souls game on Intermud-3 whose announced port serves its owner's Synology NAS admin page, so it stays listed (comment).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes