Implement MuGlyph M1 foundation plus M2–M4 subsystems - #1
Merged
Conversation
Build out the MuGlyph client from the skeleton into a working, UI-agnostic core with a Terminal.Gui v2 front end. All of MuClient.Core is unit-tested (195 tests across Core, Graphics, and Scripting). Solution - MuGlyph.slnx with six projects on net10.0, central package management, shared build props, and a GitHub Actions CI matrix (ubuntu + windows). Core (UI-agnostic, fully testable) - AnsiParser: incremental SGR (16/256/24-bit, colon + semicolon forms), CSI/OSC recognition-and-discard; styled-span line model; ScrollbackBuffer. - TcpTransport (TLS via SslStream, IPv6) behind ITransport; TelnetSession wrapping TelnetNegotiationCore 2.5.3 with GMCP/MSSP/MSDP routing and GA/EOR prompt detection. - Automation: regex TriggerEngine (gag/highlight/rewrite/respond/spawn/script), AliasEngine, MacroEngine, IntervalScheduler. - Plain-text and HTML logging; JSON configuration + best-effort BeipMU importer. - Theme model + ThemeLibrary (Dark/Light/Solarized) with palette overrides. - WorldSession/SessionManager orchestrating the full pipeline. Graphics (no UI dependency) - Kitty graphics encoder + Unicode placeholders, Sixel and half-block fallbacks, and an environment-based capability probe. Scripting - Sandboxed MoonSharp ScriptHost exposing world/output/trigger/alias/timer/ gmcp/log with hot-reload. Tui - Terminal.Gui v2 app: truecolor OutputView with wrapping/scrollback, CommandInput with history + tab-completion, status line, key routing, theming. Tests use TUnit on Microsoft.Testing.Platform; run via `dotnet run --project`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015w2WJ5M22CZmg9md67zpeB
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughMuGlyph adds a .NET 10 solution with shared build configuration, core terminal/session functionality, graphics rendering, Lua scripting, a Terminal.Gui application, automated tests, documentation, an example script, and cross-platform GitHub Actions CI. ChangesMuGlyph runtime and application stack
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (1)
src/MuClient.Scripting/ScriptException.cs (1)
27-49: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueParse
DecoratedMessagefor the runtime-error line format too.MoonSharp 2.0.0 runtime errors use
DecoratedMessageentries like[string "chunk"]:4: Division by zero!, which contain no(range. Since this method only succeeds when it finds parentheses, ordinaryerror(...)/runtime errors leaveScriptException.Lineasnull. Update the extraction to handle the standard[source]:LINE: messageformat without changing the safe fallback for unknown message shapes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MuClient.Scripting/ScriptException.cs` around lines 27 - 49, Update TryExtractLine to also parse MoonSharp runtime messages in the “[source]:LINE: message” format when no parenthesized range is present. Preserve the existing parenthesized-range parsing and return null safely for malformed or unrecognized DecoratedMessage values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 20-21: Update the actions/checkout step in the CI workflow to
disable persisted credentials by setting its persist-credentials option to
false, while leaving the existing checkout behavior unchanged.
In `@CLAUDE.md`:
- Around line 30-31: Update the project count in the status text beginning “M1
delivered, plus substantial M2–M4 work.” to state that MuGlyph.slnx builds all
seven projects, while preserving the surrounding build and test information.
In `@src/MuClient.Core/Automation/Alias.cs`:
- Around line 29-32: Update the Alias.Regex property to construct the compiled
Regex with a finite execution timeout for persisted patterns. In
AliasEngine.Expand, catch RegexMatchTimeoutException around
alias.Regex.Match(input) and handle the timeout by skipping or safely rejecting
that alias without hanging user input processing.
In `@src/MuClient.Core/Automation/IntervalScheduler.cs`:
- Around line 56-74: Update the Timer callback in IntervalScheduler to catch
exceptions thrown by callback(), preventing scheduled script/user callbacks from
escaping the ThreadPool thread and terminating the process. Report or swallow
the exception according to the scheduler’s existing error-handling conventions,
while preserving the finally block that disposes one-shot handles.
In `@src/MuClient.Core/Automation/Trigger.cs`:
- Around line 61-65: Update the Trigger.Regex property to construct the compiled
Regex with an explicit finite match timeout, preserving the existing
case-sensitivity options. Also validate or compile Pattern when triggers are
added so malformed user-supplied expressions fail immediately with a clear error
rather than during TriggerEngine.Process.
In `@src/MuClient.Core/Logging/HtmlLogSink.cs`:
- Around line 147-171: Make HtmlLogSink disposal and flushing idempotent by
updating Dispose and Flush to return without touching _writer once _closed is
true. Preserve the existing first-dispose behavior, including writing the
closing HTML, flushing, and disposing the writer when _ownsWriter is set.
In `@src/MuClient.Core/Session/WorldSession.cs`:
- Around line 144-152: Serialize trigger response sends in
WorldSession.ProcessOutputLine by awaiting each SendRawAsync call or routing
responses through a single-writer queue, and observe/log any resulting
exceptions instead of discarding tasks; update
src/MuClient.Core/Session/WorldSession.cs lines 144-152. Add synchronization
around the stream.WriteAsync operation in TcpTransport.SendAsync, using a
SemaphoreSlim or equivalent to prevent overlapping writes from any caller;
update src/MuClient.Core/Transport/TcpTransport.cs lines 74-78.
- Around line 80-110: Update ConnectAsync to clean up any existing _telnet
before replacing it with a newly created transport, including detaching all
handlers registered in ConnectAsync and disposing the previous instance. Ensure
this cleanup applies when reconnecting from Faulted or Disconnected states while
preserving the existing connection-state guard and new connection setup.
In `@src/MuClient.Core/Telnet/TelnetSession.cs`:
- Around line 69-85: Update ConnectAsync to clean up the connected transport
when BuildInterpreterAsync or subsequent setup fails before the read loop is
established. Wrap the post-connect initialization around BuildInterpreterAsync,
ByteCallbackProperty.SetValue, and linked cancellation setup in failure handling
that disconnects the transport, preserves the original exception, and leaves the
session state unconnected.
- Around line 231-253: Update DisconnectAsync to clear _interpreter after the
disconnect cleanup completes, matching the reset performed by DisposeAsync.
Ensure subsequent ConnectAsync calls can reconnect and SendAsync, SendGmcpAsync,
and SetWindowSizeAsync observe the “not connected” state.
- Around line 206-223: Update TelnetSession.SendAsync to forward its
cancellationToken to the interpreter send operation, and ensure SendLineAsync
preserves that forwarding through SendAsync. For SendGmcpAsync, pass the token
to the interpreter if supported; otherwise remove the unused cancellation-token
parameter from the affected methods so their API does not imply cancellable
writes.
In `@src/MuClient.Core/Text/AnsiParser.cs`:
- Around line 143-171: Update ProcessEscape to route ESC P, ESC X, ESC ^, and
ESC _ into the existing string-consuming state so their payloads are consumed
through ST instead of emitted as text. Preserve the current OSC behavior,
including its termination handling, and add coverage in AnsiParserTests
confirming a DCS payload followed by ST leaves only the subsequent visible text.
In `@src/MuClient.Graphics/InlineImage.cs`:
- Around line 105-111: Update the Kitty branch in InlineImageRenderer to
transmit the virtual placement before the image data and placeholder grid,
preserving the required U+10EEEE anchoring behavior. Extend InlineImageOutput to
represent the combined virtual-placement and image transmission, then update
affected Kitty tests to validate the transmission order and placeholder result.
In `@src/MuClient.Graphics/KittyGraphicsProtocol.cs`:
- Around line 155-172: Update BuildPlaceholder to prevent image IDs above 24
bits from being silently truncated: either validate and reject values greater
than 0x00FFFFFF at the public API boundary, or encode the high byte using
Kitty’s supported third cell diacritic. Preserve correct round-trip identity for
every accepted image ID.
In `@src/MuClient.Tui/MuGlyphApp.cs`:
- Around line 88-93: Update the connection flow around session.ConnectAsync in
MuGlyphApp so the continuation that reads _output.Viewport and calls
SetWindowSizeAsync executes on the UI thread. Remove the ConfigureAwait(false)
from this UI-dependent path or explicitly marshal the post-connect work through
the same Application.Invoke mechanism used by the session-driven UI updates
elsewhere in the file.
- Around line 121-130: Update OnCommandEntered so failures from the
fire-and-forget session.SendUserInputAsync call are observed and surfaced to the
user through the existing TUI feedback or error-reporting mechanism. Preserve
the current null-session early return and command dispatch behavior while
ensuring transport exceptions do not remain unhandled.
- Around line 146-153: Update the Ctrl+Q check in the key handling switch’s
default branch to compare against Terminal.Gui’s uppercase Q key code, such as
KeyCode.Q, instead of casting the lowercase character 'q'. Preserve the existing
Application.RequestStop and key.Handled behavior.
In `@tests/MuClient.Core.Tests/Automation/IntervalSchedulerTests.cs`:
- Around line 12-14: Update both scheduler tests, including After_FiresOnce and
the additionally referenced test, to replace fixed Task.Delay waits with a
TaskCompletionSource completed by each timer callback. Await the callback signal
with a bounded timeout, then retain the existing count assertions.
---
Nitpick comments:
In `@src/MuClient.Scripting/ScriptException.cs`:
- Around line 27-49: Update TryExtractLine to also parse MoonSharp runtime
messages in the “[source]:LINE: message” format when no parenthesized range is
present. Preserve the existing parenthesized-range parsing and return null
safely for malformed or unrecognized DecoratedMessage values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f584113b-77f8-4be6-8a9e-db29db599f7b
📒 Files selected for processing (90)
.github/workflows/ci.ymlCLAUDE.mdDirectory.Build.propsDirectory.Packages.propsMuGlyph.slnxREADME.mdexamples/example.luasrc/MuClient.Core/Automation/Alias.cssrc/MuClient.Core/Automation/AliasEngine.cssrc/MuClient.Core/Automation/IntervalScheduler.cssrc/MuClient.Core/Automation/Macro.cssrc/MuClient.Core/Automation/MacroEngine.cssrc/MuClient.Core/Automation/Trigger.cssrc/MuClient.Core/Automation/TriggerEngine.cssrc/MuClient.Core/Configuration/AppConfiguration.cssrc/MuClient.Core/Configuration/BeipMuImporter.cssrc/MuClient.Core/Configuration/ConfigurationStore.cssrc/MuClient.Core/Configuration/RgbJsonConverter.cssrc/MuClient.Core/Configuration/TerminalColorJsonConverter.cssrc/MuClient.Core/Configuration/WorldDefinition.cssrc/MuClient.Core/Logging/HtmlLogSink.cssrc/MuClient.Core/Logging/ILogSink.cssrc/MuClient.Core/Logging/PlainTextLogSink.cssrc/MuClient.Core/MuClient.Core.csprojsrc/MuClient.Core/Session/SessionEvents.cssrc/MuClient.Core/Session/SessionManager.cssrc/MuClient.Core/Session/WorldSession.cssrc/MuClient.Core/Telnet/ITelnetSession.cssrc/MuClient.Core/Telnet/MsspConfigReader.cssrc/MuClient.Core/Telnet/TelnetEvents.cssrc/MuClient.Core/Telnet/TelnetSession.cssrc/MuClient.Core/Text/AnsiPalette.cssrc/MuClient.Core/Text/AnsiParser.cssrc/MuClient.Core/Text/ScrollbackBuffer.cssrc/MuClient.Core/Text/StyledLine.cssrc/MuClient.Core/Text/StyledSpan.cssrc/MuClient.Core/Text/StyledText.cssrc/MuClient.Core/Text/TerminalColor.cssrc/MuClient.Core/Text/TextStyle.cssrc/MuClient.Core/Theming/Theme.cssrc/MuClient.Core/Theming/ThemeLibrary.cssrc/MuClient.Core/Transport/ConnectionOptions.cssrc/MuClient.Core/Transport/ITransport.cssrc/MuClient.Core/Transport/TcpTransport.cssrc/MuClient.Graphics/CapabilityProbe.cssrc/MuClient.Graphics/GraphicsProtocol.cssrc/MuClient.Graphics/HalfBlockRenderer.cssrc/MuClient.Graphics/IImageSource.cssrc/MuClient.Graphics/InlineImage.cssrc/MuClient.Graphics/KittyGraphicsProtocol.cssrc/MuClient.Graphics/MuClient.Graphics.csprojsrc/MuClient.Graphics/SixelEncoder.cssrc/MuClient.Graphics/TerminalCapabilities.cssrc/MuClient.Scripting/IScriptWorld.cssrc/MuClient.Scripting/MuClient.Scripting.csprojsrc/MuClient.Scripting/ScriptException.cssrc/MuClient.Scripting/ScriptHost.cssrc/MuClient.Scripting/WorldSessionScriptBridge.cssrc/MuClient.Tui/ColorMapper.cssrc/MuClient.Tui/MuClient.Tui.csprojsrc/MuClient.Tui/MuGlyphApp.cssrc/MuClient.Tui/Program.cssrc/MuClient.Tui/Views/CommandInput.cssrc/MuClient.Tui/Views/OutputView.cstests/MuClient.Core.Tests/Automation/AliasAndMacroTests.cstests/MuClient.Core.Tests/Automation/IntervalSchedulerTests.cstests/MuClient.Core.Tests/Automation/TriggerEngineTests.cstests/MuClient.Core.Tests/Configuration/ConfigurationTests.cstests/MuClient.Core.Tests/Logging/LogSinkTests.cstests/MuClient.Core.Tests/MuClient.Core.Tests.csprojtests/MuClient.Core.Tests/Session/FakeTelnetSession.cstests/MuClient.Core.Tests/Session/WorldSessionTests.cstests/MuClient.Core.Tests/Telnet/FakeTransport.cstests/MuClient.Core.Tests/Telnet/TelnetSessionTests.cstests/MuClient.Core.Tests/Text/AnsiParserTests.cstests/MuClient.Core.Tests/Text/ScrollbackBufferTests.cstests/MuClient.Core.Tests/Text/StyledTextTests.cstests/MuClient.Core.Tests/Theming/ThemeTests.cstests/MuClient.Graphics.Tests/CapabilityProbeTests.cstests/MuClient.Graphics.Tests/HalfBlockRendererTests.cstests/MuClient.Graphics.Tests/InlineImageRendererTests.cstests/MuClient.Graphics.Tests/KittyGraphicsProtocolTests.cstests/MuClient.Graphics.Tests/MemoryImageSourceTests.cstests/MuClient.Graphics.Tests/MuClient.Graphics.Tests.csprojtests/MuClient.Graphics.Tests/SixelEncoderTests.cstests/MuClient.Scripting.Tests/FakeScriptWorld.cstests/MuClient.Scripting.Tests/MuClient.Scripting.Tests.csprojtests/MuClient.Scripting.Tests/ReloadTests.cstests/MuClient.Scripting.Tests/SandboxTests.cstests/MuClient.Scripting.Tests/ScriptHostTests.cs
Comment on lines
+29
to
+32
| [JsonIgnore] | ||
| public Regex Regex => _compiled ??= new Regex( | ||
| Pattern, | ||
| RegexOptions.Compiled | (CaseSensitive ? RegexOptions.None : RegexOptions.IgnoreCase)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching Alias.cs:\n'
fd -a 'Alias\.cs$' . || true
printf '\nGit status/stat:\n'
git diff --stat || true
alias_file="$(fd 'Alias\.cs$' . | head -n 1 || true)"
if [ -n "${alias_file:-}" ]; then
printf '\nFile outline:\n'
ast-grep outline "$alias_file" || true
printf '\nRelevant source:\n'
cat -n "$alias_file"
fi
printf '\nSearch for Pattern/Regex usage in project:\n'
rg -n "class Alias|Pattern|Regex|RegexMatchTimeoutException|new Regex|RegexTimeout" -S --glob '*.cs' . || true
printf '\nSearch for expanded command handling / Alias Regex calls:\n'
rg -n "Regex\.(Match|IsMatch|Matches|Replace|Expand)|\.Regex|Alias" -S --glob '*.cs' . | head -n 200 || trueRepository: HarryCordewener/MuGlyph
Length of output: 16603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
alias_engine="$(fd 'AliasEngine\.cs$' . | head -n 1 || true)"
world_session="$(fd 'WorldSession\.cs$' . | head -n 1 || true)"
if [ -n "${alias_engine:-}" ]; then
printf 'AliasEngine.cs:\n'
cat -n "$alias_engine"
fi
if [ -n "${world_session:-}" ]; then
printf '\nWorldSession.cs around Expand/Command handling:\n'
sed -n '130,185p' "$world_session" | cat -n
fi
printf '\nCheck whether RegexMatchTimeoutException is caught anywhere:\n'
rg -n "RegexMatchTimeoutException|catch\s*\(" --glob '*.cs' . || true
printf '\nSearch for command send handling around alias expansion call:\n'
rg -n -C 4 "Aliases\.Expand|commands\.Select|Send|SendCommands|input" src tests --glob '*.cs' || trueRepository: HarryCordewener/MuGlyph
Length of output: 47446
Bound alias-regex execution time.
Pattern is persisted/imported configuration, and AliasEngine.Expand() uses alias.Regex.Match(input) before sending user input. Give Regex a timeout, and handle RegexMatchTimeoutException in AliasEngine so a pathological alias does not hang user input handling.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 29-31: A Regex is constructed from a non-literal (variable) pattern without a matchTimeout. The .NET regex engine backtracks, so an attacker-controlled pattern can cause catastrophic backtracking (ReDoS) and hang the thread. Pass a TimeSpan matchTimeout (e.g. new Regex(pattern, RegexOptions.None, TimeSpan.FromSeconds(1))), set AppDomain RegexMatchTimeout, or avoid compiling untrusted patterns at all.
Context: new Regex(
Pattern,
RegexOptions.Compiled | (CaseSensitive ? RegexOptions.None : RegexOptions.IgnoreCase))
Note: [CWE-1333] Inefficient Regular Expression Complexity.
(redos-regex-untrusted-pattern-no-timeout-csharp)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MuClient.Core/Automation/Alias.cs` around lines 29 - 32, Update the
Alias.Regex property to construct the compiled Regex with a finite execution
timeout for persisted patterns. In AliasEngine.Expand, catch
RegexMatchTimeoutException around alias.Regex.Match(input) and handle the
timeout by skipping or safely rejecting that alias without hanging user input
processing.
Source: Linters/SAST tools
Comment on lines
+155
to
+172
| // Carry the 24-bit image id in the foreground colour, per the Kitty spec. | ||
| var idColor = TerminalColor.FromRgb( | ||
| (byte)((imageId >> 16) & 0xFF), | ||
| (byte)((imageId >> 8) & 0xFF), | ||
| (byte)(imageId & 0xFF)); | ||
| var style = TextStyle.Default.WithForeground(idColor); | ||
|
|
||
| var lines = new StyledLine[rows]; | ||
| for (var row = 0; row < rows; row++) | ||
| { | ||
| var spans = new StyledSpan[cols]; | ||
| for (var col = 0; col < cols; col++) | ||
| { | ||
| var cell = new StringBuilder(4); | ||
| cell.Append(char.ConvertFromUtf32(PlaceholderCodePoint)); | ||
| cell.Append(char.ConvertFromUtf32(RowColumnDiacritics[row])); | ||
| cell.Append(char.ConvertFromUtf32(RowColumnDiacritics[col])); | ||
| spans[col] = new StyledSpan(cell.ToString(), style); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C3 'BuildPlaceholder|imageId >> 16|RowColumnDiacritics' \
src/MuClient.Graphics/KittyGraphicsProtocol.cs \
tests/MuClient.Graphics.Tests/KittyGraphicsProtocolTests.csRepository: HarryCordewener/MuGlyph
Length of output: 8077
🌐 Web query:
Kitty graphics protocol placeholder image ID diacritics high byte foreground color rowcolumn-diacritics
💡 Result:
The Kitty graphics protocol uses a Unicode placeholder mechanism (enabled by U=1 in the virtual placement command) to allow images to be displayed in environments that support text but not arbitrary graphics, such as multiplexers (tmux) or text editors (vim, Emacs) [1][2]. Placeholder Character The terminal uses the Unicode character U+10EEEE as a placeholder for an image [1][3]. This character is in the Private Use Area (PUA) and is designed to be easily distinguished from normal text [2]. Encoding Image IDs The image ID is encoded in the foreground color of the placeholder character [1]. By default, this uses the terminal's foreground color attribute [2]. - 8-bit IDs can be represented using 256-color mode [1]. - 24-bit IDs can be represented using true-color mode [1]. - For image IDs exceeding 24 bits, the most significant byte can be specified using a third diacritic [1]. Row and Column Encoding The specific row and column position of the image within the placeholder grid is encoded using combining diacritics [1]. These diacritics are appended to the placeholder character [1][3]. The terminal refers to a specific set of diacritics, documented in the protocol as rowcolumn-diacritics (often provided in a file named rowcolumn-diacritics.txt) [1][2][4]. Example Usage To print a placeholder, you emit the placeholder character followed by its diacritics: 1. Foreground color (encoding the image ID) [5][3]. 2. The placeholder character (U+10EEEE) [1][3]. 3. The row diacritic [1][5]. 4. The column diacritic [1][5]. 5. Optionally, a third diacritic for the high byte of the image ID if needed [1][4]. This approach allows host applications to manage image placement while the terminal handles the actual rendering based on the decoded ID and position [1][6].
Citations:
- 1: https://sw.kovidgoyal.net/kitty/graphics-protocol/
- 2: Alternative approach to image placement that works with tmux (proof of concept) kovidgoyal/kitty#4021
- 3: https://github.com/folke/zaly/blob/main/packages/tui/src/image/kitty.ts
- 4: https://github.com/blacktop/go-termimg/blob/main/kitty.go
- 5: https://github.com/can1357/oh-my-pi/blob/d7383294/packages/tui/src/kitty-graphics.ts
- 6: https://deepwiki.com/kovidgoyal/kitty/4.2-graphics-protocol
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '1,220p' src/MuClient.Graphics/KittyGraphicsProtocol.csRepository: HarryCordewener/MuGlyph
Length of output: 7530
Reject or fully encode image IDs above 24 bits.
BuildPlaceholder encodes only bits 0–23 in the RGBA foreground, so imageId = 0x01010001 emits placeholders for image id 1. Kitty supports encoding the high byte with a third diacritic, so either limit the public API to 0x00FFFFFF or carry the high byte in the cell diacritics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MuClient.Graphics/KittyGraphicsProtocol.cs` around lines 155 - 172,
Update BuildPlaceholder to prevent image IDs above 24 bits from being silently
truncated: either validate and reject values greater than 0x00FFFFFF at the
public API boundary, or encode the high byte using Kitty’s supported third cell
diacritic. Preserve correct round-trip identity for every accepted image ID.
Comment on lines
+88
to
+93
| try | ||
| { | ||
| await session.ConnectAsync().ConfigureAwait(false); | ||
| var size = _output.Viewport; | ||
| await session.SetWindowSizeAsync(Math.Max(1, size.Width), Math.Max(1, size.Height)).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ConfigureAwait(false) moves post-connect UI work off the UI thread.
After ConnectAsync().ConfigureAwait(false), the continuation (reading _output.Viewport, then calling SetWindowSizeAsync) resumes on a thread-pool thread instead of the UI thread. Everywhere else in this file (Lines 106-118), session-driven UI touches are explicitly marshaled back via Application.Invoke; this call site reads View state off-thread instead.
🔧 Proposed fix
- await session.ConnectAsync().ConfigureAwait(false);
- var size = _output.Viewport;
- await session.SetWindowSizeAsync(Math.Max(1, size.Width), Math.Max(1, size.Height)).ConfigureAwait(false);
+ await session.ConnectAsync();
+ var size = _output.Viewport;
+ await session.SetWindowSizeAsync(Math.Max(1, size.Width), Math.Max(1, size.Height));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try | |
| { | |
| await session.ConnectAsync().ConfigureAwait(false); | |
| var size = _output.Viewport; | |
| await session.SetWindowSizeAsync(Math.Max(1, size.Width), Math.Max(1, size.Height)).ConfigureAwait(false); | |
| } | |
| try | |
| { | |
| await session.ConnectAsync(); | |
| var size = _output.Viewport; | |
| await session.SetWindowSizeAsync(Math.Max(1, size.Width), Math.Max(1, size.Height)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MuClient.Tui/MuGlyphApp.cs` around lines 88 - 93, Update the connection
flow around session.ConnectAsync in MuGlyphApp so the continuation that reads
_output.Viewport and calls SetWindowSizeAsync executes on the UI thread. Remove
the ConfigureAwait(false) from this UI-dependent path or explicitly marshal the
post-connect work through the same Application.Invoke mechanism used by the
session-driven UI updates elsewhere in the file.
Comment on lines
+121
to
+130
| private void OnCommandEntered(string command) | ||
| { | ||
| var session = _active; | ||
| if (session is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _ = session.SendUserInputAsync(command); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fire-and-forget SendUserInputAsync swallows failures silently.
If sending fails (e.g. transport error), the exception is unobserved and the user gets no feedback that their command didn't go out.
🔧 Proposed fix
- _ = session.SendUserInputAsync(command);
+ _ = SendCommandAsync(session, command);
}
+
+ private async Task SendCommandAsync(WorldSession session, string command)
+ {
+ try
+ {
+ await session.SendUserInputAsync(command);
+ }
+ catch (Exception ex)
+ {
+ session.PrintSystem($"*** Send failed: {ex.Message}");
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void OnCommandEntered(string command) | |
| { | |
| var session = _active; | |
| if (session is null) | |
| { | |
| return; | |
| } | |
| _ = session.SendUserInputAsync(command); | |
| } | |
| private void OnCommandEntered(string command) | |
| { | |
| var session = _active; | |
| if (session is null) | |
| { | |
| return; | |
| } | |
| _ = SendCommandAsync(session, command); | |
| } | |
| private async Task SendCommandAsync(WorldSession session, string command) | |
| { | |
| try | |
| { | |
| await session.SendUserInputAsync(command); | |
| } | |
| catch (Exception ex) | |
| { | |
| session.PrintSystem($"*** Send failed: {ex.Message}"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/MuClient.Tui/MuGlyphApp.cs` around lines 121 - 130, Update
OnCommandEntered so failures from the fire-and-forget session.SendUserInputAsync
call are observed and surfaced to the user through the existing TUI feedback or
error-reporting mechanism. Preserve the current null-session early return and
command dispatch behavior while ensuring transport exceptions do not remain
unhandled.
CI failed on the timing-sensitive scheduler tests (fixed Task.Delay waits were too tight on the runner). Replace them with signal-based waits and fold in the safe, high-value CodeRabbit review findings. Tests / CI - IntervalScheduler tests now await a TaskCompletionSource signalled by the callback with a generous bounded timeout, instead of racing a fixed delay. - CI checkout uses persist-credentials: false. Correctness - IntervalScheduler: guard timer callbacks so a throwing callback can neither crash the ThreadPool thread/process nor stop a recurring schedule. - Trigger/Alias: compile user regexes with a 250ms match timeout and skip a pattern that times out (ReDoS guard) rather than blocking output/input. - TcpTransport: serialize SendAsync with a semaphore so concurrent negotiation, command, and trigger-response writes cannot interleave and corrupt framing. - AnsiParser: consume DCS/SOS/PM/APC (ESC P/X/^/_) string sequences through ST instead of leaking their payloads as text (APC covers Kitty graphics). - HtmlLogSink: make Dispose/Flush idempotent (no writes after close). - Tui: Ctrl+Q now compares against KeyCode.Q (uppercase) so quit actually fires. Docs - Correct the project count (seven) in CLAUDE.md. 198 tests pass (Core 102, Graphics 54, Scripting 42). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015w2WJ5M22CZmg9md67zpeB
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
2 participants
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.
Build out the MuGlyph client from the skeleton into a working, UI-agnostic
core with a Terminal.Gui v2 front end. All of MuClient.Core is unit-tested
(195 tests across Core, Graphics, and Scripting).
Solution
shared build props, and a GitHub Actions CI matrix (ubuntu + windows).
Core (UI-agnostic, fully testable)
CSI/OSC recognition-and-discard; styled-span line model; ScrollbackBuffer.
wrapping TelnetNegotiationCore 2.5.3 with GMCP/MSSP/MSDP routing and
GA/EOR prompt detection.
AliasEngine, MacroEngine, IntervalScheduler.
Graphics (no UI dependency)
fallbacks, and an environment-based capability probe.
Scripting
gmcp/log with hot-reload.
Tui
CommandInput with history + tab-completion, status line, key routing,
theming.
Tests use TUnit on Microsoft.Testing.Platform; run via
dotnet run --project.Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015w2WJ5M22CZmg9md67zpeB
Summary by CodeRabbit