Skip to content

fix: allowlist salvage_wreck and sell_ship in api-sync drift check - #27

Closed
statico-alt wants to merge 1 commit into
mainfrom
fix/client-api-sync-drift
Closed

fix: allowlist salvage_wreck and sell_ship in api-sync drift check#27
statico-alt wants to merge 1 commit into
mainfrom
fix/client-api-sync-drift

Conversation

@statico-alt

Copy link
Copy Markdown
Member

Summary

The api-sync drift check was failing on main: salvage_wreck and sell_ship are present in the client's COMMANDS block but missing from the live openapi.json, so the test flagged them as stale.

Both commands still exist server-side — a POST to each returns 401 (auth required), not 404 — they're simply undocumented in the OpenAPI spec. Added them to UNDOCUMENTED_IN_SPEC, which is exactly the remedy the test documents for this case. No client behavior change.

Player-Facing Release Notes

Both commands exist server-side (POST returns 401, not 404) but are absent
from openapi.json, so the api-sync drift check failed on main. Add them to
UNDOCUMENTED_IN_SPEC, matching the test's documented remedy.
@statico-alt

Copy link
Copy Markdown
Member Author

Superseded by #28 — recommend closing this one.

Two problems with the approach here:

  1. The premise doesn't hold. This PR allowlists salvage_wreck and sell_ship on the grounds that they exist server-side because a POST returns 401 rather than 404. A 401 comes from auth middleware before routing, so it can't distinguish a live command from a missing one. Both commands were genuinely removed server-side, and the release notes record it: sell_ship was removed in favour of list_ship_for_sale / sell_ship_to_order / scrap_ship, and salvage_wreck in favour of loot_wreck / tow_wrecksell_wreck / scrap_wreck. The client was advertising two dead commands.

  2. It doesn't actually fix CI. Allowlisting the two stale commands just moves the failure to the next assertion — roughly 19 shipped endpoints are missing from COMMANDS.

#28 drops the two dead commands, adds the missing endpoints, and is green. #26 (the gh#1874 wreck-render fix) is blocked behind whichever of these lands.

@statico-alt

Copy link
Copy Markdown
Member Author

Superseding this with #28, which has now merged.

The premise here was wrong: salvage_wreck and sell_ship are not merely undocumented, they are removed server-side. A POST returning 401 rather than 404 doesn't prove a command exists — auth is rejected before routing. Verified against the gameserver tree: zero Go references to either command, and both removals are recorded in the release notes (sell_ship removed in favour of list_ship_for_sale / sell_ship_to_order / scrap_ship; salvage_wreck removed in favour of loot_wreck or tow_wrecksell_wreck / scrap_wreck).

Allowlisting them would have kept the client advertising two dead commands to players. #28 removes them instead and fixes the underlying drift check. Closing.

@statico-alt
statico-alt deleted the fix/client-api-sync-drift branch July 28, 2026 02:35
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.

1 participant