Skip to content

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

Open
nschimme wants to merge 2 commits into
masterfrom
fix-gmcp-external-discord-get-5724644234687142926
Open

Fix GMCP error caused by forwarding External.Discord.Get to MUME#244
nschimme wants to merge 2 commits into
masterfrom
fix-gmcp-external-discord-get-5724644234687142926

Conversation

@nschimme

@nschimme nschimme commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Fixes an issue where MUD clients sending External.Discord.Get caused MUME to reply with a GMCP error message that MMapper printed as MUME.Client protocol error: {"message":"unsupported message \"External.Discord.Get\""}. External.Discord.Get is now registered as a GMCP message type and eaten in UserTelnet::virt_receiveGmcpMessage.


PR created automatically by Jules for task 5724644234687142926 started by @nschimme

Summary by Sourcery

Handle External.Discord.Get messages locally to avoid MUME GMCP errors.

Bug Fixes:

  • Prevent unsupported External.Discord.Get messages from being forwarded to MUME and triggering GMCP protocol errors.

Enhancements:

  • Register External.Discord.Get as a recognized GMCP message type.

Tests:

  • Add coverage for deserializing and identifying External.Discord.Get messages.

Eat External.Discord.Get in UserTelnet instead of forwarding it to
MUME. MUME does not support External.Discord.Get and responds with
a MUME.Client.Error, which MMapper was displaying as a protocol error.
@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@sourcery-ai

sourcery-ai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Registers the GMCP message type External.Discord.Get, updates the total GMCP message count, consumes the message in the telnet proxy to avoid MUME protocol errors, and extends tests to cover both External.Discord.Hello and External.Discord.Get handling.

Sequence diagram for External.Discord.Get GMCP handling

sequenceDiagram
    actor MudClient
    participant UserTelnet
    participant MUME

    MudClient->>UserTelnet: sendExternalDiscordGet()
    UserTelnet->>UserTelnet: virt_receiveGmcpMessage(msg)
    alt [msg.isExternalDiscordGet()]
        UserTelnet-->>MudClient: return (message consumed)
    else [other GMCP message]
        UserTelnet->>MUME: forwardGmcpMessage(msg)
        MUME-->>MudClient: gmcpResponse
    end
Loading

File-Level Changes

Change Details Files
Register External.Discord.Get as a recognized GMCP message type and keep the enum count consistent.
  • Added External.Discord.Get to the GMCP message type X-macro list with its internal identifier and string names.
  • Updated the static_assert for NUM_GMCP_MESSAGES to reflect the additional GMCP message type.
src/proxy/GmcpMessage.h
Prevent forwarding External.Discord.Get to MUME by consuming it in the telnet proxy.
  • Added a guard in virt_receiveGmcpMessage to early-return when the message is External.Discord.Get.
  • Documented in a comment that MUME does not support External.Discord.Get and would otherwise respond with a client error.
src/proxy/UserTelnet.cpp
Extend proxy tests to cover External.Discord.Get and strengthen checks for External.Discord.Hello.
  • Verified that deserialization of External.Discord.Hello sets the correct name and type predicate.
  • Added deserialization and assertions for External.Discord.Get, including name, JSON absence, and type predicate.
tests/TestProxy.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 2 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/TestProxy.cpp" line_range="34-42" />
<code_context>
     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());
 }

</code_context>
<issue_to_address>
**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:

```cpp
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.
</issue_to_address>

### Comment 2
<location path="tests/TestProxy.cpp" line_range="39-42" />
<code_context>
     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());
 }

</code_context>
<issue_to_address>
**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:

```cpp
    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).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread tests/TestProxy.cpp
Comment on lines 34 to +42
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());

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 thread tests/TestProxy.cpp
Comment on lines +39 to +42
GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
QVERIFY(!gmcp4.getJson());
QVERIFY(gmcp4.isExternalDiscordGet());

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).

Eat External.Discord.Get in UserTelnet instead of forwarding it to
MUME. MUME does not support External.Discord.Get and responds with
a MUME.Client.Error, which MMapper was displaying as a protocol error.
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 25.09%. Comparing base (1e3eafb) to head (5890c75).

Files with missing lines Patch % Lines
src/proxy/UserTelnet.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   25.08%   25.09%           
=======================================
  Files         528      528           
  Lines       44211    44218    +7     
  Branches     4793     4796    +3     
=======================================
+ Hits        11092    11097    +5     
- Misses      33119    33121    +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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