fix: verify the #info RustMaps render against the server's own world - #88
Merged
Conversation
The #info map is resolved from `(worldSize, seed)` alone, and that pair does not identify a Rust world. WEREWOLF GAMING reports size 3700 seed 1900693728 over Rust+ while running a pre-generated level — its `GetInfo.Map` reads `procedural__3700_FHv7dBVBBUOBMxTGE8eiuw`, not `Procedural Map`, so `server.seed`/`server.worldsize` are leftover config values — and RustMaps generated a completely different island, which the bot posted as that server's map for the whole wipe. Measured against the live server: 0 of 8 major monuments (ferry terminal, military tunnels, power plant, airfield, water treatment, excavator, dome, junkyard) land anywhere near their RustMaps counterparts, and only 11% of the 63 monuments it reports have ANY RustMaps monument within 50 m. A server that stays up across a map-gen change drifts the same way. The ground truth was already in memory. `GetMap` returns the server's own monuments and map image, cached per connected window since #87, so `RustMapsMapMatcher` checks each ready render against the monuments its requesting servers actually report before that render is ever shown. It is a position fingerprint, not a name comparison: Rust+ tokens map many-to-one onto `MonumentType` (both harbors collapse to one, swamps and labs match by prefix), so comparing types would report false mismatches. The only wrinkle is the origin — Rust+ counts from the map corner, RustMaps from the world centre. The verdicts sit far apart (identical maps agree to the metre; this one scored 11%), so the thresholds are not delicate. The verdict is per (key, server), not per key: two servers can share a (size, seed) while only one of them runs that world. Undecidable — server offline, no monuments yet — is not a verdict; the render stays withheld and the next tick tries again, so a failed fetch can never latch the wrong map in. A mismatched server gets the map Rust+ itself serves, attached to the message and shown through `attachment://`. That is the first upload the reconciler handles, and it re-renders every pass, so the file is named after a hash of its content: an unchanged name means "already posted, leave it", a changed one means the map itself moved on and only a repost can carry the new file. Discord folds an attachment that an embed references INTO that embed — it rewrites the embed's image URL to the CDN one and returns an EMPTY attachments array. Reading only that array reports "no attachment" for exactly the messages that have one, so the first live run re-uploaded 680 KB every reconcile. `LiveMessage.From` therefore recovers the name from the embed's CDN URL, minus the rotating signature query string. The fake gateway now mirrors that folding; echoing the payload back is what let the first version look correct in tests and churn in production. Committing a RustMaps fixture also woke `RustMapsParityTests`, dormant since it was written for want of one. It failed: it derived grid rows from the south edge while the projection derives them from the north, which disagree whenever the world size is not a whole multiple of the cell size (3700 is not). It now checks the projection against `MapGrid.LabelFor` — the labels the bot actually quotes to players. `tools/RustPlusBot.MapParity` had the same centred-vs-corner bug and was drawing its crosshairs off the icons it claims to land on. Verified on the live bot: the mismatch is detected 20s after connect, the #info map is replaced by an image byte-identical to the one Rust+ serves (sha256 0bcdf958d810…), and it then survives four minutes of reconciles with no repost, no edit and no rate-limit warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The reconciler’s new UpToDate shortcut can prevent legitimate non-attachment updates (e.g., localization changes) from being applied to attachment-backed messages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents the bot from posting an incorrect RustMaps #info map when a server’s reported (worldSize, seed) does not uniquely identify its actual world (e.g., custom/pregenerated levels), by verifying RustMaps renders against the server’s own monument positions and falling back to the Rust+ served map image when mismatched. It also hardens the workspace reconciler’s attachment handling to avoid repeated uploads when Discord folds attachment:// uploads into embeds.
Changes:
- Add RustMaps render verification via monument-position matching per (key, server), withholding unverified renders and falling back to a Rust+ attachment on mismatch.
- Extend workspace message reconciliation/gateway plumbing to correctly detect embed-folded attachments and avoid repost/upload churn.
- Add/adjust regression tests + fixtures, and fix coordinate-origin assumptions in parity tooling/tests.
File summaries
| File | Description |
|---|---|
| tools/RustPlusBot.MapParity/Program.cs | Fix overlay/grid projection by shifting RustMaps-centered coords to Rust+ corner-origin. |
| tests/RustPlusBot.Localization.Tests/StringsResourceParityTests.cs | Update expected key count for new localized string. |
| tests/RustPlusBot.Features.Workspace.Tests/Reconciler/WorkspaceReconcilerAttachmentTests.cs | New tests covering attachment repost vs edit behavior, including embed-folded uploads. |
| tests/RustPlusBot.Features.Workspace.Tests/Reconciler/ReconcilerHarness.cs | Add attachment-aware renderer/harness support for reconciler tests. |
| tests/RustPlusBot.Features.Workspace.Tests/Messages/ServerInfoMapMessageRendererTests.cs | Update tests to new IInfoMapReadModel.Resolve flow and add mismatch fallback assertions. |
| tests/RustPlusBot.Features.Workspace.Tests/Gateway/LiveMessageTests.cs | New tests for recovering attachment filename from embed CDN URL. |
| tests/RustPlusBot.Features.Workspace.Tests/Fakes/FakeWorkspaceGateway.cs | Fake now mirrors Discord behavior for embed-folded attachments. |
| tests/RustPlusBot.Features.Map.Tests/RustMapsParityTests.cs | Fix parity assertion to use MapGrid.LabelFor and correct origin shift. |
| tests/RustPlusBot.Features.Map.Tests/RustMaps/RustMapsMapMatcherTests.cs | New fixture-based tests for map matching (match/mismatch/unknown). |
| tests/RustPlusBot.Features.Map.Tests/RustMaps/RustMapsMapCoordinatorTests.cs | Update coordinator tests to new per-server verification + resolve semantics. |
| tests/RustPlusBot.Features.Map.Tests/Hosting/InfoMapHostedServiceTests.cs | Add tests for mismatch/unknown handling and for publishing after verification. |
| tests/RustPlusBot.Features.Map.Tests/Fixtures/rustplus-monuments-3700-1900693728.json | New Rust+ monument fixture from incident. |
| tests/RustPlusBot.Features.Map.Tests/Fixtures/rustmaps-3700-1900693728.json | New RustMaps map+monuments fixture from incident. |
| src/RustPlusBot.Localization/Strings.resx | Add map.info.custom English string for mismatch fallback footer. |
| src/RustPlusBot.Localization/Strings.fr.resx | Add map.info.custom French translation. |
| src/RustPlusBot.Features.Workspace/Reconciler/WorkspaceReconciler.cs | Use live-message inspection for attachment identity; avoid reposts when attachment unchanged. |
| src/RustPlusBot.Features.Workspace/Messages/ServerInfoMapMessageRenderer.cs | Render verified RustMaps view or attach Rust+ map on mismatch; stable content-hash filename. |
| src/RustPlusBot.Features.Workspace/Gateway/MessagePayload.cs | Add optional MessageAttachment support for uploads. |
| src/RustPlusBot.Features.Workspace/Gateway/LiveMessage.cs | New live-message model to recover attachment filenames from embed-folded uploads. |
| src/RustPlusBot.Features.Workspace/Gateway/IWorkspaceGateway.cs | Replace existence check with live-message fetch returning attachment identity. |
| src/RustPlusBot.Features.Workspace/Gateway/DiscordWorkspaceGateway.cs | Implement live-message fetch + attachment upload path for posting; leave attachments untouched on edit. |
| src/RustPlusBot.Features.Map/RustMaps/RustMapsReadyMap.cs | Store RustMaps monument list alongside ready render for later verification. |
| src/RustPlusBot.Features.Map/RustMaps/RustMapsMapMatcher.cs | New monument proximity matcher to verify render vs server world. |
| src/RustPlusBot.Features.Map/RustMaps/RustMapsMapMatch.cs | New enum for match verdict (Unknown/Match/Mismatch). |
| src/RustPlusBot.Features.Map/RustMaps/RustMapsMapCoordinator.cs | Track per-(key, server) verdicts; expose Resolve, ReadyKeys, SetMatch. |
| src/RustPlusBot.Features.Map/RustMaps/RustMapsGenerationDriver.cs | Persist monuments when marking a render ready. |
| src/RustPlusBot.Features.Map/RustMaps/IRustMapsMapCoordinator.cs | Extend coordinator contract for per-server match verdicts and ready keys. |
| src/RustPlusBot.Features.Map/Hosting/InfoMapHostedService.cs | Verify ready renders against server monuments and publish readiness per (key, server). |
| src/RustPlusBot.Abstractions/Map/InfoMapResolution.cs | New shared InfoMapStatus + InfoMapResolution abstraction. |
| src/RustPlusBot.Abstractions/Map/IInfoMapReadModel.cs | Change read seam to return an InfoMapResolution per server. |
| README.md | Document #info verification behavior and mismatch fallback. |
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
364
to
368
| foreach (var item in items) | ||
| { | ||
| if (item.IsEmpty) | ||
| if (item.IsEmpty || item.UpToDate) | ||
| { | ||
| continue; |
Owner
Author
There was a problem hiding this comment.
Good catch, fixed in 76f2462 — UpToDate is gone entirely.
You're right that the skip was too broad: the upload is the only part an edit cannot carry, so a guild switching culture would have kept its map embed in the old language for as long as the image didn't change, which for a static-per-wipe map means the whole wipe.
I verified the premise against the API rather than assuming it, since the #info map references its upload through attachment:// and I wanted to be sure Discord doesn't drop it. Posted an embed referencing its upload, then PATCHed only embeds (what ModifyMessageAsync sends):
before: title "edit-probe BEFORE", attachments [], embed image cdn.discordapp.com/attachments/...
after: title "edit-probe AFTER (title changed)", attachments [], embed image UNCHANGED (200 on fetch)
So a matching file name now just means "edit in place", and only a changed or dropped upload still forces the delete-and-repost that carries the new file. The flag, its reset in the channel-order repair and the extra MessageItem field are all removed.
The attachment tests asserted "never edited", which was the wrong expectation rather than a real guarantee — they now assert "never re-uploaded", plus Text_around_an_unchanged_upload_is_still_edited_in_place pins exactly the scenario you described (retitle around an unchanged upload → one post, one edit, new title live).
Comment on lines
100
to
+105
|
|
||
| var message = await channel.GetMessageAsync(messageId).ConfigureAwait(false); | ||
| return message is not null; | ||
| return message is null | ||
| ? null | ||
| : LiveMessage.From(message.Id, message.Attachments.FirstOrDefault()?.Filename, | ||
| message.Embeds.FirstOrDefault()?.Image?.Url); |
Owner
Author
There was a problem hiding this comment.
Agreed, fixed in 76f2462. PostMessageAsync and EditMessageAsync had the same gap, so all three now thread the token through RequestOptions.CancelToken via a single Options(cancellationToken) helper, which the already-correct DeleteMessageAsync path reuses instead of building its own.
HandyS11
added a commit
that referenced
this pull request
Sep 8, 2026
Copilot review on #88. `UpToDate` skipped the post/edit step entirely once a live message already carried the payload's upload, which is too broad: the upload is the only part an edit cannot carry. A guild switching culture would keep its map embed in the old language for as long as the image itself did not change — the whole wipe, since the image is static per map. Editing is safe because the edit never mentions attachments and Discord keeps them. Verified against the API rather than assumed: posting an embed that references its upload through `attachment://` and then PATCHing only `embeds` leaves the attachment in place, the embed's CDN image URL unchanged and still fetchable. So the flag goes entirely — a matching file name now just means "edit in place", and only a changed or dropped upload still forces the delete-and-repost that carries the new file. `GetLiveMessageAsync` ignored its cancellation token, so reconcile work could not unwind promptly on shutdown; `PostMessageAsync` and `EditMessageAsync` had the same gap. All three now thread it through `RequestOptions.CancelToken` via one helper, which the already-correct delete path reuses. The attachment tests asserted "never edited", which was the wrong expectation rather than a real guarantee — they now assert "never re-uploaded", and a new test pins the reported scenario: retitle around an unchanged upload, and the message must be edited in place (one post, one edit, new title live) instead of reposted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot review on #88. `UpToDate` skipped the post/edit step entirely once a live message already carried the payload's upload, which is too broad: the upload is the only part an edit cannot carry. A guild switching culture would keep its map embed in the old language for as long as the image itself did not change — the whole wipe, since the image is static per map. Editing is safe because the edit never mentions attachments and Discord keeps them. Verified against the API rather than assumed: posting an embed that references its upload through `attachment://` and then PATCHing only `embeds` leaves the attachment in place, the embed's CDN image URL unchanged and still fetchable. So the flag goes entirely — a matching file name now just means "edit in place", and only a changed or dropped upload still forces the delete-and-repost that carries the new file. `GetLiveMessageAsync` ignored its cancellation token, so reconcile work could not unwind promptly on shutdown; `PostMessageAsync` and `EditMessageAsync` had the same gap. All three now thread it through `RequestOptions.CancelToken` via one helper, which the already-correct delete path reuses. The attachment tests asserted "never edited", which was the wrong expectation rather than a real guarantee — they now assert "never re-uploaded", and a new test pins the reported scenario: retitle around an unchanged upload, and the message must be edited in place (one post, one edit, new title live) instead of reposted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
HandyS11
force-pushed
the
fix/verify-info-map-against-server
branch
from
September 8, 2026 00:17
6c5832a to
76f2462
Compare
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
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.
The defect
The
#infomap is resolved from(worldSize, seed)alone, and that pair does not identify a Rust world.WEREWOLF GAMING reports size 3700 / seed 1900693728 over Rust+ while running a pre-generated level — its
GetInfo.Mapreadsprocedural__3700_FHv7dBVBBUOBMxTGE8eiuw, notProcedural Map, soserver.seed/server.worldsizeare leftover config values. RustMaps duly generated a completely different island, and the bot posted it as that server's map for the whole wipe.Measured against the live server (Rust+
GetMapvs the RustMaps monument list for that key):0 of 8 major monuments line up, and only 11% of the 63 monuments the server reports have any RustMaps monument within 50 m. A server that stays up across a map-gen change drifts the same way.
The fix
The ground truth was already in memory:
GetMapreturns the server's own monuments and map image, cached per connected window since #87.RustMapsMapMatchernow checks each ready render against the monuments its requesting servers actually report, before that render is ever shown.It is a position fingerprint, not a name comparison — Rust+ tokens map many-to-one onto
MonumentType(both harbors collapse to one, swamps and labs match by prefix), so comparing types would report false mismatches. The only wrinkle is the origin: Rust+ counts from the map corner, RustMaps from the world centre. The verdicts sit far apart (identical maps agree to the metre; this one scored 11%), so the thresholds are not delicate.The verdict is per (key, server), not per key: two servers can share a
(size, seed)while only one runs that world. "Undecidable" — server offline, no monuments yet — is not a verdict: the render stays withheld and the next tick retries, so a failed fetch can never latch the wrong map in.A mismatched server gets the map Rust+ itself serves, attached to the message and shown through
attachment://, with a footer explaining why.Attachments in the reconciler
This is the first upload the reconciler handles, and it re-renders every pass, so the file is named after a hash of its content: an unchanged name means "already posted, leave it"; a changed one means the map moved on and only a repost can carry the new file.
Discord folds an attachment an embed references into that embed — it rewrites the embed image URL to the CDN one and returns an empty
attachmentsarray. Reading only that array reports "no attachment" for exactly the messages that have one, so the first live run re-uploaded 680 KB on every reconcile.LiveMessage.Fromrecovers the name from the embed's CDN URL, minus the rotating signature query string. The fake gateway now mirrors that folding — echoing the payload straight back is what let the first version look correct in tests and churn in production.Two things this uncovered
RustMapsParityTests, dormant since it was written for want of one. It failed: it derived grid rows from the south edge while the projection derives them from the north, which disagree whenever the world size is not a whole multiple of the cell size (3700 is not). It now checks the projection againstMapGrid.LabelFor— the labels the bot actually quotes to players.tools/RustPlusBot.MapParityhad the same centred-vs-corner bug and was drawing its crosshairs off the very icons its README says they land on.Verification
Run against the live bot and the real server:
The RustMaps render for size 3700 seed 1900693728 does not match the world … showing the server's own map instead.#infomap is replaced by an image byte-identical to what Rust+ serves (sha2560bcdf958d810…, 695,603 B).Both fixtures in
tests/RustPlusBot.Features.Map.Tests/Fixtures/are the real data from this incident, so the regression is pinned by the case that caused it. The embed-attachment regression test fails (3 posts instead of 1) withoutLiveMessage.From's CDN lookup.1443 tests pass.
Known gap
With no RustMaps API key configured,
IInfoMapReadModelis not registered, so a custom-map server still gets no#infomap at all — the fallback only engages when RustMaps is configured and disagrees. Worth a follow-up if that setup matters.🤖 Generated with Claude Code