Skip to content

Fix GMCP error caused by forwarding External.Discord.Get to MUME #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nschimme wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix GMCP error caused by forwarding External.Discord.Get to MUME #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5325046
Fix GMCP error caused by External.Discord.Get
nschimme Aug 20, 2026
5890c75
Fix GMCP error caused by External.Discord.Get
nschimme Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/proxy/GmcpMessage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class ParseEvent;
X(EVENT_MOVED, EventMoved, "event.moved", "Event.Moved") \
X(EVENT_MOON, EventMoon, "event.moon", "Event.Moon") \
X(EVENT_SUN, EventSun, "event.sun", "Event.Sun") \
X(EXTERNAL_DISCORD_GET, ExternalDiscordGet, "external.discord.get", "External.Discord.Get") \
X(EXTERNAL_DISCORD_HELLO, \
ExternalDiscordHello, \
"external.discord.hello", \
Expand Down Expand Up @@ -66,7 +67,7 @@ enum class NODISCARD GmcpMessageTypeEnum {
#define X_COUNT(...) +1
static constexpr const size_t NUM_GMCP_MESSAGES = XFOREACH_GMCP_MESSAGE_TYPE(X_COUNT);
#undef X_COUNT
static_assert(NUM_GMCP_MESSAGES == 30);
static_assert(NUM_GMCP_MESSAGES == 31);
DEFINE_ENUM_COUNT(GmcpMessageTypeEnum, NUM_GMCP_MESSAGES)

namespace tags {
Expand Down
5 changes: 5 additions & 0 deletions src/proxy/UserTelnet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,11 @@ void UserTelnet::virt_receiveGmcpMessage(const GmcpMessage &msg)
return;
}

// Eat External.Discord.Get as MUME does not support it and would return a MUME.Client.Error
if (msg.isExternalDiscordGet()) {
return;
}

const bool requiresRewrite = msg.getJson()
&& (msg.isCoreSupportsAdd() || msg.isCoreSupportsSet()
|| msg.isCoreSupportsRemove())
Expand Down
6 changes: 6 additions & 0 deletions tests/TestProxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ void TestProxy::gmcpMessageDeserializeTest()
GmcpMessage gmcp3 = GmcpMessage::fromRawBytes(R"(External.Discord.Hello)");
QCOMPARE(gmcp3.getName().toQByteArray(), QByteArray("External.Discord.Hello"));
QVERIFY(!gmcp3.getJson());
QVERIFY(gmcp3.isExternalDiscordHello());

GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
QVERIFY(!gmcp4.getJson());
QVERIFY(gmcp4.isExternalDiscordGet());
Comment on lines 34 to +42

@sourcery-ai sourcery-ai Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Add corresponding serialize test for External.Discord.Get to keep deserialize/serialize coverage symmetric.

To fully validate the new GMCP type wiring, please also add a gmcpMessageSerializeTest case that builds a GmcpMessage of type ExternalDiscordGet, serializes it, and verifies the raw bytes (including the name and absence of JSON). This keeps deserialize/serialize tests symmetric and will help catch regressions if the enum or name mapping changes.

Suggested implementation:

void TestProxy::gmcpMessageSerializeTest()
{
    // existing GMCP serialize tests should remain here

    // Symmetric serialize test for External.Discord.Get
    GmcpMessage gmcpExternalDiscordGet(/* TODO: use the appropriate constructor or enum for ExternalDiscordGet */);
    QByteArray rawExternalDiscordGet = gmcpExternalDiscordGet.toRawBytes();
    QCOMPARE(rawExternalDiscordGet, QByteArray("External.Discord.Get"));
    QVERIFY(!gmcpExternalDiscordGet.getJson());
}

To correctly integrate this change without overwriting existing tests, please:

  1. Move the new GmcpMessage gmcpExternalDiscordGet block into the existing body of gmcpMessageSerializeTest(), alongside the other serialize cases, instead of replacing the whole function. Place it near the serialize test for External.Discord.Hello to keep the coverage grouped.
  2. Replace the /* TODO: use the appropriate constructor or enum for ExternalDiscordGet */ with the actual way you construct a GmcpMessage of type ExternalDiscordGet in your codebase (for example, using the corresponding enum or factory method already used for ExternalDiscordHello in the serialize tests).
  3. If your serialization API uses a different method than toRawBytes() (e.g. serialize() or similar), update the call accordingly so that the comparison verifies the exact raw GMCP bytes, matching the format used in the deserialize tests.

Comment on lines +39 to +42

@sourcery-ai sourcery-ai Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Add a test that verifies UserTelnet::virt_receiveGmcpMessage eats External.Discord.Get and does not forward or produce an error.

Right now the tests only cover GMCP deserialization and isExternalDiscordGet(). To validate the actual behavior change and prevent regressions, please add or extend a test that exercises UserTelnet::virt_receiveGmcpMessage with an External.Discord.Get message and asserts there is no GMCP error and no downstream handling (no write to the MUME side, no error callback, etc.).

Suggested implementation:

    GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
    QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
    QVERIFY(!gmcp4.getJson());
    QVERIFY(gmcp4.isExternalDiscordGet());
}

void TestProxy::externalDiscordGetIsEatenByUserTelnet()
{
    // Arrange: set up a UserTelnet instance wired through the proxy test harness.
    // This should follow the same pattern as other tests that exercise
    // UserTelnet::virt_receiveGmcpMessage in this file.
    UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */;
    QVERIFY(userTelnet);

    // Use the same mechanism other tests use to observe writes to the MUME side
    // and GMCP errors (typically QSignalSpy on the appropriate signals).
    QSignalSpy mumeWriteSpy(userTelnet, SIGNAL(writeMume(QByteArray)));
    QSignalSpy gmcpErrorSpy(userTelnet, SIGNAL(gmcpError(QString)));

    // Act: deliver an External.Discord.Get GMCP message to virt_receiveGmcpMessage.
    GmcpMessage gmcpGet = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
    userTelnet->virt_receiveGmcpMessage(gmcpGet);

    // Assert: the message is eaten, no downstream handling and no error.
    QCOMPARE(mumeWriteSpy.count(), 0);
    QCOMPARE(gmcpErrorSpy.count(), 0);
}

void TestProxy::gmcpMessageSerializeTest()
  1. At the top of tests/TestProxy.cpp, add #include <QSignalSpy> if it is not already present to support the new spies.
  2. Replace the UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */; placeholder with the actual way this test suite obtains a UserTelnet instance (for example, using a member like m_userTelnet, a helper factory, or existing setup code used by other virt_receiveGmcpMessage tests).
  3. If the signals used to observe downstream handling differ from writeMume(QByteArray) and gmcpError(QString) in your codebase, adjust the QSignalSpy constructions and the assertions to match the real signal names and parameter types (e.g. writeToMume, gmcpErrorOccurred, or similar).
  4. Register the new test method with Qt’s test system the same way other tests in TestProxy are registered (usually nothing extra is needed beyond the member function, but if there is a manual test list or macro, ensure externalDiscordGetIsEatenByUserTelnet is included).

}

void TestProxy::gmcpMessageSerializeTest()
Expand Down
Loading