Skip to content

chore: report exception of command on failure#8852

Open
NickKhalow wants to merge 1 commit into
devfrom
chore/report-command-error
Open

chore: report exception of command on failure#8852
NickKhalow wants to merge 1 commit into
devfrom
chore/report-command-error

Conversation

@NickKhalow
Copy link
Copy Markdown
Contributor

@NickKhalow NickKhalow commented May 21, 2026

What does this PR change?

When a chat command throws an exception, the error was caught and a generic "🔴 Error running command." message was shown to the user — but the exception itself was silently swallowed, making it impossible to diagnose failures from logs.

This PR adds a ReportHub.LogError call inside the catch block in CommandsHandleChatMessageBus.HandleChatCommandAsync, so the full exception (message + stack trace) is now reported to the diagnostics system alongside the user-facing error message.

Changes

  • CommandsHandleChatMessageBus.cs: Capture the exception variable in the catch block and log it via ReportHub.LogError(ReportCategory.UNSPECIFIED, ...).
  • Added using DCL.Diagnostics import.

Test Instructions

Steps (standard run):

metaforge explorer run 8852

Expected result:

  1. Open the chat and trigger a command that is expected to fail (e.g., malformed input).
  2. Confirm the user still sees the "🔴 Error running command." message in chat.
  3. Check the logs — the full exception details should now appear via ReportHub.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

@NickKhalow NickKhalow self-assigned this May 21, 2026
@NickKhalow NickKhalow added the no QA needed Used to tag pull requests that does not require QA validation label May 21, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

@NickKhalow NickKhalow marked this pull request as ready for review May 21, 2026 13:45
@NickKhalow NickKhalow requested review from a team as code owners May 21, 2026 13:45
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 21, 2026

Claude finished @NickKhalow's task in 1m 18s —— View job


Review

  • Read CLAUDE.md and project standards
  • Read changed file and diff
  • Analyze for issues
  • Post review

Root-Cause Check

Problem being solved: Exceptions thrown by chat commands were silently swallowed in the catch block, making failures undiagnosable from logs. The fix adds ReportHub logging alongside the existing user-facing error message.

Assessment: The diff directly addresses the observability gap — this fixes the root cause, not a symptom.


Blocking Issues

1. OperationCanceledException should not be logged as an error

CommandsHandleChatMessageBus.cs, line 73

The catch (Exception e) block catches OperationCanceledException (a subtype of Exception). This exception is expected and benign — it's thrown when commandCts.SafeRestart() cancels the previous in-flight command. Logging it via ReportHub.LogError will produce false-positive error reports every time a user types a new command before the previous one completes.

Per CLAUDE.md §9: "Ignore OperationCanceledException".

Fix:

catch (Exception e) when (e is not OperationCanceledException)
{ 
    SendFromSystem(channelId, channelType, "🔴 Error running command."); 
    ReportHub.LogException(e, new ReportData(ReportCategory.UNSPECIFIED));
}
catch (OperationCanceledException) { /* expected on command restart */ }

Fix this →


2. Use ReportHub.LogException instead of ReportHub.LogError

CommandsHandleChatMessageBus.cs, line 76

Per CLAUDE.md §9: "Log/report all others via ReportHub.LogException". LogError takes a plain string; LogException takes the Exception directly and preserves structured exception data (type, message, stack trace) in the diagnostics system as a proper exception event.

Fix: Replace ReportHub.LogError(ReportCategory.UNSPECIFIED, $"Error running command: {e}") with ReportHub.LogException(e, new ReportData(ReportCategory.UNSPECIFIED)).

Fix this →


Non-Blocking Warning

⚠️ Using statement ordering: using DCL.Diagnostics; is appended after using Utility; (line 11) rather than sorted alphabetically within the DCL.* group (should appear after DCL.Chat.History and before DCL.RealmNavigation). This is a formatting convention issue — not a functional concern, but worth keeping consistent.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file change adding exception logging to an existing catch block in the chat command bus — no ECS, async patterns, or system registration touched.
QA_REQUIRED: YES

catch (Exception e)
{
SendFromSystem(channelId, channelType, "🔴 Error running command.");
ReportHub.LogError(ReportCategory.UNSPECIFIED, $"Error running command: {e}");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Per CLAUDE.md §9, exceptions should be reported via ReportHub.LogException (not LogError with a string). LogException preserves structured exception data in the diagnostics system.

Also, OperationCanceledException should not be logged as an error — it's expected when commandCts.SafeRestart() cancels an in-flight command.

Suggested change
ReportHub.LogError(ReportCategory.UNSPECIFIED, $"Error running command: {e}");
ReportHub.LogException(e, new ReportData(ReportCategory.UNSPECIFIED));

And add a guard to the catch clause:

catch (Exception e) when (e is not OperationCanceledException)

@github-actions github-actions Bot removed the no QA needed Used to tag pull requests that does not require QA validation label May 21, 2026
@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8852, run #26232829470

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2700 2699
CPU average 33.3 ms 33.3 ms -0.0 ms 0.0%
CPU 1% worst 33.6 ms 34.1 ms -0.5 ms 1.5%
CPU 0.1% worst 35.2 ms 40.2 ms -5.0 ms 12.5% 🟢
GPU average 7.7 ms 7.9 ms -0.2 ms 2.7%
GPU 1% worst 19.7 ms 20.2 ms -0.5 ms 2.3%
GPU 0.1% worst 23.9 ms 23.4 ms 0.5 ms -2.1%

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.

2 participants