Repository navigation
Conversation
…ty test The C API test referenced 9 of the 39 exported symbols. It now links and exercises the whole surface without initializing the library: - every TC9Set* registration function (the surface the worldserver uses), - lifecycle exports as safe no-ops before TC9InitLib, - GUID getters returning 0 and NATS publish/subscribe returning -1 before initialization, - every Go-era Call* helper reporting NoHandler/NoHook while nothing is registered, - hook round-trips (guild/group/registry/monitoring) verifying that all arguments are forwarded, including the newLeaderGuid parameter of OnGroupMemberRemovedHook. TC9InitLib is referenced but never called since it needs live services.
This was referenced Aug 30, 2026
There was a problem hiding this comment.
🟡 Changes recommended
The test claims to cover the Go-era Set/Call helper surface but currently doesn’t reference several Go-era Set* symbols for the gRPC-request families, so missing/renamed exports could still slip through undetected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands the libsidecar-cpp C API compatibility test so it exercises (and thereby helps lock down) the full exported ABI surface and key pre-initialization behavioral contracts, without calling TC9InitLib (since it requires live services).
Changes:
- Extends the compatibility test to reference all
TC9*exports and validate their expected behavior when the library is uninitialized (no-op lifecycle calls, GUID getters return 0, NATS calls fail). - Adds coverage for Go-era
Call*helpers returningNoHook/NoHandlerwhen nothing is registered. - Adds hook round-trip assertions to ensure
Set*+Call*forwards arguments correctly (includingnewLeaderGuid).
File summaries
| File | Description |
|---|---|
game-server/libsidecar-cpp/tests/c_api_compatibility_test.c |
Broadens the C ABI compatibility test to cover the full export surface and uninitialized/compat-layer contracts. |
Review details
- Files reviewed: 1/1 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
+207
to
211
| /* TC9InitLib presence (never called: it needs live services) */ | ||
| { | ||
| void (*init_ptr)(uint16_t, uint32_t, uint8_t, char*, uint32_t**, int*) = TC9InitLib; | ||
| CHECK(init_ptr != NULL, "TC9InitLib symbol missing"); | ||
| } |
Comment on lines
+8
to
+10
| * - NATS publish/subscribe report failure, | ||
| * - Call* helpers report NoHandler/NoHook until a handler is set, | ||
| * - hook Call* helpers forward every argument once a hook is set. |
… families Review follow-ups: the Go-era Set* of the items/money/interactions/ battleground families were not referenced, so a removed export would not have failed the build; they are now called (registration only - their live path is the gRPC service). The header comment now states that contract explicitly instead of implying a post-Set round-trip.
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.
Follow-up to #101. The C API compatibility test referenced 9 of the 39 exported symbols; this extends it to the whole surface, all without calling
TC9InitLib(it needs live services):TC9Set*registration function — the exact surface the worldserver uses;TC9Process*,TC9ReadyToAcceptPlayersFromMaps,TC9PlayerLeftBattleground,TC9BattlegroundStatusChanged,TC9GracefulShutdown) as safe no-ops before init;TC9NatsPublish/TC9NatsSubscribereturning -1 before init;Call*helper reporting NoHandler/NoHook while nothing is registered;newLeaderGuidparameter ofOnGroupMemberRemovedHook, which pins the compat-layer half of the fix in fix(libsidecar-cpp): close three parity gaps with the Go sidecar #101.This locks the ABI surface down before any deprecation of the Go sidecar path: a removed or renamed export now fails the build, and a behavioral drift in the uninitialized contract fails the test.
One observation while writing it: the
Call*shims of the gRPC-request families (items/money/interactions/battleground) always return NoHandler even after a handler is set (compat_api.cpplogs "Bridging not yet implemented"). That's harmless today — the live path goes through the gRPC service and the stored bindings — so the test asserts only their no-handler contract. Worth knowing if anything ever starts calling them directly.Verified: passes against current master (all assertions green,
ctestsuite unaffected).